Skip to content

fix(security): harden auth, SSRF, injection, and CORS across API routes#3792

Merged
waleedlatif1 merged 25 commits intostagingfrom
waleedlatif1/review-race-condition
Mar 26, 2026
Merged

fix(security): harden auth, SSRF, injection, and CORS across API routes#3792
waleedlatif1 merged 25 commits intostagingfrom
waleedlatif1/review-race-condition

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • File serve auth bypass: replaced user-controlled ?context query param with key-prefix check so attackers can't skip auth by appending ?context=profile-pictures to any file URL. Added og-images/ to inferContextFromKey and verifyFileAccess.
  • OTP brute-force protection: added attempt counter (Redis INCR / DB fallback) that invalidates the OTP after 5 failed attempts and returns 429. Clears attempts on new OTP generation.
  • HMAC auth tokens: replaced SHA-256 hash-based tokens with HMAC-signed tokens using BETTER_AUTH_SECRET + timingSafeEqual validation. Removed encryptedPassword from token generation.
  • CORS hardening: dropped Access-Control-Allow-Credentials: true from chat/form CORS headers. Cookies are SameSite=Lax and only used within same-origin iframe contexts, so credentials header was unnecessary.
  • SSRF protection on MCP servers: added DNS-based SSRF validation (validateMcpServerSsrf) before and after env var resolution in MCP server create/test-connection routes and service layer.
  • SSH command injection: randomized heredoc delimiter in execute-script route to prevent delimiter injection. Escaped workingDirectory in execute-command route.
  • OIDC SSRF: added validateUrlWithDNS + secureFetchWithPinnedIP to SSO register route for OIDC discovery URL fetches.
  • Input validation: tightened URL validation, domain checks, and reference validation across executor handlers, tools, and background jobs.

Type of Change

  • Bug fix

Testing

  • Ran all affected test suites (file serve, chat utils, form utils, OTP, MCP domain-check) — all passing
  • Reviewed each change for regressions against legitimate access patterns

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

waleedlatif1 and others added 5 commits March 26, 2026 13:41
…ile 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>
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>
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>
…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>
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>
@vercel
Copy link

vercel bot commented Mar 26, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 26, 2026 11:30pm

Request Review

@cursor
Copy link

cursor bot commented Mar 26, 2026

PR Summary

High Risk
High risk because it changes authentication token format/validation and file-serving authorization paths, and adds new SSRF defenses that could block previously-accepted URLs or break integrations if misconfigured.

Overview
Hardens multiple security-sensitive paths across the API.

Prevents file-serve auth bypass by removing trust in user-supplied ?context and treating only profile-pictures/ + og-images/ key prefixes as public, while adding og-images support to context inference and authorization.

Adds OTP brute-force protection for chat email auth by storing OTPs as code:attempts, atomically incrementing attempts (Redis Lua / DB optimistic update), and returning 429 + invalidating the code after 5 failures.

Strengthens deployed chat/form auth cookies by switching to HMAC-signed tokens (BETTER_AUTH_SECRET + timingSafeEqual), and hardens CORS by no longer setting Access-Control-Allow-Credentials.

Expands SSRF protections: OIDC discovery fetch now does DNS validation and IP-pinned fetch; MCP server create/update/test and service env-var resolution now perform DNS-based SSRF checks with explicit error mapping.

Mitigates command injection in SSH tool routes by escaping workingDirectory and randomizing heredoc delimiters, and tightens internal executor authentication by only trusting userId embedded in internal JWTs and propagating that userId when generating internal tokens.

Written by Cursor Bugbot for commit 4790853. Configure here.

@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

…ted 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.
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR applies a broad set of hardening fixes across authentication, SSRF, SSH injection, CORS, and OTP brute-force protection. The changes are well-scoped and address real attack vectors:

  • Auth bypass fixed: File-serve route no longer trusts the user-controlled ?context query param; now performs a key-prefix check, closing a simple bypass that let any user skip auth with ?context=profile-pictures.
  • HMAC tokens: Chat/form auth cookies are now HMAC-SHA256-signed with BETTER_AUTH_SECRET and validated with timingSafeEqual, replacing unsigned base64 tokens.
  • OTP brute-force: Attempt counter is now embedded in the stored value (code:attempts) and incremented atomically (Redis Lua script or DB optimistic-lock), returning 429 after 5 failures.
  • SSRF on MCP servers: DNS-based validation (validateMcpServerSsrf) is applied before and after env-var resolution in all three MCP routes and in the service layer as defense-in-depth.
  • SSH injection: workingDirectory is now properly escaped with escapeShellArg, and heredoc delimiters are randomized with a UUID to prevent delimiter injection.
  • OIDC SSRF: Discovery URL and all discovered OIDC endpoints are validated via validateUrlWithDNS + secureFetchWithPinnedIP.
  • CORS: Access-Control-Allow-Credentials: true removed from chat/form CORS headers (cookies are SameSite=Lax and were never sent cross-site anyway).
  • Internal JWT hardening: resolveUserFromJwt now trusts only the JWT payload, removing user-controlled userId extraction from query params and POST bodies.

