fix(server): older clients get split permissions when they pair#17341

open4
main
GitHub fork: fix/legacy-scope-token-exchange
opened26d agobyTteoaliano
View on GitHubTwo-way sync enabledPR last imported 8d ago

Released clients can request only the legacy permission vocabulary when exchanging a pairing token, leaving the new session without the corresponding split permissions. Expand explicit legacy-only requests through the existing parent mapping, then intersect with the pairing grant. Mixed and granular requests keep their exact scope; stored credentials are unchanged.

Based on teoaliano’s #17341, with original commits retained and main merged. Addresses the token-exchange portion of #16804; the other provider/UI concerns remain outside this change.

Historical requests for orchestration:read orchestration:operate terminal:operate relay:read receive those four permissions plus nine mapped children when granted: 13, excluding unrequested access:read, access:write, and relay:write. Omitted scopes retain the full grant, currently 16 standard permissions. A legacy parent can authorize its granted split child even if the grant omits the parent itself; that successful exchange consumes the pairing link normally.

Verification on f5b83e0f66:

  • Replaced the two original regressions with one two-row behavior table, keeping 23 auth cases (23 before takeover). Both fail on main by their permission assertions. Historical requests assert the exact 13 public session permissions; a limited read grant returns only read and filesystem-read, excluding unrequested granted permissions.
  • Five focused suites pass, 95 tests total: EnvironmentAuth, contracts auth, auth HTTP, PairingGrantStore, and SessionStore. Scoped server/contracts typechecks, targeted lint, formatting, and whitespace checks pass.
  • Actual Linux loopback HTTP exercised production token exchange and bearer session routes with isolated SQLite and temporary secrets. Main comparison substitutes only the two changed production modules. No live T3 state was used.
text
1POST /oauth/token → GET /api/auth/session
2historical form (retired review:write dropped): main 4 permissions → head 13
3limited legacy read request: main 1 permission → head 2
4mixed request: 2 exact permissions, unchanged
5granular request: 1 exact permission, unchanged
6omitted scope: 16 granted permissions, unchanged
7empty/retired-only request: HTTP 400, pairing link preserved

One full independent review round (Claude Opus 5.5 and GPT-6-Sol) found no code blockers. Human authorization review remains required before merge.

This is headless Linux auth-route/service proof. An actual old iOS binary, relay/tunnel transport, macOS, Windows, iOS, Android, and Electron desktop were not verified. No visual UI change; screenshots do not apply. Leave unmerged.

Tests: GPT-6-Astra; implementation: GPT-6.1-Sol; adversarial reviews: Claude Opus 5.5 and GPT-6-Sol. T3 Code using the Codex and Claude harnesses.

Checks

22 successful · 6 skipped · 1 neutral
All checks passed
f5b83e0

Added 1 commit

  • fix(server): older clients get split permissions when they pairc5644b2
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This compatibility fix changes authentication token exchange so legacy scope requests can produce additional granular permissions on newly issued sessions. Because it modifies sensitive authentication and authorization behavior, human review is required despite the focused scope and added tests.

You can add or adjust custom eligibility rules. Learn more.

Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d01411c5-362c-469d-af4e-340b367eb784
📥 Commits

Reviewing files that changed from the base of the PR and between 48b71f0eb323d08c8fcadfe54adf34becb2984da and c5644b218832e6422c13a666840680bbbb31d788.

📒 Files selected for processing (4)
  • apps/server/src/auth/EnvironmentAuth.test.ts
  • apps/server/src/auth/EnvironmentAuth.ts
  • docs/internals/environment-auth.md
  • packages/contracts/src/auth.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Bootstrap token exchange now expands explicitly supplied requests that contain only legacy scopes before resolving the grant. The expansion remains limited to granted scopes. Tests and internal documentation cover this behavior.

Changes: Bootstrap scope compatibility

Layer / File(s)Summary
Define legacy scope expansion
packages/contracts/src/auth.ts
Adds expandLegacyScopeRequest for requests that contain only legacy scopes. Clarifies how clients and servers use parent-scope checks.
Apply and verify expansion
apps/server/src/auth/EnvironmentAuth.ts, apps/server/src/auth/EnvironmentAuth.test.ts, docs/internals/environment-auth.md
Bootstrap exchange expands explicit scope requests before grant resolution and filtering. Tests cover legacy requests, pairing-grant limits, and filesystem:read expectations. Documentation describes the expansion and the failure when no requested scopes are granted.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to c5644

Older clients that request only pre-split scopes now receive the permissions split out of them, up to what the pairing grant allows. Nothing in the supplied change shows a regression or a broader grant, so it looks safe to merge. A real iOS pairing against an updated server was not tested.

### Architecture Summary

Architecture risk: 🟡 Medium · up to c5644

The change affects 3 systems.

Changed systems: packages/contracts, apps/server, docs

Architecture concerns No architecture-level concerns identified.

Review details

