orchestrate: redact the whole value in redactBody, not up to the first space - #242
Open
pucedoteth wants to merge 1 commit into
Open
orchestrate: redact the whole value in redactBody, not up to the first space#242pucedoteth wants to merge 1 commit into
pucedoteth wants to merge 1 commit into
Conversation
…t space The sensitive-assignment pattern matched `\S+` after the separator, so it consumed only the auth scheme in `Authorization: Bearer <jwt>` and only the first word of a quoted value. `redactBody` returned "Authorization=[redacted] <jwt>" with reasons reporting a successful redaction. Match to end of line instead, and cover the branch with tests -- it had none.
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.
What
redactBody's sensitive-assignment pattern matched\S+after the:/=, so it stopped at the first space:In both cases the token is the part after the space, so the function redacted the scheme (
Bearer) or the first word and left the credential in the returnedtext— whilereasonsreported a successful redaction.Matching to end of line instead makes the returned text actually redacted:
Why
Authorization: Bearer <token>is the usual shape an auth header takes in the error output and logs that get pasted into a comment body, and it is the one shape the old pattern handled worst.To be precise about impact: today's only caller is
requireSafeCommentBodyincli/comments.ts, which throws wheneverreasonsis non-empty, so nothing currently reaches Slack with a leaked credential. The defect is inredactBody's own contract — it returns atextthat is presented as redacted and isn't — so any caller that usesresult.text, which is the obvious reading of the return shape, leaks. The branch also had no test coverage at all: the existing suite covered paths, SHAs, log dumps and size, but never the sensitive-key path..does not match newlines, so redaction still stops at the end of the offending line; there's a test for that.How
One character class in
SENSITIVE_ASSIGNMENT_RE(\S+→\S.*), plus three tests. The two credential tests fail onmain:Full suite: 212 pass / 0 fail across 28 files.
tsc --noEmitandbiome checkclean.I left
orchestrate/.cursor-plugin/plugin.jsonat1.1.0since version bumps look like a deliberate maintainer step in this repo — happy to bump it if you'd like it in the same PR.Disclosure: written with AI assistance; the analysis, fix and tests were verified locally.
Note
Medium Risk
Touches secret-redaction logic used for comment bodies. The change is a small regex plus tests; current callers reject on reasons so this mainly closes a leak if
result.textis used.Overview
Fixes
redactBodyso sensitive assignments redact through end of line instead of stopping at the first space.The old
\S+match left JWTs afterAuthorization: Bearerand quoted secrets inresult.textwhile still reporting"contains sensitive key". Matching\S.*now yieldsKEY=[redacted]for those shapes, still line-scoped. Adds tests for Bearer tokens, quoted values, and neighboring lines.Reviewed by Cursor Bugbot for commit 0e213ca. Bugbot is set up for automated code reviews on this repo. Configure here.