fix(server): prevent bot user-agent regex backtracking - #2765
Conversation
Long non-matching User-Agent values made the default Google crawler suffix branch retry a greedy token scan from every character, causing quadratic request-time CPU work. Anchor that branch at token boundaries so the broad *-Google matching contract remains intact while each input segment is scanned once. Cover known suffix crawlers and the long non-match path at the public Pages detector boundary.
|
@codex review |
commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e8f5c7793
ℹ️ 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".
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: ReDoS fix looks correct and effective
I verified the change against the actual regex engine, the request-time call sites, and the Vitest runner source. The fix is sound and the regression test is valid. Approving in substance.
The vulnerability is real and request-triggerable
The pattern runs against the attacker-controlled User-Agent header at request time in multiple server paths: server/dev-server.ts:1090, server/pages-page-data.ts:1194, server/streaming-metadata.ts:8, and server/pages-page-response.ts:742. So quadratic matching on a long UA is a genuine request-time CPU DoS, not just a theoretical concern.
The fix is effective and correct
Measured old vs. new on "a".repeat(n) (Node regex engine):
| n | old [\w-]+-Google | new (?:^|[^\w-])[\w-]+-Google |
|---|---|---|
| 16,000 | 284 ms | 0.2 ms |
| 32,000 | 1,129 ms | 0.4 ms |
| 64,000 | 4,488 ms | 0.5 ms |
The old branch was quadratic; the new one is linear. I also confirmed all remaining branches (Google-[\w-]+, etc.) stay linear through 128k chars.
Correctness is preserved. Any -Google occurrence is preceded by a [\w-]+ run that is itself preceded by either the string start or a [^\w-] delimiter, so the anchor never causes a miss. Verified Mediapartners-Google, AdsBot-Google, Storebot-Google (leading, mid-string, and after !/space/tab), plus Google-Extended and negative cases — all match old behavior.
The Codex reviewer's concern about the timeout test is incorrect
Codex claimed the synchronous isPagesStreamingBot() call blocks the event loop so Vitest's timeout timer can't fire, making the test merely slow rather than failing. This is disproven by the Vitest 4.1.10 runner source. In @vitest/runner/dist/chunk-artifact.js, withTimeout records startTime = now() (line 2268) and, after a sync test returns, resolve() checks if (now() - startTime >= timeout) rejectTimeoutError() (lines 2288-2294 — the fix for vitest-dev/vitest#2920). So a synchronous body that overruns the budget does fail the test even though the setTimeout never fired. The author's response is accurate; the 500 ms bound genuinely enforces the regression. No change needed.
Minor notes (non-blocking)
- This anchoring is a deliberate divergence from Next.js's upstream
[\w-]+-Google(confirmed still unanchored on canary). The added comment documents the intent, which satisfies the AGENTS.md "deliberate divergence is OK when documented" rule. Consider adding a one-line note that this is an intentional divergence from the upstream pattern (not just a perf tweak of vinext's own list) so a future sync with Next.js doesn't silently revert it. - The 64,000-char test has ample headroom (fix runs in ~3 ms vs. the 500 ms limit), so it won't be flaky on slow CI runners.
Overall: correct fix, real security benefit, valid test. LGTM.
|
Review posted successfully. Here's my summary: Review Summary — PR #2765 (fix: prevent bot user-agent regex backtracking)Verdict: LGTM. The fix is correct, effective, and addresses a real vulnerability. Key findings
Minor non-blocking notes
I did not edit any files or run any git write operations, consistent with fork-review mode. |
Overview
*-Googlebranch at the start of each word/hyphen tokenhtml-limited-botsutility used by Pages rendering and metadata bot gatesWhy
The default bot pattern included an unanchored greedy
[\w-]+-Googlebranch. On a long word-only value without the suffix, the JavaScript regex engine retried that scan at every character. The detector runs on request User-Agent headers, so matching work must scale with input length.Requiring either the start of the string or a non-token delimiter before the greedy token preserves the broad Google crawler contract while preventing overlapping retries.
What changed
Mediapartners-Google,AdsBot-Google,Storebot-GoogleGoogle-*and fixed crawler tokenshtmlLimitedBotsconfigurationMaintainer review path
packages/vinext/src/utils/html-limited-bots.tsfor the token-boundary decision.tests/pages-page-response.test.tsfor suffix compatibility and the long-input regression.Validation
Commands
Risk / compatibility
The production change is limited to the default suffix branch and preserves boolean matches for word/hyphen crawler tokens ending in
-Google. The configured-regex path and public configuration shape are unchanged. The new timeout-backed regression has substantial headroom: the fixed 64,000-character case completes in well under the 500 ms limit locally.References