fix: a credential is recorded where it was meant to be and nowhere else

CHANGELOG.md

@@ -58,6 +58,26 @@ because it turns other people's test suites red.
 
 ### Security
 
+- **The password of a locked archive is no longer repeated in the recorded
+  command line.** It appeared twice in the manifest: under the file's own
+  `properties`, where it is written on purpose because a locked fixture nobody
+  can open checks nothing, and again inside `run.command`, which records the
+  whole command line as typed.
+
+  The second one 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 - and it
+  reads like metadata rather than like fixture data, so it was not treated with
+  the same care. It now reads `--set password=***`. The deliberate copy is
+  untouched, so nothing that opens these archives changes.
+
+  **If you compare `run.command` between runs, that string is different now.**
+
+  A manifest that carries a password is also written `0600` rather than `0644`,
+  so it is readable by its owner rather than by every account on the machine.
+  Every other manifest, and every generated file, keeps the mode it had - this
+  tool exists to produce files somebody else's CI will read. Windows has no
+  permission bits, so nothing changes there.
+
 - **Building the program yourself now needs the build tag, and says so if you
   leave it out.** The AVIF encoder has an assembly path that reads past the end
   of a buffer and takes the process down on some picture sizes. Every workflow

internal/cli/cli.go

@@ -12,10 +12,12 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"slices"
 	"strings"
 	"syscall"
 
 	"github.com/donislawdev/TestingFilesGenerator/internal/engine"
+	"github.com/donislawdev/TestingFilesGenerator/internal/format"
 	_ "github.com/donislawdev/TestingFilesGenerator/internal/format/all"
 	"github.com/donislawdev/TestingFilesGenerator/internal/legal"
 	"github.com/donislawdev/TestingFilesGenerator/internal/version"
