fix(api): allow maintainer sessions to reach validate-linked-issue - #736
Conversation
The session-path allowlist (canSessionAccessPath) listed check-before-start but omitted its sibling validate-linked-issue, so a non-operator maintainer using a browser/session token got 403 insufficient_role for their own repo before the handler ran — leaving the route's requireSessionRepoAccess guard dead. Add the path to the allowlist, mirroring check-before-start; the per-route guard still enforces repo scope. Closes JSONbored#734
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
gittensory · advisory review Reviewed 2 changed file(s) — two independent AI reviewers. Changed files: Suggested action: ✅ Safe to merge — both reviewers found no blocking issues. Reviewer A · Suggestions
Worth double-checking
Reviewer B · Worth double-checking
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #736 +/- ##
==========================================
+ Coverage 96.69% 96.72% +0.02%
==========================================
Files 94 94
Lines 13848 13850 +2
Branches 5051 5052 +1
==========================================
+ Hits 13391 13397 +6
+ Misses 88 86 -2
+ Partials 369 367 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JSONbored
left a comment
There was a problem hiding this comment.
PR #736 — fix(api): allow maintainer sessions to reach validate-linked-issue
Verdict: MERGE
Resolves issue #734: FULLY
CI: All green (test 1/2, lint, workers, ui, security). gittensory neutral.
Highlights:
- Two-line mirror of the working sibling exactly as prescribed: adds isRepoValidateLinkedIssuePath predicate + the canSessionAccessPath allowlist entry beside isRepoCheckBeforeStartPath; regex anchored and structurally identical.
- Safe: the handler's pre-existing requireSessionRepoAccess guard still enforces per-repo scope, so this only makes the route reachable, not wider than check-before-start. Fails closed.
- Regression test targets the reported asymmetry: non-operator maintainer of alice/repo-a now gets 200 on own repo (was 403 insufficient_role) and 403 forbidden_repo cross-repo — asserts body and status.
Concerns / required changes:
- None.
Completeness vs issue: Adds the predicate + allowlist entry and the missing positive maintainer-session test; the dead admit-path is now reachable. Fully resolved, no scope creep.
Summary
The coarse session-path allowlist
canSessionAccessPathdecides whether a browser/session identity may even reach a path before its route handler runs. It lists the pre-submission toolcheck-before-startbut omits its siblingvalidate-linked-issue.Both route handlers are symmetric — each authenticates, loads data, then admits a maintainer session via
requireSessionRepoAccess:But because
validate-linked-issuewas missing from the allowlist, the global middleware short-circuited every non-operator session with403 insufficient_rolebefore the handler ran:Effect: a verified maintainer of repo X, using the web UI / a session token, got a spurious
403 insufficient_roleonvalidate-linked-issuefor their own repo X, while the twincheck-before-startworked. The route's ownrequireSessionRepoAccessadmit-path was dead code. Only operators and raw server-to-server tokens (which bypass the session allowlist) could reach it. The MCP equivalent (gittensory_validate_linked_issue) was unaffected. It fails closed — a functionality/usability bug, not a security exposure.The in-code contract is explicit: "A path added here MUST be scoped by a per-route guard in its handler." This route had the guard but was never added to the allowlist — the inverse of the requirement.
Fix
Add the path to the allowlist, mirroring
check-before-startexactly:The handler's existing
requireSessionRepoAccessguard then enforces repo scope, so this exposes nothing a maintainer can't already reach viacheck-before-start.Tests
Added a regression test in
access-boundary.test.ts: a non-operator maintainer ofalice/repo-anow gets200onvalidate-linked-issuefor their own repo (was403 insufficient_role), and403 forbidden_repofor another maintainer'sbob/repo-b— proving the allowlist admits the route while the per-route guard still scopes it.Full coverage suite green locally (only the known CRLF-local
gittensory-focus-manifesttest fails locally; passes in CI).Closes #734