Harden inbound envelope handling and iOS attachment outbox - #8
Merged
Merged
Conversation
plugin/wire.ts — bound reassembly state by cumulative payload size. Reassembler and MediaReassembler now track the bytes held per in-flight id and drop the entry when a chunk would push it past the cap (1 MiB text / 32 MiB media, both overridable). Previously the only bound was the 60s GC sweep. Re-sent seqs swap their contribution rather than adding, so a replayed chunk does not inflate the running total. plugin/inbound.ts — require a fresh `ts` for the shared-secret bypass. A valid HMAC now only skips the allowlist when the envelope timestamp is within hmacMaxSkewMs (default 2 min) of local time. Outside the window, authorization falls back to the allowlist rather than being denied outright, so allowlisted peers keep their previous behavior. plugin/allowlist.ts, config.ts — canonicalize address case. The address grammar accepts hex in either case, so an allowlisted peer spelled with different casing than the configured entry was silently dropped. Entries and inbound src addrs are now compared on a single canonical form, matching what nodeIdToAddress already emits in peer-address.ts. ios/Conversation.swift — carry attachments through the outbox. drainOutbox re-sent every queued message over the text path, so a retried media message arrived as its caption alone; it now branches on the attachment and uses the media path. sendAttachment also returned early when offline, discarding the message; it now appends and persists as .sending before checking the connection, mirroring send(), so a queued attachment survives an app restart and is picked up on drain. Two existing tests asserted the superseded behavior and were updated: the allowlist case-sensitivity test and the iOS sendAttachment-when-offline test. Plugin: 252 tests pass (was 240). iOS: 150 tests pass (was 146). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes five audit findings across the plugin wire/inbound layer and the iOS conversation view-model. Each was verified against current
mainbefore changing anything, and each fix has a test that fails before and passes after.Changes
plugin/src/wire.ts— cumulative byte cap on reassembly (L17)ReassemblerandMediaReassemblerheld chunks for an in-flight id with no size bound; the only thing reclaiming them was the 60s GC sweep. Both now track bytes held per id and drop the entry when an incoming chunk would push it past the cap — 1 MiB for text, 32 MiB for media, both constructor-overridable. The media ceiling sits above the inbound pipeline's existing 25 MiB attachment cap so that cap still governs completed transfers.Re-sending a seq replaces the held chunk, so its contribution is swapped rather than added — a repeated chunk cannot inflate the running total.
plugin/src/inbound.ts— timestamp window on the shared-secret bypass (L18)A valid HMAC skipped the allowlist regardless of the envelope's
ts. The bypass now additionally requireststo be withinhmacMaxSkewMs(default 2 minutes) of local time.When the timestamp is outside the window, authorization falls back to the allowlist rather than dropping outright — so an allowlisted peer keeps its existing behavior, and only the secret-based bypass is gated.
tsis already covered by the HMAC, so the window is what bounds how long one signed envelope stays usable.plugin/src/allowlist.ts,config.ts— canonical address case (L19)isValidPilotAddressaccepts hex in either case, but the allowlist check was exact string equality, so a peer whose address was spelled with different casing than the configured entry was silently dropped. Entries are canonicalized atresolveAccountand inbound src addrs atdecideAllowlist, with a fallback scan covering sets assembled by other callers.Canonical form is upper-case, matching what
nodeIdToAddressalready emits inpeer-address.ts— so addresses derived from either path now compare equal. This also removes a hand-rolled port-strip in the HMAC branch ofinbound.tsin favor of the shared helper.ios/.../Conversation.swift— attachments through the outbox (H5, M26)Two related gaps in the same flow:
drainOutboxre-sent every queued message viaconn.send(text:), so a retried media message arrived as its caption alone and the bytes were lost. It now branches onm.attachmentsand re-sends via the media path; text-only messages are unchanged.sendAttachmentreturned early when there was no ready connection, discarding the message entirely. It now appends and persists as.sendingbefore checking the connection — mirroringsend()— so an attachment composed offline survives an app restart and is picked up by the drain.A
wireKindhelper onChatAttachmentcarries the kind mapping the drain needs.Tests
Two existing tests asserted the behavior these findings identify as wrong and were updated rather than worked around:
decideAllowlist > does not silently accept on a typo (case-sensitive net id)— now asserts that a case-variant spelling of an allowlisted address resolves to the canonical peer.testSendAttachmentWhenNotReadyIsIgnored→testSendAttachmentWhenNotReadyQueuesAsSending.New coverage: reassembly caps (including that a replayed seq does not inflate the total, and that under-cap messages still assemble), the HMAC timestamp window in all four directions (fresh / stale / future / stale-but-allowlisted), allowlist case canonicalization, and the iOS offline-attachment persist + rehydrate + drain-selection path.
Verified fail-before/pass-after by reverting the sources and re-running: 7 of the new plugin tests and 19 iOS assertions fail without the fixes.
npm test, plustsc --noEmitcleanswift testNotes
MAX_TEXT_REASSEMBLY_BYTES/MAX_MEDIA_REASSEMBLY_BYTESandhmacMaxSkewMsare exported/injectable, so deployments that need different bounds can set them without a patch. The 2-minute default skew assumes roughly-synced clocks between the iOS client and the claw host; a peer with badly-skewed time will fall back to the allowlist rather than being refused.🤖 Generated with Claude Code