Conversation
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFriend acceptance now resolves peer keys, validates directory identity, and creates or refreshes contacts and peer links. Blocking revokes matching local peer links and contacts when available. ContactsStore adds fingerprint persistence, migration, upsert, and lookup support. ChangesCollab A2 peer-link lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)Friend acceptance handshakesequenceDiagram
participant Client
participant accept_friend_request
participant Directory
participant ContactsStore
Client->>accept_friend_request: accept friend request
accept_friend_request->>Directory: resolve peer keys and endpoints
accept_friend_request->>ContactsStore: validate fingerprint and upsert contact
accept_friend_request->>ContactsStore: establish inbound peer link
ContactsStore-->>accept_friend_request: persist handshake state
accept_friend_request-->>Client: accepted
Block cascadesequenceDiagram
participant Client
participant block_peer
participant Directory
participant ContactsStore
Client->>block_peer: block peer
block_peer->>Directory: revoke upstream edge
block_peer->>ContactsStore: resolve local contact
block_peer->>ContactsStore: revoke peer link and mark contact blocked
block_peer-->>Client: blocked
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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? |
1 similar 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}
| ) | ||
|
|
||
| # Mint the inbound token WE give to the remote instance. | ||
| inbound_token = generate_peer_token() |
There was a problem hiding this comment.
WARNING: Inbound token is minted and stored (hashed) but never delivered to the remote peer
inbound_token = generate_peer_token() is created and persisted via establish_peer_link, but the plaintext is discarded immediately and _try_handshake never returns it or forwards it to the peer (the resp dict passed to _try_handshake is never mutated). The remote instance therefore never learns the token it must present on POST /api/peer/*, so the peer link is non-functional for inbound authentication until a future A3 step. Given this is a security-sensitive auth channel, the handshake should either (a) return the token to the caller so it can be exchanged, or (b) clearly document that A2 intentionally leaves the channel inert and that no peer can authenticate inbound yet. As written, any code path that trusts inbound_token_hash for auth (e.g. find_contact_by_inbound_token) will never match a real remote request.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try: | ||
| author = await store.get_author(peer) | ||
| if author and author.get("username"): | ||
| await contacts_store.revoke_peer_link(f"hub:{author['username']}") |
There was a problem hiding this comment.
WARNING: Block cascade silently no-ops when the author is absent from hub_authors
The cascade resolves peer → username via store.get_author(peer) (the hub_authors cache). If the contact was created purely from directory pubkeys and the author row was never cached (or was pruned), author is None and the if author and author.get("username") guard skips the revoke entirely. The result: an active peer link remains that can still authenticate on the peer channel after the user blocked the peer. Since block_peer is a strong local-first safety operation, an unrevoked active link is a security gap. Consider falling back to contacts_store.get_contact_by_username/iterating contacts keyed by fingerprint, or at minimum logging a warning when no author is found so the silent skip is observable.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if contacts_store is None: | ||
| return | ||
|
|
||
| username = directory_resp.get("username") or directory_resp.get("target") or "" |
There was a problem hiding this comment.
SUGGESTION: contact_id is derived from the untrusted directory username
username = directory_resp.get("username") or directory_resp.get("target") comes straight from the remote directory response, then contact_id = f"hub:{username}" is used as the TOFU key-pinning primary key. A peer controls its own username, so a single fingerprint could be pinned under different contact_id values if the name changes between the request and accept flows, fragmenting or shadowing contacts. Binding the contact to the verified peer_fingerprint (e.g. hub:{peer_fingerprint}) and storing username only as a display/lookup column would be more robust against name churn and confusion. Not blocking given the design, but worth considering before this becomes load-bearing.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| inbound_contact = await store.find_contact_by_inbound_token( | ||
| # Generate a new token and use its hash — we can't read the stored plaintext | ||
| # but we can verify the hash is deterministic. | ||
| "placeholder-not-testable-directly" |
There was a problem hiding this comment.
SUGGESTION: Dead code / unused variable left in the test
inbound_contact = await store.find_contact_by_inbound_token(
"placeholder-not-testable-directly"
)
# Actually, we should test the token flow differently.
# Let's just verify the link exists and the hash is consistent.inbound_contact is assigned but never asserted/used, and the "placeholder-not-testable-directly" input is just an arbitrary string that always returns None. This reads like an abandoned attempt to test the token flow. Either remove it, or replace it with a real assertion — e.g. mint a token, call establish_peer_link with it, then assert find_contact_by_inbound_token(token) returns the contact, proving the hash lookup actually works.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous Review Summaries (9 snapshots, latest commit 9adf6ce)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9adf6ce)Status: No Issues Found | Recommendation: Merge Files Reviewed (0 files)No files changed since previous review. Previous review (commit 7553810)Status: No Issues Found | Recommendation: Merge Files Reviewed (0 files)No files changed since previous review. Previous review (commit 8857206)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 760deb5)Status: 9 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 5281509)Status: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 948dbad)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Resolved since previous review (incremental PATCH 6, commit 948dbad)
Carried-forward open issues (unchanged by this incremental diff)
Incremental review scopeOnly PATCH 6 (commit 948dbad, since 4b5903b) was within scope: Previous review (commit 4b5903b)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Resolved since previous review (commit a0a07fd+)
Carried-forward open issues (unchanged by this incremental diff)
No NEW issues were introduced by the incremental changes — the added fingerprint guard and Previous review (commit 0254a6e)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Resolved since previous review (commit a0a07fd+)
Carried-forward open issues (unchanged by this incremental diff)
No NEW issues were introduced by the incremental changes — the added fingerprint guard and Previous review (commit 8feef8d)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by step-3.7-flash · Input: 236.1K · Output: 35.7K · Cached: 2.3M |
df2155e to
fb637cf
Compare
| @@ -0,0 +1 @@ | |||
| {"signing_private": "9af62cb1e50222bd57e894e3dab5f9444851f0309224c0b37bf45ed21143d178", "signing_public": "9ae572677819955cf2aace721129e8a1d393d21ecc47ef964db8c3f0ff3b98da", "encryption_private": "c02cc57f5a4940700d9a80dd4578467e63308a251bf643f26c13c3f049670662", "encryption_public": "a1ac687647b24884912035b4c67ae749f136477a1bb031d24817a66236717005", "created_at": 1784503810.4560094} No newline at end of file | |||
There was a problem hiding this comment.
CRITICAL: Committed private signing + encryption keys in plaintext
This file contains real private key material (signing_private, encryption_private) and is tracked by git (not in .gitignore). Private keys must NEVER be committed — once pushed they persist in history and are compromised. Rotate these keys immediately and remove the file from the branch and history (e.g. git filter-repo / BFG). The identity is generated at runtime (0600) by tinyagentos/hub/identity.py; do not seed it via a committed file.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Can't form a contact_id without a username. | ||
| return | ||
|
|
||
| contact_id = f"hub:{username}" |
There was a problem hiding this comment.
WARNING: contact_id derived from untrusted directory username enables TOFU key-pinning confusion
contact_id = f"hub:{username}" uses username taken from the directory response (directory_resp.get("username") or ...get("target")), which is attacker-influenceable. A malicious or compromised directory can pin the peer's public keys under an arbitrary/colliding hub:<name>, colliding with an existing local contact or impersonating a different user. The trust-on-first-use pin should be keyed by the verified peer fingerprint (hub:{peer_fingerprint}), which is already available, not a directory-supplied display name. This also roots the dead block-fallback below (see line 509).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Fallback: scan contacts for a matching ed25519 fingerprint. | ||
| all_contacts = await contacts_store.list_contacts() | ||
| for contact in all_contacts: | ||
| if contact.get("ed25519_pub") == peer: |
There was a problem hiding this comment.
SUGGESTION: Block fallback compares pubkey to fingerprint — effectively dead code
The fallback scans contacts and matches contact.get("ed25519_pub") == peer, but peer is the signing fingerprint (a hash, e.g. deadbeef…) whereas ed25519_pub is the raw public key (ab…). The contacts table has no fingerprint column (see contacts_store.py:14). These encodings never match, so this branch always falls through to the "could not resolve" warning — the exact no-op the PATCH 2/2 fallback was meant to fix. Store the peer fingerprint on the contact (e.g. a peer_fingerprint column) and match on that, or resolve via hub_authors before scanning.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # outbound token) completes the two-way exchange. Until then, the | ||
| # inbound auth channel is inert (no remote request will carry this | ||
| # token) and find_contact_by_inbound_token() will never match. | ||
| inbound_token = generate_peer_token() |
There was a problem hiding this comment.
SUGGESTION: Peer link created in a non-functional/inert state
The inbound token is minted and stored but never delivered to the remote peer (no exchange channel yet), and outbound_token is an empty placeholder — so find_contact_by_inbound_token() can never match and the link cannot authenticate inbound requests until A3. The row existing in active/established state may be mistaken for a ready channel by future code (or monitoring). Consider tracking an explicit handshake state (e.g. pending/awaiting_reply) so the inert row is distinguishable from a live peer link.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![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 `@data/hub/identity.json`:
- Line 1: Remove the committed runtime identity artifact at
data/hub/identity.json, add data/hub/*.json (or the equivalent specific ignore
rule) to .gitignore, and treat the exposed signing and encryption keys as
compromised by rotating them if they were used against a real hub.
In `@tests/test_collab_a2_handshake.py`:
- Around line 225-235: Close each manually created HubStore after its
upsert_author call completes to prevent leaked database connections: add the
cleanup at tests/test_collab_a2_handshake.py lines 225-235 and 398-408,
preferably using try/finally so HubStore.close runs even if upsert_author fails.
In `@tinyagentos/routes/hub.py`:
- Around line 499-520: The fallback scan in the hub block flow should compare
the peer signing-key fingerprint with a fingerprint derived from each contact’s
ed25519_pub, not the raw public-key value. Update the loop around
contacts_store.list_contacts and revoke_peer_link to use the existing
fingerprint derivation utility, preserving the matching contact_id revocation
behavior, and add coverage for the missing or stale hub_authors fallback path.
- Around line 119-142: Before saving contact keys in _try_handshake, compute the
fingerprint of ed25519_pub with identity.fingerprint and compare it to
peer_fingerprint. If they differ, log a warning identifying the contact and skip
the handshake; only continue to add the contact when the fingerprints match.
🪄 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: d4d3c999-8a91-4629-b44b-730514716980
📒 Files selected for processing (3)
data/hub/identity.jsontests/test_collab_a2_handshake.pytinyagentos/routes/hub.py
|
Good use of the supersede convention on #2046, but the survivor is missing half the slice. #2046 carried tinyagentos/peer.py (the send_handshake sender side); this PR only has the hub.py receive side plus tests. Before this merges as THE A2 slice:
CI is green and the PR is mergeable, but I am holding until 1 and 3 are answered and the applicable parts of 2 are folded or explicitly relocated. |
e1076db to
0254a6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tinyagentos/routes/hub.py (1)
136-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: local variable
hub_storeshadows the module-levelhub_storeimport.
hub_store = await _get_store(request)rebinds the name also used as a module reference elsewhere in this file (e.g. line 93'shub_store.HubStore(...)). Not a bug today (not reused after), but risks confusion if the function grows. Consider naming itstorefor consistency withblock_peer's usage.🤖 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/hub.py` around lines 136 - 139, The fallback branch in the relevant route shadows the module-level hub_store import. Rename the local result of _get_store(request) to store and update the subsequent get_author call, matching block_peer’s naming without changing behavior.tests/test_collab_a2_handshake.py (1)
307-346: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRevocation-clearing assertion doesn't test revocation-clearing.
The comment claims
revoked_at is Noneproves "re-establish clears revocation" (Line 346), but the link is never revoked between the two accept calls —revoked_atwas alreadyNonebeforehand, so this passes regardless of whether re-establish actually clears revocation.🧪 Suggested fix to actually exercise the clearing behavior
store = app_with_contacts.state.contacts_store first_link = await store.get_peer_link(f"hub:{_PEER_USERNAME}") first_established = first_link["established_at"] + # Simulate a prior revocation to verify re-establish actually clears it. + await store.revoke_peer_link(f"hub:{_PEER_USERNAME}") + revoked_link = await store.get_peer_link(f"hub:{_PEER_USERNAME}") + assert revoked_link["revoked_at"] is not None + # Second accept with different endpoints — should update dir_resp_body["endpoints"] = ["https://second.example.com:6969"](Adjust the revoke call to whatever method
contacts_storeactually exposes.)🤖 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_collab_a2_handshake.py` around lines 307 - 346, Update test_accept_reupsert_contact to revoke the stored peer link after the first accept and before the second accept, using the contacts_store revocation method exposed by the implementation. Assert the link is revoked before re-accepting, then retain the existing second-accept assertions to verify re-establishment clears revoked_at and refreshes the endpoints.
🤖 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_collab_a2_handshake.py`:
- Around line 381-458: Add a test alongside
test_block_cascades_to_contacts_store that creates the contact and peer link but
omits the HubStore.upsert_author setup, then posts to /api/hub/friends/block
using the peer fingerprint. Assert the request succeeds with blocked state and
verify get_peer_link reports revoked_at and get_contact reports status
"revoked", covering the handler’s fallback username resolution.
In `@tinyagentos/routes/hub.py`:
- Around line 511-533: Update the block cascade around contacts_store and
revoke_peer_link to resolve the blocked peer from a signing-fingerprint value
persisted on the contacts row, rather than relying solely on store.get_author
and hub_authors. Add the required contacts-store schema, persistence, and lookup
support, revoke the matching peer link even when no hub_authors entry exists,
and add coverage for blocking a peer that was never upserted as a contact.
- Around line 133-160: Widen the exception boundary in _try_handshake to cover
the hub_authors lookup, public-key validation, and identity.fingerprint check,
not only the handshake side effect. Ensure malformed directory keys or database
lookup failures are caught and logged, then return without propagating an
exception so accept_friend_request preserves its successful-response guarantee.
---
Nitpick comments:
In `@tests/test_collab_a2_handshake.py`:
- Around line 307-346: Update test_accept_reupsert_contact to revoke the stored
peer link after the first accept and before the second accept, using the
contacts_store revocation method exposed by the implementation. Assert the link
is revoked before re-accepting, then retain the existing second-accept
assertions to verify re-establishment clears revoked_at and refreshes the
endpoints.
In `@tinyagentos/routes/hub.py`:
- Around line 136-139: The fallback branch in the relevant route shadows the
module-level hub_store import. Rename the local result of _get_store(request) to
store and update the subsequent get_author call, matching block_peer’s naming
without changing 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: 0d2de826-ccbe-4cfd-8abd-365ad5ef0d0c
📒 Files selected for processing (3)
.gitignoretests/test_collab_a2_handshake.pytinyagentos/routes/hub.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
|
Deep review at head 4b5903b. Good movement: the fingerprint-fallback block cascade, widened exception boundary, endpoint normalization, and the 9 new tests are all solid, and most earlier bot findings are genuinely fixed at head. But this is a HOLD on one reproduced upgrade-breaking defect plus two unanswered folds:
Item 1 is the gate. Everything else can land in the same push. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata and changed files =='
gh pr view 2043 --repo jaylfc/taOS --json number,headRefOid,body,commits,files \
--jq '{number, headRefOid, body, commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline}], files: [.files[].path]}'
echo
echo '== Relevant file locations =='
fd -a -t f '^(contacts_store|db_migrations|knowledge_store|peer|hub)\.py$|^cross-user-collaboration\.md$|^\.gitignore$|^test.*collab.*|^test.*contact.*' . \
| sed -n '1,160p'
echo
echo '== Structural outlines =='
for f in $(fd -a -t f '^(contacts_store|db_migrations|knowledge_store|peer|hub)\.py$' .); do
echo "--- $f"
ast-grep outline "$f" --items all --view compact || true
done
echo
echo '== Migration, handshake, and contact references =='
rg -n -C 4 \
-e 'MIGRATIONS' \
-e '_post_init' \
-e 'migration_v1_add_user_id' \
-e 'peer_fingerprint' \
-e 'get_contact_by_fingerprint' \
-e 'send_handshake' \
-e 'resolve_local_identity_id' \
-e 'inbound_token' \
-e 'data/hub' \
--glob '*.py' --glob '*.md' --glob '.gitignore' . \
| sed -n '1,500p'
echo
echo '== Relevant commit file lists =='
for sha in 2b28043a 500da607 4b5903b3; do
echo "--- $sha"
git show --no-ext-diff --format='%H %s' --name-status "$sha" 2>&1 | sed -n '1,100p' || true
doneLength of output: 42728 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="jaylfc/taOS"
echo '== ContactsStore schema, migration, and lifecycle =='
contacts=$(fd -a -t f '^contacts_store\.py$' . | head -n1)
dbm=$(fd -a -t f '^db_migrations\.py$' . | head -n1)
ks=$(fd -a -t f '^knowledge_store\.py$' . | head -n1)
tests=$(fd -a -t f '^test_collab_a2_handshake\.py$' . | head -n1)
peer=$(fd -a -t f '^peer\.py$' . | rg '/tinyagentos/peer\.py$' | head -n1)
sed -n '1,260p' "$contacts"
echo '--- db_migrations ---'
sed -n '1,240p' "$dbm"
echo '--- analogous guarded migration ---'
sed -n '90,145p' "$ks"
echo '== A2 tests relevant to legacy upgrades and block cascade =='
rg -n -C 5 \
-e 'upgrade|existing|legacy|peer_fingerprint|never.*upsert|block_cascade' \
"$tests" tests \
| sed -n '1,360p'
echo '== Sender implementation definitions/call sites =='
rg -n -P -C 3 '\b(send_handshake|build_envelope|resolve_local_identity_id)\s*\(' \
"$peer" tinyagentos tests \
| sed -n '1,360p'
echo '== Design specification: A2/A3 handshake statements =='
design=$(fd -a -t f '^cross-user-collaboration\.md$' . | head -n1)
rg -n -i -C 4 'day 0|A2|A3|handshake|token' "$design" | sed -n '1,360p'
echo '== Existing GitHub issues/PRs mentioning sender-side handshake or fingerprint-keyed contacts =='
gh search issues \
--repo "$repo" --state open \
--match title,body \
--limit 100 \
--json number,title,url,state \
'send_handshake OR "fingerprint-keyed" OR "contact_id" OR "peer handshake"' \
--jq '.[] | {number,title,url,state}' || trueLength of output: 76029
Required follow-up before re-review
The remaining holds also remain valid:
After the migration fix and regression test are pushed, re-trigger 🐇🔍 ✏️ Learnings added
|
|
Thanks for the thorough deep review. All six findings folded at 948dbad: 1. BLOCKER — fixed. Replaced the 2. Fold 1 (send_handshake) — deferred. A2 intentionally stores the inbound token locally without delivering it (the token exchange channel doesn't exist yet). The 3. Fold 3 (.gitignore) — answered. The 4. Key hygiene — confirmed. The keys at 2b28043 were throwaway test keys never used against real endpoints. Squash merge removes them from dev history. Re #2042: coordination note added — when #2042 lands on dev, the 5. CodeRabbit — re-triggered. @coderabbitai review 6. Track-don't-block (Kilo W1) — accepted. The documented NOTE about BONUS: re-upsert test now actually revokes between accepts so the |
|
I’ll review the current head to verify the implementation and regression coverage. 🐇✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
|
Held after a deep review. No auth-bypass or data-integrity blocker under the single-user self-hosted model, and the schema migration is done RIGHT (guarded MEDIUM - re-accept resurrects a blocked contact (verified). The upsert nulls MEDIUM - the anti-imposter control is untested. The fingerprint-mismatch skip branch (routes/hub.py:784-791) is the PR's key security control and has zero coverage; a future refactor could silently disable it. Add a test where the directory returns a signing_pubkey whose fingerprint != peer_fingerprint, asserting no contact/peer-link is created. MEDIUM - authz-rejection is untested (and was explicitly requested). Add a test where the upstream accept returns 403/404, asserting LOW: client body outranks the directory-verified peer ( The authz delegation to the directory and the idempotency (ON CONFLICT upsert) are both correct. Add the block-state check and the two security tests, then it is good to go. |
948dbad to
ae6e75a
Compare
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 jaylfc#2012 (cross-user collaboration), milestone A2. Closes jaylfc#2014.
…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)
…allback, 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)
…erification - 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)
…ade 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.
…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).
ae6e75a to
5281509
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_collab_a2_handshake.py (1)
122-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: fixture reaches into
store._dbprivate attribute.Reading
store._dbto decide whether to close-and-reinit couples the test toContactsStore's internal implementation. If a publicis_initialized/initializedproperty (or simply always callingclose()defensively) exists or can be added, prefer that instead.🤖 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_collab_a2_handshake.py` around lines 122 - 125, Update the fixture setup around ContactsStore.close and init to avoid reading the private store._db attribute. Prefer an existing public initialization-state property, or add and use one; otherwise make close() safely idempotent and always call it before store.init().
🤖 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_collab_a2_handshake.py`:
- Around line 159-378: The TestFriendAcceptHandshake suite lacks coverage for
four acceptance safeguards. Add tests covering re-accepting a locally blocked
contact without changing its blocked status, rejecting mismatched directory
signing-key fingerprints without creating a contact or peer link, skipping the
handshake and persistence on directory 403/404 responses, and ensuring the
directory-verified fingerprint overrides a conflicting client-supplied
peer_fingerprint; extend test_accept_reupsert_contact only for revocation
behavior and keep these cases independently asserted.
In `@tinyagentos/contacts_store.py`:
- Around line 134-141: Prevent the contact upsert and accept-handshake flow in
contacts_store from reactivating a locally blocked peer: check the persisted
block marker before updating status or clearing revoked_at, and preserve the
blocked state unless an explicit unblock flow has run. Ensure acceptance does
not re-establish the peer link for blocked contacts, and add a regression test
covering block, re-accept, and continued blocked status.
---
Nitpick comments:
In `@tests/test_collab_a2_handshake.py`:
- Around line 122-125: Update the fixture setup around ContactsStore.close and
init to avoid reading the private store._db attribute. Prefer an existing public
initialization-state property, or add and use one; otherwise make close() safely
idempotent and always call it before store.init().
🪄 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: 07ca8319-ec3c-4436-a69f-9af393eee645
📒 Files selected for processing (5)
.gitignoretests/test_collab_a2_handshake.pytests/test_store_upgrades.pytinyagentos/contacts_store.pytinyagentos/routes/hub.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- tinyagentos/routes/hub.py
| ON CONFLICT(contact_id) DO UPDATE SET | ||
| ed25519_pub = excluded.ed25519_pub, | ||
| x25519_pub = excluded.x25519_pub, | ||
| peer_fingerprint = excluded.peer_fingerprint, | ||
| display_name = excluded.display_name, | ||
| status = excluded.status, | ||
| local_crm_id = excluded.local_crm_id, | ||
| revoked_at = NULL""", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not reactivate locally blocked contacts on re-accept.
This upsert always writes status = "active" and clears revoked_at. The accept handshake then unconditionally re-establishes the peer link, clearing its revocation too. A locally blocked peer can therefore be resurrected unless acceptance checks the block marker before either operation. Only an explicit unblock flow should permit this; add a blocked-then-accept regression test.
🤖 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/contacts_store.py` around lines 134 - 141, Prevent the contact
upsert and accept-handshake flow in contacts_store from reactivating a locally
blocked peer: check the persisted block marker before updating status or
clearing revoked_at, and preserve the blocked state unless an explicit unblock
flow has run. Ensure acceptance does not re-establish the peer link for blocked
contacts, and add a regression test covering block, re-accept, and continued
blocked status.
| return None | ||
| async with self._db.execute( | ||
| "SELECT * FROM contacts WHERE peer_fingerprint = ?", | ||
| (peer_fingerprint,), |
There was a problem hiding this comment.
WARNING: get_contact_by_fingerprint returns only the first row, leaving other same-fingerprint contacts active
SELECT * FROM contacts WHERE peer_fingerprint = ? returns all matches but the function returns only rows[0]. If a peer changes username and is re-accepted, two contacts share the same peer_fingerprint. The block cascade revokes only the first, leaving the other active with a live peer link.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "friend-accept handshake: contact=%s endpoints=%s", | ||
| contact_id, endpoints, | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
WARNING: _try_handshake partial failure leaves orphaned contact
If add_contact commits but establish_peer_link raises, the except Exception catches it without rolling back the already-committed contact row. The accept succeeds with an active contact that has no peer link; the next re-accept is required to heal it.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| await self._db.execute("PRAGMA table_info(contacts)") | ||
| ).fetchall() | ||
| } | ||
| if "peer_fingerprint" not in existing_cols: |
There was a problem hiding this comment.
SUGGESTION: Missing index on peer_fingerprint
Block cascade queries contacts by peer_fingerprint via get_contact_by_fingerprint, but the column has no index, so every block does a full table scan. Add CREATE INDEX idx_contacts_peer_fingerprint ON contacts(peer_fingerprint) alongside the ALTER TABLE in _post_init.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…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.
|
All contributors have signed the CLA ✍️ ✅ |
e1e4746 to
c159a7f
Compare
…r_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.
…r_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.
c159a7f to
08eebd2
Compare
Docs-Reviewed: retrigger CI after author identity fix; no API surface changes
e131983 to
08c6af8
Compare
|
All three issues from the HOLD review were already addressed on the fork branch: migration uses _post_init with PRAGMA check (commit 2edecb7), send_handshake/deliver_handshake folded in from #2046 (commit c746748), .gitignore line justified (hub stores identity/DB under data/hub/). Re-requesting review. |
Blocking: the TOFU pin is keyed on a peer-controlled username, and this module's own design says it must not beReviewed at head against current dev. The handshake structure is good — the fingerprint check before pinning ( contact_id = f"hub:{username}" # username comes from the directory response
The TOFU pin is the one place that principle matters most, and it is the one place the username is load-bearing. Consequences, all reachable from that one line:
The demonstrable failure: a block that reports success and revokes nothingauthor = await store.get_author(peer)
cid = None
if author and author.get("username"):
cid = f"hub:{author['username']}"
await contacts_store.revoke_peer_link(cid)
else:
contact = await contacts_store.get_contact_by_fingerprint(peer)
...The fallback only fires when the author row is missing. It does not fire when the row is present and stale — which is the case the comment three lines above anticipates ("independent of the volatile hub_authors cache"). If the peer's cached username no longer matches the async def revoke_peer_link(self, contact_id: str) -> None:
await self._db.execute("UPDATE peer_links SET revoked_at = ? WHERE contact_id = ?", ...)
await self._db.execute("UPDATE contacts SET status = 'revoked', ... WHERE contact_id = ?", ...)
await self._db.commit()Two UPDATEs matching zero rows, a successful commit, no return value, and no rowcount check at the call site. To be exact about severity, because it changes the priority rather than the verdict: this is currently latent, not live. As your own comment says, A2 mints the inbound token but never delivers it, so This section is read from source, not executed — I did not stand up the hub + contacts fixtures to run it. The path is small enough that I am confident, but the red evidence below is what settles it. The demandKey the contact on the verified fingerprint, not the supplied username. With it, two supporting changes:
Red evidence required: a test where Order of operations: rebase first, then CI
Already resolved — no action neededFor the record, so you don't re-litigate them from the bot threads: kilo's CRITICAL committed private keys ( |
…me (jaylfc#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.
|
Keyed the TOFU contact pin on the signing-key fingerprint ( |
…ylfc#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).
|
Addressed the active blocker (TOFU pin keyed on peer-controlled username) plus the two supporting changes from the review. Force-pushed to Fingerprint keying (already in
The two supporting changes from your demand list, now in:
Tests:
Re: |
|
@coderabbitai review |
|
|
Re-review done — this is solid work. Every demanded change is real at head
Before merge: (1) real CodeRabbit review on this head — re-triggered, the current "pass" is the paused fake-green; (2) drop the One follow-up to file rather than fix here: |
|
@hognek — see #2070 (comment) for the full note and the apology it owes you. Short version for this PR: It has been clean, green and mergeable with no human review for five weeks, and that is on us, not on the work. Your 18 Aug commit (revoke-match reporting, revoke all fingerprints) has never been looked at by a person. A fresh CodeRabbit pass is queued at the current head, since the earlier bot review predates that push. After that I am reading the revoke and fingerprint logic myself rather than merging on green. Revocation is one of those paths where "the tests pass" and "it actually revokes" are not the same claim. You will hear back on this PR specifically, not as part of a batch. |
|
@hognek — security read on this PR as promised, at head What I verified as SOUND — please don't re-do this work
1. BLOCKING — the block cascade fails open on every upgraded install
The comment at The fix is cheap and the data is already in the row: in A red for this must be built on a legacy row — seed a v0 contact + its 2. The pinned encryption key is not covered by the fingerprint check
Calibrated: latent, not exploitable today. 3. The table rebuild is only ever exercised on an EMPTY database
Suggest: seed two contacts (deliberately sharing a Related hygiene: the rebuild leaves 4. Minor:
|
|
@hognek — #2070 is merged ( Blocking, and it fails open. Your plural-lookup fix does not reach these rows — they do not share a fingerprint, they have none at all. The fix is cheap: backfill Note the test shape too, because it is why this is invisible: Two non-blocking notes, for your judgement, not merge conditions:
No rush and no deadline from me. The 39-day delay on these was ours, not yours — see the note on #2070 for what actually caused it. |
|
Backfill fix landed in #2561 — backfills peer_fingerprint from ed25519_pub for all pre-existing rows in _post_init (both the ALTER path and the UNIQUE-index rebuild path). Also added the x25519 and deliver_handshake comments flagged above. |
|
Agreed — closing as superseded by #2561. I verified the supersession rather than taking it on trust: My blocking finding on this PR is closed. The backfill now runs after both the ALTER path and the UNIQUE-index rebuild, and Review is already posted on #2561 (5445279029) — it went up shortly before your message here, so you may not have seen it. Short version: the fix is correct and merges once two things are done, neither of them your code's fault:
The Thanks for folding the malformed- |
…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>
2 participants
On friend-accept, create the contact row (pin Ed25519/X25519 pubkeys), mint the inbound peer token, exchange endpoints, and record the peer_link. Subscribe to friend block/revoke → cascade.
Changes
tinyagentos/routes/hub.py: Added_try_handshake()helper that extracts pubkeys from the directory response (with hub_authors fallback), creates a contact row, mints an inbound token, and establishes a peer link. Called fromaccept_friend_requestafter the hub relationship is recorded.block_peernow cascades tocontacts_store.revoke_peer_link().tests/test_collab_a2_handshake.py: 8 new integration tests covering the full accept→contact+link flow and block→revoke cascade.Design decisions
hub_authorstable (populated during friend-request flow)hub_authorscacheTests
test_contacts_peer.pytests pass (no regressions)Part of #2012 (cross-user collaboration), milestone A2.
Part of #2014 (re-scoped: A2 ships mint-without-delivery; requester side lands in A2a).
Summary by CodeRabbit
peer_fingerprintsupport to re-resolve and refresh peer contacts.