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

Jump to bottom

Prevent getSelector from returning URLs as selector #32586

Merged
merged 11 commits into from Feb 3, 2021
Merged

Prevent getSelector from returning URLs as selector #32586

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
20fca9b
added checks to getSelector in util to prevent returning hrefs that a…
Dec 21, 2020
24fe99c
restored compatibility for the class selector and added test cases fo…
Dec 22, 2020
d5f340d
Merge branch 'main' into 32273-dropdown-item-error-with-external-link
RoflCopter24 Dec 22, 2020
9540d6c
Code review: merge null check into following condition
Dec 22, 2020
0896531
Merge branch 'main' into 32273-dropdown-item-error-with-external-link
RoflCopter24 Dec 22, 2020
7883619
Update index.spec.js
XhmikosR Dec 29, 2020
82b1274
Merge branch 'main' into 32273-dropdown-item-error-with-external-link
XhmikosR Jan 28, 2021
2349e56
Update index.js
XhmikosR Jan 30, 2021
9636ebf
Merge branch 'main' into 32273-dropdown-item-error-with-external-link
RoflCopter24 Feb 1, 2021
c8629a7
Update index.js
XhmikosR Feb 3, 2021
544c852
Merge branch 'main' into 32273-dropdown-item-error-with-external-link
XhmikosR Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion js/src/util/index.js
View file
Open in desktop
Expand Up @@ -36,7 +36,19 @@ const getSelector = element => {
let selector = element.getAttribute('data-bs-target')

if (!selector || selector === '#') {
const hrefAttr = element.getAttribute('href')
let hrefAttr = element.getAttribute('href')

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

return null
}

// just in case some cms put out a full url with the anchor appended
if (hrefAttr.includes('#') && !hrefAttr.startsWith('#')) {
hrefAttr = '#' + hrefAttr.split('#')[1]
}

selector = hrefAttr && hrefAttr !== '#' ? hrefAttr.trim() : null
}
Expand Down
22 changes: 22 additions & 0 deletions js/tests/unit/util/index.spec.js
View file
Open in desktop
Expand Up @@ -57,6 +57,28 @@ describe('Util', () => {
expect(Util.getSelectorFromElement(testEl)).toEqual('.target')
})

it('should return null if a selector from a href is a url without an anchor', () => {
fixtureEl.innerHTML = [
'<a id="test" data-bs-target="#" href="foo/bar.html"></a>',
'<div class="target"></div>'
].join('')

const testEl = fixtureEl.querySelector('#test')

expect(Util.getSelectorFromElement(testEl)).toBeNull()
})

it('should return the anchor if a selector from a href is a url', () => {
fixtureEl.innerHTML = [
'<a id="test" data-bs-target="#" href="foo/bar.html#target"></a>',
'<div id="target"></div>'
].join('')

const testEl = fixtureEl.querySelector('#test')

expect(Util.getSelectorFromElement(testEl)).toEqual('#target')
})

it('should return null if selector not found', () => {
fixtureEl.innerHTML = '<a id="test" href=".target"></a>'

Expand Down