Closes #98.
Why
Two pieces of untrusted text decide where a file is written and what a browser executes:
- Path validators:
cleanPathturns a request path into a path in a published tree;ValidateArtifactPathandValidatePagesPathcheck a workflow’s glob and site directory before either reaches a checkout. server/web/markdown.gois the boundary where a pushed body becomes HTML that a page trusts, via goldmark and bluemonday.
Neither was fuzzed, and both are string handling, which fuzzing does better than reading.
What changed
| Target | Property |
|---|---|
FuzzRenderMarkdown |
The rendered HTML, parsed, has no script-shaped element, no on* attribute, and no scheme outside http, https, mailto — checked for a body with no tree place and for a README whose links are rewritten |
FuzzCleanPath |
An accepted path is relative, holds no separator the tree cannot take, has no dot or empty segment, resolves to a place under the site root, and cleaning it again does not move it |
FuzzValidateArtifactPath |
An accepted glob is relative, has no .. segment, and is one path.Match can compile |
FuzzValidatePagesPath |
An accepted directory is relative, has no .. segment, carries none of the refused characters, and joined to the checkout stays inside it |
The sanitizer check parses instead of searching. That is the point of it: a
<script> tag that survives as text is a text node and harmless, while a
hand-written <script> looks identical to a substring search. Parsing asks
what the browser will do, not what the bytes contain.
The path targets state the guarantee the way the caller uses it — joined to a root, the result stays under that root — rather than restating the validator’s own conditions. If the implementation changes shape, the tests still mean something.
What it turned up
cleanPath’s comment was wrong. It claimed dot segments were refused; they are resolved first, so../../etc/passwdbecomesetc/passwd. The behaviour is safe — a tree is a map of names, not a filesystem — but the comment said something the code does not do. The comment now describes the resolution and why it is still refused unless every segment is a name, and the fuzz property pins it.- The artifact collector cannot be tricked by a bracket class.
[.][.]looks like..to a reader, andValidateArtifactPathaccepts it because/[.] [.]/holds no/../. It is harmless: the collectorWalkDirs the job tree and matches each pattern against a path it derived itself, so a pattern can only ever match something inside the root. Worth knowing before someone “fixes” the validator by adding a substring check that does nothing. - A
..in a pages directory would reach a filesystem, which is why that validator is the stricter of the two. Both are now covered.
Verification
- 60 seconds per target:
FuzzRenderMarkdown1.75 M execs (1042 corpus entries),FuzzCleanPath11.2 M,FuzzValidateArtifactPath18.5 M,FuzzValidatePagesPath18.1 M — all clean. vole workflow run verifyin the pinned image: green.go.modgainsgolang.org/x/netas a direct requirement. It was already an indirect one; the sanitizer test usesx/net/htmlto parse the output, and parsing is the whole assertion.
What this does not claim
A minute of fuzzing does not prove the sanitizer has no bypass, and I am not
claiming it does. What it gives is a corpus of 1000+ shapes that will keep being
tried, and an assertion that fails the moment the policy is loosened — including
if someone swaps bluemonday.UGCPolicy() for something more permissive, which is
the change most likely to reintroduce this class of bug.