Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Jan 4, 2018
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d14e8b8
Escape invalid urls
kgram Oct 31, 2017
2db8469
Correct existing tests to expect double-quotes
kgram Oct 31, 2017
74431ba
Add url-escape specific tests
kgram Oct 31, 2017
fdac0b0
Switch to regex to detect quote-wrapped urls
kgram Oct 31, 2017
1ac7661
Rename and rearrange url-escape variables
kgram Nov 1, 2017
87caf33
Rename lib/escape-url.js -> lib/url/escape.js
kgram Nov 1, 2017
d34841f
Add semicolon to url escape-helper output
kgram Nov 1, 2017
f144280
Add test for url-loader output
kgram Nov 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions lib/escape-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
module.exports = function(url) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// If url is already wrapped in quotes, remove them
if (/^['"].*['"]$/.test(url)) {
url = url.slice(1, -1);
}
// Should url be wrapped?
// See https://drafts.csswg.org/css-values-3/#urls
if (/["'() \t\n]/.test(url)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return '"' + url.replace(/"/g, '\\"').replace(/\n/g, '\\n') + '"'
}

return url
}
19 changes: 13 additions & 6 deletions lib/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,13 @@ module.exports = function(content, map) {
"[" + JSON.stringify(importItem.export) + "] + \"";
}

var escapeUrlHelper;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

\n after the variable decl please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

cssAsString = cssAsString.replace(result.urlItemRegExpG, function(item) {
var match = result.urlItemRegExp.exec(item);
var idx = +match[1];
Expand All @@ -95,15 +100,16 @@ module.exports = function(content, map) {
if(idx > 0) { // idx === 0 is catched by isUrlRequest
// in cases like url('webfont.eot?#iefix')
urlRequest = url.substr(0, idx);
return "\" + require(" + loaderUtils.stringifyRequest(this, urlRequest) + ") + \"" +
return "\" + escapeUrl(require(" + loaderUtils.stringifyRequest(this, urlRequest) + ")) + \"" +
url.substr(idx);
}
urlRequest = url;
return "\" + require(" + loaderUtils.stringifyRequest(this, urlRequest) + ") + \"";
return "\" + escapeUrl(require(" + loaderUtils.stringifyRequest(this, urlRequest) + ")) + \"";
}.bind(this));
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

escapeUrlHelper = ""
}



var exportJs = compileExports(result, importItemMatcher.bind(this), camelCaseKeys);
if (exportJs) {
exportJs = "exports.locals = " + exportJs + ";";
Expand All @@ -127,7 +133,8 @@ module.exports = function(content, map) {
}

// embed runtime
callback(null, "exports = module.exports = require(" +
callback(null, escapeUrlHelper +
"exports = module.exports = require(" +
loaderUtils.stringifyRequest(this, require.resolve("./css-base.js")) +
")(" + query.sourceMap + ");\n" +
"// imports\n" +
Expand Down
6 changes: 2 additions & 4 deletions lib/processCss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,8 @@ var parserPlugin = postcss.plugin("css-loader-parser", function(options) {
break;
case "url":
if (options.url && item.url.replace(/\s/g, '').length && !/^#/.test(item.url) && (isAlias(item.url) || loaderUtils.isUrlRequest(item.url, options.root))) {
// Don't remove quotes around url when contain space
if (item.url.indexOf(" ") === -1) {
item.stringType = "";
}
// Strip quotes, they will be re-added if the module needs them
item.stringType = "";
delete item.innerSpacingBefore;
delete item.innerSpacingAfter;
var url = item.url;
Expand Down
2 changes: 2 additions & 0 deletions test/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ function getEvaluated(output, modules) {
fn(m, m.exports, function(module) {
if(module.indexOf("css-base") >= 0)
return require("../lib/css-base");
if(module.indexOf("escape-url") >= 0)
return require("../lib/escape-url");
if(module.indexOf("-!/path/css-loader!") === 0)
module = module.substr(19);
if(modules && modules[module])
Expand Down
2 changes: 1 addition & 1 deletion test/moduleTestCases/urls/expected.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
background: url({./module});
background: url({./module});
background: url("{./module module}");
background: url('{./module module}');
background: url("{./module module}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume other loaders will make similar choices - If there is one thing I have learned about this ecosystem, that assumption is almost guaranteed to break something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but "break something" here means "result in a slightly longer string since double-quotes are escaped". The whole point of this change is to make it impossible for other loaders to break your CSS.

My arguments for using only double-quotes summed up:

  1. Double-quote wrapping is the expected default for URLs in HTML in all the view-engines I'm familiar with, so I think it is a fair assumption that most loaders will expect this and avoid double-quotes. If they don't, nothing breaks.
  2. A user shouldn't have to consider this detail. My own sass-lint rules force single-quotes since this is used across the project, which would work against svg-url-loader's change from double to single-quotes.
  3. I don't think the quotes written by the user ought to be considered equal to the quotes produced in the output. The user quotes the module-name, the loader quotes the actual URL, there's no reason to expect similar requirements.

Of course it is not my package, and I'm not that experienced with writing loaders, so I defer entirely to your judgements. If I have failed to convince you, I'll change it so there is a separate escape-function for single-quotes. If that is the case, should I also make an unquoted escape-function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no debating correctness I'm just saying that I can all but guarantee that at some point after this lands in master, it will have broken something, somewhere.

That has nothing to do with not merging it, just saying it's going to happen & we now we have context as to why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to keep in mind that css-loader sits at the core of the entire style chain and in some places, that's a long & convoluted chain for better or worse.

Anything that lands in this repo has to be vetted, pondered, picked apart & poked so we at the very least can have a reason why there are 4 new issues with version 0.0.x broke Y

Changes in behaviors get that & the rubber glove treatment, it's just too easy to not account for a path in this vast ecosystem where Change X has an unintended side effect on Y & people show up with torches & pitch forks.

background: url({./module});
background: url({./module}#?iefix);
background: url("#hash");
Expand Down
21 changes: 20 additions & 1 deletion test/urlTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe("url", function() {
[1, ".class { background: green url(\"{./img img.png}\") xyz }", ""]
]);
test("background 2 img contain space in name", ".class { background: green url( 'img img.png' ) xyz }", [
[1, ".class { background: green url('{./img img.png}') xyz }", ""]
[1, ".class { background: green url(\"{./img img.png}\") xyz }", ""]
]);
test("background img absolute", ".class { background: green url(/img.png) xyz }", [
[1, ".class { background: green url(/img.png) xyz }", ""]
Expand Down Expand Up @@ -103,6 +103,25 @@ describe("url", function() {
test("external schema-less url", ".class { background: green url(//raw.githubusercontent.com/webpack/media/master/logo/icon.png) xyz }", [
[1, ".class { background: green url(//raw.githubusercontent.com/webpack/media/master/logo/icon.png) xyz }", ""]
]);

test("module wrapped in spaces", ".class { background: green url(module) xyz }", [
[1, ".class { background: green url(module-wrapped) xyz }", ""]
], "", { './module': "\"module-wrapped\"" });
test("module with space", ".class { background: green url(module) xyz }", [
[1, ".class { background: green url(\"module with space\") xyz }", ""]
], "", { './module': "module with space" });
test("module with quote", ".class { background: green url(module) xyz }", [
[1, ".class { background: green url(\"module\\\"with\\\"quote\") xyz }", ""]
], "", { './module': "module\"with\"quote" });
test("module with quote wrapped", ".class { background: green url(module) xyz }", [
[1, ".class { background: green url(\"module\\\"with\\\"quote\\\"wrapped\") xyz }", ""]
], "", { './module': "\"module\"with\"quote\"wrapped\"" });
test("module with parens", ".class { background: green url(module) xyz }", [
[1, ".class { background: green url(\"module(with-parens)\") xyz }", ""]
], "", { './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" });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to test url-loader output here aswell

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


test("background img with url", ".class { background: green url( \"img.png\" ) xyz }", [
[1, ".class { background: green url( \"img.png\" ) xyz }", ""]
Expand Down