Skip to content

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

Open
teoaliano wants to merge 4 commits into
pingdotgg:mainfrom
teoaliano:fix/legacy-scope-token-exchange
Open

teoaliano wants to merge 4 commits into
pingdotgg:mainfrom
teoaliano:fix/legacy-scope-token-exchange

Conversation

@teoaliano

@teoaliano teoaliano commented Oct 8, 2026 •

Copy link
Copy Markdown

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.
POST /oauth/token → GET /api/auth/session
historical form (retired review:write dropped): main 4 permissions → head 13
limited legacy read request:                  main 1 permission  → head 2
mixed request:                               2 exact permissions, unchanged
granular request:                            1 exact permission, unchanged
omitted scope:                               16 granted permissions, unchanged
empty/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.

Released clients such as iOS 2.0.0 request the pre-split scope names at
token exchange. The exchange kept only those names, so new sessions lost
filesystem:read, providers:manage, source-control:write and the other
permissions split out of them, even when the pairing grant included them.

Expand a request made only of pre-split scopes by the permissions split
out of each requested parent, then intersect with the grant as before.
Granular requests and requests without a scope are unchanged, and stored
credentials are still never widened.

Fixes pingdotgg#16804

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 8, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

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.

@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

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 48b71f0 and c5644b2.

📒 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 name Status Explanation Resolution
Linked Issues check Warning 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 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 check Warning 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… 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 name Status Explanation
Out of Scope Changes check Passed 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 request…
Title check Passed The 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.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
@juliusmarminge juliusmarminge self-assigned this Oct 19, 2026
@juliusmarminge

Copy link
Copy Markdown
Member

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.

@juliusmarminge

Copy link
Copy Markdown
Member

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.

@juliusmarminge

Copy link
Copy Markdown
Member

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.

This branch has not been deployed

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

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants