refactor(visibility): route is_owner through the shared did_matches matcher (#153)

[Gravirei]

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 owner matching for visibility checks so equivalent did:key representations are recognized consistently.
  • Preserved access restrictions across different DID methods, preventing incorrect matches when identifiers only share the same trailing value.
  • Added unit test coverage for the DID matching behavior, including empty-input and regression cases.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

is_owner in crates/gitlawb-node/src/visibility.rs now delegates owner matching to crate::api::did_matches. New tests cover empty DID matching and regression cases for bare did:key owners and cross-method callers.

Changes

Owner DID matching fix

Layer / File(s)	Summary
DID matching regression test crates/gitlawb-node/src/api/mod.rs	did_matches is tested for empty inputs and for matching an empty string against the bare did:key: prefix.
Owner matching logic and regression tests crates/gitlawb-node/src/visibility.rs	is_owner now uses crate::api::did_matches, and visibility_check tests cover bare-owner/full-caller matching plus 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: Also concerns did_matches-style handling for bare versus full did:key values.

Possibly related PRs

• Gitlawb/node#25: Directly changes the owner-matching path in visibility.rs and updates related tests.
• Gitlawb/node#68: Also uses crate::api::did_matches for owner/DID comparison logic.
• Gitlawb/node#157: Touches the same repo-read authorization path affected by visibility_check owner matching.

Suggested reviewers: jatmn, kevincodex1

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes match #153 by fixing owner matching with did_matches, preserving cross-method denial, and adding the requested regressions.
Out of Scope Changes check	✅ Passed	The only extra change is a supporting did_matches test, which is directly related to the linked issue and not out of scope.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title clearly summarizes the main change: switching is_owner to shared did_matches matching.
Description check	✅ Passed	The description follows the template and includes summary, motivation, change list, verification, and protocol impact.

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

_{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.

[jatmn]

LGTM

[beardthelion]

Re-approving on 73bd7a4. My earlier approval was on dce4330; the only change since is the added empty_did_matches test, so I re-ran the owner-gate matrix by execution against the current head.

Routing is_owner through did_matches fixes the #153 mirror-owner lockout: a bare owner key with a full did:key caller now resolves to Allow, and reverting is_owner reproduces the lockout. The security-critical negatives hold under mutation. Cross-method (did:gitlawb: / did:web: against a bare key) denies, and a trailing-segment over-widen makes cross_method_did_denied_with_bare_owner go red, so that guard is load-bearing. A distinct full key denies. The empty-DID match is unreachable: an empty or bare did:key: id never resolves through to_verifying_key, so the caller reaching the owner arm is always a verified did:key or None. Quarantine still drops before the owner short-circuit (authorize_repo_read and the git-serve paths), so a newly bare-key-matched mirror cannot surface a hidden repo.

Findings

• [P3] Pin the fail-safe negative in empty_did_matches. crates/gitlawb-node/src/api/mod.rs:118. The test pins the degenerate positives (did_matches("",""), did_matches("","did:key:")) but not the security-relevant negative: an empty owner_did must never match a real key. It is unreachable today, so this is not a live issue, but a future over-widening refactor would keep the test green. I confirmed a strip + starts_with matcher leaves this test passing, while adding assert!(!did_matches("", "did:key:z6MkReal")) turns it red. Consider adding that negative, and optionally an empty-id guard inside did_matches so the invariant lives at the matcher (the #153 tests still pass with the guard).

Optional and non-blocking. @kevincodex1 good to merge.

[kevincodex1]

please fix conflicts

[beardthelion]

Verified by execution that this is behaviorally a no-op on the current base, not a live lockout fix. Routing is_owner through did_matches is identical to the two-side normalize_owner_key already on main after #157: I ran the owner gate across the clone/fetch path, list_repos, and subtree withholding, as the owner, as a cross-method did:gitlawb: caller, and anonymous, against both the did_matches body and the base body, with identical Allow/Deny outcomes. The code is safe and consolidating onto the shared matcher is a genuine improvement. The changes I'm asking for are to the framing and one test comment so the record stays accurate.

Findings

• [P2] Fix the false "previously denied (lockout)" comment; the case already passes on the current base.
  crates/gitlawb-node/src/visibility.rs:434
  The lockout lived on the pre-#157 single-side is_owner. On this PR's base (26bc3f6, which already normalizes both sides) is_owner("z6MkOwner", "did:key:z6MkOwner") returns Allow, so reverting is_owner to the base body leaves both new tests green. Keep them as forward guards against a revert to single-side, but correct the comment (and the "fixes #153 lockout" assertion message on line 438) to say that, rather than claiming this diff lifts a lockout.

• [P3] Retitle from fix(...lockout) to refactor(...) so the changelog is accurate.
  crates/gitlawb-node/src/visibility.rs:32
  Since #157 already resolved the read lockout, a fix title records a changelog Bug Fix for a behavioral no-op. Suggest: refactor(visibility): route is_owner through the shared did_matches matcher (#153). It still legitimately closes #153, which also asked for the stale did_matches doc comment to be corrected, and this does that.

Stale approval on an earlier head; superseded by my review on 0be2f18.

Stale approval on an earlier head; superseded by my review on 0be2f18.

Superseded — re-reviewed and approving on the current head 96cd9e1. My request-changes was framing-only and its substantive ask (the false "lockout" test comment) is resolved on this head.

[beardthelion]

Re-reviewed on 96cd9e1. The is_owner→did_matches reroute is a verified no-op, and my earlier request-changes was framing-only — its substantive ask is resolved on this head, so I've dismissed it and I'm approving.

Verified by execution on this head: did_matches is byte-identical to the old double-normalize_owner_key body across an adversarial truth table (cross-method, did:key:z:extra, empty, bare-vs-full) — zero divergence, so no visibility decision changes on any reader. Both new guards are load-bearing — reverting is_owner to single-side normalize reds bare_owner_full_caller_allows_owner, and to a method-blind segment match reds cross_method_did_denied_with_bare_owner. Cross-method stays denied, INV-9 parity with the SQL owner key holds (normalize_owner_key_matches_sql_case), the owner short-circuit still serves the owner through the real feed reader, and the quarantine drop still precedes it (feed_quarantined_* stays withheld from the owner even with is_owner neutralized). The stale "lockout" comment is corrected.

Findings

• [P3] Retitle the PR so the changelog stays accurate. crates/gitlawb-node/src/visibility.rs (#153). The commit is refactor(visibility): … but the PR title still reads fix(visibility): … to fix bare-key mirror lockout (#153). Since #157 already lifted the read lockout, a squash-merge on the current title records a Bug Fix for a behavioral no-op. Retitle to refactor(visibility): route is_owner through the shared did_matches matcher (#153), or squash using the commit subject.

@kevincodex1 good to merge once the title's sorted.