Systems and components

  • observed — packages/contracts (library) was modified; 1 changed file maps to changed impact.
  • observed — apps/server (service) was modified; 2 changed files map to changed impact.
  • observed — docs (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/auth/EnvironmentAuth.test.ts: The reusable dev-token exchange assertions now request and expect filesystem:read for bearer and DPoP access tokens instead of orchestration:read; the exchanges and token-type checks remain.
  • observed — Modified behavior in apps/server/src/auth/EnvironmentAuth.test.ts: The pairing-credential exchange test now requests and verifies filesystem:read instead of orchestration:read, including the requested scope on the follow-up reuse attempt.
  • observed — Modified behavior in apps/server/src/auth/EnvironmentAuth.test.ts: Adds coverage that a request using the pre-split scope vocabulary receives all standard client scopes after exchange.
  • observed — Modified behavior in apps/server/src/auth/EnvironmentAuth.test.ts: Adds coverage that a pairing grant containing orchestration:read, orchestration:operate, and filesystem:read limits a request for the two orchestration scopes to those granted scopes.

Reliability and maintainability

  • inferred — Risk-relevant change factors for packages/contracts: blast_radius_1; direct_dependents_1
Pre-merge checks: Passed 2 · Failed 2

Failed Failed checks: 2 warnings

Check nameStatusExplanationResolution
Linked Issues checkWarningIssue #16804 requires provider-management clients to receive providers:manage and use provider-auth subscription, while restricted clients must see an explicit missing-permission UI state. The chang…Implement the remaining #16804 coding requirements, including the restricted-client missing-permission UI behavior and provider-auth subscription coverage, or separate this server compatibility change from the full #16804 fix and track the …
Description checkWarningThe description clearly explains the problem, implementation, scope, behavior changes, verification results, limitations, and agent usage. However, the required Scope and approval information does not…Add a link to the triaged issue or discussion containing explicit maintainer approval of the direction and scope, including the approval comment. If no approval is required, explain why this focused fix qualifies for the repository exemptio…
Passed Passed checks: 2 passed
Check nameStatusExplanation
Out of Scope Changes checkPassedThe changed server code, contract helper, documentation, and tests directly support the legacy-scope token-exchange compatibility rule from #16804. The expansion remains limited to legacy-only request…
Title checkPassedThe title clearly identifies the server fix that gives older clients split permissions during pairing.
Full details: Out of Scope Changes check

Explanation

The changed server code, contract helper, documentation, and tests directly support the legacy-scope token-exchange compatibility rule from #16804. The expansion remains limited to legacy-only requests and remains capped by the pairing grant. No unrelated change is established.

Full details: Linked Issues check

Explanation

Issue #16804 requires provider-management clients to receive providers:manage and use provider-auth subscription, while restricted clients must see an explicit missing-permission UI state. The change expands legacy requests and preserves the pairing-grant cap. The reported tests cover token exchange and bearer authorization, but the change does not implement the restricted-client UI state or establish provider-auth subscription coverage. The issue's full coding requirements are therefore not met.

Resolution

Implement the remaining #16804 coding requirements, including the restricted-client missing-permission UI behavior and provider-auth subscription coverage, or separate this server compatibility change from the full #16804 fix and track the remaining work in a linked issue.

Full details: Description check

Explanation

The description clearly explains the problem, implementation, scope, behavior changes, verification results, limitations, and agent usage. However, the required Scope and approval information does not include an explicit maintainer approval comment or a clear explanation that this focused fix qualifies for an exemption.

Resolution

Add a link to the triaged issue or discussion containing explicit maintainer approval of the direction and scope, including the approval comment. If no approval is required, explain why this focused fix qualifies for the repository exemption. Organize the content under the required Problem, Change, Scope and approval, and Verification headings if possible.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Added 3 commits

  • Merge remote-tracking branch 'origin/main' into batch/ssh-relay-remote-03/17341f3ef102
  • test(auth): verify legacy scope exchange permissions07f24a5
  • refactor(auth): clarify legacy token exchange boundariesf5b83e0

Taking over this one. I’m retaining @teoaliano’s legacy-request translation and original commits, merging main, and verifying the exact public permissions against existing grants. The historical request now receives 13 mapped permissions, without the three unrequested management scopes. Independent reviews and isolated HTTP proof are included; leaving it unmerged for authorization review.

CI follow-up on f5b83e0f66: lint, build/typecheck, all server shards, web and release smoke passed. Generic Test stopped before tests because the Electron download could not resolve release-assets.githubusercontent.com (curl exit 6). Retrying failed jobs once on the same head; no product or CI changes. Macroscope correctness passed; its neutral approvability result still requires human authorization review.

Takeover complete at f5b83e0f66. Current-head build, lint, and test jobs pass; skipped jobs remain skipped. No unresolved inline findings. One full independent review round with Claude Opus 5.5 and GPT-6-Sol found no code blockers. The PR body contains the current focused tests, headless proof, original-author credit, and platform limits.

The unchanged-head rerun passed after the Electron download DNS failure. Macroscope correctness passed; its neutral approvability result explicitly requires human authorization review. Leaving unmerged.