fix(redact): pattern-match secret-bearing argv flags
What does this MR do and why?
A secret-bearing argv flag that was neither one of the built-in three nor declared by its driver reached CLIDetail.Argv verbatim, and no later layer scrubbed it. argv renders to stdout, the JUnit XML and the detail view, so oras push --api-key hunter2 put the credential in a CI artifact. Every other secret-bearing output field was already covered.
This matches an argv flag name against the same five patterns RedactField applies to a field name, with leading dashes and any =value removed.
References
Closes #51 (closed)
Which reading is normative, and why
The issue asked for a decision. S04 stated this rule twice with two different sets, and this takes the five-pattern reading as normative.
It is what §Security Considerations / Field-name redaction and §Resolutions / Cross-spec consistency already promise, the latter saying the set was adopted "everywhere — CLI driver scrubbing" included. It is also what the field-name layer twenty lines away already does. A driver author reading either of those would conclude argv names were pattern-matched and skip WithSecretFlags, which is exactly the decision that produces the leak.
The three narrow passages are reworded to match: the invocation contract, §Security Considerations and AC #51 (closed). All five now describe one rule.
Option (b), keeping exact match and rewording the other two, would have left the next driver one forgotten declaration away from a leak, with nothing to catch it.
Differences
Before: RedactArgv(["push", "--api-key", "hunter2"]) returned the argv unchanged unless the driver had declared --api-key.
After: it returns ["push", "--api-key", "[REDACTED]"]. Inline form too.
Driver declarations still work and still matter for a name no pattern reaches. --cred is covered only when declared, and there is a test for each direction.
Only an argument beginning with - is pattern-matched. A positional such as my-password.txt names a file, not a flag, and consuming the argument after it would redact something that is not a credential. Also tested.
Two things a reviewer should weigh
One existing test case changes with the behaviour. undeclared driver flag is left alone asserted that --registry-password passed through unredacted when its driver had not declared it. That assertion is the leak. It now asserts the opposite.
I am aware of the rule that a test change and an implementation change do not belong in the same MR. That rule is about not weakening a conformance test to make an implementation pass. This is the inverse: a unit test of the redaction helper that pinned the vulnerable behaviour, and the change strengthens it. Splitting it out would leave main with a red suite in between. Happy to split if you read it differently.
Substring matching over-redacts. The patterns are substrings, so --token-file and --password-stdin count as secret-bearing and the argument after them is redacted too. The executor's captured-stream scrub keys on the same redacted argv, so that consumed value is also stripped from stdout and stderr, not only from the rendered command line. Over-redacting a CI artifact is the cheaper failure than leaking a credential into it.
How to set up and validate locally
-
go test -count=1 ./pkg/conformance/redact/ ./pkg/conformance/cliexec/ ./internal/report/ -
Confirm the tests catch the gap. In
secretFlag, delete:if fieldNamePattern.MatchString(strings.TrimLeft(name, "-")) { return name, inline }Ten cases fail: eight in
TestRedactArgv_DriverSecretFlags, plusapi-key inlineandapi-key separateinTestExecutorRun_ScrubsSecrets.
MR acceptance checklist
- Argv matched against the same five-pattern set as field names
- AC #51 (closed)'s fixture exercises
--api-key, outside the built-in set and across the[_-]?alternation - All five S04 passages describe one rule
- Tests added, verified to fail without the pattern match
- Driver declarations still honoured, with a test both ways
- Conforms to the code review guidelines