fix(node): scope the refs listing by the canonical repo slug - #388
fix(node): scope the refs listing by the canonical repo slug#388beardthelion wants to merge 1 commit into
Conversation
`list_refs` built its `branch_cids` filter from the raw URL path segments while the only production writer stores the canonical `normalize_owner_key(owner_did)/name` slug. `get_repo` resolves both `did:key:zX` and bare `zX` to the same record, so a request in the full DID form passed the read gate and then matched zero rows, returning 200 with an empty ref list. A denial shaped like a success is worse than an error: a caller cannot tell an empty repository from one it queried under a form the table does not carry. Bind the record `authorize_repo_read` already returns and build the filter from it, so the queried resource is the one that was authorized. This is the existing pattern in `api/events.rs` and in this file's own push path. Two regressions, each proven to fail without the line it guards. The first drives both owner-DID forms through `build_router` and asserts they return the same refs, with a second repo whose refs must not appear; widening the query to match any owner turns that half red. The second covers the deny direction, which the endpoint had never had: a private repo answers an anonymous caller with the same 404 a missing repo gets and puts no CID in the body. Without it, removing the read gate outright left the suite green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthrough
ChangesRepository refs lookup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change scopes reference listing to the authorized canonical repository and includes focused coverage plus clean validation checks; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description provides the issue link, motivation, implementation details, test coverage, verification results, and scope. It does not reproduce every template heading or checkbox, but it contains the required information. Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 too large.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
2 participants
Closes #387.
list_refsbuilt itsbranch_cidsfilter from the raw URL path segments while the only production writer stores the canonicalnormalize_owner_key(owner_did)/nameslug.get_reponormalizes the owner before matching, sodid:key:zXand barezXresolve to the same repository and both pass the read gate, but only one of them matches the exactWHERE repo = $1that follows. The full-DID form came back200 {"count":0,"refs":[]}.The fix binds the record
authorize_repo_readalready returns and builds the filter from it, so the resource that gets queried is the one that was authorized.api/events.rsand this file's own push path already do exactly this, so the three now agree.Tests
list_refshad no coverage at all before this, which is why the bug survived since the initial release. Two tests, each confirmed to fail without the line it guards rather than assumed to:list_refs_resolves_both_owner_did_forms_to_the_canonical_slugdrives both owner forms throughbuild_routerand asserts they return the same refs, with a second repository whose refs must not appear. Restoring the raw path slug turns the positive half red; widening the query to match any owner turns the negative half red.list_refs_denies_an_anonymous_caller_on_a_private_repocovers the deny direction. Replacingauthorize_repo_readwith a bareget_repolookup turns it red on its own message. Without this test the first one passes with the read gate removed entirely, since every repository it seeds is public.Full suite 1090 passed, 0 failed.
cargo clippy -D warningsclean,cargo fmt --checkclean, andcargo +1.91 check --all-targets --lockedclean against the pinned MSRV.Scope
Only
list_refschanges. The rawformat!("{owner}/{repo}")shape appears in plenty of other handlers, but everywhere else it feeds aRepoNotFoundmessage or a JSON echo field. As a database query key it occurred once, and this is it.Reader and writer now hold the slug formula in two matching expressions kept in step by a comment. A shared helper would make that hold by construction, which is worth doing across every slug-keyed reader at once rather than piecemeal here.
Summary by CodeRabbit
Bug Fixes
Tests