fix(core): honor wildcard capabilities in Ucan::can, matching is_attenuated_by#200
fix(core): honor wildcard capabilities in Ucan::can, matching is_attenuated_by#200euxaristia wants to merge 2 commits into
Conversation
…nuated_by Ucan::can did strict equality on both resource and action, so a validly delegated wildcard capability (with: "*", or can: "*" / "repo/admin") could never match a concrete runtime request even though is_attenuated_by treats those same values as wildcards during delegation-chain verification. Not a security issue: the mismatch only produces false negatives (denying a capability the delegation chain actually grants), never false positives. Dormant today since no production code path calls can() yet — auth's caller_authorized_to_push is pure did_matches(), with the UCAN-capability check noted as future (Phase 2) work. Makes can() mirror is_attenuated_by's semantics: the stored capability plays the "parent" role, so with == "*" matches any resource and can == "*" or "repo/admin" matches any action. Closes Gitlawb#199
|
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 Run ID: 📒 Files selected for processing (1)
✨ 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
left a comment
There was a problem hiding this comment.
Verified the core is correct by execution: can() is an exact mirror of is_attenuated_by with the stored capability as the parent, so it grants exactly what the delegation chain already accepts. I enumerated the full cap x resource x action matrix (including "*", "repo/admin", and empty strings) and found no case where can() grants beyond is_attenuated_by — only false negatives removed, no new grant. Reverting can() to the old strict equality turns both new tests red, and there's no production caller yet, so this is a correct, dormant fix. One gap before merge.
Findings
-
[P2] Add a test for the new
cap.can == "*"branch
crates/gitlawb-core/src/ucan.rs:198
Of the three match alternatives the fix introduces, onlywith == "*"andrepo/adminare exercised. Dropping just|| cap.can == "*"leaves bothcan_honors_*tests green, so the full-action-wildcard branch has no coverage. On an authorization primitive every match branch should be load-bearing. One test closes it:let ucan = Ucan::issue( &issuer, audience, vec![Capability::new("gitlawb://repos/test/repo", "*")], None, ) .unwrap(); assert!(ucan.can("gitlawb://repos/test/repo", caps::GIT_PUSH)); assert!(ucan.can("gitlawb://repos/test/repo", caps::PR_MERGE)); // two actions via can == "*" assert!(!ucan.can("gitlawb://repos/other/repo", caps::GIT_PUSH)); // wrong resource still denied
-
[P3, pre-existing]
constraints(nb) is unenforced. Bothcan()andis_attenuated_byignore the field, so a capability like{with, can: "repo/admin", nb: {...}}grants broadly regardless of the constraint. Not introduced here, but this PR widens where it matters oncecan()gets wired into the authorization path — worth settling constraint enforcement before that Phase 2 wiring lands. Note, not a blocker. -
[P3, pre-existing]
repo/adminis treated as a universal action wildcard (equal to"*"), so it grantsnetwork/join,agent/deploy, etc. on the resource, not just repo actions. This lives inis_attenuated_by;can()faithfully inherits it. Worth confirming that breadth is intended. Design note.
The retarget of the matching semantics to mirror is_attenuated_by is sound and correctly scoped. The only blocker is the untested can == "*" branch.
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] Add load-bearing coverage for the
can == "*"wildcard branch
crates/gitlawb-core/src/ucan.rs:198
The implementation adds three wildcard alternatives, but the new tests exercise onlywith == "*"andcan == "repo/admin". Removing just|| cap.can == "*"leaves every added test green, so a regression would silently reintroduce false negatives for valid full-action-wildcard UCANs once this method is used for authorization. Add a test with a concrete resource andcan: "*"that proves multiple actions match while the same action remains denied for a different resource.
|
Pushed a fixup addressing beardthelion's and jatmn's shared finding: added can_honors_action_wildcard, a test with a concrete resource and can: "*" that asserts multiple actions match while the same action stays denied for a different resource. All 18 ucan tests pass. |
Dismissing my stale CHANGES_REQUESTED — it was on cd96add, before the contributor pushed the load-bearing can=="*" test. Re-reviewed the current head; approving separately.
beardthelion
left a comment
There was a problem hiding this comment.
The blocker from my last pass is closed. can_honors_action_wildcard is load-bearing: I dropped || cap.can == "*" from can() and it goes red (panic at the pr/merge assertion) while the other 17 ucan tests stay green, then restored and confirmed 18/18. That pins the exact branch jatmn and I flagged. The core is unchanged from my prior review and still correct: can() is a term-for-term mirror of is_attenuated_by with the stored capability in the parent slot, so it only removes false negatives and never grants past a real delegation. I re-checked the requested-side wildcard case too (can("*", ...) against a concrete stored cap denies, matching is_attenuated_by's "wildcards on self carry no meaning").
Confirmed dormant on this head: the only production authz path is verify_chain() at crates/gitlawb-node/src/auth/mod.rs:301; every .can() caller in the tree is test-only. LGTM.
One forward-looking note, not blocking this PR: can() trusts payload.att raw, with no signature/expiry/chain check of its own. That's the right split for a capability-matching primitive, but the wildcard change widens the blast radius if a future caller forgets to validate first (one ("*","repo/admin") cap now answers can(any, any) = true). When Phase 2 wires can() into the auth path, the caller must gate on verify_chain before calling it. Worth a one-line precondition on the doc comment while it's cheap; fine to fold into the Phase 2 change.
@kevincodex1 approved and ready to merge.
Summary
Capability::is_attenuated_by(crates/gitlawb-core/src/ucan.rs:52-57) treatswith: "*"andcan: "*"/"repo/admin"on a parent capability as wildcards during delegation-chain verification, so a valid UCAN can end with a capability like{"with": "*", "can": "git/push"}. ButUcan::candid strict equality on both fields, so that same capability could never match a concrete(resource, action)request at runtime.Closes #199
Why this is a bug fix, not a security fix
The mismatch only produces false negatives: a validly-delegated wildcard capability gets denied, never something unauthorized getting granted. It's also dormant today —
Ucan::canisn't called from any production authorization path yet (auth::caller_authorized_to_pushis puredid_matches(...), with UCAN-capability checking noted as future Phase 2 work). This closes the gap before that wiring lands.What changed
Ucan::cannow mirrorsis_attenuated_by's wildcard semantics from the matching side: the stored capability plays the "parent" role, sowith == "*"matches any resource andcan == "*"or"repo/admin"matches any action.repo/adminaction wildcard.How a reviewer can verify
Kind of change
Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsNo wire-format or signing change —
can()is a local, in-process capability check over an already-verified UCAN's payload. Behavior only changes for capabilities that already contain a literal"*"or"repo/admin", whichis_attenuated_byalready treats as wildcards during chain verification.