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

#1621 fix: correct escaping of backslashes in URL wrapping #1622

Closed
wants to merge 4 commits into from
+6 −1
Closed

#1621 fix: correct escaping of backslashes in URL wrapping #1622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0d0e829
#1621 fix: correct escaping of backslashes in URL wrapping
ruchira-net Nov 22, 2024
65245c9
#1621 fix: complication error
ruchira-net Nov 26, 2024
e027815
test: add snapshot and test case for URL escaping with backslashes (#…
ruchira-net Nov 26, 2024
28ca565
test: update URL snapshot and test case for escaping backslashes (#1621)
ruchira-net Dec 2, 2024
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
2 changes: 1 addition & 1 deletion src/runtime/getUrl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
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 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

Copy link
Author

@ruchira-net ruchira-net Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@alexander-akait alexander-akait Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

@ruchira-net ruchira-net Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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"); }

Copy link
Member

Choose a reason for hiding this comment

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

How does this violate security?

Copy link
Author

@ruchira-net ruchira-net Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@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

return `"${url.replace(/"/g, '\\"').replace(/\n/g, "\\n")}"`;
return `"${url.replace(/\\/g, '\\\\').replace(/"/g, '\\"').replace(/\n/g, "\\n")}"`;
}

return url;
Expand Down
2 changes: 2 additions & 0 deletions test/runtime/__snapshots__/getUrl.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,5 @@ exports[`escape should escape url 41`] = `""image other.png#hash""`;
exports[`escape should escape url 42`] = `""image other.png#hash""`;

exports[`escape should escape url 43`] = `""image other.png#hash""`;

exports[`escape should escape url 44`] = `"https://www.example.com?path=path\\to\\resource"`;
3 changes: 3 additions & 0 deletions test/runtime/getUrl.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,5 +127,8 @@ describe("escape", () => {
{ hash: "#hash", needQuotes: true },
),
).toMatchSnapshot();
expect(
getUrl("https://www.example.com?path=path\\to\\resource"),
).toMatchSnapshot();
});
});