From 3909b1072cd563764a7464462b77bdfe8857ef4a Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 08:44:24 -0400 Subject: [PATCH] Close the RSBK name-collision class: ASCII case folding, UTF-8 well-formedness, nested-path traversal All three are format-locked and validated on encode and decode. Repeated known keys now reject at the root and inside an entry rather than last-wins. --- src/core/package/CLAUDE.md | 52 ++++++++-- src/core/package/package_format.cpp | 79 +++++++++++++-- src/core/package/package_format.h | 42 +++++--- src/core/package/package_manifest.cpp | 30 ++++-- src/core/package/package_manifest.h | 21 ++-- tests/test_bank_package.cpp | 40 +++++++- tests/test_package_format.cpp | 113 ++++++++++++++++++++- tests/test_package_manifest.cpp | 139 ++++++++++++++++++++++---- 8 files changed, 442 insertions(+), 74 deletions(-) diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index 373b512..3c37197 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -25,13 +25,22 @@ landing after the format. header (so the refusal can name the writer's semver and version) and nothing else — no manifest, no layout, no half-success. The fields through the writer semver are FROZEN for all future versions to keep that refusal producible. -- **Path expression is structurally impossible.** Entry names are bare file - names (`isValidEntryName`: no separators, no `..` component, no - drive/UNC/rooted form, no control bytes, no Windows-reserved character, no - trailing dot/space, no DOS device name), enforced on encode AND decode - because a package can arrive from anywhere. There is no field in the format - capable of expressing a path. The rule set is the full authority; see - `package_format.h`'s doc comment for the itemized list. +- **Every name and path in the format is validated on encode AND decode**, + because a package can arrive from anywhere. Three rules, all in + `package_format`, whose doc comments are the itemized authority: + - `isValidEntryName` — a payload's name is a bare file name (no separators, + no `..` component, no drive/UNC/rooted form, no control bytes, no + Windows-reserved character, no trailing dot/space, no DOS device name, + well-formed UTF-8 only). Path expression is impossible in this field. + - `sameEntryName` — two entry names differing only by ASCII case are ONE + name. Windows and macOS's default APFS would extract them onto a single + file, and a bank authored on a case-sensitive filesystem produces the pair + honestly. + - `isValidNestedSamplePath` — the nested `Sample::relativePath` IS a path by + design, and is the one field here that can express one. It refuses a `..` + component and every absolute form; `BankModel::add` checks only the latter, + so traversal would otherwise reach a future `import_plan` inside a record + the format vouched for. - **Framing only, never a payload.** `bank_package` produces header bytes and an ordered `{name, offset, length}` layout; it never holds, copies, or hashes an entry's audio. `decodePackage` proves prefix + payload lengths equal the @@ -48,8 +57,8 @@ landing after the format. - `package_format` — the contract: magic, `kPackageFormatVersion` / `kPackageMinReaderVersion`, the ladder comment, the three-way - `classifyPackageVersion` (`Readable` / `TooNew` / `Malformed`), the - entry-name rule, and `PackageHeader`. + `classifyPackageVersion` (`Readable` / `TooNew` / `Malformed`), the three + naming rules above, and `PackageHeader`. - `package_manifest` — the manifest model (`PackageEntry` / `PackageManifest`) and its JSON codec. Per entry: bare name, byte length, and a whole-file `capture::hashBytes` digest (deliberately NOT `hashWavContent`, which skips @@ -87,11 +96,36 @@ landing after the format. region" — both refuse whole and write nothing, so the safety property is unchanged, only the message. `classifyPackageVersion` and the frozen-region `TooNew` path are unaffected; this is the post-manifest-parse branch only. +- **The parse branch is the ONLY one that relabels**, deliberately: a newer + package that trips the manifest cap, a short manifest read, the layout + overflow, or the exact-size proof still reports `Malformed` even with + `formatVersion` above ours. The size proof clearly should — "install 1.9.0" + does not fix a truncated download — and the other three are indistinguishable + from ordinary corruption at the point they fail. Don't "complete" the relabel + across them for symmetry; the split is the answer, not an omission. - The format carries no algorithm tag for `byteHash` — it is FNV-1a (`capture::hashBytes`) implicitly. Changing the digest algorithm is a `minReaderVersion` bump, not additive: an old reader would otherwise compare a stored digest against bytes hashed the new way and silently misjudge corruption. +- **Obligation on the export track: sanitize, don't relay the refusal.** + `serializeManifest` returns one indistinguishable `nullopt` for every rejection + — an unrepresentable name, a case-folded collision, a traversing nested path, a + zero-length entry, a record `BankModel::add` refuses — and most of the naming + rules are Windows'. A bank ingested on macOS/Linux legitimately holds + `Hit?.wav`, `snare .wav`, or two names differing only by case, and a nested + `relativePath` is only checked for the absolute forms where it is written. + Relaying the `nullopt` makes ONE such file an unactionable total failure of the + whole export. `export_plan` must map bank entries to package + names that satisfy these rules (and disambiguate case-folded collisions) before + calling this layer; the codec's refusal is the backstop, not the user-facing + behaviour. +- **NFC/NFD normalization collisions are accepted, not solved.** macOS compares + file names normalization-insensitively, so the NFC and NFD spellings of one + accented name are two manifest entries that extract onto one file — the same + collision class as the ASCII case fold, which `sameEntryName` does catch. A + table-free fix does not exist, and restricting names to ASCII would be + genuinely over-strict for non-English users. Left open knowingly. - **Cross-module contract with `src/shell/package`:** a genuinely zero-length entry cannot round-trip through the filesystem seam there (`appendPayload` refuses an empty payload — an empty buffer signals an upstream read failure, diff --git a/src/core/package/package_format.cpp b/src/core/package/package_format.cpp index d07aaec..76808ac 100644 --- a/src/core/package/package_format.cpp +++ b/src/core/package/package_format.cpp @@ -1,6 +1,8 @@ #include "core/package/package_format.h" -#include +#include + +#include "core/util/relative_path.h" namespace reasampler::package { @@ -14,21 +16,64 @@ PackageReadability classifyPackageVersion(std::uint32_t formatVersion, namespace { +// Hand-rolled rather than std::tolower: that fold is locale-dependent, so two +// machines reading the same package could disagree on which names collide. +char lowerAscii(unsigned char c) { + return (c >= 'A' && c <= 'Z') ? static_cast(c - 'A' + 'a') : static_cast(c); +} + // Windows device names claim the whole entry regardless of extension // (CON, CON.wav, con.WAV are all the same reserved device) — checked against -// the portion before the first dot only. +// the portion before the first dot only. The trailing three pairs are the UTF-8 +// spellings of COM¹/COM²/COM³/LPT¹/LPT²/LPT³: Windows reads those ISO 8859-1 +// superscripts as digits in a device name. COM0/LPT0 are NOT reserved. bool isDosDeviceName(const std::string& name) { std::string base = name.substr(0, name.find('.')); - for (char& c : base) c = static_cast(std::toupper(static_cast(c))); + for (char& c : base) c = lowerAscii(static_cast(c)); static const std::string kReserved[] = { - "CON", "PRN", "AUX", "NUL", - "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", - "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + "con", "prn", "aux", "nul", + "com1", "com2", "com3", "com4", "com5", "com6", "com7", "com8", "com9", + "lpt1", "lpt2", "lpt3", "lpt4", "lpt5", "lpt6", "lpt7", "lpt8", "lpt9", + "com\xC2\xB9", "com\xC2\xB2", "com\xC2\xB3", + "lpt\xC2\xB9", "lpt\xC2\xB2", "lpt\xC2\xB3", }; for (const auto& r : kReserved) if (base == r) return true; return false; } +// Table-free UTF-8 well-formedness. Overlong encodings, surrogate halves and +// code points past U+10FFFF are rejected as hard as a structural length error: +// the UTF-8 -> UTF-16 conversion a host must perform maps an ill-formed +// sequence to U+FFFD unless it opts into failing, so two names differing only +// in invalid bytes would otherwise collapse onto one destination file. +bool isWellFormedUtf8(const std::string& s) { + const auto* p = reinterpret_cast(s.data()); + const std::size_t n = s.size(); + for (std::size_t i = 0; i < n;) { + const unsigned char c = p[i]; + std::size_t extra = 0; + std::uint32_t cp = 0; + if (c < 0x80) { ++i; continue; } + else if ((c & 0xE0) == 0xC0) { extra = 1; cp = c & 0x1Fu; } + else if ((c & 0xF0) == 0xE0) { extra = 2; cp = c & 0x0Fu; } + else if ((c & 0xF8) == 0xF0) { extra = 3; cp = c & 0x07u; } + else return false; // a stray continuation byte, or a 5/6-byte lead + if (i + extra >= n) return false; + for (std::size_t k = 1; k <= extra; ++k) { + const unsigned char cont = p[i + k]; + if ((cont & 0xC0) != 0x80) return false; + cp = (cp << 6) | (cont & 0x3Fu); + } + if (extra == 1 && cp < 0x80) return false; + if (extra == 2 && cp < 0x800) return false; + if (extra == 3 && cp < 0x10000) return false; + if (cp > 0x10FFFF) return false; + if (cp >= 0xD800 && cp <= 0xDFFF) return false; + i += extra + 1; + } + return true; +} + } // namespace bool isValidEntryName(const std::string& name) { @@ -46,7 +91,29 @@ bool isValidEntryName(const std::string& name) { if (c == '*' || c == '?' || c == '|' || c == '<' || c == '>' || c == '"') return false; } if (isDosDeviceName(name)) return false; + return isWellFormedUtf8(name); +} + +bool sameEntryName(const std::string& a, const std::string& b) { + if (a.size() != b.size()) return false; + for (std::size_t i = 0; i < a.size(); ++i) + if (lowerAscii(static_cast(a[i])) != + lowerAscii(static_cast(b[i]))) + return false; return true; } +bool isValidNestedSamplePath(const std::string& path) { + if (util::isAbsolutePath(path)) return false; + // Component-wise, not a substring scan: "take..final/a.wav" is a legal + // relative path, "bank/../evil.wav" is not. + for (std::size_t start = 0;; ) { + const std::size_t sep = path.find_first_of("/\\", start); + const std::size_t end = (sep == std::string::npos) ? path.size() : sep; + if (path.compare(start, end - start, "..") == 0) return false; + if (sep == std::string::npos) return true; + start = sep + 1; + } +} + } // namespace reasampler::package diff --git a/src/core/package/package_format.h b/src/core/package/package_format.h index 53b9538..185f890 100644 --- a/src/core/package/package_format.h +++ b/src/core/package/package_format.h @@ -56,25 +56,39 @@ enum class PackageReadability { PackageReadability classifyPackageVersion(std::uint32_t formatVersion, std::uint32_t minReaderVersion); -// The entry-name rule that makes path expression structurally impossible: a -// bare file name only. Rejects empty, ".", the exact ".." component (a name -// can only ever be one component, since separators are banned below — a -// substring scan would over-reject legal names like "take..final.wav"), any -// control byte (NUL included — truncates at the first filesystem call and -// collides two distinct manifest entries onto one file) or 0x7F, any '/', -// '\\' or ':' (which also bans every absolute form — drive, UNC, rooted), any -// Windows-reserved character (`*?|<>"`), a trailing dot or space (silently -// stripped at file creation, so "a.wav " and "a.wav" would collide), a DOS -// device name (CON/PRN/AUX/NUL/COM1-9/LPT1-9, case-insensitive, with or -// without an extension), and names over kMaxEntryNameBytes. Enforced on -// encode AND decode by package_manifest. +// The entry-name rule: a bare file name only. Rejects empty, ".", the exact +// ".." component (a name can only ever be one component, since separators are +// banned below — a substring scan would over-reject legal names like +// "take..final.wav"), any control byte (NUL included — truncates at the first +// filesystem call and collides two distinct manifest entries onto one file) or +// 0x7F, any '/', '\\' or ':' (which also bans every absolute form — drive, UNC, +// rooted), any Windows-reserved character (`*?|<>"`), a trailing dot or space +// (silently stripped at file creation, so "a.wav " and "a.wav" would collide), +// a DOS device name (CON/PRN/AUX/NUL/COM1-9/LPT1-9 plus the superscript +// COM/LPT 1-3 forms, case-insensitive, with or without an extension), names +// over kMaxEntryNameBytes, and any byte sequence that is not well-formed UTF-8. bool isValidEntryName(const std::string& name); +// The format's name-equivalence rule: two entry names that differ only by ASCII +// case are ONE name. Windows and macOS's default APFS are case-insensitive, so +// "Kick.wav" and "kick.wav" would extract onto a single file — and a bank +// authored on a case-sensitive filesystem produces that pair honestly. Non-ASCII +// bytes compare exactly (see this directory's CLAUDE.md on NFC/NFD). +bool sameEntryName(const std::string& a, const std::string& b); + +// The one field in the format that CAN express a path: a nested Sample's +// relativePath, which is bank-relative by design. Rejects every absolute form +// (the shared util::isAbsolutePath test) and any ".." component — BankModel::add +// checks only the former, so traversal reaches the format without this. +bool isValidNestedSamplePath(const std::string& path); + // The fixed header, informational semver included. writerVersion is // version::stampVersion() on the write side — it exists so a TooNew refusal can // tell the user which build to install; it never gates. Defaults are 0/0, NOT -// the current ladder pair — a caller reading `header` after a Malformed -// verdict must see an obviously-unset value, not a plausible-looking 1/1. +// the current ladder pair, so a header that never parsed reads as obviously +// unset rather than as a plausible 1/1. The fields are meaningful whenever they +// are non-zero, not only on success: a decode that got past the header and +// failed later (a corrupt manifest) reports the real pair alongside Malformed. struct PackageHeader { std::uint32_t formatVersion = 0; std::uint32_t minReaderVersion = 0; diff --git a/src/core/package/package_manifest.cpp b/src/core/package/package_manifest.cpp index 4740b3e..5a2763e 100644 --- a/src/core/package/package_manifest.cpp +++ b/src/core/package/package_manifest.cpp @@ -13,11 +13,12 @@ using json::numToStr; using ObjWriter = json::Writer; // Shared by serializeManifest and deserializeManifest — see this directory's -// CLAUDE.md for why duplicate names are rejected both ways. +// CLAUDE.md for why duplicate names are rejected both ways. Equivalence is the +// format's, not std::string's: sameEntryName folds ASCII case. bool duplicateName(const std::vector& entries) { for (std::size_t i = 0; i < entries.size(); ++i) for (std::size_t j = i + 1; j < entries.size(); ++j) - if (entries[i].fileName == entries[j].fileName) return true; + if (sameEntryName(entries[i].fileName, entries[j].fileName)) return true; return false; } @@ -45,6 +46,7 @@ bool PackageManifest::operator==(const PackageManifest& o) const { std::optional serializeManifest(const PackageManifest& m) { for (const auto& e : m.entries) { if (!isValidEntryName(e.fileName)) return std::nullopt; + if (!isValidNestedSamplePath(e.sample.relativePath)) return std::nullopt; // Cross-module contract with src/shell/package — see this directory's // CLAUDE.md. if (e.byteLength == 0) return std::nullopt; @@ -122,19 +124,22 @@ bool parseEntry(json::Reader& r, PackageEntry& e) { std::string key; if (!r.parseKey(key)) return false; + // A repeated key is rejected here exactly as at the root — same format + // question, one level down. if (key == "name") { - if (!r.parseString(e.fileName)) return false; + if (haveName || !r.parseString(e.fileName)) return false; haveName = true; } else if (key == "length") { std::int64_t v = 0; - if (!r.parseInt64(v)) return false; + if (haveLength || !r.parseInt64(v)) return false; if (v < 0) return false; e.byteLength = static_cast(v); haveLength = true; } else if (key == "hash") { - if (!r.parseString(e.byteHash)) return false; + if (haveHash || !r.parseString(e.byteHash)) return false; haveHash = true; } else if (key == "index") { + if (haveSample) return false; std::string raw; if (!r.captureValue(raw)) return false; auto idx = model::BankModel::deserialize(raw); @@ -150,25 +155,31 @@ bool parseEntry(json::Reader& r, PackageEntry& e) { if (!r.consume('}')) return false; if (!haveName || !haveLength || !haveHash || !haveSample) return false; - return isValidEntryName(e.fileName); + return isValidEntryName(e.fileName) && isValidNestedSamplePath(e.sample.relativePath); } bool parseManifest(json::Reader& r, PackageManifest& m) { if (!r.consume('{')) return false; r.skipWs(); - bool haveEntries = false; + // "Which duplicate keys are legal" is a format contract, so it is answered + // for every root key rather than only for the one that would accumulate: + // a repeated key is rejected, never last-wins. Unknown keys may repeat — + // they are skipped, and a future format must stay free to add them. + bool haveBankName = false, haveExported = false, haveEntries = false, haveSlots = false; + const auto firstTime = [](bool& seen) { const bool ok = !seen; seen = true; return ok; }; if (!r.consume('}')) { // not the empty-object shortcut: parse the members do { std::string key; if (!r.parseKey(key)) return false; if (key == "bankName") { + if (!firstTime(haveBankName)) return false; if (!r.parseString(m.bankDisplayName)) return false; } else if (key == "exported") { + if (!firstTime(haveExported)) return false; if (!r.parseInt64(m.exportTimestamp)) return false; } else if (key == "entries") { - if (haveEntries) return false; // a repeated key must not accumulate - haveEntries = true; + if (!firstTime(haveEntries)) return false; if (!r.consume('[')) return false; r.skipWs(); if (!r.consume(']')) { @@ -180,6 +191,7 @@ bool parseManifest(json::Reader& r, PackageManifest& m) { if (!r.consume(']')) return false; } } else if (key == "slots") { + if (!firstTime(haveSlots)) return false; if (!parseSlots(r, m.slots)) return false; } else { if (!r.skipValue()) return false; // forward-compat unknown keys diff --git a/src/core/package/package_manifest.h b/src/core/package/package_manifest.h index 0b31ce0..906f0b6 100644 --- a/src/core/package/package_manifest.h +++ b/src/core/package/package_manifest.h @@ -40,18 +40,21 @@ struct PackageManifest { bool operator==(const PackageManifest& o) const; }; -// Emits the manifest JSON. nullopt when the manifest cannot be represented: -// an invalid or duplicate entry name, a zero-length entry (see this -// directory's CLAUDE.md — the shell's payload-append seam cannot round-trip -// one), or a sample record BankModel itself would reject (empty id, absolute -// path) — refusing on encode so an undecodable package is never written. +// Emits the manifest JSON. nullopt when the manifest cannot be represented: an +// invalid or duplicate entry name (duplicate by sameEntryName, not string +// equality), a nested relativePath isValidNestedSamplePath refuses, a +// zero-length entry (see this directory's CLAUDE.md — the shell's payload-append +// seam cannot round-trip one), or a sample record BankModel itself would reject +// (empty id, absolute path) — refusing on encode so an undecodable package is +// never written. std::optional serializeManifest(const PackageManifest& m); // Parses manifest JSON (nullopt on malformed input, never UB). Unknown keys are -// skipped at every level, so an additive newer manifest still parses. Rejects -// what encode rejects — entry names are validated on BOTH directions because a -// package can arrive from anywhere — plus a missing per-entry field or a -// negative length. +// skipped at every level, so an additive newer manifest still parses; a repeated +// KNOWN root key is rejected rather than last-wins. Rejects what encode rejects +// except the zero-length entry — names and nested paths are validated on BOTH +// directions because a package can arrive from anywhere — plus a missing +// per-entry field or a negative length. std::optional deserializeManifest(const std::string& json); } // namespace reasampler::package diff --git a/tests/test_bank_package.cpp b/tests/test_bank_package.cpp index 27f16af..8783230 100644 --- a/tests/test_bank_package.cpp +++ b/tests/test_bank_package.cpp @@ -142,9 +142,7 @@ static void testEncodeRefusesWhatManifestRefuses() { m.entries[0].fileName = "../evil.wav"; CHECK(!encodePackage(m).has_value()); - // A zero-length entry cannot round-trip through the shell's filesystem - // seam (src/shell/package's appendPayload refuses an empty payload) — the - // format layer must never produce one. + // See src/core/package/CLAUDE.md for the shell seam that forces this. PackageManifest zeroLen = fixture(1, 1); zeroLen.entries[1].byteLength = 0; CHECK(!encodePackage(zeroLen).has_value()); @@ -185,6 +183,41 @@ static void testTruncationAtEveryByteOffsetIsMalformed() { CHECK(decodePackage(file, file.size() - 1).status == PackageReadability::Malformed); } +// --- header defaults --------------------------------------------------------- + +// The 0/0 defaults are not the current ladder pair, so a header that never +// parsed cannot be mistaken for a plausible 1/1. They mean "unset", NOT "the +// decode failed": a decode that got past the header reports the real pair +// alongside its Malformed verdict. +static void testHeaderDefaultsMeanUnparsed() { + CHECK(PackageHeader{}.formatVersion == 0); + CHECK(PackageHeader{}.minReaderVersion == 0); + CHECK(PackageHeader{}.writerVersion.empty()); + + // Failed before the header: bad magic, and a semver truncated mid-string. + std::vector badMagic = rawHeader(1, 1, "1.0.0"); + appendManifest(badMagic, "{}"); + badMagic[0] = 'Z'; + const DecodedPackage magic = decodePackage(badMagic, badMagic.size()); + CHECK(magic.status == PackageReadability::Malformed); + CHECK(magic.header == PackageHeader{}); + + std::vector cutSemver = rawHeader(1, 1, "1.0.0"); + cutSemver.resize(18); + CHECK(decodePackage(cutSemver, cutSemver.size()).header == PackageHeader{}); + + // Failed after it: a same-version package with a corrupt manifest is + // Malformed, and its header is fully populated. + std::vector corrupt = + rawHeader(kPackageFormatVersion, kPackageMinReaderVersion, "1.0.0"); + appendManifest(corrupt, "not json"); + const DecodedPackage late = decodePackage(corrupt, corrupt.size()); + CHECK(late.status == PackageReadability::Malformed); + CHECK(late.header.formatVersion == kPackageFormatVersion); + CHECK(late.header.minReaderVersion == kPackageMinReaderVersion); + CHECK(late.header.writerVersion == "1.0.0"); +} + // --- version ladder: TooNew refuses whole ------------------------------------ static void testTooNewProducesNoManifest() { @@ -365,6 +398,7 @@ int main() { testEncodeDecodeRoundTrip(); testEncodeRefusesWhatManifestRefuses(); testTruncationAtEveryByteOffsetIsMalformed(); + testHeaderDefaultsMeanUnparsed(); testTooNewProducesNoManifest(); testAdditiveUnparseableManifestIsTooNew(); testNewerAdditiveFormatReads(); diff --git a/tests/test_package_format.cpp b/tests/test_package_format.cpp index 56d3cb8..adf79d6 100644 --- a/tests/test_package_format.cpp +++ b/tests/test_package_format.cpp @@ -1,7 +1,7 @@ // Standalone tests for reasampler::package's format contract — no REAPER, no // test framework. Pins the version-ladder classification (both integers, every -// branch) and the entry-name rule that makes path expression structurally -// impossible in a package. +// branch) and the three naming rules: the entry-name rule, the ASCII-folding +// name equivalence, and the nested-path traversal guard. #include "../src/core/package/package_format.h" @@ -111,8 +111,110 @@ static void testEntryNameRejectsWindowsHostileNames() { CHECK(!isValidEntryName("COM1")); CHECK(!isValidEntryName("com1.txt")); CHECK(!isValidEntryName("LPT1")); - // Not a device name: a real filename that merely starts with one. + // The superscript device forms (COM¹ COM² COM³ LPT¹ LPT² LPT³ in UTF-8): + // Windows reads those as digits in a device name, so "COM².wav" is COM2. + CHECK(!isValidEntryName("COM\xC2\xB9.wav")); + CHECK(!isValidEntryName("com\xC2\xB2")); + CHECK(!isValidEntryName("COM\xC2\xB3.wav")); + CHECK(!isValidEntryName("LPT\xC2\xB9")); + CHECK(!isValidEntryName("lpt\xC2\xB2.txt")); + CHECK(!isValidEntryName("LPT\xC2\xB3.wav")); + // Not a device name: a real filename that merely starts with one, and the + // zero forms, which Windows does not reserve. CHECK(isValidEntryName("console.wav")); + CHECK(isValidEntryName("COM0.wav")); + CHECK(isValidEntryName("LPT0")); + // Nor does a superscript past 3 name a device. + CHECK(isValidEntryName("COM\xE2\x81\xB4.wav")); // U+2074 SUPERSCRIPT FOUR +} + +// --- isValidEntryName: UTF-8 well-formedness --------------------------------- + +static void testEntryNameAcceptsWellFormedUtf8() { + CHECK(isValidEntryName("caf\xC3\xA9.wav")); // 2-byte: é + CHECK(isValidEntryName("\xE2\x99\xAA.wav")); // 3-byte: ♪ + CHECK(isValidEntryName("\xF0\x9F\x8E\xB5.wav")); // 4-byte: 🎵 + CHECK(isValidEntryName("\xEF\xBB\xBF.wav")); // U+FEFF, ugly but well-formed + CHECK(isValidEntryName("\xF4\x8F\xBF\xBF.wav")); // U+10FFFF, the last code point +} + +static void testEntryNameRejectsIllFormedUtf8() { + // Two names differing ONLY in their invalid bytes: a host converting to + // UTF-16 substitutes U+FFFD for both by default, collapsing them onto one + // file — the duplicate-name collision the manifest cannot otherwise see. + CHECK(!isValidEntryName("a\x80.wav")); // stray continuation byte + CHECK(!isValidEntryName("a\x81.wav")); + // Structural: truncated sequences (a lead byte the name ends inside). + CHECK(!isValidEntryName("a\xC3")); + CHECK(!isValidEntryName("a\xE2\x99")); + CHECK(!isValidEntryName("a\xF0\x9F\x8E")); + // A lead byte followed by a non-continuation. + CHECK(!isValidEntryName("a\xC3\x41.wav")); + // Overlong encodings: an alternate spelling of an ASCII byte we ban. + CHECK(!isValidEntryName("a\xC0\xAF.wav")); // overlong '/' + CHECK(!isValidEntryName("a\xC0\x80.wav")); // overlong NUL + CHECK(!isValidEntryName("a\xE0\x80\xAF.wav")); // overlong '/', 3-byte + CHECK(!isValidEntryName("a\xF0\x80\x80\xAF.wav")); // overlong '/', 4-byte + // Surrogate halves: no code point, and unrepresentable in UTF-16. + CHECK(!isValidEntryName("a\xED\xA0\x80.wav")); // U+D800 + CHECK(!isValidEntryName("a\xED\xBF\xBF.wav")); // U+DFFF + // Past U+10FFFF, and the 5/6-byte leads that never encode anything. + CHECK(!isValidEntryName("a\xF4\x90\x80\x80.wav")); // U+110000 + CHECK(!isValidEntryName("a\xF5\x80\x80\x80.wav")); + CHECK(!isValidEntryName("a\xFC\x80\x80\x80\x80\x80.wav")); + CHECK(!isValidEntryName("a\xFF.wav")); +} + +// --- sameEntryName ----------------------------------------------------------- + +static void testSameEntryNameFoldsAsciiCase() { + // A bank authored on a case-sensitive filesystem produces this pair + // honestly; Windows and default APFS would extract both onto one file. + CHECK(sameEntryName("Kick.wav", "kick.wav")); + CHECK(sameEntryName("KICK.WAV", "kick.wav")); + CHECK(sameEntryName("kick.wav", "kick.wav")); + CHECK(!sameEntryName("kick.wav", "snare.wav")); + CHECK(!sameEntryName("kick.wav", "kick.wave")); // length alone decides + CHECK(!sameEntryName("", "a")); + CHECK(sameEntryName("", "")); + // ASCII only: "é" vs "É" are two names here (the NFC/NFD limitation this + // shares — see this directory's CLAUDE.md). + CHECK(!sameEntryName("caf\xC3\xA9.wav", "caf\xC3\x89.wav")); + // Only the letters fold — the bytes flanking the ASCII range must not. + CHECK(!sameEntryName("a[b", "a{b")); // 0x5B vs 0x7B, 'Z'+1 and 'z'+1 + CHECK(!sameEntryName("a@b", "a`b")); // 0x40 vs 0x60, 'A'-1 and 'a'-1 +} + +// --- isValidNestedSamplePath ------------------------------------------------- + +static void testNestedSamplePathAcceptsRelative() { + CHECK(isValidNestedSamplePath("a.wav")); + CHECK(isValidNestedSamplePath("reasampler_bank/kick.wav")); + CHECK(isValidNestedSamplePath("reasampler_bank\\kick.wav")); + CHECK(isValidNestedSamplePath("deep/dir/tree/a.wav")); + // A ".." that is not a whole component is an ordinary name. + CHECK(isValidNestedSamplePath("take..final/a.wav")); + CHECK(isValidNestedSamplePath("bank/..hidden")); + CHECK(isValidNestedSamplePath("a..b")); +} + +static void testNestedSamplePathRejectsTraversalAndAbsolute() { + // BankModel::add catches only the absolute forms, so traversal reaches the + // format unless this rule stops it. + CHECK(!isValidNestedSamplePath("..")); + CHECK(!isValidNestedSamplePath("../evil.wav")); + CHECK(!isValidNestedSamplePath("..\\evil.wav")); + CHECK(!isValidNestedSamplePath("bank/../../evil.wav")); + CHECK(!isValidNestedSamplePath("bank\\..\\evil.wav")); + CHECK(!isValidNestedSamplePath("bank/..")); + CHECK(!isValidNestedSamplePath("bank/../")); + // Everything util::isAbsolutePath already catches. + CHECK(!isValidNestedSamplePath("/rooted.wav")); + CHECK(!isValidNestedSamplePath("\\rooted.wav")); + CHECK(!isValidNestedSamplePath("C:/abs.wav")); + CHECK(!isValidNestedSamplePath("C:\\abs.wav")); + CHECK(!isValidNestedSamplePath("c:relative-to-drive.wav")); + CHECK(!isValidNestedSamplePath("\\\\server\\share.wav")); } int main() { @@ -123,6 +225,11 @@ int main() { testEntryNameRejectsSeparatorsAndDots(); testEntryNameRejectsAbsolutePrefixes(); testEntryNameRejectsWindowsHostileNames(); + testEntryNameAcceptsWellFormedUtf8(); + testEntryNameRejectsIllFormedUtf8(); + testSameEntryNameFoldsAsciiCase(); + testNestedSamplePathAcceptsRelative(); + testNestedSamplePathRejectsTraversalAndAbsolute(); if (g_fail == 0) { std::printf("package_format_tests: all passed\n"); diff --git a/tests/test_package_manifest.cpp b/tests/test_package_manifest.cpp index e418ed7..f644f22 100644 --- a/tests/test_package_manifest.cpp +++ b/tests/test_package_manifest.cpp @@ -1,7 +1,7 @@ // Standalone tests for reasampler::package's manifest codec — no REAPER, no // test framework. The round-trip fixture exercises every manifest field and // every Sample optional in both present and absent states; the rejection suite -// pins the entry-name rule on encode AND decode. +// pins the naming, case-folding and traversal rules on encode AND decode. #include "../src/core/package/package_manifest.h" @@ -132,16 +132,58 @@ static void testEncodeRejectsBadEntryName() { } } +// One entry, `name` spliced in as raw manifest text so a hostile spelling +// (escapes included) is expressible — a package is not limited to what encode +// emits. +static std::string oneEntryJson(const std::string& name, + const std::string& relativePath = "bank/a.wav") { + return "{\"entries\":[{\"name\":\"" + name + + "\",\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\"," + "\"relativePath\":\"" + relativePath + "\"}]}}]}"; +} + static void testDecodeRejectsBadEntryName() { - for (const char* bad : {"..\\\\evil.wav", "../evil.wav", "dir/a.wav", "C:\\\\a.wav", ".."}) { - // Hand-rolled JSON: a hostile package is not limited to what encode emits. - std::string json = - std::string("{\"entries\":[{\"name\":\"") + bad + - "\",\"length\":1,\"hash\":\"h\"," - "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\"," - "\"relativePath\":\"bank/a.wav\"}]}}]}"; - CHECK(!deserializeManifest(json).has_value()); - } + for (const char* bad : {"..\\\\evil.wav", "../evil.wav", "dir/a.wav", "C:\\\\a.wav", ".."}) + CHECK(!deserializeManifest(oneEntryJson(bad)).has_value()); + + // The rest of the rule set through decode — the direction that matters, + // since a package can arrive from anywhere. + CHECK(!deserializeManifest(oneEntryJson("CON.wav")).has_value()); // DOS device + CHECK(!deserializeManifest(oneEntryJson("COM\xC2\xB2.wav")).has_value()); // COM² + CHECK(!deserializeManifest(oneEntryJson("a.wav ")).has_value()); // trailing space + CHECK(!deserializeManifest(oneEntryJson("a.wav.")).has_value()); // trailing dot + CHECK(!deserializeManifest(oneEntryJson("a*b.wav")).has_value()); // reserved char + // A NUL smuggled in as a JSON escape: the manifest text is legal, the + // decoded name is not. + CHECK(!deserializeManifest(oneEntryJson("a\\u0000b.wav")).has_value()); + // Ill-formed UTF-8 as raw bytes. + CHECK(!deserializeManifest(oneEntryJson("a\xC3.wav")).has_value()); + // A lone surrogate never reaches the name rule — the JSON reader refuses + // the unpaired \uD800 first. Pinned so that refusal cannot silently become + // "decoded to U+FFFD and accepted". + CHECK(!deserializeManifest(oneEntryJson("a\\ud800b.wav")).has_value()); +} + +static void testDecodeRejectsTraversalInNestedPath() { + // The one field in the format that CAN express a path. BankModel::add + // catches the absolute forms only, so ".." arrives unless the package layer + // refuses it. + CHECK(!deserializeManifest(oneEntryJson("a.wav", "../../evil.wav")).has_value()); + CHECK(!deserializeManifest(oneEntryJson("a.wav", "bank/../evil.wav")).has_value()); + CHECK(!deserializeManifest(oneEntryJson("a.wav", "..")).has_value()); + // A ".." that is not a whole component still reads. + CHECK(deserializeManifest(oneEntryJson("a.wav", "take..final/a.wav")).has_value()); +} + +static void testEncodeRejectsTraversalInNestedPath() { + PackageManifest m = fixture(); + m.entries[0].sample.relativePath = "../../evil.wav"; + CHECK(!serializeManifest(m).has_value()); + + PackageManifest m2 = fixture(); + m2.entries[0].sample.relativePath = "bank/../evil.wav"; + CHECK(!serializeManifest(m2).has_value()); } static void testDuplicateEntryNamesRejectedBothWays() { @@ -149,40 +191,92 @@ static void testDuplicateEntryNamesRejectedBothWays() { m.entries[1].fileName = m.entries[0].fileName; CHECK(!serializeManifest(m).has_value()); + // Case-folded: two names one case-insensitive filesystem extracts onto a + // single file are one name here too, in both directions. + PackageManifest folded = fixture(); + folded.entries[1].fileName = "KICK.WAV"; // entries[0] is "kick.wav" + CHECK(!serializeManifest(folded).has_value()); + // Decode side, from a hand-built duplicate (a hostile package is not // limited to what encode emits). const std::string dup = "{\"entries\":[" "{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\"," "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}]}}," - "{\"name\":\"a.wav\",\"length\":2,\"hash\":\"i\"," + "{\"name\":\"A.WAV\",\"length\":2,\"hash\":\"i\"," "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s2\",\"relativePath\":\"q\"}]}}]}"; CHECK(!deserializeManifest(dup).has_value()); + + // Two names that differ outside the ASCII letters are still two names. + const std::string distinct = + "{\"entries\":[" + "{\"name\":\"a1.wav\",\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}]}}," + "{\"name\":\"a2.wav\",\"length\":2,\"hash\":\"i\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s2\",\"relativePath\":\"q\"}]}}]}"; + CHECK(deserializeManifest(distinct).has_value()); } -static void testDuplicateEntriesKeyRejected() { - // A repeated "entries" key must not accumulate into two arrays' worth of - // entries — reject rather than silently union them. - const std::string json = +static void testRepeatedRootKeyRejected() { + // A repeated "entries" must not accumulate into two arrays' worth of + // entries; the other three assign rather than append, but "which duplicate + // keys are legal" is one format answer, not four. + const std::string entriesTwice = "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\"," "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}]}}]," "\"entries\":[{\"name\":\"b.wav\",\"length\":1,\"hash\":\"h\"," "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s2\",\"relativePath\":\"q\"}]}}]}"; - CHECK(!deserializeManifest(json).has_value()); + CHECK(!deserializeManifest(entriesTwice).has_value()); + + CHECK(!deserializeManifest("{\"bankName\":\"A\",\"bankName\":\"B\"}").has_value()); + CHECK(!deserializeManifest("{\"exported\":1,\"exported\":2}").has_value()); + CHECK(!deserializeManifest("{\"slots\":[],\"slots\":[]}").has_value()); + // Unknown keys stay repeatable: they are skipped, and a future format must + // be free to add them. + CHECK(deserializeManifest("{\"future\":1,\"future\":2}").has_value()); + + // The same answer one level down, so a hostile manifest cannot make two + // readers disagree about which spelling of an entry field is the real one. + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"name\":\"b.wav\",\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}]}}]}") + .has_value()); + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"length\":2,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}]}}]}") + .has_value()); + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\",\"hash\":\"i\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}]}}]}") + .has_value()); + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}]}," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s2\",\"relativePath\":\"q\"}]}}]}") + .has_value()); } // --- rejection: structural --------------------------------------------------- -// A zero-length entry cannot round-trip through the shell's filesystem seam -// (src/shell/package's appendPayload refuses an empty payload) — the format -// layer must never produce one, so encode refuses it. Decode does not enforce -// this (a hostile/older package declaring one is not this codec's concern). +// See src/core/package/CLAUDE.md for the shell seam that forces this. static void testEncodeRejectsZeroLengthEntry() { PackageManifest m = fixture(); m.entries[0].byteLength = 0; CHECK(!serializeManifest(m).has_value()); } +// The asymmetry is deliberate: refusing a zero-length entry is an obligation on +// what this layer WRITES, not a claim about what a package may declare. Pinned +// so it is not "fixed" into a decode-side rejection. +static void testDecodeAcceptsZeroLengthEntry() { + auto m = deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":0,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}]}}]}"); + CHECK(m.has_value()); + CHECK(m->entries.size() == 1); + CHECK(m->entries[0].byteLength == 0); +} + static void testEncodeRejectsUnrepresentableSample() { PackageManifest m = fixture(); m.entries[0].sample.id.clear(); // BankModel::add rejects an empty id @@ -261,9 +355,12 @@ int main() { testUnknownKeysSkippedAtEveryLevel(); testEncodeRejectsBadEntryName(); testDecodeRejectsBadEntryName(); + testDecodeRejectsTraversalInNestedPath(); + testEncodeRejectsTraversalInNestedPath(); testDuplicateEntryNamesRejectedBothWays(); - testDuplicateEntriesKeyRejected(); + testRepeatedRootKeyRejected(); testEncodeRejectsZeroLengthEntry(); + testDecodeAcceptsZeroLengthEntry(); testEncodeRejectsUnrepresentableSample(); testDecodeRejectsMalformedShapes();