fix(tools): reject path traversal in Drive, BigQuery, Box, Supabase and Contacts ids

apps/sim/lib/internal/github/operations.test.ts

@@ -13,6 +13,7 @@ vi.mock('@/lib/core/security/input-validation.server', () => ({
   validateUrlWithDNS: mocks.validateUrlWithDNS,
 }))
 
+import { GitHubOperationError } from '@/lib/internal/github/errors'
 import { getGitHubLatestCommit } from '@/lib/internal/github/operations'
 
 describe('getGitHubLatestCommit', () => {
@@ -61,3 +62,40 @@ describe('getGitHubLatestCommit', () => {
     )
   })
 })
+
+/**
+ * The path guards throw a plain `Error`, which `executeGitHubTool` maps to 500.
+ * Every value they reject is caller-supplied, so it must surface as a 400 with
+ * the guard's own named message rather than as a server failure.
+ */
+describe('getGitHubLatestCommit path validation', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mocks.validateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '203.0.113.1' })
+  })
+
+  it.each([
+    ['..', 'path traversal is not allowed'],
+    ['../../orgs/secret', 'cannot contain a path separator'],
+  ])('reports owner %j as a client error, not a server failure', async (owner, message) => {
+    const error = await getGitHubLatestCommit(
+      { owner, repo: 'sim', apiKey: 'token' },
+      { requestId: 'request-1' }
+    ).catch((caught: unknown) => caught)
+
+    expect(error).toBeInstanceOf(GitHubOperationError)
+    expect((error as GitHubOperationError).status).toBe(400)
+    expect((error as GitHubOperationError).message).toContain(message)
+    expect(mocks.secureFetchWithPinnedIP).not.toHaveBeenCalled()
+  })
+
+  it('rejects a branch that is a bare dot segment', async () => {
+    const error = await getGitHubLatestCommit(
+      { owner: 'simstudioai', repo: 'sim', branch: '..', apiKey: 'token' },
+      { requestId: 'request-1' }
+    ).catch((caught: unknown) => caught)
+
+    expect(error).toBeInstanceOf(GitHubOperationError)
+    expect((error as GitHubOperationError).status).toBe(400)
+  })
+})

apps/sim/lib/internal/github/operations.ts

@@ -1,4 +1,5 @@
 import { createLogger } from '@sim/logger'
+import { getErrorMessage } from '@sim/utils/errors'
 import { isRecordLike } from '@sim/utils/object'
 import {
   secureFetchWithPinnedIP,
@@ -19,6 +20,11 @@ import type {
 } from '@/tools/github/types'
 import { secureGitHubRequest } from '@/tools/github/utils.server'
 import type { ToolResponse } from '@/tools/types'
+import {
+  safeEncodedUrlPathSegment,
+  safeUrlPathSegment,
+  strictUrlPathSegment,
+} from '@/tools/url-path'
 
 const logger = createLogger('GitHubLatestCommitOperation')
 const MAX_COMMIT_RESPONSE_BYTES = 10 * 1024 * 1024
@@ -97,8 +103,40 @@ function githubHeaders(apiKey: string): Record<string, string> {
   }
 }
 
+/**
+ * Runs the path guards for one provider URL, reporting a rejected value as a
+ * client error.
+ *
+ * The guards in `@/tools/url-path` throw a plain `Error`, and the catch in
+ * `executeGitHubTool` maps anything that is not a `GitHubOperationError` to
+ * 500. Every value they reject is caller-supplied — an `owner` of `..`, a
+ * `branch` carrying a separator — so reporting it as a server failure both
+ * misattributes the fault and hides the guard's message behind a generic
+ * status. 400 is the accurate answer, and it keeps the named
+ * "<param> cannot be ..." text reaching the caller who can act on it.
+ */
+function buildGuardedUrl(build: () => string): string {
+  try {
+    return build()
+  } catch (error) {
+    throw new GitHubOperationError(getErrorMessage(error, 'Invalid GitHub request path'), 400)
+  }
+}
+
+/**
+ * The pull-request URL, and the base for the comment and review URLs built from
+ * it.
+ *
+ * Uses the strict guards even though this same URL is also fetched with a GET
+ * to read the head SHA: every caller reaches it on the way to creating a
+ * comment or a review, so the operation as a whole changes state and must not
+ * have a padded identifier quietly resolved to a real pull request.
+ */
 function pullRequestUrl(params: CreateCommentParams): string {
-  return `${GITHUB_API_BASE}/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}`
+  return buildGuardedUrl(
+    () =>
+      `${GITHUB_API_BASE}/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}`
+  )
 }
 
 function isFileCommentRequest(params: CreateCommentParams): boolean {
@@ -352,10 +390,12 @@ export async function getGitHubLatestCommit(
   context: GitHubOperationContext
 ): Promise<LatestCommitResponse> {
   context.signal?.throwIfAborted()
-  const owner = encodeURIComponent(input.owner)
-  const repo = encodeURIComponent(input.repo)
-  const revision = encodeURIComponent(input.branch || 'HEAD')
-  const commitUrl = `https://api.github.com/repos/${owner}/${repo}/commits/${revision}`
+  const commitUrl = buildGuardedUrl(() => {
+    const owner = safeUrlPathSegment(input.owner, 'owner')
+    const repo = safeUrlPathSegment(input.repo, 'repo')
+    const revision = safeEncodedUrlPathSegment(input.branch || 'HEAD', 'branch')
+    return `https://api.github.com/repos/${owner}/${repo}/commits/${revision}`
+  })
   const validation = await validateUrlWithDNS(commitUrl, 'commitUrl')
   context.signal?.throwIfAborted()
   if (!validation.isValid || !validation.resolvedIP) {

apps/sim/lib/internal/supabase/operations.test.ts

@@ -124,3 +124,44 @@ describe('executeSupabaseStorageUpload', () => {
     ).rejects.toMatchObject({ name: 'AbortError' })
   })
 })
+
+/**
+ * The storage path guards throw a plain `Error` on caller-supplied values.
+ * Before they existed nothing here threw, so an unmapped throw would surface as
+ * HTTP 500 and blame the server for the caller's `..` — while
+ * `validateSupabaseProjectId` one line above already reports a bad project id
+ * as 400. These assertions pin the attribution and the named message.
+ */
+describe('executeSupabaseStorageUpload path-guard failures', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mocks.assertToolFileAccess.mockResolvedValue(null)
+    vi.stubGlobal('fetch', vi.fn().mockResolvedValue(Response.json({ Key: 'k' })))
+  })
+
+  it.each([
+    ['a traversal path', { path: '../..', fileName: 'x.txt' }],
+    ['an empty path segment', { path: 'a//b', fileName: 'x.txt' }],
+    ['a bucket that is a dot segment', { bucket: '..', fileName: 'x.txt' }],
+    ['a bucket carrying a separator', { bucket: 'a/b', fileName: 'x.txt' }],
+  ])('reports %s as 400, not 500', async (_label, overrides) => {
+    const response = await executeSupabaseStorageUpload(
+      { ...BASE_INPUT, fileData: 'hello', ...overrides },
+      { userId: 'user-1', requestId: 'request-1' }
+    )
+
+    expect(response.status).toBe(400)
+    await expect(response.json()).resolves.toMatchObject({ success: false })
+    expect(fetch).not.toHaveBeenCalled()
+  })
+
+  it('names the offending parameter in the error', async () => {
+    const response = await executeSupabaseStorageUpload(
+      { ...BASE_INPUT, bucket: '..', fileData: 'hello' },
+      { userId: 'user-1', requestId: 'request-1' }
+    )
+    const body = (await response.json()) as { error?: string }
+
+    expect(body.error).toMatch(/bucket/)
+  })
+})

apps/sim/lib/internal/supabase/operations.ts

