Conversation
On hub friend-accept, create the contact row (pinning Ed25519/X25519 pubkeys from the directory response), mint the inbound peer token, fire a background handshake envelope to the peer's endpoints, and record the peer_link. Block cascades to revoke_peer_link. - routes/hub.py: extend accept_friend_request to create contact row, mint peer token, and deliver handshake (fire-and-forget). Extend block_peer to cascade to contacts_store.revoke_peer_link via fingerprint→username lookup through hub_authors. - peer.py: add send_handshake() and deliver_handshake() for building and delivering handshake envelopes to peer endpoints. - tests: 4 new tests — accept creates contact, accept creates peer_link, accept without contacts_store doesn't crash, block cascades to revoke peer_link. All 53 targeted tests pass (hub + contacts + peer). Task: t_a3481cde Closes jaylfc#2014
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? |
hognek
marked this pull request as ready for review
July 19, 2026 12:23
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
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? |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if contacts is not None: | ||
| try: | ||
| # Resolve fingerprint → hub_username via the local hub_authors table. | ||
| author = await store.get_author(peer) |
There was a problem hiding this comment.
WARNING: Block cascade will not revoke the peer link in the normal flow.
This relies on store.get_author(peer) (hub_authors by fingerprint) to map the blocked peer fingerprint -> hub_username -> contact_id. But accept_friend_request (the A2 block above) never inserts a hub_authors row for the peer — it only records the friend edge in relationships and creates the contact/peer_link directly from the directory response. So in production get_author(peer) returns None and the revoke_peer_link call is skipped; the peer link silently stays active after a block.
The new test_block_revokes_peer_link only passes because it manually upsert_author("peerFP", username="hogne") first — that step is not performed by the real accept handler, so the test does not reflect production behavior. Either accept_friend_request should persist the peer's fingerprint->username mapping in hub_authors (or another stable lookup), or block_peer should resolve the contact_id via the contact row / relationships edge instead of hub_authors.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| from tinyagentos.hub import identity as _hub_identity | ||
|
|
||
| local_ident = _hub_identity.public_identity() | ||
| from_username = resolve_local_identity_id() |
There was a problem hiding this comment.
WARNING: send_handshake resolves the local identity without a data_dir.
Here resolve_local_identity_id() is called with no data_dir, so it resolves hub.db from TAOS_DATA_DIR env or the default project data/hub path. But the caller in routes/hub.py resolves the request's configured identity via _resolve_local_identity_id(request.app.state.data_dir) (line 394) and intends this same identity to sign the envelope. In any deployment where the hub data dir differs from the default, send_handshake can resolve a different local identity (or raise RuntimeError("cannot send handshake: no local hub identity")), causing the handshake to be built/signed under the wrong identity or fail entirely.
send_handshake should accept a data_dir parameter (or take the already-resolved identity id) and pass it through to resolve_local_identity_id(data_dir).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| try: | ||
| for ep in peer_endpoints: | ||
| url = ep.rstrip("/") + "/api/peer/inbox" |
There was a problem hiding this comment.
SUGGESTION: Handshake transmits the plaintext inbound_token secret; enforce TLS and surface delivery failures.
The envelope body carries inbound_token — the secret the peer uses as Bearer to authenticate against our /api/peer/* routes. It is POSTed to the peer's peer_endpoints, which are attacker/peer-supplied and may be plain http://. If a peer endpoint is HTTP, the token travels in cleartext. Consider rejecting/warning on non-HTTPS endpoints, and ensure TLS verification stays on.
Also, the inner except Exception: continue (line 248) silently swallows all errors for every endpoint, so a misconfigured/broken handshake delivery produces no log and always returns False without diagnostics. At minimum, log the failure reason per endpoint so the fire-and-forget task isn't entirely invisible.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 81.8K · Output: 6.9K · Cached: 206.7K |
Owner
|
This and #2043 are both the A2 friend-accept slice from sibling branches (feat/collab-a2-friend-accept vs feat/collab-a2-friend-accept-handshake). This one looks like the superset (adds peer.py wiring + the fuller test file). Please close #2043 if this supersedes it, or tell me what #2043 carries that this does not - one PR per slice (pitfall 13). |
Owner
|
Three Kilo findings to fold before merge (CI is green and the token-hash storage is right, so these are the only gate):
Also: please close #2043 or state what it carries beyond this PR - still open from my earlier ask. Fold these and A2 merges. |
Contributor
Author
|
Superseded by #2043. Both PRs implement the A2 friend-accept slice and Kilo flagged the same block-cascade issue on both (the hub_authors lookup returns None because accept_friend_request never caches the peer fingerprint→username mapping, so revoke_peer_link is silently skipped). #2043 has the more comprehensive implementation with 8 tests (vs 4), additional findings to address, and jaylfc's preference. Closing in favor of #2043. |
Contributor
Author
hognek
added a commit
to hognek/tinyagentos
that referenced
this pull request
Jul 20, 2026
…gerprint jaylfc deep review at 4b5903b — fold all six findings: 1. BLOCKER: peer_fingerprint retrofit migration was a no-op on every pre-existing DB. BaseStore's migration runner uses baseline-at-latest semantics — existing DBs get stamped at version 1 without executing the ALTER, so the column was absent after init(). The broad except in _try_handshake swallowed the resulting OperationalError, and the block-cascade security fix was similarly swallowed. Replaced the MIGRATIONS list with a guarded _post_init that checks PRAGMA table_info('contacts') and ALTER TABLE ADD COLUMN only when peer_fingerprint is absent. Same pattern as agent_registry_store's _migration_v1_add_status. Fresh databases still get the column from SCHEMA; upgraded databases get it from _post_init. Added two ContactsStore upgrade tests in test_store_upgrades.py following the existing pattern — column-presence check and add_contact-after-upgrade. 2. Fold 1 (send_handshake): A2 intentionally stores the inbound token locally without delivering it — the token exchange channel doesn't exist yet. A3 completes the two-way exchange. The send_handshake envelope builder from jaylfc#2046 is deferred to a follow-up PR linked from the tracking issue. This is a spec deviation from cross-user-collaboration.md Day 0 (mint token on BOTH sides), filed as a tracking issue. 3. Fold 3 (.gitignore): the data/hub/ ignore line is justified — this branch's own history committed identity.json with throwaway test keys at 2b28043 (removed at 500da60). The .gitignore prevents future accidental commits. Squash merge will keep dev history clean. 4. Key hygiene: the keys in 2b28043 were throwaway test keys never used against real endpoints. Squash merge removes them from dev history. jaylfc#2042 re-commits the same file; coordination note added in-thread. 5. Re-trigger: @coderabbitai review after push. 6. Track-don't-block (Kilo W1): accepted the documented NOTE about contact_id bound to peer-controllable username. Follow-up issue filed for fingerprint-keyed contact IDs in a future slice. BONUS: Fixed CodeRabbit nit from head review — test_accept_reupsert_contact now actually revokes between accepts to verify re-establishment clears revoked_at (was a no-op assertion before).
hognek
added a commit
to hognek/tinyagentos
that referenced
this pull request
Jul 27, 2026
…gerprint jaylfc deep review at 4b5903b — fold all six findings: 1. BLOCKER: peer_fingerprint retrofit migration was a no-op on every pre-existing DB. BaseStore's migration runner uses baseline-at-latest semantics — existing DBs get stamped at version 1 without executing the ALTER, so the column was absent after init(). The broad except in _try_handshake swallowed the resulting OperationalError, and the block-cascade security fix was similarly swallowed. Replaced the MIGRATIONS list with a guarded _post_init that checks PRAGMA table_info('contacts') and ALTER TABLE ADD COLUMN only when peer_fingerprint is absent. Same pattern as agent_registry_store's _migration_v1_add_status. Fresh databases still get the column from SCHEMA; upgraded databases get it from _post_init. Added two ContactsStore upgrade tests in test_store_upgrades.py following the existing pattern — column-presence check and add_contact-after-upgrade. 2. Fold 1 (send_handshake): A2 intentionally stores the inbound token locally without delivering it — the token exchange channel doesn't exist yet. A3 completes the two-way exchange. The send_handshake envelope builder from jaylfc#2046 is deferred to a follow-up PR linked from the tracking issue. This is a spec deviation from cross-user-collaboration.md Day 0 (mint token on BOTH sides), filed as a tracking issue. 3. Fold 3 (.gitignore): the data/hub/ ignore line is justified — this branch's own history committed identity.json with throwaway test keys at 2b28043 (removed at 500da60). The .gitignore prevents future accidental commits. Squash merge will keep dev history clean. 4. Key hygiene: the keys in 2b28043 were throwaway test keys never used against real endpoints. Squash merge removes them from dev history. jaylfc#2042 re-commits the same file; coordination note added in-thread. 5. Re-trigger: @coderabbitai review after push. 6. Track-don't-block (Kilo W1): accepted the documented NOTE about contact_id bound to peer-controllable username. Follow-up issue filed for fingerprint-keyed contact IDs in a future slice. BONUS: Fixed CodeRabbit nit from head review — test_accept_reupsert_contact now actually revokes between accepts to verify re-establishment clears revoked_at (was a no-op assertion before).
hognek
added a commit
to hognek/tinyagentos
that referenced
this pull request
Jul 27, 2026
…gerprint jaylfc deep review at 4b5903b — fold all six findings: 1. BLOCKER: peer_fingerprint retrofit migration was a no-op on every pre-existing DB. BaseStore's migration runner uses baseline-at-latest semantics — existing DBs get stamped at version 1 without executing the ALTER, so the column was absent after init(). The broad except in _try_handshake swallowed the resulting OperationalError, and the block-cascade security fix was similarly swallowed. Replaced the MIGRATIONS list with a guarded _post_init that checks PRAGMA table_info('contacts') and ALTER TABLE ADD COLUMN only when peer_fingerprint is absent. Same pattern as agent_registry_store's _migration_v1_add_status. Fresh databases still get the column from SCHEMA; upgraded databases get it from _post_init. Added two ContactsStore upgrade tests in test_store_upgrades.py following the existing pattern — column-presence check and add_contact-after-upgrade. 2. Fold 1 (send_handshake): A2 intentionally stores the inbound token locally without delivering it — the token exchange channel doesn't exist yet. A3 completes the two-way exchange. The send_handshake envelope builder from jaylfc#2046 is deferred to a follow-up PR linked from the tracking issue. This is a spec deviation from cross-user-collaboration.md Day 0 (mint token on BOTH sides), filed as a tracking issue. 3. Fold 3 (.gitignore): the data/hub/ ignore line is justified — this branch's own history committed identity.json with throwaway test keys at 2b28043 (removed at 500da60). The .gitignore prevents future accidental commits. Squash merge will keep dev history clean. 4. Key hygiene: the keys in 2b28043 were throwaway test keys never used against real endpoints. Squash merge removes them from dev history. jaylfc#2042 re-commits the same file; coordination note added in-thread. 5. Re-trigger: @coderabbitai review after push. 6. Track-don't-block (Kilo W1): accepted the documented NOTE about contact_id bound to peer-controllable username. Follow-up issue filed for fingerprint-keyed contact IDs in a future slice. BONUS: Fixed CodeRabbit nit from head review — test_accept_reupsert_contact now actually revokes between accepts to verify re-establishment clears revoked_at (was a no-op assertion before).
hognek
pushed a commit
to hognek/tinyagentos
that referenced
this pull request
Aug 2, 2026
…into peer.py Fold the sender-side handshake code from PR jaylfc#2046 into this branch's peer.py. The send_handshake() function builds an Ed25519-signed handshake envelope addressed to a remote contact, carrying the inbound peer token, advertised endpoints, and public keys. deliver_handshake() delivers the envelope to the peer's endpoints (best-effort, first-2xx). This resolves jaylfc's HOLD (1): the PR previously only had hub.py receive side — the sender side from jaylfc#2046 is now included. HOLD (2) — the peer_fingerprint migration — was already resolved in a prior commit (5281509) which replaced the MIGRATIONS entry with a guarded _post_init (PRAGMA table_info + ALTER TABLE). Existing DB upgrade tests (test_store_upgrades.py::TestContactsStoreUpgrade) pass.
hognek
added a commit
to hognek/tinyagentos
that referenced
this pull request
Aug 2, 2026
…into peer.py Fold the sender-side handshake code from PR jaylfc#2046 into this branch's peer.py. The send_handshake() function builds an Ed25519-signed handshake envelope addressed to a remote contact, carrying the inbound peer token, advertised endpoints, and public keys. deliver_handshake() delivers the envelope to the peer's endpoints (best-effort, first-2xx). This resolves jaylfc's HOLD (1): the PR previously only had hub.py receive side — the sender side from jaylfc#2046 is now included. HOLD (2) — the peer_fingerprint migration — was already resolved in a prior commit (5281509) which replaced the MIGRATIONS entry with a guarded _post_init (PRAGMA table_info + ALTER TABLE). Existing DB upgrade tests (test_store_upgrades.py::TestContactsStoreUpgrade) pass.
hognek
added a commit
to hognek/tinyagentos
that referenced
this pull request
Aug 2, 2026
…gerprint jaylfc deep review at 4b5903b — fold all six findings: 1. BLOCKER: peer_fingerprint retrofit migration was a no-op on every pre-existing DB. BaseStore's migration runner uses baseline-at-latest semantics — existing DBs get stamped at version 1 without executing the ALTER, so the column was absent after init(). The broad except in _try_handshake swallowed the resulting OperationalError, and the block-cascade security fix was similarly swallowed. Replaced the MIGRATIONS list with a guarded _post_init that checks PRAGMA table_info('contacts') and ALTER TABLE ADD COLUMN only when peer_fingerprint is absent. Same pattern as agent_registry_store's _migration_v1_add_status. Fresh databases still get the column from SCHEMA; upgraded databases get it from _post_init. Added two ContactsStore upgrade tests in test_store_upgrades.py following the existing pattern — column-presence check and add_contact-after-upgrade. 2. Fold 1 (send_handshake): A2 intentionally stores the inbound token locally without delivering it — the token exchange channel doesn't exist yet. A3 completes the two-way exchange. The send_handshake envelope builder from jaylfc#2046 is deferred to a follow-up PR linked from the tracking issue. This is a spec deviation from cross-user-collaboration.md Day 0 (mint token on BOTH sides), filed as a tracking issue. 3. Fold 3 (.gitignore): the data/hub/ ignore line is justified — this branch's own history committed identity.json with throwaway test keys at 2b28043 (removed at 500da60). The .gitignore prevents future accidental commits. Squash merge will keep dev history clean. 4. Key hygiene: the keys in 2b28043 were throwaway test keys never used against real endpoints. Squash merge removes them from dev history. jaylfc#2042 re-commits the same file; coordination note added in-thread. 5. Re-trigger: @coderabbitai review after push. 6. Track-don't-block (Kilo W1): accepted the documented NOTE about contact_id bound to peer-controllable username. Follow-up issue filed for fingerprint-keyed contact IDs in a future slice. BONUS: Fixed CodeRabbit nit from head review — test_accept_reupsert_contact now actually revokes between accepts to verify re-establishment clears revoked_at (was a no-op assertion before).
hognek
added a commit
to hognek/tinyagentos
that referenced
this pull request
Aug 2, 2026
…into peer.py Fold the sender-side handshake code from PR jaylfc#2046 into this branch's peer.py. The send_handshake() function builds an Ed25519-signed handshake envelope addressed to a remote contact, carrying the inbound peer token, advertised endpoints, and public keys. deliver_handshake() delivers the envelope to the peer's endpoints (best-effort, first-2xx). This resolves jaylfc's HOLD (1): the PR previously only had hub.py receive side — the sender side from jaylfc#2046 is now included. HOLD (2) — the peer_fingerprint migration — was already resolved in a prior commit (5281509) which replaced the MIGRATIONS entry with a guarded _post_init (PRAGMA table_info + ALTER TABLE). Existing DB upgrade tests (test_store_upgrades.py::TestContactsStoreUpgrade) pass.
jaylfc
added a commit
that referenced
this pull request
Aug 28, 2026
…st_init (#2561) * feat(hub): friend-accept creates contact row and peer-link handshake On friend-accept: - Extract peer Ed25519/X25519 pubkeys from directory response - Fall back to hub_authors cache when directory omits pubkeys - Create contact row (trust-on-first-use key pinning) - Mint inbound peer token (hashed at rest) - Establish peer link with advertised endpoints - Handshake is best-effort — failures never block the accept On block: - Cascade to contacts_store.revoke_peer_link() - Resolve fingerprint->username via hub_authors cache Tests: 8/8 pass (contact creation, pubkey fallback, no-pubkey skip, endpoint parsing, re-upsert, missing-store guard, block cascade, block cascade missing-store). Existing 37 contacts_peer tests unaffected. Part of #2012 (cross-user collaboration), milestone A2. Closes #2014. * fix(hub): address Kilo findings — block-cascade fallback, token-flow doc, dead test code - WARNING: block cascade now falls back to contact-table scan when hub_authors cache is missing, with explicit log warning on failure - WARNING: document that A2 intentionally stores inbound token locally without delivering it (A3 completes the exchange) - SUGGESTION: remove dead test code (placeholder token lookup) * fix(hub): address Kilo round 2 — remove committed keys, fix cascade fallback, doc contact_id - CRITICAL: remove committed data/hub/identity.json (test-generated keys) and add data/hub/ to .gitignore - WARNING: document that contact_id is derived from untrusted directory username (TOFU key-pinning bound to peer-controllable name) with future direction - SUGGESTION: remove broken fingerprint-vs-pubkey fallback in block cascade (peer fingerprint != ed25519_pub key — comparison would never match) * fix(hub): address CodeRabbit findings — HubStore close, fingerprint verification - SUGGESTION: wrap HubStore init/upsert in try/finally with close() in both test_collab_a2_handshake.py locations to prevent leaked database connections - SUGGESTION: verify directory-supplied ed25519_pub fingerprint matches expected peer_fingerprint in _try_handshake; skip handshake on mismatch to avoid pinning TOFU keys from an imposter - Update _PEER_FP test constant to actual fingerprint of _PEER_SIGNING_PUB so the new fingerprint check passes consistently Tests: 103/103 pass (collab A2 handshake + hub + contacts peer) * fix(hub): widen handshake exception boundary and implement block-cascade fingerprint fallback - Widen try/except in _try_handshake to cover hub_authors lookup, fingerprint validation, and endpoint processing — prevents ValueError from bytes.fromhex() on malformed directory pubkeys from crashing the accept endpoint (CodeRabbit CRITICAL). - Add peer_fingerprint column to contacts table with migration, store it at friend-accept for stable fingerprint→contact lookup. - Implement fingerprint-based fallback in block_peer's contact cascade: when hub_authors is missing or stale, resolve via get_contact_by_fingerprint() instead of silently skipping. - Rename hub_store→store in _try_handshake to avoid shadowing the module-level import (CodeRabbit nit). - Add test_block_cascade_fingerprint_fallback: verifies block revokes peer link via fingerprint when hub_authors is empty. * fix(contacts): replace migration with guarded _post_init for peer_fingerprint jaylfc deep review at 4b5903b — fold all six findings: 1. BLOCKER: peer_fingerprint retrofit migration was a no-op on every pre-existing DB. BaseStore's migration runner uses baseline-at-latest semantics — existing DBs get stamped at version 1 without executing the ALTER, so the column was absent after init(). The broad except in _try_handshake swallowed the resulting OperationalError, and the block-cascade security fix was similarly swallowed. Replaced the MIGRATIONS list with a guarded _post_init that checks PRAGMA table_info('contacts') and ALTER TABLE ADD COLUMN only when peer_fingerprint is absent. Same pattern as agent_registry_store's _migration_v1_add_status. Fresh databases still get the column from SCHEMA; upgraded databases get it from _post_init. Added two ContactsStore upgrade tests in test_store_upgrades.py following the existing pattern — column-presence check and add_contact-after-upgrade. 2. Fold 1 (send_handshake): A2 intentionally stores the inbound token locally without delivering it — the token exchange channel doesn't exist yet. A3 completes the two-way exchange. The send_handshake envelope builder from #2046 is deferred to a follow-up PR linked from the tracking issue. This is a spec deviation from cross-user-collaboration.md Day 0 (mint token on BOTH sides), filed as a tracking issue. 3. Fold 3 (.gitignore): the data/hub/ ignore line is justified — this branch's own history committed identity.json with throwaway test keys at 2b28043 (removed at 500da60). The .gitignore prevents future accidental commits. Squash merge will keep dev history clean. 4. Key hygiene: the keys in 2b28043 were throwaway test keys never used against real endpoints. Squash merge removes them from dev history. #2042 re-commits the same file; coordination note added in-thread. 5. Re-trigger: @coderabbitai review after push. 6. Track-don't-block (Kilo W1): accepted the documented NOTE about contact_id bound to peer-controllable username. Follow-up issue filed for fingerprint-keyed contact IDs in a future slice. BONUS: Fixed CodeRabbit nit from head review — test_accept_reupsert_contact now actually revokes between accepts to verify re-establishment clears revoked_at (was a no-op assertion before). * fix(hub): normalize endpoints to dict form and guard re-accept on REL_BLOCK 1) _try_handshake stores directory_resp['endpoints'] as a list of strings but the only consumer (#2045's contact grid) expects dicts with url/kind/priority fields. Normalize bare strings to {'kind': 'hub', 'url': e, 'priority': i} in the handshake path. 2) A blocked peer (REL_BLOCK edge in hub_relationships) is resurrected on re-accept because _try_handshake runs unconditionally. Guard the handshake with a has_edge check before any contact-store operations. * fix(hub): address 3 small items from jaylfc review on #2043 1. Security regression tests: anti-imposter (mismatched pubkey → no contact), authz-rejection (403 → no handshake), REL_BLOCK guard (blocked contact not resurrected by re-accept) 2. Docs deviation: note mint-without-delivery for A2 friend-accept in cross-user-collaboration.md 3. Block cascade: call set_contact_status(cid, 'blocked') so the distinct status is used rather than leaving it at the prior accepted state * fix(hub): move set_contact_status call after both block-cascade branches * fix(tests): repair two security regression tests for #2043 - test_authz_rejection: accept route returns upstream status code (403), not 200 wrapped — update assertion and state check - test_block_guard: _try_handshake guard checks hub REL_BLOCK not contact status — add REL_BLOCK relationship in test setup * ci: retrigger CI after sniffio infra failure in shard (3.13, 4) * fix(collab): fold send_handshake + deliver_handshake from #2046 into peer.py Fold the sender-side handshake code from PR #2046 into this branch's peer.py. The send_handshake() function builds an Ed25519-signed handshake envelope addressed to a remote contact, carrying the inbound peer token, advertised endpoints, and public keys. deliver_handshake() delivers the envelope to the peer's endpoints (best-effort, first-2xx). This resolves jaylfc's HOLD (1): the PR previously only had hub.py receive side — the sender side from #2046 is now included. HOLD (2) — the peer_fingerprint migration — was already resolved in a prior commit (5281509) which replaced the MIGRATIONS entry with a guarded _post_init (PRAGMA table_info + ALTER TABLE). Existing DB upgrade tests (test_store_upgrades.py::TestContactsStoreUpgrade) pass. * fix: address CodeRabbit findings on PR #2043 — block-guard, peer_links assertions, fixture leak - Wrap block-guard has_edge() call inside try block so a store failure never blocks the accept (best-effort handshake contract). - Add peer_links assertions to three negative-path tests (no-pubkeys, imposter pubkey, 403 rejection) verifying that no token-bearing artifact is created when the handshake is skipped. - Convert app_with_contacts fixture to yield/close to prevent contacts_store database file leak during tmp_data_dir teardown. * chore: retrigger CI (CLA author fix + doc-gate) Docs-Reviewed: retrigger CI after author identity fix; no API surface changes * fix(hub): key TOFU contact pin on signing-key fingerprint, not username (#2043) contact_id was derived from the peer-controlled directory username, so a username collision or rename could overwrite a pinned contact's key material or fragment the same peer across two contact rows. Key on the fingerprint (contact_id = 'hub:{fingerprint}'), drop the UNIQUE constraint on hub_username, and make block_peer resolve via get_contact_by_fingerprint as the primary path. * fix(hub): report revoke matches + revoke all fingerprint contacts (#2043) Complete the two supporting changes jaylfc required alongside the fingerprint-keyed TOFU pin: 1. revoke_peer_link now returns a bool (True when a peer_link row matched) and block_peer logs loudly when a revoke matched zero rows, so a fail-open revoke can never be silently reported as success. 2. The block cascade now revokes every contact pinned to a fingerprint via get_contacts_by_fingerprint instead of get_contact_by_fingerprint's rows[0]. Legacy username-keyed rows (or a rename mid-flight) can leave several contacts sharing a fingerprint; revoking only the first would leave a live peer link behind. Adds test_block_cascade_revokes_all_contacts_sharing_fingerprint (two legacy contacts, one fingerprint, both must end revoked+blocked). * fix(contacts): backfill peer_fingerprint for pre-existing rows in _post_init Without backfill, contacts that predate the peer_fingerprint column keep DEFAULT '' forever. block_peer (routes/hub.py) resolves peers by fingerprint only, so every pre-existing contact is unreachable by the block path — the peer link is never revoked and the blocked peer keeps authenticating on /api/peer/*. - Backfill peer_fingerprint from identity.fingerprint(ed25519_pub) for all rows where peer_fingerprint is empty but ed25519_pub is set. - Add regression test that seeds a v0 (pre-column) contacts DB and verifies fingerprints are backfilled on upgrade. - Document x25519_pub as accepted unverified (no verification protocol at this head; re-pinned every accept). - Flag deliver_handshake SSRF risk: POSTs to peer-supplied URLs with no guard — wire an ssrf-safe transport before adding a caller. * fix(contacts): guard fingerprint backfill against malformed ed25519_pub _bytes.fromhex in _compute_fingerprint raises ValueError on non-hex key material (odd-length strings, non-hex chars, embedded NULs). A single v0 row with malformed ed25519_pub would crash init() and brick the entire contacts store on upgrade. - Wrap per-row _compute_fingerprint in try/except (ValueError, TypeError); log a warning and skip the row so the rest of the backfill completes. - Add regression test: seed a v0 DB with one valid and one malformed ed25519_pub row; assert init() completes, the valid row is backfilled, and the malformed row is left alone (not bricked). * fix(tests): arm collab_a2_handshake client with CSRF event hooks after #2547 inversion The conftest CSRF inversion (#2547, f1b01d9) made verify_csrf enforce for every test that builds its own AsyncClient. The client_with_contacts fixture injected taos_session but was missing event_hooks, so every POST to /api/hub/friends/requests/{rid}/accept and /api/hub/friends/block returned 403 instead of 200. Add the csrf_event_hooks import and pass it at client construction time, matching the pattern applied to 42 other test modules in the CSRF sweep. * docs(changelog): add fragment for #2561 contacts fingerprint keying --------- Co-authored-by: hognek <227774406+hognek@users.noreply.github.com> Co-authored-by: jaylfc <jaylfc25@gmail.com>
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
2 participants
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.
A2: Friend-accept → contact row + peer-link handshake
On hub friend-accept, creates the contact row (pinning Ed25519/X25519 pubkeys from the directory response), mints the inbound peer token, fires a background handshake envelope to the peer's endpoints, and records the peer_link. Block cascades to revoke_peer_link.
Changes
accept_friend_requestto create contact row, mint peer token, and deliver handshake (fire-and-forget viaasyncio.create_task). Extendblock_peerto cascade tocontacts_store.revoke_peer_linkvia fingerprint→username lookup through hub_authors.send_handshake()anddeliver_handshake()for building and delivering handshake envelopes to peer endpoints.test_routes_hub_slice3.py— accept creates contact, accept creates peer_link, accept without contacts_store doesn't crash, block cascades to revoke peer link. All 53 targeted tests pass (hub + contacts + peer).Task
Test results
Design notes
asyncio.create_task) — we don't block the HTTP response waiting for the peeroutbound_tokenstarts empty in the initial peer_link; it's filled when the peer responds to our handshakestore.get_author(fingerprint)to resolve peer fingerprint → hub username → contact_id