Skip to content

Avoid duplicate trailing delimiter matches#443

Open
vibhor-aggr wants to merge 2 commits into
pillarjs:masterfrom
vibhor-aggr:fix/avoid-duplicate-trailing-delimiter
Open

Avoid duplicate trailing delimiter matches#443
vibhor-aggr wants to merge 2 commits into
pillarjs:masterfrom
vibhor-aggr:fix/avoid-duplicate-trailing-delimiter

Conversation

@vibhor-aggr

Copy link
Copy Markdown

Fixes #441.

pathToRegexp('/about/') currently appends the optional trailing delimiter after a source that already ends in /, so the generated matcher accepts /about//. This moves the optional trailing delimiter into each flattened alternative and skips it when that alternative already ends with the configured delimiter.

Verification:

  • ./node_modules/.bin/vitest run src/index.spec.ts -t '/test//'
  • ./node_modules/.bin/vitest run src/index.spec.ts
  • npm run build
  • npm run size
  • git diff --check

Note: I also attempted npm test; it reached 477 passing Vitest tests but the local ReDoS helper binary requires GLIBC_2.32/GLIBC_2.34 while this host has glibc 2.28, so that command exits with recheck-linux-x64 EPIPE errors in this environment.

@blakeembrey blakeembrey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not really convinced this is worth doing. No one has run into it as an issue, and while it might be considered a minor correctness issue, the logic of it is "simple" today. The change has to land in a major just to be sure no one is relying on it accidentally. If we can do it without any change in overall bytes I'd consider it.

Comment thread src/index.ts Outdated
@iambumblehead

Copy link
Copy Markdown

@vibhor-aggr thanks; here's hoping this PR goes well and eventually gets in. Very nice that its requires only a few lines.

@vibhor-aggr

Copy link
Copy Markdown
Author

Thanks for the review. I moved the trailing delimiter handling into toRegExpSource in 4137f2e7e29731830da32513b072721d6abf7e7b, so each flattened alternative decides whether it needs the optional delimiter. The current diff removes the global trailing suffix and avoids appending it when the alternative already ends with the delimiter.

Verification from the follow-up run:

  • focused /test// Vitest: 3 passing
  • full src/index.spec.ts: 376 passing
  • npm run build: passed
  • npm run size: 1.94 kB minified and brotlied, under the 2 kB limit
  • git diff --check master..HEAD: passed

I also attempted full npm test; it reached 477 passing Vitest tests, but this host cannot run the ReDoS helper binary because recheck-linux-x64 requires GLIBC_2.32/GLIBC_2.34, so that command exits with EPIPE after the loader failure.

@iambumblehead

Copy link
Copy Markdown

This PR is strange in various ways and Blake expressed disinterest in this PR and its ticket. Perhaps "vibhor" in "vibhor-aggr" stands for something viber/vibe-coder and Blake and I are the only real people here. Maybe Blake is not "real" either I don't know anymore.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

regexp should not end with both explicit end-slash and optional end-slash

4 participants