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

Prevent getSelector from returning URLs as selector #32586

Merged

Conversation

RoflCopter24
Copy link
Contributor

@RoflCopter24 RoflCopter24 commented Dec 22, 2020

As discussed in issue #32273 the Dropdown and Scrollspy Modules crash with Uncaught DOMException: Failed to execute 'querySelector' on 'Document': 'foo/bar.html' is not a valid selector when there happens to be an actual URL in the href.

The culprit seems to be, that

const getSelector = element => {
let selector = element.getAttribute('data-bs-target')
if (!selector || selector === '#') {
const hrefAttr = element.getAttribute('href')
selector = hrefAttr && hrefAttr !== '#' ? hrefAttr.trim() : null
}
return selector
}

does no checks if that href contains a valid selector.

This pull request aims to prevent that case by adding conditions that only allow class selectors (.target) and anchors/IDs (#target) to be returned as selectors, thus preventing crashes caused by real URLs.

Fixes #32273

js/src/util/index.js Outdated Show resolved Hide resolved
@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Dec 22, 2020
@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0-beta2 Dec 22, 2020
// The only valid content that could double as a selector are ids, so everything starting with . or #
// If a 'real' url is used as the selector, document.querySelector will rightfully
// complain it is invalid. (see twbs#32273)
if (!hrefAttr || (!hrefAttr.includes('#') && !hrefAttr.startsWith('.'))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It restricts the user to use only the class and id selectors in the href attribute. This restriction was not before 🤔

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 do not see a problem there. I did not find any mention in the documentation, that anything other than IDs in the href are even supported.

Considering that that freedom for using invalid hrefs as selectors was exactly what caused problems and lead to this PR (and this is the big jump to v5), I think this would be the last opportunity to make such a change before possibly a myriad of users starts misusing the href in that way.

Besides, that's what the data-bs-target attribute is for, right? (which is encouraged everywhere in the docs)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this would fix the issue we have. I guess it's a good solution for v5. We can revisit in v6.

@XhmikosR XhmikosR changed the title [JS] Prevent getSelector from returning URLs as selector Prevent getSelector from returning URLs as selector Jan 28, 2021
v5.0.0-beta2 automation moved this from Review to Approved Jan 28, 2021
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

The comment on line 41 is a little bit misleading, id selector does not start with a dot (.).
So this comment should be removed IMO. 🤔 

@XhmikosR XhmikosR merged commit 2a9d721 into twbs:main Feb 3, 2021
v5.0.0-beta2 automation moved this from Approved to Done Feb 3, 2021
@RoflCopter24 RoflCopter24 deleted the 32273-dropdown-item-error-with-external-link branch February 3, 2021 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

Dropdown-item error with external link when using Scrollspy.
4 participants