fix(electron): validate IPC sender frame for token-cache and OAuth handlers#9167
Open
dominic-clerk wants to merge 2 commits into
Open
fix(electron): validate IPC sender frame for token-cache and OAuth handlers#9167dominic-clerk wants to merge 2 commits into
dominic-clerk wants to merge 2 commits into
Conversation
…ndlers Token-cache and OAuth-transport IPC handlers accepted requests from any renderer frame. Untrusted content in a subframe or <webview> sharing the Clerk preload could read the persisted client JWT or drive the OAuth transport. Guard both against non-main-frame senders, mirroring the existing passkey handler check via a shared isMainFrameEvent helper.
🦋 Changeset detectedLatest commit: 6610740 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughToken-cache and OAuth IPC handlers now require requests to originate from a window’s main frame. Passkey validation reuses a shared helper, tests cover subframe and webview rejection, and a patch changeset documents the update. ChangesIPC frame-origin security
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RendererFrame
participant IPCHandler
participant isMainFrameEvent
participant TokenStorage
RendererFrame->>IPCHandler: invoke token or OAuth IPC
IPCHandler->>isMainFrameEvent: validate sender frame
isMainFrameEvent-->>IPCHandler: validation result
IPCHandler->>TokenStorage: perform token operation
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/electron/src/main/ipc-handlers.ts (1)
8-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "assert main frame or throw" logic across
ipc-handlers.tsandoauth-transport.ts.Both files re-implement the same check-and-throw wrapper around
isMainFrameEventinstead of sharing one helper fromvalidate-sender.ts. Consolidating avoids the two implementations drifting apart if the rejection behavior ever changes.
packages/electron/src/main/ipc-handlers.ts#L8-L12: move thisassertMainFrameSenderlogic intovalidate-sender.ts(e.g. accepting acontextstring for the message) and import it here instead of defining it locally.packages/electron/src/main/oauth-transport.ts#L95-L106: replace both inlinedif (!isMainFrameEvent(event)) { throw ... }blocks with a call to the same shared assert helper.♻️ Proposed shared helper
// packages/electron/src/main/validate-sender.ts export function isMainFrameEvent(event: IpcMainInvokeEvent): boolean { return Boolean(event.senderFrame) && event.senderFrame === event.sender.mainFrame; } + +export function assertMainFrameSender(event: IpcMainInvokeEvent, context: string): void { + if (!isMainFrameEvent(event)) { + throw new Error(`Clerk: ${context} request did not originate from a window's main frame.`); + } +}// packages/electron/src/main/ipc-handlers.ts -function assertMainFrameSender(event: IpcMainInvokeEvent): void { - if (!isMainFrameEvent(event)) { - throw new Error("Clerk: token-cache request did not originate from a window's main frame."); - } -} +import { assertMainFrameSender } from './validate-sender';(call sites become
assertMainFrameSender(event, 'token-cache'))// packages/electron/src/main/oauth-transport.ts ipcMain.handle(OAUTH_TRANSPORT_CHANNELS.getRedirectUrl, event => { - if (!isMainFrameEvent(event)) { - throw new Error("Clerk: OAuth request did not originate from a window's main frame."); - } + assertMainFrameSender(event, 'OAuth'); return redirectUrl; }); ipcMain.handle(OAUTH_TRANSPORT_CHANNELS.open, async (event, url: string) => { - if (!isMainFrameEvent(event)) { - throw new Error("Clerk: OAuth request did not originate from a window's main frame."); - } + assertMainFrameSender(event, 'OAuth');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/electron/src/main/ipc-handlers.ts` around lines 8 - 12, Consolidate the duplicated main-frame validation by moving assertMainFrameSender into validate-sender.ts with a context parameter for the rejection message. In packages/electron/src/main/ipc-handlers.ts lines 8-12, remove the local implementation and import the shared helper; in packages/electron/src/main/oauth-transport.ts lines 95-106, replace both inline isMainFrameEvent checks with calls to that helper, preserving each request’s context-specific message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/electron/src/main/ipc-handlers.ts`:
- Around line 8-12: Consolidate the duplicated main-frame validation by moving
assertMainFrameSender into validate-sender.ts with a context parameter for the
rejection message. In packages/electron/src/main/ipc-handlers.ts lines 8-12,
remove the local implementation and import the shared helper; in
packages/electron/src/main/oauth-transport.ts lines 95-106, replace both inline
isMainFrameEvent checks with calls to that helper, preserving each request’s
context-specific message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f4eb8873-fd8a-4f60-87ca-856e7f334f00
📒 Files selected for processing (7)
.changeset/fine-results-crash.mdpackages/electron/src/main/__tests__/create-clerk-bridge.test.tspackages/electron/src/main/__tests__/ipc-handlers.test.tspackages/electron/src/main/ipc-handlers.tspackages/electron/src/main/oauth-transport.tspackages/electron/src/main/passkey-handlers.tspackages/electron/src/main/validate-sender.ts
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
dominic-clerk
commented
Jul 15, 2026
isMainFrameEvent only proved the sender was the top document of its own WebContents, so a <webview> (a separate WebContents) sharing the Clerk preload satisfied senderFrame === mainFrame and passed the guard. Require getType() === 'window' to tie privileged IPC to a top-level BrowserWindow, rejecting <webview> guests and BrowserViews. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Good call. Looks good from reading the code. Will run validation e2e to ensure no breakage. Thank you! |
jeremy-clerk
approved these changes
Jul 16, 2026
jeremy-clerk
left a comment
jeremy-clerk
left a comment
Contributor
There was a problem hiding this comment.
👍 want to get @wobsoriano's input as well though
wobsoriano
approved these changes
Jul 16, 2026
wobsoriano
left a comment
wobsoriano
left a comment
Member
There was a problem hiding this comment.
Nice fix and thanks for the test coverage!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Token-cache and OAuth-transport IPC handlers accepted requests from any renderer frame. Untrusted content in a subframe or sharing the Clerk preload could read the persisted client JWT or drive the OAuth transport. Guard both against non-main-frame senders, mirroring the existing passkey handler check via a shared isMainFrameEvent helper.
Fixes SDK-142
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit