Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions apps/sim/app/(auth)/auth-redirect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,20 @@ export function resolveAuthRedirect({ redirect, callbackUrl, inviteFlow }: AuthR
}
}

/** OAuth reauthentication must reach the form even when an older session cookie exists. */
/**
* OAuth reauthentication must reach the form even when an older session cookie exists.
* Only an OAuth client's request qualifies: a workspace account launch on the same path
* would otherwise turn a cross-site link into a same-origin request after sign-in.
*/
export function isOAuthAuthorizationCallback(callbackUrl: string, origin: string): boolean {
if (!callbackUrl) return false
try {
const callback = new URL(callbackUrl, origin)
return callback.origin === origin && callback.pathname === '/api/auth/oauth2/authorize'
return (
callback.origin === origin &&
callback.pathname === '/api/auth/oauth2/authorize' &&
callback.searchParams.has('client_id')
)
} catch {
return false
}
Expand Down
41 changes: 38 additions & 3 deletions apps/sim/app/api/auth/oauth2/authorize/route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@sim/testing'
import { oauthUtilsMock, oauthUtilsMockFns } from '@sim/testing/mocks/oauth-utils.mock'
import { urlsMockFns } from '@sim/testing/mocks/urls.mock'
import type { NextRequest } from 'next/server'
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
import { ForbiddenOperationError } from '@/lib/core/application/forbidden'
import { OrchestrationError } from '@/lib/core/orchestration/types'
Expand Down Expand Up @@ -62,17 +63,19 @@ vi.mock('@/lib/oauth/quickbooks-state', () => ({
createQuickBooksOAuthState: mocks.createQuickBooksState,
}))

import { GET } from '@/app/api/auth/oauth2/authorize/route'
import { GET as routeGET } from '@/app/api/auth/oauth2/authorize/route'

const GET = (req: NextRequest) => routeGET(req, { params: Promise.resolve({}) })
Comment thread
waleedlatif1 marked this conversation as resolved.
Resolved

const { mockGetPerRequestOAuthLinkScopes, mockGetCanonicalScopesForProvider } = oauthUtilsMockFns

const BASE_URL = 'https://sim.test'
const WORKSPACE_ID = '11111111-2222-4333-8444-555555555555'

function request(query: Record<string, string>) {
function request(query: Record<string, string>, headers: Record<string, string> = {}) {
const url = new URL('/api/auth/oauth2/authorize', BASE_URL)
for (const [key, value] of Object.entries(query)) url.searchParams.set(key, value)
return createMockRequest('GET', undefined, {}, url.toString())
return createMockRequest('GET', undefined, headers, url.toString())
}

function linkResponse(url = 'https://provider.example/authorize') {
Expand Down Expand Up @@ -439,6 +442,38 @@ describe('OAuth2 authorize route', () => {
expect(mocks.betterAuthGET).not.toHaveBeenCalled()
})

it('refuses a cross-site workspace launch before creating a connection draft', async () => {
const response = await GET(
request(
{ providerId: 'google-email', workspaceId: WORKSPACE_ID, credentialId: 'credential-1' },
{ 'sec-fetch-site': 'cross-site' }
)
)

expect(response.headers.get('location')).toBe(`${BASE_URL}/home?error=oauth_link_invalid`)
expect(mocks.createConnection).not.toHaveBeenCalled()
expect(mocks.linkAccount).not.toHaveBeenCalled()
})

it.each([{ 'sec-fetch-site': 'none' }, { 'sec-fetch-site': 'same-site' }])(
'starts a workspace launch that is not cross-site: %o',
async (headers) => {
const response = await GET(
request({ providerId: 'google-email', workspaceId: WORKSPACE_ID }, headers)
)

expect(mocks.createConnection).toHaveBeenCalledOnce()
expect(response.headers.get('location')).toBe('https://provider.example/authorize')
}
)

it('starts a cross-site launch of the signed-in user’s own connection draft', async () => {
const response = await GET(request({ draftId: 'draft-1' }, { 'sec-fetch-site': 'cross-site' }))

expect(mocks.launchConnection).toHaveBeenCalledOnce()
expect(response.headers.get('location')).toBe('https://provider.example/authorize')
})

it('requires a configured OAuth client before creating a legacy draft', async () => {
mocks.requireClient.mockImplementationOnce(() => {
throw new Error('OAuth client is not configured')
Expand Down
7 changes: 7 additions & 0 deletions apps/sim/app/api/auth/oauth2/authorize/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { ForbiddenOperationError } from '@/lib/core/application/forbidden'
import { requireConfiguredOAuthClient } from '@/lib/core/config/env-capabilities.server'
import { isAuthDisabled } from '@/lib/core/config/env-flags'
import { OrchestrationError } from '@/lib/core/orchestration/types'
import { isCrossSiteSessionRequest } from '@/lib/core/security/same-origin'
import { getBaseUrl } from '@/lib/core/utils/urls'
import { isSameOrigin } from '@/lib/core/utils/validation'
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
Expand Down Expand Up @@ -257,6 +258,12 @@ export const GET = withRouteHandler(async (request: NextRequest) => {

if (!fromConnectionDraft) {
if (!workspaceId) throw new Error('Workspace OAuth launch is missing its owner')
// The query alone picks the workspace and credential, so a cross-site page
// must not be able to start this launch with the viewer's session cookie.
if (isCrossSiteSessionRequest(request)) {
logger.warn('Rejected cross-site OAuth workspace launch', { userId, providerId })
return NextResponse.redirect(`${baseUrl}${APP_ENTRY_PATH}?error=oauth_link_invalid`)
Comment thread
waleedlatif1 marked this conversation as resolved.
Resolved
}
try {
const connection = await createCredentialConnection.execute({
principal,
Expand Down
24 changes: 24 additions & 0 deletions apps/sim/proxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,3 +280,27 @@ describe('proxy matcher', () => {
expect(matches('/api/internal/file-doc/seed')).toBe(true)
})
})

describe('signed-in login with an OAuth authorize callback', () => {
function signedInLogin(callbackUrl: string) {
const url = new URL('https://app.sim.test/login')
url.searchParams.set('callbackUrl', callbackUrl)
return new NextRequest(url, {
headers: { cookie: 'better-auth.session_token=session-token.signature' },
})
}

it('shows the form so an OAuth client can reauthenticate its user', async () => {
const response = await proxy(
signedInLogin('/api/auth/oauth2/authorize?client_id=sim-cli&response_type=code')
)
expect(response.headers.get('location')).toBeNull()
})

it('sends a workspace account launch to the app instead of through sign-in', async () => {
const response = await proxy(
signedInLogin('/api/auth/oauth2/authorize?providerId=google-email&workspaceId=workspace-1')
)
expect(response.headers.get('location')).toBe('https://app.sim.test/home')
})
})
2 changes: 1 addition & 1 deletion scripts/check-test-types.baseline.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
},
"apps/sim/app/api/auth/oauth2/authorize/route.test.ts": {
"TS2345": 2,
"TS2554": 22
"TS2554": 1
},
"apps/sim/app/api/auth/oauth2/callback/quickbooks/route.test.ts": {
"TS2554": 3
Expand Down
Loading