From 043558a54dab4d740e33036471d4df183d13d158 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 07:27:43 -0400 Subject: [PATCH 1/5] =?UTF-8?q?Land=20src/core/package:=20the=20pure=20RSB?= =?UTF-8?q?K=20container=20=E2=80=94=20format=20ladder,=20JSON=20manifest,?= =?UTF-8?q?=20framing/layout=20codec?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two-integer ladder (formatVersion/minReaderVersion), bare-name-only entries validated on encode and decode, prefix decode that proves exact file size without ever reading a payload. --- CMakeLists.txt | 1 + src/core/package/CLAUDE.md | 78 ++++++ src/core/package/CMakeLists.txt | 14 ++ src/core/package/bank_package.cpp | 139 +++++++++++ src/core/package/bank_package.h | 71 ++++++ src/core/package/package_format.cpp | 23 ++ src/core/package/package_format.h | 80 ++++++ src/core/package/package_manifest.cpp | 201 +++++++++++++++ src/core/package/package_manifest.h | 56 +++++ tests/test_bank_package.cpp | 344 ++++++++++++++++++++++++++ tests/test_package_format.cpp | 96 +++++++ tests/test_package_manifest.cpp | 248 +++++++++++++++++++ 12 files changed, 1351 insertions(+) create mode 100644 src/core/package/CLAUDE.md create mode 100644 src/core/package/CMakeLists.txt create mode 100644 src/core/package/bank_package.cpp create mode 100644 src/core/package/bank_package.h create mode 100644 src/core/package/package_format.cpp create mode 100644 src/core/package/package_format.h create mode 100644 src/core/package/package_manifest.cpp create mode 100644 src/core/package/package_manifest.h create mode 100644 tests/test_bank_package.cpp create mode 100644 tests/test_package_format.cpp create mode 100644 tests/test_package_manifest.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index f783871..ed9f193 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -92,3 +92,4 @@ 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/package/CLAUDE.md b/src/core/package/CLAUDE.md new file mode 100644 index 0000000..cb56e08 --- /dev/null +++ b/src/core/package/CLAUDE.md @@ -0,0 +1,78 @@ +# src/core/package — the pure RSBK bank-package codec + +## Scope + +The hand-rolled `RSBK` bank-package container, entirely pure (REAPER-free, +unit-tested outside the DAW): the format contract and version ladder, the JSON +manifest, and the framing/layout codec. No filesystem — the shell +(`src/shell/package`) streams bytes against the layouts produced here. The +export/import *decisions* (`export_plan` / `import_plan`) are separate modules +landing after the format. + +## Invariants + +- **The container is the proprietary `RSBK` — ruled, not revisitable here.** No + ZIP, no compressor, no link edge to `vendor/WDL/WDL/zlib/`. The version + ladder, not a format swap, is how the format moves. +- **Two version integers, two jobs.** `formatVersion` = what the writer + emitted; `minReaderVersion` = the oldest reader that can read it safely. The + reader's whole rule is `minReaderVersion <= kPackageFormatVersion`. Additive + changes (a new optional manifest key, a new enum value with a defined + degrade) bump `formatVersion` only; structural changes bump both. The full + ladder lives as a comment in `package_format.h` and is READ and validated, + never merely written. +- **TooNew refuses whole.** A `minReaderVersion` above this build yields the + 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 `..`, 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. +- **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 + observed file size exactly, so truncation and trailing garbage are Malformed + without any payload being read. +- **Per-sample shape has one owner.** Each entry nests a one-sample `BankModel` + blob emitted/parsed by `bank_model`'s own codec (the `bank_book_json` + precedent), so a future `Sample` field reaches packages with no change here. +- **Hostile input: error signaled, never UB** — the `bank_model.h` deserialize + standard, plus allocation caps on every length field so a forged header + cannot demand gigabytes. + +## Modules + +- `package_format` — the contract: magic, `kPackageFormatVersion` / + `kPackageMinReaderVersion`, the ladder comment, the three-way + `classifyPackageVersion` (`Readable` / `TooNew` / `Malformed`), the + entry-name rule, 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 + chunks and cannot answer "did these bytes survive") — the digest is carried + here, computed where payloads are streamed (shell). The bank's `slot_map` + rides along. Unknown keys skip at every level; duplicate entry names are + rejected both ways. +- `bank_package` — framing and arithmetic composing the two above: + `encodePackage` (prefix bytes + layout + total size, stamping this build's + ladder pair and `version::stampVersion()`), `decodePackage` (prefix + observed + file size in; header/manifest/layout out), and `requiredPrefixSize` (the + incremental-read seam for the shell). Framing rides `core/wire/bytes.h`. + +## Gotchas + +- Enums nested inside the `BankModel` blob follow `bank_model`'s own rule — an + out-of-range `sourceMode`/`tier` REJECTS the parse — so growing one of those + vocabularies is a `minReaderVersion` bump for packages, not an additive + change. Any enum integer the manifest itself ever adds must instead follow + the degrade-to-`Unknown` rule (`core/wire`'s `BakeStatus` precedent) to stay + additive. The manifest carries no enum of its own today. +- Sample-id rules (remap, collision, dedup across the destination) are + deliberately NOT enforced by the codec — they are `import_plan` decisions. The + codec rejects only what makes the container itself incoherent (duplicate + entry names, invalid names, a non-single-sample nested index). +- `requiredPrefixSize` trusts fields beyond the frozen region only when the + 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. diff --git a/src/core/package/CMakeLists.txt b/src/core/package/CMakeLists.txt new file mode 100644 index 0000000..fc7fd2d --- /dev/null +++ b/src/core/package/CMakeLists.txt @@ -0,0 +1,14 @@ +reasampler_pure_library(package_format SOURCES package_format.cpp) +reasampler_test(package_format LINK package_format) + +reasampler_pure_library(package_manifest + SOURCES package_manifest.cpp + LINK PUBLIC bank_model slot_map PRIVATE package_format json) +reasampler_test(package_manifest LINK package_manifest) + +# bytes.h is header-only (see src/core/wire/CLAUDE.md) — no wire link edge needed. +reasampler_pure_library(bank_package + SOURCES bank_package.cpp + LINK PUBLIC package_format package_manifest PRIVATE app_version) +# app_version: the tests pin the stamped writer semver against stampVersion(). +reasampler_test(bank_package LINK bank_package app_version) diff --git a/src/core/package/bank_package.cpp b/src/core/package/bank_package.cpp new file mode 100644 index 0000000..b5a9389 --- /dev/null +++ b/src/core/package/bank_package.cpp @@ -0,0 +1,139 @@ +#include "core/package/bank_package.h" + +#include + +#include "core/version/app_version.h" +#include "core/wire/bytes.h" + +// Byte offsets (format 1, see package_format.h's ladder): magic at 0, u32 +// formatVersion at 4, u32 minReaderVersion at 8, u32 semver length W at 12, +// semver at 16, u32 manifest length M at 16+W, manifest at 20+W, payloads at +// 20+W+M. The region through the semver is the FROZEN refusal surface. + +namespace reasampler::package { + +namespace { + +constexpr std::size_t kMagicBytes = 4; +constexpr std::size_t kSemverLenAt = 12; +constexpr std::size_t kSemverAt = 16; + +bool magicMatches(const std::vector& bytes) { + return bytes.size() >= kMagicBytes && + std::memcmp(bytes.data(), kPackageMagic, kMagicBytes) == 0; +} + +std::uint32_t u32At(const std::vector& bytes, std::size_t at) { + std::uint32_t v = 0; + for (std::size_t b = 0; b < 4; ++b) + v |= static_cast(bytes[at + b]) << (b * 8); + return v; +} + +// Appends the payload spans for `entries` starting at `firstOffset`. False on +// u64 overflow (a forged length field summing past 2^64 must not wrap into a +// plausible layout). +bool appendSpans(const std::vector& entries, std::uint64_t firstOffset, + std::vector& out, std::uint64_t& end) { + std::uint64_t offset = firstOffset; + for (const auto& e : entries) { + out.push_back({e.fileName, offset, e.byteLength}); + if (offset + e.byteLength < offset) return false; + offset += e.byteLength; + } + end = offset; + return true; +} + +} // namespace + +std::optional encodePackage(const PackageManifest& m) { + auto manifestJson = serializeManifest(m); + if (!manifestJson) return std::nullopt; + if (manifestJson->size() > kMaxManifestBytes) return std::nullopt; + + const std::string& writer = version::stampVersion(); + if (writer.size() > kMaxWriterVersionBytes) return std::nullopt; + + EncodedPackage enc; + auto& out = enc.prefix; + out.insert(out.end(), kPackageMagic, kPackageMagic + kMagicBytes); + wire::putLE(out, kPackageFormatVersion); + wire::putLE(out, kPackageMinReaderVersion); + wire::putLE(out, static_cast(writer.size())); + out.insert(out.end(), writer.begin(), writer.end()); + wire::putLE(out, static_cast(manifestJson->size())); + out.insert(out.end(), manifestJson->begin(), manifestJson->end()); + + if (!appendSpans(m.entries, out.size(), enc.layout, enc.totalSize)) + return std::nullopt; + return enc; +} + +DecodedPackage decodePackage(const std::vector& prefix, + std::uint64_t totalFileSize) { + DecodedPackage dec; // status starts Malformed; every early return means it + + wire::ByteReader r(prefix); + if (r.str(kMagicBytes) != std::string(kPackageMagic, kMagicBytes)) return dec; + + const std::uint32_t formatVersion = r.u32(); + const std::uint32_t minReader = r.u32(); + if (!r.ok) return dec; + const PackageReadability verdict = classifyPackageVersion(formatVersion, minReader); + if (verdict == PackageReadability::Malformed) return dec; + + const std::uint32_t semverLen = r.u32(); + if (!r.ok || semverLen > kMaxWriterVersionBytes) return dec; + std::string writer = r.str(semverLen); + if (!r.ok) return dec; + + dec.header = PackageHeader{formatVersion, minReader, std::move(writer)}; + if (verdict == PackageReadability::TooNew) { + // Refuse whole: the header names the writer for the message; nothing + // past the frozen region is read, and no manifest is produced. + dec.status = PackageReadability::TooNew; + return dec; + } + + const std::uint32_t manifestLen = r.u32(); + if (!r.ok || manifestLen > kMaxManifestBytes) return dec; + const std::string manifestJson = r.str(manifestLen); + if (!r.ok) return dec; + + auto manifest = deserializeManifest(manifestJson); + if (!manifest) return dec; + + std::uint64_t end = 0; + std::vector layout; + if (!appendSpans(manifest->entries, r.pos, layout, end)) return dec; + // Exact-size proof: a byte missing (truncation) or a byte extra (trailing + // garbage) both fail, even though no payload is read here. + if (end != totalFileSize) return dec; + + dec.status = PackageReadability::Readable; + dec.manifest = std::move(*manifest); + dec.layout = std::move(layout); + dec.prefixSize = r.pos; + return dec; +} + +std::optional requiredPrefixSize(const std::vector& bytes) { + if (bytes.size() < kSemverAt) return kSemverAt; + if (!magicMatches(bytes)) return std::nullopt; + + const auto verdict = classifyPackageVersion(u32At(bytes, 4), u32At(bytes, 8)); + if (verdict == PackageReadability::Malformed) return std::nullopt; + + const std::uint32_t semverLen = u32At(bytes, kSemverLenAt); + if (semverLen > kMaxWriterVersionBytes) return std::nullopt; + const std::uint64_t throughSemver = kSemverAt + semverLen; + if (verdict == PackageReadability::TooNew) return throughSemver; + + if (bytes.size() < throughSemver + 4) return throughSemver + 4; + const std::uint32_t manifestLen = u32At(bytes, static_cast(throughSemver)); + if (manifestLen > kMaxManifestBytes) return std::nullopt; + return throughSemver + 4 + manifestLen; +} + +} // namespace reasampler::package diff --git a/src/core/package/bank_package.h b/src/core/package/bank_package.h new file mode 100644 index 0000000..5fb0e94 --- /dev/null +++ b/src/core/package/bank_package.h @@ -0,0 +1,71 @@ +#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. + +#include +#include +#include +#include + +#include "core/package/package_format.h" +#include "core/package/package_manifest.h" + +namespace reasampler::package { + +// Where one payload sits in the finished package file (absolute byte offset). +struct PackageEntrySpan { + std::string name; + std::uint64_t offset = 0; + std::uint64_t length = 0; + + bool operator==(const PackageEntrySpan& o) const { + return name == o.name && offset == o.offset && length == o.length; + } +}; + +// The write side's product: the prefix bytes (magic through manifest, written +// verbatim as the file's head), the layout to stream each payload at, and the +// finished file's exact size — what the shell verifies after the last append. +struct EncodedPackage { + std::vector prefix; + std::vector layout; + std::uint64_t totalSize = 0; +}; + +// Encodes the package prefix for `m`, stamping this build's version pair and +// version::stampVersion() as the writer semver. nullopt when the manifest +// cannot be represented (serializeManifest's rejections) — refused on encode so +// an undecodable package is never written. +std::optional encodePackage(const PackageManifest& m); + +// The read side's product. header is meaningful for Readable and TooNew (a +// refusal must still name the writer); manifest, layout, and prefixSize only +// for Readable — TooNew produces NO manifest, so a refused decode cannot +// half-succeed. +struct DecodedPackage { + PackageReadability status = PackageReadability::Malformed; + PackageHeader header; + PackageManifest manifest; + std::vector layout; + std::uint64_t prefixSize = 0; +}; + +// Decodes a package's leading bytes. `totalFileSize` is the on-disk size the +// caller observed: decode proves prefix + payload lengths equal it exactly, so +// a truncated or garbage-extended file is Malformed even though the payloads +// themselves are never read here. `prefix` may be the whole file or any head of +// it that requiredPrefixSize accepted. Error signaled, never UB. +DecodedPackage decodePackage(const std::vector& prefix, + std::uint64_t totalFileSize); + +// How many leading bytes decodePackage needs. May grow as bytes arrive: with +// fewer than the returned count on hand, read to that count and ask again. +// For a TooNew package it stops at the frozen region (through the writer +// semver) — field positions beyond it belong to the newer format and are not +// trusted. nullopt: these bytes can never frame a package (bad magic, +// incoherent versions, an over-cap length field) — stop reading. +std::optional requiredPrefixSize(const std::vector& bytes); + +} // namespace reasampler::package diff --git a/src/core/package/package_format.cpp b/src/core/package/package_format.cpp new file mode 100644 index 0000000..139163b --- /dev/null +++ b/src/core/package/package_format.cpp @@ -0,0 +1,23 @@ +#include "core/package/package_format.h" + +namespace reasampler::package { + +PackageReadability classifyPackageVersion(std::uint32_t formatVersion, + std::uint32_t minReaderVersion) { + if (formatVersion == 0 || minReaderVersion == 0) return PackageReadability::Malformed; + if (minReaderVersion > formatVersion) return PackageReadability::Malformed; + if (minReaderVersion > kPackageFormatVersion) return PackageReadability::TooNew; + return PackageReadability::Readable; +} + +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 (c == '/' || c == '\\' || c == ':') return false; + } + return true; +} + +} // namespace reasampler::package diff --git a/src/core/package/package_format.h b/src/core/package/package_format.h new file mode 100644 index 0000000..acb70f3 --- /dev/null +++ b/src/core/package/package_format.h @@ -0,0 +1,80 @@ +#pragma once +// package_format — the RSBK bank-package contract: magic, the version ladder, +// the readability classification, and the entry-name rule. Pure: standard +// library only. The framing codec that acts on this contract is bank_package; +// the manifest grammar is package_manifest. + +#include +#include + +namespace reasampler::package { + +// Version ladder for the RSBK container (read-and-validate, like the origin +// ledger's "v"): +// +// format 1 (current) magic "RSBK" | u32 formatVersion | u32 minReaderVersion +// | u32 len + writer semver | u32 len + JSON manifest +// | payloads concatenated in manifest entry order. +// All integers little-endian. +// +// Two integers, two jobs: formatVersion is what the writer emitted (monotonic, +// bumped on ANY change); minReaderVersion is the oldest reader that can read the +// package safely (bumped only on a STRUCTURAL change — a field's meaning shifts, +// a section is removed, framing moves; an additive change — a new optional +// manifest key, a new enum value with a defined degrade — leaves it alone). The +// reader's whole rule: read iff minReaderVersion <= kPackageFormatVersion. +// formatVersion beyond that is message text and log material only. +// +// FROZEN FOR ALL FUTURE VERSIONS: the fields through the writer semver. A +// too-new package must still yield the writer's version so the refusal can name +// what to install — a structural change may rearrange anything after the semver, +// never before it. +inline constexpr char kPackageMagic[4] = {'R', 'S', 'B', 'K'}; +inline constexpr std::uint32_t kPackageFormatVersion = 1; +inline constexpr std::uint32_t kPackageMinReaderVersion = 1; + +// Hostile-input allocation caps (error signaled, never a multi-gigabyte +// allocation off a forged length field). Generous against real content: a +// semver is ~10 bytes; a manifest for hundreds of samples is well under 1 MB. +inline constexpr std::uint32_t kMaxWriterVersionBytes = 64; +inline constexpr std::uint32_t kMaxManifestBytes = 64u * 1024u * 1024u; +inline constexpr std::size_t kMaxEntryNameBytes = 255; + +// The three-way read verdict (the FutureVersion precedent): TooNew refuses the +// whole package before anything is produced; Malformed is a header no honest +// writer emits. Also the status of a full prefix decode in bank_package. +enum class PackageReadability { + Readable, + TooNew, + Malformed, +}; + +// Classify a stored header pair against THIS build's ladder. minReaderVersion +// above kPackageFormatVersion is TooNew; a zero version or minReader > +// formatVersion is Malformed (a writer cannot require a reader newer than what +// it wrote). +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. +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. +struct PackageHeader { + std::uint32_t formatVersion = kPackageFormatVersion; + std::uint32_t minReaderVersion = kPackageMinReaderVersion; + std::string writerVersion; + + bool operator==(const PackageHeader& o) const { + return formatVersion == o.formatVersion && + minReaderVersion == o.minReaderVersion && + writerVersion == o.writerVersion; + } +}; + +} // namespace reasampler::package diff --git a/src/core/package/package_manifest.cpp b/src/core/package/package_manifest.cpp new file mode 100644 index 0000000..10cc267 --- /dev/null +++ b/src/core/package/package_manifest.cpp @@ -0,0 +1,201 @@ +#include "core/package/package_manifest.h" + +#include + +#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 { + +using json::numToStr; +using ObjWriter = json::Writer; + +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; + return false; +} + +// The one-sample BankModel image of `s` — bank_model's own writer, verbatim, so +// the per-sample shape has exactly one owner. nullopt when add() would reject +// the record (its guards are the format's guards too). +std::optional nestSample(const model::Sample& s) { + model::BankModel one; + if (one.add(s) != model::AddResult::Added) return std::nullopt; + return one.serialize(); +} + +} // namespace + +bool PackageEntry::operator==(const PackageEntry& o) const { + return fileName == o.fileName && byteLength == o.byteLength && + byteHash == o.byteHash && sample == o.sample; +} + +bool PackageManifest::operator==(const PackageManifest& o) const { + return bankDisplayName == o.bankDisplayName && exportTimestamp == o.exportTimestamp && + entries == o.entries && slots == o.slots; +} + +std::optional serializeManifest(const PackageManifest& m) { + for (const auto& e : m.entries) + if (!isValidEntryName(e.fileName)) return std::nullopt; + if (duplicateName(m.entries)) return std::nullopt; + + std::string out; + { + ObjWriter root(out); + root.keyStr("bankName", m.bankDisplayName); + root.keyRaw("exported", numToStr(m.exportTimestamp)); + + root.keyBegin("entries"); + out += '['; + for (std::size_t i = 0; i < m.entries.size(); ++i) { + const auto& e = m.entries[i]; + auto nested = nestSample(e.sample); + if (!nested) return std::nullopt; + if (i) out += ','; + ObjWriter w(out); + w.keyStr("name", e.fileName); + // byteLength rides as a signed decimal; 2^63 bytes is beyond any file. + w.keyRaw("length", numToStr(static_cast(e.byteLength))); + w.keyStr("hash", e.byteHash); + w.keyRaw("index", *nested); + } + out += ']'; + + root.keyBegin("slots"); + out += m.slots.serialize(); + } // root closes here (NRVO note in json::Writer) + return out; +} + +namespace { + +// Mirrors bank_book_json's private slots parser: [{id, slot}, ...] pairs handed +// to SlotMap::fromEntries, which owns the defensive repair rules. +bool parseSlots(json::Reader& r, model::SlotMap& out) { + std::vector> pairs; + if (!r.consume('[')) return false; + r.skipWs(); + if (r.consume(']')) { + out = model::SlotMap::fromEntries(pairs); + return true; + } + do { + if (!r.consume('{')) return false; + std::string id; + int slot = 0; + bool haveId = false, haveSlot = false; + do { + std::string k; + if (!r.parseKey(k)) return false; + if (k == "id") { if (!r.parseString(id)) return false; haveId = true; } + else if (k == "slot") { if (!r.parseInt(slot)) return false; haveSlot = true; } + else { if (!r.skipValue()) return false; } + } while (r.consume(',')); + if (!r.consume('}')) return false; + if (!haveId || !haveSlot) return false; + pairs.emplace_back(std::move(id), slot); + } while (r.consume(',')); + if (!r.consume(']')) return false; + out = model::SlotMap::fromEntries(pairs); + return true; +} + +bool parseEntry(json::Reader& r, PackageEntry& e) { + if (!r.consume('{')) return false; + r.skipWs(); + if (r.consume('}')) return false; // an entry needs all four fields + + bool haveName = false, haveLength = false, haveHash = false, haveSample = false; + do { + std::string key; + if (!r.parseKey(key)) return false; + + if (key == "name") { + if (!r.parseString(e.fileName)) return false; + haveName = true; + } else if (key == "length") { + std::int64_t v = 0; + if (!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; + haveHash = true; + } else if (key == "index") { + std::string raw; + if (!r.captureValue(raw)) return false; + auto idx = model::BankModel::deserialize(raw); + // Exactly one sample: add()'s silent drop (rejected record) or a + // multi-sample blob both fail the entry rather than half-parse. + if (!idx || idx->size() != 1) return false; + e.sample = idx->all().front(); + haveSample = true; + } else { + if (!r.skipValue()) return false; // forward-compat unknown keys + } + } while (r.consume(',')); + + if (!r.consume('}')) return false; + if (!haveName || !haveLength || !haveHash || !haveSample) return false; + return isValidEntryName(e.fileName); +} + +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 + + 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; + } + } else if (key == "slots") { + if (!parseSlots(r, m.slots)) return false; + } else { + if (!r.skipValue()) return false; // forward-compat unknown keys + } + } while (r.consume(',')); + + if (!r.consume('}')) return false; + r.skipWs(); + if (!r.eof()) return false; // trailing garbage + return !duplicateName(m.entries); +} + +} // namespace + +std::optional deserializeManifest(const std::string& json) { + PackageManifest m; + json::Reader r(json); + if (!parseManifest(r, m)) return std::nullopt; + return m; +} + +} // namespace reasampler::package diff --git a/src/core/package/package_manifest.h b/src/core/package/package_manifest.h new file mode 100644 index 0000000..3afe70d --- /dev/null +++ b/src/core/package/package_manifest.h @@ -0,0 +1,56 @@ +#pragma once +// package_manifest — the RSBK manifest model and its JSON codec. Per-sample +// shape is NOT owned here: each entry nests a one-sample BankModel blob emitted +// by bank_model's own writer (the bank_book_json precedent), so a future Sample +// field reaches packages for free. Pure: no filesystem, no host types. + +#include +#include +#include +#include + +#include "core/model/bank_model.h" +#include "core/model/slot_map.h" + +namespace reasampler::package { + +// One payload's transport record. `byteHash` is capture::hashBytes over the +// payload's raw bytes — the whole-file digest, deliberately NOT hashWavContent +// (which skips chunks and so cannot answer "did these bytes survive the trip"). +// FNV-1a: a corruption detector, not a cryptographic checksum. The codec only +// carries the digest; hashing happens where the payload is streamed (shell). +struct PackageEntry { + std::string fileName; // bare name inside the package (isValidEntryName) + std::uint64_t byteLength = 0; + std::string byteHash; + model::Sample sample; + + bool operator==(const PackageEntry& o) const; +}; + +// Everything the manifest carries besides the payloads: informational envelope +// (source bank name, export moment), the entries, and the bank's display +// positions (a bank's arrangement is part of what the user built). +struct PackageManifest { + std::string bankDisplayName; + std::int64_t exportTimestamp = 0; // unix epoch seconds + std::vector entries; + model::SlotMap slots; + + bool operator==(const PackageManifest& o) const; +}; + +// Emits the manifest JSON. nullopt when the manifest cannot be represented: +// an invalid or duplicate entry name, 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. +std::optional deserializeManifest(const std::string& json); + +} // namespace reasampler::package diff --git a/tests/test_bank_package.cpp b/tests/test_bank_package.cpp new file mode 100644 index 0000000..5279bd7 --- /dev/null +++ b/tests/test_bank_package.cpp @@ -0,0 +1,344 @@ +// Standalone tests for reasampler::package::bank_package — no REAPER, no test +// framework. Byte-level suites hand-roll RSBK images with wire::putLE rather +// than calling encodePackage, so a layout regression in encode cannot hide from +// decode (the two sides are pinned against each other AND against raw bytes). + +#include "../src/core/package/bank_package.h" + +#include +#include +#include +#include + +#include "../src/core/version/app_version.h" +#include "../src/core/wire/bytes.h" + +using namespace reasampler::package; +using namespace reasampler::model; +namespace wire = reasampler::wire; +namespace version = reasampler::version; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +// --- fixtures ---------------------------------------------------------------- + +// Every Sample field populated, every optional PRESENT. +static Sample fullSample() { + Sample s; + s.id = "smp-full"; + s.displayName = "Kick (wet)"; + s.relativePath = "reasampler_bank/kick.wav"; + s.sourceMode = SourceMode::RazorArea; + s.sourceRange = {1.25, 3.5, 480.0, 1920.0}; + s.trackGuids = {"{AAA}", "{BBB}"}; + s.wetDry = 0.75; + s.channelCount = 2; + s.sampleRate = 48000; + s.lengthSeconds = 2.25; + s.lengthBeats = 4.5; + s.captureTempo = 120.5; + s.captureTimeSigNum = 7; + s.captureTimeSigDenom = 8; + s.key = "F#m"; + s.rootNote = 60; + s.loop = LoopPoints{100, 4800}; + s.levels = {-0.3, -12.7, -14.0}; + s.clipped = true; + s.tier = Tier::Archive; + s.contentHash = "W0123456789abcdef"; + s.provenance = Provenance{"smp-parent", "fx-snapshot"}; + s.createdTimestamp = 1754000000; + return s; +} + +// Every Sample optional ABSENT (key, rootNote, loop, provenance). +static Sample bareSample() { + Sample s; + s.id = "smp-bare"; + s.displayName = "Snare"; + s.relativePath = "reasampler_bank/snare.wav"; + s.sourceMode = SourceMode::Realtime; + s.contentHash = "Wfedcba9876543210"; + s.createdTimestamp = 1754000001; + return s; +} + +static PackageManifest fixture(std::uint64_t len0, std::uint64_t len1) { + PackageManifest m; + m.bankDisplayName = "Drums \"live\""; + m.exportTimestamp = 1754100000; + m.entries.push_back({"kick.wav", len0, "1111222233334444", fullSample()}); + m.entries.push_back({"snare.wav", len1, "5555666677778888", bareSample()}); + m.slots.append("smp-full"); + m.slots.append("smp-bare"); + return m; +} + +// A hand-rolled RSBK image: frozen region + a raw tail (manifest framing or +// deliberate garbage), independent of encodePackage. +static std::vector rawHeader(std::uint32_t fv, std::uint32_t mv, + const std::string& semver) { + std::vector out; + out.insert(out.end(), kPackageMagic, kPackageMagic + 4); + wire::putLE(out, fv); + wire::putLE(out, mv); + wire::putLE(out, static_cast(semver.size())); + out.insert(out.end(), semver.begin(), semver.end()); + return out; +} + +static void appendManifest(std::vector& out, const std::string& json) { + wire::putLE(out, static_cast(json.size())); + out.insert(out.end(), json.begin(), json.end()); +} + +// One-entry manifest JSON with `extra` spliced in as additional root content +// ("" for none) — for images a current writer would never emit. +static std::string handManifest(const std::string& name, int length, + const std::string& extra) { + return std::string("{") + extra + + "\"entries\":[{\"name\":\"" + name + + "\",\"length\":" + std::to_string(length) + ",\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\"," + "\"relativePath\":\"bank/a.wav\"}]}}]}"; +} + +// --- encode / decode round trip ---------------------------------------------- + +static void testEncodeDecodeRoundTrip() { + const PackageManifest m = fixture(96000, 0); // a zero-length payload is legal + auto enc = encodePackage(m); + CHECK(enc.has_value()); + + // Layout arithmetic: payloads start at the prefix end, in manifest order. + CHECK(enc->layout.size() == 2); + CHECK(enc->layout[0].name == "kick.wav"); + CHECK(enc->layout[0].offset == enc->prefix.size()); + CHECK(enc->layout[0].length == 96000); + CHECK(enc->layout[1].offset == enc->prefix.size() + 96000); + CHECK(enc->layout[1].length == 0); + CHECK(enc->totalSize == enc->prefix.size() + 96000); + + // decodePackage(encodePackage(x)) == x — payloads are never read by the + // codec, so the prefix plus the true total size is the whole input. + const DecodedPackage dec = decodePackage(enc->prefix, enc->totalSize); + CHECK(dec.status == PackageReadability::Readable); + CHECK(dec.manifest == m); + CHECK(dec.layout == enc->layout); + CHECK(dec.prefixSize == enc->prefix.size()); + + // The header stamps this build's ladder pair and its informational semver. + CHECK(dec.header.formatVersion == kPackageFormatVersion); + CHECK(dec.header.minReaderVersion == kPackageMinReaderVersion); + CHECK(dec.header.writerVersion == version::stampVersion()); +} + +static void testEncodeRefusesWhatManifestRefuses() { + PackageManifest m = fixture(1, 1); + m.entries[0].fileName = "../evil.wav"; + CHECK(!encodePackage(m).has_value()); +} + +// --- truncation: every byte offset ------------------------------------------- + +static void testTruncationAtEveryByteOffsetIsMalformed() { + const PackageManifest m = fixture(3, 5); + auto enc = encodePackage(m); + CHECK(enc.has_value()); + + // The complete on-disk image: prefix + both payloads. + std::vector file = enc->prefix; + for (std::uint8_t b : {1, 2, 3, 10, 20, 30, 40, 50}) file.push_back(b); + CHECK(file.size() == enc->totalSize); + CHECK(decodePackage(file, file.size()).status == PackageReadability::Readable); + + for (std::size_t cut = 0; cut < file.size(); ++cut) { + const std::vector truncated(file.begin(), file.begin() + cut); + const DecodedPackage dec = decodePackage(truncated, truncated.size()); + if (dec.status != PackageReadability::Malformed) { + std::printf("FAIL: truncation at %zu not Malformed\n", cut); + ++g_fail; + break; + } + // A refused decode must not half-succeed at any cut either. + CHECK(dec.manifest.entries.empty()); + CHECK(dec.layout.empty()); + } + + // One byte extra (trailing garbage) is as Malformed as one byte missing. + std::vector extended = file; + extended.push_back(0); + CHECK(decodePackage(extended, extended.size()).status == PackageReadability::Malformed); + // And a file size that disagrees with the same bytes. + CHECK(decodePackage(file, file.size() + 1).status == PackageReadability::Malformed); + CHECK(decodePackage(file, file.size() - 1).status == PackageReadability::Malformed); +} + +// --- version ladder: TooNew refuses whole ------------------------------------ + +static void testTooNewProducesNoManifest() { + // A future structural format: only the frozen region is trustworthy, so the + // tail is deliberate garbage that would crash a parser that kept reading. + std::vector bytes = rawHeader(9, 9, "9.9.9"); + for (int i = 0; i < 32; ++i) bytes.push_back(0xFF); + + const DecodedPackage dec = decodePackage(bytes, bytes.size()); + CHECK(dec.status == PackageReadability::TooNew); + // The refusal message's three facts survive... + CHECK(dec.header.formatVersion == 9); + CHECK(dec.header.minReaderVersion == 9); + CHECK(dec.header.writerVersion == "9.9.9"); + // ...and nothing else is produced: no manifest, no layout, no half-success. + CHECK(dec.manifest.entries.empty()); + CHECK(dec.manifest == PackageManifest{}); + CHECK(dec.layout.empty()); + CHECK(dec.prefixSize == 0); + + // Boundary: minReader exactly one past this build. + auto boundary = rawHeader(kPackageFormatVersion + 1, kPackageFormatVersion + 1, "2.0.0"); + CHECK(decodePackage(boundary, boundary.size()).status == PackageReadability::TooNew); + + // A TooNew header truncated inside the frozen region cannot name the + // writer, so it is Malformed, not an unactionable refusal. + std::vector cut = rawHeader(9, 9, "9.9.9"); + cut.resize(18); // mid-semver + CHECK(decodePackage(cut, cut.size()).status == PackageReadability::Malformed); +} + +// --- 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. +static void testNewerAdditiveFormatReads() { + const std::string manifest = handManifest( + "a.wav", 4, + "\"instrumentState\":{\"future\":[1,2,3]},\"anotherNewKey\":\"x\","); + std::vector bytes = + rawHeader(kPackageFormatVersion + 1, kPackageMinReaderVersion, "1.9.0"); + appendManifest(bytes, manifest); + const std::uint64_t total = bytes.size() + 4; // the one entry's payload + + const DecodedPackage dec = decodePackage(bytes, total); + CHECK(dec.status == PackageReadability::Readable); + CHECK(dec.header.formatVersion == kPackageFormatVersion + 1); + CHECK(dec.header.writerVersion == "1.9.0"); + CHECK(dec.manifest.entries.size() == 1); + CHECK(dec.manifest.entries[0].fileName == "a.wav"); + CHECK(dec.manifest.entries[0].sample.id == "s1"); + CHECK(dec.layout.size() == 1); + CHECK(dec.layout[0].offset == bytes.size()); + CHECK(dec.layout[0].length == 4); +} + +// --- hostile headers --------------------------------------------------------- + +static void testHostileHeadersAreMalformed() { + // Wrong magic. + std::vector bad = rawHeader(1, 1, "1.0.0"); + appendManifest(bad, "{}"); + bad[0] = 'Z'; + CHECK(decodePackage(bad, bad.size()).status == PackageReadability::Malformed); + + // Incoherent version pairs (the classify rules, proven through the framing). + for (auto [fv, mv] : {std::pair{0, 0}, {0, 1}, + {1, 0}, {1, 2}}) { + std::vector b = rawHeader(fv, mv, "1.0.0"); + appendManifest(b, "{}"); + CHECK(decodePackage(b, b.size()).status == PackageReadability::Malformed); + } + + // A forged semver length over the cap must not read past the buffer. + std::vector overSemver; + overSemver.insert(overSemver.end(), kPackageMagic, kPackageMagic + 4); + wire::putLE(overSemver, std::uint32_t{1}); + wire::putLE(overSemver, std::uint32_t{1}); + wire::putLE(overSemver, kMaxWriterVersionBytes + 1); + CHECK(decodePackage(overSemver, overSemver.size()).status == + PackageReadability::Malformed); + + // A forged manifest length over the cap: refused before any allocation. + std::vector overManifest = rawHeader(1, 1, "1.0.0"); + wire::putLE(overManifest, kMaxManifestBytes + 1); + CHECK(decodePackage(overManifest, overManifest.size()).status == + PackageReadability::Malformed); + + // A manifest whose entry name expresses a path: rejected on decode even + // though no current encoder would write it. + std::vector traversal = rawHeader(1, 1, "1.0.0"); + appendManifest(traversal, handManifest("../evil.wav", 4, "")); + CHECK(decodePackage(traversal, traversal.size() + 4).status == + PackageReadability::Malformed); +} + +// --- requiredPrefixSize ------------------------------------------------------ + +static void testRequiredPrefixSizeGrowsToTheFullPrefix() { + auto enc = encodePackage(fixture(3, 5)); + CHECK(enc.has_value()); + const auto& prefix = enc->prefix; + + // Empty: the fixed region first. + CHECK(requiredPrefixSize({}) == std::uint64_t{16}); + + // With the fixed region: asks through the semver + manifest-length field. + const std::string& semver = version::stampVersion(); + std::vector first16(prefix.begin(), prefix.begin() + 16); + CHECK(requiredPrefixSize(first16) == std::uint64_t{16 + semver.size() + 4}); + + // With that much: the full prefix size. And the answer is a fixpoint. + std::vector upToManifestLen( + prefix.begin(), prefix.begin() + 20 + static_cast(semver.size())); + CHECK(requiredPrefixSize(upToManifestLen) == std::uint64_t{prefix.size()}); + CHECK(requiredPrefixSize(prefix) == std::uint64_t{prefix.size()}); + + // The returned count is exactly enough for decodePackage. + CHECK(decodePackage(prefix, enc->totalSize).status == PackageReadability::Readable); +} + +static void testRequiredPrefixSizeRefusals() { + // Bad magic: stop reading. + std::vector bad(16, 0); + CHECK(!requiredPrefixSize(bad).has_value()); + + // Incoherent versions: stop reading. + std::vector zeroed = rawHeader(0, 0, "1.0.0"); + CHECK(!requiredPrefixSize(zeroed).has_value()); + + // TooNew: asks only through the frozen region — the manifest-length field + // belongs to the newer format and is never trusted. + std::vector tooNew = rawHeader(9, 9, "9.9.9"); + for (int i = 0; i < 8; ++i) tooNew.push_back(0xFF); // garbage where M would be + CHECK(requiredPrefixSize(tooNew) == std::uint64_t{16 + 5}); + + // Oversize length fields: stop reading. + std::vector overSemver; + overSemver.insert(overSemver.end(), kPackageMagic, kPackageMagic + 4); + wire::putLE(overSemver, std::uint32_t{1}); + wire::putLE(overSemver, std::uint32_t{1}); + wire::putLE(overSemver, kMaxWriterVersionBytes + 1); + CHECK(!requiredPrefixSize(overSemver).has_value()); + + std::vector overManifest = rawHeader(1, 1, "1.0.0"); + wire::putLE(overManifest, kMaxManifestBytes + 1); + CHECK(!requiredPrefixSize(overManifest).has_value()); +} + +int main() { + testEncodeDecodeRoundTrip(); + testEncodeRefusesWhatManifestRefuses(); + testTruncationAtEveryByteOffsetIsMalformed(); + testTooNewProducesNoManifest(); + testNewerAdditiveFormatReads(); + testHostileHeadersAreMalformed(); + testRequiredPrefixSizeRefusals(); + testRequiredPrefixSizeGrowsToTheFullPrefix(); + + if (g_fail == 0) { + std::printf("bank_package_tests: all passed\n"); + return 0; + } + std::printf("bank_package_tests: %d failure(s)\n", g_fail); + return 1; +} diff --git a/tests/test_package_format.cpp b/tests/test_package_format.cpp new file mode 100644 index 0000000..442d2ef --- /dev/null +++ b/tests/test_package_format.cpp @@ -0,0 +1,96 @@ +// 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. + +#include "../src/core/package/package_format.h" + +#include +#include + +using namespace reasampler::package; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +// --- classifyPackageVersion -------------------------------------------------- + +static void testClassifyReadable() { + CHECK(classifyPackageVersion(kPackageFormatVersion, kPackageMinReaderVersion) == + PackageReadability::Readable); + // The additive-forward-compat direction: a newer writer whose minReader + // still reaches back to this build reads fine. + CHECK(classifyPackageVersion(kPackageFormatVersion + 5, kPackageMinReaderVersion) == + PackageReadability::Readable); + // Boundary: minReader exactly this build's format version. + CHECK(classifyPackageVersion(kPackageFormatVersion + 1, kPackageFormatVersion) == + PackageReadability::Readable); +} + +static void testClassifyTooNew() { + // Boundary: one past this build's format version refuses. + CHECK(classifyPackageVersion(kPackageFormatVersion + 1, kPackageFormatVersion + 1) == + PackageReadability::TooNew); + CHECK(classifyPackageVersion(99, 42) == PackageReadability::TooNew); +} + +static void testClassifyMalformed() { + // Zero versions: no honest writer emits them (the ladder starts at 1). + CHECK(classifyPackageVersion(0, 0) == PackageReadability::Malformed); + CHECK(classifyPackageVersion(1, 0) == PackageReadability::Malformed); + CHECK(classifyPackageVersion(0, 1) == PackageReadability::Malformed); + // A writer cannot require a reader newer than what it wrote. + CHECK(classifyPackageVersion(1, 2) == PackageReadability::Malformed); + // Incoherence outranks TooNew: even with both above this build, minReader > + // formatVersion is Malformed, not a refusal message. + CHECK(classifyPackageVersion(5, 9) == PackageReadability::Malformed); +} + +// --- isValidEntryName -------------------------------------------------------- + +static void testEntryNameAccepts() { + CHECK(isValidEntryName("kick.wav")); + CHECK(isValidEntryName("Snare 03 (wet).wav")); + CHECK(isValidEntryName("no-extension")); + 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 +} + +static void testEntryNameRejectsSeparatorsAndDots() { + CHECK(!isValidEntryName("")); + CHECK(!isValidEntryName(".")); + 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")); +} + +static void testEntryNameRejectsAbsolutePrefixes() { + CHECK(!isValidEntryName("C:\\abs.wav")); + CHECK(!isValidEntryName("C:/abs.wav")); + CHECK(!isValidEntryName("c:relative-to-drive.wav")); // ':' bans drive forms + CHECK(!isValidEntryName("\\\\server\\share.wav")); // UNC + CHECK(!isValidEntryName(std::string(kMaxEntryNameBytes + 1, 'x'))); // over cap +} + +int main() { + testClassifyReadable(); + testClassifyTooNew(); + testClassifyMalformed(); + testEntryNameAccepts(); + testEntryNameRejectsSeparatorsAndDots(); + testEntryNameRejectsAbsolutePrefixes(); + + if (g_fail == 0) { + std::printf("package_format_tests: all passed\n"); + return 0; + } + std::printf("package_format_tests: %d failure(s)\n", g_fail); + return 1; +} diff --git a/tests/test_package_manifest.cpp b/tests/test_package_manifest.cpp new file mode 100644 index 0000000..c9ecbde --- /dev/null +++ b/tests/test_package_manifest.cpp @@ -0,0 +1,248 @@ +// 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. + +#include "../src/core/package/package_manifest.h" + +#include +#include +#include + +using namespace reasampler::package; +using namespace reasampler::model; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +// --- fixtures ---------------------------------------------------------------- + +// Every Sample field populated, every optional PRESENT. +static Sample fullSample() { + Sample s; + s.id = "smp-full"; + s.displayName = "Kick (wet)"; + s.relativePath = "reasampler_bank/kick.wav"; + s.sourceMode = SourceMode::RazorArea; + s.sourceRange = {1.25, 3.5, 480.0, 1920.0}; + s.trackGuids = {"{AAA}", "{BBB}"}; + s.wetDry = 0.75; + s.channelCount = 2; + s.sampleRate = 48000; + s.lengthSeconds = 2.25; + s.lengthBeats = 4.5; + s.captureTempo = 120.5; + s.captureTimeSigNum = 7; + s.captureTimeSigDenom = 8; + s.key = "F#m"; + s.rootNote = 60; + s.loop = LoopPoints{100, 4800}; + s.levels = {-0.3, -12.7, -14.0}; + s.clipped = true; + s.tier = Tier::Archive; + s.contentHash = "W0123456789abcdef"; + s.provenance = Provenance{"smp-parent", "fx-snapshot"}; + s.createdTimestamp = 1754000000; + return s; +} + +// Every Sample optional ABSENT (key, rootNote, loop, provenance). +static Sample bareSample() { + Sample s; + s.id = "smp-bare"; + s.displayName = "Snare"; + s.relativePath = "reasampler_bank/snare.wav"; + s.sourceMode = SourceMode::Realtime; + s.contentHash = "Wfedcba9876543210"; + s.createdTimestamp = 1754000001; + return s; +} + +static PackageManifest fixture() { + PackageManifest m; + m.bankDisplayName = "Drums \"live\""; // escaping exercised + m.exportTimestamp = 1754100000; + m.entries.push_back({"kick.wav", 96000, "1111222233334444", fullSample()}); + m.entries.push_back({"snare.wav", 0, "5555666677778888", bareSample()}); // 0-length legal + m.slots.append("smp-full"); + m.slots.append("smp-bare"); + m.slots.remove("smp-full"); // leaves a gap: slots round-trip must keep it + return m; +} + +// --- round trip -------------------------------------------------------------- + +static void testRoundTripEveryField() { + const PackageManifest m = fixture(); + auto json = serializeManifest(m); + CHECK(json.has_value()); + auto back = deserializeManifest(*json); + CHECK(back.has_value()); + CHECK(*back == m); + // Spot-check both optional states survived (== above proves it; these name + // the claim so a failure reads directly). + CHECK(back->entries[0].sample.loop.has_value()); + CHECK(back->entries[0].sample.provenance.has_value()); + CHECK(!back->entries[1].sample.key.has_value()); + CHECK(!back->entries[1].sample.rootNote.has_value()); + CHECK(back->slots.idAt(0).empty()); // the slot gap survived + CHECK(back->slots.slotOf("smp-bare") == 1); +} + +static void testEmptyManifestRoundTrips() { + PackageManifest m; + auto json = serializeManifest(m); + CHECK(json.has_value()); + auto back = deserializeManifest(*json); + CHECK(back.has_value()); + CHECK(*back == m); +} + +// --- forward compatibility --------------------------------------------------- + +static void testUnknownKeysSkippedAtEveryLevel() { + // A future additive manifest: unknown keys at the root and inside an entry. + 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\"," + "\"relativePath\":\"bank/a.wav\"}]}}]," + "\"slots\":[],\"trailingUnknown\":null}"; + auto m = deserializeManifest(json); + CHECK(m.has_value()); + CHECK(m->bankDisplayName == "B"); + CHECK(m->exportTimestamp == 7); + CHECK(m->entries.size() == 1); + CHECK(m->entries[0].fileName == "a.wav"); + CHECK(m->entries[0].byteLength == 10); + CHECK(m->entries[0].sample.id == "s1"); +} + +// --- rejection: entry names, both directions --------------------------------- + +static void testEncodeRejectsBadEntryName() { + for (const char* bad : {"..\\evil.wav", "../evil.wav", "dir/a.wav", "C:\\a.wav", "", ".."}) { + PackageManifest m = fixture(); + m.entries[0].fileName = bad; + CHECK(!serializeManifest(m).has_value()); + } +} + +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()); + } +} + +static void testDuplicateEntryNamesRejectedBothWays() { + PackageManifest m = fixture(); + m.entries[1].fileName = m.entries[0].fileName; + CHECK(!serializeManifest(m).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\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s2\",\"relativePath\":\"q\"}]}}]}"; + CHECK(!deserializeManifest(dup).has_value()); +} + +// --- rejection: structural --------------------------------------------------- + +static void testEncodeRejectsUnrepresentableSample() { + PackageManifest m = fixture(); + m.entries[0].sample.id.clear(); // BankModel::add rejects an empty id + CHECK(!serializeManifest(m).has_value()); + + PackageManifest m2 = fixture(); + m2.entries[0].sample.relativePath = "C:/abs/kick.wav"; // and an absolute path + CHECK(!serializeManifest(m2).has_value()); +} + +static void testDecodeRejectsMalformedShapes() { + CHECK(!deserializeManifest("").has_value()); + CHECK(!deserializeManifest("not json").has_value()); + CHECK(!deserializeManifest("[]").has_value()); + CHECK(!deserializeManifest("{\"entries\":[{}]}").has_value()); // entry missing fields + // Missing one required entry field apiece. + CHECK(!deserializeManifest( + "{\"entries\":[{\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s\",\"relativePath\":\"p\"}]}}]}") + .has_value()); + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s\",\"relativePath\":\"p\"}]}}]}") + .has_value()); + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s\",\"relativePath\":\"p\"}]}}]}") + .has_value()); + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\"}]}").has_value()); + // Negative length. + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":-1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s\",\"relativePath\":\"p\"}]}}]}") + .has_value()); + // A nested index that is not exactly one sample (zero and two). + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[]}}]}").has_value()); + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s1\",\"relativePath\":\"p\"}," + "{\"id\":\"s2\",\"relativePath\":\"q\"}]}}]}").has_value()); + // A nested sample BankModel::add drops (absolute path) fails the entry — + // the silent drop must not half-parse into an empty index. + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s\"," + "\"relativePath\":\"C:/abs.wav\"}]}}]}").has_value()); + // An out-of-range enum inside the nested Sample blob fails the entry: the + // manifest defines no enum of its own, and bank_model's codec REJECTS an + // unknown sourceMode/tier rather than degrading — so growing one of those + // vocabularies is a minReaderVersion bump, not an additive change (see this + // directory's CLAUDE.md). + CHECK(!deserializeManifest( + "{\"entries\":[{\"name\":\"a.wav\",\"length\":1,\"hash\":\"h\"," + "\"index\":{\"version\":1,\"samples\":[{\"id\":\"s\",\"relativePath\":\"p\"," + "\"sourceMode\":99}]}}]}").has_value()); + // Trailing garbage after the root object. + auto json = serializeManifest(fixture()); + CHECK(json.has_value()); + CHECK(!deserializeManifest(*json + "x").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()); + CHECK(!deserializeManifest(json->substr(0, 1)).has_value()); +} + +int main() { + testRoundTripEveryField(); + testEmptyManifestRoundTrips(); + testUnknownKeysSkippedAtEveryLevel(); + testEncodeRejectsBadEntryName(); + testDecodeRejectsBadEntryName(); + testDuplicateEntryNamesRejectedBothWays(); + testEncodeRejectsUnrepresentableSample(); + testDecodeRejectsMalformedShapes(); + + if (g_fail == 0) { + std::printf("package_manifest_tests: all passed\n"); + return 0; + } + std::printf("package_manifest_tests: %d failure(s)\n", g_fail); + return 1; +} From e0b4ec2e21256b55b25439e31b24bae801f804cd Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 07:56:45 -0400 Subject: [PATCH 2/5] 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 | 1 - src/core/CMakeLists.txt | 1 + src/core/package/CLAUDE.md | 14 ++++-- src/core/package/bank_package.h | 5 +-- src/core/package/package_format.cpp | 35 +++++++++++++-- src/core/package/package_format.h | 22 ++++++--- src/core/package/package_manifest.cpp | 64 +++++++++++++-------------- tests/test_bank_package.cpp | 15 ++++--- tests/test_package_format.cpp | 39 +++++++++++++++- tests/test_package_manifest.cpp | 21 ++++++++- 10 files changed, 161 insertions(+), 56 deletions(-) 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(); From 1aebf519386710abcf58e653a475106e6d12ffde Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 08:14:55 -0400 Subject: [PATCH 3/5] Relabel post-manifest-parse failure as TooNew; refuse zero-length package entries at encode An additively-tagged newer package that fails to parse now reports TooNew (with writer semver) instead of unactionable Malformed. Format layer also refuses encoding a zero-length entry, honoring the shell's appendPayload contract; both test-covered. --- src/core/package/CLAUDE.md | 14 ++++++++++ src/core/package/bank_package.cpp | 8 +++++- src/core/package/package_manifest.cpp | 6 ++++- src/core/package/package_manifest.h | 7 ++--- tests/test_bank_package.cpp | 38 ++++++++++++++++++++++++--- tests/test_package_manifest.cpp | 13 ++++++++- 6 files changed, 77 insertions(+), 9 deletions(-) diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index a8bfe62..373b512 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -79,8 +79,22 @@ 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. +- A package whose header classifies `Readable` (fv > ours, minReader still + within reach — the additive case) but whose manifest fails to parse is + reported `TooNew`, not `Malformed`: the header is valid and already carries + the writer's semver, so the refusal can still name what to install. This + widens `TooNew` to cover "read and failed" as well as "stopped at the frozen + 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 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. +- **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, + not a real entry). `serializeManifest` refuses a zero-length `PackageEntry` + at encode so this layer never produces one; decode does not enforce it (a + hostile/older package declaring one is not this track's concern). diff --git a/src/core/package/bank_package.cpp b/src/core/package/bank_package.cpp index b5a9389..9b0edbe 100644 --- a/src/core/package/bank_package.cpp +++ b/src/core/package/bank_package.cpp @@ -102,7 +102,13 @@ DecodedPackage decodePackage(const std::vector& prefix, if (!r.ok) return dec; auto manifest = deserializeManifest(manifestJson); - if (!manifest) return dec; + if (!manifest) { + // A newer additive format's parse failure reports TooNew, not the + // unactionable Malformed — the header (with the writer semver) is + // already valid here. See this directory's CLAUDE.md for the tradeoff. + if (formatVersion > kPackageFormatVersion) dec.status = PackageReadability::TooNew; + return dec; + } std::uint64_t end = 0; std::vector layout; diff --git a/src/core/package/package_manifest.cpp b/src/core/package/package_manifest.cpp index a4d4ba9..4740b3e 100644 --- a/src/core/package/package_manifest.cpp +++ b/src/core/package/package_manifest.cpp @@ -43,8 +43,12 @@ bool PackageManifest::operator==(const PackageManifest& o) const { } std::optional serializeManifest(const PackageManifest& m) { - for (const auto& e : m.entries) + for (const auto& e : m.entries) { if (!isValidEntryName(e.fileName)) return std::nullopt; + // Cross-module contract with src/shell/package — see this directory's + // CLAUDE.md. + if (e.byteLength == 0) return std::nullopt; + } if (duplicateName(m.entries)) return std::nullopt; std::string out; diff --git a/src/core/package/package_manifest.h b/src/core/package/package_manifest.h index 3afe70d..0b31ce0 100644 --- a/src/core/package/package_manifest.h +++ b/src/core/package/package_manifest.h @@ -41,9 +41,10 @@ struct PackageManifest { }; // Emits the manifest JSON. nullopt when the manifest cannot be represented: -// an invalid or duplicate entry name, or a sample record BankModel itself would -// reject (empty id, absolute path) — refusing on encode so an undecodable -// package is never written. +// 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. std::optional serializeManifest(const PackageManifest& m); // Parses manifest JSON (nullopt on malformed input, never UB). Unknown keys are diff --git a/tests/test_bank_package.cpp b/tests/test_bank_package.cpp index 565ab7c..27f16af 100644 --- a/tests/test_bank_package.cpp +++ b/tests/test_bank_package.cpp @@ -110,7 +110,7 @@ static std::string handManifest(const std::string& name, int length, // --- encode / decode round trip ---------------------------------------------- static void testEncodeDecodeRoundTrip() { - const PackageManifest m = fixture(96000, 0); // a zero-length payload is legal + const PackageManifest m = fixture(96000, 48000); auto enc = encodePackage(m); CHECK(enc.has_value()); @@ -120,8 +120,8 @@ static void testEncodeDecodeRoundTrip() { CHECK(enc->layout[0].offset == enc->prefix.size()); CHECK(enc->layout[0].length == 96000); CHECK(enc->layout[1].offset == enc->prefix.size() + 96000); - CHECK(enc->layout[1].length == 0); - CHECK(enc->totalSize == enc->prefix.size() + 96000); + CHECK(enc->layout[1].length == 48000); + CHECK(enc->totalSize == enc->prefix.size() + 96000 + 48000); // decodePackage(encodePackage(x)) == x — payloads are never read by the // codec, so the prefix plus the true total size is the whole input. @@ -141,6 +141,13 @@ static void testEncodeRefusesWhatManifestRefuses() { PackageManifest m = fixture(1, 1); 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. + PackageManifest zeroLen = fixture(1, 1); + zeroLen.entries[1].byteLength = 0; + CHECK(!encodePackage(zeroLen).has_value()); } // --- truncation: every byte offset ------------------------------------------- @@ -209,6 +216,30 @@ static void testTooNewProducesNoManifest() { CHECK(decodePackage(cut, cut.size()).status == PackageReadability::Malformed); } +// An additively-tagged package (fv > ours, minReader still within reach) whose +// manifest fails to parse: the header classifies Readable, so decode reads +// into the manifest and fails there. That failure must still report TooNew — +// the header is valid and already carries the writer's semver — not the +// unactionable Malformed a genuinely corrupt header produces. +static void testAdditiveUnparseableManifestIsTooNew() { + std::vector bytes = rawHeader(kPackageFormatVersion + 1, kPackageMinReaderVersion, "1.9.0"); + appendManifest(bytes, "not json"); + const DecodedPackage dec = decodePackage(bytes, bytes.size()); + CHECK(dec.status == PackageReadability::TooNew); + CHECK(dec.header.formatVersion == kPackageFormatVersion + 1); + CHECK(dec.header.minReaderVersion == kPackageMinReaderVersion); + CHECK(dec.header.writerVersion == "1.9.0"); + CHECK(dec.manifest.entries.empty()); + CHECK(dec.layout.empty()); + + // Same-version unparseable manifest stays Malformed: nothing "newer" + // excuses it, so this is not a blanket "unparseable == TooNew" rule. + std::vector sameVersion = + rawHeader(kPackageFormatVersion, kPackageMinReaderVersion, "1.0.0"); + appendManifest(sameVersion, "not json"); + CHECK(decodePackage(sameVersion, sameVersion.size()).status == PackageReadability::Malformed); +} + // --- version ladder: additive forward compatibility -------------------------- // The reason two integers exist: a NEWER formatVersion whose minReaderVersion @@ -335,6 +366,7 @@ int main() { testEncodeRefusesWhatManifestRefuses(); testTruncationAtEveryByteOffsetIsMalformed(); testTooNewProducesNoManifest(); + testAdditiveUnparseableManifestIsTooNew(); testNewerAdditiveFormatReads(); testHostileHeadersAreMalformed(); testRequiredPrefixSizeRefusals(); diff --git a/tests/test_package_manifest.cpp b/tests/test_package_manifest.cpp index 8d35475..e418ed7 100644 --- a/tests/test_package_manifest.cpp +++ b/tests/test_package_manifest.cpp @@ -64,7 +64,7 @@ static PackageManifest fixture() { m.bankDisplayName = "Drums \"live\""; // escaping exercised m.exportTimestamp = 1754100000; m.entries.push_back({"kick.wav", 96000, "1111222233334444", fullSample()}); - m.entries.push_back({"snare.wav", 0, "5555666677778888", bareSample()}); // 0-length legal + m.entries.push_back({"snare.wav", 48000, "5555666677778888", bareSample()}); m.slots.append("smp-full"); m.slots.append("smp-bare"); m.slots.remove("smp-full"); // leaves a gap: slots round-trip must keep it @@ -173,6 +173,16 @@ static void testDuplicateEntriesKeyRejected() { // --- 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). +static void testEncodeRejectsZeroLengthEntry() { + PackageManifest m = fixture(); + m.entries[0].byteLength = 0; + CHECK(!serializeManifest(m).has_value()); +} + static void testEncodeRejectsUnrepresentableSample() { PackageManifest m = fixture(); m.entries[0].sample.id.clear(); // BankModel::add rejects an empty id @@ -253,6 +263,7 @@ int main() { testDecodeRejectsBadEntryName(); testDuplicateEntryNamesRejectedBothWays(); testDuplicateEntriesKeyRejected(); + testEncodeRejectsZeroLengthEntry(); testEncodeRejectsUnrepresentableSample(); testDecodeRejectsMalformedShapes(); From 3909b1072cd563764a7464462b77bdfe8857ef4a Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 08:44:24 -0400 Subject: [PATCH 4/5] 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(); From 181b4f2edb33c0ece38cd39e4de020bb4cc15307 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sun, 2 Aug 2026 09:02:23 -0400 Subject: [PATCH 5/5] Record parseSlots exemption and fix stale package doc comments Documents why parseSlots skips repeat-key rejection, corrects two drifted doc lines (naming-rule count, Malformed-after-header header validity), and records two forward obligations for import_plan in CLAUDE.md. --- src/core/package/CLAUDE.md | 24 ++++++++++++++++++++---- src/core/package/bank_package.h | 6 ++++-- src/core/package/package_format.h | 6 +++--- src/core/package/package_manifest.cpp | 9 ++++++++- 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/core/package/CLAUDE.md b/src/core/package/CLAUDE.md index 3c37197..9114f83 100644 --- a/src/core/package/CLAUDE.md +++ b/src/core/package/CLAUDE.md @@ -25,9 +25,10 @@ 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. -- **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: +- **Every name and path in the format is validated on encode AND decode, to + the extent stated below**, 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, @@ -40,7 +41,13 @@ landing after the format. 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. + the format vouched for. **Scope is traversal and absolute-form only** — no + UTF-8 well-formedness check (unlike `isValidEntryName`), no device-name + check, no case-fold dedup on `relativePath` (unlike `sameEntryName` on the + entry name). Correct for what this field is — a *record* field, not a + filesystem destination; `BankModel::add` owns the rest. Forward contract + for `import_plan`: **the destination file is derived from the entry name, + never from `relativePath`.** - **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 @@ -126,6 +133,15 @@ landing after the format. 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. +- **`duplicateName` is O(n²) over `entries` on the decode path** — pre-existing + shape (the double loop is unchanged since `af35fc5`; only the comparator + changed). Under the `kMaxManifestBytes` cap (64 MB) a minimal entry is + ~100 bytes, so a hostile package can declare ~670k entries — ~2×10¹¹ pair + comparisons, a multi-minute hang on import. It signals an error rather than + UB, so the hostile-input invariant above still holds, but it sits against + this module's "a forged header cannot demand gigabytes" posture. Forward + obligation for `import_plan`: fold this into a sorted vector or hash set + when that track lands; not changed here. - **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/bank_package.h b/src/core/package/bank_package.h index 677b567..84de7e9 100644 --- a/src/core/package/bank_package.h +++ b/src/core/package/bank_package.h @@ -40,8 +40,10 @@ struct EncodedPackage { std::optional encodePackage(const PackageManifest& m); // The read side's product. header is meaningful for Readable and TooNew (a -// refusal must still name the writer); manifest, layout, and prefixSize only -// for Readable — TooNew produces NO manifest, so a refused decode cannot +// refusal must still name the writer), and on any Malformed reached after the +// header parsed (a corrupt manifest at this build's own version carries the +// real pair, not the 0/0 unparsed default); manifest, layout, and prefixSize +// only for Readable — TooNew produces NO manifest, so a refused decode cannot // half-succeed. struct DecodedPackage { PackageReadability status = PackageReadability::Malformed; diff --git a/src/core/package/package_format.h b/src/core/package/package_format.h index 185f890..fd7c7a9 100644 --- a/src/core/package/package_format.h +++ b/src/core/package/package_format.h @@ -1,8 +1,8 @@ #pragma once // package_format — the RSBK bank-package contract: magic, the version ladder, -// the readability classification, and the entry-name rule. Pure: standard -// library only. The framing codec that acts on this contract is bank_package; -// the manifest grammar is package_manifest. +// the readability classification, and the three naming rules below. Pure: +// standard library only. The framing codec that acts on this contract is +// bank_package; the manifest grammar is package_manifest. #include #include diff --git a/src/core/package/package_manifest.cpp b/src/core/package/package_manifest.cpp index 5a2763e..df2f772 100644 --- a/src/core/package/package_manifest.cpp +++ b/src/core/package/package_manifest.cpp @@ -84,7 +84,14 @@ std::optional serializeManifest(const PackageManifest& m) { namespace { // Mirrors bank_book_json's private slots parser: [{id, slot}, ...] pairs handed -// to SlotMap::fromEntries, which owns the defensive repair rules. +// to SlotMap::fromEntries, which owns the defensive repair rules. Deliberately +// does NOT reject a repeated "id"/"slot" key the way the root and entry parsers +// below reject theirs — this grammar belongs to core/model's bank_book_json, and +// diverging here would give one wire shape two behaviours in two files. The +// stakes differ too: a repeated "name" decides which file an entry lands on, +// while a repeated "id" here still feeds SlotMap::fromEntries's deterministic +// first-wins/never-double-occupy repair, so no ambiguity survives. Do not +// "finish" the repeat-key rejection here to match the parsers below. bool parseSlots(json::Reader& r, model::SlotMap& out) { std::vector> pairs; if (!r.consume('[')) return false;