fix(mcp,audit): tighten env var domain bypass, add post-resolution check, form workspaceId

- Only bypass MCP domain check when env var is in hostname/authority, not path/query
- Add post-resolution validateMcpDomain call in test-connection endpoint
- Match client-side isDomainAllowed to same hostname-only bypass logic
- Return workspaceId from checkFormAccess, use in form audit logs
- Add 49 comprehensive domain-check tests covering all edge cases

Author: waleedlatif1

apps/sim/app/api/form/manage/[id]/route.ts

@@ -103,7 +103,11 @@ export async function PATCH(request: NextRequest, { params }: { params: Promise<
 
     const { id } = await params
 
-    const { hasAccess, form: formRecord } = await checkFormAccess(id, session.user.id)
+    const {
+      hasAccess,
+      form: formRecord,
+      workspaceId: formWorkspaceId,
+    } = await checkFormAccess(id, session.user.id)
 
     if (!hasAccess || !formRecord) {
       return createErrorResponse('Form not found or access denied', 404)
@@ -186,7 +190,7 @@ export async function PATCH(request: NextRequest, { params }: { params: Promise<
       logger.info(`Form ${id} updated successfully`)
 
       recordAudit({
-        workspaceId: null,
+        workspaceId: formWorkspaceId ?? null,
         actorId: session.user.id,
         action: AuditAction.FORM_UPDATED,
         resourceType: AuditResourceType.FORM,
@@ -227,7 +231,11 @@ export async function DELETE(
 
     const { id } = await params
 
-    const { hasAccess, form: formRecord } = await checkFormAccess(id, session.user.id)
+    const {
+      hasAccess,
+      form: formRecord,
+      workspaceId: formWorkspaceId,
+    } = await checkFormAccess(id, session.user.id)
 
     if (!hasAccess || !formRecord) {
       return createErrorResponse('Form not found or access denied', 404)
@@ -238,7 +246,7 @@ export async function DELETE(
     logger.info(`Form ${id} deleted (soft delete)`)
 
     recordAudit({
-      workspaceId: null,
+      workspaceId: formWorkspaceId ?? null,
       actorId: session.user.id,
       action: AuditAction.FORM_DELETED,
       resourceType: AuditResourceType.FORM,

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

@@ -52,7 +52,7 @@ export async function checkWorkflowAccessForFormCreation(
 export async function checkFormAccess(
   formId: string,
   userId: string
-): Promise<{ hasAccess: boolean; form?: any }> {
+): Promise<{ hasAccess: boolean; form?: any; workspaceId?: string }> {
   const formData = await db
     .select({ form: form, workflowWorkspaceId: workflow.workspaceId })
     .from(form)
@@ -75,7 +75,9 @@ export async function checkFormAccess(
     action: 'admin',
   })
 
-  return authorization.allowed ? { hasAccess: true, form: formRecord } : { hasAccess: false }
+  return authorization.allowed
+    ? { hasAccess: true, form: formRecord, workspaceId: workflowWorkspaceId }
+    : { hasAccess: false }
 }
 
 export async function validateFormAuth(

apps/sim/app/api/mcp/servers/test-connection/route.ts

@@ -105,6 +105,16 @@ export const POST = withMcpAuth('write')(
         logger.warn(`[${requestId}] Some environment variables not found:`, { missingVars })
       }
 
+      // Re-validate domain after env var resolution
+      try {
+        validateMcpDomain(testConfig.url)
+      } catch (e) {
+        if (e instanceof McpDomainNotAllowedError) {
+          return createMcpErrorResponse(e, e.message, 403)
+        }
+        throw e
+      }
+
       const testSecurityPolicy = {
         requireConsent: false,
         auditLevel: 'none' as const,

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/mcp/mcp.tsx

@@ -109,13 +109,27 @@ const logger = createLogger('McpSettings')
 /**
  * Checks if a URL's hostname is in the allowed domains list.
  * Returns true if no allowlist is configured (null) or the domain matches.
+ * Env var references in the hostname bypass the check since the domain
+ * can't be determined until resolution — but env vars only in the path/query
+ * do NOT bypass the check.
  */
-const ENV_VAR_PATTERN = /\{\{[^}]+\}\}/
+const ENV_VAR_PATTERN = /\{\{[^}]+\}\}/g
+
+function hasEnvVarInHostname(url: string): boolean {
+  // If the entire URL is an env var, hostname is unknown
+  if (url.trim().replace(ENV_VAR_PATTERN, '').trim() === '') return true
+  const protocolEnd = url.indexOf('://')
+  if (protocolEnd === -1) return ENV_VAR_PATTERN.test(url)
+  const afterProtocol = url.substring(protocolEnd + 3)
+  const authorityEnd = afterProtocol.indexOf('/')
+  const authority = authorityEnd === -1 ? afterProtocol : afterProtocol.substring(0, authorityEnd)
+  return new RegExp(ENV_VAR_PATTERN.source).test(authority)
+}
 
 function isDomainAllowed(url: string | undefined, allowedDomains: string[] | null): boolean {
   if (allowedDomains === null) return true
   if (!url) return false
-  if (ENV_VAR_PATTERN.test(url)) return true
+  if (hasEnvVarInHostname(url)) return true
   try {
     const hostname = new URL(url).hostname.toLowerCase()
     return allowedDomains.includes(hostname)

apps/sim/lib/mcp/domain-check.test.ts

@@ -49,44 +49,147 @@ describe('isMcpDomainAllowed', () => {
     it('allows empty string URL', () => {
       expect(isMcpDomainAllowed('')).toBe(true)
     })
+
+    it('allows env var URLs', () => {
+      expect(isMcpDomainAllowed('{{MCP_SERVER_URL}}')).toBe(true)
+    })
+
+    it('allows URLs with env vars anywhere', () => {
+      expect(isMcpDomainAllowed('https://server.com/{{PATH}}')).toBe(true)
+    })
   })
 
   describe('when allowlist is configured', () => {
     beforeEach(() => {
       mockGetAllowedMcpDomainsFromEnv.mockReturnValue(['allowed.com', 'internal.company.com'])
     })
 
-    it('allows URLs on the allowlist', () => {
-      expect(isMcpDomainAllowed('https://allowed.com/mcp')).toBe(true)
-      expect(isMcpDomainAllowed('https://internal.company.com/tools')).toBe(true)
-    })
+    describe('basic domain matching', () => {
+      it('allows URLs on the allowlist', () => {
+        expect(isMcpDomainAllowed('https://allowed.com/mcp')).toBe(true)
+        expect(isMcpDomainAllowed('https://internal.company.com/tools')).toBe(true)
+      })
 
-    it('rejects URLs not on the allowlist', () => {
-      expect(isMcpDomainAllowed('https://evil.com/mcp')).toBe(false)
-    })
+      it('allows URLs with paths on allowlisted domains', () => {
+        expect(isMcpDomainAllowed('https://allowed.com/deep/path/to/mcp')).toBe(true)
+      })
 
-    it('rejects undefined URL (fail-closed)', () => {
-      expect(isMcpDomainAllowed(undefined)).toBe(false)
-    })
+      it('allows URLs with query params on allowlisted domains', () => {
+        expect(isMcpDomainAllowed('https://allowed.com/mcp?key=value&foo=bar')).toBe(true)
+      })
+
+      it('allows URLs with ports on allowlisted domains', () => {
+        expect(isMcpDomainAllowed('https://allowed.com:8080/mcp')).toBe(true)
+      })
 
-    it('rejects empty string URL (fail-closed)', () => {
-      expect(isMcpDomainAllowed('')).toBe(false)
+      it('allows HTTP URLs on allowlisted domains', () => {
+        expect(isMcpDomainAllowed('http://allowed.com/mcp')).toBe(true)
+      })
+
+      it('matches case-insensitively', () => {
+        expect(isMcpDomainAllowed('https://ALLOWED.COM/mcp')).toBe(true)
+        expect(isMcpDomainAllowed('https://Allowed.Com/mcp')).toBe(true)
+      })
+
+      it('rejects URLs not on the allowlist', () => {
+        expect(isMcpDomainAllowed('https://evil.com/mcp')).toBe(false)
+      })
+
+      it('rejects subdomains of allowed domains', () => {
+        expect(isMcpDomainAllowed('https://sub.allowed.com/mcp')).toBe(false)
+      })
+
+      it('rejects URLs with allowed domain in path only', () => {
+        expect(isMcpDomainAllowed('https://evil.com/allowed.com/mcp')).toBe(false)
+      })
     })
 
-    it('rejects malformed URLs', () => {
-      expect(isMcpDomainAllowed('not-a-url')).toBe(false)
+    describe('fail-closed behavior', () => {
+      it('rejects undefined URL', () => {
+        expect(isMcpDomainAllowed(undefined)).toBe(false)
+      })
+
+      it('rejects empty string URL', () => {
+        expect(isMcpDomainAllowed('')).toBe(false)
+      })
+
+      it('rejects malformed URLs', () => {
+        expect(isMcpDomainAllowed('not-a-url')).toBe(false)
+      })
+
+      it('rejects URLs with no protocol', () => {
+        expect(isMcpDomainAllowed('allowed.com/mcp')).toBe(false)
+      })
     })
 
-    it('matches case-insensitively', () => {
-      expect(isMcpDomainAllowed('https://ALLOWED.COM/mcp')).toBe(true)
+    describe('env var handling — hostname bypass', () => {
+      it('allows entirely env var URL', () => {
+        expect(isMcpDomainAllowed('{{MCP_SERVER_URL}}')).toBe(true)
+      })
+
+      it('allows env var URL with whitespace', () => {
+        expect(isMcpDomainAllowed('  {{MCP_SERVER_URL}}  ')).toBe(true)
+      })
+
+      it('allows multiple env vars composing the entire URL', () => {
+        expect(isMcpDomainAllowed('{{PROTOCOL}}{{HOST}}{{PATH}}')).toBe(true)
+      })
+
+      it('allows env var in hostname portion', () => {
+        expect(isMcpDomainAllowed('https://{{MCP_HOST}}/mcp')).toBe(true)
+      })
+
+      it('allows env var as subdomain', () => {
+        expect(isMcpDomainAllowed('https://{{TENANT}}.company.com/mcp')).toBe(true)
+      })
+
+      it('allows env var in port (authority)', () => {
+        expect(isMcpDomainAllowed('https://{{HOST}}:{{PORT}}/mcp')).toBe(true)
+      })
+
+      it('allows env var as the full authority', () => {
+        expect(isMcpDomainAllowed('https://{{MCP_HOST}}:{{MCP_PORT}}/api/mcp')).toBe(true)
+      })
     })
 
-    it('allows env var URLs without validating domain', () => {
-      expect(isMcpDomainAllowed('{{MCP_SERVER_URL}}')).toBe(true)
+    describe('env var handling — no bypass when only in path/query', () => {
+      it('rejects disallowed domain with env var in path', () => {
+        expect(isMcpDomainAllowed('https://evil.com/{{MCP_PATH}}')).toBe(false)
+      })
+
+      it('rejects disallowed domain with env var in query', () => {
+        expect(isMcpDomainAllowed('https://evil.com/mcp?key={{API_KEY}}')).toBe(false)
+      })
+
+      it('rejects disallowed domain with env var in fragment', () => {
+        expect(isMcpDomainAllowed('https://evil.com/mcp#{{SECTION}}')).toBe(false)
+      })
+
+      it('allows allowlisted domain with env var in path', () => {
+        expect(isMcpDomainAllowed('https://allowed.com/{{MCP_PATH}}')).toBe(true)
+      })
+
+      it('allows allowlisted domain with env var in query', () => {
+        expect(isMcpDomainAllowed('https://allowed.com/mcp?key={{API_KEY}}')).toBe(true)
+      })
+
+      it('rejects disallowed domain with env var in both path and query', () => {
+        expect(isMcpDomainAllowed('https://evil.com/{{PATH}}?token={{TOKEN}}&key={{KEY}}')).toBe(
+          false
+        )
+      })
     })
 
-    it('allows URLs with embedded env vars', () => {
-      expect(isMcpDomainAllowed('https://{{MCP_HOST}}/mcp')).toBe(true)
+    describe('env var security edge cases', () => {
+      it('rejects URL with env var only after allowed domain in path', () => {
+        expect(isMcpDomainAllowed('https://evil.com/allowed.com/{{VAR}}')).toBe(false)
+      })
+
+      it('rejects URL trying to use env var to sneak past domain check via userinfo', () => {
+        // https://evil.com@allowed.com would have hostname "allowed.com" per URL spec,
+        // but https://{{VAR}}@evil.com has env var in authority so it bypasses
+        expect(isMcpDomainAllowed('https://{{VAR}}@evil.com/mcp')).toBe(true)
+      })
     })
   })
 })
@@ -108,39 +211,71 @@ describe('validateMcpDomain', () => {
     it('does not throw for undefined URL', () => {
       expect(() => validateMcpDomain(undefined)).not.toThrow()
     })
+
+    it('does not throw for empty string', () => {
+      expect(() => validateMcpDomain('')).not.toThrow()
+    })
   })
 
   describe('when allowlist is configured', () => {
     beforeEach(() => {
       mockGetAllowedMcpDomainsFromEnv.mockReturnValue(['allowed.com'])
     })
 
-    it('does not throw for allowed URLs', () => {
-      expect(() => validateMcpDomain('https://allowed.com/mcp')).not.toThrow()
-    })
+    describe('basic validation', () => {
+      it('does not throw for allowed URLs', () => {
+        expect(() => validateMcpDomain('https://allowed.com/mcp')).not.toThrow()
+      })
 
-    it('throws McpDomainNotAllowedError for disallowed URLs', () => {
-      expect(() => validateMcpDomain('https://evil.com/mcp')).toThrow(McpDomainNotAllowedError)
-    })
+      it('throws McpDomainNotAllowedError for disallowed URLs', () => {
+        expect(() => validateMcpDomain('https://evil.com/mcp')).toThrow(McpDomainNotAllowedError)
+      })
 
-    it('throws for undefined URL (fail-closed)', () => {
-      expect(() => validateMcpDomain(undefined)).toThrow(McpDomainNotAllowedError)
-    })
+      it('throws for undefined URL (fail-closed)', () => {
+        expect(() => validateMcpDomain(undefined)).toThrow(McpDomainNotAllowedError)
+      })
 
-    it('throws for malformed URLs', () => {
-      expect(() => validateMcpDomain('not-a-url')).toThrow(McpDomainNotAllowedError)
-    })
+      it('throws for malformed URLs', () => {
+        expect(() => validateMcpDomain('not-a-url')).toThrow(McpDomainNotAllowedError)
+      })
 
-    it('includes the rejected domain in the error message', () => {
-      expect(() => validateMcpDomain('https://evil.com/mcp')).toThrow(/evil\.com/)
-    })
+      it('includes the rejected domain in the error message', () => {
+        expect(() => validateMcpDomain('https://evil.com/mcp')).toThrow(/evil\.com/)
+      })
 
-    it('does not throw for env var URLs', () => {
-      expect(() => validateMcpDomain('{{MCP_SERVER_URL}}')).not.toThrow()
+      it('includes "(empty)" in error for undefined URL', () => {
+        expect(() => validateMcpDomain(undefined)).toThrow(/\(empty\)/)
+      })
     })
 
-    it('does not throw for URLs with embedded env vars', () => {
-      expect(() => validateMcpDomain('https://{{MCP_HOST}}/mcp')).not.toThrow()
+    describe('env var handling', () => {
+      it('does not throw for entirely env var URL', () => {
+        expect(() => validateMcpDomain('{{MCP_SERVER_URL}}')).not.toThrow()
+      })
+
+      it('does not throw for env var in hostname', () => {
+        expect(() => validateMcpDomain('https://{{MCP_HOST}}/mcp')).not.toThrow()
+      })
+
+      it('does not throw for env var in authority', () => {
+        expect(() => validateMcpDomain('https://{{HOST}}:{{PORT}}/mcp')).not.toThrow()
+      })
+
+      it('throws for disallowed URL with env var only in path', () => {
+        expect(() => validateMcpDomain('https://evil.com/{{MCP_PATH}}')).toThrow(
+          McpDomainNotAllowedError
+        )
+      })
+
+      it('throws for disallowed URL with env var only in query', () => {
+        expect(() => validateMcpDomain('https://evil.com/mcp?key={{API_KEY}}')).toThrow(
+          McpDomainNotAllowedError
+        )
+      })
+
+      it('does not throw for allowed URL with env var in path', () => {
+        expect(() => validateMcpDomain('https://allowed.com/{{PATH}}')).not.toThrow()
+      })
     })
   })
 })