Skip to content

feat(core): add OAuth authorization-code + PKCE auth mode - #1162

Merged
joshuaellis merged 2 commits into
mainfrom
feat/SDK-2471
Sep 10, 2026
Merged

joshuaellis merged 2 commits into
mainfrom
feat/SDK-2471

Conversation

@joshuaellis

@joshuaellis joshuaellis commented Sep 3, 2026 •

Copy link
Copy Markdown
Member

Description

Adds oauth as a fourth auth mode in packages/core, alongside studio, dashboard, and standalone. Configured via auth.oauth (clientId, redirectUri, organizationId), it uses the OAuth 2.0 authorization-code + PKCE flow and scopes tokens to an organisation via RFC 8707 resource indicators.

Core handles PKCE generation, the authorize redirect, code exchange, token refresh, revocation, and cross-tab token persistence, exposed as bound actions (startOAuthAuthorization, handleOAuthCallback, refreshOAuthTokens, revokeOAuthTokens) and a getOAuthTokensState accessor. handleOAuthCallback is single-flight so a repeated or StrictMode-double-invoked call does not attempt a second exchange of a single-use code.

Fun Gif

gif-by-lakeshore-records

@joshuaellis
joshuaellis requested a review from a team as a code owner September 3, 2026 19:11
@joshuaellis
joshuaellis requested a review from gu-stav September 3, 2026 19:11
@claude

claude Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @joshuaellis's task in 2m 33s —— View job


Answer: CodeQL js/clear-text-storage-of-sensitive-data at oauthActions.ts:101

  • Gather context
  • Understand the request
  • Answer the CodeQL question

Short version: this one is a false positive — dismiss it, don't restructure the code. The genuinely interesting storage in this PR is somewhere else (see the last section).

Why it's a false positive

js/clear-text-storage-of-sensitive-data picks its source by identifier name, then flags any Storage.setItem sink it reaches. oauthState trips the name heuristic; the value on the line above it (codeVerifier, packages/core/src/auth/oauth/oauthActions.ts:100) is the one that actually is a secret and it isn't flagged — which is a good indicator you're looking at a naming match rather than a dataflow finding.

The OAuth state value is public by construction:

  • you put it in the authorize URL query string yourself (oauthActions.ts:110), and the IdP sends it back in the redirect URL query string (oauthActions.ts:149) — so it's already in browser history, the referrer, and potentially server logs;
  • its only requirements are non-guessability (satisfied — crypto.getRandomValues, 32 bytes, pkce.ts:25-29) and that it be readable only by your own origin on callback;
  • sessionStorage is exactly what the OAuth 2.0 for Browser-Based Apps BCP recommends for both state and code_verifier across a full-page redirect. In-memory storage isn't an option here because window.location.assign (oauthActions.ts:117) destroys the document.

There's no "encrypt it" alternative either — any key would have to live in the same origin the attacker already has.

Recommended action
  1. Dismiss alert #11 in the Security tab as False positive, with a one-line note ("OAuth state is a public CSRF nonce transmitted in the authorize/redirect URLs"). There's no codeql workflow or .github/codeql/codeql-config.yml in this repo, so this is default setup — a path/query-filter config isn't available, and inline // codeql[...] / // lgtm[...] comments are not reliably honoured by code scanning default setup. The UI dismissal is the route that actually sticks.
  2. Add a two-line comment above oauthActions.ts:99-101 recording why these two values go to sessionStorage, so the next reader (or the next scanner) doesn't re-litigate it.

If you'd rather the alert never reappear, the only code-level options are to fold both values into one sessionStorage entry under a neutrally-named key, or to derive state from the verifier (base64url(sha256(verifier + ':state'))) and recompute it on callback so only the verifier is stored. Both work, neither improves security — they just move or remove the identifier the heuristic matches. I'd skip them; renaming to satisfy a scanner is worse documentation than a dismissal note.

Where the real exposure is (not flagged by CodeQL)

options.storageArea defaults to localStorage (utils.ts:127-136), so the access token and refresh token are written there in cleartext at oauthActions.ts:222 and oauthActions.ts:312 via serializeTokens. That's the value an XSS on the app origin actually wants: it's long-lived, it survives tab close, and a refresh token is a re-mintable credential. CodeQL didn't flag it because serializeTokens/tokens don't match its name heuristic — the inverse of the line-101 situation.

This is a deliberate trade-off given the design (tokens must survive the redirect, and subscribeToOAuthStorageEvents at oauthAuth.ts:118-135 depends on localStorage for cross-tab sync), so I'm not suggesting you change it in this PR. But it's worth calling out in the auth.oauth TSDoc in packages/core/src/config/authConfig.ts that tokens land in localStorage by default and that apps can pass their own storageArea — app authors with stricter requirements need to know that's the knob. Given your @experimental question elsewhere in the thread, documenting the storage contract now is cheap insurance either way.

