Fix `function-url-quotes` false positives for SCSS variable and `@` character #7416

mattxwang · 2023-12-18T22:20:50Z

Which issue, if any, is this issue related to?

Closes #7404 (though, see discussion below).

Is there anything in the PR that needs further explanation?

tl;dr: this PR (right now) contains a bandaid fix, but there may be a bigger problem of isStandardSyntaxUrl's heuristic regex not doing what it should be. Would love to get some opinions since I don't have all the context for the util!

long explanation / context for problem

First, to ~~briefly~~ explain why #7404 is happening: function-url-quotes uses isStandardSyntaxUrl to bail out for nonstandard urls within url():

stylelint/lib/utils/isStandardSyntaxUrl.mjs

Lines 41 to 46 in b34a184

    
           // In url without quotes scss variable can be everywhere 
        
           // But in this case it is allowed to use only specific characters 
        
           // Also forbidden "/" at the end of url 
        
           if (url.includes('$') && IS_SCSS_VARIABLE_IN_URL.test(url) && !url.endsWith('/')) { 
        
           	return false; 
        
           }

The regex IS_SCSS_VARIABLE_IN_URL is:

stylelint/lib/utils/isStandardSyntaxUrl.mjs

Line 7 in b34a184

const IS_SCSS_VARIABLE_IN_URL = /^[$\s\w+\-,./*'"]+$/;

Based off of the name and the comment in the condition, it seems like the purpose of this regex is to capture an SCSS variable within url. Makes sense! In fact, we have tests for SCSS variables + interpolation (which left me very confused as to why adding an @ sign caused an error):

stylelint/lib/utils/__tests__/isStandardSyntaxUrl.test.mjs

Lines 137 to 139 in b34a184

    
           it('sass variable concat with single quotes string', () => { 
        
           	expect(isStandardSyntaxUrl("$sass-variable + 'foo'")).toBeFalsy(); 
        
           });

However, I want to point out that the regex is anchored. It only matches if the entire string matches that regex. So, in $sass-variable + 'foo', IS_SCSS_VARIABLE_IN_URL is matching all of $sass-variable + 'foo', instead of just $sass-variable. So, we're not "really" dealing with concatenation; we're just pretending that there is one SCSS variable called $sass-variable + 'foo'. Here's a regex101 explaining this.

This then explains why #7404 occurred: because @ is not part of the character set, IS_SCSS_VARIABLE_IN_URL doesn't match the entire string $asset-prefix + 'assets/images/fa_calculator_blue@2x.png', even though it probably "should" match $asset-prefix. Here is an accompanying regex101.

I've introduced some failing tests in 48485ef to showcase this.

solutions?

The minimum solution to fix this problem is to just add @ to the regex. I did this in 90422c2, which resolves the failing tests in 48485ef. In terms of the above explanation, this now just lets us treat $asset-prefix + 'assets/images/fa_calculator_blue@2x.png' as "one-giant SCSS variable".

However, I'm not super satisfied with this, since (in my eyes) it punts the problem down the line. For example, the similar declaration still has an error (see accompanying regex101):

'https://' + $asset-prefix + 'assets/images/fa_calculator_blue@2x.png'

Unanchoring itself doesn't work, since it will then incorrectly flag strings with $ within them, like this current test case:

http://domain.com:8080/path/to$to/file$2x.ext?$1=$2

So, a "robust"/full solution probably needs to actually parse the value. I'd be happy to spend some time exploring this if we think it's worthwhile, but we could also stick with the current heuristic and add @ to resolve the immediate issue. I'm also cognizant that I may be missing context (e.g. I didn't write the original util). What are our thoughts?

changeset-bot · 2023-12-18T22:20:54Z

🦋 Changeset detected

Latest commit: 57c373d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
stylelint	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

mattxwang · 2023-12-18T22:21:05Z

lib/utils/__tests__/isStandardSyntaxUrl.test.mjs

+	it('url with at sign at the start', () => {
 		expect(isStandardSyntaxUrl('@url/path')).toBeTruthy();
 	});
-	it('url with ampersand in the middle', () => {
+	it('url with at sign in the middle', () => {
 		expect(isStandardSyntaxUrl('some/@url/path')).toBeTruthy();
 	});
-	it('url with ampersand at the end', () => {
+	it('url with at sign at the end', () => {


Separately fixing typos I noticed!

mattxwang · 2023-12-18T22:21:24Z

lib/utils/__tests__/isStandardSyntaxUrl.test.mjs

-	it('sass variable concat with escaped single quotes string', () => {
-		expect(isStandardSyntaxUrl("$sass-variable + 'foo'")).toBeFalsy();
-	});


This is a duplicate test case that doesn't actually escape the quotes!

mattxwang · 2023-12-18T22:21:39Z

lib/utils/__tests__/isStandardSyntaxUrl.test.mjs

 	it('sass variable concat with double quotes string', () => {
-		expect(isStandardSyntaxUrl("$sass-variable + 'foo'")).toBeFalsy();
+		expect(isStandardSyntaxUrl('$sass-variable + "foo"')).toBeFalsy();


This test case didn't do what it said earlier (?)

ybiquitous · 2023-12-20T14:49:43Z

@mattxwang Thanks for the pull request!

First, don't hesitate to make a PR ready for review if you finish implementation. If a PR is still draft, we may ignore it ("Oh, it's not ready"). 😃

Second, I don't think it's necessary to parse SCSS code in isStandardSyntaxUrl(). It's just a heuristic and not expected to be more accurate than needed. If we implemented a parser for SCSS, the code would be much more complex, and the package size would increase. I believe it's a plugin's task like stylelint-scss.

To resolve the example below that you mentioned, adding : to the regex seems enough. Such a case is rare, so I don't believe it's worth the increased code.

'https://' + $asset-prefix + 'assets/images/fa_calculator_blue@2x.png'

In conclusion, I agree with your current solution adding @ to the regex. 👍🏼

mattxwang · 2023-12-22T08:38:54Z

First, don't hesitate to make a PR ready for review if you finish implementation. If a PR is still draft, we may ignore it ("Oh, it's not ready"). 😃

Ah good point, my mistake - I'll make sure not to do that in the future!

Second, I don't think it's necessary to parse SCSS code in isStandardSyntaxUrl(). It's just a heuristic and not expected to be more accurate than needed. If we implemented a parser for SCSS, the code would be much more complex, and the package size would increase. I believe it's a plugin's task like stylelint-scss.

To resolve the example below that you mentioned, adding : to the regex seems enough. Such a case is rare, so I don't believe it's worth the increased code.
'https://' + $asset-prefix + 'assets/images/fa_calculator_blue@2x.png'

Gotcha, I agree with the overall rationale. Unfortunately, this actually (by itself) doesn't resolve the problem, since this will still then (incorrectly) trip this condition first:

stylelint/lib/utils/isStandardSyntaxUrl.mjs

Lines 26 to 32 in b34a184

    
           if ((url.startsWith(`'`) && url.endsWith(`'`)) || (url.startsWith(`"`) && url.endsWith(`"`))) { 
        
           	if (hasLessInterpolation(url)) { 
        
           		return false; 
        
           	} 
        
           	return true; 
        
           }

So, we'd need to shuffle around the order of checks - though that introduces more of a headache 😓

In conclusion, I agree with your current solution adding @ to the regex. 👍🏼

Maybe I'll leave this PR as-is for now, and address the :-related use-case if a user submits a bug?

(with that in mind, have marked this PR as ready to review)

ybiquitous · 2023-12-24T04:00:52Z

Maybe I'll leave this PR as-is for now, and address the :-related use-case if a user submits a bug?

Ya, sounds good for now 👍🏼

ybiquitous

Thank you. LGTM 👍🏼

mattxwang added 2 commits December 18, 2023 12:15

Failing tests (for isStandardSynatxUtil and function-url-quotes)

48485ef

Bandaid fix (should not merge?)

90422c2

mattxwang commented Dec 18, 2023

View reviewed changes

mattxwang mentioned this pull request Dec 18, 2023

Fix function-url-quotes false positives for SCSS variable and @ character #7404

Closed

npm run build + npx changeset

0912fe4

ybiquitous mentioned this pull request Dec 20, 2023

Release 16.1.0 #7421

Closed

9 tasks

mattxwang marked this pull request as ready for review December 22, 2023 08:39

Merge branch 'main' into fix-function-url-quotes-fp-scss-and-at

57c373d

ybiquitous reviewed Dec 24, 2023

View reviewed changes

ybiquitous approved these changes Dec 24, 2023

View reviewed changes

ybiquitous merged commit cb509a0 into main Dec 24, 2023
16 checks passed

ybiquitous deleted the fix-function-url-quotes-fp-scss-and-at branch December 24, 2023 04:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `function-url-quotes` false positives for SCSS variable and `@` character #7416

Fix `function-url-quotes` false positives for SCSS variable and `@` character #7416

mattxwang commented Dec 18, 2023

changeset-bot bot commented Dec 18, 2023 •

edited

mattxwang Dec 18, 2023

mattxwang Dec 18, 2023

mattxwang Dec 18, 2023

ybiquitous commented Dec 20, 2023

mattxwang commented Dec 22, 2023

ybiquitous commented Dec 24, 2023

ybiquitous left a comment

	// In url without quotes scss variable can be everywhere
	// But in this case it is allowed to use only specific characters
	// Also forbidden "/" at the end of url
	if (url.includes('$') && IS_SCSS_VARIABLE_IN_URL.test(url) && !url.endsWith('/')) {
	return false;
	}

	it('sass variable concat with single quotes string', () => {
	expect(isStandardSyntaxUrl("$sass-variable + 'foo'")).toBeFalsy();
	});

Fix function-url-quotes false positives for SCSS variable and @ character #7416

Fix function-url-quotes false positives for SCSS variable and @ character #7416

Conversation

mattxwang commented Dec 18, 2023

long explanation / context for problem

solutions?

changeset-bot bot commented Dec 18, 2023 • edited

🦋 Changeset detected

mattxwang Dec 18, 2023

Choose a reason for hiding this comment

mattxwang Dec 18, 2023

Choose a reason for hiding this comment

mattxwang Dec 18, 2023

Choose a reason for hiding this comment

ybiquitous commented Dec 20, 2023

mattxwang commented Dec 22, 2023

ybiquitous commented Dec 24, 2023

ybiquitous left a comment

Choose a reason for hiding this comment

Fix `function-url-quotes` false positives for SCSS variable and `@` character #7416

Fix `function-url-quotes` false positives for SCSS variable and `@` character #7416

changeset-bot bot commented Dec 18, 2023 •

edited