2.9 KiB
Upload names: two crashes found, then two holes in the fix
2026-09-24. Commits 92c774e, 225ba32; heid bug-hunt thread 01M3AXG27KQDMA6P3RTAAYMEHP.
What was wrong
design-dev's r3 bug hunt (hulda) found that /upload returned a 500 for a
multipart filename carrying a NUL: safe_upload_name stripped path, dots and
length but not NUL, and (dest / name).open("wb") raised ValueError. The
upload's except Exception tore the booth down and re-raised.
Fixing it turned up a second 500 on the same line: the cap was
base[:200], 200 CHARACTERS. NAME_MAX is 255 BYTES, so 200 two-byte
characters (é, or any CJK name) raised ENAMETOOLONG.
The first fix, and the two holes a single arm found in it
92c774e stripped NUL first, then applied the dot rule, then capped at 200
UTF-8 bytes via encode("utf-8", "surrogatepass")[:200].decode("utf-8", "ignore"), taking the cut out of the stem so the extension survived.
Heid's own review of that diff found nothing. hulda, reading the whole snapshot against the declared invariants, found:
- The surrogate was dropped LAST. The final
decode("ignore")removed a lone surrogate afterlstrip(".")had already run, so"\ud800.forever"came out as.forever, which is the keep marker, and"\ud800.."as... The NUL had been moved first for exactly this reason; the same discipline was not applied to the other droppable class. It was not reachable over HTTP: Starlette decodes a multipart filename strictly (utf-8, else latin-1), so it never yields a lone surrogate. The fix made the helper right by construction anyway. - A cut could manufacture a kind. A suffix too long to keep (>16 bytes)
was cut like text, and the cut could land on a shorter suffix that means
something:
"a"*196 + ".png" + "x"*17became….png, an image. - The 16-byte extension threshold was unguarded: every test suffix was 4
bytes, so
<= 4survived. A.jpegcase now pins it.
225ba32 does one pass first (NUL and everything unencodable), then basename
and the dot rule, then the byte cap. A cut whose classify/doc_kind differs
from the original's has its dots replaced with _. Falsifiers:
tests/mutations/upload_names.toml, 7/7.
The lessons
- A sanitiser drops everything droppable FIRST, then applies the structural rules. Anything dropped after a rule can defeat that rule.
- A NUL test through httpx
files=proves nothing. httpx percent-escapes the NUL, so the server sees a literal%00. Post a raw multipart body. The first integration test passed pre-fix for this reason. - Truncating by length can change a file's meaning. In the Booth the kind comes from the extension, so a cut has to preserve the kind, not only the byte count.
- The single-arm hunt earned its cost. Heid's own read traced only the hunks; hulda read the declared invariants against the whole bundle.