Previous review comments (TOCTOU, Redis KEEPTTL compat, legacy OTP format, DNS vs SSRF error distinction, unreachable fetch fallback) have all been addressed.

Confidence Score: 5/5

Safe to merge — all raised security concerns from previous review rounds are resolved, and no new regressions or vulnerabilities were found.

All eight stated attack vectors are correctly addressed. Previously flagged issues (TOCTOU race, Redis KEEPTTL compatibility, legacy OTP format backward-compat, DNS vs SSRF error distinction, unreachable fetch fallback) are all resolved. The single remaining note is a minor P2 style suggestion to parallelise four sequential DNS lookups during SSO registration — it does not affect correctness or security.

No files require special attention. apps/sim/app/api/auth/sso/register/route.ts has a minor sequential DNS lookup pattern worth optimising but it is not blocking.

Important Files Changed

Filename Overview
apps/sim/app/api/files/serve/[...path]/route.ts Auth bypass closed: key-prefix check replaces user-controlled ?context param; context is now always inferred from the storage key.
apps/sim/lib/core/security/deployment.ts HMAC-signed auth tokens replace unsigned SHA-256 tokens; timingSafeEqual used correctly; isEmailAllowed extracted as shared utility.
apps/sim/app/api/chat/[identifier]/otp/route.ts OTP brute-force protection added: atomic Redis Lua script and DB optimistic-lock; legacy format backward-compat guard included; crypto.randomInt used for CSPRNG.
apps/sim/lib/mcp/domain-check.ts New validateMcpServerSsrf with DNS lookup; McpSsrfError and McpDnsResolutionError correctly distinguished; localhost/loopback bypassed for dev.
apps/sim/app/api/auth/sso/register/route.ts OIDC discovery URL validated via validateUrlWithDNS + secureFetchWithPinnedIP; discovered OIDC endpoints also individually validated, but sequentially rather than in parallel.
apps/sim/app/api/tools/ssh/execute-script/route.ts Heredoc delimiter injection fixed: random UUID suffix makes the delimiter unpredictable, preventing an attacker from crafting script content that terminates the heredoc early.
apps/sim/app/api/tools/ssh/execute-command/route.ts workingDirectory now escaped via escapeShellArg (replaces single-quotes correctly with ''' idiom), preventing shell injection.
apps/sim/lib/auth/hybrid.ts resolveUserFromJwt now only trusts userId from JWT payload, removing user-controlled extraction from query params and POST body.
apps/sim/lib/mcp/service.ts SSRF check added as defense-in-depth inside resolveConfigEnvVars; validates after env-var expansion so late-binding DNS rebinding is caught at execution time.
apps/sim/app/api/tools/onepassword/utils.ts validateConnectServerUrl added to pin resolved IP before every connectRequest; link-local blocking is intentional; return type changed to non-nullable string.

Sequence Diagram

sequenceDiagram
    participant Client
    participant MCPRoute as MCP Route (POST/PATCH)
    participant DomainCheck as validateMcpDomain
    participant SsrfCheck as validateMcpServerSsrf
    participant DNS as DNS Resolver
    participant Service as McpService.resolveConfigEnvVars
    participant DB as Database

    Client->>MCPRoute: POST /api/mcp/servers {url}
    MCPRoute->>DomainCheck: validateMcpDomain(url)
    alt Domain not allowed
        DomainCheck-->>MCPRoute: throw McpDomainNotAllowedError
        MCPRoute-->>Client: 403 Domain not allowed
    end
    MCPRoute->>SsrfCheck: validateMcpServerSsrf(url) [pre-resolution]
    SsrfCheck->>DNS: dns.lookup(hostname)
    alt DNS failure
        DNS-->>SsrfCheck: error
        SsrfCheck-->>MCPRoute: throw McpDnsResolutionError
        MCPRoute-->>Client: 502 DNS resolution failed
    else Resolves to private IP
        SsrfCheck-->>MCPRoute: throw McpSsrfError
        MCPRoute-->>Client: 403 SSRF blocked
    end
    MCPRoute->>DB: INSERT server
    MCPRoute-->>Client: 201 Created

    Note over Service,DNS: At tool-execution time (defense-in-depth)
    Service->>DNS: dns.lookup(resolvedUrl)
    alt DNS rebinding detected
        DNS-->>Service: private IP
        Service-->>Service: throw McpSsrfError (fail-closed)
    end
Loading

Reviews (7): Last reviewed commit: "fix: format long lines in chat/form test..." | Re-trigger Greptile

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>
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>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

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>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

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>
- 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>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

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>
…ution

- 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>
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>
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>
…n-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>
@waleedlatif1
Copy link
Collaborator Author

@cursor review

@waleedlatif1
Copy link
Collaborator Author

@greptile

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>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

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>
@waleedlatif1 waleedlatif1 merged commit 66ce673 into staging Mar 26, 2026
6 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/review-race-condition branch March 26, 2026 23:32
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.

1 participant