Fix this →

No files changed — this was a question, so I answered rather than pushing a commit. Say the word if you want the explanatory comments and the authConfig.ts TSDoc note committed.
· feat/SDK-2471

@joshuaellis
joshuaellis requested review from cngonzalez and ryanbonial and removed request for gu-stav September 3, 2026 19:12
@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

📦 Bundle Stats — @sanity/sdk

Compared against main (4b976d56) · v3.1.0 (npm)

@sanity/sdk

Metric Value vs main (4b976d5) vs v3.1.0
Internal (raw) 306.5 KB +14.8 KB, +5.1% +14.8 KB, +5.1%
Internal (gzip) 75.2 KB +3.4 KB, +4.7% +3.4 KB, +4.7%
Bundled (raw) 1.15 MB +14.8 KB, +1.3% +8.8 KB, +0.8%
Bundled (gzip) 266.3 KB +3.4 KB, +1.3% +2.2 KB, +0.9%
Import time 252ms +2ms, +1.0% +13ms, +5.6%

@sanity/sdk/_internal

Metric Value vs main (4b976d5) vs v3.1.0
Internal (raw) 132.6 KB +4.0 KB, +3.1% +4.0 KB, +3.1%
Internal (gzip) 34.5 KB +813 B, +2.4% +813 B, +2.4%
Bundled (raw) 674.1 KB +4.0 KB, +0.6% -2.1 KB, -0.3%
Bundled (gzip) 152.0 KB +814 B, +0.5% -363 B, -0.2%
Import time 203ms -2ms, -1.0% +11ms, +5.6%

@sanity/sdk/agent

Metric Value vs main (4b976d5) vs v3.1.0
Internal (raw) 62.5 KB +4.0 KB, +6.9% +4.0 KB, +6.9%
Internal (gzip) 16.1 KB +835 B, +5.3% +835 B, +5.3%
Bundled (raw) 416.6 KB +4.0 KB, +1.0% +1.2 KB, +0.3%
Bundled (gzip) 93.1 KB +822 B, +0.9% +497 B, +0.5%
Import time 166ms +0ms, +0.2% +8ms, +5.2%

@sanity/sdk/comlink

Metric Value vs main (4b976d5) vs v3.1.0
Internal (raw) 21.4 KB - -
Internal (gzip) 6.2 KB - -
Bundled (raw) 248.1 KB - -3.3 KB, -1.3%
Bundled (gzip) 59.6 KB - -769 B, -1.2%
Import time 179ms +1ms, +0.4% +8ms, +4.5%

🗺️ . · ./_internal · ./agent · ./comlink · Artifacts

Details
  • Import time regressions over 10% are flagged with ⚠️
  • Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.

📦 Bundle Stats — @sanity/sdk-react

Compared against main (4b976d56) · v3.1.0 (npm)

@sanity/sdk-react

Metric Value vs main (4b976d5) vs v3.1.0
Internal (raw) 173.5 KB - -
Internal (gzip) 45.6 KB - -
Bundled (raw) 1.43 MB +14.9 KB, +1.0% +8.8 KB, +0.6%
Bundled (gzip) 340.8 KB +3.4 KB, +1.0% +2.2 KB, +0.7%
Import time 295ms +2ms, +0.7% +15ms, +5.5%

@sanity/sdk-react/dashboard

Metric Value vs main (4b976d5) vs v3.1.0
Internal (raw) 23.5 KB - -
Internal (gzip) 7.2 KB - -
Bundled (raw) 1.14 MB +4.3 KB, +0.4% -1.8 KB, -0.2%
Bundled (gzip) 265.2 KB +913 B, +0.3% -265 B, -0.1%
Import time 260ms -1ms, -0.4% +14ms, +5.7%

🗺️ . · ./dashboard · Artifacts

Details
  • Import time regressions over 10% are flagged with ⚠️
  • Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

📚 TypeDoc Generation Result

✅ TypeDoc generated successfully!

  • JSON file size: 2.8M
  • Total exports: 401
  • Artifact: typedoc-html-pr-1162
  • HTML docs preview: Download artifact

The package TypeDoc JSON files and combined HTML site have been generated successfully.

@joshuaellis

Copy link
Copy Markdown
Member Author

i think the refresh logic will go away and the client will handle it but to be safe and because code is cheap, its added here.

Although if I do delete it, it'll be a BC to the SDK right? so maybe I should just mark it as @experimental? 😅

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 97.13% (🎯 94%) 5593 / 5758
🟢 Statements 95.81% (🎯 93%) 6072 / 6337
🟢 Functions 97.15% (🎯 94%) 1637 / 1685
🟢 Branches 89.68% (🎯 86%) 3279 / 3656
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
packages/core/src/auth/authMode.ts 100% 100% 100% 100%
packages/core/src/auth/authStore.ts 95.83% 81.81% 100% 95.83% 331-335
packages/core/src/auth/authStrategy.ts 0% 0% 0% 0%
packages/core/src/auth/logout.ts 100% 91.66% 100% 100%
packages/core/src/auth/oauth/oauthActions.ts 100% 94.73% 100% 100%
packages/core/src/auth/oauth/oauthAuth.ts 97.5% 97.43% 88.88% 97.43% 124
packages/core/src/auth/oauth/pkce.ts 100% 100% 100% 100%
packages/core/src/auth/oauth/types.ts 0% 0% 0% 0%
packages/core/src/config/authConfig.ts 0% 0% 0% 0%
Generated in workflow #5927 for commit 74a6db6 by the Vitest Coverage Report Action

@github-actions

github-actions Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Bundle Stats

Warning

2 significant changes.

@sanity/sdk

🔴 @sanity/sdk (export)
Gzip: 258.3 KB, up 3.6 KB (1.4%)
Raw: 1.13 MB, up 15.5 KB (1.4%)
Import: 257 ms, up 1 ms (0.2%)

@sanity/sdk-react

🔴 @sanity/sdk-react (export)
Gzip: 328.6 KB, up 3.7 KB (1.1%)
Raw: 1.41 MB, up 15.6 KB (1.1%)
Import: 301 ms, down 2 ms (0.8%)

All scenario measurements (8)

🗺️ @sanity/sdk / @sanity/sdk · @sanity/sdk / @sanity/sdk/_internal · @sanity/sdk / @sanity/sdk/agent · @sanity/sdk / @sanity/sdk/comlink · @sanity/sdk / @sanity/sdk/dashboard · @sanity/sdk-react / @sanity/sdk-react · @sanity/sdk-react / @sanity/sdk-react/dashboard · @sanity/sdk-react / @sanity/sdk-react/dashboard/_internal · Artifacts

Scenario Kind Bundle (raw / gzip) Gzip change Import time Import change
🔴 @sanity/sdk / @sanity/sdk export 1.13 MB / 258.3 KB +3.6 KB, +1.4% 257 ms +1 ms, +0.2%
⚪ @sanity/sdk / @sanity/sdk/_internal export 587.6 KB / 133.9 KB +857 B, +0.6% 190 ms -2 ms, -0.8%
⚪ @sanity/sdk / @sanity/sdk/agent export 414.5 KB / 90.9 KB +805 B, +0.9% 172 ms +1 ms, +0.5%
⚪ @sanity/sdk / @sanity/sdk/comlink export 257.3 KB / 59.8 KB None 186 ms +1 ms, +0.3%
⚪ @sanity/sdk / @sanity/sdk/dashboard export 45.8 KB / 10.7 KB None 72 ms +1 ms, +1.5%
🔴 @sanity/sdk-react / @sanity/sdk-react export 1.41 MB / 328.6 KB +3.7 KB, +1.1% 301 ms -2 ms, -0.8%
⚪ @sanity/sdk-react / @sanity/sdk-react/dashboard export 651.6 KB / 148.8 KB +850 B, +0.6% 273 ms -3 ms, -0.9%
⚪ @sanity/sdk-react / @sanity/sdk-react/dashboard/_internal export 50.5 KB / 11.0 KB None 189 ms -1 ms, -0.5%

Significant means at least 1.0 KB and 1% gzip, or at least 5 ms and 10% import time.

Comment thread packages/core/src/auth/oauth/oauthActions.ts Dismissed
)

// Single-flight refresh shared across concurrent callers. Safe as a module
// singleton because `authStore` is a global store (one shared state).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Heads up, this is really one shared state per tab, so it could be possible (although unlikely) for two tabs with the same app loaded to race for a token refresh.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤔 good point. I wonder how other oauth clients handle this.

Comment thread packages/core/src/auth/oauth/oauthActions.ts
Comment thread packages/core/src/auth/oauth/oauthActions.ts
@joshuaellis
joshuaellis merged commit 1513fbf into main Sep 10, 2026
23 of 24 checks passed
@squiggler-app squiggler-app Bot mentioned this pull request Sep 9, 2026
@joshuaellis
joshuaellis deleted the feat/SDK-2471 branch September 14, 2026 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants