Skip to content

fix(api): allow sessions to reach focus-manifest refresh - #601

Merged
JSONbored merged 3 commits into
JSONbored:mainfrom
galuis116:fix/focus-manifest-refresh-session-access
Jun 12, 2026
Merged

fix(api): allow sessions to reach focus-manifest refresh#601
JSONbored merged 3 commits into
JSONbored:mainfrom
galuis116:fix/focus-manifest-refresh-session-access

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Closes #600.

#523 added POST /v1/repos/:owner/:repo/focus-manifest/refresh, but isRepoFocusManifestPath (used by canSessionAccessPath) was anchored at …/focus-manifest$, so it did not match the /refresh sub-path. Since requiresApiToken treats the refresh path as protected, the blanket * middleware rejected a session-authenticated owner/maintainer with 403 insufficient_role before the route handler ran — even though the handler's requireAppRole + requireSessionRepoAccess would have allowed them. GET/PUT worked for sessions; only the new refresh route was unreachable.

Change

  • Match the optional /refresh sub-path: …/focus-manifest(?:/refresh)?$, so the route handler (not the blanket middleware) gates the refresh route.
  • Add a regression test: a same-repo owner session POSTing …/focus-manifest/refresh gets 200, while a cross-repo owner session still gets 403 forbidden_repo from requireSessionRepoAccess.

Verification

  • routes-focus-manifest.test.ts 12/12 (the new same-repo case fails on the old code with insufficient_role); tsc --noEmit clean; related auth/route suites green.

Same access-path-matcher class as #513 / #508; a new gap specific to the #523 sub-path.

isRepoFocusManifestPath was anchored at /focus-manifest, so the POST /focus-manifest/refresh route added in JSONbored#523 was not covered by canSessionAccessPath -- a session-authenticated owner/maintainer was 403'd by the blanket guard before reaching the handler that would have allowed them. Match the optional /refresh sub-path so the route handler's own requireAppRole + requireSessionRepoAccess gate it.
@ghost

ghost commented Jun 11, 2026

Copy link
Copy Markdown

Note

Gittensory Gate skipped

PR closed before full evaluation. No late first comment was created.

Signal Result Evidence Action
Gate result ⚠️ Skipped #601 is no longer open. No action.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

@ghost ghost added the gittensory:reviewed label Jun 11, 2026
@ghost

ghost commented Jun 11, 2026

Copy link
Copy Markdown

reviewbot · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR expands the focus-manifest route regex to accept an optional '/refresh' suffix and adds a test confirming same-repo owner sessions can POST to that endpoint while cross-repo owners are still forbidden. The change is small, well‑scoped, and includes appropriate test coverage.

Suggestions

  • Add a short comment next to the regex explaining the optional '/refresh' segment for future maintainers.
  • Consider adding a test case for a GET request to '/focus-manifest/refresh' to verify it returns the expected status (e.g., 405 or 404) if such behavior is defined.

Worth double-checking

  • Ensure no other route logic unintentionally treats '/focus-manifest/refresh' as a standard focus-manifest request.
  • Verify that the session gate now correctly distinguishes between same-repo and cross-repo owners for the refresh endpoint.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR extends the focus-manifest path regex to optionally allow a '/refresh' suffix, enabling same-repo owner sessions to POST to the focus-manifest refresh endpoint. A new unit test verifies that the owner gets a 200 response while a cross-repo owner gets a 403, confirming the intended behavior.

Suggestions

  • Consider adding a test for a malformed path like '/focus-manifest/refresh/extra' to ensure it returns 404 (not matched by the route).
  • If the refresh handler has additional logic, ensure it is covered by existing tests; otherwise the new test is sufficient.

Worth double-checking

  • Verify that the updated regex does not unintentionally capture other routes (e.g., paths with extra segments after '/refresh').
  • Confirm that the refresh endpoint's authentication/authorization matches the intended policy (same-repo owner only).

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@ghost

ghost commented Jun 12, 2026

Copy link
Copy Markdown

gittensory · advisory review

Reviewed 2 changed file(s) — two independent AI reviewers.

Suggested action:Safe to merge — both reviewers found no blocking issues.

Reviewer A · gpt-oss-120b — recommends ✅ merge
The PR expands the focus-manifest route regex to optionally include a "/refresh" suffix and adds a test confirming that same‑repo owner sessions can POST to this new endpoint while cross‑repo owners are correctly forbidden. The change is minimal, well‑scoped, and includes coverage.

Suggestions

  • Add a brief comment next to the regex explaining the purpose of the optional "/refresh" segment.
  • Consider updating any route‑generation utilities or documentation to reflect the new "/refresh" endpoint.

Worth double-checking

  • Ensure no other route handlers unintentionally match the new pattern (e.g., future sub‑paths under focus-manifest).
  • Verify that client code constructing URLs for the refresh endpoint uses the correct path.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The PR tightens the focus-manifest path regex to optionally allow a '/refresh' suffix, enabling sessions to hit the focus-manifest refresh endpoint. A new unit test verifies that same-repo owner sessions succeed (200) while cross-repo owner sessions are still rejected (403). The change is small, focused, and appears correct.

Suggestions

  • Consider adding a test case for an invalid path like '/v1/repos/owner/repo/focus-manifest/refresh/extra' to ensure it is not matched.
  • If the refresh handler has its own authorization logic, verify that the test's expectation of 200 aligns with that logic (already looks good).

Worth double-checking

  • Ensure the regex does not inadvertently match unintended paths (e.g., with extra segments).
  • Confirm that the refresh handler exists and returns the expected shape for 200 responses.

@ghost ghost added the gittensory-review label Jun 12, 2026
@dosubot dosubot Bot added the lgtm label Jun 12, 2026
@JSONbored JSONbored added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jun 12, 2026
@JSONbored
JSONbored merged commit 6315277 into JSONbored:main Jun 12, 2026
7 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug]: Focus-manifest POST /focus-manifest/refresh 403s maintainer/owner web sessions — canSessionAccessPath doesn't match the new sub-path

2 participants