Security: broaden diagnostic bundle redaction checks#99
Conversation
athena-omt
left a comment
There was a problem hiding this comment.
The redaction broadened correctly in the obvious cases, but the new keyword detector is too coarse and will now fail closed on legitimate diagnostic prose.
rust/src/bundle.rs:419-432useslower.contains(...)forpassword,secret,token,bearer,pin, etc. That means normal bundle text like “OAuth token flow”, “secret sharing”, or “pin the version” will abort bundle creation even when no credential is present.- This is especially risky because the bundle already includes free-text fields and guidance strings, so the heuristic now has a high false-positive surface without any boundary or context check.
- The added tests cover secret-shaped strings, but they do not protect against these ordinary-language regressions.
I’d want the keyword branch tightened to something like field-aware or token-boundary matching, or converted into a scoring/structured rule that only fires on credential-like contexts rather than any substring match.
Summary: broadening is good, but the current keyword substring rule is too blunt for a diagnostic bundle path.
athena-omt
left a comment
There was a problem hiding this comment.
Review summary: the secret-shape broadening is directionally right, but the current keyword branch still fails closed on ordinary prose, so this isn’t ready to merge yet.
rust/src/bundle.rs:419-432still does raw substring matching forpassword,secret,token,bearer,pin, and friends. That means benign bundle text like “OAuth token flow”, “secret sharing”, or “pin the version” will abort bundle creation even though no credential is present. This is the same false-positive class as the prior review, and the new tests don’t constrain it.rust/src/bundle.rs:631-649adds “normal sentence” coverage, but none of those assertions exercise the actual failure mode created bycontains(...)on common diagnostic prose containing those keywords.
Blockers:
- The keyword detector is still too coarse for a diagnostic-bundle path.
- The regression suite does not protect against ordinary-language false positives from the new keyword branch.
athena-omt
left a comment
There was a problem hiding this comment.
I could not complete the required review setup for this PR. The worker path was prepared and the PR head was fetched into /openclaw-data/src/_worktrees/athena/omt-global/apw-cli/pr-99, but the required gitcrawl sync OMT-Global/apw-cli --numbers 99 --include-comments --with pr-details --json step failed with missing GitHub token: set GITHUB_TOKEN or authenticate gh.
Per the review workflow, I am stopping here rather than approving or requesting changes from incomplete/stale GitHub context. Once Athena's GitHub auth is restored, this PR should be re-reviewed from the synced PR context.
athena-omt
left a comment
There was a problem hiding this comment.
The old keyword-substring false-positive issue appears materially addressed: the matcher now uses boundary/payload checks and adds regression coverage for OAuth token flow, secret sharing, and pin the version. However, there is a new security blocker.
rust/src/bundle.rs:319returnsfalseimmediately whenlooks_like_safe_diagnostic_text()matches, before the demo-password sentinel, vendor prefixes, keyword payload checks, entropy checks, and short-secret checks run. Because that safe-text helper accepts broad substrings such asrequires(rust/src/bundle.rs:461-473) and any path-like string (rust/src/bundle.rs:529-538), a diagnostic value likerequires password=CorrectHorseBatteryStapleor/tmp/sk-abc123can skip all later credential detection. For a fail-closed diagnostic bundle redaction path, the safe-text carve-outs need to be narrower or applied only after proving the value does not contain a secret-shaped payload.
I checked branch freshness and status: head 586f4fa0336aa09ae1c025191f3b246213708618 is current against main, CI is green, and auto-merge is already enabled. I attempted targeted Cargo validation with an isolated target dir, but the worker command timed out during dependency build; I stopped the leftover Cargo processes.
4b46144 to
099a894
Compare
099a894 to
0084512
Compare
athena-omt
left a comment
There was a problem hiding this comment.
Reviewed current head 0084512 from the Athena node. Branch is current with main and GitHub CI is green. The follow-up commits address my prior redaction blocker by preventing broad safe-diagnostic text from bypassing embedded secret-shaped payload checks, with regressions for mixed prose/password and path/vendor-prefix cases. Local git diff --check passed; no remaining blocker found.
Summary
Tighten
looks_secret_like()inrust/src/bundle.rsso diagnostic-bundle redaction catches more credential shapes that were previously missed:Validation
cargo teston the worker image becausecargois not installed on PATHMerge Automation
Auto-merge not enabled yet; leaving for maintainer review because this is a security-sensitive heuristic change.