Repository navigation
fix(bin): scrub secrets split from stderr to stdout - #6
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: addf1d3de5
ℹ️ 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".
| if stderr.endswith(value[:split]) and stdout.startswith(value[split:]): | ||
| stderr = stderr[:-split] + replacement | ||
| stdout = stdout[len(value) - split :] |
There was a problem hiding this comment.
When another scrubber equals the cross-stream prefix—for example, TOKEN=foobar alongside OTHER_TOKEN=foo—the preceding scrub_output calls replace foo before this condition runs. The condition can no longer recognize the split foobar, so stdout returns raw bar, violating the new test's intended neither-fragment guarantee and disclosing part of the selected credential; boundary matching should use the original buffers or occur before per-stream scrubbing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in c8b5d8c. Boundary matches for every secret are now collected on the original stdout and stderr (within each stream and across both boundary orders), merged per stream, and redacted in a single pass, so overlapping secrets such as TOKEN=foobar with OTHER_TOKEN=foo no longer leak the suffix. Regression tests cover the foobar/foo overlap in both stream orders, the cab/cab case, and the abcdef/cdefgh case, all with dummy values.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Profreshor
changed the title
Intent
Scrub secrets split from stderr into stdout. The Codex review of fork PR #3 found: when a child writes a credential prefix to stderr and its suffix to stdout, both fragments survive because the check in bin/fm-secrets.py (around lines 487-491) only covers the opposite stdout-prefix/stderr-suffix ordering. For example, splitting 'abcdef' as stderr 'abc' and stdout 'def' exposes every credential byte across the returned streams despite run's scrubbing guarantee. Add the symmetric boundary check and regression coverage. The captain accepted filing this as a follow-up after merging PR #3, and the fix ships as a PR on the Profreshor/firstmate fork. Follow-up decision: the boundary check for both orderings must run on the raw streams before per-stream scrubbing, so overlapping secrets such as TOKEN=foobar plus OTHER_TOKEN=foo leak nothing, with a regression test for that overlap in both orderings using dummy values.
What Changed
Risk Assessment
✅ Low: The raw-coordinate range plan collects all per-stream and both boundary-order matches before mutation, merges them with deterministic scrubber-order labels, and is bounded to the intended secret-redaction behavior.
Testing
The focused behavior suite passed, and live CLI runs spawned real child commands and captured both returned streams for every requested boundary/overlap case; the transcript shows intact redaction markers with no synthetic credential fragments. The change diff was also whitespace-clean.
<redacted:TOKEN>on both returned streams.<redacted:TOKEN>markers on both streams after each raw direction is considered.ghsuffix.Evidence: Live fm-secrets stream-scrubbing transcript
Source: Live fm-secrets stream-scrubbing transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (3) ✅
bin/fm-secrets.py:491- The required “both orderings must run on the raw streams” invariant is still bypassable: if a child writescabto each stream forTOKEN=abc, the stdout→stderr match replaces stdout’s trailingaband consumes stderr’s leadingc, thenbreakprevents the stderr→stdout check. The returned stderrabplus stdoutc<redacted:TOKEN>reconstructsabc. Handle both raw boundary directions before either mutation can suppress the other, and add this executable regression.🔧 Fix applied.
1 warning still open:
bin/fm-secrets.py:507- Sequentially applying both raw-stream matches can slice into the first replacement. WithTOKEN=aaaaandaaawritten to each stream, both directions select split 1; stdout becomesaa<redacted:TOKEN>then loses its first three bytes, returning malformedredacted:TOKEN>. Apply edits using original coordinates so both replacement markers remain intact.🔧 Fix applied.
1 error still open:
bin/fm-secrets.py:493- Contradicts the required criterion that “both orderings must run on the raw streams before per-stream scrubbing, so overlapping secrets … leak nothing.” The loop mutatesstdout/stderrfor the first scrubber before locating the next scrubber's raw boundary. With env-file orderTOKEN=abcdef,OTHER_TOKEN=cdefgh, and child output stdoutabc, stderrdefgh, TOKEN's split-3 edit returns<redacted:TOKEN>andgh; OTHER_TOKEN is then no longer detectable and itsghsuffix is disclosed. Compute every scrubber's boundary matches from the original streams, then apply one original-coordinate redaction plan (with defined overlap precedence).🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
<redacted:TOKEN>on both returned streams.<redacted:TOKEN>markers on both streams after each raw direction is considered.ghsuffix.bin/fm-test-run.sh --jobs 1 tests/fm-secrets.test.shManual live invocations ofbin/fm-secrets.sh runwith separately captured stdout/stderr for symmetric, opposing-direction, and overlapping-secret casesgit diff --check 0f8cb488a5d804a4bef6c3be4d1324908f525373 adbc59c7e84cd5655adbc47c9c90d5058c3ec96f✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.