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.

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

Conversation

kgram
Copy link
Contributor

@kgram kgram commented Oct 31, 2017

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
Copy link

jsf-clabot commented Oct 31, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

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

@@ -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]) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

@michael-ciniawsky michael-ciniawsky changed the title URL escaping and quote-wrapping fix: proper URL escaping and wrapping Oct 31, 2017
@michael-ciniawsky michael-ciniawsky added this to the 0.28.8 milestone Oct 31, 2017
@michael-ciniawsky
Copy link
Member

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
Copy link

codecov bot commented Oct 31, 2017

Codecov Report

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

Impacted file tree graph

@@            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
Copy link

codecov bot commented Oct 31, 2017

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

michael-ciniawsky commented Oct 31, 2017

Could you check if

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

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

@kgram
Copy link
Contributor Author

kgram commented Oct 31, 2017

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

}
// 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.

lib/loader.js Outdated
@@ -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?

lib/loader.js Outdated
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?

lib/loader.js Outdated
}.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.

], "", { './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.

@@ -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.

@michael-ciniawsky
Copy link
Member

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

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

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

@kgram
Copy link
Contributor Author

kgram commented Nov 1, 2017

I think everything is either fixed or addressed now :)

@alexander-akait
Copy link
Member

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

@kgram
Copy link
Contributor Author

kgram commented Nov 9, 2017

@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?

@@ -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.

@DawnWright
Copy link

This PR introduced the following bug: #659

