fix(visibility): route is_owner through did_matches to fix bare-key mirror lockout (#153) by Gravirei · Pull Request #156 · Gitlawb/node

Gravirei · 2026-07-06T07:36:42Z

Summary

Switch is_owner from normalize_owner_key (single-side normalization) to did_matches (two-side normalization with cross-method rejection) so a mirror-only repo owner authenticated with their full did:key: is not denied read to their own repo.

Motivation & context

Closes #153

visibility::is_owner used normalize_owner_key(owner_did) which only strips did:key: off the owner side. When a mirror row stores owner_did as bare short key and the caller authenticates with full did:key:, the comparison fails and the owner is locked out.

The old doc comment warned that did_matches "would let a non-key canonical row bypass the #124 visibility gate" — but did_matches already rejects cross-method collisions (did:key:X != did:gitlawb:X), so that reasoning is stale.

Kind of change

Bug fix

What changed

crates/gitlawb-node/src/visibility.rs — Replaced is_owner body with crate::api::did_matches(owner_did, caller) and updated the doc comment to reflect current semantics.
Tests — Added bare_owner_full_caller_allows_owner (regression for the lockout) and cross_method_did_denied_with_bare_owner (cross-method bypass guard).

How a reviewer can verify

cargo test -p gitlawb-node -- visibility::tests

Protocol & signing impact

Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats
Discussed in an issue before implementation
Backward-compatible with existing nodes and previously signed history

Summary by CodeRabbit

Bug Fixes
- Improved repository owner matching so access checks work consistently across equivalent identifier formats (e.g., full vs shortened did:key forms).
- Ensured that different DID method types (such as did:web:* vs did:key:*) are still not treated as the same owner.
Tests
- Added regression tests to verify did:key representation-agnostic matches and to confirm cross-method matches are denied.

Gitlawb#126) The IPFS visibility gate used withheld_blob_oids (a deny-set enumerating only reachable blobs), so a dangling/unreachable blob was absent from the set and served in cleartext to anonymous callers. Flip to an allowed-set (allowed_blob_set_for_caller) that enumerates reachable blobs the caller may read: a dangling blob has no path, is never in the set, and 404s.

Move store::read_object before the allowed_blob_set_for_caller spawn_blocking call so random-CID spray against repos with path-scoped rules cannot trigger full-history git walks on repos that don't carry the object.

…tion ✓ P3 blocker fixed: cargo fmt applied — the format gate will pass. ✓ P3 cleanup resolved: withheld_blob_oids is still used by replication code in repos.rs, so it stays. • P2 follow-up: Tree/commit disclosure tracked in Gitlawb#135 — out of scope here.

…ntent reading

…irror lockout (Gitlawb#153)

coderabbitai · 2026-07-06T07:36:55Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 540338cd-ab1e-4c9e-bed5-dbca4fa5d805

📥 Commits

Reviewing files that changed from the base of the PR and between ca579c6 and dce4330.

📒 Files selected for processing (1)

crates/gitlawb-node/src/visibility.rs

🚧 Files skipped from review as they are similar to previous changes (1)

crates/gitlawb-node/src/visibility.rs

📝 Walkthrough

Walkthrough

visibility.rs now matches repo owners through crate::api::did_matches instead of direct owner-key equality, so bare and full did:key forms compare consistently while cross-method DID matches still fail. Two regression tests cover the updated read authorization behavior.

Changes

Owner DID Matching Fix

Layer / File(s)	Summary
Owner matching logic and regression tests `crates/gitlawb-node/src/visibility.rs`	`is_owner` now uses `did_matches` for owner comparison, and `visibility_check` gains tests for bare-owner/full-`did:key` allowance and cross-method denial.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Paged repo-list owner filter misses bare-owner mirror rows when given a full DID #102: Covers the same full-vs-bare did:key owner-matching semantics for mirror rows, though in a different read path.

Possibly related PRs

Gitlawb/node#25: Modifies the same visibility owner-matching logic and related tests in visibility_check.
Gitlawb/node#68: Also switches owner matching to crate::api::did_matches for did:key representation consistency.
Gitlawb/node#52: Routes REST repo reads through authorize_repo_read, which depends on visibility_check outcomes.

Suggested labels: crate:node, subsystem:visibility, kind:bug

Suggested reviewers: kevincodex1, jatmn

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main behavior fix in visibility ownership matching.
Description check	✅ Passed	The description follows the template well and includes summary, motivation, change list, verification, and protocol impact.
Linked Issues check	✅ Passed	The changes satisfy `#153` by switching to did_matches, fixing the mirror-owner lockout, preserving cross-method denial, and adding regressions.
Out of Scope Changes check	✅ Passed	The PR stays focused on the ownership-matching bug fix, the stale comment update, and the required regression tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

jatmn

Thanks for the contribution. I do not see any actionable issues from my review.

@kevincodex1 LGTM

beardthelion

Verified the security-critical property by execution, not inspection: routing is_owner through did_matches widens who counts as owner by exactly one case, the bare-key mirror owner matched by the same key's full did:key: caller (the #153 fix). Reverting is_owner makes bare_owner_full_caller_allows_owner fail, so it's load-bearing. Cross-method stays denied because only did:key: is stripped, so did:gitlawb:zX keeps its method and can't equal a bare id. Ran the full owner/DID truth table and the node suite (424 green). The short-circuit isn't exploitable by the widening: the newly-matched caller is the Ed25519-verified owner, and this converges the read gate onto the same did:key-aware normalization the mutation gate and SQL owner filter already use, so it reduces gate/query skew.

Findings

[P3] Pin the empty-DID invariant on the owner gate
crates/gitlawb-node/src/api/mod.rs:70
did_matches("", "did:key:") and did_matches("", "") return true. Unreachable today (the read-gate caller is always a verified did:key or None, and an empty-owner mirror slug is quarantined before the gate) and it predates this PR since require_repo_owner already uses did_matches. But this change makes did_matches load-bearing on the read path too, so a one-line test pinning "the gate caller is a validated did:key or None" is cheap insurance. Optional.

Concur with the approval; the note is defense-in-depth, not a blocker.

Gravirei added 12 commits June 30, 2026 19:52

perf(ipfs): check object existence before allowed-blob walk

3aa7bf0

Move store::read_object before the allowed_blob_set_for_caller spawn_blocking call so random-CID spray against repos with path-scoped rules cannot trigger full-history git walks on repos that don't carry the object.

refactor(ipfs): streamline object retrieval by separating type and co…

002f354

…ntent reading

docs(ipfs): update get_by_cid comment to reflect split object retrieval

f2c91a8

Merge remote-tracking branch 'upstream/main'

0b7c494

Run cargo fmt on store.rs

03ba714

Merge remote-tracking branch 'upstream/main'

03c90f8

Merge remote-tracking branch 'upstream/main'

94bb216

Merge remote-tracking branch 'upstream/main'

79650df

Merge remote-tracking branch 'upstream/main'

26d8d48

fix(visibility): route is_owner through did_matches to fix bare-key m…

ca579c6

…irror lockout (Gitlawb#153)

Gravirei marked this pull request as ready for review July 6, 2026 07:37

Copilot AI review requested due to automatic review settings July 6, 2026 07:37

Copilot AI reviewed Jul 6, 2026

style(visibility): fmt is_owner test lines

dce4330

beardthelion added crate:node gitlawb-node — the serving node and REST API kind:bug Defect fix — wrong or unsafe behavior subsystem:visibility Path-scoped visibility and content withholding labels Jul 6, 2026

jatmn approved these changes Jul 6, 2026

View reviewed changes

beardthelion approved these changes Jul 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(visibility): route is_owner through did_matches to fix bare-key mirror lockout (#153)#156

fix(visibility): route is_owner through did_matches to fix bare-key mirror lockout (#153)#156
Gravirei wants to merge 13 commits into
Gitlawb:mainfrom
Gravirei:fix/issue-153-bare-key-mirror-deny

Gravirei commented Jul 6, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 6, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

Copilot AI left a comment

Uh oh!

jatmn left a comment

Uh oh!

beardthelion left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Gravirei commented Jul 6, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation & context

Kind of change

What changed

How a reviewer can verify

Protocol & signing impact

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

jatmn left a comment

Choose a reason for hiding this comment

Uh oh!

beardthelion left a comment

Choose a reason for hiding this comment

Findings

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Gravirei commented Jul 6, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 6, 2026 •

edited

Loading