fix(harness): strip trailing slashes linearly in buildLeafPrompt (js/polynomial-redos) - #204
Merged
Merged
Conversation
…polynomial-redos)
CodeQL flagged `workspaceRef.replace(/\/+$/, "")` in buildLeafPrompt as a
polynomial ReDoS on library input. It is a real quadratic, not a theoretical
one, and the input is caller-controlled: workspaceRef is a LeafEnvelope field,
so it arrives straight off the POST body and reaches the regex with no
validation or normalisation in between (server.ts hands `body` to runLeaf).
The regex retries from every start position across a run of slashes, so an
input of "/w" + N slashes + a non-slash costs O(N^2):
N=10000 regex 146.7ms linear 0.118ms
N=50000 regex 3636.9ms linear 0.015ms
N=100000 regex 14718.2ms linear 0.005ms
One request field can therefore burn ~15s of CPU per call.
8efd213 already made this exact rewrite for the same regex in buildSolvePrompt
and for toSessionId, naming js/polynomial-redos in its message -- it simply
missed the copy in buildLeafPrompt. So rather than add a third inline scan,
extract the one buildSolvePrompt was using into `stripTrailingSlashes` and
call it from both builders: one implementation, no second copy to miss.
Behaviour is unchanged. 22 cases pin both builders to the old regex's exact
output -- including no slash, one, many, all-slashes, empty, trailing space,
interior slash and non-ASCII -- and they pass identically before and after the
rewrite (they were written against the regex first). A 23rd test is the
regression guard: it asserts a 100k-slash input completes in under 1s, and it
fails at 14740ms on the old code.
Verified: harness `tsc --noEmit` clean; `pnpm -r test` 851 passed, 15 skipped;
run-leaf.test.ts now runs in 3ms.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
pdettori
added a commit
to pdettori/serverless-harness
that referenced
this pull request
Sep 2, 2026
Mechanical output of `make fmt`, no hand edits. This is the backlog from the previous commit: the prettier hook aborted config validation, and its `types_or` never named a real TypeScript tag anyway, so `.ts` files were never formatted even when the config loaded. Kept as its own commit so the fix that unblocks it stays reviewable. Mostly quote style (`.prettierrc` sets singleQuote), trailing commas, comment alignment, markdown table padding and YAML flow-sequence reflow. Verified semantics-preserving: - all 31 changed YAML/JSON files parse to documents identical to their previous contents (compared as parsed structures, not text) - `make typecheck` clean - `pnpm -r test`: 851 passed, 15 skipped - `make test-deploy` passes - `pre-commit run --all-files` exits 0 with all nine hooks running Regenerated rather than replayed when rebasing onto main after rossoctl#203 and rossoctl#204, so it also covers the code rossoctl#204 added -- replaying the old diff would have conflicted with it on run-leaf.ts for no benefit, formatting being mechanical. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clears the CodeQL alert that is the remaining red check on #202. Fixing it rather than dismissing it, because it turns out to be a real quadratic on a request-controlled field, and because the repo already decided to fix this exact pattern once.
Correction to what I said on #202
On #202 I wrote that
workspaceRef"is harness-supplied rather than request data, so the practical exposure looks low." That was wrong.workspaceRefis a field onLeafEnvelope, andserver.tspasses the parsed POSTbodystraight torunLeaf:There is no validation or normalisation of
workspaceRefanywhere in between —validateItemcoversitem, not this field. So the value reaches the regex verbatim from the caller.It is a genuine quadratic
/\/+$/retries from every start position across a run of slashes. With"/w" + N slashes + a non-slash(nothing to strip, worst case):Doubling N roughly quadruples the time. One request field can burn ~15 s of CPU per call, and the leaf path is reachable both inline and via the async queue.
Why a shared helper rather than a third inline scan
8efd213("Replace polynomial trim regexes with linear scans (CodeQL)") already made exactly this rewrite fortoSessionIdand for the same/\/+$/inbuildSolvePrompt— its comment even namesjs/polynomial-redos. It just missed the copy inbuildLeafPrompt, twenty lines away.So this extracts the scan
buildSolvePromptwas already using intostripTrailingSlashesand calls it from both builders. One implementation means there is no second copy for the next sweep to miss. Net effect onrun-leaf.tsis +19/-6, andbuildSolvePrompt's behaviour is untouched.I also checked for other stragglers: no polynomial trim regex remains anywhere in
harness/src. The three remaining/^[^A-Za-z0-9]+|[^A-Za-z0-9]+$/ghits are inpackages/knative-server/test/*— test doubles forleafSessionId, not library-input paths, and unflagged.Behaviour is unchanged, and pinned
The tests were written against the regex first and pass identically before and after the rewrite. 22 cases pin both builders to the old regex's exact output — no slash, one, many, all-slashes, empty, trailing space, interior slash,
/w/./, and non-ASCII (/wörk/):A 23rd test is the regression guard, and it earns its place — on the old code it fails outright:
After the fix:
Tests 24 passed, and the wholerun-leaf.test.tsfile runs in 3 ms.Verification
harnesstsc --noEmitclean.pnpm -r test: 851 passed, 15 skipped (harness 260, up 24 from the new cases).harness/src.Note on #202
This touches
harness/src/run-leaf.tsandharness/test/run-leaf.test.ts, which #202's repo-wide Prettier commit also reformats, so whichever lands second needs a trivial rebase on those files (quote style only). I kept this branch inmain's current double-quote style and within the 100-colprintWidthso it stays clean either way. Happy to rebase #202 once this merges.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com