fix(security): enforce URL validation across connectors, providers, and auth flows (SSRF + open-redirect hardening)#4236
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Connectors/providers now enforce SSRF guards before making server-side requests: Obsidian vault URLs are resolved/validated via Reviewed by Cursor Bugbot for commit 5d26e7b. Configure here. |
Greptile SummaryThis PR hardens SSRF and open-redirect attack surfaces across connectors, providers, and auth flows by centralizing validation logic in
Confidence Score: 4/5Safe to merge for most surfaces, but the Obsidian DNS-rebinding gap should be resolved or explicitly descoped before merging. The bulk of the security hardening (ServiceNow allowlist, Workday allowlist, Azure DNS validation, open-redirect guards) is well-implemented and tested. The one P1 finding is the gap between the stated and actual Obsidian protection: the PR advertises DNS-pinning but ships without it, leaving a rebinding vector open. apps/sim/connectors/obsidian/obsidian.ts — DNS rebinding mitigation described in PR but not implemented
|
| Filename | Overview |
|---|---|
| apps/sim/lib/core/security/input-validation.ts | Adds validateCallbackUrl (same-origin enforcement via URL API), validateServiceNowInstanceUrl (domain allowlist + HTTPS), and validateWorkdayTenantUrl (domain allowlist + HTTPS); all correctly guard against the described attack vectors. |
| apps/sim/connectors/obsidian/obsidian.ts | Adds resolveVaultEndpoint with validateExternalUrl, but lacks the DNS-pinning protection (secureFetchWithPinnedIPAndRetry) claimed in the PR description; DNS rebinding is unmitigated. |
| apps/sim/connectors/servicenow/servicenow.ts | resolveServiceNowInstanceUrl correctly delegates to validateServiceNowInstanceUrl on every listDocuments/getDocument/validateConfig call, with proper error propagation. |
| apps/sim/providers/azure-openai/index.ts | validateUrlWithDNS applied to user-supplied azureEndpoint only; env-provided endpoint correctly bypasses the check as a trusted value. |
| apps/sim/providers/azure-anthropic/index.ts | Same pattern as Azure OpenAI: validateUrlWithDNS on user-supplied azureEndpoint, env fallback bypasses validation correctly. |
| apps/sim/tools/workday/soap.ts | buildWsdlUrl now validates via validateWorkdayTenantUrl and uses validation.sanitized, protecting all 10 Workday route handlers through a single chokepoint. |
| apps/sim/app/(auth)/signup/signup-form.tsx | validateCallbackUrl applied to redirect/callbackUrl params; logger.warn is called during render rather than in useEffect, which is a minor React rules violation. |
| apps/sim/app/(auth)/verify/use-verification.ts | validateCallbackUrl applied to both sessionStorage-stored redirect and redirectAfter query param; unsafe stored values are actively removed from sessionStorage. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User-supplied URL] --> B{Entry point}
B --> C[Workday SOAP buildWsdlUrl]
B --> D[ServiceNow resolveServiceNowInstanceUrl]
B --> E[Obsidian resolveVaultEndpoint]
B --> F[Azure OpenAI/Anthropic executeRequest]
B --> G[Auth signup redirect param]
B --> H[Auth verify redirectAfter / sessionStorage]
C --> I[validateWorkdayTenantUrl *.workday.com allowlist]
D --> J[validateServiceNowInstanceUrl *.service-now.com allowlist]
E --> K[validateExternalUrl HTTPS + port + IP checks]
F --> L[validateUrlWithDNS DNS resolution + IP checks]
G --> M[validateCallbackUrl same-origin check]
H --> M
I --> N{Valid?}
J --> N
K --> N
L --> N
M --> N
N -->|No| O[Throw / drop / warn]
N -->|Yes| P[Proceed with request]
K -.->|No IP pinning| Q[fetchWithRetry DNS rebinding possible]
Reviews (2): Last reviewed commit: "fix(obsidian): use isomorphic SSRF valid..." | Re-trigger Greptile
- Azure OpenAI/Anthropic: validate user-supplied azureEndpoint with validateUrlWithDNS to block SSRF to private IPs, localhost (in hosted mode), and dangerous ports. - ServiceNow connector: enforce ServiceNow domain allowlist via validateServiceNowInstanceUrl before calling the instance URL. - Obsidian connector: validate vaultUrl with validateUrlWithDNS and reuse the resolved IP via secureFetchWithPinnedIPAndRetry to block DNS rebinding between validation and request. - Signup + verify flows: pass redirect/callbackUrl/redirectAfter and stored inviteRedirectUrl through validateCallbackUrl; drop unsafe values and log a warning. - lib/knowledge/documents/utils.ts: add secureFetchWithPinnedIPAndRetry wrapper around secureFetchWithPinnedIP (used by Obsidian).
The Obsidian connector is reachable from client bundles via `connectors/registry.ts` (the knowledge UI reads metadata like `.icon`/`.name`). Importing `validateUrlWithDNS` / `secureFetchWithPinnedIP` from `input-validation.server` pulled `dns/promises`, `http`, `https`, `net` into client chunks, breaking the Turbopack build: Module not found: Can't resolve 'dns/promises' ./apps/sim/lib/core/security/input-validation.server.ts [Client Component Browser] ./apps/sim/connectors/obsidian/obsidian.ts [Client Component Browser] ./apps/sim/connectors/registry.ts [Client Component Browser] Once that file polluted a browser context, Turbopack also failed to resolve the Node builtins in its legitimate server-route imports, cascading the error across App Routes and Server Components. Fix: switch the Obsidian connector to the isomorphic `validateExternalUrl` + `fetchWithRetry` helpers, matching the pattern used by every other connector in the registry. This keeps the core SSRF protections: - hosted Sim: blocks localhost, private IPs, HTTP (HTTPS enforced) - self-hosted Sim: allows localhost + HTTP, still blocks non-loopback private IPs and dangerous ports (22, 25, 3306, 5432, 6379, 27017, 9200) Drops the DNS-rebinding defense specifically (the IP-pinned fetch chain). The trade-off is acceptable because the vault URL is entered by the workspace admin — not arbitrary untrusted input — and hosted deployments already force the plugin to be exposed through a public URL (tunnel/port-forward), making rebinding a narrow threat. Also reverts the `secureFetchWithPinnedIPAndRetry` wrapper in `lib/knowledge/documents/utils.ts` (no longer needed, and its `.server` import was the original source of the client-bundle pollution).
|
@greptile |
|
@cursor review |
Match listDocuments behavior — invalid instance URL should surface as a configuration error rather than being swallowed into a "document not found" null response during sync. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5d26e7b. Configure here.
| if (url && !url.startsWith('https://') && !url.startsWith('http://')) { | ||
| url = `https://${url}` | ||
| } | ||
| const validation = validateExternalUrl(url, 'vaultUrl', { allowHttp: true }) |
There was a problem hiding this comment.
allowHttp defeats HTTPS enforcement in hosted mode
Medium Severity
The { allowHttp: true } option passed to validateExternalUrl permits HTTP for all hosts in all deployment modes, directly contradicting the JSDoc at line 32 which states "hosted Sim: blocks localhost, private IPs, HTTP (forces HTTPS)." The default behavior of validateExternalUrl without allowHttp already provides the documented policy: HTTPS enforced everywhere except localhost in self-hosted mode (line 728 of input-validation.ts). With allowHttp: true, an admin in hosted mode could configure http://public-host/ as the vault URL, exposing the Bearer access token over plaintext.
Reviewed by Cursor Bugbot for commit 5d26e7b. Configure here.
Summary
Hardens user-controlled URL surfaces across the app against SSRF (server-side forgery to internal metadata endpoints, private IPs, arbitrary hosts) and open-redirect attacks on auth flows. Every fix is centralized through shared validators in
@/lib/core/security/input-validationso each consumer is a thin call site.New / updated validators (
lib/core/security/input-validation.ts)validateCallbackUrl— rewritten to resolve against a safe server-side base vianew URL(), rejecting protocol-relative (//evil.com), backslash/whitespace bypasses, userinfo smuggling (https://safe@evil.com), and non-HTTP schemes while still accepting legitimate same-origin relative/absolute callbacks.validateServiceNowInstanceUrl— delegates HTTPS + private-IP + port checks tovalidateExternalUrl, then enforces a ServiceNow-owned hostname allowlist.validateWorkdayTenantUrl— same pattern, enforcing*.workday.comand*.myworkday.com(covers sandboxwd[N]-impl-services[N]and productionwd[N]-services[N]+ customer-facing*.myworkday.com).169.254.169.254, RFC1918), blocked ports, null/empty, malformed URLs, and case-insensitive matching.SSRF fixes at the request chokepoints
tools/workday/soap.ts) —buildWsdlUrlnow callsvalidateWorkdayTenantUrlbefore constructing the WSDL URL. This is the single code path for all 10 Workday route handlers, so every operation (get_workers,get_job_postings,submit_time_off, …) is protected without per-route duplication. Usesvalidation.sanitizedso any future normalization (credential stripping, Unicode) flows through automatically.azureEndpointviavalidateUrlWithDNSbefore instantiating the SDK client. Blocks private IPs, localhost in hosted mode, and dangerous ports; env-provided endpoints bypass the check (trusted).resolveServiceNowInstanceUrlrunsvalidateServiceNowInstanceUrlon everylistDocuments,fetchDocument, andvalidateConfigcall before any request is made.resolveVaultEndpointruns the isomorphicvalidateExternalUrlhelper, which blocks private IPs and dangerous ports in hosted mode, forces HTTPS, and carves out127.0.0.1/localhostfor self-hosted deployments only.connectors/registry.ts(the knowledge UI reads.icon/.name), which forces the connector module graph to stay client-safe. This means the DNS-pinned fetch chain (which would also defend against DNS rebinding) cannot live in the connector without pullingdns/promises/http/httpsinto the browser bundle (see commitfa5ab2812). The remaining attack requires an admin-entered hostname that flips between validation and request — narrow because the vault URL is entered by the workspace admin (trusted), and hosted deployments already require exposing the plugin through a public tunnel. Closing this gap will require a registry architecture split (metadata registry for client, handler registry for server), tracked as a follow-up.Open-redirect fixes on auth flows
(auth)/signup/signup-form.tsx) —redirectandcallbackUrlquery params pass throughvalidateCallbackUrl; invalid values are dropped and logged.(auth)/verify/use-verification.ts) — bothsessionStorage.inviteRedirectUrland?redirectAfter=are validated; stored unsafe values are actively removed from sessionStorage.Type of Change
Testing
input-validation.test.tssuite — 363 tests passing, including 60 new adversarial cases.bunx turbo run build --filter=sim— compiles successfully (remainingNEXT_PUBLIC_APP_URLcollect-page-data error is local-env only).*.workday.com(sandbox + production service endpoints) and*.myworkday.com(customer-facing production endpoints).evilworkday.com), embedded substrings (workday.com.evil.com), userinfo smuggling (https://attacker.com@wd5.workday.com), protocol-relative (//evil.com), backslash tricks — all correctly blocked.Checklist