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 all 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
18 changes: 12 additions & 6 deletions lib/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,13 @@ module.exports = function(content, map) {
}

cssAsString = cssAsString.replace(result.importItemRegExpG, importItemMatcher.bind(this));
if(query.url !== false) {

// helper for ensuring valid CSS strings from requires
var urlEscapeHelper = "";

if(query.url !== false && result.urlItems.length > 0) {
urlEscapeHelper = "var escape = require(" + loaderUtils.stringifyRequest(this, require.resolve("./url/escape.js")) + ");\n";

cssAsString = cssAsString.replace(result.urlItemRegExpG, function(item) {
var match = result.urlItemRegExp.exec(item);
var idx = +match[1];
Expand All @@ -95,15 +101,14 @@ 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 "\" + escape(require(" + loaderUtils.stringifyRequest(this, urlRequest) + ")) + \"" +
url.substr(idx);
}
urlRequest = url;
return "\" + require(" + loaderUtils.stringifyRequest(this, urlRequest) + ") + \"";
return "\" + escape(require(" + loaderUtils.stringifyRequest(this, urlRequest) + ")) + \"";
}.bind(this));
}



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

// embed runtime
callback(null, "exports = module.exports = require(" +
callback(null, urlEscapeHelper +
"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
13 changes: 13 additions & 0 deletions lib/url/escape.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
module.exports = function escape(url) {
// 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)) {
return '"' + url.replace(/"/g, '\\"').replace(/\n/g, '\\n') + '"'
}

return url
}
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("url/escape") >= 0)
return require("../lib/url/escape");
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
24 changes: 23 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,28 @@ 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("module from url-loader", ".class { background: green url(module) xyz }", [
[1, ".class { background: green url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAA) xyz }", ""]
], "", { './module': "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAA" });

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