Tighten RSBK package-format validation for review remediation

Reject NUL/control bytes and Windows-hostile names in entry names, relax
the over-broad ".." substring ban to component-only, close the
trailing-garbage gap on empty manifests, and relocate the package
CMake subdirectory to its ladder home.

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)

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)

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),
+  names (`isValidEntryName`: no separators, no `..` component, no
-  enforced on encode AND decode because a package can arrive from anywhere.
+  drive/UNC/rooted form, no control bytes, no Windows-reserved character, no
-  There is no field in the format capable of expressing a path.
+  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.

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,
+// decode, and the ordered {name, offset, length} entry layout. See this
-// never a payload: this module never holds, copies, or hashes an entry's audio
+// directory's CLAUDE.md for the framing-only invariant. Pure: no filesystem.
-// — the shell streams payloads one at a time against the layout produced here.
 
 #include <cstdint>
 #include <optional>

src/core/package/package_format.cpp

@@ -1,5 +1,7 @@
 #include "core/package/package_format.h"
 
+#include <cctype>
+
 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<char>(std::toupper(static_cast<unsigned char>(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 == "." || name == "..") return false;
-    if (name.find("..") != std::string::npos) return false;
+    // fopen/CreateFileW both silently strip a trailing dot or space at
-    for (char c : name) {
+    // 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;
 }
 

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 '/', '\\'
+// bare file name only. Rejects empty, ".", the exact ".." component (a name
-// or ':' (which also bans every absolute form — drive, UNC, rooted), and names
+// can only ever be one component, since separators are banned below — a
-// over kMaxEntryNameBytes. Enforced on encode AND decode by package_manifest.
+// 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 formatVersion = 0;
-    std::uint32_t minReaderVersion = kPackageMinReaderVersion;
+    std::uint32_t minReaderVersion = 0;
     std::string writerVersion;
 
     bool operator==(const PackageHeader& o) const {

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<PackageEntry>& entries) {
     for (std::size_t i = 0; i < entries.size(); ++i)
         for (std::size_t j = i + 1; j < entries.size(); ++j)
@@ -155,8 +152,8 @@ 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;
@@ -166,6 +163,8 @@ bool parseManifest(json::Reader& r, PackageManifest& m) {
             } 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(']')) {
@@ -184,8 +183,9 @@ bool parseManifest(json::Reader& r, PackageManifest& m) {
         } while (r.consume(','));
 
         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);
 }
 

tests/test_bank_package.cpp

@@ -95,13 +95,15 @@ static void appendManifest(std::vector<std::uint8_t>& 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<std::uint8_t> bytes =
         rawHeader(kPackageFormatVersion + 1, kPackageMinReaderVersion, "1.9.0");
     appendManifest(bytes, manifest);

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<b>.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");

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();