Skip to content

fix(security): rewrite two ReDoS-able regular expressions flagged by code scanning - #604

Open
devin-ai-integration[bot] wants to merge 4 commits into
devfrom
devin/1789979353-codeql-redos-fixes
Open

devin-ai-integration[bot] wants to merge 4 commits into
devfrom
devin/1789979353-codeql-redos-fixes

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 21, 2026

Copy link
Copy Markdown

Summary

GitHub code scanning (CodeQL javascript-code-scanning suite) flags two regular expressions as ReDoS-able on uncontrolled input. Both are rewritten to be unambiguous while accepting the same language:

  • src/lab/events/limits.tsRAW_POSIX_PATH_RE (error severity). The path body (?:\/|[^/\0\r\n]+)+ could repartition a run of non-slash characters exponentially, and the trailing \/? let a slash run split two ways. It now alternates strictly between non-slash segments and slash runs: [^/\0\r\n]+(?:\/+[^/\0\r\n]+)*\/* — one parse per string. The regex runs on client-supplied lab-event string fields, so a crafted value is a CPU-exhaustion vector on the ingest path.
  • src/adapters/exec-tool-result-normalize.tsEMPTY_EXEC_OUTPUT_REGEX (warning severity). Adjacent \n+ and \s* quantifiers could split a newline block between sections combinatorially; each wildcard run is now pinned to its maximal match via the (?=(X))\1 lookahead-capture idiom. Measured: 80k-newline input went from ~3.9s to <1ms (quadratic → linear).

All other code-scanning findings on this ruleset were triaged and found to be false positives (SHA-256 key fingerprinting reported as password hashing, Math.random() session-affinity IDs, gated URL substring checks, test-file matches) and are not touched here.

Verification

  • Acceptance equivalence: fuzzed old vs. new patterns on 200k randomized strings each plus hand-picked edge cases (bare /, // in path, trailing slashes, blank-line-separated exec wrappers) — zero mismatches.
  • bun test tests/adapters/exec-tool-result-normalize.test.ts tests/lab/lab-post-merge-hardening.test.ts tests/lab/lab-evidence-ledger.test.ts tests/providers/kiro/kiro-adapter.test.ts tests/adapters/tool-catalog-nudge.test.ts tests/responses/openai-responses-passthrough.test.ts — 386 pass, 0 fail.
  • bun run typecheck, bun run privacy:scan — clean.
  • bun run test:changed — 3 failures in tests/vision/vision-routed.test.ts (EBUSY temp-dir cleanup → SPEND_LEDGER_OWNER_HOME_CONFLICT); reproduced identically on a clean dev worktree, so unrelated to this change (Windows file-locking environment issue).

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (Not needed — internal regex refactor, no behavior change.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Link to Devin session: https://app.devin.ai/sessions/18173a51bcf04824a2e66ca1d2e42ccf
Open in Devin Desktop: https://app.devin.ai/desktop/session/18173a51bcf04824a2e66ca1d2e42ccf?variant=devin
Requested by: @luvs01


Devin Review

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

- lab/events/limits.ts: the path-body alternation (\/|[^/\0\r\n]+)+ could
  repartition a non-slash run exponentially; rewritten as a strictly
  alternating segment/slash pattern accepting the same strings.
- exec-tool-result-normalize.ts: adjacent \n+/\s* quantifiers in
  EMPTY_EXEC_OUTPUT_REGEX could repartition a newline block
  combinatorially; each run is now pinned to its maximal match via the
  lookahead-capture idiom. Verified same-language by fuzzing against the
  previous patterns (200k randomized cases each, zero mismatches) and by
  quadratic growth in the old pattern (80k newlines: 3.9s -> <1ms).

Co-Authored-By: Epinephrine <luvs01@hanmail.net>
@devin-ai-integration

Copy link
Copy Markdown
Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: luvs01/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 89c12bd4-502d-4a4b-a662-5f194ee2d424

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

devin-ai-integration[bot]

This comment was marked as resolved.

Coverage for the code-scanning fixes: section-combination acceptance
for EMPTY_EXEC_OUTPUT_REGEX plus a pathological-whitespace case, and
near-4KiB pathological inputs for RAW_POSIX_PATH_RE exercised through
enforceEventStructureLimits.

Co-Authored-By: Epinephrine <luvs01@hanmail.net>
@github-actions

Copy link
Copy Markdown

Deterministic PR hygiene checks passed.

@devin-ai-integration

Copy link
Copy Markdown
Author

Regression coverage added in 675d795:

  • tests/adapters/exec-tool-result-normalize.test.ts — section-combination acceptance corpus for EMPTY_EXEC_OUTPUT_REGEX plus pathological-whitespace inputs (200k newline runs) that would surface a repartitioning regression as a timeout.
  • tests/lab/lab-post-merge-hardening.test.ts — near-4KiB adversarial path strings (a/ segment chains, slash-dense chains, long URL non-path) exercised through enforceEventStructureLimits so RAW_POSIX_PATH_RE stays both correct and linear.

bun test on both files: 34 pass, 0 fail.

@github-actions github-actions Bot added the bug Something isn't working label Sep 21, 2026
@github-actions

github-actions Bot commented Sep 21, 2026

Copy link
Copy Markdown

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

@github-actions
github-actions Bot marked this pull request as draft September 21, 2026 09:20
@github-actions
github-actions Bot marked this pull request as ready for review September 21, 2026 12:33
…test.ts banner (20-min cancel, zero test lines)
@github-actions
github-actions Bot marked this pull request as draft September 21, 2026 14:51
@github-actions
github-actions Bot marked this pull request as ready for review September 21, 2026 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant