feat(desktop): re-land explicit OAuth scopes now ceilings are seeded - #76812
feat(desktop): re-land explicit OAuth scopes now ceilings are seeded#76812charlesvien wants to merge 2 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Prompt To Fix All With AI### Issue 1
products/desktop/packages/shared/src/oauth.ts:230
**Development OAuth rejects privileged scope**
When signing in against the documented local development OAuth application with an empty scope ceiling, the shared scope list requests privileged `llm_gateway:read`, causing `/oauth/authorize` to return `invalid_scope` and preventing developer sign-in. The development application's ceiling needs to be seeded for this scope as part of this change, or the development flow needs a compatible scope list.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(oauth): re-land explicit scopes now..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Privileged: embedded agent model calls go through PostHog's LLM gateway | ||
| // (gateway.{region}.posthog.com), which requires this scope. Not in the | ||
| // advertised set above; granted via this app's seeded ceiling. | ||
| "llm_gateway:read", |
There was a problem hiding this comment.
When signing in against the documented local development OAuth application with an empty scope ceiling, the shared scope list requests privileged llm_gateway:read, causing /oauth/authorize to return invalid_scope and preventing developer sign-in. The development application's ceiling needs to be seeded for this scope as part of this change, or the development flow needs a compatible scope list.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/desktop/packages/shared/src/oauth.ts
Line: 230
Comment:
**Development OAuth rejects privileged scope**
When signing in against the documented local development OAuth application with an empty scope ceiling, the shared scope list requests privileged `llm_gateway:read`, causing `/oauth/authorize` to return `invalid_scope` and preventing developer sign-in. The development application's ceiling needs to be seeded for this scope as part of this change, or the development flow needs a compatible scope list.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Right that privileged scopes fail under an empty ceiling, but the premise doesn't hold for the documented dev app. generate_demo_data creates it with an explicit ceiling of UNPRIVILEGED_SCOPES | {llm_gateway:read, llm_gateway:write, wizard_session:read, wizard_session:write} (products/demo/backend/logic/products/hedgebox/matrix.py:1823), not an empty one, so llm_gateway:read resolves fine locally.
It did surface a real bug one line up though. The list also carried OAUTH_SCOPES_HIDDEN from the generated MCP scope file — batch_import_support:*, query_performance:*, wizard_session:*. Those are subtracted out of UNPRIVILEGED_SCOPES, so @default can never cover them, and validate_scopes rejects the whole request rather than dropping the offenders. Checked against posthog/scopes.py on master:
- 212 scopes vs
["@default", "llm_gateway:read"]: rejected on all six - 212 scopes vs the dev app ceiling: rejected on
batch_import_support:*andquery_performance:* - 206 scopes (hidden ones removed): accepted against both
Removed in a432d28.
There was a problem hiding this comment.
Thanks for the correction — you're right, the dev app ceiling is explicitly seeded in generate_demo_data, not empty, so llm_gateway:read resolves fine there. My premise was wrong.
The hidden scope analysis is solid: batch_import_support, query_performance, and wizard_session are subtracted from UNPRIVILEGED_SCOPES, so @default can never cover them, and OAuthValidator.validate_scopes treats an explicit request as all-or-nothing — one unresolvable scope rejects the entire grant. The table in the PR description confirms it cleanly. The fix in a432d28 is correct.
dmarticus
left a comment
dmarticus
left a comment
There was a problem hiding this comment.
greptile comment seems legit so just get that before you merge.
Request the live scopes_supported set plus llm_gateway:read instead of "*", and bump OAUTH_SCOPE_VERSION 5 → 6 so installs re-authorize. The Jul 22 revert (#3668) was required because #3411 shipped the explicit list before US/EU OAuth app ceilings were seeded. Both Code apps are now seeded to [@default, llm_gateway:read] and *→ceiling narrowing is live, so the client change is unblocked. Regenerated from us.posthog.com scopes_supported (203 scopes, includes loop:* and post-#3411 additions); drop agents:* which left the advertised set. Comment documents the seed-before-release guardrail so it doesn't get bundled again without the ceiling.
5d9625e to
3e1ef67
Compare
batch_import_support, query_performance, and wizard_session are OAUTH_SCOPES_HIDDEN server-side: subtracted out of UNPRIVILEGED_SCOPES, so the app's "@default" ceiling can never cover them. /authorize treats an explicit scope list as all-or-nothing, so requesting them would fail every sign-in with invalid_scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CAPcQaAZNzcWNhCh1nyBPi
|
React Doctor found 50 issues in 13 files · 8 errors & 42 warnings. Errors
42 warnings
Reviewed by React Doctor for commit |
3 participants
Problem
Ports PostHog/code#3974. PostHog Code still requests
scope=*at sign-in, which blocks retiring the wildcard server-side and keeps the desktop token full-privilege.PostHog/code#3411 shipped an explicit scope list and bumped
OAUTH_SCOPE_VERSION, but the US/EU Code OAuth app ceilings were not seeded yet, so fresh sign-ins hitinvalid_scopeand PostHog/code#3668 reverted to*. Seeding and the*-to-ceiling narrowing (#70538) are now live in both regions, so the client change is unblocked.Changes
All under
products/desktop/packages/shared/:OAUTH_SCOPES: 206 scopes — the 205 advertised inscopes_supportedonus.posthog.comandeu.posthog.com(both regions publish identical sets), plus privilegedllm_gateway:readlast.OAUTH_SCOPE_VERSION6 to 7 so existing installs re-authorize onto the narrower set.*client, and never copyOAUTH_SCOPES_HIDDENout of the generated MCP scope file.llm_gateway:readlast, well-formed strings) plus a fingerprint guard.Why the hidden scopes are excluded
services/mcp/src/lib/oauth-scopes.generated.tsexports two lists:OAUTH_SCOPES_SUPPORTEDandOAUTH_SCOPES_HIDDEN(batch_import_support,query_performance,wizard_session). Only the first belongs here. The hidden ones are staff-only,is_staff-gated, and subtracted out ofUNPRIVILEGED_SCOPESinposthog/scopes.py, so a["@default", "llm_gateway:read"]ceiling can never cover them.OAuthValidator.validate_scopestreats an explicit scope list as all-or-nothing, so including even one would fail every sign-in withinvalid_scope.Verified by running the branch's scope list against
posthog/scopes.pyon master:["@default", "llm_gateway:read"]Preconditions (already done): #70538 deployed in US and EU, Code app ceilings seeded to
["@default", "llm_gateway:read"]in both regions, real*sign-in verified to narrow correctly in US.Follow-up worth tracking separately
Pinning an explicit list trades one failure mode for another. A
*request is rewritten to the app's ceiling byvalidate_scopes, so it can never go stale. An explicit list cannot: if a scope is ever renamed or retired server-side, every already-installed build that still pins it fails/authorizewithinvalid_scopeuntil users update. That is not a regression in this PR, but it does mean scope removals become a breaking change for shipped desktop builds, and there is currently no tolerance path at/authorize.How did you test this code?
pnpm --filter @posthog/shared testfromproducts/desktop/: 782 passed, including the oauth fingerprint and structural guards.pnpm --filter @posthog/shared typecheckclean.posthog/scopes.pyon master; itsget_oauth_scopes_supported()reproduces exactly the 205 scopes live on both regions.Automatic notifications
Docs update
N/A
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude ported this from PostHog/code#3974 at @charlesvien's direction using /porting-code-prs, applying the source patch with
git am --directory=products/desktop/so the commit keeps @MattBro's authorship. A follow-up commit removed the six hidden scopes after checking the list against the server's ceiling resolution.