fix: proper URL escaping and wrapping (`url()`) #627

kgram · 2017-10-31T19:04:45Z

What kind of change does this PR introduce?
Bugfix

Did you add tests for your changes?
Yes. 2 existing tests changed to account for new behaviour (no single-quotes), 6 new tests of escaping.

If relevant, did you update the README?
I don't believe it's relevant. Unless just to document the quoting behaviour.

Summary
Fixes #615. Previously, modules had their quotes stripped unless the module-path contained a space. Now, quotes are always stripped and the URL returned from the module is escaped and wrapped in double-quotes if necessary. Modules returning a quote-wrapped string will have their own wrapping-quotes stripped before wrapping/escaping.

Does this PR introduce a breaking change?
I don't think it does, modules wrapping in quotes like svg-url-loader should still function.

The only consequence is that it is impossible to wrap in single-quotes, although it's hard to see when this would be a necessity.

Other information
There is one weird case I'm not sure how to handle: newlines should be escaped in the quoted url. The spec states that only line feeds (\n) count as newlines, since carriage return (\r) and form feed (\f) are automatically converted during preprocessing. However, if left unescaped they would be converted to unescaped line feeds. If escaped, they wouldn't be converted. I'm not sure if the best step would be to convert them to escaped line feeds manually or if it even matters. Thoughts?

jsf-clabot · 2017-10-31T19:04:46Z

All committers have signed the CLA.

michael-ciniawsky

Before I review this further does JSON.stringify(url) eventually do the trick here ?

michael-ciniawsky · 2017-10-31T19:43:05Z

lib/escape-url.js

@@ -0,0 +1,13 @@
+module.exports = function(url) {
+    // If url is already wrapped in quotes, remove them
+    if ((url[0] === '"' || url[0] === '\'') && url[0] === url[url.length - 1]) {


if (/^['"].*['"]$/.test(url)) { url = url.slice(1, -1); }

michael-ciniawsky · 2017-10-31T19:55:03Z

const url1 = 'path/to/image.png'
const url2 = '"path/to/image.png"'

console.log(JSON.stringify(url1))
console.log(JSON.stringify(url2))

"path/to/image.png"
"\"path/to/image.png\""

codecov · 2017-10-31T20:52:31Z

Codecov Report

Merging #627 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #627      +/-   ##
==========================================
+ Coverage   98.92%   98.94%   +0.02%     
==========================================
  Files          10       11       +1     
  Lines         371      379       +8     
  Branches       87       88       +1     
==========================================
+ Hits          367      375       +8     
  Misses          4        4

Impacted Files	Coverage Δ
lib/processCss.js	`98.66% <100%> (-0.01%)`	⬇️
lib/loader.js	`100% <100%> (ø)`	⬆️
lib/escape-url.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05c36db...fdac0b0. Read the comment docs.

codecov · 2017-10-31T20:52:31Z

Codecov Report

Merging #627 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #627      +/-   ##
==========================================
+ Coverage   98.92%   98.94%   +0.02%     
==========================================
  Files          10       11       +1     
  Lines         371      379       +8     
  Branches       87       88       +1     
==========================================
+ Hits          367      375       +8     
  Misses          4        4

Impacted Files	Coverage Δ
lib/escape-url.js	`100% <100%> (ø)`
lib/loader.js	`100% <100%> (ø)`	⬆️
lib/processCss.js	`98.66% <100%> (-0.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05c36db...f144280. Read the comment docs.

michael-ciniawsky · 2017-10-31T21:33:21Z

Could you check if

return "JSON.stringify(require(" + loaderUtils.stringifyRequest(this, urlRequest) + "))";

also works ? Not 💯 if I placed it correctly :)

kgram · 2017-10-31T21:54:26Z

I can't find a reason JSON.stringify shouldn't work, there doesn't seem to be any conflicts between the formats. It feels a little wrong, though, which I guess is why I spent so long looking for a problem. But I guess if it works that ought to be enough.

Two things to note: if you replace the entire escapeUrl function with JSON.stringify all URLs are wrapped in quotes no matter their content and quote-wrapped strings (such as from svg-url-loader) will keep their quotes. I think it would have to be released as a major version to avoid breaking changes then. Maybe an inline .replace(/^['"](.*)['"]$/, '$1') could be used to avoid it.

Your call :)

michael-ciniawsky · 2017-10-31T22:09:51Z

lib/escape-url.js

+    }
+    // Should url be wrapped?
+    // See https://drafts.csswg.org/css-values-3/#urls
+    if (/["'() \t\n]/.test(url)) {


What exactly is tested here, could you give a few examples please ?

["'()=> <=\t\n] => \s ?

/["'() \t\n]/.test(url) => /["'() \t\n]/g.test(url) (/RegExp/g) ?

They are the characters outlined as requiring escaping in unquoted URLs in the CSS spec.

https://drafts.csswg.org/css-values-3/#urls

If any of them are present, the string is wrapped in quotes and quotes/newline is escaped. Like with JSON.stringify, yes, it's basically the same as \s in regex, just from different standards. There seems to be some rarely used characters regex considers whitespace that CSS doesn't, but again, it is unlikely to matter. I just prefer to be specific when the standard is right there.

Some examples that would require either escaping or wrapping:

url(open(parens) => url(open$parens) / url("open(parens")

url(close)parens) => url(close$parens) / url("close)parens")

url(quote"in"middle) => url("quote\"in\"middle")

Regarding your second point: Why would it be better to test with a global regex? I'm not aware of any difference caused by this.

michael-ciniawsky · 2017-10-31T22:10:38Z

lib/loader.js

@@ -82,8 +82,13 @@ module.exports = function(content, map) {
 				"[" + JSON.stringify(importItem.export) + "] + \"";
 		}

+		var escapeUrlHelper;


// url() helper to escape quotes var escape = ""

\n after the variable decl please

The variable is now named urlEscapeHelper in the loader and escape in the output. If you feel that is incorrect could you be a bit more specific about the naming you would prefer?

michael-ciniawsky · 2017-10-31T22:15:57Z

lib/loader.js

 		cssAsString = cssAsString.replace(result.importItemRegExpG, importItemMatcher.bind(this));
-		if(query.url !== false) {
+		if(query.url !== false && result.urlItems.length > 0) {
+			escapeUrlHelper = "var escapeUrl = require(" +


escape = "require(" + loaderUtils.stringifyRequest(this, require.resolve("./url/escape.js")) + ")\n"

I'm a little confused here. Surely you mean escape = "var escape = require(" + ..., right?

Don't you think it gets a little confusing when you call both the variable in the loader and in the string by the same name?

michael-ciniawsky · 2017-10-31T22:18:05Z

lib/loader.js

 			}.bind(this));
+		} else {


rm the else as initializing escape with a default value ("") should be enough

michael-ciniawsky · 2017-10-31T22:20:12Z

test/urlTest.js

+	], "", { './module': 'module(with-parens)' });
+	test("module with newline", ".class { background: green url(module) xyz }", [
+		[1, ".class { background: green url(\"module\\nwith\\nnewline\") xyz }", ""]
+	], "", { './module': "module\nwith\nnewline" });


We need to test url-loader output here aswell

Test added of one of the url-loader outputs from this article. It's a very small file, but as far as I know all base64-encoded data-URIs are valid in CSS when unquoted. If you have an example in mind of something that might fail I'll add that too though.

michael-ciniawsky · 2017-10-31T22:22:14Z

lib/escape-url.js

@@ -0,0 +1,13 @@
+module.exports = function(url) {


lib/escape-url-js => lib/url/escape.js

module.exports = function escape (url) {

So we hopefully can require the named {Function} without having to decl a new variable later :)

michael-ciniawsky · 2017-10-31T22:26:07Z

What about data URL's, e.g from the url-loader ?

joshwiens

You should be able to grab the expected out of a few test cases in url-loader

kgram · 2017-11-01T19:50:03Z

I think everything is either fixed or addressed now :)

alexander-akait · 2017-11-02T09:21:04Z

Just information, webpack-contrib/sass-loader#490 maybe related

kgram · 2017-11-09T13:45:08Z

@michael-ciniawsky, @evilebottnawi: Have you seen my comments? I think I've either fixed or addressed everything now, could you have a look at it again?

alexander-akait · 2017-11-09T13:50:04Z

test/moduleTestCases/urls/expected.css

@@ -2,7 +2,7 @@
 	background: url({./module});
 	background: url({./module});
 	background: url("{./module module}");
-	background: url('{./module module}');
+	background: url("{./module module}");


Why we change ' to "? Can we save original quotes?

In principle it could be done. It will complicate things quite a bit, though, since the whole point of the update is to quote based on module-content instead of name. The type of quotes would have to be passed to the client (something like escapeSingle(require(<resource>))).

I skipped it, since I can't really imagine a scenario where it would be important to use single quotes. svg-url-loader already assumes double-quotes, since this is what is commonly used in HTML, I assume other loaders will make similar choices. If not, the worst case scenario is a slightly longer string.

If this is implemented, there should maybe also be an escapeUnquoted function for handling unquoted modules. That way the quotes used will be entirely up to the user, the loader will just ensure valid CSS is produced.

kgram added 3 commits October 31, 2017 19:25

Escape invalid urls

d14e8b8

Correct existing tests to expect double-quotes

2db8469

Add url-escape specific tests

74431ba

michael-ciniawsky requested changes Oct 31, 2017

View reviewed changes

michael-ciniawsky changed the title ~~URL escaping and quote-wrapping~~ fix: proper URL escaping and wrapping Oct 31, 2017

michael-ciniawsky added this to the 0.28.8 milestone Oct 31, 2017

michael-ciniawsky added pr_action: Review semver: Patch status: Pending type: Bug labels Oct 31, 2017

michael-ciniawsky requested review from alexander-akait and joshwiens October 31, 2017 19:49

Switch to regex to detect quote-wrapped urls

fdac0b0

michael-ciniawsky requested changes Oct 31, 2017

View reviewed changes

joshwiens reviewed Nov 1, 2017

View reviewed changes

kgram added 4 commits November 1, 2017 19:54

Rename and rearrange url-escape variables

1ac7661

Rename lib/escape-url.js -> lib/url/escape.js

87caf33

Add semicolon to url escape-helper output

d34841f

Add test for url-loader output

f144280

alexander-akait reviewed Nov 9, 2017

View reviewed changes

ubnt-marc-khouri mentioned this pull request Aug 10, 2018

Invalid markup generated when combined with html-loader bhovhannes/svg-url-loader#126

Closed

snyk-bot mentioned this pull request Aug 16, 2019

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 DriveClutch/sellpy-dockerfiles#4

Closed

snyk-bot mentioned this pull request Jan 8, 2020

[Snyk] Upgrade css-loader from 0.17.0 to 0.28.11 timscharfenort/kibana#3

Open

This was referenced Jan 22, 2020

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 anyulled/Learn-Redux-Starter-Files#1

Open

[Snyk] Upgrade css-loader from 0.24.0 to 0.28.11 anyulled/react-skeleton#2

Closed

This was referenced Feb 23, 2020

[Snyk] Upgrade css-loader from 0.26.4 to 0.28.11 saurabharch/stitch-examples#2

Open

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 saurabharch/guesstimate-app#3

Open

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 saurabharch/CodeGenius#2

Open

snyk-bot mentioned this pull request Mar 7, 2020

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 yowainwright/react-workshop#4

Closed

This was referenced Apr 2, 2020

[Snyk] Upgrade css-loader from 0.26.4 to 0.28.11 fiverr/rollout_dashboard#48

Open

[Snyk] Upgrade css-loader from 0.19.0 to 0.28.11 rangle/react-day-picker#2

Open

snyk-bot mentioned this pull request Apr 24, 2020

[Snyk] Upgrade css-loader from 0.16.0 to 0.28.11 walmartlabs/bolt-standard-flux#3

Open

This was referenced May 4, 2020

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 Barnstorm-Online/votetracker#1

Open

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 React-Junkie/react-coffee-and-friends#2

Open

KrisPeachyKen mentioned this pull request Jul 30, 2024

[Snyk] Upgrade css-loader from 0.18.0 to 0.28.11 KrisPeachyKen/ai#2

Open

superadmiral mentioned this pull request Jul 30, 2024

[Snyk] Upgrade css-loader from 0.18.0 to 0.28.11 superadmiral/ai#2

Open

YinghanFeng mentioned this pull request Aug 2, 2024

[Snyk] Upgrade css-loader from 0.9.1 to 0.28.11 YinghanFeng/deco-ide#1

Open

go-benchmark1 mentioned this pull request Aug 16, 2024

[Snyk] Upgrade css-loader from 0.23.1 to 0.28.11 go-benchmark1/pome#1

Open

Vishesh7797 mentioned this pull request Oct 26, 2024

[Snyk] Upgrade css-loader from 0.26.4 to 0.28.11 Vishesh7797/Sorting-Visualizer#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: proper URL escaping and wrapping (`url()`) #627

fix: proper URL escaping and wrapping (`url()`) #627

kgram commented Oct 31, 2017

jsf-clabot commented Oct 31, 2017 •

edited

Loading

michael-ciniawsky left a comment •

edited

Loading

michael-ciniawsky Oct 31, 2017

kgram Nov 1, 2017

michael-ciniawsky commented Oct 31, 2017

codecov bot commented Oct 31, 2017

codecov bot commented Oct 31, 2017 •

edited

Loading

michael-ciniawsky commented Oct 31, 2017 •

edited

Loading

kgram commented Oct 31, 2017

michael-ciniawsky Oct 31, 2017

kgram Oct 31, 2017

kgram Nov 1, 2017

michael-ciniawsky Oct 31, 2017

kgram Nov 1, 2017

michael-ciniawsky Oct 31, 2017

kgram Oct 31, 2017

michael-ciniawsky Oct 31, 2017

kgram Nov 1, 2017

michael-ciniawsky Oct 31, 2017

kgram Nov 1, 2017

michael-ciniawsky Oct 31, 2017

kgram Nov 1, 2017

michael-ciniawsky commented Oct 31, 2017

joshwiens left a comment

kgram commented Nov 1, 2017

alexander-akait commented Nov 2, 2017

kgram commented Nov 9, 2017

alexander-akait Nov 9, 2017

kgram Nov 9, 2017

fix: proper URL escaping and wrapping (url()) #627

fix: proper URL escaping and wrapping (url()) #627

Conversation

kgram commented Oct 31, 2017

jsf-clabot commented Oct 31, 2017 • edited Loading

michael-ciniawsky left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky commented Oct 31, 2017

codecov bot commented Oct 31, 2017

Codecov Report

codecov bot commented Oct 31, 2017 • edited Loading

Codecov Report

michael-ciniawsky commented Oct 31, 2017 • edited Loading

kgram commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky commented Oct 31, 2017

joshwiens left a comment

Choose a reason for hiding this comment

kgram commented Nov 1, 2017

alexander-akait commented Nov 2, 2017

kgram commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix: proper URL escaping and wrapping (`url()`) #627

fix: proper URL escaping and wrapping (`url()`) #627

jsf-clabot commented Oct 31, 2017 •

edited

Loading

michael-ciniawsky left a comment •

edited

Loading

codecov bot commented Oct 31, 2017 •

edited

Loading

michael-ciniawsky commented Oct 31, 2017 •

edited

Loading