fix(protocol): enforce max chunk size on manifests and chunk data - #70
fix(protocol): enforce max chunk size on manifests and chunk data#70lukyrys wants to merge 34 commits into
Conversation
…unk-size # Conflicts: # backend/src/protocol/chunk-downloader.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d440031ba0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!('manifest' in response)) throw new CodedError(ErrorCodes.PEER_INVALID_REQUEST, `getLish ${lishID}: missing manifest`); | ||
| // A manifest from the network is untrusted input — validate chunk-size bounds and | ||
| // manifest consistency before it can reach any caller (DB persist / import / probe). | ||
| validateLISHStructure(response.manifest, maxChunkSize); |
There was a problem hiding this comment.
When a peer-supplied manifest fails this validation, validateLISHStructure throws local LISH_* codes rather than a PEER_* code. The Browse Network fallback loop (withPeerFallback) retries only PEER_*/tryNextPeer errors, so if the first peer listed for a LISH returns a malformed or spoofed manifest while a later peer can serve a valid one, the Details flow aborts immediately instead of trying the remaining peers. Consider mapping validation failures from untrusted peer responses to a retryable peer-scoped error (or otherwise marking them retryable) while preserving the final user-facing limit error after fallback is exhausted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a881fcd — requestManifest now maps structural validation failures on peer manifests to PEER_INVALID_REQUEST so withPeerFallback moves on to the next peer. LISH_CHUNK_SIZE_TOO_LARGE stays terminal on purpose: chunk size is a property of the LISH itself, every honest peer serves the same manifest, so retrying other peers cannot help and would only delay the user-facing limit error.
| if (lish.files) { | ||
| if (!Array.isArray(lish.files)) throw new CodedError(ErrorCodes.LISH_INVALID_MANIFEST, 'files is not an array'); |
There was a problem hiding this comment.
Because the new Array.isArray guard is nested under if (lish.files), peer manifests with files: null, false, or 0 skip the check and are accepted as though file metadata were simply absent. On the network/import paths this lets malformed peer input be persisted or treated as an empty metadata-only manifest instead of being rejected, which can leave downloads stuck re-requesting a manifest or incorrectly proceeding with no chunks. Check for presence rather than truthiness before accepting the optional field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fa81f8 — the guard now checks presence instead of truthiness, so files: null / false / 0 are rejected as LISH_INVALID_MANIFEST; only a genuinely absent field is treated as metadata-only.
| const expectedLen = expectedChunkLength(lish, chunk.fileIndex, chunk.chunkIndex); | ||
| let rejectReason: string | null = expectedLen >= 0 && data.length !== expectedLen ? `wrong length: expected ${expectedLen}B, got ${data.length}B` : null; |
There was a problem hiding this comment.
When a checksum appears in multiple slots, writeChunkToAllSlots later writes this same payload to every slot sharing the chunk ID, but this new check only compares the length for the queued slot. With a malformed manifest that lists the same checksum for a full chunk before a shorter last-chunk slot, a full-length payload passes here and is then written past the end of the shorter file while all matching slots are marked downloaded. Reject duplicate checksum groups with mismatched expected lengths, or validate the payload against every target before accepting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3fa81f8 at the root cause — validateLISHStructure now rejects manifests where the same checksum is claimed by slots with different expected lengths (unsatisfiable by any single payload), so such a manifest never reaches the duplicate-slot write path. Tracking only short last chunks keeps the check O(#files) in memory.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a881fcd796
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // protocol error so fallback loops move on to the next peer. An over-limit | ||
| // chunkSize is a property of the LISH itself (every honest peer serves the same | ||
| // manifest), so it stays a terminal local error. | ||
| if (e instanceof CodedError && e.code !== ErrorCodes.LISH_CHUNK_SIZE_TOO_LARGE) throw new CodedError(ErrorCodes.PEER_INVALID_REQUEST, `getLish ${lishID}: ${e.message}`); |
There was a problem hiding this comment.
When Browse Network tries a row with multiple peers, this exception leaves LISH_CHUNK_SIZE_TOO_LARGE as a local/non-retryable error, so withPeerFallback stops on the first peer that returns an oversized spoofed manifest instead of trying the later peers that may have a valid manifest for the same advertised id. Fresh evidence in this revision is the explicit exception here; LISH ids are random UUIDs rather than content-bound, and search rows are grouped only by peer-reported id, so an oversized response is still attributable to that peer until all candidates fail.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9880048 — isRetryablePeerError now treats LISH_CHUNK_SIZE_TOO_LARGE as retryable, so a spoofed oversized manifest from one peer no longer blocks later honest peers. When the LISH genuinely exceeds the local limit every peer throws the same code, the loop exhausts and the user still sees the specific limit error (the last one thrown) instead of a generic peer failure.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9880048faf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Over-limit chunkSize is terminal: every honest peer serves the same | ||
| // manifest, so retrying peers forever would leave the download stuck in | ||
| // awaiting-manifest with no user-visible error. | ||
| if (error instanceof CodedError && error.code === ErrorCodes.LISH_CHUNK_SIZE_TOO_LARGE) { |
There was a problem hiding this comment.
When an awaiting-manifest download has multiple peers and the first peer returns a forged or stale manifest whose chunkSize exceeds the local limit, this branch calls setError and returns before querying the remaining peers. requestManifest() derives this error from a single peer response, and the frontend fallback treats the same code as retryable for exactly this spoofing case, so one bad peer can prevent importing a valid manifest from another peer; exhaust the available peers first, then surface the limit error if they all fail the same way.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5cad078 — the awaiting-manifest loop now drops a peer whose manifest exceeds the limit (a forged/stale one from a single peer) and keeps trying the rest; another peer can still serve a valid manifest. The terminal LISH_CHUNK_SIZE_TOO_LARGE is surfaced only once every peer has been dropped this way (the LISH itself is over-limit), so one bad peer can no longer block a valid import, and a genuinely oversized LISH no longer stalls silently in awaiting-manifest. The probe path just drops such peers too and leaves the terminal decision to this loop.
| if (typeof chunkSize !== 'number' || !Number.isFinite(chunkSize) || chunkSize <= 0) throw new CodedError(ErrorCodes.LISH_INVALID_CHUNK_SIZE, String(chunkSize)); | ||
| if (chunkSize > maxChunkSize) throw new CodedError(ErrorCodes.LISH_CHUNK_SIZE_TOO_LARGE, `${chunkSize} > ${maxChunkSize}`); | ||
| if (chunkSize > maxChunkSize) throw new CodedError(ErrorCodes.LISH_CHUNK_SIZE_TOO_LARGE, `${formatBytes(chunkSize)} > ${formatBytes(maxChunkSize)}`); |
There was a problem hiding this comment.
This create path still accepts any finite positive chunkSize, but the new manifest validator now rejects non-integer chunk sizes. With a direct API call such as chunkSize: 1024.5 plus lishFile and no add-to-sharing/downloading, createLISH() and exportLISHToFile() run and return a .lish file that this same version later refuses to import; reject non-integers here before hashing/export so creation and import use the same contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 835cb4d — the create path now rejects a non-integer chunkSize (Number.isInteger), matching validateLISHStructure, so a LISH this version creates/exports is always one it can also import.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cad0789d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Any manifest error (unreachable, forged/over-limit, malformed) → drop this peer | ||
| // and let another serve it; doWork surfaces the terminal over-limit case once no | ||
| // peer can provide a valid manifest. |
There was a problem hiding this comment.
When an awaiting-manifest download only discovers peers through probeTopicPeers (for example, topic peers that did not answer the WANT/HAVE path), an over-limit manifest is caught here before the peer is ever added to peerManager; the peer is just dropped, foundNew stays false, and the Phase 1 terminal check never runs. If every probed peer serves the same oversized LISH, the download remains stuck in awaiting-manifest and retries after dropped-peer resets instead of showing LISH_CHUNK_SIZE_TOO_LARGE. Track this code in the probe path and set the downloader error once probes are exhausted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 71bafca — the probe path records an over-limit manifest locally (probed peers never enter peerManager) and, when probes were the only discovery source and every answering peer served an over-limit LISH with nobody connected to try instead, sets the terminal LISH_CHUNK_SIZE_TOO_LARGE mirroring Phase 1's check, instead of silently re-probing forever.
| if (error instanceof CodedError && error.code === ErrorCodes.LISH_CHUNK_SIZE_TOO_LARGE) { | ||
| oversized = { code: error.code, detail: error.detail }; | ||
| this.peerManager.remove(peerID, 'drop'); | ||
| continue; |
There was a problem hiding this comment.
When an awaiting-manifest download receives a structurally invalid peer manifest, requestManifest() now throws PEER_INVALID_REQUEST, but this Phase 1 catch only removes peers for LISH_CHUNK_SIZE_TOO_LARGE; malformed peers fall through to the warning and stay in peerManager. With no later valid connected peer, the download remains stuck waiting for a manifest and future discovery skips that same connected bad peer, so drop retryable peer-scoped manifest failures here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 71bafca — Phase 1 now also drops a peer whose manifest fails structural validation (PEER_INVALID_REQUEST), so a connected bad peer can no longer keep the download stuck while discovery skips it as already-connected. Covered by a new unit test (bad peer dropped, manifest imported from the next peer).
| // remember the reason, and try the rest. Only when every peer is gone this way | ||
| // (the LISH truly exceeds the limit) is it terminal — handled after the loop. | ||
| let oversized: { code: ErrorCode; detail: string | undefined } | null = null; | ||
| for (const [peerID, client] of [...this.peerManager.entries()]) { |
There was a problem hiding this comment.
Because this loop now snapshots peerManager.entries(), a peer that connects via HAVE while Phase 1 is awaiting another peer's manifest is not included in the current pass; its onHaveAnnouncement call to doWork() returns immediately because workMutex is locked, and nothing re-runs when this snapshot finishes with needsManifest still true. In that timing window the newly connected valid peer can sit idle until the next discovery timer/manual trigger, delaying or stalling import of the manifest; either keep the iteration live or schedule another Phase 1 pass when peers were added during the snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 71bafca — Phase 1 iterates the peer map live again (Map iterators tolerate mid-loop deletes and visit entries added during iteration), so a peer joining via HAVE while another peer's manifest is awaited gets its turn in the same pass instead of waiting for the next discovery cycle.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71bafca9a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Probes were the only discovery source and every answering peer served an | ||
| // over-limit manifest, with nobody connected to try instead — terminal, mirror | ||
| // Phase 1's check so the download shows the error instead of re-probing forever. | ||
| if (this.needsManifest && probeOversized && !foundNew && this.peerManager.size() === 0 && !this.destroyed) { |
There was a problem hiding this comment.
In the probe-only awaiting-manifest path, this terminal check fires after any over-limit response as long as no peer was added. If one topic peer returns a forged oversized manifest while another probe times out, is unreachable, or returns a non-over-limit manifest error, foundNew stays false and peerManager.size() is 0, so setError(LISH_CHUNK_SIZE_TOO_LARGE) disables the downloader and unregisters future HAVE handling even though not all candidates proved the LISH exceeds the local limit. Track whether every attempted probe failed with the limit error, or keep awaiting when failures are mixed, before setting the terminal error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 981f9ff — the probe path applies the same rule: an unreachable dial, a non-over-limit manifest error, or an answered-but-no-manifest probe marks the pass as mixed, and the terminal error requires unanimous over-limit evidence (probeOversized set, no mixed failure, nothing connected).
| } | ||
| // Every peer that answered did so with an over-limit manifest (all dropped, none | ||
| // reachable remain) → the LISH itself is too large: surface it instead of stalling. | ||
| if (this.needsManifest && oversized && this.peerManager.size() === 0) { |
There was a problem hiding this comment.
During the connected-peer awaiting-manifest pass, this condition reports LISH_CHUNK_SIZE_TOO_LARGE whenever any peer returned that code and the peer map is empty, but the same loop also removes peers for malformed manifests via PEER_INVALID_REQUEST. With one malformed/spoofed peer and one oversized/spoofed peer, the downloader is put into error state and unregisters future HAVE handling even though the remaining failures do not prove the LISH itself exceeds the local limit. Count whether every attempted peer failed specifically with the size limit before making this terminal; otherwise keep awaiting discovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 981f9ff — the connected-peer pass now tracks non-over-limit failures (malformed drops, transient warns) as mixed evidence; the terminal LISH_CHUNK_SIZE_TOO_LARGE fires only when EVERY attempted peer failed specifically with the size limit and none remain. Mixed failures keep the download awaiting discovery. Covered by a new unit test (one oversized + one malformed peer → both dropped, state stays awaiting-manifest, no error).
| if (lish.files !== undefined) { | ||
| if (!Array.isArray(lish.files)) throw new CodedError(ErrorCodes.LISH_INVALID_MANIFEST, 'files is not an array'); |
There was a problem hiding this comment.
This validation now protects untrusted peer manifests before import, but it only rejects malformed files; a peer can still send truthy non-arrays like directories: {} or links: {} and pass requestManifest(). The import/download paths then reach dataServer.add(), which iterates those fields with for ... of and throws a raw TypeError, aborting the add/download instead of treating that peer as invalid and falling back. Validate these optional manifest arrays before accepting the manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d8b4e4f — validateLISHStructure now applies the same presence check to the optional directories and links fields: a truthy non-array is rejected as LISH_INVALID_MANIFEST before the manifest can reach dataServer.add and crash its for..of iteration with a raw TypeError.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 981f9ff535
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A manifest from the network is untrusted input — validate chunk-size bounds and | ||
| // manifest consistency before it can reach any caller (DB persist / import / probe). | ||
| try { | ||
| validateLISHStructure(response.manifest, maxChunkSize); |
There was a problem hiding this comment.
When a peer manifest has a valid chunk layout but an unsupported checksumAlgo, this new network-manifest validation still accepts it because validateLISHStructure does not perform the algorithm check that local file/JSON imports get from validateImportedLISH. The manifest can then be persisted/imported, and the download or verification path later reaches new Bun.CryptoHasher(lish.checksumAlgo as any), which throws for unsupported algorithms instead of treating that peer as invalid and falling back to another peer. Validate checksumAlgo here (or in validateLISHStructure) before returning the peer manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c699249 — validateLISHStructure now validates checksumAlgo against SUPPORTED_ALGOS (matching validateImportedLISH), so an unsupported algorithm is rejected as LISH_UNSUPPORTED_CHECKSUM before download/verify reaches new Bun.CryptoHasher and throws.
| // A manifest from the network is untrusted input — validate chunk-size bounds and | ||
| // manifest consistency before it can reach any caller (DB persist / import / probe). | ||
| try { | ||
| validateLISHStructure(response.manifest, maxChunkSize); |
There was a problem hiding this comment.
When a peer answers getLish with a structurally valid manifest whose id differs from the requested lishID, this validation still returns it as a success. In Browse Network fallback that makes the first spoofing peer win and prevents trying later peers for the advertised LISH, and addPeerLish can import the wrong LISH under the peer-supplied id. Reject mismatched manifest IDs as PEER_INVALID_REQUEST before returning the manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 94c355c — requestManifest now rejects a manifest whose id differs from the requested lishID as PEER_INVALID_REQUEST (retryable), so a spoofing peer cannot win the fallback loop or get its LISH imported under the requested id.
| // false) and a float size makes every chunk "wrong length", banning honest peers. | ||
| if (typeof file.size !== 'number' || !Number.isInteger(file.size) || file.size < 0) throw new CodedError(ErrorCodes.LISH_INVALID_MANIFEST, `${file.path}: invalid size ${String(file.size)}`); | ||
| const expected = file.size === 0 ? 0 : Math.ceil(file.size / lish.chunkSize); | ||
| if (!Array.isArray(file.checksums) || file.checksums.length !== expected) { |
There was a problem hiding this comment.
When an untrusted peer sends a file entry with an integer size and the right checksum count but non-string fields, such as path: {} or checksums: [{}], this validation accepts it because it only checks the array shape and length. The manifest then reaches dataServer.add, where SQLite binding throws a raw TypeError/constraint error, so Browse Network add/detail paths fail as a local/internal error instead of treating that peer as invalid and trying the next one. Validate file paths and each checksum value before accepting the manifest.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c699249 — validateLISHStructure now requires each file.path to be a string and each checksum entry to be a string, so a manifest with path:{} or checksums:[{}] is rejected as LISH_INVALID_MANIFEST before it can reach the SQLite bind in dataServer.add.
…unk-size # Conflicts: # backend/src/api/lishnets.ts # backend/src/protocol/lish-protocol.ts
…bersoft-org/libershare into fix/60-enforce-max-chunk-size
…bersoft-org/libershare into fix/60-enforce-max-chunk-size
2 participants
Enforces the
network.maxChunkSizesetting so oversized chunks can no longer crash the app.LISH_CHUNK_SIZE_TOO_LARGE