Skip to content

fix(worker): prevent long repository inputs from stalling requests - #120

Merged
vincentkoc merged 1 commit into
mainfrom
security/codeql-repository-normalization-20260829
Aug 29, 2026
Merged

fix(worker): prevent long repository inputs from stalling requests#120
vincentkoc merged 1 commit into
mainfrom
security/codeql-repository-normalization-20260829

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Aug 29, 2026

Copy link
Copy Markdown
Member

Related: https://github.com/openclaw/crabfleet/security/code-scanning/3

What Problem This Solves

Fixes an issue where repository inputs containing long internal slash runs could stall Worker requests during normalization.

Why This Change Was Made

Trailing slash removal now uses one reverse index scan and one final slice instead of a backtracking regular expression. Existing trimming, case normalization, GitHub URL removal, .git handling, and valid outputs are unchanged.

User Impact

Repository normalization remains responsive for adversarial input without changing accepted repository values.

Evidence

  • Pre-fix focused reproduction: 32,768 internal slashes took 583 ms and failed the 250 ms regression bound.
  • Fixed focused test: the same input completes in approximately 0.1 ms.
  • Generated equivalence check: 10,000 inputs matched the previous valid output behavior.
  • pnpm check
  • pnpm test (1,003 tests)
  • pnpm build
  • DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer pnpm macos:test
  • Codex Sol/high autoreview: no accepted or actionable findings.
  • AWS Crabbox run_7584c26fa9cb: clean no-hydration checkout at the exact head; Docker-backed Wrangler dry-run, focused test, check, full test, and build passed; owned lease released.
  • Hosted exact-head Worker and CodeQL gates passed, including JavaScript/TypeScript and Swift analysis.
  • Latest exact-head ClawSweeper review was read; its changelog finding was explicitly skipped under the frozen two-file execution scope, and every requested verification gate was completed.

@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 29, 2026
@clawsweeper

clawsweeper Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed August 29, 2026, 2:02 AM ET / 06:02 UTC.

ClawSweeper review

What this changes

The PR replaces regex-based trailing-slash removal in Worker repository normalization with a linear reverse scan and adds regression coverage for very long internal slash runs.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

This PR remains necessary because current main still uses the pathological trailing-slash regex. The linear replacement is sound, but the required Unreleased changelog entry remains absent.

Priority: P2
Reviewed head: e9c1033d483c806ecff4e9477a6596adc692c4cd

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The focused production fix and regression test are solid; the remaining required release-note entry is a small merge-readiness gap.
Proof confidence 🌊 off-meta tidepool Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate; its body nonetheless reports an exact-head terminal timing result and clean Crabbox validation for the changed Worker path.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate; its body nonetheless reports an exact-head terminal timing result and clean Crabbox validation for the changed Worker path.
Evidence reviewed 5 items Current main retains the vulnerable normalization step: The pinned main revision retains the anchored trailing-slash regular expression; the PR replaces only that step with a reverse scan while retaining the surrounding normalization chain.
Focused regression coverage: The introduced test checks established normalization outputs and asserts the 32,768-slash internal-run case completes within 250 ms.
Required release note is absent: The repository policy requires one dense Unreleased line for each user-visible change, while CHANGELOG.md has an empty Unreleased section and the PR changes no changelog file.
Findings 1 actionable finding [P3] Add the required Unreleased changelog entry
Security None None.

How this fits together

Repository input is normalized in the Worker before it is parsed into GitHub owner and repository parts for downstream API requests. This change makes that input-cleanup step responsive under adversarial slash runs.

flowchart LR
A[Repository input] --> B[Worker normalization]
B --> C[Trailing slash handling]
C --> D[Owner and repository parser]
D --> E[GitHub request handling]
Loading

Before merge

  • Add the required Unreleased changelog entry (P3) - This PR changes user-visible Worker responsiveness, but CHANGELOG.md still has an empty Unreleased section. The repository policy requires one dense Unreleased line for every user-visible change, so add that entry before landing.
  • Resolve merge risk (P1) - Without a dense Unreleased entry, this user-visible responsiveness fix would be omitted from the release notes.

