feat(mesh): surface guest peer nodes in mesh_status and add guest-preauth proxy endpoint#2038
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe account proxy validates and forwards guest preauth requests, removes credential fields, and stores join intents. Mesh status now reports tagged guest peers with normalized fields while preserving fail-soft handling for unavailable or malformed status data. ChangesGuest collaboration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountProxy
participant UpstreamCluster
Client->>AccountProxy: POST guest preauth with contact_id
AccountProxy->>UpstreamCluster: POST guest-preauth
UpstreamCluster-->>AccountProxy: preauth response
AccountProxy->>AccountProxy: strip secrets and store join intent
AccountProxy-->>Client: sanitized response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
| pattern as the host join poll) — the caller (peer channel, D1) delivers it | ||
| directly to the guest instance out-of-band. | ||
| """ | ||
| return await _forward(request, "cluster_guest_preauth") |
There was a problem hiding this comment.
[CRITICAL]: Preauth key is leaked to the browser; docstring security claim is false.
The docstring (lines 424-426) states the preauth key is "stripped from any browser-facing response (same security pattern as the host join poll)". But this endpoint just calls await _forward(...), which relays upstream.content verbatim via _forward_to (account_proxy.py:198-202). It does NOT go through the _persist_join_credentials stripping path that the host join poll uses. The test test_cluster_guest_preauth_forwards_to_upstream even asserts r.json()["preauth_key"] == "guest-key-1" is returned to the client.
This means a single-use, ACL-pinned (tag:guest) preauth key — a credential that lets a guest instance join the host's mesh — is exposed to browser JavaScript. That is a security regression relative to the stated intent and to the existing headscale_preauth_key stripping in _persist_join_credentials (account_proxy.py:431-477).
Additionally, the new upstream field is named preauth_key, which is NOT in _STRIP_KEYS (line 37: _JOIN_SERVICE_TOKENS + ("headscale_preauth_key",)), so even the existing stripping mechanism wouldn't catch it. The endpoint must parse the upstream JSON, persist/deliver the key server-side (out-of-band to the guest peer as the docstring describes), and return a response with the secret stripped.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| pattern as the host join poll) — the caller (peer channel, D1) delivers it | ||
| directly to the guest instance out-of-band. | ||
| """ | ||
| return await _forward(request, "cluster_guest_preauth") |
There was a problem hiding this comment.
[WARNING]: No validation/authorization of the request body on a credential-minting endpoint.
cluster_guest_preauth forwards the incoming JSON body verbatim to taos.my. Unlike the rid-style endpoints (e.g. lines 400-411) which validate the path token with _valid_rid, this endpoint performs no validation of contact_id or any other payload field before minting a guest preauth key upstream. Consider validating contact_id format (and that the caller is authorized to mint guest keys for the target contact) to avoid minting guest credentials for arbitrary/forged identities.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ips = node.get("TailscaleIPs") or [] | ||
| return { | ||
| "hostname": node.get("HostName"), | ||
| "node_ip": ips[0] if ips else None, |
There was a problem hiding this comment.
[SUGGESTION]: node_ip may surface a non-primary/multi-homed IP or None without indication.
_guest_peer_entry takes ips[0] (the first entry of TailscaleIPs). Tailscale can return multiple addresses (e.g. IPv4 + IPv6, or the 100.x + a subnet route). The host's own mesh_status uses the same ips[0] heuristic for consistency, so this is internally consistent, but consumers of guests[*].node_ip should be aware it may be None (when TailscaleIPs is empty) or pick an unexpected address family. Consider documenting the selection or surfacing all IPs if callers need to choose. (Not a bug given fail-soft peers are skipped, just a future-risk note.)
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| json={"contact_id": "hub:hogne"}, | ||
| ) | ||
| assert r.status_code == 200 | ||
| assert r.json()["preauth_key"] == "guest-key-1" |
There was a problem hiding this comment.
[WARNING]: Test asserts the preauth key IS returned to the browser, which contradicts the endpoint's stated security contract.
The test asserts r.json()["preauth_key"] == "guest-key-1". If the endpoint is fixed to actually strip the key (per the CRITICAL finding on cluster_guest_preauth), this assertion will fail and must change. As written, the test locks in the insecure behavior and provides false assurance. The test should instead verify the key is NOT present in the browser-facing response and is delivered/stripped server-side.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Overview
Files Reviewed (2 files in incremental diff)
Resolved since previous review (commit d22a6da)
Notes (unchanged files, out of incremental scope)
Previous Review Summaries (5 snapshots, latest commit d22a6da)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit d22a6da)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Resolved since previous review (commit d22a6da)
Fix these issues in Kilo Cloud Previous review (commit 2f1e85e)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Resolved since previous review (commit c2c3ee8)
Fix these issues in Kilo Cloud Previous review (commit c2c3ee8)Status: No Issues Found | Recommendation: Merge All previously reported issues have been resolved in the changed code, and the incremental change since the last review (commit Resolved (verified against current HEAD
|
| Severity | Count |
|---|---|
| CRITICAL | 1 |
| WARNING | 2 |
| SUGGESTION | 1 |
Issue Details (click to expand)
CRITICAL
| File | Line | Issue |
|---|---|---|
tinyagentos/routes/account_proxy.py |
428 | Guest preauth key is relayed verbatim to the browser; the docstring's claim that it is "stripped from any browser-facing response" is false. Endpoint uses _forward (no _persist_join_credentials stripping) and the new preauth_key field is not in _STRIP_KEYS. |
WARNING
| File | Line | Issue |
|---|---|---|
tinyagentos/routes/account_proxy.py |
428 | No validation/authorization of contact_id (or any body field) before minting a guest credential upstream. |
tests/test_routes_account_proxy.py |
262 | Test asserts the preauth key IS returned to the browser, locking in the insecure behavior and contradicting the endpoint's security contract. |
SUGGESTION
| File | Line | Issue |
|---|---|---|
tinyagentos/taosnet/mesh.py |
117 | node_ip uses ips[0] and may be None or an unexpected address family for multi-homed peers; document selection or surface all IPs. |
Files Reviewed (4 files)
tinyagentos/taosnet/mesh.py- 1 suggestiontinyagentos/routes/account_proxy.py- 1 critical, 1 warningtests/test_mesh.py- 0 issuestests/test_routes_account_proxy.py- 1 warning
Reviewed by hy3:free · Input: 37.1K · Output: 1.8K · Cached: 106.1K
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tinyagentos/routes/account_proxy.py`:
- Around line 414-430: Update cluster_guest_preauth in
tinyagentos/routes/account_proxy.py (lines 414-430) to intercept the upstream
response, route the preauth_key through the existing out-of-band guest delivery
mechanism, and return JSON without that key to the browser. Update
tests/test_routes_account_proxy.py (lines 238-267) to assert the returned JSON
excludes preauth_key and verify the key is routed correctly.
🪄 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: 518c3b03-2e1c-4801-8cf4-b7edc329e503
📒 Files selected for processing (4)
tests/test_mesh.pytests/test_routes_account_proxy.pytinyagentos/routes/account_proxy.pytinyagentos/taosnet/mesh.py
… contact_id Kilo review (jaylfc#2038): CRITICAL — guest preauth key was relayed verbatim to the browser. WARNING — no contact_id validation before minting. - Add 'preauth_key' and 'guest_preauth_key' to _STRIP_KEYS set so _persist_join_credentials strips them from all responses. - Rewrite cluster_guest_preauth to validate contact_id (hub: prefix, length 5-256) at the edge before forwarding to taos.my. - Apply _persist_join_credentials stripping to the guest-preauth response (same pattern as the host join poll). - Update test to assert keys are stripped (not returned to browser) and add rejection test for 6 invalid contact_id shapes.
… contact_id Kilo review (jaylfc#2038): CRITICAL — guest preauth key was relayed verbatim to the browser. WARNING — no contact_id validation before minting. - Add 'preauth_key' and 'guest_preauth_key' to _STRIP_KEYS set so _persist_join_credentials strips them from all responses. - Rewrite cluster_guest_preauth to validate contact_id (hub: prefix, length 5-256) at the edge before forwarding to taos.my. - Apply _persist_join_credentials stripping to the guest-preauth response (same pattern as the host join poll). - Update test to assert keys are stripped (not returned to browser) and add rejection test for 6 invalid contact_id shapes.
9d4ee3e to
c2c3ee8
Compare
…auth proxy endpoint - Extend mesh_status() to parse the tailscale Peer map and return guest nodes tagged tag:guest (cross-user C2 transport). - Add POST /api/account/cluster/join/guest-preauth proxy endpoint that forwards to taos.my for minting ACL-pinned guest preauth keys. - Add 5 mesh tests (guest detection, non-guest exclusion, malformed data handling) and 1 account_proxy integration test. Part of jaylfc#2018 (EPIC jaylfc#2012 C2).
… contact_id Kilo review (jaylfc#2038): CRITICAL — guest preauth key was relayed verbatim to the browser. WARNING — no contact_id validation before minting. - Add 'preauth_key' and 'guest_preauth_key' to _STRIP_KEYS set so _persist_join_credentials strips them from all responses. - Rewrite cluster_guest_preauth to validate contact_id (hub: prefix, length 5-256) at the edge before forwarding to taos.my. - Apply _persist_join_credentials stripping to the guest-preauth response (same pattern as the host join poll). - Update test to assert keys are stripped (not returned to browser) and add rejection test for 6 invalid contact_id shapes.
c2c3ee8 to
2f1e85e
Compare
| # Strip the preauth key from the response so it never reaches the browser. | ||
| # The caller (peer channel / delegation-accept handler) must capture the key | ||
| # from the stripped response upstream before returning to the browser. | ||
| stripped, _ = _persist_join_credentials(resp) |
There was a problem hiding this comment.
[WARNING]: The guest preauth key is stripped and then discarded — it can never be delivered out-of-band as the docstring (lines 424-428) and PR claim require.
_persist_join_credentials builds join_intent only from headscale_preauth_key (account_proxy.py:488-494), never from the guest preauth_key that it strips at line 496. The call here discards the second tuple element (stripped, _ = ...), so the guest key is erased with no server-side capture path. The docstring's instruction that "the caller (peer channel, D1) must extract the key server-side before the response is sent to the browser" is not actually possible with this code — the key is already gone. Either _persist_join_credentials should populate join_intent with the guest preauth_key (and this endpoint must use it), or the endpoint must read the key from the upstream response before stripping. As written, the feature mints a credential that is neither returned to the browser nor delivered to the guest, making the delegation-accept flow non-functional. The test only asserts the key is stripped, so this gap is untested.
Kilo WARNING (jaylfc#2038, post-rebase): _persist_join_credentials only captured headscale_preauth_key into join_intent, never the guest preauth_key. The cluster_guest_preauth endpoint stripped and discarded it (_, _ pattern), so the key could never reach the guest instance. - _persist_join_credentials now captures preauth_key (guest) in addition to headscale_preauth_key (host), with guest key taking priority. - Add _guest_preauth_intents module-level dict keyed by contact_id so the D1 delegation-accept handler can retrieve the key for out-of-band delivery via the peer channel. - cluster_guest_preauth now captures and stores join_intent. - Test asserts join_intent is stored server-side.
| # via the peer channel (D1 delegation-accept handler). | ||
| stripped, join_intent = _persist_join_credentials(resp) | ||
| if join_intent: | ||
| _guest_preauth_intents[cid] = join_intent |
There was a problem hiding this comment.
[WARNING]: Guest preauth keys are now persisted server-side, but this PR stores live single-use credentials in an unbounded in-memory dict with no eviction/rotation and no consuming handler.
_guest_preauth_intents[cid] = join_intent holds real single-use preauth keys indefinitely. The D1 delegation-accept consumer that is supposed to read and clear these entries is not in this PR (only the producer is), so every minted guest key accumulates in memory for the process lifetime. Combine that with the lack of a TTL/pop, and a host that triggers delegation-accept repeatedly (or a bad actor polling the endpoint) can grow this map without bound while preserving valid secrets.
Recommend: consume-and-delete (pop) the intent in the D1 handler, add a TTL/eviction (single-use keys are stale after a short window anyway — see the _attempted_preauth rationale), and treat the dict as secret-bearing state. As written, the keys are captured but never delivered or cleared.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tinyagentos/routes/account_proxy.py (1)
71-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused
cluster_guest_preauthfrom_ACTIONS.With the introduction of the dedicated
@router.post("/api/account/cluster/join/guest-preauth")endpoint that correctly validates the request body and strips secrets using_persist_join_credentials, this entry in_ACTIONSis now unused. Leaving it here is a potential security risk: if a dynamic route dispatcher ever maps_ACTIONSvia the generic_forward()method, it could bypass your new sanitization logic and leak the unstrippedpreauth_keyto the client.♻️ Proposed fix
- # Guest preauth key minting (cross-user C2). The host calls same-origin - # /api/account/cluster/join/guest-preauth at delegation-accept time to mint - # a scoped, ACL-pinned preauth key the guest instance will use to join the - # host's mesh. The taos.my side creates the key via the Headscale admin API. - "cluster_guest_preauth": ("POST", "/api/cluster/join/guest-preauth"), }🤖 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 71 - 75, Remove the unused "cluster_guest_preauth" mapping from the _ACTIONS definition, leaving the dedicated guest-preauth route and its _persist_join_credentials sanitization logic unchanged.tests/test_mesh.py (1)
90-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove this redundant and environment-dependent test.
This test does not mock the
_runcall or Tailscale installation status. In a typical CI environment where Tailscale is not running,s.get("joined")will evaluate toFalse, causing the test to silently skip its assertion.Furthermore, the behavior it aims to verify (that
guestsis an empty list when there are no peers) is already effectively covered intest_joined_parseon line 88.♻️ Proposed refactor
- async def test_guests_field_present_even_when_no_peers(self): - """mesh_status always includes ``guests`` (empty list when no guest peers).""" - s = await mesh.mesh_status() - # Fast-path: not installed returns joined=False with detail, never raises. - assert isinstance(s, dict) - if s.get("joined"): - assert isinstance(s.get("guests"), list) -🤖 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 `@tests/test_mesh.py` around lines 90 - 97, Remove the redundant environment-dependent test_guests_field_present_even_when_no_peers test; retain coverage through test_joined_parse, which already verifies the guests behavior without relying on the local Tailscale installation state.
🤖 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.
Nitpick comments:
In `@tests/test_mesh.py`:
- Around line 90-97: Remove the redundant environment-dependent
test_guests_field_present_even_when_no_peers test; retain coverage through
test_joined_parse, which already verifies the guests behavior without relying on
the local Tailscale installation state.
In `@tinyagentos/routes/account_proxy.py`:
- Around line 71-75: Remove the unused "cluster_guest_preauth" mapping from the
_ACTIONS definition, leaving the dedicated guest-preauth route and its
_persist_join_credentials sanitization logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 42d884ed-e870-4d4f-a470-a3bb728e412d
📒 Files selected for processing (4)
tests/test_mesh.pytests/test_routes_account_proxy.pytinyagentos/routes/account_proxy.pytinyagentos/taosnet/mesh.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tinyagentos/taosnet/mesh.py
|
@kilo-code-bot review |
Kilo WARNING (jaylfc#2038, round 2): _guest_preauth_intents was an unbounded in-memory dict with no eviction, holding live single-use credentials indefinitely without a consuming handler. - Add _GUEST_PREAUTH_TTL_SECONDS (300s, Headscale default expiry). - Replace plain dict with timestamped tuples (float, dict). - Add _pop_guest_preauth_intent() accessor: consume-and-delete with lazy sweep of all stale entries on every access. - Test verifies one-shot consumption (second pop returns None).
|
@kilo-code-bot review |
|
@kilo-code-bot review |
Code Review SummaryStatus: No New Issues Found (incremental) | Recommendation: Merge Overview
Incremental Changes Reviewed (1 file)
Re-verified prior findings (resolved / documented)
Previous Review Summary (commit 39b2e02)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 39b2e02)Status: 1 Suggestion Found | Recommendation: Merge (minor follow-up)
Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Verified resolved since prior rounds
Reviewed by hy3:free · Input: 35.6K · Output: 1.6K · Cached: 98.2K |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tinyagentos/routes/account_proxy.py (1)
447-448: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent unbounded memory growth on the insertion path.
Although stale entries are swept during read operations (
_pop_guest_preauth_intent), the dictionary will still grow without bounds if a malicious actor repeatedly polls this endpoint and the downstream consumer is inactive or fails to pop the entries. Consider sweeping stale entries on insertion as well to ensure the memory footprint remains strictly bounded by the TTL window.🛠️ Proposed fix
if join_intent: - _guest_preauth_intents[cid] = (time.monotonic(), join_intent) + now = time.monotonic() + _guest_preauth_intents[cid] = (now, join_intent) + # Sweep stale entries to prevent unbounded growth if consumer is inactive + expired = [k for k, (ts, _) in _guest_preauth_intents.items() if now - ts > _GUEST_PREAUTH_TTL_SECONDS] + for k in expired: + _guest_preauth_intents.pop(k, None)(Note: If you prefer, you can extract the 3-line sweep logic into a reusable helper function and call it from both the insertion and pop paths to avoid code duplication).
🤖 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 447 - 448, Update the join_intent insertion path to sweep expired entries from _guest_preauth_intents before storing the new (time.monotonic(), join_intent) value, reusing the existing stale-entry logic from _pop_guest_preauth_intent or extracting it into a shared helper so the dictionary remains bounded by the TTL window.
🧹 Nitpick comments (1)
tinyagentos/routes/account_proxy.py (1)
532-533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant expiration check.
The expiration check
now - entry[0] > _GUEST_PREAUTH_TTL_SECONDSis logically redundant and will always evaluate toFalse. Any expired entry would have already been removed by the sweep on lines 528-530 using the exact samenowtimestamp. It can be safely removed.♻️ Proposed refactor
- if entry is None or now - entry[0] > _GUEST_PREAUTH_TTL_SECONDS: + if entry is None: return None🤖 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 532 - 533, Remove the redundant expiration condition from the entry lookup guard in the account proxy flow, leaving only the None check after the preceding sweep. Preserve the existing sweep in the surrounding preauthorization logic, which remains responsible for removing expired entries.
🤖 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.
Duplicate comments:
In `@tinyagentos/routes/account_proxy.py`:
- Around line 447-448: Update the join_intent insertion path to sweep expired
entries from _guest_preauth_intents before storing the new (time.monotonic(),
join_intent) value, reusing the existing stale-entry logic from
_pop_guest_preauth_intent or extracting it into a shared helper so the
dictionary remains bounded by the TTL window.
---
Nitpick comments:
In `@tinyagentos/routes/account_proxy.py`:
- Around line 532-533: Remove the redundant expiration condition from the entry
lookup guard in the account proxy flow, leaving only the None check after the
preceding sweep. Preserve the existing sweep in the surrounding preauthorization
logic, which remains responsible for removing expired entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cd3c45c-bd60-4f9e-a953-4ee5902a3b87
📒 Files selected for processing (2)
tests/test_routes_account_proxy.pytinyagentos/routes/account_proxy.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_routes_account_proxy.py
…stream by taos.my Addresses Kilo review SUGGESTION: the proxy validates format at the edge (defense-in-depth), while contact-to-caller ownership authorization is the responsibility of the upstream taos.my service which has access to the contacts database.
…emove redundant expiry check
Summary
Implements the taOS-side pieces of C2 (guest-preauth mint + ACL automation + mesh-status guest links) from EPIC #2012.
Closes #2018.
Changes
1. Extend
mesh_status()to surface guest peer nodesPeermap fromtailscale status --jsontag:guest(ACL-pinned guest instances)guestslist:[{hostname, node_ip, online}]2. Add guest-preauth proxy endpoint
POST /api/account/cluster/join/guest-preauthforwards to taos.mysingle-use preauth key for the guest collaborator instance
from browser-facing responses, delivered out-of-band via peer channel
3. Tests: 16 mesh tests + 1 account_proxy integration test
Files changed (4)
tinyagentos/taosnet/mesh.py— +40/-3tinyagentos/routes/account_proxy.py— +22tests/test_mesh.py— +51 (5 new tests)tests/test_routes_account_proxy.py— +30 (1 new test)Test results
Summary by CodeRabbit
POST /api/account/cluster/join/guest-preauthto mint and temporarily hold guest join preauthorization intents.guestslist for peers tagged as guests (hostname, IP, online state).contact_idvalues.