fix: strict DID matching in get_profile - #298
Conversation
Replace the broad LIKE suffix match in profile resolution with the same normalize_owner_key / CASE pattern already used by get_repo, so non-key DID methods that share a short-id suffix cannot resolve the wrong profile. Add a sqlx test locking in bare, did:key, and non-key DID behavior.
github-actions
Bot
added
needs-issue
|
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. |
|
Warning Review limit reached
Next review available in: 55 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughProfile operations now use did:key-aware normalized matching. Upserts preserve stored DID values when called through aliases. Integration tests cover method isolation and normalization parity. Symlink- and permission-based tests now compile only on Unix. ChangesProfile DID lookup
Unix test guards
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 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/db/mod.rs`:
- Around line 3518-3531: Update upsert_profile to use the normalized existing
profile identity when performing updates: after resolving the row with
normalize_owner_key and PROFILE_DID_CASE_SQL, bind existing.did rather than the
raw input did. Preserve insert behavior for new profiles, and add regression
coverage that upserts a bare stored profile via its full did:key: form and
verifies the persisted fields are updated.
🪄 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 Plus
Run ID: 99f20673-ce5d-46da-948b-7939dda95e85
📒 Files selected for processing (1)
crates/gitlawb-node/src/db/mod.rs
|
🦁 |
get_profile equates bare short ids with did:key forms. upsert_profile must UPDATE by existing.did so a did:key: alias against a bare-stored profile actually persists. set_profile_cid uses the same CASE match. Add a regression test covering both write paths.
|
Addressed CodeRabbit's finding in 9836c3a:
@coderabbitai full review |
|
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 46 minutes. |
cargo audit fails on ruint 1.17.2 (incorrect overflow flags / truncated shift amounts). Upgrade the lockfile to the patched 1.20.0 release.
|
@beardthelion pls review 💪 |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The get_profile CASE + normalize_owner_key contract matches get_repo: bare and did:key alias, non-key methods stay distinct. I reverted get_profile to the old LIKE matcher against a did:gitlawb-only row and bare short resolved that row; with CASE it returns None. CodeRabbit's write-path ask landed: gutting the existing.did UPDATE bind leaves display_name at "before", and gutting set_profile_cid's CASE leaves profile_cid unset. Both shipped sqlx tests are green on this head after those checks.
Findings
-
[P2] Pin the cross-method deny with a lone non-key fixture
crates/gitlawb-node/src/db/mod.rs:4698
get_profile_does_not_match_non_key_did seeds both a bare row and a did:gitlawb twin, then asserts bare short returns the bare row. Under the old LIKE matcher Postgres can still return the bare row first, so that assertion can stay green while the bug the PR names is live. Add a case with only the non-key row present and assert get_profile(bare) is None (and keep the dual-row / did:key alias coverage you already have). -
[P2] Pin PROFILE_DID_CASE_SQL with the owner-key boundary matrix
crates/gitlawb-node/src/db/mod.rs:922
normalize_owner_key_matches_sql_case only exercises OWNER_KEY_CASE_SQL. PROFILE_DID_CASE_SQL is a hand-renamed twin on column did, and a drift there would reopen the same class silently. Run the same boundary VALUES list through PROFILE_DID_CASE_SQL (alias the column did) and assert Rust normalize_owner_key agrees.
Not an ask, recorded only: get_repo still carries ORDER BY / LIMIT 1 for mirror preference and get_profile does not. Auth cannot plant bare+did:key dual rows today, so I am not asking for a preference clause on this round. agents.rs short-id resolve stays method-blind; profile GET does not go through it.
…ary matrix Add a lone non-key DID fixture check to get_profile_does_not_match_non_key_did to verify that a bare short query returns None when only a non-key profile exists. Add profile_did_case_sql_matches_normalize_owner_key to pin PROFILE_DID_CASE_SQL against Rust normalize_owner_key across the full owner-key boundary matrix. Add cfg(unix) target gates to Unix-specific symlink and permission test helpers. Refs Gitlawb#298
|
Addressed review findings in b4016dc:
@coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/gitlawb-node/src/db/mod.rs (1)
920-923: 🚀 Performance & Scalability | 🔵 TrivialVerify index usage for
PROFILE_DID_CASE_SQL.
PROFILE_DID_CASE_SQLwrapsagent_profiles.didin aCASE. The new read and CID-update predicates compare that expression to a parameter. A normal B-tree index ondidwill not normally support this expression. Full-DID lookups can therefore regress to sequential scans.Run
EXPLAINwith production-like data. If the table can grow, add a matching expression index or preserve an indexable exact/alias branch.🤖 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 920 - 923, Verify the query plans for predicates using PROFILE_DID_CASE_SQL with production-like agent_profiles data via EXPLAIN, checking whether the did index is used for full-DID lookups. If scans occur and the table can grow, add a matching expression index or retain an indexable exact/alias predicate branch while preserving normalized-key matching.
🤖 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/db/mod.rs`:
- Around line 3461-3463: Update the agent_profiles schema/migration flow to
canonicalize equivalent profile DIDs and prevent both bare IDs and their did:key
aliases from coexisting. Add a de-duplication migration for existing rows, then
enforce uniqueness on the normalized DID value (or consistently canonicalize all
inserts) so get_profile and upsert_profile cannot select or update different
matching rows.
---
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 920-923: Verify the query plans for predicates using
PROFILE_DID_CASE_SQL with production-like agent_profiles data via EXPLAIN,
checking whether the did index is used for full-DID lookups. If scans occur and
the table can grow, add a matching expression index or retain an indexable
exact/alias predicate branch while preserving normalized-key matching.
🪄 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 Plus
Run ID: 9c46183e-7568-44fa-a2b6-6193c7977cf4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/git/repo_store.rscrates/gitlawb-node/src/sync.rs
Superseded by re-review of b4016dc; prior asks landed.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Both prior asks landed on b4016dc and are load-bearing. Reverting get_profile to the old LIKE matcher reds the lone non-key fixture. Drifting PROFILE_DID_CASE_SQL reds the new boundary matrix. Gutting the existing.did UPDATE bind and set_profile_cid's CASE still red the alias write test. CI is green on this head and the base is current with main.
Findings
(none)
Not an ask, recorded only: agent_profiles still keys the raw did PK, so bare and did:key: siblings can coexist and fetch_optional stays plan-order under that state. Auth writes only the did:key form, so the API cannot plant that pair today. Same residual as last round. Repos already carry an expression index for the twin CASE; profiles do not. Table is small enough that I am not asking for the migration on this PR.
@kevincodex1 this is ready to merge.
2 participants
Summary
get_profileused a broad SQL suffix match (did LIKE '%:' || $1) when resolving profiles by short id. That can match a different DID method that happens to share the same trailing id, so the wrong profile row can be returned (or the query can fail when multiple rows match).This aligns profile lookup with the existing
get_repo/normalize_owner_keycontract:did:key:only when the remainder is a bare key idCASEexpression (PROFILE_DID_CASE_SQL) instead of a wildcard suffixTest plan
get_profile_does_not_match_non_key_did(sqlx): bare short id anddid:key:resolve the key-form profile; a non-key DID with the same suffix resolves only itselfcargo test -p gitlawb-nodeNotes
Same pattern as the owner-key hardening already applied to repository lookup. Code-only change; no migration.
Summary by CodeRabbit
Bug Fixes
Tests