Sanitize server-controlled filenames used as output paths #9

New Issue

clawbot · 2026-08-09T03:44:48+02:00

clawbot commented

2026-08-09 03:44:48 +02:00

Problem

downloadFile (src/download/index.ts:71) falls back to file.metadata.title as the output
path when no outPath is given, and downloadThumbnail (:84) uses it with a thumb_
prefix. file.metadata.title is decrypted from server-supplied metadata; it is fully
attacker-controlled if the account is compromised or the server is hostile, and quak's entire
premise is that the server is not trusted with plaintext.

A title of ../../.ssh/authorized_keys writes outside the intended directory. bin/quak.ts
passes opts.out straight through, which is undefined unless --out was given, so
quak get <id> in a shell hits this path directly.

src/backup.ts has a sanitizePath helper for the backup tree, but the download layer does
not use it and sanitizePath itself is not applied to path separators in a way that has been
tested against traversal payloads.

src/model/decrypt.ts:100 compounds this: the decrypted metadata JSON is parsed as implicit
any and every field is read unchecked, so a non-string title propagates as-is.

Definition of done

When no explicit output path is supplied, the filename derived from server metadata is
sanitized so it can only ever name a file in the intended directory: no path separators, no
.. components, no absolute paths, no leading dots, no NUL bytes, no reserved device names.
An explicitly supplied outPath is honoured verbatim — the caller is trusted, the server
is not. This distinction is documented in a code comment.
A title that sanitizes to the empty string falls back to a deterministic safe name derived
from the file ID.
decryptFile validates the shape of the decrypted metadata JSON rather than trusting it:
a non-string title, or a missing one, produces a well-defined result instead of
propagating an arbitrary value.
Sanitization lives in one place and is shared with src/backup.ts rather than duplicated.
Tests cover, as literate documentation of the threat model: ../ traversal, absolute path,
embedded separators, NUL byte, empty title, non-string title, and a normal title passing
through unchanged.
make check green.
TODO.md updated in the same commit.

## Problem `downloadFile` (`src/download/index.ts:71`) falls back to `file.metadata.title` as the output path when no `outPath` is given, and `downloadThumbnail` (`:84`) uses it with a `thumb_` prefix. `file.metadata.title` is decrypted from server-supplied metadata; it is fully attacker-controlled if the account is compromised or the server is hostile, and quak's entire premise is that the server is not trusted with plaintext. A title of `../../.ssh/authorized_keys` writes outside the intended directory. `bin/quak.ts` passes `opts.out` straight through, which is `undefined` unless `--out` was given, so `quak get <id>` in a shell hits this path directly. `src/backup.ts` has a `sanitizePath` helper for the backup tree, but the download layer does not use it and `sanitizePath` itself is not applied to path separators in a way that has been tested against traversal payloads. `src/model/decrypt.ts:100` compounds this: the decrypted metadata JSON is parsed as implicit `any` and every field is read unchecked, so a non-string `title` propagates as-is. ## Definition of done 1. When no explicit output path is supplied, the filename derived from server metadata is sanitized so it can only ever name a file in the intended directory: no path separators, no `..` components, no absolute paths, no leading dots, no NUL bytes, no reserved device names. 2. An explicitly supplied `outPath` is honoured verbatim — the caller is trusted, the server is not. This distinction is documented in a code comment. 3. A title that sanitizes to the empty string falls back to a deterministic safe name derived from the file ID. 4. `decryptFile` validates the shape of the decrypted metadata JSON rather than trusting it: a non-string `title`, or a missing one, produces a well-defined result instead of propagating an arbitrary value. 5. Sanitization lives in one place and is shared with `src/backup.ts` rather than duplicated. 6. Tests cover, as literate documentation of the threat model: `../` traversal, absolute path, embedded separators, NUL byte, empty title, non-string title, and a normal title passing through unchanged. 7. `make check` green. 8. `TODO.md` updated in the same commit.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:44:48 +02:00

clawbot self-assigned this 2026-08-09 03:44:48 +02:00

clawbot referenced this issue

2026-08-09 03:47:27 +02:00

Tag v1.0.0 #15

clawbot referenced this issue

2026-08-09 04:12:18 +02:00

Verify secretstream TAG_FINAL and write downloads atomically (closes #1) #20

clawbot referenced this issue

2026-08-09 04:26:52 +02:00

Verify secretstream TAG_FINAL and write downloads atomically (closes #1) #20

clawbot referenced this issue