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

avoid backtracking #100

Merged
merged 1 commit into from
Mar 28, 2022
Merged

avoid backtracking #100

merged 1 commit into from
Mar 28, 2022

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Mar 28, 2022

Fixes #97. Supersedes #89 and #99. The problem was that this expression is fundamentally ambiguous:

/\s*([+-]?\d*\.?\d+(?:[eE][+-]?\d+)?)%\s*/

Since both the dot and the digits preceding the dot are optional, there’s a combinatorial explosion of possible valid matches. If we instead combine it into an optional group and make the dot required for that group, the explosion is avoided:

/\s*([+-]?(?:\d*\.)?\d+(?:[eE][+-]?\d+)?)%\s*/

Demo: https://observablehq.com/d/4b1d645fe3da1226

@mbostock mbostock requested a review from Fil March 28, 2022 03:41
Copy link
Member

@Fil Fil left a comment

Choose a reason for hiding this comment

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

elegant!

@mbostock mbostock merged commit 994d8fd into main Mar 28, 2022
@mbostock mbostock deleted the mbostock/avoid-backtracking branch March 28, 2022 16:08
@demartsc
Copy link

demartsc commented Jun 3, 2022

Is it possible to apply this change to v1.4.1 of d3-color as well? For those who still need to support ES5 it would be helpful to have the vulnerability fixed on the ES5 compatible version of d3-color as well. I am happy to submit a PR off of the v1.4.1 tag if bumping that lesser version to v1.4.2 is something you are willing to do. Please let me know?

@jayuen
Copy link

jayuen commented Sep 29, 2022

@mbostock Same boat as @demartsc above. From #100, it looks like the change in the regex string would be compatible with the code in the 1.4.1 tag (I ran it locally and all the specs passed).

Would it be acceptable to get access to make a PR off the v1.4.1 tag for your review?

@G-Rath
Copy link

G-Rath commented Oct 3, 2022

Would love to see a backport if possible; we would also appreciate if we could get a backport to the v2 tag as well because that's the highest major version supported by d3-interpolate and v3 of that package switches it to using ESM modules which we can't use in our applications.

I'm happy to help with these if there's anything I can do, including getting the GitHub advisories updated.

@spreusler
Copy link

@mbostock Same boat as @demartsc above. From #100, it looks like the change in the regex string would be compatible with the code in the 1.4.1 tag (I ran it locally and all the specs passed).

Would it be acceptable to get access to make a PR off the v1.4.1 tag for your review?

This would be great!

jayuen added a commit to jayuen/d3-color that referenced this pull request Oct 5, 2022
@AtishayMsft
Copy link

Would love to see a backport if possible; we would also appreciate if we could get a backport to the v2 tag as well because that's the highest major version supported by d3-interpolate and v3 of that package switches it to using ESM modules which we can't use in our applications.

I'm happy to help with these if there's anything I can do, including getting the GitHub advisories updated.

We would also like the dependencies of d3-color updated for v2 for d3-interpolate and d3-scale to support ES5 compatibility.

@jayuen
Copy link

jayuen commented Oct 6, 2022

Would love to see a backport if possible; we would also appreciate if we could get a backport to the v2 tag as well because that's the highest major version supported by d3-interpolate and v3 of that package switches it to using ESM modules which we can't use in our applications.

I'm happy to help with these if there's anything I can do, including getting the GitHub advisories updated.

@G-Rath and others who have 👍. I'm new to contributing to open source so would appreciate your help if possible. Since I don't have PR access on this repo, I forked it to jayuen/d3-color, created a branch off the v1.4.1 tag, and backported the fix there. I imagine we can create similar branches for the V2 and V3 tags. However, I'm not really sure what the next step is to a) silence the advisories and b) get this PR up for review in this repo. Any guidance would be appreciated!

@G-Rath
Copy link

G-Rath commented Oct 6, 2022

@jayuen there's nothing more you can do for now - it's up to how @mbostock wants to handle that since no branch exists for those versions so there's nothing to PR into right now.

@AtishayMsft
Copy link

All, I have logged an issue with d3 for backporting this fix to v2.x at #108.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Avoid catastrophic backtracking when parsing
7 participants