eliperelman pushed a commit to neutrinojs/neutrino that referenced this pull request Jul 10, 2018
This Pull Request updates dependency [css-loader](https://github.com/webpack-contrib/css-loader) from `^0.28.11` to `^1.0.0`



<details>
<summary>Release Notes</summary>

### [`v1.0.0`](https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md#&#8203;100httpsgithubcomwebpack-contribcss-loadercomparev02811v100-2018-07-06)
[Compare Source](webpack-contrib/css-loader@v0.28.11...v1.0.0)
##### BREAKING CHANGES

* remove `minimize` option, use [`postcss-loader`](https://github.com/postcss/postcss-loader) with [`cssnano`](https://github.com/cssnano/cssnano) or use [`optimize-cssnano-plugin`](https://github.com/intervolga/optimize-cssnano-plugin) plugin
* remove `module` option, use `modules` option instead
* remove `camelcase` option, use `camelCase` option instead
* remove `root` option, use [`postcss-loader`](https://github.com/postcss/postcss-loader) with [`postcss-url`](https://github.com/postcss/postcss-url) plugin
* remove `alias` option, use [`resolve.alias`](https://webpack.js.org/configuration/resolve/) feature or use [`postcss-loader`](https://github.com/postcss/postcss-loader) with [`postcss-url`](https://github.com/postcss/postcss-url) plugin
* update `postcss` to `6` version
* minimum require `nodejs` version is `6.9`
* minimum require `webpack` version is `4`
#### [0.28.11](webpack-contrib/css-loader@v0.28.10...v0.28.11) (2018-03-16)
##### Bug Fixes

* **lib/processCss:** don't check `mode` for `url` handling (`options.modules`) ([#&#8203;698](`webpack-contrib/css-loader#698)) ([c788450](webpack-contrib/css-loader@c788450))
#### [0.28.10](webpack-contrib/css-loader@v0.28.9...v0.28.10) (2018-02-22)
##### Bug Fixes

* **getLocalIdent:** add `rootContext` support (`webpack >= v4.0.0`) ([#&#8203;681](`webpack-contrib/css-loader#681)) ([9f876d2](webpack-contrib/css-loader@9f876d2))
#### [0.28.9](webpack-contrib/css-loader@v0.28.8...v0.28.9) (2018-01-17)
##### Bug Fixes

* ignore invalid URLs (`url()`) ([#&#8203;663](`webpack-contrib/css-loader#663)) ([d1d8221](webpack-contrib/css-loader@d1d8221))
#### [0.28.8](webpack-contrib/css-loader@v0.28.7...v0.28.8) (2018-01-05)
##### Bug Fixes

* **loader:** correctly check if source map is `undefined` ([#&#8203;641](`webpack-contrib/css-loader#641)) ([0dccfa9](webpack-contrib/css-loader@0dccfa9))
* proper URL escaping and wrapping (`url()`) ([#&#8203;627](`webpack-contrib/css-loader#627)) ([8897d44](webpack-contrib/css-loader@8897d44))
#### [0.28.7](webpack-contrib/css-loader@v0.28.6...v0.28.7) (2017-08-30)
##### Bug Fixes

* pass resolver to `localsLoader` (`options.alias`)  ([#&#8203;601](`webpack-contrib/css-loader#601)) ([8f1b57c](webpack-contrib/css-loader@8f1b57c))
#### [0.28.6](webpack-contrib/css-loader@v0.28.5...v0.28.6) (2017-08-30)
##### Bug Fixes

* add support for aliases starting with `/` (`options.alias`) ([#&#8203;597](`webpack-contrib/css-loader#597)) ([63567f2](webpack-contrib/css-loader@63567f2))
#### [0.28.5](webpack-contrib/css-loader@v0.28.4...v0.28.5) (2017-08-17)
##### Bug Fixes

* match mutliple dashes (`options.camelCase`) ([#&#8203;556](`webpack-contrib/css-loader#556)) ([1fee601](webpack-contrib/css-loader@1fee601))
* stricter `[@import]` tolerance ([#&#8203;593](`webpack-contrib/css-loader#593)) ([2e4ec09](webpack-contrib/css-loader@2e4ec09))
#### [0.28.4](webpack-contrib/css-loader@v0.28.3...v0.28.4) (2017-05-30)
##### Bug Fixes

* preserve leading underscore in class names ([#&#8203;543](`webpack-contrib/css-loader#543)) ([f6673c8](webpack-contrib/css-loader@f6673c8))
#### [0.28.3](webpack-contrib/css-loader@v0.28.2...v0.28.3) (2017-05-25)
##### Bug Fixes

* correct plugin order for CSS Modules ([#&#8203;534](`webpack-contrib/css-loader#534)) ([b90f492](webpack-contrib/css-loader@b90f492))
#### [0.28.2](webpack-contrib/css-loader@v0.28.1...v0.28.2) (2017-05-22)
##### Bug Fixes

* source maps path on `windows` ([#&#8203;532](`webpack-contrib/css-loader#532)) ([c3d0d91](webpack-contrib/css-loader@c3d0d91))
#### [0.28.1](webpack-contrib/css-loader@v0.28.0...v0.28.1) (2017-05-02)
##### Bug Fixes

* allow to specify a full hostname as a root URL ([#&#8203;521](`webpack-contrib/css-loader#521)) ([06d27a1](webpack-contrib/css-loader@06d27a1))
* case insensitivity of [@&#8203;import] ([#&#8203;514](`webpack-contrib/css-loader#514)) ([de4356b](webpack-contrib/css-loader@de4356b))
* don't handle empty [@&#8203;import] and url() ([#&#8203;513](`webpack-contrib/css-loader#513)) ([868fc94](webpack-contrib/css-loader@868fc94))
* imported variables are replaced in exports if followed by a comma ([#&#8203;504](`webpack-contrib/css-loader#504)) ([956bad7](webpack-contrib/css-loader@956bad7))
* loader now correctly handles `url` with space(s) ([#&#8203;495](`webpack-contrib/css-loader#495)) ([534ea55](webpack-contrib/css-loader@534ea55))
* url with a trailing space is now handled correctly ([#&#8203;494](`webpack-contrib/css-loader#494)) ([e1ec4f2](webpack-contrib/css-loader@e1ec4f2))
* use `btoa` instead `Buffer` ([#&#8203;501](`webpack-contrib/css-loader#501)) ([fbb0714](webpack-contrib/css-loader@fbb0714))
##### Performance Improvements

* generate source maps only when explicitly set ([#&#8203;478](`webpack-contrib/css-loader#478)) ([b8f5c8f](webpack-contrib/css-loader@b8f5c8f))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing quotes in url() for data URIs returned from another loader
6 participants