Skip to content

Commit 66ce673

Browse files
waleedlatif1claude
andauthored
fix(security): harden auth, SSRF, injection, and CORS across API routes (#3792)
* fix: prevent auth bypass via user-controlled context query param in file serve The /api/files/serve endpoint trusted a user-supplied `context` query parameter to skip authentication. An attacker could append `?context=profile-pictures` to any file URL and download files without auth. Now the public access gate checks the key prefix instead of the query param, and `og-images/` is added to `inferContextFromKey`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use randomized heredoc delimiter in SSH execute-script route Prevents accidental heredoc termination if script content contains the delimiter string on its own line. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: escape workingDirectory in SSH execute-command route Use escapeShellArg() with single quotes for the workingDirectory parameter, consistent with all other SSH routes (execute-script, create-directory, delete-file, move-rename). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: harden chat/form deployment auth (OTP brute-force, CSPRNG, HMAC tokens) - Add brute-force protection to OTP verification with attempt tracking (CWE-307) - Replace Math.random() with crypto.randomInt() for OTP generation (CWE-338) - Replace unsigned Base64 auth tokens with HMAC-SHA256 signed tokens (CWE-327) - Use shared isEmailAllowed utility in OTP route instead of inline duplicate - Simplify Redis OTP update to single KEEPTTL call Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: harden SSRF protections and input validation across API routes Add DNS-based SSRF validation for MCP server URLs, secure OIDC discovery with IP-pinned fetch, strengthen OTP/chat/form input validation, sanitize 1Password vault parameters, and tighten deployment security checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * lint * fix(file-serve): remove user-controlled context param from authenticated path The `?context` query param was still being passed to `handleCloudProxy` in the authenticated code path, allowing any logged-in user to spoof context as `profile-pictures` and bypass ownership checks in `verifyFileAccess`. Now always use `inferContextFromKey` from the server-controlled key prefix. * fix: handle legacy OTP format in decodeOTPValue for deploy-time compat Add guard for OTP values without colon separator (pre-deploy format) to avoid misparse that would lock out users with in-flight OTPs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(mcp): distinguish DNS resolution failures from SSRF policy blocks DNS lookup failures now throw McpDnsResolutionError (502) instead of McpSsrfError (403), so transient DNS hiccups surface as retryable upstream errors rather than confusing permission rejections. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: make OTP attempt counting atomic to prevent TOCTOU race Redis path: use Lua script for atomic read-increment-conditional-delete. DB path: use optimistic locking (UPDATE WHERE value = currentValue) with re-read fallback on conflict. Prevents concurrent wrong guesses from each counting as a single attempt. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: check attempt count before OTP comparison to prevent bypass Reject OTPs that have already reached max failed attempts before comparing the code, closing a race window where a correct guess could bypass brute-force protection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: validate OIDC discovered endpoints against SSRF The discovery URL itself was SSRF-validated, but endpoint URLs returned in the discovery document (tokenEndpoint, userInfoEndpoint, jwksEndpoint) were stored without validation. A malicious OIDC issuer on a public IP could return internal network URLs in the discovery response. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove duplicate OIDC endpoint SSRF validation block Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: validate OIDC discovered endpoints and pin DNS for 1Password Connect - SSRF-validate all endpoint URLs returned by OIDC discovery documents before storing them (authorization, token, userinfo, jwks endpoints) - Pin DNS resolution in 1Password Connect requests using secureFetchWithPinnedIP to prevent TOCTOU DNS rebinding attacks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * lint * fix: replace KEEPTTL with TTL+EX for Redis <6.0 compat, add DB retry loop - Lua script now reads TTL and uses SET...EX instead of KEEPTTL - DB optimistic locking now retries up to 3 times on conflict Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review feedback on OTP atomicity and 1Password fetch - Replace Redis KEEPTTL with TTL+SET EX for Redis <6.0 compatibility - Add retry loop to DB optimistic lock path so concurrent OTP attempts are actually counted instead of silently dropped - Remove unreachable fallback fetch in 1Password Connect; make validateConnectServerUrl return non-nullable string Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: treat Lua nil return as locked when OTP key is missing When the Redis key is deleted/expired between getOTP and incrementOTPAttempts, the Lua script returns nil. Handle this as 'locked' instead of silently treating it as 'incremented'. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: handle Lua nil as locked OTP and add SSRF check to MCP env resolution - Treat Redis Lua nil return (expired/deleted key) as 'locked' instead of silently treating it as a successful increment - Add validateMcpServerSsrf to MCP service resolveConfigEnvVars so env-var URLs are SSRF-validated after resolution at execution time Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: narrow resolvedIP type guard instead of non-null assertion Replace urlValidation.resolvedIP! with proper type narrowing by adding !urlValidation.resolvedIP to the guard clause, so TypeScript can infer the string type without a fragile assertion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bind auth tokens to deployment password for immediate revocation Include a SHA-256 hash of the encrypted password in the HMAC-signed token payload. Changing the deployment password now immediately invalidates all existing auth cookies, restoring the pre-HMAC behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bind auth tokens to deployment password and remove resolvedIP non-null assertion - Include SHA-256 hash of encryptedPassword in HMAC token payload so changing a deployment's password immediately invalidates all sessions - Pass encryptedPassword through setChatAuthCookie/setFormAuthCookie and validateAuthToken at all call sites - Replace non-null assertion on resolvedIP with proper narrowing guard Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: update test assertions for new encryptedPassword parameter Tests now expect the encryptedPassword arg passed to validateAuthToken and setDeploymentAuthCookie after the password-binding change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: format long lines in chat/form test assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: pass encryptedPassword through OTP route cookie generation Select chat.password in PUT handler DB query and pass it to setChatAuthCookie so OTP-issued tokens include the correct password slot for subsequent validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent f37e4b6 commit 66ce673

File tree

29 files changed

+924
-340
lines changed

29 files changed

+924
-340
lines changed

apps/sim/app/api/auth/sso/register/route.ts

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@ import { z } from 'zod'
44
import { auth, getSession } from '@/lib/auth'
55
import { hasSSOAccess } from '@/lib/billing'
66
import { env } from '@/lib/core/config/env'
7+
import {
8+
secureFetchWithPinnedIP,
9+
validateUrlWithDNS,
10+
} from '@/lib/core/security/input-validation.server'
711
import { REDACTED_MARKER } from '@/lib/core/security/redaction'
812

913
const logger = createLogger('SSORegisterRoute')
@@ -156,24 +160,66 @@ export async function POST(request: NextRequest) {
156160
hasJwksEndpoint: !!oidcConfig.jwksEndpoint,
157161
})
158162

159-
const discoveryResponse = await fetch(discoveryUrl, {
160-
headers: { Accept: 'application/json' },
161-
})
163+
const urlValidation = await validateUrlWithDNS(discoveryUrl, 'OIDC discovery URL')
164+
if (!urlValidation.isValid || !urlValidation.resolvedIP) {
165+
logger.warn('OIDC discovery URL failed SSRF validation', {
166+
discoveryUrl,
167+
error: urlValidation.error,
168+
})
169+
return NextResponse.json(
170+
{ error: urlValidation.error ?? 'SSRF validation failed' },
171+
{ status: 400 }
172+
)
173+
}
174+
175+
const discoveryResponse = await secureFetchWithPinnedIP(
176+
discoveryUrl,
177+
urlValidation.resolvedIP,
178+
{
179+
headers: { Accept: 'application/json' },
180+
}
181+
)
162182

163183
if (!discoveryResponse.ok) {
164184
logger.error('Failed to fetch OIDC discovery document', {
165185
status: discoveryResponse.status,
166-
statusText: discoveryResponse.statusText,
167186
})
168187
return NextResponse.json(
169188
{
170-
error: `Failed to fetch OIDC discovery document from ${discoveryUrl}. Status: ${discoveryResponse.status}. Provide all endpoints explicitly or verify the issuer URL.`,
189+
error:
190+
'Failed to fetch OIDC discovery document. Provide all endpoints explicitly or verify the issuer URL.',
171191
},
172192
{ status: 400 }
173193
)
174194
}
175195

176-
const discovery = await discoveryResponse.json()
196+
const discovery = (await discoveryResponse.json()) as Record<string, unknown>
197+
198+
const discoveredEndpoints: Record<string, unknown> = {
199+
authorization_endpoint: discovery.authorization_endpoint,
200+
token_endpoint: discovery.token_endpoint,
201+
userinfo_endpoint: discovery.userinfo_endpoint,
202+
jwks_uri: discovery.jwks_uri,
203+
}
204+
205+
for (const [key, value] of Object.entries(discoveredEndpoints)) {
206+
if (typeof value === 'string') {
207+
const endpointValidation = await validateUrlWithDNS(value, `OIDC ${key}`)
208+
if (!endpointValidation.isValid) {
209+
logger.warn('OIDC discovered endpoint failed SSRF validation', {
210+
endpoint: key,
211+
url: value,
212+
error: endpointValidation.error,
213+
})
214+
return NextResponse.json(
215+
{
216+
error: `Discovered OIDC ${key} failed security validation: ${endpointValidation.error}`,
217+
},
218+
{ status: 400 }
219+
)
220+
}
221+
}
222+
}
177223

178224
oidcConfig.authorizationEndpoint =
179225
oidcConfig.authorizationEndpoint || discovery.authorization_endpoint
@@ -196,7 +242,8 @@ export async function POST(request: NextRequest) {
196242
})
197243
return NextResponse.json(
198244
{
199-
error: `Failed to fetch OIDC discovery document from ${discoveryUrl}. Please verify the issuer URL is correct or provide all endpoints explicitly.`,
245+
error:
246+
'Failed to fetch OIDC discovery document. Please verify the issuer URL is correct or provide all endpoints explicitly.',
200247
},
201248
{ status: 400 }
202249
)

apps/sim/app/api/chat/[identifier]/otp/route.test.ts

