Log oversized compressed payloads before rejection - #1682
Conversation
Fixes permissionlesstech#1628. When a compressed packet declares an expanded size above FileTransferLimits.maxFramedFileBytes, the decoder rejects it silently. This is especially difficult to diagnose when Android permits a larger expanded payload than iOS. Log the declared size and configured ceiling at the existing security warning level before returning nil. Packet acceptance behavior is unchanged. Add a v2 regression test that exercises the expanded-size guard without attempting decompression.
Restore the verified local BinaryProtocol.swift contents after correcting the UInt16 encoder line. The oversized compressed-payload logging fix remains unchanged.
Add regression coverage for issue permissionlesstech#1628 by constructing a valid v2 compressed frame whose declared expanded size exceeds FileTransferLimits.maxFramedFileBytes and asserting that BinaryProtocol.decode rejects it.
![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: e90fd3869f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| SecureLogger.warning( | ||
| "🚫 Compressed payload expanded size exceeds limit: \(originalSize) bytes > \(FileTransferLimits.maxFramedFileBytes) bytes", | ||
| category: .security | ||
| ) |
There was a problem hiding this comment.
When an oversized compressed frame is padded, decode(_:) first calls decodeCore on the padded data and then retries after MessagePadding.unpad; both attempts reach this warning before returning nil, so one received frame emits two identical security warnings and inflates any diagnostic count. Log the rejection only once across the two decode attempts, or propagate the rejection reason to the outer decoder.
Useful? React with 👍 / 👎.
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
this is the same BinaryProtocol.swift guard as #1623. please close this as a duplicate — the new test would already pass on main (decode already returns nil) and never asserts the log.
2 participants
Fixes #1628.
Summary
When a compressed packet declares an expanded size above
FileTransferLimits.maxFramedFileBytes, the decoder currently rejects it without logging the reason. This makes oversized compressed payloads difficult to diagnose, particularly when platform-specific limits differ.This change logs the declared expanded size and configured ceiling at the existing security warning level before rejecting the packet. Acceptance behavior is unchanged.
Tests
Added a v2 regression test that constructs a valid compressed frame whose declared expanded size exceeds the configured limit and verifies that
BinaryProtocol.decoderejects it before attempting decompression.The local source and test files pass deterministic source checks and
git diff --check. The full Swift/Xcode suite could not be run in this Linux environment becauseswiftandxcodebuildare unavailable.