Scrub a short secret, and both spellings of a proxy password

[artyomsv]

Closes the two security findings PR #106's review raised and dismissed with reasons. Both were held
back deliberately: they change what a credential's forms are, on paths #106 did not otherwise
touch, and bolting that onto a debt round would have made a product decision by accident.

Both are about the lifetime of a credential's forms rather than the scrubber's arithmetic — which
is why the round that built the shared scrubber did not see either.

1 — A short secret was silently not scrubbed

The shared scrubber skipped anything under eight characters, and I documented that as a deliberate
trade: redacting a short string turns every innocent occurrence of it into the marker and leaves an
operator a failure detail they cannot read.

The reasoning about readability is right. The trade is wrong at the point of use. At that instant
the value is a live credential and the cost of leaking it is unbounded, while noise in a message is
recoverable.

It was not hypothetical. Gitea and Forgejo accept an account password for git-over-HTTP with a
default minimum of six characters, nothing validates the length of a factory token an operator
pastes into the registry, and such a password reached factory_run.failure_detail verbatim — written
by the container that holds the git write token.

The floor no longer gates the scrub. It survives as MIN_PLAUSIBLE_SECRET_LENGTH, and a secret below
it is scrubbed and logged once, naming the consequence. A warning an operator can read beats a
silence they cannot. The log line carries the length and never the value.

Two things deliberately not done

No refusal at registration. "A credential must be eight characters" is a new rule about what an
operator may configure. It would block a working Gitea deployment, and inventing it under cover of a
logging fix is how a product decision gets made by accident.

The proxy password's startup refusal is removed, not kept. Its entire stated premise was
"SecretScrub ignores it" — a refusal whose reason has become false is worse than no refusal,
because the next reader trusts the reason.

2 — The proxy password was scrubbed in a spelling the container does not carry

Measured rather than argued. An operator writes p%2fss%20x; credentialIn decoded that to
p/ss x; and SecretScrub derives its URL-encoded form with URLEncoder, which emits uppercase
hex and + for a space — p%2Fss+x. The value actually set in every container is the raw URL, which
contains neither.

That matters because env and printenv are routine agent actions, and their output lands in
run_event, which a viewer can read.

Both spellings are collected now: raw first, because that is the environment variable; decoded
second, because that is what a Proxy-Authorization: Basic header and a verbose curl print.
distinct() collapses them when the password carries no escapes, so the common case costs nothing.

Four tests pinned the behaviour being reversed

Rewritten rather than deleted, each saying what changed and why — two of them written by the round
that introduced the floor.

The publisher's floor test is the interesting one. With the floor gone, nothing in that file
separates the shared class from a hypothetical local copy — and that is the correct state rather
than a gap. The two now agree on every path that class can reach, since it holds exactly one
credential, so ordering is unreachable. What guarantees the publisher reaches the shared class is
structural: its own copy is deleted and it has no other scrub.

Verification

Three mutations, each killed by exactly the intended test: restoring the length floor, dropping the
second spelling, and returning only the decoded form.

302 suites, 2603 tests, 0 failures (testFast + testServices).

---

Review round

Four lenses. One HIGH, found by three of them independently — and it is a defect this branch
introduced. Not in what was added, in what was deleted.

The HIGH

requireScrubbableProxyPasswords was, unnoticed, the only startup caller of
proxyCredentials(). Its stated premise was a length rule; that premise had become false, so
it was deleted on that basis. The call had a second job nobody had named: URLDecoder throws on
a bare % — 100%secure, pa%ss — and a bare % is a legal password character an
operator writes. That throw used to happen at boot.

Afterwards the first caller is on the run-launch path:

1. RunLauncher builds the transcript scrub after runtime.create(unit) has put the model
   key and the git write token into three containers. It throws.
2. RunDispatcher's catch calls failures.of(...), which re-enters the same code and throws a
   second time.
3. That escapes before the finally that calls registry.forget. The dispatcher's own comment
   states the consequence: isExecuting() is the watchdog's one absolute exemption, so the
   sandbox is permanently unreclaimable.

One mistyped character in a proxy URL, every run, deterministically.

decode never throws now. Returning the value unchanged is the truthful answer as well as the
safe one — a % the operator did not mean as an escape means the value already is its own
decoded form. The exception is deliberately not logged: its message quotes two characters of the
password.

The lesson is worth more than the fix. Deleting a control because its stated reason has
become false is only safe once you know what else the control was doing. The reason was
documentation; the call was behaviour. One grep for callers would have shown it.

Four more in the production path

• scrubFor's proxy call was unguarded while the two decrypt calls above it were, so one bad
  proxy value disarmed the SCM and harness scrub too — the opposite of the per-credential
  degradation that method documents.
• The write token was paired with the READ username. Credentials.Scm carries a
  writeUsername precisely "so the call sites are already correct when a deployment issues
  two", and the comment ten lines below in the same method states the rule the code broke. The
  identical defect this branch fixes for the proxy credential, two credentials apart, in one method.
• URLDecoder is a FORM decoder and turns + into a space, which no URI userinfo means by
  it. A password a+b yielded a "decoded" form a b that appears on no wire while the real
  header form went uncovered.
• The registry credential reached no scrub. Defence in depth; closed for one entry.

Two tests were weaker than they looked

• The WARN was asserted by nothing. It is the entire stated compensation for removing the
  floor, and in a class whose purpose is keeping credentials out of logs, adding the secret to its
  arguments would have passed every test in the repository. Now asserted to fire, to carry the
  length, and to carry neither the value nor the username — with a negative control.
• bothSpellingsOfOneProxyPasswordAreScrubbed could not see a decode-only regression.
  URLEncoder re-encodes p@ss back to the operator's own p%40ss — no hex letter, so no
  case to differ on — so the fixture cancelled out the very mechanism it was written to prove.
  Measured: it survived that mutation. %2f removes the coincidence.

Both new proxy tests would also have passed a scrub that redacted everything; the host must
survive, or an operator gets a failure that names nothing.

One guard that was cited but never written

SecretScrubHasOneHomeTest did not exist, while SecretScrub's javadoc cited the guard as
precedent and OutcomeWriterTest rested on "structural, not an assertion". Worse: the moment
the two implementations stopped differing was the moment a re-added local copy stopped failing any
test. Written, allowlist empty, mutation-verified against a re-added copy.

An old record that already knew

The M1 task-1 review record's [sec-L2] had named all three of the read-username pairing, the
length floor and the form-encoding — as one Open entry, correctly noting they were "latent
while a deployment carries one alphanumeric token". It sat there while two later rounds found them
one at a time. An Open entry naming several defects is not a single item, and the half that
goes latent last is the one nobody re-reads.

Documentation

Three documents still described the deleted rule, two of them operator-facing: .env.example and
CORPORATE-ENVIRONMENT.md both promised a startup refusal, and MODULES.md documented the floor
in full and named a constant that no longer exists. Also "logged once", which was never true — a
scrub is built per launch and again per failure.

Four findings are dismissed with reasons rather than fixed. Full dispositions:
.claude/reviews/global/credential-scrub-forms.md.