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;