@@ -279,12 +281,48 @@ func (p propertyFlag) Set(v string) error {
 func args2(args []string) []string {
 	out := make([]string, 0, len(args)+1)
 	out = append(out, "generate")
+	secrets := format.SecretProperties()
+	afterSet := false
 	for _, a := range args {
-		out = append(out, quoteArg(a))
+		out = append(out, quoteArg(withoutSecret(a, afterSet, secrets)))
+		afterSet = a == "--set" || a == "-set"
 	}
 	return out
 }
 
+// redactedValue stands in the recorded command where a credential was typed.
+const redactedValue = "***"
+
+// withoutSecret is the argument with a credential taken out of it, or the
+// argument unchanged.
+//
+// The manifest records the password of a locked archive on purpose, under that
+// file's own properties, and archive.go says why: a locked fixture whose
+// password is not written down is worth nothing. This is about the OTHER place
+// it used to appear. run.command is the line people copy - into a bug report,
+// into a README, into a commit beside a fixture set - and it read like
+// metadata rather than like fixture data, so it was not treated with the same
+// care. The two places were one accident apart.
+//
+// Both shapes are handled because the flag package takes both: "--set" with
+// "password=x" as the next argument, and "--set=password=x" as one. A single
+// dash is the same flag to that package, so it is the same flag here.
+func withoutSecret(arg string, afterSet bool, secrets []string) string {
+	prefix, rest := "", arg
+	if !afterSet {
+		var found bool
+		if prefix, rest, found = strings.Cut(arg, "="); !found || (prefix != "--set" && prefix != "-set") {
+			return arg
+		}
+		prefix += "="
+	}
+	name, _, ok := strings.Cut(rest, "=")
+	if !ok || !slices.Contains(secrets, name) {
+		return arg
+	}
+	return prefix + name + "=" + redactedValue
+}
+
 func quoteArg(a string) string {
 	if a != "" && !strings.ContainsAny(a, " \t\"\\") {
 		return a

internal/format/archive/archive.go

@@ -155,7 +155,8 @@ var axes = map[string]format.Property{
 	},
 	Password: {
 		Name: Password, Kind: format.PropertyText,
-		Shape: "the password, in plain text",
+		Secret: true,
+		Shape:  "the password, in plain text",
 		// No default, and that is the point. A box somebody types in arrives
 		// empty from a window, so leaving it alone is how "no password" is
 		// said - see the pair rule in readLock.

internal/format/format.go

@@ -166,6 +166,21 @@ type Property struct {
 	// Detail is one sentence for a person, and it is what tfg formats prints
 	// and what a window shows beside the field.
 	Detail string
+
+	// Secret marks a value that is a credential rather than a description of
+	// the file, and there is exactly one of them today: the password an
+	// archive is locked with.
+	//
+	// It does NOT mean the value is hidden. A locked fixture whose password is
+	// not written down is worth nothing, so the manifest records it on purpose
+	// and says so in that property's own Detail. What this flag decides is
+	// everything AROUND that one deliberate place: the recorded command line
+	// does not repeat it, and the manifest that carries it is written for its
+	// owner rather than for everyone on the machine.
+	//
+	// Declared here rather than known by the places that care, because a
+	// second secret property added later would otherwise have to find them.
+	Secret bool
 }
 
 // JointLimit is a rule binding two settings that neither of them can state

internal/format/registry.go

@@ -118,6 +118,45 @@ func All() []Descriptor {
 	return out
 }
 
+// SecretProperties are the names of every property any registered format
+// declares as a credential, sorted.
+//
+// A name rather than a format and a name, because the places that ask are
+// looking at a value somebody typed - "--set password=..." on a command line
+// carries no format with it, and a recipe can name several. One name being
+// secret anywhere is enough for those places to treat it as secret, which errs
+// in the direction that cannot leak.
+func SecretProperties() []string {
+	mu.RLock()
+	defer mu.RUnlock()
+
+	seen := map[string]struct{}{}
+	for _, d := range registry {
+		collectSecrets(d.Properties, seen)
+	}
+	names := make([]string, 0, len(seen))
+	for name := range seen {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	return names
+}
+
+// collectSecrets adds the names of the secret properties in props to into.
+//
+// A function of its own rather than the inner loop of the one above, for the
+// reason written beside the same split in internal/recipe: together they nest
+// three deep - the loop over formats, the loop over properties, the test - and
+// the shape guard counts how many functions sit that deep as well as how deep
+// the deepest one is.
+func collectSecrets(props []Property, into map[string]struct{}) {
+	for _, p := range props {
+		if p.Secret {
+			into[p.Name] = struct{}{}
+		}
+	}
+}
+
 // IDs returns the registered format ids, sorted.
 func IDs() []string {
 	mu.RLock()

internal/guard/credentials_test.go

@@ -0,0 +1,179 @@
+package guard
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"runtime"
+	"slices"
+	"strings"
+	"testing"
+
+	"github.com/donislawdev/TestingFilesGenerator/internal/format"
+	_ "github.com/donislawdev/TestingFilesGenerator/internal/format/all"
+	"github.com/donislawdev/TestingFilesGenerator/internal/manifest"
+)
+
+// The password of a locked archive appears in the manifest once, where it was
+// meant to, and not in the line people copy.
+//
+// It is written down on purpose and that is not the finding. archive.go says
+// why in the property's own Detail: a locked fixture whose password is not
+// written down checks nothing. What it also did, until 2026-09-06, was appear a
+// second time - inside run.command, the whole command line recorded verbatim.
+//
+// The two places are one accident apart and they are read differently.
+// files[].properties is fixture data and a reader treats it as such.
+// run.command is the reproduction line: pasted into a bug report, quoted in a
+// README, committed beside a fixture set. Found by an outside review on
+// 2026-09-05, confirmed here against the build.
+//
+// Both shapes are asked because the flag package takes both, and a fix that
+// covered one would look complete.
+func TestTheRecordedCommandDoesNotRepeatACredential(t *testing.T) {
+	const secret = "hunter2SECRET"
+
+	shapes := map[string][]string{
+		"the value as its own argument": {"--set", "password=" + secret},
+		"the value joined to the flag":  {"--set=password=" + secret},
+	}
+
+	for what, setArgs := range shapes {
+		t.Run(what, func(t *testing.T) {
+			out := filepath.Join(t.TempDir(), "out")
+			args := append([]string{"generate", "--format", "zip",
+				"--set", "entries=2", "--set", "encryption=aes-256"}, setArgs...)
+			args = append(args, "--size", "64kb", "--count", "1", "--out", out)
+
+			if code, _, errOut := run(t, args...); code != 0 {
+				t.Fatalf("the run ended %d rather than 0, so this guard never reached a manifest:\n%s", code, errOut)
+			}
+
+			raw, err := os.ReadFile(filepath.Join(out, "manifest.json"))
+			if err != nil {
+				t.Fatalf("reading the manifest: %v", err)
+			}
+			var m struct {
+				Run struct {
+					Command string `json:"command"`
+				} `json:"run"`
+				Files []struct {
+					Properties map[string]any `json:"properties"`
+				} `json:"files"`
+			}
+			if err := json.Unmarshal(raw, &m); err != nil {
+				t.Fatalf("reading the manifest as JSON: %v", err)
+			}
+
+			if strings.Contains(m.Run.Command, secret) {
+				t.Errorf("run.command repeats the password:\n  %s\n"+
+					"That field is the line people copy into a bug report. The password belongs in "+
+					"the file's own properties, where a reader expects fixture data.", m.Run.Command)
+			}
+			if !strings.Contains(m.Run.Command, "password=***") {
+				t.Errorf("run.command does not show that a value was taken out:\n  %s\n"+
+					"A line with the setting missing altogether would not reproduce the run and "+
+					"would not say why.", m.Run.Command)
+			}
+
+			// The deliberate copy is still there. A fix that took the password
+			// out of both places would leave a locked archive nothing can open,
+			// which is the whole reason it is recorded.
+			if len(m.Files) == 0 {
+				t.Fatal("the manifest lists no files, so the half about the deliberate copy asks nothing")
+			}
+			if got := m.Files[0].Properties["password"]; got != secret {
+				t.Errorf("the file's own properties no longer carry the password: %v.\n"+
+					"A test that cannot open the archive cannot check anything - archive.go says so "+
+					"beside that setting.", got)
+			}
+		})
+	}
+}
+
+// A manifest carrying a credential is written for its owner rather than for
+// everyone with an account.
+//
+// The ordinary mode is 0644 on purpose and against the usual advice: this tool
+// exists to produce files somebody else's CI will read, and .golangci.yml turns
+// gosec's permission rules off here for exactly that reason. A manifest holding
+// a password is the one file that argument does not cover.
+//
+// Windows has no permission bits, so this is skipped there and says so. The
+// other half of this pair runs everywhere.
+func TestAManifestCarryingACredentialIsWrittenForItsOwner(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Windows has no permission bits - Go maps only the owner write bit onto its " +
+			"read only attribute - so there is nothing to read here. The pairing between this " +
+			"package and the registry is asked by the guard below, which runs everywhere.")
+	}
+
+	cases := []struct {
+		what  string
+		props map[string]any
+		want  os.FileMode
+	}{
+		{"a manifest with a password", map[string]any{"password": "hunter2"}, 0o600},
+		{"a manifest without one", map[string]any{"entries": 2}, 0o644},
+		// An empty value is how "no password" is said from a window, where a box
+		// somebody never typed in arrives as an empty string.
+		{"a password property that is empty", map[string]any{"password": ""}, 0o644},
+	}
+
+	for _, c := range cases {
+		t.Run(c.what, func(t *testing.T) {
+			path := filepath.Join(t.TempDir(), "manifest.json")
+			m := manifest.New("testing-files-generator", "0.0.0-test", "run_x", "tfg generate", 1, "linux", "amd64")
+			m.Add(manifest.File{ID: "files", Path: "a.zip", Name: "a.zip", Bytes: 1024, Properties: c.props})
+			if err := m.Save(path); err != nil {
+				t.Fatalf("saving: %v", err)
+			}
+			info, err := os.Stat(path)
+			if err != nil {
+				t.Fatalf("reading the mode back: %v", err)
+			}
+			if got := info.Mode().Perm(); got != c.want {
+				t.Errorf("%s came out %04o and should be %04o.\n"+
+					"A manifest holding a password is readable by every account on the machine at "+
+					"0644, and a fixture that nobody else's CI can read is a defect in the product.",
+					c.what, got, c.want)
+			}
+		})
+	}
+}
+
+// What the record calls a credential is what the registry declares as one.
+//
+// Two copies of one fact, compared - the same arrangement the licence notices
+// and the registry already have, and for the same reason. internal/manifest
+// records what a run produced and knows nothing about formats, so asking the
+// registry from there would tie the record to it and would answer "no secrets"
+// quietly in a process that registered none.
+//
+// This is the half that runs on every system, which matters because the mode
+// cannot be read on Windows and a mutation has to be provable somewhere.
+func TestTheRecordAndTheRegistryAgreeOnWhatIsACredential(t *testing.T) {
+	declared := format.SecretProperties()
+	known := manifest.SecretProperties()
+
+	if len(declared) == 0 {
+		t.Fatal("no registered format declares a secret property, so this guard compares two " +
+			"empty lists and proves nothing. archive.password is one - if it has stopped being " +
+			"declared, that is the finding.")
+	}
+
+	for _, name := range declared {
+		if !slices.Contains(known, name) {
+			t.Errorf("a format declares %q as a credential and internal/manifest does not know it.\n"+
+				"A manifest carrying it would be written 0644, readable by every account on the "+
+				"machine, and nothing would say so. Add it to secretProperties there.", name)
+		}
+	}
+	for _, name := range known {
+		if !slices.Contains(declared, name) {
+			t.Errorf("internal/manifest treats %q as a credential and no format declares it.\n"+
+				"An entry that has outlived its property will quietly cover the next setting "+
+				"given that name. Delete it, or declare Secret on the property it means.", name)
+		}
+	}
+}

internal/guard/mutationcoverage_test.go

@@ -115,7 +115,8 @@ var notProvenByMutation = map[string]bool{
 // "proven another way" are different states and lumping them together would
 // send a later session to re-prove what is already proven.
 var provenByProbe = map[string]string{
-	"TestTheEncoderSurvivesTheSizeThatCrashedItsAssembly": "proved by tools/probes/avifasm on 2026-08-29, which is the sweep that found the fault in the first place. Run with the assembly, a 640x256 picture killed the process inside cflAcMain8AVX2 at av1/cfl_amd64.s:281 with an access violation - in two runs out of three, so it turns on what the heap looks like rather than on the input alone. One size out of 240 crashed. Run with the tag this project ships, 240 out of 240 encoded and the bytes were identical either way. A probe rather than a mutation entry because what would have to be broken is a BUILD FLAG, not a line of code: the substitution that removes the tag lives in .github/build-tags, and a run without it does not fail this guard, it takes the whole test binary down with it. That is loud, and it is the honest shape for a guard against memory read outside its buffer, but it is not something the runner can score.",
+	"TestAManifestCarryingACredentialIsWrittenForItsOwner": "broken by hand on 2026-09-06 and put back, because the mutation is expressible and the OBSERVATION is not - Windows has no permission bits, Go maps only the owner write bit onto its read only attribute, and this machine is the one the mutation runner runs on. Changed internal/manifest mode() from 0o600 to 0o666, cross compiled the guard binary for linux/amd64 and ran it in a debian container against the real repository: red, naming the case - \"a manifest with a password came out 0644 and should be 0600\" - while the two cases that must stay 0644 stayed green. A probe rather than a mutation entry because a runner on Windows would score this NOT CAUGHT about a healthy guard, which is the worst answer of the three. The half of this pair that runs everywhere is TestTheRecordAndTheRegistryAgreeOnWhatIsACredential, and that one has a mutation.",
+	"TestTheEncoderSurvivesTheSizeThatCrashedItsAssembly":  "proved by tools/probes/avifasm on 2026-08-29, which is the sweep that found the fault in the first place. Run with the assembly, a 640x256 picture killed the process inside cflAcMain8AVX2 at av1/cfl_amd64.s:281 with an access violation - in two runs out of three, so it turns on what the heap looks like rather than on the input alone. One size out of 240 crashed. Run with the tag this project ships, 240 out of 240 encoded and the bytes were identical either way. A probe rather than a mutation entry because what would have to be broken is a BUILD FLAG, not a line of code: the substitution that removes the tag lives in .github/build-tags, and a run without it does not fail this guard, it takes the whole test binary down with it. That is loud, and it is the honest shape for a guard against memory read outside its buffer, but it is not something the runner can score.",
 	"TestTheIconMacOSReadsCarriesEverySizeItIsAskedFor": "broken by hand on 2026-08-28, three ways, and put back byte for byte - the file is untracked in git, so the restore was checked by hash rather than by a clean diff. " +
 		"Cutting the icp4 chunk out made it red naming that entry and the 16 px it holds, which is the failure that matters most: the file still opens, still shows an icon, and is missing the size a screen without Retina asks for. " +
 		"Resizing the 1024 px picture to 900 made it red as well, which is what an upscale from the wrong master would look like. " +