fix(redactors): assert embedded temp paths stay in their temp directory

[rustic]

Addresses CodeQL alert 3820 (go/zipslip, high, 19 sinks). Base is main.

The alert is a false positive

The flow it reports is real — a zip entry name does reach the writes that materialize an embedded part — but an escape was already impossible:

• The entry name never becomes a filename. inPath/outPath use the literals "embedded" and "redacted". Only the extension comes from the archive.
• embedded.SafeExt is a character allowlist, not a filter: a dot plus 1–10 bytes from [a-z0-9], else the .bin fallback. .., separators, NUL and drive letters are unrepresentable in the result rather than stripped from it.
• TestSafeExtNeverYieldsAPathComponent already asserts that against 21 hostile entry names (../../../../etc/passwd, ..\..\..\evil.png, NUL injection, strings.Repeat("../", 512), absolute and Windows drive paths).

All 19 sinks are downstream of that one construction — the image/office/legacyole/pdf/plaintext sinks are os.Open/os.Create/os.ReadFile on the path handed to them, not a path re-derived from an entry name. It's one flow, not 19 defects.

CodeQL misses it because go/zipslip models a cleaned-prefix containment test as a barrier and does not model a per-character allowlist as one.

So why change anything

The refactor. The safety of those two writes currently rests on a basename that happens to be a literal three lines above them. Someone deriving the basename from the entry name — to keep part names recognizable in a debug dump, say — reintroduces the traversal, and nothing local to the write says otherwise. A traversal here writes a document's unredacted bytes to an attacker-chosen path, which is worse than the leak this package exists to prevent.

Alert hygiene. 19 open high-severity alerts is a pile a reviewer learns to skip, and the next genuine path-traversal finding lands in it. Clearing the class is worth three lines.

I can't promise CodeQL retires the alert — that's its call, not something the commit controls. I've dismissed it as a false positive with this reasoning regardless, so the outcome doesn't depend on the query's barrier modelling.

What's here

withinDir cleans both sides before comparing, so ./.. elements resolve rather than compare literally, and appends the separator to the directory so a sibling sharing its name prefix isn't mistaken for a child — /tmp/ferret-embedded-1-evil vs /tmp/ferret-embedded-1 is what a naive HasPrefix gets wrong. Mutation-tested: dropping the separator fails the test.

Also corrects a comment in two places claiming SafeExt "returns one of a fixed set of .xyz literals". It doesn't — it returns any charset-validated [a-z0-9]{1,10} extension, or .bin. Same security property, but the wording overstated the mechanism, and that comment's whole job is telling the next reader why the sink is safe.

Tests

TestHostilePartNamesStayInTheTempDir drives RedactEmbedded with nine hostile entry names — relative and absolute traversal, Windows separators, NUL injection, 256 stacked ../ — against canary files planted in os.TempDir() and its parent, asserting they're byte-identical afterwards. Through the public entry point, so it covers the composition (allowlist + hardcoded basename + containment) rather than one link.

TestWithinDirRejectsEscapes covers the predicate directly. TestSafeExtStillGuardsTheExtension pins that the allowlist stays the primary control and the new check hasn't quietly become the only one.

go test ./... (66 packages), go vet ./..., make build — clean. No behavior change on any non-hostile input: the check cannot fire on today's code, which is the point.

[github-actions]

🔍 Ferret Scan Results

✅ No sensitive data detected in the changed files.

---

🤖 This comment was automatically generated by Ferret Scan