fix(node): carry full owner DID on ref-update wire event (#144)#145
fix(node): carry full owner DID on ref-update wire event (#144)#145Gravirei wants to merge 14 commits into
Conversation
github-actions
Bot
added
the
needs-tests
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
|
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:
📝 WalkthroughWalkthroughAdds an optional ChangesOwner DID propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
beardthelion
added
crate:node
github-actions
Bot
removed
the
needs-tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/gitlawb-node/src/test_support.rs (1)
1969-2043: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared builder for
ReceivedRefUpdatetest fixtures.Both tests construct near-identical
ReceivedRefUpdateliterals inline. A shared helper (similar to theupdate(...)fixture already used in db/mod.rs tests) would reduce duplication and make future field additions (likeowner_did) less error-prone to keep in sync across test files.🤖 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 `@crates/gitlawb-node/src/test_support.rs` around lines 1969 - 2043, The `events_returns_inserted_ref_updates` and `events_limit_respects_limit_param` tests duplicate the same `crate::db::ReceivedRefUpdate` setup inline, so add a shared test fixture/builder for `ReceivedRefUpdate` in `test_support.rs` and use it in both cases. Model it after the existing `update(...)` helper used in the db tests, and make sure the helper accepts overrides for fields like `repo`, `owner_did`, `new_sha`, and timestamps so future schema changes stay consistent across tests.crates/gitlawb-node/src/db/mod.rs (1)
3919-4077: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
owner_didin the filtered-by-repo test too.
list_ref_updates_filtered_by_repo(lines 4027-4058) inserts records with distinctowner_didvalues but only assertsid/count, notowner_did, unlike the siblinglist_repo_ref_updates_filters_by_repotest which does check it. Adding that assertion would close a small gap in coverage for the exact field this PR introduces.♻️ Proposed test strengthening
let filtered = db .list_ref_updates_filtered(Some("ownerA/proj"), 100) .await .unwrap(); assert_eq!(filtered.len(), 1); assert_eq!(filtered[0].id, "u5"); + assert_eq!(filtered[0].owner_did.as_deref(), Some("did:key:zA"));🤖 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 `@crates/gitlawb-node/src/db/mod.rs` around lines 3919 - 4077, The list_ref_updates_filtered_by_repo test is missing coverage for the new owner_did behavior. Update ref_update_db_tests::list_ref_updates_filtered_by_repo to assert the returned row’s owner_did matches the inserted value, similar to list_repo_ref_updates_filters_by_repo, so the filtered path verifies both repo filtering and owner_did preservation.
🤖 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 `@crates/gitlawb-node/src/test_support.rs`:
- Around line 1969-2007: The ref-update events payload returned by the events
API is still missing owner_did, so the current test only verifies persistence in
Postgres. Update the ref-update feed serialization in the events response path
to include owner_did, using the relevant event model/handler that backs
events_router and GET /api/v1/events/ref-updates, and then extend
events_returns_inserted_ref_updates to assert events[0]["owner_did"] matches the
inserted value.
---
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 3919-4077: The list_ref_updates_filtered_by_repo test is missing
coverage for the new owner_did behavior. Update
ref_update_db_tests::list_ref_updates_filtered_by_repo to assert the returned
row’s owner_did matches the inserted value, similar to
list_repo_ref_updates_filters_by_repo, so the filtered path verifies both repo
filtering and owner_did preservation.
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 1969-2043: The `events_returns_inserted_ref_updates` and
`events_limit_respects_limit_param` tests duplicate the same
`crate::db::ReceivedRefUpdate` setup inline, so add a shared test
fixture/builder for `ReceivedRefUpdate` in `test_support.rs` and use it in both
cases. Model it after the existing `update(...)` helper used in the db tests,
and make sure the helper accepts overrides for fields like `repo`, `owner_did`,
`new_sha`, and timestamps so future schema changes stay consistent across tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d0fa77a-014d-4a04-af46-9ef99e20fe27
📒 Files selected for processing (5)
crates/gitlawb-node/src/api/peers.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/p2p/mod.rscrates/gitlawb-node/src/test_support.rs
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The write-side plumbing is right: the full DID comes from the local repo record, it's threaded through both the gossip and HTTP-notify paths, and the #[serde(default)] / nullable column keeps older peers compatible. CodeRabbit's feed-serialization ask is genuinely addressed too. Two things block merge, and the first is a hard one: the schema change breaks every existing node on upgrade, and the PR closes #144 without fixing #144's symptom.
Findings
-
[P1] Ship
owner_didas a new migration version, not appended to v1
crates/gitlawb-node/src/db/mod.rs:542
TheALTER TABLE ... ADD COLUMN owner_didandidx_ref_updates_ownerwere added insideMigration { version: 1 }, butrun_migrationsskips any version already recorded inschema_migrations, and the catalogue comment right above the array says new schema must be a new version (v2, v3, ...). Every existing node has v1 recorded, so on upgrade the column is never created;insert_ref_update(binds$12) and all threelist_*SELECTs then reference a missing column and error, which stops ref-update ingestion on both the gossip and notify paths and 500s the events feed. Fresh databases pass because they run v1 from scratch, which is why CI is green. I reproduced it against the real migration runner: an existing node (v1..v9 recorded, noowner_did) upgraded to this build leaves the column absent and the insert/select fail; moving both statements into a newMigration { version: 10 }makes the same upgrade round-trip cleanly. -
[P2] Don't close #144 on this PR; nothing consumes
owner_didyet
crates/gitlawb-node/src/api/events.rs:38
#144 has two halves: carry the full owner DID (done here) and make the feed gate use it to stop the over-drop. Nothing in this diff readsowner_didfor a visibility decision; it's stored and echoed in the feed JSON only, andidx_ref_updates_owneris an index no query uses. The gate that over-drops isn't in this branch, so merging this auto-closes #144 while the reported symptom persists. Keep the plumbing, but change "Closes #144" to "Refs #144" and land the gate consumption (the did:key-aware match #144 describes) in the follow-up that sits on top of the gate. -
[P3] Populate
owner_didon the other read surfaces, or hold the JSON change
crates/gitlawb-node/src/graphql/query.rs:61,crates/gitlawb-node/src/api/events.rs:117
The field lands on the global REST feed but the GraphQLref_updatesresolver drops it (RefUpdateTypehas no such field) and the repo-scopedlist_repo_eventsgossip block omits it. No consumer reads it yet so there's no impact today, but the asymmetry is the same drift the duplicated four-copy SELECT column list invites, and it becomes a trap once something depends on the field. Either populate all three surfaces or defer the JSON exposure until the gate needs it. While here, add a migration upgrade-path test (seedschema_migrationsat v1, run migrations, assert the column round-trips) so the v1-append class can't recur.
The direction is right and the receive/publish wiring is sound; once the migration moves to its own version this is close.
P1 migration break resolved on 72ccb28; re-reviewed the new head, posting a fresh review.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on 72ccb28d. The P1 migration break is resolved and I verified it by execution: the owner_did DDL now lives in a new Migration{version:10}, and on a simulated existing node (schema_migrations at v1..v9, received_ref_updates without the column) migrate() adds the column and index and records v10. Putting the DDL back inside the already-applied v1 makes that same check fail (the column is never re-added), so the guard is load-bearing. Wire compat (#[serde(default)] Option<String>) and the DB plumbing are sound, and "Refs #144" is correct. Four items to fold in.
Findings
-
[P2] Drop the premature
idx_ref_updates_ownerindex, or defer it to the gate PR
crates/gitlawb-node/src/db/mod.rs:833
Nothing readsreceived_ref_updatesbyowner_didyet: every reference is a projection, no WHERE/JOIN/ORDER. Migrations run inside a transaction soCREATE INDEX CONCURRENTLYisn't available here (mod.rs:426), which means this index builds under a write-blocking lock at startup on exactly the populated nodes this PR targets, for no current benefit. Ship the column now and add the index in #143 next to the query that reads it. -
[P2] Expose owner_did on the repo-scoped events feed
crates/gitlawb-node/src/api/events.rs:116
list_ref_updatesreturnsowner_didnow, but thegossip_eventsblock inlist_repo_eventsomits it, soGET /api/v1/repos/{owner}/{repo}/eventssilently drops the field. Add"owner_did": u.owner_didto thatjson!block to match line 38. -
[P2] Carry owner_did through the GraphQL query and subscription
crates/gitlawb-node/src/graphql/types.rs:52
RefUpdateTypeandRefUpdateBroadcast(state.rs:12) have noowner_did, so theref_updatesquery (query.rs:61) and the subscription drop it while REST exposes it. Addowner_did: Option<String>to both structs and populate it at query.rs, subscription.rs, and the broadcast send in repos.rs. -
[P2] Add an existing-node upgrade test for v10
crates/gitlawb-node/src/db/mod.rs:3200
migration_v10_creates_owner_did_columnruns only a fresh v1..v10 chain, which can't catch DDL appended to an already-applied version. Seedschema_migrationswith v1..v9 and areceived_ref_updateswithout the column, thenmigrate()and assert the column, index, and v10 row appear. I ran exactly this against the head and it passes; the same body fails when the DDL sits back in v1, so it is the missing regression guard, not a live bug.
One forward note for #143, not a blocker here: the wire owner_did is attacker-controlled (the peer signature authenticates the relaying node, not this field) and is stored verbatim. Every authorization path today reads the trusted record.owner_did from the local repo row, so nothing consumes the wire value yet, but the gate must cross-check it against a trusted source and must not fall back to the lossy last-segment / did_matches matcher the full DID exists to avoid.
@Gravirei migration fix verified, thanks. The above are plumbing-consistency and deploy-window items.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/gitlawb-node/src/db/mod.rs (2)
3207-3218: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the column is actually nullable.
The tests fetch
is_nullablebut only assert the name/type. Since older peers can omitowner_did, please assert"YES"so a futureNOT NULLchange is caught.Proposed fix
assert_eq!(col.0, "owner_did"); assert_eq!(col.1, "text"); + assert_eq!(col.2, "YES");Apply the same assertion in both migration tests.
Also applies to: 3274-3284
🤖 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 `@crates/gitlawb-node/src/db/mod.rs` around lines 3207 - 3218, The migration tests for received_ref_updates currently verify owner_did exists and is text, but they ignore the fetched is_nullable value. Update both migration test blocks that query information_schema.columns in db::mod to assert the third tuple field from sqlx::query_as is "YES" alongside the existing name/type checks, so changes to nullable behavior are caught.
2103-2107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve later
owner_didenrichment on duplicate ref-update IDs.With
ON CONFLICT(id) DO NOTHING, a mixed-version network can store the olderNonepayload first and ignore a later duplicate carrying the full owner DID, leaving the feed unable to disambiguate that event.Proposed fix
(id, node_did, pusher_did, repo, ref_name, old_sha, new_sha, timestamp, cert_id, received_at, from_peer, owner_did) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12) - ON CONFLICT(id) DO NOTHING", + ON CONFLICT(id) DO UPDATE + SET owner_did = COALESCE(received_ref_updates.owner_did, EXCLUDED.owner_did) + WHERE received_ref_updates.owner_did IS NULL + AND EXCLUDED.owner_did IS NOT NULL",🤖 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 `@crates/gitlawb-node/src/db/mod.rs` around lines 2103 - 2107, The INSERT into received_ref_updates currently uses ON CONFLICT(id) DO NOTHING, which prevents a later duplicate from enriching an earlier row with owner_did. Update the upsert logic in the received ref-update insert path so duplicates with the same id can fill in a missing owner_did while still avoiding overwriting existing non-null data. Refer to the received_ref_updates insert statement and the surrounding db write function that persists ref updates.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/db/mod.rs (1)
4096-4129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a same-slug owner collision regression.
This test uses different
repovalues, so it does not cover the PR’s core case: two updates with the same trailing repo slug but different fullowner_didvalues. Add a row pair likealice/myrepo+did:web:host:aliceandalice/myrepo+did:gitlawb:alice.🤖 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 `@crates/gitlawb-node/src/db/mod.rs` around lines 4096 - 4129, The current `list_repo_ref_updates_filters_by_repo` test only verifies filtering by full repo name, not the same-slug owner collision case. Update this test in `db` to insert two ref updates with the same trailing repo slug but different full owner identifiers, using the existing `insert_ref_update`, `update`, and `list_repo_ref_updates` helpers, and assert the query returns the correct row for each full repo owner path. Keep the original repo-filter assertions if helpful, but add coverage for the `owner_did` collision scenario the PR is targeting.
🤖 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 `@crates/gitlawb-node/src/api/events.rs`:
- Line 130: The event payload in list_repo_events is only populating owner_did
for gossipsub events, so local cert events in the same feed remain inconsistent.
Update the local event mapping in events.rs to also set owner_did from
record.owner_did, keeping the repo-scoped payload shape consistent with the
existing gossipsub path and preserving consumers that dedup or gate on the full
owner DID.
---
Outside diff comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 3207-3218: The migration tests for received_ref_updates currently
verify owner_did exists and is text, but they ignore the fetched is_nullable
value. Update both migration test blocks that query information_schema.columns
in db::mod to assert the third tuple field from sqlx::query_as is "YES"
alongside the existing name/type checks, so changes to nullable behavior are
caught.
- Around line 2103-2107: The INSERT into received_ref_updates currently uses ON
CONFLICT(id) DO NOTHING, which prevents a later duplicate from enriching an
earlier row with owner_did. Update the upsert logic in the received ref-update
insert path so duplicates with the same id can fill in a missing owner_did while
still avoiding overwriting existing non-null data. Refer to the
received_ref_updates insert statement and the surrounding db write function that
persists ref updates.
---
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 4096-4129: The current `list_repo_ref_updates_filters_by_repo`
test only verifies filtering by full repo name, not the same-slug owner
collision case. Update this test in `db` to insert two ref updates with the same
trailing repo slug but different full owner identifiers, using the existing
`insert_ref_update`, `update`, and `list_repo_ref_updates` helpers, and assert
the query returns the correct row for each full repo owner path. Keep the
original repo-filter assertions if helpful, but add coverage for the `owner_did`
collision scenario the PR is targeting.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f70fb07-7129-4c96-9e77-7b2943106a56
📒 Files selected for processing (7)
crates/gitlawb-node/src/api/events.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/graphql/query.rscrates/gitlawb-node/src/graphql/subscription.rscrates/gitlawb-node/src/graphql/types.rscrates/gitlawb-node/src/state.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gitlawb-node/src/api/repos.rs
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on 63ab6015. All four items from my last pass are in, CodeRabbit's local-cert-events fix landed, and I verified the security- and deploy-relevant paths by execution:
-
Migration, both ways: on a simulated existing node (v1..v9 recorded, no
owner_didcolumn),migrate()adds the column and records v10; moving that DDL back into the already-applied v1 makesmigration_v10_existing_node_upgradefail (RowNotFound) while the fresh-DB test still passes, so the upgrade guard is load-bearing. A pre-upgrade row (owner_didNULL, since the ALTER has no default) round-trips through the feed as JSON null, no 500. The index is correctly deferred to #143. -
Feed provenance, executed: the repo-scoped feed's local block carries the trusted
record.owner_did, and the gossip block echoes the stored wire value verbatim, which I confirmed with a spoofedowner_didsurfacing unchanged (display-only, nothing filters on it). All fourreceived_ref_updatesSELECTs projectowner_did, so the shared row mapper never hits a missing column. The global feed and GraphQL (query, subscription, broadcast) carry the field through as a passthrough. -
[P3] The stored
received_ref_updates.owner_didis attacker-controlled (the peer signature authenticates the relaying node, not this field, andnotify_syncaccepts it unsigned by default) and is echoed verbatim, as the spoof test shows. It is display-only and nothing gates on it, so not a break. A short comment at the two ingest sites (p2p/mod.rs,api/peers.rs:398) marking the column untrusted would keep #143's DID-aware feed gate from consuming it as trusted; that gate must reconcile it against the repo slug or a verified cert.
@Gravirei the P2s and the migration all check out, thanks. @kevincodex1 this is ready.
|
hello @Gravirei kindly rebase to main and fix conflicts |
63ab601 to
b46005b
Compare
Stale after the rebase to b46005b; re-reviewing the new head.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed on b46005bf. The owner_did work (#144) is solid: owner_did is carried on all four emit paths, the shared received_ref_updates mapper and migration v10 line up, and the feed gate never trusts the stored value. But the rebase onto 0.5.0 pulled api/ipfs.rs backward relative to main and reintroduces an unauthenticated availability regression, so I can't approve this head.
Findings
-
[P1] Restore the pre-walk object-existence check on
GET /ipfs/{cid}
crates/gitlawb-node/src/api/ipfs.rs(get_by_cid)
Current main (via #133,466a550) runsstore::object_typeandcontinues before the reachability walk, so a random CID can't trigger a full-history walk on repos that don't hold the object. This branch removes that guard and runsallowed_blob_set_for_caller(git rev-list --all+ a per-commitls-tree) beforeread_object. The route isoptional_signatureand unthrottled, andget_by_ciditerates the fulllist_all_repos(), so one anonymous request with an absent CID walks every public path-scoped repo's history. I reproduced it: an anon request for an object not in the repo fires the walk on this head and fires zero on main. Restore main'sobject_typepre-filter, or gate the walk behind an indexed proof the object is present. -
[P2] Drop the
/ipfs/{cid}change from this PR
crates/gitlawb-node/src/api/ipfs.rs
d995c4deis a ~150-line ipfs gate revision unrelated to #144, and main already carries #133. Rebasing onto current main to drop it (keeping main'sipfs.rs) removes both the P1 and the scope creep in one move. -
[P2] Correct the docstring claiming owner_did feeds the feed gate
crates/gitlawb-node/src/p2p/mod.rs:42
The comment says owner_did was added "so the feed gate can distinguish different DID methods that share the same trailing segment," butvisibility::ref_update_row_names_repomatches onrow_repoplus the local record and collapses both via.split(':').next_back(), never reading the wire owner_did. The field is additive display/storage today. Either wire owner_did into the matcher or fix the doc so it doesn't imply that collision is closed. -
[P3] Exercise the real v9 to v10 upgrade over populated data
crates/gitlawb-node/src/db/mod.rs(migration_v10_creates_owner_did_column)
The test runs the full v1..v10 chain on a fresh pool; nothing seedsschema_migrationsat v9 with existingreceived_ref_updatesrows before applying v10. The migration is safe (ADD COLUMN IF NOT EXISTS, nullable, Option readers), so this is coverage rather than a defect. A test that migrates to v9, inserts a row without owner_did, then applies v10 and assertsowner_did IS NULLpins the actual upgrade path. -
[P3] Don't render peer-supplied owner_did as authoritative
crates/gitlawb-node/src/api/events.rs:261
Gossip and notify rows store owner_did verbatim off the wire with no signature binding it to the owner's key, and the feed echoes it. It isn't an authz break (the gate uses the matched local record, never the stored value) and it's the same spoofable class aspusher_did/node_did, but rendering the display owner from the local record for locally-known rows would close it.
The owner_did core is ready; the ipfs regression is the only merge blocker. @Gravirei recommend rebasing onto current main to drop d995c4de and pick up the #133 ipfs gate.
b46005b to
1ddbc5b
Compare
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of review-feedback/test issues that need to be addressed before this is ready.
Findings
-
[P2] Make the v10 migration test actually start from a v9 schema
crates/gitlawb-node/src/db/mod.rs:3320
This test says it simulates an existing v9 node, but it first runsdb.migrate(), which applies v10 and createsreceived_ref_updates.owner_didbefore the test deletes and reseedsschema_migrationsat v9. The later insert withoutowner_didtherefore runs against a table that already has the new column, so the test would still pass if the upgrade path being claimed here was not actually exercised. Please build the pre-v10 schema without applying migration 10 first, or explicitly drop the new column/index before the reseed, then apply v10 and assert the old populated row survives withowner_did IS NULL. -
[P3] Complete CodeRabbit's owner_did feed assertion
crates/gitlawb-node/src/test_support.rs:2432
CodeRabbit's request to exposeowner_didin/api/v1/events/ref-updatesis marked addressed, and the handler now serializes the field, but the end-to-end test still only asserts the returnedrepo. Since the row inserted above hasowner_did: Some(owner.into()), this test would pass even if the API regressed to omitting the new field again. Please complete that review item by assertingevents[0]["owner_did"]matches the inserted owner DID.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I found one remaining issue that needs to be addressed before this is ready.
Findings
- [P2] Defer the unused owner_did index until the gate query lands
crates/gitlawb-node/src/db/mod.rs:834
Migration v10 createsidx_ref_updates_owner, but this PR only stores and projectsreceived_ref_updates.owner_did; no current query filters, joins, or orders that table byowner_did. Since migrations run inside a transaction here, this index cannot be built concurrently, so existing nodes with populated ref-update tables take avoidable write-blocking startup work for no current read path. Please ship just the nullable column in this plumbing PR, then add the index in the follow-up that actually consumesowner_didfor the feed gate.
58d5c76 to
2107633
Compare
Superseded by a fresh re-review on the current head (2107633).
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-reviewed the current head. The owner_did wire plumbing is sound, and I re-verified the security-critical property by execution on this head rather than reading it: the wire owner_did is display-only and never a visibility-gate input. Driving a forged wire owner_did that names a private repo through the resolver, the row stays dropped for anon and for a caller authenticated as the forged DID, and only the true owner sees it; forcing the gate open flips it to a leak, so the gate is load-bearing. The rebase is clean, #149's list_ref_certificates bound and #180's per-IP creation limiter both survive (verified by their tests). Migration is a correct new v11 (nullable, idempotent) with a load-bearing upgrade test, all four emit paths carry owner_did, and the shared mapper round-trips a null owner_did with no panic.
One item from my last pass is still open, plus two small ones.
Findings
-
[P2] Drop the redundant per-request repo dedup on the ref-updates feed.
crates/gitlawb-node/src/api/events.rs:145,crates/gitlawb-node/src/graphql/query.rs:80—collect_visible_ref_updatesalready loadslist_all_repos_deduped()for the gate (events.rs:57), but both handlers load it a second time for the owner_did display fallback, so every anonymous request now runs the full-table dedup CTE twice (one before this PR). The second load is unconditional: it fires even when every returned row already carries a wire owner_did and the fallback is never consulted, which is the steady state once peers upgrade. Thread the set the collector already holds through to the handler, or gate the second load onupdates.iter().any(|u| u.owner_did.is_none()). -
[P3] Resolve the display owner with an exact key match, not the fail-safe gate matcher.
crates/gitlawb-node/src/visibility.rs:229— the legacy-None fallback reusesref_update_row_names_repo, whoserecord_key.starts_with(row_key)is always true when an empty-owner-segment slug (/name) reducesrow_keyto"", so a legacy row can display the wrong owner. Display-only: the same over-match in the gate only ever drops (covered byfeed_empty_owner_slug_matches_and_drops), and it affects only pre-#144 rows stored withowner_did = None. Resolve withnormalize_owner_keyand require a unique match, skipping on ambiguity. -
[P3] Add a regression test for a forged wire owner_did. The existing gate tests forge the slug or set
owner_didtoNone; none drives a forged wireowner_didfield, which is the exact must-not (a peer-supplied owner_did must not unlock a private row). The property holds today, but a permanent test guards a future change that starts keying on the wire value. A drop-in that mirrors the existing resolver tests:#[sqlx::test] async fn forged_wire_owner_did_never_unlocks_private_row(pool: PgPool) { let db = db(pool).await; db.create_repo(&repo("r1", OWNER, "widget", false)).await.unwrap(); // private let mut row = ref_row("u1", "z6MkOwner/widget"); row.owner_did = Some("did:key:z6MkAttacker".into()); // forged wire owner db.insert_ref_update(&row).await.unwrap(); let schema = schema(db); let q = r#"{ refUpdates(repo: "z6MkOwner/widget") { refName ownerDid } }"#; assert_eq!(count(&anon(&schema, q).await), 0); assert_eq!(count(&authed(&schema, q, "did:key:z6MkAttacker").await), 0); assert_eq!(count(&authed(&schema, q, OWNER).await), 1); }
I ran this against the branch (passes) and against a fail-open gate (fails), so it is load-bearing.
The core is solid and the security properties hold. The P2 is the one I'd want fixed before merge, and it's a small change; the two P3s are polish.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore the iCaptcha PoW protocol support before merging
crates/icaptcha-client/src/lib.rs:194
This head is behind the current PR base and removes theChallenge.powfield, its solver, andpowNoncefrom the/v1/answerrequest (as well as deletingpow.rs). A PoW-enabled iCaptcha service therefore has its requiredpowextension silently ignored and receives only{token, answer}, so it rejects the solve andglcannot mint a proof for register/repository-create/fork writes. Rebase onto currentmainand preserve the optional PoW parsing, solving, nonce submission, and regression coverage. -
[P1] Keep iCaptcha key trust scoped to the full origin
crates/icaptcha-client/src/lib.rs:110
x-icaptcha-urlis controlled by the node, but the newhost_ofcomparison accepts any HTTPS port on the configured operator host and marks itkey_trusted. For example, an operator configured withhttps://icap.example:443can be redirected tohttps://icap.example:8443; both challenge and answer requests then sendGITLAWB_ICAPTCHA_API_KEYto that distinct listener. The base implementation compared scheme, host, and effective port specifically to prevent this. Restore full-origin matching for both acceptance and bearer-key attachment. -
[P2] Return the received event's owner DID on the repo-scoped feed
crates/gitlawb-node/src/api/events.rs:284
The new field is stored onReceivedRefUpdate, and the global REST and GraphQL feeds prefer that wire value, but this projection unconditionally substitutesrecord.owner_did. A received row whose slug collides with a hosted repo is consequently reported as owned by the local record rather than by its stored full DID—the exact identity ambiguity this change is intended to carry. Projectu.owner_didwhen present and use the local value only as the legacy-Nonefallback, with a collision regression test. -
[P2] Do not perform a second full repository-dedup scan for every feed read
crates/gitlawb-node/src/api/events.rs:145
collect_visible_ref_updatesalready loadslist_all_repos_deduped()for its visibility gate, but the new REST projection immediately loads it again for the legacy fallback; the GraphQL resolver repeats the same pattern. This doubles the full-repository scan on every anonymous global-feed request even when all bounded result rows already containowner_did. Thread the collector's deduped records through, or load them only if a returned legacy row actually needs fallback resolution.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not persist peer-supplied
owner_didas trusted event ownership
crates/gitlawb-node/src/api/peers.rs:398
The HTTP notify path copiesreq.owner_didstraight intoReceivedRefUpdate, and the gossipsub receive path does the same withevent.owner_did. The new REST and GraphQL projections then prefer that stored wire value whenever it is present. In deployments that still accept unsigned peer notify, any caller can setnode_didto a known peer and make a visible row forzAlice/widgetreportowner_did: "did:key:zVictim"; even with signatures enforced, a compromised or buggy known peer can poison persisted event ownership. Because this field is presented as the full owner DID and is intended to become DID-aware feed-gate input, please either validate/bind it to the row's repo owner before storing/exposing it or treat it as untrusted and derive display ownership from trusted local state when possible. -
[P3] Resolve legacy fallback owners with an exact unique match
crates/gitlawb-node/src/api/events.rs:153
Theowner_did = Nonefallback reusesref_update_row_names_repo, which is deliberately loose for the fail-closed visibility gate: it strips both owners to their last colon segment and accepts prefix matches. That is safe for dropping rows, but it is not safe for display attribution. For example, a legacy visible row such as/widgetoralice/widgetcan match the first local readablewidgetrepo whose owner key shares that empty/short/trailing segment, so REST and GraphQL can synthesize the wrong full owner DID for the exact cross-method collision class this PR is trying to disambiguate. Please use an exact/normalized, unique local match for legacy display fallback, or leaveowner_didnull when the owner cannot be proven.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Batch the owner-DID resolution instead of issuing a query per feed row
crates/gitlawb-node/src/api/events.rs:146
A publicGET /api/v1/events/ref-updates?limit=200now starts up to 200get_repoqueries throughjoin_allafter the collector has already fetched the repository/rule data; GraphQL performs the same lookups serially. The node's pool defaults to 20 connections, so a few ordinary max-page feed requests can queue or time out the pool and starve other API work. Resolve the bounded page in one batch (or reuse a resolver map built from the collector's repository snapshot) rather than querying once per event. -
[P2] Do not turn owner-resolution database failures into successful incomplete feeds
crates/gitlawb-node/src/api/events.rs:153
Both REST projections coerce everyresolve_ref_update_owner_diderror tonull, so a pool timeout or database failure after the visibility collector succeeds returns HTTP 200 with missing ownership that clients cannot distinguish from an unknown owner. The GraphQL projection propagates the identical resolver error, and the surrounding REST code explicitly treats database errors as errors. Propagate this failure (or return an explicit per-row error state) instead of silently emitting partial data. -
[P2] Preserve a full DID when the matching repository is a mirror-only row
crates/gitlawb-node/src/db/mod.rs:2380
upsert_mirror_repostores a mirror'sowner_didas the bare normalized key, andget_repointentionally returns that mirror when no canonical record exists. A received event carryingowner_did: "did:key:z..."then passes the normalized comparison but the resolver returns the mirror's bare"z...", defeating this PR's full-DID wire/API contract for mirror-only repos. Return the validated full wire DID in this case, or preserve a canonical full DID for mirror records, and cover the mirror-only path.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Approving on a05f61d. I re-verified the security-critical properties by execution on this head, not by reading.
jatmn's P1 (peer-supplied owner_did persisted as trusted) is resolved: the display resolver echoes a wire owner_did only when it normalizes to the local repo's canonical owner, so a forged value drops to null. I drove the real resolver across a forged and method-spoof matrix (did:key:zVictim, did:web:evil.com:zAlice, and the mirror variants); all dropped. I confirmed the guard is load-bearing by removing the normalize check and watching the forged values leak, and the committed events_drop_forged_peer_owner_did test fails when the binding is removed. End to end through list_ref_updates, a stranger who forges ownership of a private repo still sees count=0 with no new_sha leak, the owner sees the row, and the same row on a public repo is visible, so the empty result is real gating rather than a vacuous feed. normalize_owner_key only collapses did:key:X and bare X, so no cross-identity or cross-method spoof is possible, and the visibility gate keys on the local repo record and is unchanged by this PR.
The rest of the round is addressed and I verified each: batched owner-DID resolution (one query per distinct local repo), error propagation (a DB failure inside the resolver now surfaces as 500, not a 200 with null ownership), full-DID preservation for mirror-only repos, exact-match legacy fallback, and the iCaptcha PoW and key-origin restoration (pow.rs and lib.rs are byte-identical to current main). The new owner_did column is a v11 migration with an upgrade-path test, it is projected in the shared received_ref_updates mapper, and the feed keyset cursor stays internal (no client-facing cursor). CI is green.
@kevincodex1 this is ready to merge.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Rebase this stale branch onto current
mainbefore merge
crates/gl/src/http.rs:189
The head is still based at3ec8cbf, before the merged iCaptcha work in #181 and its client retry tests in #168. As a result, the PR's base-to-head diff includes that unrelated already-merged work and, when compared to currentmain, removes the entireglHTTP/iCaptcha test module. Git's synthetic merge happens to restore those tests, but that means the tree being merged is not the head CI and review are evaluating. This also leaves the repeated maintainer rebase request unresolved. Rebase onto currentmain, preserve the current iCaptcha changes/tests, and rerun the checks so the reviewable diff is only the owner-DID work. -
[P2] Resolve display owners without serial per-slug database queries
crates/gitlawb-node/src/db/mod.rs:2403
resolve_ref_update_owner_didsis invoked by the anonymous REST and GraphQL feeds after they can collect up to 200 rows, but it awaitsget_repoonce for every distinct received slug. Received notify/gossip rows can contain 200 different remote slugs, including slugs with no local repository, so one public feed request now adds 200 serialized SQL round trips after the collector has already loaded the local repository/rule data. Repeated reads can therefore turn peer-controlled feed contents into avoidable connection-pool pressure and latency. Resolve the distinct keys in one set-based query (or reuse/filter the collector's already-loaded local records) instead of issuing one awaited lookup per key.
…mprove migration test, use local owner_did
…omplete owner_did API assertion
…ion test, add owner_did echo assertions
…ion; mirror projection in GraphQL
…fix feed owner_did projections
sha2 was at line 16 in our branch but at line 18 in upstream/main. The 3-way merge in CI's pull/145/merge checkout kept both lines, causing a 'duplicate key' error. Moving sha2 after tracing matches upstream/main exactly, so the merge commit will have a single entry.
The peer-supplied owner_did (HTTP notify + gossipsub receive) and the repo slug arrive over untrusted paths, yet REST and GraphQL projected the wire value as trusted feed ownership. - P1: add Db::resolve_ref_update_owner_did so a wire owner_did is echoed only when it matches the canonical owner of the actual local repo at that slug; otherwise it is dropped (returns null). The canonical DID is returned, never the raw wire bytes. - P3: legacy owner_did = None rows are now attributed via an exact, normalized, unique local match (get_repo), not the prefix-tolerant drop gate, so a cross-method slug collision cannot synthesize the wrong owner. - Unify REST (global + per-repo gossip) and GraphQL on the shared resolver so the two surfaces cannot drift. Add tests for forged-wire drop and exact-legacy attribution.
…for mirrors P2 batch: replace per-row join_all with resolve_ref_update_owner_dids that issues at most one get_repo query per distinct local repo in the page rather than one per event row. Applies to both REST and GraphQL surfaces. P2 error: REST handler now propagates DB errors (rather than silently coercing them to null) by calling the batch method with ?. P2 mirror: when the validated wire DID carries a full method prefix (e.g. did:key:z...) and the matching repo is a mirror storing only the bare key, return the full wire DID instead of the bare key to preserve the DID-method contract. P1/P3 already addressed in fb553e6.
…resolution Rebased from stale 3ec8cbf base onto current upstream/main (ad7c2b2), dropping the already-merged iCaptcha work from the diff. The PR diff is now only the 11 owner-DID files. The batch owner-DID resolver now issues a single set-based SQL query with one OR condition per distinct slug, replacing the loop of per-key get_repo calls. A 200-row page of 200 distinct remote slugs now costs 1 SQL round-trip instead of 200. P1/P3 already addressed in c64c082. P2 mirror, error propagation, and the earlier batch refactor preserved in the rebased commits.
a05f61d to
083540b
Compare
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Refreshing my approval on the current head. My last one sat on a05f61d3; the delta since then is exactly your two P2s, and both are clean.
The rebase is real, not just a synthetic merge: merge-base(083540b5, main) is current main and the three-dot diff no longer carries the iCaptcha/#181/#168 work, so the reviewable tree is only the owner-DID change. I checked the rebase didn't drop a gate that main added after the old base, the canonical-over-mirror (#124), quarantine-drop (#143), and repo-scoped read gating (#157) fixes are all present and untouched on the head.
The N+1 fix is sound. resolve_ref_update_owner_dids now resolves every distinct slug in one batched query instead of a get_repo per key. I verified it's equivalent to the loop it replaced: same WHERE (OWNER_KEY_CASE) = $p AND name = $q and the same ORDER BY [canonical-over-mirror], created_at ASC, id ASC, with first-wins or_insert, so each key resolves to the exact row get_repo's LIMIT 1 returned, canonical preferred over mirror preserved. The SQL is injection-safe (the CASE fragment is a const, every value is bound), the disjuncts hit idx_repos_owner_key_name (the CASE is byte-identical to the index expression), and the page is clamped to 200 so the OR-list is bounded. The re-key fails safe: any Rust/SQL normalization drift yields row-not-found → null owner, never a wrong owner.
The security-critical property is intact and I re-verified it by execution on this head, not by reading. Step 3, the forged-owner drop, is byte-identical to my prior approved head, and events_drop_forged_peer_owner_did is load-bearing: making the mismatch arm echo the wire owner turns it red, restoring returns it green. The full gitlawb-node suite is 501 passed / 0 failed, including the whole feed visibility matrix and the mirror-doesn't-unlock-private-canonical guard. Migration is v11 (new highest block, upgrade-path test green), owner_did is projected in the shared feed SELECT, and the keyset cursor stays internal.
@kevincodex1 this is ready to merge.
Summary
Carry the full owner DID on the ref-update wire event so the feed gate can distinguish different DID methods that share the same trailing segment (e.g.
did:web:host:alicevsdid:gitlawb:alice).Motivation & context
Refs #144
The ref-update wire slug was previously constructed from the last colon-segmented component of the owner DID (
{owner_did.split(":").last}/{name}), which is lossy. Two owners with different DID methods but the same trailing segment would produce identical slugs, causing the feed gate to over-drop remote rows for anonymous callers.This PR is the plumbing half of #144: it carries the full owner DID on the wire and stores it in the database so a follow-up can teach the feed gate to use
did_matchesfor proper disambiguation. The gate itself (the second half of #144) will land in a separate change on top of this.Kind of change
What changed
crates/gitlawb-node (gitlawb-node):
p2p/mod.rs: Addedowner_did: Option<String>toRefUpdateEventwith#[serde(default)]for backward compat with older peers that do not include the fielddb/mod.rs: Addedowner_did: Option<String>toReceivedRefUpdate, added migration v10 to addowner_did TEXTcolumn andidx_ref_updates_ownerindex to thereceived_ref_updatestable, updated all SELECT/INSERT queries to include the columnapi/repos.rs: Setowner_did: Some(record.owner_did)when publishing ref-update events over gossip; addedowner_didto the HTTP sync notify bodyapi/peers.rs: Addedowner_didtoNotifyRequestand wired it through toReceivedRefUpdateHow a reviewer can verify
DATABASE_URL=postgresql://gitlawb:changeme@172.19.0.2:5432/gitlawb cargo test -p gitlawb-node cargo fmt --check cargo clippy --workspace --all-targets -- -D warningsBefore you request review
cargo test --workspacepasses locally (327/327)cargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfix(...),test(...),style(...)).env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
This is a backward-compatible wire-format addition: the new
owner_didfield is#[serde(default)], so events from older peers deserialize without it (owner_did: None). Therepofield format is unchanged. Migration v10 adds the column to existing databases;ADD COLUMN IF NOT EXISTSis idempotent for re-runs.did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsSummary by CodeRabbit
New Features
owner_didwhen available.owner_didas optional (omittable).Bug Fixes
owner_didis missing or null in incoming notifications.