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

  1. go test -count=1 ./pkg/conformance/redact/ ./pkg/conformance/cliexec/ ./internal/report/

  2. 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, plus api-key inline and api-key separate in TestExecutorRun_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
Edited by Sylvia Shen

Merge request reports

Loading
Loading