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(get-selector): do not URL encode or token escape attribute selectors #3215

Merged
merged 4 commits into from
Nov 9, 2021

Conversation

straker
Copy link
Contributor

@straker straker commented Oct 14, 2021

When trying to get the selector for the images on #3109, the end result of the getAttributeNameValue would end up being URI encoded and token escaped.

What that means is that instead of returning [href$="1 Seater"] (a valid CSS attribute selector) it would first URI encode it, which would turn into [href$="1%20Seater"], and then token escape it which would try to prevent the first character from being a number (an invalid CSS token) and escape it, resulting in [href$="\31 %20Seater"]. This selector would not find anything in the document so :root would be returned by default.

Since attribute selectors can be strings and since we're wrapping the value in quotes, it wasn't necessary to do any escaping or encoding.

Attribute values must be <ident-token>s or <string-token>s

Closes issue: #3109

@straker straker requested a review from a team as a code owner October 14, 2021 20:32
lib/core/utils/get-selector.js Outdated Show resolved Hide resolved
dylanb
dylanb previously requested changes Oct 15, 2021
Copy link
Contributor

@dylanb dylanb left a comment

Choose a reason for hiding this comment

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

Need to also not escape when the full URL is used

lib/core/utils/get-selector.js Outdated Show resolved Hide resolved
@straker straker dismissed stale reviews from dylanb and WilcoFiers October 15, 2021 15:51

Resolved

function escapeAttribute(str) {
return (
str
// @see https://www.py4u.net/discuss/286669
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this supposed to go? I get a 502 on this.

Copy link
Contributor Author

@straker straker Oct 18, 2021

Choose a reason for hiding this comment

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

Well it did go to a discussion about what values of an attribute string should be replaced. It was the only place I could find something. Would you happen to know of an official spec for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to be working again

test/core/utils/get-selector.js Show resolved Hide resolved
test/core/utils/get-selector.js Show resolved Hide resolved
test/core/utils/get-selector.js Show resolved Hide resolved
lib/core/utils/get-selector.js Show resolved Hide resolved
Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

Would using CSS.escape() or a ponyfil make more sense than a custom implementation here?

@straker
Copy link
Contributor Author

straker commented Oct 18, 2021

I looked at CSS.escape but it works like our escapeSelector code in that it doesn't take into account that the string you pass is used in an attribute selector, so will still escape 1 Seater as '\31 \ Seater' which wouldn't match.

I'd be fine replacing our escapeSelector code with CSS.escape, but in a different pr.

@straker straker merged commit 6f7e183 into develop Nov 9, 2021
@straker straker deleted the get-selector-escape branch November 9, 2021 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants