-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Prevent getSelector
from returning URLs as selector
#32586
Conversation
…re invalid selectors
…r keeping urls from being returned as a selector
// 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('.'))) { |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
getSelector
from returning URLs as selector
There was a problem hiding this 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. 🤔
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
bootstrap/js/src/util/index.js
Lines 35 to 45 in 9b7bb7b
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