Lines changed: 128 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,14 @@ const {
1010
mockRedisSet,
1111
mockRedisGet,
1212
mockRedisDel,
13+
mockRedisTtl,
14+
mockRedisEval,
1315
mockGetRedisClient,
1416
mockRedisClient,
1517
mockDbSelect,
1618
mockDbInsert,
1719
mockDbDelete,
20+
mockDbUpdate,
1821
mockSendEmail,
1922
mockRenderOTPEmail,
2023
mockAddCorsHeaders,
@@ -29,15 +32,20 @@ const {
2932
const mockRedisSet = vi.fn()
3033
const mockRedisGet = vi.fn()
3134
const mockRedisDel = vi.fn()
35+
const mockRedisTtl = vi.fn()
36+
const mockRedisEval = vi.fn()
3237
const mockRedisClient = {
3338
set: mockRedisSet,
3439
get: mockRedisGet,
3540
del: mockRedisDel,
41+
ttl: mockRedisTtl,
42+
eval: mockRedisEval,
3643
}
3744
const mockGetRedisClient = vi.fn()
3845
const mockDbSelect = vi.fn()
3946
const mockDbInsert = vi.fn()
4047
const mockDbDelete = vi.fn()
48+
const mockDbUpdate = vi.fn()
4149
const mockSendEmail = vi.fn()
4250
const mockRenderOTPEmail = vi.fn()
4351
const mockAddCorsHeaders = vi.fn()
@@ -53,11 +61,14 @@ const {
5361
mockRedisSet,
5462
mockRedisGet,
5563
mockRedisDel,
64+
mockRedisTtl,
65+
mockRedisEval,
5666
mockGetRedisClient,
5767
mockRedisClient,
5868
mockDbSelect,
5969
mockDbInsert,
6070
mockDbDelete,
71+
mockDbUpdate,
6172
mockSendEmail,
6273
mockRenderOTPEmail,
6374
mockAddCorsHeaders,
@@ -80,11 +91,13 @@ vi.mock('@sim/db', () => ({
8091
select: mockDbSelect,
8192
insert: mockDbInsert,
8293
delete: mockDbDelete,
94+
update: mockDbUpdate,
8395
transaction: vi.fn(async (callback: (tx: Record<string, unknown>) => unknown) => {
8496
return callback({
8597
select: mockDbSelect,
8698
insert: mockDbInsert,
8799
delete: mockDbDelete,
100+
update: mockDbUpdate,
88101
})
89102
}),
90103
},
@@ -126,12 +139,24 @@ vi.mock('@/lib/messaging/email/mailer', () => ({
126139
sendEmail: mockSendEmail,
127140
}))
128141

129-
vi.mock('@/components/emails/render-email', () => ({
142+
vi.mock('@/components/emails', () => ({
130143
renderOTPEmail: mockRenderOTPEmail,
131144
}))
132145

133-
vi.mock('@/app/api/chat/utils', () => ({
146+
vi.mock('@/lib/core/security/deployment', () => ({
134147
addCorsHeaders: mockAddCorsHeaders,
148+
isEmailAllowed: (email: string, allowedEmails: string[]) => {
149+
if (allowedEmails.includes(email)) return true
150+
const atIndex = email.indexOf('@')
151+
if (atIndex > 0) {
152+
const domain = email.substring(atIndex + 1)
153+
if (domain && allowedEmails.some((allowed: string) => allowed === `@${domain}`)) return true
154+
}
155+
return false
156+
},
157+
}))
158+
159+
vi.mock('@/app/api/chat/utils', () => ({
135160
setChatAuthCookie: mockSetChatAuthCookie,
136161
}))
137162

@@ -209,6 +234,7 @@ describe('Chat OTP API Route', () => {
209234
mockRedisSet.mockResolvedValue('OK')
210235
mockRedisGet.mockResolvedValue(null)
211236
mockRedisDel.mockResolvedValue(1)
237+
mockRedisTtl.mockResolvedValue(600)
212238

213239
const createDbChain = (result: unknown) => ({
214240
from: vi.fn().mockReturnValue({
@@ -225,6 +251,11 @@ describe('Chat OTP API Route', () => {
225251
mockDbDelete.mockImplementation(() => ({
226252
where: vi.fn().mockResolvedValue(undefined),
227253
}))
254+
mockDbUpdate.mockImplementation(() => ({
255+
set: vi.fn().mockReturnValue({
256+
where: vi.fn().mockResolvedValue(undefined),
257+
}),
258+
}))
228259

229260
mockGetStorageMethod.mockReturnValue('redis')
230261

@@ -349,7 +380,7 @@ describe('Chat OTP API Route', () => {
349380
describe('PUT - Verify OTP (Redis path)', () => {
350381
beforeEach(() => {
351382
mockGetStorageMethod.mockReturnValue('redis')
352-
mockRedisGet.mockResolvedValue(mockOTP)
383+
mockRedisGet.mockResolvedValue(`${mockOTP}:0`)
353384
})
354385

355386
it('should retrieve OTP from Redis and verify successfully', async () => {
@@ -374,9 +405,7 @@ describe('Chat OTP API Route', () => {
374405
await PUT(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
375406

376407
expect(mockRedisGet).toHaveBeenCalledWith(`otp:${mockEmail}:${mockChatId}`)
377-
378408
expect(mockRedisDel).toHaveBeenCalledWith(`otp:${mockEmail}:${mockChatId}`)
379-
380409
expect(mockDbSelect).toHaveBeenCalledTimes(1)
381410
})
382411
})
@@ -405,7 +434,7 @@ describe('Chat OTP API Route', () => {
405434
}
406435
return Promise.resolve([
407436
{
408-
value: mockOTP,
437+
value: `${mockOTP}:0`,
409438
expiresAt: new Date(Date.now() + 10 * 60 * 1000),
410439
},
411440
])
@@ -475,7 +504,7 @@ describe('Chat OTP API Route', () => {
475504
})
476505

477506
it('should delete OTP from Redis after verification', async () => {
478-
mockRedisGet.mockResolvedValue(mockOTP)
507+
mockRedisGet.mockResolvedValue(`${mockOTP}:0`)
479508

480509
mockDbSelect.mockImplementationOnce(() => ({
481510
from: vi.fn().mockReturnValue({
@@ -519,7 +548,7 @@ describe('Chat OTP API Route', () => {
519548
return Promise.resolve([{ id: mockChatId, authType: 'email' }])
520549
}
521550
return Promise.resolve([
522-
{ value: mockOTP, expiresAt: new Date(Date.now() + 10 * 60 * 1000) },
551+
{ value: `${mockOTP}:0`, expiresAt: new Date(Date.now() + 10 * 60 * 1000) },
523552
])
524553
}),
525554
}),
@@ -543,6 +572,97 @@ describe('Chat OTP API Route', () => {
543572
})
544573
})
545574

575+
describe('Brute-force protection', () => {
576+
beforeEach(() => {
577+
mockGetStorageMethod.mockReturnValue('redis')
578+
})
579+
580+
it('should atomically increment attempts on wrong OTP', async () => {
581+
mockRedisGet.mockResolvedValue('654321:0')
582+
mockRedisEval.mockResolvedValue('654321:1')
583+
584+
mockDbSelect.mockImplementationOnce(() => ({
585+
from: vi.fn().mockReturnValue({
586+
where: vi.fn().mockReturnValue({
587+
limit: vi.fn().mockResolvedValue([{ id: mockChatId, authType: 'email' }]),
588+
}),
589+
}),
590+
}))
591+
592+
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
593+
method: 'PUT',
594+
body: JSON.stringify({ email: mockEmail, otp: 'wrong1' }),
595+
})
596+
597+
await PUT(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
598+
599+
expect(mockRedisEval).toHaveBeenCalledWith(
600+
expect.any(String),
601+
1,
602+
`otp:${mockEmail}:${mockChatId}`,
603+
5
604+
)
605+
expect(mockCreateErrorResponse).toHaveBeenCalledWith('Invalid verification code', 400)
606+
})
607+
608+
it('should invalidate OTP and return 429 after max failed attempts', async () => {
609+
mockRedisGet.mockResolvedValue('654321:4')
610+
mockRedisEval.mockResolvedValue('LOCKED')
611+
612+
mockDbSelect.mockImplementationOnce(() => ({
613+
from: vi.fn().mockReturnValue({
614+
where: vi.fn().mockReturnValue({
615+
limit: vi.fn().mockResolvedValue([{ id: mockChatId, authType: 'email' }]),
616+
}),
617+
}),
618+
}))
619+
620+
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
621+
method: 'PUT',
622+
body: JSON.stringify({ email: mockEmail, otp: 'wrong5' }),
623+
})
624+
625+
await PUT(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
626+
627+
expect(mockRedisEval).toHaveBeenCalled()
628+
expect(mockCreateErrorResponse).toHaveBeenCalledWith(
629+
'Too many failed attempts. Please request a new code.',
630+
429
631+
)
632+
})
633+
634+
it('should store OTP with zero attempts on generation', async () => {
635+
mockDbSelect.mockImplementationOnce(() => ({
636+
from: vi.fn().mockReturnValue({
637+
where: vi.fn().mockReturnValue({
638+
limit: vi.fn().mockResolvedValue([
639+
{
640+
id: mockChatId,
641+
authType: 'email',
642+
allowedEmails: [mockEmail],
643+
title: 'Test Chat',
644+
},
645+
]),
646+
}),
647+
}),
648+
}))
649+
650+
const request = new NextRequest('http://localhost:3000/api/chat/test/otp', {
651+
method: 'POST',
652+
body: JSON.stringify({ email: mockEmail }),
653+
})
654+
655+
await POST(request, { params: Promise.resolve({ identifier: mockIdentifier }) })
656+
657+
expect(mockRedisSet).toHaveBeenCalledWith(
658+
`otp:${mockEmail}:${mockChatId}`,
659+
expect.stringMatching(/^\d{6}:0$/),
660+
'EX',
661+
900
662+
)
663+
})
664+
})
665+
546666
describe('Behavior consistency between Redis and Database', () => {
547667
it('should have same behavior for missing OTP in both storage methods', async () => {
548668
mockGetStorageMethod.mockReturnValue('redis')

0 commit comments

Comments
 (0)