Skip to content

Scan out-of-process values with the vocabulary but no watcher deadline - #1147

Merged
alexeyzimarev merged 2 commits into
mainfrom
fix/out-of-process-redaction-unbounded
Sep 24, 2026
Merged

alexeyzimarev merged 2 commits into
mainfrom
fix/out-of-process-redaction-unbounded

Conversation

@alexeyzimarev

@alexeyzimarev alexeyzimarev commented Sep 24, 2026 •

Copy link
Copy Markdown
Member

No GitHub issue and no Linear issue of its own — surfaced by kurrent-io/kcap-server#2042 (AI-3115)

What & why

SecretRedactor.RedactValue and IsSecretKey exist so an out-of-process caller can scan a whole recording with the production vocabulary. The generated patterns carry a 100 ms match timeout and every call a one-second record budget, so those entries throw on any multi-megabyte value: a 6M-char tool result trips the env-var pattern on an M-series Mac, and 400K-char ones trip it on a loaded CI runner. The vocabulary now lives in SecretPatterns, built twice: the generated set keeps the watcher's deadline for RedactLine, and the same patterns rebuilt under a 30 s per-call deadline serve the out-of-process entries with an unlimited record budget. That deadline is a safety net, not a budget: the env-var scan costs about 27 ns per char, so 16M chars take under half a second, while the JSON-key pattern is quadratic on a quote-less run of keywords (2K keywords 1.4 ms, 4K 5.7 ms, 8K 21 ms) and would otherwise never return.

Where to look

RedactLine is unchanged, including its regex_timeout loss marker. Its 100 ms deadline is tight for the 4 MB records that path admits: a delimiter-rich record above roughly 3.5 MB is loss-marked on fast hardware and smaller ones on a loaded laptop. Left as is here.

Verification

  • RedactValue_ScansAMultiMegabyteValue_WithoutTheWatcherDeadline fails on main with RegexMatchTimeoutException after ~220 ms and passes here; ASuperLinearScan_StillMeetsItsDeadline_OnARebuiltSet pins that a rebuilt set still ends the quadratic case.
  • CI build and test green on ubuntu, macOS and Windows; both AOT publish checks green. Locally, Capacitor.Cli.Tests.Unit is 4,777 total, 0 failed, 21 skipped, and a local dotnet publish -c Release (osx-arm64) built the native binary with no IL2026/IL3050 warnings.
  • kcap-server FixtureIntegrityTests with src/cli at this branch: 65 passed; the previous pin failed five Committed_fixtures_are_clean cases in CI and the huge-bodies ones locally.

🤖 Generated with Claude Code

A generated regex bakes its match timeout into the instance, so the
out-of-process set is the same patterns rebuilt without one, and the
record budget those callers were handed had no loop to protect.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-24T16:53:03.379630Z 90a8621 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Remove deadlines from out-of-process secret scans

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Prevent out-of-process redaction from timing out on multi-megabyte recording values.
• Share one secret vocabulary across bounded watcher and unbounded recording scans.
• Add regression coverage for unlimited budgets and timeout-free pattern parity.
Diagram

graph TD
    W["Watcher Loop"] -->|RedactLine| R["SecretRedactor"] -->|timed matches| B["Bounded Patterns"] -->|checks| D["One Second Budget"]
    S["Recording Scanner"] -->|value APIs| R -->|unbounded matches| U["Unbounded Patterns"] -->|checks| I["Unlimited Budget"]
    B -->|rebuilds regex text| U
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Generate duplicate timeout-free regexes
  • ➕ Preserves source-generated execution for both pattern sets
  • ➕ Avoids runtime regex construction and compilation
  • ➖ Duplicates twelve regex declarations and their options
  • ➖ Creates vocabulary drift risk despite parity tests
  • ➖ Increases maintenance cost for every pattern change

Recommendation: Keep the PR's shared-vocabulary approach. Lazily rebuilding the bounded patterns with infinite timeouts cleanly separates watcher responsiveness from whole-recording completeness, while parity tests prevent semantic drift; duplicate generated declarations would add substantial maintenance risk.

Files changed (5) +190 / -106

Bug fix (3) +152 / -106
RedactionBudget.csSupport configurable and unlimited redaction budgets +7/-3

Support configurable and unlimited redaction budgets

• Makes the elapsed-time limit configurable while preserving the existing one-second default. Adds a shared unlimited budget for whole-recording scans.

src/Capacitor.Cli/Capture/RedactionBudget.cs

SecretPatterns.csCentralize secret pattern execution and timeout cloning +119/-0

Centralize secret pattern execution and timeout cloning

• Introduces a reusable container for the complete secret-detection vocabulary and redaction pipeline. It can rebuild generated regexes with infinite match timeouts while retaining matching options, delimiter gates, and replacement behavior.

src/Capacitor.Cli/SecretPatterns.cs

SecretRedactor.csRoute watcher and out-of-process scans through separate deadlines +26/-103

Route watcher and out-of-process scans through separate deadlines

• Keeps watcher line redaction on generated, timeout-bound patterns and its one-second record budget. Routes IsSecretKey and RedactValue through a lazy timeout-free pattern set with an unlimited budget.

src/Capacitor.Cli/SecretRedactor.cs

Tests (2) +38 / -0
RedactionBudgetTests.csVerify unlimited budgets do not expire +8/-0

Verify unlimited budgets do not expire

• Adds coverage proving a maximum-duration redaction budget remains valid after a simulated year.

test/Capacitor.Cli.Tests.Unit/Capture/RedactionBudgetTests.cs

SecretRedactorTests.csCover large scans and pattern-set parity +30/-0

Cover large scans and pattern-set parity

• Adds a multi-megabyte value regression test that exercises every delimiter gate before redacting a trailing secret. Verifies the bounded and unbounded sets retain identical patterns and options while differing only in timeout behavior.

test/Capacitor.Cli.Tests.Unit/SecretRedactorTests.cs

@qodo-code-review

qodo-code-review Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Malformed records can expose secrets ✗ Dismissed 📘 Rule violation ≡ Correctness
Description
RedactLineWithOutcome catches JsonException and runs Bounded.RedactSecrets over the complete
rawJsonlLine instead of decoded values. When malformed input places a secret object or array
beneath a sensitive key, the textual key regex can miss its contents and direct RedactLine callers
receive the fallback line.
Code

src/Capacitor.Cli/SecretRedactor.cs[59]

+                line = Bounded.RedactSecrets(rawJsonlLine, budget);
Relevance

●●● Strong

Exact same-file precedent accepted: malformed redaction must emit a fixed safe placeholder, not
regex-process raw input.

PR-#1138

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Rule 2943475 prohibits applying regex replacement to a complete JSON line without decoded-value
processing, while rule 2943490 requires rejected redaction input to produce a fixed placeholder. The
changed catch branch applies the pattern pipeline directly to rawJsonlLine, marks the result as
malformed, and the public RedactLine wrapper returns the outcome's line directly.

Rule 2943475: Redact secrets only on decoded JSON values, not on raw serialized lines
Rule 2943490: Redaction writer must never emit unredacted rejected lines
src/Capacitor.Cli/SecretRedactor.cs[42-65]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Malformed JSON currently falls back to regex replacement over the complete serialized line, which can miss secrets that require decoded key and value context and can return the rejected line through `RedactLine`.

## Fix Focus Areas
- src/Capacitor.Cli/SecretRedactor.cs[56-60]

## Recommended Fix
Return the fixed malformed-input loss placeholder when JSON parsing fails instead of applying `RedactSecrets` to `rawJsonlLine`. If plain non-JSON records must remain supported, classify them before the JSON redaction path and handle them through a separate API that cannot be mistaken for successfully decoded JSON.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
  Explored: repo: kurrent-io/kcap-server (branch: alexeyzimarev/ai-3115-capture-fix, sha: 03cf9b63) — View relationship
Review mode: ⚖️ Balanced: This changes security-sensitive secret-redaction behavior, regex timeout semantics, and shared pattern construction across multiple code paths, warranting a careful single-pass review.

Grey Divider

Tip of the day
💡 Did you know, you can choose which labels appear on a finding, and whether they show icons or text

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Capacitor.Cli/SecretRedactor.cs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 90a86218c8

ℹ️ 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".

Comment thread src/Capacitor.Cli/SecretPatterns.cs Outdated
// Compiled is ignored under NativeAOT, where this set is never built; elsewhere the interpreter
// would scan a recording several times slower than the generated code does.
static Regex WithoutMatchTimeout(Regex generated) =>
new(generated.ToString(), generated.Options | RegexOptions.Compiled, Regex.InfiniteMatchTimeout);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep a guard on backtracking patterns

For an out-of-process value consisting of an opening quote followed by many repetitions of secret and then :\n without a closing quote, the delimiter gate admits JsonKeySecretRegex, whose adjacent greedy character-class repetitions retry partitions at every keyword occurrence and exhibit quadratic runtime. Rebuilding every pattern with Regex.InfiniteMatchTimeout allows a sufficiently large, untrusted recording value to pin the scanner indefinitely instead of terminating after the former 100 ms guard; retain a safety timeout or make these patterns linear/non-backtracking before removing it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and kept a safety net in b8bf49e: the rebuilt set now carries a 30 s per-call deadline (OutOfProcessMatchTimeout) instead of none. Measured on the quote-less keyword run you describe, the JSON-key pattern grows roughly 4x per doubling (2K keywords 1.4 ms, 4K 5.7 ms, 8K 21 ms), so a multi-megabyte run would take minutes; the linear scans cost about 27 ns per char, so 16M chars finish in under half a second. ASuperLinearScan_StillMeetsItsDeadline_OnARebuiltSet pins that a rebuilt set still ends that case.

A quote-less run of keywords makes the JSON-key pattern quadratic, so a
scan with no deadline could run for minutes on a few megabytes; thirty
seconds is beyond any linear scan and still ends that one.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@alexeyzimarev
alexeyzimarev merged commit 9a0e6ce into main Sep 24, 2026
8 checks passed
@alexeyzimarev
alexeyzimarev deleted the fix/out-of-process-redaction-unbounded branch September 24, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant