Skip to content

fix: allow provider-subtree path-token scopes#56

Closed
khaliqgant wants to merge 1 commit into
mainfrom
fix/path-token-provider-subtree
Closed

fix: allow provider-subtree path-token scopes#56
khaliqgant wants to merge 1 commit into
mainfrom
fix/path-token-provider-subtree

Conversation

@khaliqgant

@khaliqgant khaliqgant commented Jun 19, 2026

Copy link
Copy Markdown
Member

Summary

  • allow path-token scope validation to accept a requested relayfile fs scope that covers all requested paths for the same read/write operation
  • keep whole-tree path-token scopes (*, /, /*, /**) rejected
  • add route coverage for provider-subtree writeback scopes, cross-provider rejection, and whole-tree rejection

Security / Merge Hold

This intentionally widens path-token minting from exact/narrower-than-path scopes to named-subtree scopes that cover the requested paths, e.g. relayfile:fs:write:/linear/** for /linear/issues/123.json. The widening is still bounded by the caller parent grant via scopesWithinGrant and does not permit whole-tree scopes.

Merge is held pending @Human explicit approval of this provider-subtree write tradeoff.

Caller-Side Requirement

This PR does not make global fallback scopes valid. Cloud/relayfile callers must request a specific /<provider>/** scope for path-token mints, or avoid the path-token endpoint for genuine whole-tree needs. Sending relayfile:fs:write:/** will continue to return invalid_scope.

Tests

  • node --test --import tsx packages/server/src/__tests__/tokens-route.test.ts
  • npm --workspace @relayauth/server run typecheck

Closes #55

Review in cubic

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a0450dba-08a3-4173-8abb-7684926dc82b

📥 Commits

Reviewing files that changed from the base of the PR and between dff0bf8 and 56ad69b.

📒 Files selected for processing (2)
  • packages/server/src/__tests__/tokens-route.test.ts
  • packages/server/src/routes/tokens.ts

📝 Walkthrough

Walkthrough

scopeWithinPaths in tokens.ts is updated to extract the read|write operation from an incoming scope via regex, compute operation-specific grants per requested path, and apply revised matchScope containment logic (any-grant match vs. all-grants match), returning false on pattern mismatch or errors. Three new tests cover the success case for covering provider-subtree scopes, rejection of non-covering scopes, and rejection of whole-tree wildcard patterns.

Changes

scopeWithinPaths Fix and Tests

Layer / File(s) Summary
scopeWithinPaths containment logic fix
packages/server/src/routes/tokens.ts
Rewrites scopeWithinPaths to regex-match the relayfile:fs:(read|write): pattern, compute per-path operation-specific grants, and evaluate containment with any-grant vs. all-grants matchScope semantics; returns false on mismatch or matchScope errors.
Provider-subtree scope tests
packages/server/src/__tests__/tokens-route.test.ts
Three new POST /v1/tokens/workspace-path tests: covering subtree scopes are accepted and token paths/accessClaims.scopes are validated; non-covering scopes return 400 invalid_scope; whole-tree wildcards (*, /, /*, /**) each return 400 invalid_scope.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Possibly related PRs

  • AgentWorkforce/relayauth#48: Introduced the POST /v1/tokens/workspace-path endpoint and the original scopeWithinPaths logic that this PR refines.

Poem

🐇 Hoppity-hop through the scope-matching gate,
The wildcard wildcards once caused a sad fate.
Now read/write operations are parsed with care,
Provider subtrees minted — no 400 despair!
The relayfile writeback can push without dread,
Thanks to the grants, all the right paths are read. 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: allow provider-subtree path-token scopes' directly describes the main change: enabling provider-subtree scopes in path-token minting, which is the primary objective of the PR.
Description check ✅ Passed The description comprehensively explains the scope change, security considerations, test requirements, and links to issue #55, all directly related to the changeset.
Linked Issues check ✅ Passed The PR directly addresses #55 by allowing provider-subtree scopes (e.g. relayfile:fs:write:/linear/**) to mint path-tokens when paths fall within that subtree, while rejecting whole-tree scopes and maintaining parent grant boundaries.
Out of Scope Changes check ✅ Passed All changes are within scope: the tokens.ts modification implements the provider-subtree validation logic, and the test additions cover the three required cases (acceptance, cross-provider rejection, whole-tree rejection).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/path-token-provider-subtree

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the scopeWithinPaths function in packages/server/src/routes/tokens.ts to support provider-subtree scopes. It extracts the operation (read/write) from the scope and validates it against the requested paths by checking if the scope matches any path grant, or if all path grants match the scope. Additionally, new test cases have been added in packages/server/src/__tests__/tokens-route.test.ts to verify that provider-subtree scopes covering requested writeback paths are allowed, while non-covering scopes and whole-tree path-token scopes are rejected. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@khaliqgant

Copy link
Copy Markdown
Member Author

Duplicate of #57 (same provider-subtree path-token scope fix; opened 31s apart in the session churn). Consolidating on #57 — relayauth-expert's verified branch (full 364-test suite). My review applies verbatim: bidirectional coverage with .every for cross-provider safety, whole-tree rejected, parent grant intact.

@khaliqgant khaliqgant closed this Jun 19, 2026
@khaliqgant khaliqgant deleted the fix/path-token-provider-subtree branch June 19, 2026 10:46

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 56ad69b3f9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

}
});

return pathGrants.every((grant) => matchScope(grant, [scope]));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject normalized root wildcard scopes

When a requested path-token scope uses duplicated slashes such as relayfile:fs:write://* or relayfile:fs:write:////, normalizePathTokenPath collapses it to /* after the explicit whole-tree checks. This new coverage check then accepts it because /* covers the requested path grant, so callers with a broad parent grant can mint a token scoped to the whole relayfile tree even though *, /, /*, and /** are intended to be rejected.

Useful? React with 👍 / 👎.

@agent-relay-code

Copy link
Copy Markdown

ℹ️ pr-reviewer: review only — no file changes were applied to the PR (nothing to commit after review). The notes below are advisory and were not pushed.

Working tree is clean. The artifact was tracked (not ignored) and I restored it so no stray change gets pushed.

PR #56 Review — fix: allow provider subtree path-token scopes

Summary

This PR relaxes scopeWithinPaths in packages/server/src/routes/tokens.ts so a relayfile:fs:{read,write} scope is accepted not only when it is within (narrower-or-equal to) a requested path, but also when it is a subtree scope that covers every requested path (e.g. requesting path /linear/issues/123.json with scope relayfile:fs:write:/linear/*). Three tests are added covering the allow case, the non-covering reject case, and continued rejection of whole-tree scopes (*, /, /*, /**).

Assessment

The change is correct, minimal, and security-sound:

  • Authorization boundary is untouched. The real escalation guard is scopesWithinGrant(accessScopes.scopes, auth.claims.scopes) at tokens.ts:483, which still enforces that minted scopes never exceed the caller's own grant. scopeWithinPaths is only a self-consistency check between a token's declared paths and its scopes, not the auth gate. No fail-open conversion is introduced.
  • The new .every() branch is correctly bounded (tokens.ts:1506): a subtree scope is allowed only if it covers all requested paths. I traced multi-path and non-covering cases — all reject as expected.
  • Whole-tree scopes remain rejected via normalizePathTokenScopenormalizePathTokenPath (tokens.ts:1429), which returns null for *////*//** before scopeWithinPaths runs; line 1464 short-circuits. Good defense in depth.
  • try/catch still fails closed (returns false) on parser errors.

Verification (against current checkout)

  • npm run typecheck (turbo, all 11 packages): pass
  • packages/server tests: 364 pass, 0 fail, including the three new subtree tests
  • @relayauth/server build: clean

Note: the repo-wide npm test had unrelated failures — the @relayauth/cli build was killed with exit 137 (sandbox OOM) and @relayauth/sdk tests reported "Promise resolution still pending" (event-loop/sandbox flakiness). Neither involves the files this PR touches (packages/server only); both reproduce independent of the diff and are environmental, not regressions from this change. I could not get a fully green repo-wide run in this sandbox due to those resource limits.

Addressed comments

  • No bot or human reviewer comments were present in .workforce/context.json or any .workforce/ metadata. No review/approval is recorded on the PR, so I reviewed in full-edit mode but found nothing mechanical to fix.

Files changed by me

  • None. The PR code is correct as-is; no lint/format/typo/import fixes were warranted. I restored a tsconfig.tsbuildinfo artifact that my local build regenerated so no stray file is pushed.

I am not printing READY: I could not complete the repo's canonical full npm test/turbo build end-to-end in this sandbox (the unrelated cli build was OOM-killed and sdk tests are environmentally flaky), and I cannot confirm all required CI checks are green or that the PR is mergeable — those are post-harness facts cloud will report. The PR change itself is verified green at the package level and, in my judgment, is correct and safe.

@agent-relay-code

Copy link
Copy Markdown

Review: PR #56fix: allow provider subtree path-token scopes

Summary

The PR rewrites scopeWithinPaths in packages/server/src/routes/tokens.ts:1492 so that POST /v1/tokens/workspace-path accepts provider-subtree scopes (e.g. relayfile:fs:write:/linear/*) that cover the requested writeback paths, in addition to the pre-existing narrow case (scopes equal to or under a literal requested path). Three tests are added to lock in the new and existing contracts.

Correctness & safety analysis

I traced the change against matchScope (packages/sdk/typescript/src/scope-matcher.ts:52) and the call site (tokens.ts:1464):

  • Old: for each requested path, build read+write grants and ask "is scope covered by a grant rooted at this literal path?". This only permitted scopes narrower than or equal to a literal path; broader subtree scopes like /linear/* were rejected.
  • New: parse the read/write operation off scope, build per-operation path grants, then accept if either (a) scope is covered by one of the path grants (old direction), or (b) scope covers every requested path grant (the new subtree direction).

This is sound and conservatively scoped:

  • The every(...) branch requires the subtree scope to cover all requested paths, so a /linear/* scope with a mixed ["/linear/a.json","/github/b.json"] request is correctly rejected — no over-broadening.
  • Whole-tree scopes remain rejected. *, /, /*, /** are filtered upstream in normalizePathTokenPath (tokens.ts:1429), so scopeWithinPaths never receives a whole-tree scope; the new every branch can't be reached for them. Verified by the new "continues rejecting whole-tree path-token scopes" test.
  • No fail-open / authorization change. scopeWithinPaths is only a path-relevance validator. The actual authorization boundary is scopesWithinGrant(accessScopes.scopes, auth.claims.scopes) at tokens.ts:483, which is untouched — the issuer still must hold the scopes it mints. The try/catch still returns false on error (fail-closed), unchanged.
  • Empty-paths is impossible here (rejected at tokens.ts:1416), so the vacuous-every edge can't occur.

The single caller is internal to tokens.ts; no other packages consume scopeWithinPaths.

Verification (run the way CI does)

  • tsc --noEmit for @relayauth/server: pass (exit 0)
  • @relayauth/sdk + @relayauth/types typecheck: pass
  • Full server test suite (node --test src/__tests__/*.test.ts): 364 pass / 0 fail, including all 65 in tokens-route.test.ts and the 3 new subtree tests
  • SDK test suite: 150 pass / 0 fail
  • Root tests/agent-token.test.ts: pass

Note: turbo build for the SDK was OOM-killed (exit 137) in this sandbox — a resource limit, not a code issue. I worked around it by building dependency dists individually (memory-capped) so the server/SDK tests could resolve @relayauth/sdk/dist. Type/test steps that matter for this change all pass.

No source edits were needed — the change is correct and tests are green. (I restored packages/types/tsconfig.tsbuildinfo, a tracked build artifact my local build run regenerated, so the working tree is clean.)

Addressed comments

  • No bot or human review comments were present in .workforce/context.json, and there are no comment artifacts in .workforce/. Nothing to address.

Advisory Notes

  • None. The change is minimal, in-scope, and does not warrant any unrelated cleanup.

This PR is correct, in-scope, fully tested, and verified locally. CI status, mergeability, and any required human approval are post-harness facts I can't confirm here, so I'm not declaring it ready for human merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant