Skip to content

Fix session fixation attack - #8759

Merged
rkarodia merged 1 commit into
opal-eolfrom
security-fix
Aug 14, 2026
Merged

Fix session fixation attack#8759
rkarodia merged 1 commit into
opal-eolfrom
security-fix

Conversation

@rkarodia

Copy link
Copy Markdown
Contributor

Fix session fixation vulnerability in OAuth connection flow

Summary

Mitigates an OAuth session fixation attack by deferring the token grant request (/connection/grant/) until after the opener window has verified the nonce against its local session state. In addition, migrates the /grant API endpoint from GET to POST with a JSON payload for defense-in-depth.

Key Changes

1. Defer Token Exchange to Opener Tab

  • packages/visual-editor/src/ui/elements/connection/connection-broker.ts:
    • Removed the direct /grant fetch from the popup window.
    • ConnectionBroker now relays the raw authorization code, nonce, redirectPath, scopes, and authuser to window.opener via postMessage.
  • packages/visual-editor/src/ui/utils/oauth-based-opal-shell.ts:
    • In #listenForSignIn, the opener tab verifies that popupMessage.nonce === nonce before making the network request to /connection/grant/.
    • If valid, it sends a POST request with { code, redirect_path } JSON payload to exchange the authorization code for tokens.

2. Migrate /grant Endpoint to POST

  • packages/unified-server/src/connection/server.ts:
    • Changed the /grant route from GET to POST.
    • Registered express.json() middleware.
  • packages/unified-server/src/connection/api/grant.ts:
    • Updated grant() handler to read request parameters from req.body (while maintaining fallback parsing from query parameters).

3. Update OAuth Message Types

  • packages/types/src/oauth.ts:
    • Added the OAuthPopupPayload discriminated union type.
    • Updated OAuthPopupMessage to carry the payload containing the authorization code and nonce (or error).

@github-actions

Copy link
Copy Markdown

📊 Coverage Report

Metric PR Main Delta
Lines 96.55% 96.55% ⚪ +0.00%
Functions 96.98% 96.98% ⚪ +0.00%
Branches 93.15% 93.15% ⚪ +0.00%

@kevinpschaaf kevinpschaaf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good!

I think you want this PR targeting the opal-eol branch though, per Tim's setup.

Let's also get @aomarks and @timswanson-google 's reviews

@rkarodia
rkarodia changed the base branch from main to opal-eol August 11, 2026 17:50
@rkarodia
rkarodia merged commit ed998e6 into opal-eol Aug 14, 2026
6 checks passed
@rkarodia
rkarodia deleted the security-fix branch August 14, 2026 16:00
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.

4 participants