fix(security): KB fileUrl LFI, MCP/Agiloft SSRF pinning, form OTP, KB authz by waleedlatif1 · Pull Request #4639 · simstudioai/sim

waleedlatif1 · 2026-05-17T04:47:40Z

Summary

Bundled security hardening across multiple surfaces:

KB fileUrl LFI: POST /api/knowledge/[id]/documents/upsert previously accepted any non-empty string for fileUrl, which the background processor then passed to fs.readFile / parseFile, allowing authenticated arbitrary local file reads (e.g. /app/.env, /etc/passwd). Now gated at the boundary via knowledgeDocumentFileUrlSchema (only data: URIs or http(s):// URLs); defense-in-depth in document-processor.ts throws on any other scheme.
KB authorization & transaction hardening: introduces KnowledgeBasePermissionError, tightens permission checks, and wraps multi-step KB mutations in transactions.
MCP DNS-rebinding (SSRF): outbound MCP calls now go through pinned-fetch.ts (undici Agent with a pinned lookup) so the host the policy validated is the host actually connected to.
Agiloft SSRF/DNS pinning: resolveAgiloftInstance + pinned fetch for attach / retrieve routes.
Grafana baseUrl validation: update_alert_rule / update_dashboard route through secureFetchWithValidation.
Form email-OTP flow: new POST /api/form/[identifier]/otp route + email-auth.tsx UI; shared OTP module under lib/core/security/otp.ts keyed by DeploymentKind, with the chat OTP route refactored to consume it.
Audit script ratchet: bumps the API-validation baseline by one route for the new form OTP endpoint.

Test plan

bun run check:api-validation:strict passes
Lint passes
194 unit tests pass across affected areas (contracts, route handlers, services, pinned-fetch, OTP)
Production data audited via PlanetScale MCP — 10,501 legitimate relative /api/files/serve/... rows have zero completed status (already failing under the old fs.readFile path); change is strictly a clearer error, not a regression
Backwards-compat verified end-to-end on staging

🤖 Generated with Claude Code

vercel · 2026-05-17T04:47:45Z

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

Project	Deployment	Actions	Updated (UTC)
docs	Ready	Preview, Comment	May 17, 2026 5:51am

gitguardian · 2026-05-17T04:47:46Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

cursor · 2026-05-17T04:47:49Z

PR Summary

High Risk
High risk because it changes authentication flows and adds/changes SSRF and file-ingestion validation logic that can block previously accepted inputs or affect outbound integrations (MCP/Agiloft/Grafana). The changes are broad and touch critical request/response paths and network I/O behavior.

Overview
Adds email OTP authentication for deployed forms. Introduces new POST/PUT /api/form/[identifier]/otp endpoints plus client UI (EmailAuth) and query hooks/contracts, and changes form auth validation to require OTP (otp_required) instead of granting access based solely on an allowed email match.

Centralizes OTP storage/attempt tracking. Moves chat OTP logic into a shared lib/core/security/otp module (Redis/DB storage, rate limits, attempt locking), and refactors the chat OTP route to use the shared implementation.

Closes a KB document ingestion LFI vector. Tightens fileUrl contract validation to only allow data: or http(s):// URLs and removes local filesystem reads in the document processor (fails closed on unsupported schemes), with new tests covering allowed/rejected inputs.

Pins and validates outbound connections to reduce SSRF/TOCTOU risk. MCP SSRF validation now returns a resolved IP and, when available, the MCP client uses an undici-based pinned fetch to prevent DNS rebinding; Agiloft attach/retrieve routes similarly resolve once and pin all subsequent requests; Grafana update tools now validate baseUrl before issuing requests.

Hardens knowledge base workspace authorization. Adds KnowledgeBasePermissionError, enforces actor-aware permission checks for workspace transfers/clearing in updateKnowledgeBase within a transaction/row lock, and maps these failures to 403 in the API routes with added tests.

Updates API validation baseline and adds extensive unit tests across the new and hardened paths.

^{Reviewed by Cursor Bugbot for commit 21a93a5. Configure here.}

greptile-apps · 2026-05-17T04:59:54Z

Greptile Summary

This PR delivers a bundled security hardening across five surfaces: LFI prevention on Knowledge Base file upload, KB permission errors and transaction hardening, DNS-rebinding (SSRF) pinning for MCP and Agiloft, basic URL validation for Grafana tools, and a new email-OTP gate for form deployments with a shared otp.ts module reused by both chat and form routes.

LFI & KB hardening: knowledgeDocumentFileUrlSchema restricts accepted fileUrl values to data: URIs and http(s):// URLs at the Zod boundary; document-processor.ts removes the fs.readFile fallback. KnowledgeBasePermissionError is introduced and multi-step KB mutations are wrapped in transactions with SELECT … FOR UPDATE.
MCP DNS pinning: validateMcpServerSsrf now returns the resolved IP; createMcpPinnedFetch (backed by undici's fetch) forces all subsequent transport connections to that IP, closing the TOCTOU window between validation and connection.
Agiloft DNS pinning: resolveAgiloftInstance + secureFetchWithPinnedIP give the Agiloft attach/retrieve routes the same resolve-once-pin-always protection as MCP.
Grafana URL validation: validateExternalUrl is applied before each Grafana fetch call, blocking IP-literal SSRF; DNS rebinding is not mitigated (see comment).
Form OTP: New POST /api/form/[identifier]/otp + PUT routes mirror the existing chat OTP flow, sharing the centralized lib/core/security/otp.ts module.

Confidence Score: 5/5

Safe to merge; the Grafana URL validation gap does not regress from the pre-PR state and is noted for a follow-up improvement.

The five hardened surfaces (KB LFI, KB authz, MCP DNS pinning, Agiloft DNS pinning, form OTP) are all correctly implemented. The Grafana tools move from no validation to basic format checking — an improvement, not a regression — but do not reach the DNS-pinned level of the Agiloft fix. All previous review comments were addressed in follow-up commits. The OTP module's Redis Lua path, DB optimistic-lock fail-closed path, and rate-limiting are sound.

apps/sim/tools/grafana/update_alert_rule.ts and apps/sim/tools/grafana/update_dashboard.ts — both validate baseUrl with the synchronous format-only check and still use plain fetch; a follow-up to add DNS resolution and IP pinning would bring them in line with the Agiloft pattern.

Security Review

Grafana SSRF (DNS rebinding) — update_alert_rule.ts / update_dashboard.ts use validateExternalUrl (synchronous, format-only) before a plain fetch. This blocks IP-literal SSRF but not DNS-rebinding; a hostname that resolves to a private IP at connection time would bypass the check. The Agiloft routes were upgraded to validateUrlWithDNS + secureFetchWithPinnedIP; the same pattern should be applied to the Grafana tools to close the gap.
No new secrets or credentials exposed in this change.
KB LFI fix (fs.readFile path removed) and MCP/Agiloft DNS-pinning are correctly implemented.
Form OTP uses crypto.randomInt for code generation, rate-limits at both IP and email granularity, and fails closed on DB-path retry exhaustion — all correct.

Important Files Changed

Filename	Overview
apps/sim/lib/core/security/otp.ts	New shared OTP module: correct Redis Lua atomic increment, DB optimistic-lock retry with fail-closed exhaustion, per-kind key namespacing, and clean encode/decode for the code:attempts format.
apps/sim/lib/mcp/pinned-fetch.ts	New pinned-fetch implementation using undici's typed fetch export with a createPinnedLookup dispatcher; correctly bridges DOM/undici type gap via documented cast and preserves hostname for TLS SNI.
apps/sim/lib/mcp/domain-check.ts	Returns resolved IP from validateMcpServerSsrf for downstream pinning; loopback is now blocked on hosted environments; DNS failure path is now distinct from SSRF-block path.
apps/sim/tools/agiloft/utils.server.ts	New server-only Agiloft helpers: resolves DNS once via validateUrlWithDNS, then uses secureFetchWithPinnedIP for login/logout/attach/retrieve, closing the TOCTOU SSRF window.
apps/sim/tools/grafana/update_alert_rule.ts	Adds validateExternalUrl before the Grafana API call — blocks IP-literal SSRF but does not resolve DNS, leaving DNS-rebinding attacks open (inconsistent with the Agiloft/MCP pattern).
apps/sim/tools/grafana/update_dashboard.ts	Same as update_alert_rule — synchronous format-only validation with plain fetch; DNS rebinding SSRF not mitigated.
apps/sim/lib/knowledge/service.ts	KB update wrapped in a SELECT FOR UPDATE transaction; workspace-change permission check added via actorUserId; KnowledgeBasePermissionError distinguishes auth failures from 500s.
apps/sim/lib/knowledge/documents/document-processor.ts	Removes fs.readFile fallback; all non-data:/http(s):// paths now throw; case-insensitive regex checks applied consistently across downloadFileForBase64 and parseWithFileParser.
apps/sim/app/api/form/[identifier]/otp/route.ts	New form OTP route mirrors chat OTP pattern precisely; IP and email rate limiting, allowlist check before OTP store/compare, generic 500 messages, and auth cookie set on success.
apps/sim/lib/api/contracts/knowledge/shared.ts	Adds knowledgeDocumentFileUrlSchema with case-insensitive data: and https?:// guards; correctly applied at the Zod validation boundary.
apps/sim/app/form/[identifier]/components/email-auth.tsx	New email-auth UI component with two-step flow (email → OTP), countdown resend timer, 6-digit OTP auto-submit, and clear error messaging.

_{Reviews (2): Last reviewed commit: "fix(mcp): annotate undici/DOM type-bridg..." | Re-trigger Greptile}

… authz

…haust - Chat/form OTP routes: replace `error.message || fallback` with generic `Failed to process request` in 500 responses (logger still captures detail). - otp.ts incrementOTPAttempts DB path: on MAX_RETRIES exhaustion, delete the verification row and return `'locked'` instead of trusting a possibly- undercounted final read. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Replace `globalThis.fetch` + double-cast with `undici.fetch` so the `dispatcher` option is part of the real type contract. This guarantees pinning won't silently break if a future runtime swaps the underlying fetch implementation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Tool config files are statically reachable from the client bundle (via tools/registry.ts → tools/{service}/index.ts). Importing `@/lib/core/security/input-validation.server` from these files pulled `node:dns/promises` into the Turbopack client bundle and broke the build. Split agiloft utils into client-safe (`utils.ts`, plain fetch + sync `validateExternalUrl`) and server-only (`utils.server.ts`, DNS-pinned variants). Routes that need TOCTOU protection import the pinned helpers; the executor-side tool path falls back to sync URL validation (matches the supabase precedent and pre-PR baseline). Grafana update tools likewise switch from `secureFetchWithValidation` (server-only) to inline sync `validateExternalUrl` + plain fetch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Boundary schema accepted uppercase schemes (e.g. HTTPS://, DATA:) via the case-insensitive http regex, but the processor's case-sensitive startsWith('data:') / startsWith('http') / startsWith('https://') checks rejected them with a confusing "Unsupported fileUrl scheme" error. Aligns processor checks to the schema using case-insensitive regex per RFC 3986 §3.1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Strict audit was failing on two new `as unknown as` casts in pinned-fetch.ts. They bridge DOM `RequestInit`/`Response` ↔ undici equivalents (structurally compatible at runtime since Node's global fetch is undici) and are required to satisfy the FetchLike contract. Annotate so they count as documented exemptions instead of new violations. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

waleedlatif1 · 2026-05-17T05:46:09Z

@greptile

waleedlatif1 · 2026-05-17T05:46:12Z

@cursor review

waleedlatif1 · 2026-05-17T05:56:31Z

@cursor review

cursor

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 21a93a5. Configure here.}

cursor · 2026-05-17T06:09:14Z

+  form: {
+    redisKey: (email: string, deploymentId: string) => `form-otp:${email}:${deploymentId}`,
+    dbIdentifier: (email: string, deploymentId: string) => `form-otp:${deploymentId}:${email}`,
+  },


Form OTP key component ordering inconsistent across storage backends

Low Severity

The form OTP key format has reversed component ordering between Redis (form-otp:${email}:${deploymentId}) and DB (form-otp:${deploymentId}:${email}). For chat, this inconsistency is preserved for backward compatibility with in-flight OTPs (documented in the comment above). But form is an entirely new key format with no legacy data — there's no reason to replicate the asymmetry. This makes the code harder to reason about and could cause confusion when debugging OTP issues or if a deployment ever switches storage backends mid-flight.

^{Reviewed by Cursor Bugbot for commit 21a93a5. Configure here.}

vercel Bot deployed to Preview May 17, 2026 04:51 View deployment

cursor Bot reviewed May 17, 2026

View reviewed changes

Comment thread apps/sim/lib/api/contracts/knowledge/shared.ts

greptile-apps Bot reviewed May 17, 2026

View reviewed changes

Comment thread apps/sim/app/api/form/[identifier]/otp/route.ts Outdated

Comment thread apps/sim/lib/core/security/otp.ts

Comment thread apps/sim/lib/api/contracts/knowledge/shared.ts

Comment thread apps/sim/app/api/form/utils.ts

Comment thread apps/sim/lib/mcp/pinned-fetch.ts Outdated

vercel Bot temporarily deployed to Preview May 17, 2026 05:10 Inactive

vercel Bot temporarily deployed to Preview May 17, 2026 05:12 Inactive

vercel Bot temporarily deployed to Preview May 17, 2026 05:18 Inactive

vercel Bot temporarily deployed to Preview May 17, 2026 05:22 Inactive

waleedlatif1 and others added 6 commits May 16, 2026 22:45

fix(security): KB fileUrl LFI, MCP/Agiloft SSRF pinning, form OTP, KB…

4d3e1b3

… authz

waleedlatif1 force-pushed the waleedlatif1/fix-kb-fileurl-lfi branch from abdc919 to 21a93a5 Compare May 17, 2026 05:45

cursor Bot reviewed May 17, 2026

View reviewed changes

Comment thread apps/sim/tools/grafana/update_alert_rule.ts

vercel Bot deployed to Preview May 17, 2026 05:51 View deployment

waleedlatif1 merged commit 08eeecb into staging May 17, 2026
14 checks passed

waleedlatif1 deleted the waleedlatif1/fix-kb-fileurl-lfi branch May 17, 2026 06:05

TheodoreSpeaks mentioned this pull request May 17, 2026

v0.6.84: redis pub/sub SNI override, security hardening, copilot reads trimmed #4641

Merged

cursor Bot reviewed May 17, 2026

View reviewed changes

Conversation

waleedlatif1 commented May 17, 2026

Summary

Test plan

Uh oh!

vercel Bot commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitguardian Bot commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

️✅ There are no secrets present in this pull request anymore.

Uh oh!

cursor Bot commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

Uh oh!

greptile-apps Bot commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Security Review

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented May 17, 2026

Uh oh!

waleedlatif1 commented May 17, 2026

Uh oh!

Uh oh!

waleedlatif1 commented May 17, 2026

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot May 17, 2026

Choose a reason for hiding this comment

Form OTP key component ordering inconsistent across storage backends

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel Bot commented May 17, 2026 •

edited

Loading

gitguardian Bot commented May 17, 2026 •

edited

Loading

cursor Bot commented May 17, 2026 •

edited

Loading

greptile-apps Bot commented May 17, 2026 •

edited

Loading