@@ -120,8 +120,23 @@ export async function executeSupabaseStorageUpload(
     const fullPath = input.path
       ? `${input.path.endsWith('/') ? input.path : `${input.path}/`}${input.fileName}`
       : input.fileName
-    const encodedBucket = encodeStorageSegment(input.bucket)
-    const encodedPath = encodeStoragePath(fullPath)
+    /**
+     * The storage path guards throw a plain `Error`, and the catch at the end of
+     * this function maps anything that is not a payload-size failure to 500.
+     * Every value they reject is caller-supplied — a `bucket` of `..`, a `path`
+     * with an empty segment — so reporting it as a server fault both
+     * misattributes the blame and buries the guard's named message behind a
+     * generic status. 400 is the accurate answer, and it matches how
+     * `validateSupabaseProjectId` above already reports a bad project id.
+     */
+    let encodedBucket: string
+    let encodedPath: string
+    try {
+      encodedBucket = encodeStorageSegment(input.bucket)
+      encodedPath = encodeStoragePath(fullPath)
+    } catch (error) {
+      return failureResponse(getErrorMessage(error, 'Invalid storage path'), 400)
+    }
     const baseUrl = `https://${projectValidation.sanitized}.supabase.co/storage/v1/object`
     const headers: Record<string, string> = {
       apikey: input.apiKey,

apps/sim/lib/internal/supabase/operations/storage-get-public-url.ts

@@ -1,12 +1,29 @@
+import { getErrorMessage } from '@sim/utils/errors'
 import type { InternalToolOperationImplementation } from '@/lib/internal/tool-operations/types'
 import type { SupabaseStorageGetPublicUrlParams } from '@/tools/supabase/types'
 import { encodeStoragePath, encodeStorageSegment, supabaseBaseUrl } from '@/tools/supabase/utils'
 
 export const executeStorageGetPublicUrlOperation: InternalToolOperationImplementation<
   SupabaseStorageGetPublicUrlParams
 > = async (params: SupabaseStorageGetPublicUrlParams) => {
-  const bucket = encodeStorageSegment(params.bucket)
-  const path = encodeStoragePath(params.path)
+  /**
+   * Same reasoning as the upload operation: the path guards throw on
+   * caller-supplied values, and an uncaught throw here escapes as an opaque
+   * server failure rather than the guard's named message. This operation
+   * reports failure in its own result shape, so the rejection is surfaced there.
+   */
+  let bucket: string
+  let path: string
+  try {
+    bucket = encodeStorageSegment(params.bucket)
+    path = encodeStoragePath(params.path)
+  } catch (error) {
+    return {
+      success: false,
+      output: { message: getErrorMessage(error, 'Invalid storage path'), publicUrl: '' },
+      error: getErrorMessage(error, 'Invalid storage path'),
+    }
+  }
   let publicUrl = `${supabaseBaseUrl(params.projectId)}/storage/v1/object/public/${bucket}/${path}`
 
   if (params.download) {

apps/sim/tools/__tests__/path-safety-matcher.test.ts

@@ -0,0 +1,50 @@
+/**
+ * @vitest-environment node
+ *
+ * Contract for `namesParam`, the matcher behind every "names the parameter"
+ * assertion in the path-safety suites.
+ *
+ * It gets its own test because the assertion it powers is only as strong as it
+ * is, and the previous substring implementation passed all seven suites while
+ * accepting a message that named the **wrong** parameter. A weakness here is
+ * invisible everywhere else.
+ */
+import { describe, expect, it } from 'vitest'
+import { namesParam } from '@/tools/__tests__/path-safety'
+
+describe('namesParam', () => {
+  it.each([
+    ['projectId must not have leading or trailing whitespace', 'projectId'],
+    ['signRequestId must not have leading or trailing whitespace', 'signRequestId'],
+    ['bucket cannot contain a path separator', 'bucket'],
+    ['path cannot contain an empty or whitespace-only path segment', 'path'],
+    ['tableId cannot be "." (path traversal is not allowed)', 'tableId'],
+    ['Invalid table: must start with a letter or underscore', 'table'],
+  ])('accepts %j as naming %j', (message, paramName) => {
+    expect(namesParam(message, paramName)).toBe(true)
+  })
+
+  /**
+   * A stricter service validator spells the name as prose. Joining adjacent
+   * tokens is what keeps that a correct naming rather than a near-miss.
+   */
+  it('accepts a prose spelling split across words', () => {
+    expect(namesParam('Invalid function name: must contain only letters', 'functionName')).toBe(
+      true
+    )
+  })
+
+  /** Each of these was accepted by the previous substring implementation. */
+  it.each([
+    ['a generic message', 'Invalid input', 'id'],
+    ['a message naming a different parameter', 'projectId cannot be ".."', 'id'],
+    ['a longer parameter name containing this one', 'tableId cannot be "."', 'table'],
+    ['the name as a substring of an unrelated word', 'pathological failure', 'path'],
+  ])('rejects %s', (_label, message, paramName) => {
+    expect(namesParam(message, paramName)).toBe(false)
+  })
+
+  it('rejects an unrelated message outright', () => {
+    expect(namesParam('Something went wrong', 'path')).toBe(false)
+  })
+})