diff --git a/CMakeLists.txt b/CMakeLists.txt index ed9f193..f783871 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -92,4 +92,3 @@ enable_testing() add_subdirectory(src/core) add_subdirectory(src/app) add_subdirectory(src/shell/instrument) -add_subdirectory(src/core/package) diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt index de07360..0eeee11 100644 --- a/src/core/CMakeLists.txt +++ b/src/core/CMakeLists.txt @@ -13,6 +13,7 @@ add_subdirectory(capture) add_subdirectory(tracking) add_subdirectory(reclaim) add_subdirectory(version) +add_subdirectory(package) add_subdirectory(view) add_subdirectory(ui) add_subdirectory(instrument) diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index cb56e08..a8bfe62 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -26,9 +26,12 @@ landing after the format. 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 `..`, no drive/UNC/rooted form), - enforced on encode AND decode because a package can arrive from anywhere. - There is no field in the format capable of expressing a path. + 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. - **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 @@ -76,3 +79,8 @@ landing after the format. version pair classifies `Readable`; for `TooNew` it stops at the semver — don't "fix" it to read the manifest length there, a future structural format may have moved it. +- 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. diff --git a/src/core/package/bank_package.h b/src/core/package/bank_package.h index 5fb0e94..677b567 100644 --- a/src/core/package/bank_package.h +++ b/src/core/package/bank_package.h @@ -1,8 +1,7 @@ #pragma once // bank_package — RSBK framing and layout arithmetic: header encode, prefix -// decode, and the ordered {name, offset, length} entry layout. Framing only, -// never a payload: this module never holds, copies, or hashes an entry's audio -// — the shell streams payloads one at a time against the layout produced here. +// decode, and the ordered {name, offset, length} entry layout. See this +// directory's CLAUDE.md for the framing-only invariant. Pure: no filesystem. #include #include diff --git a/src/core/package/package_format.cpp b/src/core/package/package_format.cpp index 139163b..d07aaec 100644 --- a/src/core/package/package_format.cpp +++ b/src/core/package/package_format.cpp @@ -1,5 +1,7 @@ #include "core/package/package_format.h" +#include + namespace reasampler::package { PackageReadability classifyPackageVersion(std::uint32_t formatVersion, @@ -10,13 +12,40 @@ PackageReadability classifyPackageVersion(std::uint32_t formatVersion, return PackageReadability::Readable; } +namespace { + +// 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. +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))); + 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", + }; + for (const auto& r : kReserved) if (base == r) return true; + return false; +} + +} // namespace + bool isValidEntryName(const std::string& name) { if (name.empty() || name.size() > kMaxEntryNameBytes) return false; - if (name == ".") return false; - if (name.find("..") != std::string::npos) return false; - for (char c : name) { + if (name == "." || name == "..") return false; + // fopen/CreateFileW both silently strip a trailing dot or space at + // creation, so "a.wav " and "a.wav" would collide on one file. + if (name.back() == '.' || name.back() == ' ') return false; + for (unsigned char c : name) { + // NUL and other control bytes truncate at the first filesystem call + // (std::ofstream, fopen, CreateFileW off .c_str()) — two names that + // differ only after the NUL land on the same file. + if (c < 0x20 || c == 0x7F) return false; if (c == '/' || c == '\\' || c == ':') return false; + if (c == '*' || c == '?' || c == '|' || c == '<' || c == '>' || c == '"') return false; } + if (isDosDeviceName(name)) return false; return true; } diff --git a/src/core/package/package_format.h b/src/core/package/package_format.h index acb70f3..53b9538 100644 --- a/src/core/package/package_format.h +++ b/src/core/package/package_format.h @@ -57,17 +57,27 @@ 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, ".", any ".." occurrence, any '/', '\\' -// or ':' (which also bans every absolute form — drive, UNC, rooted), and names -// over kMaxEntryNameBytes. Enforced on encode AND decode by package_manifest. +// 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. bool isValidEntryName(const std::string& name); // 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. +// 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. struct PackageHeader { - std::uint32_t formatVersion = kPackageFormatVersion; - std::uint32_t minReaderVersion = kPackageMinReaderVersion; + std::uint32_t formatVersion = 0; + std::uint32_t minReaderVersion = 0; std::string writerVersion; bool operator==(const PackageHeader& o) const { diff --git a/src/core/package/package_manifest.cpp b/src/core/package/package_manifest.cpp index 10cc267..a4d4ba9 100644 --- a/src/core/package/package_manifest.cpp +++ b/src/core/package/package_manifest.cpp @@ -5,11 +5,6 @@ #include "core/json/json.h" #include "core/package/package_format.h" -// Duplicate entry names are rejected in both directions: two payloads landing -// on one destination name is incoherent, and on import it would be a silent -// overwrite. Sample-id rules (remap, collision) are deliberately NOT enforced -// here — they are the import plan's decisions, not the codec's. - namespace reasampler::package { namespace { @@ -17,6 +12,8 @@ namespace { 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. 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) @@ -155,37 +152,40 @@ bool parseEntry(json::Reader& r, PackageEntry& e) { bool parseManifest(json::Reader& r, PackageManifest& m) { if (!r.consume('{')) return false; r.skipWs(); - if (r.consume('}')) return true; // empty object: a valid empty manifest + bool haveEntries = false; + if (!r.consume('}')) { // not the empty-object shortcut: parse the members + do { + std::string key; + if (!r.parseKey(key)) return false; - do { - std::string key; - if (!r.parseKey(key)) return false; - - if (key == "bankName") { - if (!r.parseString(m.bankDisplayName)) return false; - } else if (key == "exported") { - if (!r.parseInt64(m.exportTimestamp)) return false; - } else if (key == "entries") { - if (!r.consume('[')) return false; - r.skipWs(); - if (!r.consume(']')) { - do { - PackageEntry e; - if (!parseEntry(r, e)) return false; - m.entries.push_back(std::move(e)); - } while (r.consume(',')); - if (!r.consume(']')) return false; + if (key == "bankName") { + if (!r.parseString(m.bankDisplayName)) return false; + } else if (key == "exported") { + if (!r.parseInt64(m.exportTimestamp)) return false; + } else if (key == "entries") { + if (haveEntries) return false; // a repeated key must not accumulate + haveEntries = true; + if (!r.consume('[')) return false; + r.skipWs(); + if (!r.consume(']')) { + do { + PackageEntry e; + if (!parseEntry(r, e)) return false; + m.entries.push_back(std::move(e)); + } while (r.consume(',')); + if (!r.consume(']')) return false; + } + } else if (key == "slots") { + if (!parseSlots(r, m.slots)) return false; + } else { + if (!r.skipValue()) return false; // forward-compat unknown keys } - } else if (key == "slots") { - if (!parseSlots(r, m.slots)) return false; - } else { - if (!r.skipValue()) return false; // forward-compat unknown keys - } - } while (r.consume(',')); + } while (r.consume(',')); - if (!r.consume('}')) return false; + if (!r.consume('}')) return false; + } r.skipWs(); - if (!r.eof()) return false; // trailing garbage + if (!r.eof()) return false; // trailing garbage — even after an empty object return !duplicateName(m.entries); } diff --git a/tests/test_bank_package.cpp b/tests/test_bank_package.cpp index 5279bd7..565ab7c 100644 --- a/tests/test_bank_package.cpp +++ b/tests/test_bank_package.cpp @@ -95,13 +95,15 @@ static void appendManifest(std::vector& out, const std::string& js } // One-entry manifest JSON with `extra` spliced in as additional root content -// ("" for none) — for images a current writer would never emit. +// ("" for none) and `sampleExtra` spliced into the nested Sample object — for +// images a current writer would never emit. static std::string handManifest(const std::string& name, int length, - const std::string& extra) { + const std::string& extra, + const std::string& sampleExtra = "") { return std::string("{") + extra + "\"entries\":[{\"name\":\"" + name + "\",\"length\":" + std::to_string(length) + ",\"hash\":\"h\"," - "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\"," + sampleExtra + "\"relativePath\":\"bank/a.wav\"}]}}]}"; } @@ -210,11 +212,14 @@ static void testTooNewProducesNoManifest() { // --- version ladder: additive forward compatibility -------------------------- // The reason two integers exist: a NEWER formatVersion whose minReaderVersion -// still reaches back to this build must read, with its unknown keys skipped. +// still reaches back to this build must read, with its unknown keys skipped — +// at the manifest level AND inside the nested Sample blob, the actual +// motivating case for the two-integer ladder (see this directory's CLAUDE.md). static void testNewerAdditiveFormatReads() { const std::string manifest = handManifest( "a.wav", 4, - "\"instrumentState\":{\"future\":[1,2,3]},\"anotherNewKey\":\"x\","); + "\"instrumentState\":{\"future\":[1,2,3]},\"anotherNewKey\":\"x\",", + "\"someFutureSampleField\":42,"); std::vector bytes = rawHeader(kPackageFormatVersion + 1, kPackageMinReaderVersion, "1.9.0"); appendManifest(bytes, manifest); diff --git a/tests/test_package_format.cpp b/tests/test_package_format.cpp index 442d2ef..56d3cb8 100644 --- a/tests/test_package_format.cpp +++ b/tests/test_package_format.cpp @@ -56,6 +56,12 @@ static void testEntryNameAccepts() { CHECK(isValidEntryName(".hidden")); // a leading dot is a bare name CHECK(isValidEntryName("a.b.c.wav")); // single dots are fine CHECK(isValidEntryName(std::string(kMaxEntryNameBytes, 'x'))); // at the cap + // Legal names containing a ".." substring that is not the whole name: a + // name can only ever be one path component (separators are banned), so + // ".." as a component is the only expressible traversal. + CHECK(isValidEntryName("take..final.wav")); + CHECK(isValidEntryName("loop...wav")); + CHECK(isValidEntryName("a..b")); } static void testEntryNameRejectsSeparatorsAndDots() { @@ -64,11 +70,16 @@ static void testEntryNameRejectsSeparatorsAndDots() { CHECK(!isValidEntryName("..")); CHECK(!isValidEntryName("..\\evil.wav")); CHECK(!isValidEntryName("../evil.wav")); - CHECK(!isValidEntryName("a..b.wav")); // any ".." occurrence rejects CHECK(!isValidEntryName("dir/inner.wav")); CHECK(!isValidEntryName("dir\\inner.wav")); CHECK(!isValidEntryName("/rooted.wav")); CHECK(!isValidEntryName("\\rooted.wav")); + // Embedded NUL: every plausible filesystem call (ofstream, fopen, + // CreateFileW off .c_str()) truncates at it, so two names differing only + // after the NUL would collide on one file. + CHECK(!isValidEntryName(std::string("a\0b.wav", 7))); + // Other control bytes (newline here) are equally hostile to logs/UI. + CHECK(!isValidEntryName("a\nb.wav")); } static void testEntryNameRejectsAbsolutePrefixes() { @@ -79,6 +90,31 @@ static void testEntryNameRejectsAbsolutePrefixes() { CHECK(!isValidEntryName(std::string(kMaxEntryNameBytes + 1, 'x'))); // over cap } +static void testEntryNameRejectsWindowsHostileNames() { + // Reserved characters. + CHECK(!isValidEntryName("a*b.wav")); + CHECK(!isValidEntryName("a?b.wav")); + CHECK(!isValidEntryName("a|b.wav")); + CHECK(!isValidEntryName("a.wav")); + CHECK(!isValidEntryName("\"q\".wav")); + // Trailing dot or space (silently stripped at creation on Windows). + CHECK(!isValidEntryName("trailing ")); + CHECK(!isValidEntryName("trailing.")); + CHECK(!isValidEntryName(" ")); + CHECK(!isValidEntryName(" ")); + // DOS device names, case-insensitive, with and without an extension. + CHECK(!isValidEntryName("NUL")); + CHECK(!isValidEntryName("CON")); + CHECK(!isValidEntryName("con.wav")); + CHECK(!isValidEntryName("PRN")); + CHECK(!isValidEntryName("AUX")); + CHECK(!isValidEntryName("COM1")); + CHECK(!isValidEntryName("com1.txt")); + CHECK(!isValidEntryName("LPT1")); + // Not a device name: a real filename that merely starts with one. + CHECK(isValidEntryName("console.wav")); +} + int main() { testClassifyReadable(); testClassifyTooNew(); @@ -86,6 +122,7 @@ int main() { testEntryNameAccepts(); testEntryNameRejectsSeparatorsAndDots(); testEntryNameRejectsAbsolutePrefixes(); + testEntryNameRejectsWindowsHostileNames(); 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 c9ecbde..8d35475 100644 --- a/tests/test_package_manifest.cpp +++ b/tests/test_package_manifest.cpp @@ -102,13 +102,14 @@ static void testEmptyManifestRoundTrips() { // --- forward compatibility --------------------------------------------------- static void testUnknownKeysSkippedAtEveryLevel() { - // A future additive manifest: unknown keys at the root and inside an entry. + // A future additive manifest: unknown keys at the root, inside an entry, + // and inside the nested index blob itself. const std::string json = "{\"bankName\":\"B\",\"exported\":7," "\"instrumentState\":{\"nested\":[1,2,{\"x\":\"y\"}]}," "\"entries\":[{\"name\":\"a.wav\",\"length\":10,\"hash\":\"h\"," "\"futureField\":\"ignored\"," - "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\"," + "\"index\":{\"version\":1,\"futureIndexField\":42,\"samples\":[{\"id\":\"s1\"," "\"relativePath\":\"bank/a.wav\"}]}}]," "\"slots\":[],\"trailingUnknown\":null}"; auto m = deserializeManifest(json); @@ -159,6 +160,17 @@ static void testDuplicateEntryNamesRejectedBothWays() { CHECK(!deserializeManifest(dup).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 = + "{\"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()); +} + // --- rejection: structural --------------------------------------------------- static void testEncodeRejectsUnrepresentableSample() { @@ -223,6 +235,10 @@ static void testDecodeRejectsMalformedShapes() { auto json = serializeManifest(fixture()); CHECK(json.has_value()); CHECK(!deserializeManifest(*json + "x").has_value()); + // Trailing garbage after the EMPTY-object shortcut specifically: this path + // returned early before reaching the eof check, so "{}JUNK" parsed valid. + CHECK(!deserializeManifest("{}JUNK").has_value()); + CHECK(deserializeManifest("{}").has_value()); // Truncation at a few JSON-level offsets (byte-level truncation of the whole // package is bank_package's suite). CHECK(!deserializeManifest(json->substr(0, json->size() / 2)).has_value()); @@ -236,6 +252,7 @@ int main() { testEncodeRejectsBadEntryName(); testDecodeRejectsBadEntryName(); testDuplicateEntryNamesRejectedBothWays(); + testDuplicateEntriesKeyRejected(); testEncodeRejectsUnrepresentableSample(); testDecodeRejectsMalformedShapes();