fix(visibility): route is_owner through did_matches to fix bare-key mirror lockout (#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 regression coverage for both matching and non-matching owner identity cases.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a51546a0-c765-49bd-a5a8-cb23f09d3c40

📥 Commits

Reviewing files that changed from the base of the PR and between 73bd7a4 and 0be2f18.

📒 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

The is_owner function in crates/gitlawb-node/src/visibility.rs was changed to delegate owner-identity matching to crate::api::did_matches instead of using normalize_owner_key and strict equality. Two regression tests were added covering bare-key/full-DID owner matching and cross-method denial.

Changes

Owner DID Matching Fix

Layer / File(s)	Summary
Owner matching logic and regression tests crates/gitlawb-node/src/visibility.rs	is_owner now delegates to crate::api::did_matches for representation-agnostic did:key comparison, fixing a lockout where a bare did:key owner denied access to its own repo when authenticated with the full form, while still denying cross-method DID matches; two new tests validate both scenarios.

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

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant visibility_check
  participant is_owner
  participant did_matches

  Caller->>visibility_check: request repo read
  visibility_check->>is_owner: check ownership(owner_did, caller)
  is_owner->>did_matches: compare(owner_did, caller)
  did_matches-->>is_owner: match result (representation-agnostic)
  is_owner-->>visibility_check: allow/deny
  visibility_check-->>Caller: Allow or Deny

Loading

Possibly related issues

• Paged repo-list owner filter misses bare-owner mirror rows when given a full DID #102: Both issues fix owner DID matching to treat full and bare did:key forms consistently via did_matches.

Possibly related PRs

• Gitlawb/node#25: Both PRs modify the repo-owner matching logic inside visibility.rs and its unit-test coverage.
• Gitlawb/node#68: Both PRs change owner matching to use crate::api::did_matches for representation-agnostic full-vs-short did:key comparisons.
• Gitlawb/node#141: Both PRs change how owner DID matching treats did:key full vs short forms across related authorization code paths.

Suggested reviewers: jatmn, beardthelion, kevincodex1

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main fix: routing is_owner through did_matches to resolve the bare-key mirror lockout.
Description check	✅ Passed	The PR description matches the template well, covering summary, motivation, change list, verification, and impact notes.
Linked Issues check	✅ Passed	The changes satisfy #153 by switching to did_matches, correcting the stale comment, and adding both required regressions.
Out of Scope Changes check	✅ Passed	The PR appears scoped to the visibility fix and its tests, with no unrelated code changes introduced.
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

• 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.