feat(hub): add X25519 sealed-envelope relay for store-and-forward through taos.my - #2034
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughHub sealed-envelope primitives provide encrypted, size-limited envelopes and canonical payload bytes. Account-proxy routes validate relay recipients, enforce local identity matching, and forward valid drop and poll requests upstream. ChangesHub sealed-envelope relay
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant account_proxy
participant taos_upstream
Client->>account_proxy: Submit relay drop or poll request
account_proxy->>account_proxy: Validate recipient and body
account_proxy->>taos_upstream: Forward relay action
taos_upstream-->>account_proxy: Return relay response
account_proxy-->>Client: Return status and response body
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| recipient_x25519_pub_hex=recipient_x25519_pub_hex) | ||
| outer["recipient"] = recipient | ||
| # Enforce max size: the JSON-serialised envelope must be ≤ 32 KB. | ||
| raw = json.dumps(outer, sort_keys=True, separators=(",", ":")) |
There was a problem hiding this comment.
WARNING: Envelope size limit is measured on the raw JSON, not the base64-encoded wire form the docstring promises.
The module docstring (line 23) states the limit is 32 KB (outer envelope base64-encoded), but here you measure len(json.dumps(...).encode("utf-8")). base64 inflates by ~33%, so a 32 KB JSON envelope serializes to ~42 KB on the wire — the documented limit is never actually enforced at the transport layer. Either measure the base64-encoded body the hub will actually receive, or correct the docstring to say the limit applies to the UTF-8 JSON representation.
Also raw.encode("utf-8") is computed twice (here and again inside the ValueError); compute it once into a variable.
| # never sees plaintext — it only inspects ``recipient`` for routing. | ||
|
|
||
| @router.post("/api/account/hub/relay/drop") | ||
| async def hub_relay_drop(request: Request): |
There was a problem hiding this comment.
WARNING: hub_relay_drop forwards the request body (including the in-body recipient) verbatim with no validation tying the recipient to the authenticated session.
Unlike hub_relay_poll (which validates recipient via _valid_rid and rejects bad input before forwarding), this endpoint relays the recipient field from the POST body straight to taos.my. A client authenticated as hub:alice can therefore drop a sealed envelope addressed to hub:bob (or any arbitrary rid). Even though the payload is sealed to Bob's X25519 key so Alice can't read it, this permits recipient-address spam / mailbox flooding and bypasses the routing restriction that the poll side enforces. Consider validating the in-body recipient against _valid_rid and/or asserting it matches the session identity before forwarding.
| assert r.status_code == 200 | ||
| assert "taos_session" not in captured.get("cookie", "") | ||
| assert captured.get("cookie", "") == "" | ||
|
|
There was a problem hiding this comment.
SUGGESTION: The PR description notes these 3 new account-proxy tests "require full app fixture (pre-existing conftest hang on this machine)" and were not actually run. Confirm they execute in CI before merge — the inline monkeypatch/_FakeResp/_patch_upstream fixtures must be available in this file's scope, and a hang or skip here would leave the new relay routes effectively untested in the pipeline.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Incremental Check (since previous review commit
|
| File | Line | Previous | Status |
|---|---|---|---|
tinyagentos/hub/relay.py |
167 | Size limit measured on raw JSON, not base64; raw.encode computed twice |
RESOLVED — docstring now reads "JSON-serialised envelope"; raw_bytes computed once (lines 167-168) |
tinyagentos/routes/account_proxy.py |
377 | hub_relay_drop forwarded body without recipient validation |
RESOLVED — _valid_hub_recipient(recipient) check present at line 386 before forwarding |
tinyagentos/routes/account_proxy.py |
381 | Non-dict JSON body 500ed instead of 400 | RESOLVED — isinstance(payload, dict) guard at line 383; UnicodeDecodeError added to except at line 381; unit test test_hub_relay_drop_rejects_non_dict_body added |
All previously active Code Review Findings are resolved. No new issues introduced in the incremental change.
Previous Review Summaries (4 snapshots, latest commit 81d9f18)
Current summary above is authoritative. Previous snapshots are kept for context only.
Previous review (commit 81d9f18)
Status: No Issues Found | Recommendation: Merge
Files Reviewed (5 files)
tinyagentos/hub/relay.py- 0 issues (previous size-limit doc + double-encode finding RESOLVED)tinyagentos/routes/account_proxy.py- 0 issues (recipient validation + non-dict body guard both RESOLVED)tests/test_hub_relay.py- 0 issues (new, 13 passing tests)tests/test_routes_account_proxy.py- 0 issues (new relay forwarding/validation tests)tests/test_routes_github.py- 0 issues (test mock fix, unrelated to relay feature)
Previous Findings — Re-verified / Resolved
| File | Line | Previous | Status |
|---|---|---|---|
tinyagentos/hub/relay.py |
167 | Size limit measured on raw JSON, not base64; raw.encode computed twice | RESOLVED — docstring corrected to “JSON-serialised outer envelope, UTF-8 encoded”; raw_bytes computed once |
tinyagentos/routes/account_proxy.py |
377 | hub_relay_drop forwarded body without recipient validation |
RESOLVED — _valid_hub_recipient(recipient) check now present before forwarding |
tinyagentos/routes/account_proxy.py |
381 | Non-dict JSON body 500ed instead of 400 | RESOLVED — isinstance(payload, dict) guard added; UnicodeDecodeError added to except clause |
tests/test_routes_account_proxy.py |
624 | New relay tests not executed locally | INFO — pre-existing CI-confidence note; not a code defect. All relay tests now structurally present and the PR confirms they pass under Python 3.11. |
Previous review (commit 7d45621)
Status: No Issues Found | Recommendation: Merge
Files Reviewed (2 files)
tinyagentos/routes/account_proxy.py- 0 issues (previously reported non-dict body 500 and missing recipient validation both RESOLVED in this commit)tests/test_routes_account_proxy.py- 0 issues (newtest_hub_relay_drop_rejects_non_dict_bodyadded covering the fix)
Previous Findings — Re-verified / Resolved
| File | Line | Previous | Status |
|---|---|---|---|
tinyagentos/routes/account_proxy.py |
383-384 | Non-dict JSON body 500ed instead of 400 | RESOLVED — isinstance(payload, dict) guard added before .get(); UnicodeDecodeError also added to the except clause |
tinyagentos/routes/account_proxy.py |
386 | hub_relay_drop forwarded without recipient validation |
RESOLVED — _valid_hub_recipient(recipient) check now present before forwarding |
tinyagentos/hub/relay.py |
167 | Envelope size limit measured on raw JSON, not base64 wire form | Out of scope for this incremental diff (unchanged file); carried forward for tracking |
Previous review (commit 1faa15b)
Status: 2 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 2 |
| SUGGESTION | 0 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
tinyagentos/routes/account_proxy.py |
381 | Non-dict JSON body (array/string/scalar) passes json.loads but payload.get(...) raises AttributeError, uncaught by the (json.JSONDecodeError, TypeError) clause, so the request 500s instead of returning 400; mirror the isinstance(payload, dict) guard used in _validate_subdomain_name |
tinyagentos/routes/account_proxy.py |
377 | hub_relay_drop now validates _valid_hub_recipient before forwarding — RE-VERIFIED: previously reported missing validation is fixed |
Files Reviewed (4 files)
tinyagentos/hub/relay.py- 0 issues (previous size-limit docstring / double-encode WARNING resolved in this commit)tinyagentos/routes/account_proxy.py- 1 new issue (non-dict body 500), 1 re-verified resolvedtests/test_routes_account_proxy.py- 0 new (prior SUGGESTION about unrun tests remains; newtest_hub_relay_drop_rejects_invalid_recipientadded and is structurally identical to passing tests)tests/test_hub_relay.py- 0 issues
Fix these issues in Kilo Cloud
Previous review (commit c47d898)
Status: 3 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 2 |
| SUGGESTION | 1 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
tinyagentos/hub/relay.py |
167 | Envelope size limit measured on raw JSON, not base64 wire form promised by docstring; encode computed twice |
tinyagentos/routes/account_proxy.py |
367 | hub_relay_drop forwards in-body recipient verbatim with no session/rid validation (mailbox spam risk) |
SUGGESTION
| File | Line | Issue |
|---|---|---|
tests/test_routes_account_proxy.py |
624 | New account-proxy relay tests reportedly not run (conftest hang); verify they pass in CI |
Files Reviewed (4 files)
tinyagentos/hub/relay.py- 1 issuetinyagentos/routes/account_proxy.py- 1 issuetests/test_routes_account_proxy.py- 1 issuetests/test_hub_relay.py- 0 issues
Reviewed by hy3:free · Input: 36.4K · Output: 2.6K · Cached: 159.6K
|
Thanks - this builds nicely on the #2025 peer channel. Fold list before merge (test 3.13 is also failing, likely the unrun tests below): MUST-FIX (security):
Then: Once the two security items are folded and CI is green I will re-review and merge. |
| body = await request.body() | ||
| try: | ||
| payload = json.loads(body) | ||
| recipient = str(payload.get("recipient", "")) |
There was a problem hiding this comment.
WARNING: A JSON body that parses to a non-dict (e.g. a JSON array [1,2,3], a string, or a number) passes json.loads but then payload.get(...) raises AttributeError, which is not caught by the except (json.JSONDecodeError, TypeError) clause — so the request 500s instead of returning 400.
The sibling validator _validate_subdomain_name already guards with if not isinstance(payload, dict). Mirror that here: after parsing, reject non-dict bodies before calling .get.
| recipient = str(payload.get("recipient", "")) | |
| if not isinstance(payload, dict): | |
| return JSONResponse({"error": "invalid body"}, status_code=400) | |
| recipient = str(payload.get("recipient", "")) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
All jaylfc MUST-FIX items from the 09:53 review are now addressed: 1. relay.py:167 — docstring/limit mismatch → Fixed in ed89178: docstring updated to "JSON-serialised outer envelope, UTF-8 encoded" to match code behavior. 2. account_proxy.py:377 — missing recipient validation → Fixed in ed89178: 3. test routes 3 new tests not run → Fixed in 81d9f18:
Kilo Code Review: All 4 findings (2 WARNING, 1 SUGGESTION, 1 WARNING-N/A) addressed in these same commits. Test matrix requires workflow approval for re-run (fork CI policy). |
…ough taos.my Add hub/relay.py module with X25519 ECDH+HKDF→ChaCha20-Poly1305 seal/unseal, envelope building with size limits (32KB), and canonical-JSON serialization. Add account proxy forwarding for POST /api/account/hub/relay/drop and GET /api/account/hub/relay/poll with recipient validation. Implements cross-user collab A3 (issue jaylfc#2015): the hub sealed-envelope relay for T1 transport. Hub never sees plaintext; inner payload is Ed25519-signed then X25519-sealed. Tests: 13/13 pass — round-trip, key mismatch, tamper detection, size limit, unicode, canonical serialization, base64 url-safety. Account proxy route tests added (3 new) but require full app fixture (pre-existing conftest hang on this machine; structurally identical to existing passing account-proxy hub tests).
- Add _valid_hub_recipient() for hub:username format validation - Validate recipient in hub_relay_drop before forwarding (was bypassed) - Fix hub_relay_poll to use hub-specific validator (was using generic _valid_rid which rejects colons in hub usernames) - Correct envelope size docstring: base64-encoded → UTF-8 JSON - Compute raw_bytes once instead of double-encoding in build_envelope - Add test_hub_relay_drop_rejects_invalid_recipient test All 4 relay tests pass (Python 3.11).
Add isinstance(payload, dict) check after json.loads() to prevent AttributeError on non-dict bodies (arrays, strings, numbers). Also add test_hub_relay_drop_rejects_non_dict_body test.
…pient MUST-FIX items from jaylfc review on PR jaylfc#2034: 1. relay.py build_envelope: measure base64-encoded wire form (not raw JSON) for the 32 KB envelope size limit, matching the docstring. Base64 inflates ~33 %, so a raw-JSON-under-limit envelope can exceed the transport cap. Error message now says 'base64-encoded'. 2. account_proxy.py hub_relay_drop: bind in-body recipient to caller's local hub identity (resolve_local_identity_id from peer.py) before forwarding — same audience-binding pattern as jaylfc#2025 peer channel fix. Rejects mismatched recipient with 403 when hub identity is registered. Tests: - test_size_limit_measured_on_base64_not_raw_json: 24 KB payload exceeds base64-encoded limit while raw JSON is under 32 KB - test_small_envelope_passes_size_check: sanity check small payload - test_hub_relay_drop_rejects_recipient_mismatch: 403 on mismatch - test_hub_relay_drop_accepts_matching_recipient: 200 on match All 15 relay tests pass.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_routes_account_proxy.py`:
- Around line 704-734: Update test_hub_relay_drop_forwards_envelope to mock
resolve_local_identity_id explicitly, matching the setup used by the later
recipient-mismatch and matching-recipient tests. Configure the mock to return
None so the test consistently exercises the successful forwarding path without
depending on ambient TAOS_DATA_DIR or keystore state.
In `@tinyagentos/hub/relay.py`:
- Around line 192-195: Update canonicalize() to reject NaN and positive or
negative infinity while serializing the payload without the sig field, using
strict JSON serialization or equivalent validation so non-standard numeric
values cannot produce signed bytes. Add coverage in TestCanonicalize for NaN and
infinity inputs.
In `@tinyagentos/routes/account_proxy.py`:
- Around line 405-418: The hub_relay_poll handler must bind the requested
recipient to the caller’s local hub identity, not just validate its format.
Mirror the identity-matching check used by hub_relay_drop before calling
_forward_to, rejecting any well-formed hub:<user> recipient that does not match
the local hub identity.
- Around line 375-402: Update the hub_relay_drop route to offload the blocking
resolve_local_identity_id() call to a worker thread using the file’s existing
asyncio import, adding that import only if absent; preserve the current identity
comparison and 403 response behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ebb08327-b9a8-4180-9c4f-d744fe948c0d
📒 Files selected for processing (4)
tests/test_hub_relay.pytests/test_routes_account_proxy.pytinyagentos/hub/relay.pytinyagentos/routes/account_proxy.py
|
Held after the deepest review of the batch, because this is crypto. The cryptography itself is correct and safe - merge that part with confidence. The blocker is an authorization control placed on the wrong endpoint, and the tests currently lock in the wrong behavior. The crypto is sound (verified, not assumed): HIGH - the audience binding is inverted (verified). MEDIUM - So the fix is a swap: remove the binding from drop, move it to poll. Latent rather than a live regression (nothing is wired to the relay yet, grep confirms), but it must be fixed before any caller lands or the follow-up wiring is dead on arrival. LOW - outer envelope fields unauthenticated: Fix the drop/poll swap and add a poll-side binding test, then this is ready. The crypto foundation is genuinely good work. |
- relay.py: add allow_nan=False to canonicalize() json.dumps - account_proxy.py: offload resolve_local_identity_id() to thread - account_proxy.py: bind hub_relay_poll recipient to local identity - tests: mock resolve_local_identity_id for deterministic relay tests - tests: add NaN/infinity rejection coverage in TestCanonicalize
|
Deep review done. The cryptography is competent: fresh ephemeral X25519 per message, HKDF-SHA256 with domain separation, ChaCha20-Poly1305 with a per-message nonce, raw ECDH output never used as a key, fails closed, no secrets logged. I found no misuse of the primitives. Two changes are required before this can merge, and the first is the important one. 1. The recipient binding in
The commit cites the audience-binding pattern from #2025, but that pattern lives on the INBOUND peer route ( Fix: remove the binding from Then invert 2.
Worth doing in the same milestone (hardening, not blockers):
Note also that the server half (#2015, the hub endpoints on taos.my) is a different repo, so every real limit (sender identity, friend-edge check, queue cap, TTL, size cap) is unverifiable from here. The constants in Tests were verified to actually run, no skip guards, and both files pass on the PR head. |
…resolve_local_identity_id Drop is outbound send — a node drops envelopes addressed to any valid hub recipient. Recipient binding now only applies to hub_relay_poll (polling your own queue). Also pass request.app.state.data_dir to resolve_local_identity_id in hub_relay_poll, matching every sibling route. Invert test_hub_relay_drop_rejects_recipient_mismatch to test_hub_relay_drop_allows_different_recipient, asserting 200 for a drop to a different user. Fixes jaylfc review findings on PR jaylfc#2034.
|
Fixes for the two REQUIRED changes: 1. Removed recipient binding from 2. Pass |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tinyagentos/routes/account_proxy.py (2)
411-416: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed when no local hub identity is available.
resolve_local_identity_id()returnsNoneboth for an unregistered identity and lookup failures. Line 412 therefore forwards any valid recipient, allowing polling of another queue precisely when local binding is unavailable. Require a resolved identity and equality before forwarding.
tinyagentos/routes/account_proxy.py#L411-L416: return403whenlocal_id is Noneor it differs fromrecipient.tests/test_routes_account_proxy.py#L739-L760: mockhub:aliceand verify pollinghub:aliceforwards; add ahub:bobcase that returns403without upstream I/O.tests/test_routes_account_proxy.py#L847-L879: remove or repurpose the obsolete drop-recipient-binding test for the poll authorization cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/account_proxy.py` around lines 411 - 416, Update the poll authorization in tinyagentos/routes/account_proxy.py lines 411-416 to return 403 unless local_id is resolved and equals recipient. In tests/test_routes_account_proxy.py lines 739-760, mock hub:alice and verify matching polling forwards, then verify hub:bob returns 403 without upstream I/O. In tests/test_routes_account_proxy.py lines 847-879, remove or repurpose the obsolete drop-recipient-binding test for these poll authorization cases.
381-395: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the incoming body before buffer+relay.
hub_relay_dropcallsawait request.body()and then sends the bytes upstream; the app/server startup uses plainuvicorn.run/uvicorn.Configwith no request limit enabled. Other proxy routes already reject oversizedContent-Lengthbefore reading, so add the same guard here or another request-size cap that also enforces the 32KB envelope spec.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/account_proxy.py` around lines 381 - 395, Add the existing proxy-route request-size guard to hub_relay_drop before await request.body(), rejecting Content-Length values above the 32KB envelope limit with the established error response. Ensure the cap also protects requests lacking Content-Length or using chunked transfer before buffering and forwarding through _forward_to.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tinyagentos/routes/account_proxy.py`:
- Around line 411-416: Update the poll authorization in
tinyagentos/routes/account_proxy.py lines 411-416 to return 403 unless local_id
is resolved and equals recipient. In tests/test_routes_account_proxy.py lines
739-760, mock hub:alice and verify matching polling forwards, then verify
hub:bob returns 403 without upstream I/O. In tests/test_routes_account_proxy.py
lines 847-879, remove or repurpose the obsolete drop-recipient-binding test for
these poll authorization cases.
- Around line 381-395: Add the existing proxy-route request-size guard to
hub_relay_drop before await request.body(), rejecting Content-Length values
above the 32KB envelope limit with the established error response. Ensure the
cap also protects requests lacking Content-Length or using chunked transfer
before buffering and forwarding through _forward_to.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 109bbc07-4ace-48a1-93b7-995903ae2630
📒 Files selected for processing (2)
tests/test_routes_account_proxy.pytinyagentos/routes/account_proxy.py
2 participants
Summary
Implements cross-user collab A3 (issue #2015): the hub sealed-envelope relay for T1 transport on taos.my.
What's added
tinyagentos/hub/relay.py— X25519 envelope sealing module:seal()/unseal()for keypair-based encrypt/decryptbuild_envelope()— wraps a sealed payload in a hub-routable outer envelope with recipient field and size enforcement (≤32KB)canonicalize()— canonical JSON serialization for Ed25519 signing (design doc section 2)tinyagentos/routes/account_proxy.py— hub relay forwarding:_ACTIONSentries:hub_relay_drop(POST) andhub_relay_poll(GET)POST /api/account/hub/relay/drop— forwards sealed envelope to taos.myGET /api/account/hub/relay/poll?recipient=...— polls for queued envelopes with recipient validationDesign
The hub never sees plaintext. Nodes seal inner (Ed25519-signed) payloads to the recipient's X25519 public key. The outer envelope only exposes
recipientfor routing andcreated_atfor TTL expiry. Recipient nodes poll and unseal locally.Tests
tests/test_hub_relay.py— 13 tests (all passing):tests/test_routes_account_proxy.py— 3 new tests added:References
docs/design/cross-user-collaboration.md(sections 6, 7)Summary by CodeRabbit
New Features
Tests
NaN/Infinity), and proxy request forwarding/400/403 validation.