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(dom): Implement getElementsByClassName as specified #213

Merged
merged 10 commits into from Jul 27, 2021

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Apr 8, 2021

Reimplements Document.getElementsByClassName without using a dynamically generated regex.

  1. Unlike CSS grammar, there seems to be no limitation on which classnames are valid in HTML/DOM (given that they don't contain space chars). That is, [ seems to be a valid classname, which was broken in this impl as it was fed into a regexp unescaped and caused xmldom to throw.
  2. Whitespace processing should be now correct. \s in JS regexps includes more chars than space-separated tokens of html.
  3. This now should support a list of space-separated classnames as an argument. Previously, that was broken and order-dependent.
  4. This prevents causing a ReDoS by passing in exponential regexps as strings to getElementsByClassName() method,
    e.g. new RegExp("(^|\\s)" + '(((a||||)+)+)+' + "(\\s|$)").test('aaaaab').

Refs:

  1. https://dom.spec.whatwg.org/#dom-document-getelementsbyclassname
  2. https://dom.spec.whatwg.org/#dom-element-getelementsbyclassname
  3. https://dom.spec.whatwg.org/#concept-getelementsbyclassname
  4. https://www.w3.org/TR/html52/dom.html#element-attrdef-global-class
  5. https://www.w3.org/TR/html52/infrastructure.html#set-of-space-separated-tokens
  6. Implement Document.getElementsByClassName #24
  7. Fix #24 Implement Document.getElementsByClassName #25

Note: I couldn't find the tests for this and am unsure how to test this. cc @codler perhaps — could you confirm this would work?
Seems to work for me with simple playground tests.

@ChALkeR ChALkeR force-pushed the fix/getElementsByClassName branch 2 times, most recently from 50ea8ff to 4800332 Compare April 8, 2021 07:50
@ChALkeR ChALkeR marked this pull request as ready for review April 8, 2021 08:05
Copy link
Member

@karfau karfau left a comment

Choose a reason for hiding this comment

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

Thank you for discovering this and for your contribution.
I added some suggestions and recommendations.
If you don't have the capacity or knowledge, to do what I'm asking for, feel free to ask any questions for support, or let us know if/when it's beyond your capacity.

lib/dom.js Outdated Show resolved Hide resolved
lib/dom.js Outdated Show resolved Hide resolved
lib/dom.js Outdated Show resolved Hide resolved
lib/dom.js Outdated Show resolved Hide resolved
lib/dom.js Outdated Show resolved Hide resolved
@karfau
Copy link
Member

karfau commented Apr 8, 2021

This PR will also have an impact on #100 since that one tries to add the ability to also call getElementsByClassName on an Element.

@karfau karfau added bug Something isn't working spec:DOM Living Standard https://dom.spec.whatwg.org/ labels Apr 8, 2021
@karfau karfau added this to the next release milestone Apr 8, 2021
@karfau
Copy link
Member

karfau commented May 29, 2021

@ChALkeR please let us know if you plan to continue to work on this on your own, thx

@karfau karfau force-pushed the fix/getElementsByClassName branch from 0178a7f to f6126f8 Compare July 27, 2021 04:38
@karfau karfau force-pushed the fix/getElementsByClassName branch from d743533 to 9d4280c Compare July 27, 2021 05:52
@karfau karfau changed the title fix: reimplement getElementsByClassName without regex fix(dom): Implement getElementsByClassName as specified Jul 27, 2021
@karfau
Copy link
Member

karfau commented Jul 27, 2021

I resolved all the remarks that I added earlier. Thanks again for your contribution.

I needed to force push the rebased branch, other wise github was showing changes to some 30 files and conflicts.

I took the liberty to rename the PR and do a minor tweak to the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working spec:DOM Living Standard https://dom.spec.whatwg.org/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants