#1621 fix: correct escaping of backslashes in URL wrapping #1622
Conversation
@alexander-akait Done. Thanks! |
test/runtime/getUrl.test.js
Outdated
| @@ -127,5 +127,6 @@ describe("escape", () => { | |||
| { hash: "#hash", needQuotes: true }, | |||
| ), | |||
| ).toMatchSnapshot(); | |||
| expect(getUrl("http://url/path\\/")).toMatchSnapshot(); | |||
There was a problem hiding this comment.
Can you provide a real example of the problem? Using url()?
There was a problem hiding this comment.
@alexander-akait I've added a more real URL example. Hope that's good enough. Thanks!
| @@ -21,7 +21,7 @@ module.exports = (url, options) => { | |||
| // Should url be wrapped? | |||
| // See https://drafts.csswg.org/css-values-3/#urls | |||
| if (/["'() \t\n]|(%20)/.test(url) || options.needQuotes) { | |||
There was a problem hiding this comment.
Why we need escape \\ in runtime? We don't touch such stuff at all, even more, it can literally break already escaped URL
Sorry, we don't need this changes here, even more it worsens the performance
There was a problem hiding this comment.
@alexander-akait we are not escaping the \\ in runtime. we are replacing single \ with \\ to escape the URL. \\ is there in the code to escape a single \. Otherwise JavaScript consider single \ as an escaping character itself.
There was a problem hiding this comment.
And what could be the problem? Please provide a real scenario that could be used in url() and CSS and put it there - https://github.com/webpack-contrib/css-loader/blob/master/test/fixtures/url/url.css, I wouldn't want us to waste perf to something that can never really happen.
That's is why unit tests are not good
There was a problem hiding this comment.
@alexander-akait what if developers have css like this? Does it work right now? I don't think so as the current code doesn't escape the backslashes
.backslashes { background: url("https://www.example.com?path=path\\to\\resource"); }
There was a problem hiding this comment.
How does this violate security?
There was a problem hiding this comment.
@alexander-akait sorry to answer a question with a question, but why are you escaping the newline characters and quotes in the URL then?
This is the same. Whether it's a security concern or not, special characters like backslashes in the URLs should be escaped.
There was a problem hiding this comment.
@ruchira-net Some of them is ident some of them is string, and depending on this we should escaping them or not, I don't see a real integration test of the problem, so I asked to put it in the integration tests and not the unit, I don't want to degrade performance because there is no real problem
This PR contains a:
Motivation / Use-Case
Fixing bug #1621
Breaking Changes
None
Additional Info