Skip to content

Commit 8a1c6a9

Browse files
donislawdevclaude
andcommitted
test: every registered preset renders a recipe that parses back
Finding S8 of the outside security review: the preset renderer builds its source by writing strings into a template, and what keeps that safe is a whitelist of the characters a size is written with - added after fuzzing found the hole in the first place. The review's point was that the whitelist has to be remembered by whoever writes the next preset. The review's remedy was to compose through recipe.Compose. Measured with tools/probes/composeeject and turned down: the source comes out 627 B rather than 965 B, the comment header saying where the file came from disappears, and every number is quoted because the draft type holds strings. internal/cli/preset.go records recipe.Hash of exactly those bytes as recipe_hash in the manifest - checked on a real run - so that is a breaking change in other people's records rather than a tidy. The property the whitelist provides is asked for directly instead: whatever a preset is given, the source it produces is either refused with a sentence or parses back into a recipe with targets. FuzzPresetExpansion already asks that, well, and asks it of "size-boundaries" by name - so a second preset would arrive uncovered. This asks it of every registered preset and every parameter it declares, with fourteen hostile values, and it is deterministic rather than fuzzed because a fuzz target runs where somebody runs it. One mutation: taking the whitelist off turns it red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent c096ec9 commit 8a1c6a9

1 file changed

Lines changed: 129 additions & 0 deletions

File tree

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
package guard
2+
3+
import (
4+
"strings"
5+
"testing"
6+
7+
_ "github.com/donislawdev/TestingFilesGenerator/internal/format/all"
8+
"github.com/donislawdev/TestingFilesGenerator/internal/preset"
9+
"github.com/donislawdev/TestingFilesGenerator/internal/recipe"
10+
)
11+
12+
// hostileParameterValues are values that would break a document written by
13+
// pasting strings together.
14+
//
15+
// The first of them is not invented. Fuzzing on 2026-08-05 produced "1\rB",
16+
// which the size parser reads as one byte because it trims the ends - and the
17+
// carriage return then reached the recipe source raw and broke the document.
18+
// The comment on the renderer said at the time that no value there needed
19+
// quoting. That was true of every value except the one a caller supplies.
20+
//
21+
// The rest are the shapes that turn one line of YAML into two, or into a
22+
// comment, or into a key of their own.
23+
var hostileParameterValues = []string{
24+
"1\rB",
25+
"1kb\n id: injected",
26+
"1kb#comment",
27+
"1kb: value",
28+
"1kb\ttab",
29+
`1kb"quote`,
30+
"1kb'quote",
31+
"- 1kb",
32+
"1kb ",
33+
" 1kb",
34+
"[1kb]",
35+
"{1kb}",
36+
"1kb\\",
37+
"1kb\x00",
38+
}
39+
40+
// Every preset renders a recipe that means what it says, whatever it is given.
41+
//
42+
// internal/preset builds its source by writing strings into a template, which an
43+
// outside review on 2026-09-05 raised as finding S8: nothing in the shape of
44+
// that code stops a value from carrying a newline or a colon into the document.
45+
// What keeps it safe today is firstUnusable, a whitelist of the characters a
46+
// size is written with, and the review's point was that the whitelist has to be
47+
// remembered by whoever adds the next preset.
48+
//
49+
// The review's remedy - composing through recipe.Compose, which lets the
50+
// encoder quote - was measured and turned down. It is in
51+
// docs/SECURITY-REVIEW-2026-09-06.md section 13 with the numbers: the source
52+
// comes out 627 B rather than 965 B, and internal/cli/preset.go records
53+
// recipe.Hash of exactly those bytes as recipe_hash in the manifest. Changing
54+
// the renderer changes that hash for everybody who runs a preset, which is a
55+
// promise about other people's records rather than a tidy.
56+
//
57+
// So the guarantee is held by asking rather than by rewriting: whatever any
58+
// preset is given, the source it produces either is refused with a sentence or
59+
// parses back into a recipe with targets. That is the property the whitelist
60+
// exists to provide, and it is asked of every REGISTERED preset rather than of
61+
// the one that exists today - FuzzPresetExpansion asks it well and asks it of
62+
// "size-boundaries" by name, so a second preset would arrive uncovered.
63+
//
64+
// Deterministic rather than fuzzed on purpose. A fuzz target runs where somebody
65+
// runs it, and this runs on every push.
66+
func TestEveryPresetRendersARecipeThatParsesBack(t *testing.T) {
67+
presets := preset.All()
68+
if len(presets) == 0 {
69+
t.Fatal("no preset is registered, so this guard checked nothing")
70+
}
71+
72+
checked := 0
73+
for _, p := range presets {
74+
for _, param := range p.Parameters {
75+
for _, hostile := range hostileParameterValues {
76+
checked++
77+
args := preset.Args{param.Name: hostile}
78+
expanded, err := preset.Expand(p.ID, args)
79+
if err != nil {
80+
// Refusing a value the declaration does not allow is an
81+
// answer, and it has to carry a sentence somebody can act
82+
// on.
83+
if strings.TrimSpace(err.Error()) == "" {
84+
t.Errorf("%s refused %s=%q with an empty message", p.ID, param.Name, hostile)
85+
}
86+
continue
87+
}
88+
assertParsesBack(t, p.ID, param.Name, hostile, expanded.Source)
89+
}
90+
}
91+
}
92+
if checked == 0 {
93+
t.Fatal("no preset declares a parameter, so nothing hostile was tried")
94+
}
95+
t.Logf("%d preset(s), %d value(s) tried", len(presets), checked)
96+
}
97+
98+
// assertParsesBack is the whole property: source a preset accepted has to be a
99+
// recipe, and a recipe with something in it.
100+
//
101+
// Parsed rather than pattern matched, because what a broken document does is
102+
// mean something ELSE - an injected key is perfectly good YAML - and only the
103+
// parser can say what it came to.
104+
func assertParsesBack(t *testing.T, id, param, value string, src []byte) {
105+
t.Helper()
106+
107+
rec, err := recipe.Parse(src, "preset-"+id)
108+
if err != nil {
109+
t.Errorf("%s accepted %s=%q and produced source the parser refuses: %v\n--- source ---\n%s",
110+
id, param, value, err, src)
111+
return
112+
}
113+
if len(rec.Targets) == 0 {
114+
t.Errorf("%s accepted %s=%q and produced a recipe with no targets:\n%s", id, param, value, src)
115+
return
116+
}
117+
// A value that reached the document as structure would show up here as a
118+
// target nobody asked for, or as one whose id is not the one the preset
119+
// builds.
120+
for _, target := range rec.Targets {
121+
if strings.TrimSpace(target.ID) == "" {
122+
t.Errorf("%s accepted %s=%q and produced a target with no id:\n%s", id, param, value, src)
123+
}
124+
if target.Format == "" {
125+
t.Errorf("%s accepted %s=%q and produced target %q with no format:\n%s",
126+
id, param, value, target.ID, src)
127+
}
128+
}
129+
}

0 commit comments

Comments
 (0)