56 lines
2.9 KiB
Markdown
56 lines
2.9 KiB
Markdown
# 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:
|
|
|
|
1. **The surrogate was dropped LAST.** The final `decode("ignore")` removed a
|
|
lone surrogate after `lstrip(".")` 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.
|
|
2. **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"*17` became `….png`, an image.
|
|
3. The 16-byte extension threshold was unguarded: every test suffix was 4
|
|
bytes, so `<= 4` survived. A `.jpeg` case 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.
|