Findings

  • [P3] Add the required Unreleased changelog entry — src/worker/repositories.ts:7
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Diff scope production +5/-4, tests +28 across 2 files The repair is narrowly confined to repository normalization and direct regression coverage.
Adversarial regression case 1 test with 32,768 slashes It directly covers the input shape reported to trigger the Worker stall.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Retain the linear normalization repair and its focused regression test, then add one dense Unreleased entry describing the bounded repository-input handling.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best possible solution:

Retain the linear normalization repair and its focused regression test, then add one dense Unreleased entry describing the bounded repository-input handling.

Do we have a high-confidence way to reproduce the issue?

Yes, with high confidence from the current-main anchored slash regex and the supplied 32,768-slash pre-fix timing scenario; this review did not execute target code in the read-only checkout.

Is this the best way to solve the issue?

Yes, the reverse scan removes the pathological backtracking opportunity while preserving the existing trim, case-folding, GitHub-prefix, trailing-slash, and .git normalization order.

Full review comments:

  • [P3] Add the required Unreleased changelog entry — src/worker/repositories.ts:7
    This PR changes user-visible Worker responsiveness, but CHANGELOG.md still has an empty Unreleased section. The repository policy requires one dense Unreleased line for every user-visible change, so add that entry before landing.
    Confidence: 0.99

Overall correctness: patch is correct
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ec33952bac50.

Labels

Label justifications:

  • P2: This is a bounded Worker responsiveness defect in shared repository input normalization.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate; its body nonetheless reports an exact-head terminal timing result and clean Crabbox validation for the changed Worker path.

Evidence

Acceptance criteria:

  • [P1] pnpm check.
  • [P1] pnpm test.
  • [P1] pnpm macos:test.

What I checked:

  • Current main retains the vulnerable normalization step: The pinned main revision retains the anchored trailing-slash regular expression; the PR replaces only that step with a reverse scan while retaining the surrounding normalization chain. (src/worker/repositories.ts:1, ec33952bac50)
  • Focused regression coverage: The introduced test checks established normalization outputs and asserts the 32,768-slash internal-run case completes within 250 ms. (tests/repositories.test.ts:7, e9c1033d483c)
  • Required release note is absent: The repository policy requires one dense Unreleased line for each user-visible change, while CHANGELOG.md has an empty Unreleased section and the PR changes no changelog file. (CHANGELOG.md:3, ec33952bac50)
  • Feature provenance: Blame attributes the original normalization implementation, including the trailing-slash regex, to the commit that introduced this module; its history identifies Vincent Koc as the original feature contributor. (src/worker/repositories.ts:1, 74e04a2bec23)
  • Introduced-change ownership is verified: The verified merge base equals pinned main, and the exact head introduces only the production normalization change and its focused test. (src/worker/repositories.ts:1, e9c1033d483c)

Likely related people:

  • vincentkoc: Vincent Koc introduced the repository-normalization module and authored the current focused repair. (role: original feature contributor and recent area contributor; confidence: high; commits: 74e04a2bec23, e9c1033d483c; files: src/worker/repositories.ts)
  • Peter Steinberger: Repository history associates the preceding Worker architecture refactor with this normalization surface. (role: adjacent architecture contributor; confidence: medium; commits: df9bdc9952d4; files: src/worker/repositories.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add one dense Unreleased changelog line, then rerun the repository-required gates.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-08-29T05:54:05.819Z sha e9c1033 :: needs changes before merge. :: [P3] Add the required Unreleased changelog entry

@vincentkoc

Copy link
Copy Markdown
Member Author

ClawSweeper disposition for exact head e9c1033d483c806ecff4e9477a6596adc692c4cd:

  • Changelog finding: intentionally skipped. This bounded CodeQL repair preserves all valid repository-normalization outputs, and the independently reviewed execution contract explicitly excludes changelog changes and restricts write scope to the production owner plus its focused regression test.
  • Exact-head gates completed: pnpm check, pnpm test, pnpm build, pnpm macos:test, and Codex Sol/high autoreview.
  • Clean AWS Crabbox proof: run_7584c26fa9cb used a fresh no-hydration checkout at the exact head; focused test, check, full test, build, and Docker-backed Wrangler dry-run passed. The owned lease was released.

No rank-up move is being passed silently.

@vincentkoc
vincentkoc marked this pull request as ready for review August 29, 2026 06:13
@vincentkoc
vincentkoc merged commit 4e10690 into main Aug 29, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant