fix(semver): remove exponential backtracking from the prerelease pattern - #3075
Conversation
CodeQL js/redos, high severity, found on the v2.38.0 promotion PR. The prerelease section used the semver.org pattern verbatim. Its three identifier alternatives overlap: 0 | [1-9]\d* | [0-9A-Za-z-]*[A-Za-z-][0-9A-Za-z-]* and wrapping that in (?:\.identifier)* lets a backtracking engine try an exponential number of ways to split the same string. The cost is real, not theoretical. "0.0.0-0." followed by repetitions of "--." took 522ms for a single 125-character input — inside the 128-character ceiling this module enforces, and inside the 96-character one its only caller passes. A length cap does not fix superlinear growth; it only picks where on the curve the input lands. Going from 20 to 39 repetitions moved 16ms to 524ms. Match the prerelease in one non-backtracking pass and validate each identifier separately with anchored regexes that contain no repeated alternation. Same input took 0.024ms afterwards. Behaviour is unchanged: 36 grammar cases spanning the semver.org examples and the leading-zero, empty-identifier and malformed rejections agree with the old pattern exactly, and the parsed core/prerelease shape is identical. The regressions fail against the old pattern at 524ms and 520ms.
|
✅ Deterministic PR hygiene checks passed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe semver parser replaces a backtracking-prone prerelease regex with loose matching and per-identifier validation. Tests verify ReDoS resistance, length limits, semver grammar, prerelease splitting, numeric conversion, and empty prerelease results. ChangesStrict semver hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR replaces a backtracking-prone prerelease parser with linear validation while preserving existing semver behavior. The change is localized, and no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2858fe7015
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const started = performance.now(); | ||
| expect(parseStrictSemver(attack)).toBeNull(); | ||
| const elapsed = performance.now() - started; |
There was a problem hiding this comment.
On a loaded CI worker, the process can be descheduled or paused for GC between these performance.now() calls, so a correct linear parse taking microseconds of CPU can still report more than 50 ms and fail the suite; the measure() helper below has the same wall-clock dependency. Use process.cpuUsage()—as the repository already does for bounded-work regression tests—or another deterministic mechanism that excludes scheduler pauses.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 67 / 80이 PR은 새 기능이 아닙니다. 지금 구멍이 진짜인지는 패턴만 봐도 알 수 있습니다. 식별자 세 갈래가 겹치고, 그 식별자를 이 함수를 지금 부르는 곳은 고치는 방법은 맞습니다. 미리보기 덩어리는 점수 67은 '진짜 ReDoS이고 고침이 작고 맞지만, 요청마다 도는 파서가 아니라서 75는 아니다'라는 뜻입니다. #3073 승격을 붙잡고 있는 CodeQL 높은 수위 경고를 푸는 값입니다. 제품 동작을 바꾸려는 PR이 아닙니다. 예전과 같은 수락·거절을 유지한 채, 엔진이 같은 문자열을 지수로 나눠 보지 않게만 바꿉니다. src/lib/strict-semver.ts 1줄 (현재 dev) - 미리보기 식별자 세 갈래가 겹치고 그걸 점 반복으로 감싼 예전 패턴이 아직 HEAD에 있다. 이 PR이 없애려는 바로 그 줄이다 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
CodeQL flagged
js/redosat high severity on the v2.38.0 promotion PR (#3073). It is a genuine finding, not noise, and the release is held until it lands.src/lib/strict-semver.tsmatched the prerelease section with the semver.org pattern verbatim. Its three identifier alternatives overlap:Wrapping that in
(?:\.identifier)*gives a backtracking engine an exponential number of ways to split the same string.The cost is measured, not theoretical
The attack input CodeQL named —
0.0.0-0.followed by repetitions of--.— measured on this machine:522ms for a single 125-character input. That is inside the 128-character ceiling this module already enforced, and inside the 96-character ceiling its only caller (
validatedVersioninsrc/codex/cli-install-provenance.ts) passes.That is the part worth being precise about: a length cap does not fix superlinear growth. It only decides where on the curve the input lands. The guard was doing its job and the blowup happened anyway.
The fix
Match the prerelease in one non-backtracking pass (
[0-9A-Za-z.-]+), then validate each identifier separately with anchored regexes that contain no repetition of an alternation. Each check is linear in that identifier's length.Same 125-character input afterwards: 0.024ms.
Behaviour is unchanged
36 cases covering the semver.org examples plus the rejections — leading zeroes, empty identifiers, trailing and doubled dots, numeric identifiers with leading zeroes, non-semver shapes — agree with the old pattern exactly, 0 mismatches. The parsed
coreandprereleaseshape is identical, including thebigintversusstringsplit (1.0.0-0.3.7-xstill yields[0n, 3n, "7-x"]).Verification
Regressions driven red against the old pattern:
With the fix:
bun test tests/strict-semver.test.ts tests/codex-cli-install-provenance.test.ts— 23 pass / 0 fail.bun run typecheckandbun run privacy:scanclean.The other alert
CodeQL also raised
js/stack-trace-exposureat medium onsrc/server/auth-cors.ts:235(jsonResponse). I checked it rather than assuming: there is no.stackaccess anywhere in the server response paths, and everyjsonResponseerror caller passeserror.messageonly. It is a false positive from the taint tracker followingJSON.stringify(data), so nothing is changed for it.Checklist
bun run typecheckpassesbun run privacy:scanpassesdevSummary by CodeRabbit
Bug Fixes
Tests