fix: a credential is recorded where it was meant to be and nowhere else - #66
Merged
Conversation
The password of a locked archive was in the manifest twice. Under the file's own properties, where it is written on purpose - archive.go says why beside that setting, and a locked fixture nobody can open checks nothing. And again inside run.command, which records the whole command line as it was typed. The second was a side effect. run.command is the line people copy: into a bug report, into a README, into a commit beside a set of fixtures. It reads like metadata rather than like fixture data, so it is not treated with the same care, and the two places were one accident apart. It now reads --set password=***, in both flag shapes the flag package takes. Which properties are credentials is declared where the property is declared, so a second one added later does not have to find the places that care. internal/manifest keeps its own copy of the names and a guard compares the two - that package records what a run produced and knows nothing about formats, and asking the registry from there would answer "no secrets" quietly in a process that registered none. A manifest carrying one is written 0600 rather than 0644. Every other manifest and every generated file keeps the mode it had: this tool exists to produce files somebody else's CI will read, which is why .golangci.yml turns gosec's permission rules off here. Windows has no permission bits, so nothing changes there - and that is why the guard for it is proven by hand on Linux and recorded in provenByProbe rather than by the mutation runner, which runs on Windows and would score NOT CAUGHT about a healthy guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Finding
S4of the outside security review, confirmed here before anything was written.The password of a locked archive was in the manifest twice.
properties, on purpose.archive.gosays why beside that setting: a locked fixture nobody can open checks nothing. No argument, and this PR does not touch it.run.command, which records the whole command line as typed. That one was a side effect.run.commandis the line people copy - into a bug report, into a README, into a commit beside a set of fixtures. It reads like metadata rather than like fixture data, so it is not treated with the same care, and the two places were one accident apart.What changed
The recorded command now reads
--set password=***, in both shapes the flag package takes (--set password=xand--set=password=x). The deliberate copy is untouched, and the guard asserts that too - a fix that removed both would trade a leak for an archive nothing can open.Which properties are credentials is declared where the property is declared, so a second one added later does not have to go looking for the places that care.
A manifest that carries one is written
0600rather than0644. Every other manifest and every generated file keeps the mode it had: this tool exists to produce files somebody else's CI will read, which is exactly why.golangci.ymlturns gosec's permission rules off here. Windows has no permission bits, so nothing changes there.internal/manifestkeeps its own copy of the secret names and a guard compares the two against the registry. That package records what a run produced and knows nothing about formats - asking the registry from inside it would tie the record to it and would answer "no secrets" quietly in a process that registered none.Behaviour change worth knowing
run.commandis a different string now. If you compare that field between runs, it will differ. It is inCHANGELOG.md.Guards
Three, and one of them is proven differently on purpose.
TestAManifestCarryingACredentialIsWrittenForItsOwnercannot be observed on Windows - no permission bits - and the mutation runner runs there, so it would scoreNOT CAUGHTabout a healthy guard. It was broken by hand instead:0o600changed to0o666, guard binary cross compiled for linux/amd64 and run in a container against the real repository, red naming the case while the two that must stay0644stayed green. Recorded inprovenByProbe. The half that runs everywhere has a mutation.Two other mutations, both caught, plus three existing entries repaired rather than counted after this code moved under them.
🤖 Generated with Claude Code