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

LLM-writable ids were interpolated into request paths, so a value like
`../../files/victim` re-aimed an authenticated request — carrying the user's
OAuth token or the workspace's Supabase service-role key — at a different
resource on the same host, including on DELETE.

The headline case is Supabase `encodeStoragePath`, which read as sanitisation
and was a no-op for traversal: it split the object key on `/` and ran
`encodeURIComponent` over each piece, but `.` and `..` are unreserved, so
`../..` came back byte-for-byte unchanged and the URL parser removed the dot
segments after decoding.

Single-segment ids go through `safeUrlPathSegment`. The two genuinely
hierarchical values — Supabase storage keys and Google Contacts `resourceName` —
go through `safeUrlPath` from #7262, so the tree carries one multi-segment guard
rather than a second near-duplicate. That also adopts `safeUrlPath`'s rejection
of empty segments: a leading or doubled separator addresses a different object
than the caller wrote, and the upload operation normalizes its own trailing
separator, so no real key needs one.

Supabase `table` is left alone — `validateDatabaseIdentifier` already refuses
anything outside the SQL identifier alphabet, so it is not in this risk class.

Each service gets a `path_safety.test.ts` over a shared harness that enumerates
(tool, parameter) pairs from the barrel and fuzzes one parameter at a time with
its siblings held safe. Every value that encoding cannot neutralize is asserted
to throw and to name its parameter, because a shape-only assertion is blind to a
dot segment in the final path position: `https://x/a/.` normalizes to
`https://x/a/` with the segment count intact. Discovery also harvests the
literals a URL builder compares against, so a parameter reachable only on one
branch cannot hide, and reports any tool whose URL will not build at all instead
of dropping it.

Author: waleedlatif1

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

@@ -0,0 +1,377 @@
+/**
+ * Shared harness for the per-service `path_safety.test.ts` suites.
+ *
+ * Each suite enumerates its service's **(tool, parameter) pairs** from the
+ * barrel rather than listing them by hand, so a newly added tool — or a new id
+ * parameter on an existing tool — is covered without anyone remembering to
+ * register it.
+ *
+ * Three details are load-bearing, and each exists because an earlier version of
+ * this harness got it wrong.
+ *
+ * **One parameter at a time.** The first version filled *every* string
+ * parameter with the same hostile value and swallowed the throw, so the moment
+ * one parameter was guarded its siblings stopped being exercised: reverting the
+ * guard on `google_drive_unshare`'s `permissionId` while its sibling `fileId`
+ * stayed guarded left the suite reporting 285/285 green. Each pair is therefore
+ * driven on its own, with every sibling held at a safe value.
+ *
+ * **Rejection, not shape.** A shape-only assertion is blind to a dot segment in
+ * the *final* position: `https://x/a/.` normalizes to `https://x/a/`, which
+ * preserves the segment count and every other segment, so the check passes with
+ * the guard removed. Tools whose path ends in the guarded id — the Drive
+ * `delete_*` family, `box_sign_get_request` — are exactly where that blind spot
+ * lives, so every value in {@link MUST_REJECT} is asserted to *throw*.
+ *
+ * **Every branch.** A parameter that only reaches the path on one branch of a
+ * conditional builder is invisible to a single-shot probe. Discovery therefore
+ * reads the literals the builder compares against out of its own source and
+ * probes each one.
+ *
+ * Every assertion resolves the built URL with `new URL(...)` — the same
+ * normalization `fetch` performs — instead of string-matching the template
+ * output. String matching is exactly what let dot-segment traversal through:
+ * the template looks correct and the parser rewrites it afterwards.
+ */
+import { getErrorMessage } from '@sim/utils/errors'
+import { expect, it } from 'vitest'
+
+/**
+ * The structural shape this harness needs from a tool.
+ *
+ * Declared locally rather than as `ToolConfig<any, any>` so the harness carries
+ * no `any`: the barrels export tools over many different parameter types, and
+ * nothing here needs to know any of them beyond "there are declared params and
+ * a URL builder".
+ */
+export interface PathTool {
+  id: string
+  params?: Record<string, { type?: string } | undefined>
+  buildUrl: (params: Record<string, unknown>) => string
+}
+
+/** Narrows an unknown barrel export to the shape this harness can drive. */
+function asPathTool(value: unknown): PathTool | undefined {
+  if (typeof value !== 'object' || value === null) return undefined
+
+  const candidate = value as {
+    id?: unknown
+    params?: Record<string, { type?: string } | undefined>
+    request?: { url?: unknown }
+  }
+
+  if (typeof candidate.id !== 'string' || typeof candidate.request?.url !== 'function') {
+    return undefined
+  }
+
+  return {
+    id: candidate.id,
+    params: candidate.params,
+    buildUrl: candidate.request.url as (params: Record<string, unknown>) => string,
+  }
+}
+
+/**
+ * Values that no guard may ever accept, because each one either *is* a dot
+ * segment or contains one, and no encoding neutralizes that — the URL parser
+ * removes it after decoding.
+ *
+ * These are asserted to throw. A shape check alone cannot see them when the
+ * parameter sits in the final path position.
+ */
+export const MUST_REJECT = [
+  '..',
+  '.',
+  '  ..  ',
+  '../',
+  './',
+  '../../about',
+  'abc/../../../drives',
+  'abc/items/../../../v2/other',
+  '\\..\\..',
+] as const
+
+/**
+ * Values a guard may legitimately *accept* — percent-encoding renders them
+ * inert — but which must never restructure the resolved URL. `%2f` is not
+ * decoded before dot segments are removed, and `?`/`#` are escaped, so these
+ * survive as opaque text inside one segment.
+ */
+export const MUST_NOT_RESHAPE = [
+  '..%2f..%2fabout',
+  'abc?injectedProbe=attacker',
+  'abc#fragment',
+] as const
+
+/** A single parameter of a single tool that reaches a URL path segment. */
+export interface PathParam {
+  label: string
+  tool: PathTool
+  paramName: string
+  /**
+   * The sibling values that make this parameter reach the path — the service's
+   * fixed params plus, where the builder branches, the literal that selects the
+   * branch the parameter lives on.
+   */
+  context: Record<string, unknown>
+}
+
+/** A tool whose URL will not build even from all-safe values. */
+export interface UnbuildableTool {
+  id: string
+  reason: string
+}
+
+const SAFE_ID = 'SAFEID'
+
+/** Sentinel for the one parameter under test, so its slots are identifiable. */
+const PROBE_ID = 'PROBEID'
+
+/** Not a declared parameter — leaves every real one at its safe value. */
+const ALL_SAFE = '__all_safe__'
+
+/**
+ * Fills every declared parameter with a type-appropriate safe value, then
+ * overrides the single parameter under test.
+ */
+function buildParams(
+  tool: PathTool,
+  paramName: string,
+  value: string,
+  fixed: Record<string, unknown>
+): Record<string, unknown> {
+  const params: Record<string, unknown> = {}
+  for (const [name, def] of Object.entries(tool.params ?? {})) {
+    const type = def?.type
+    if (type === 'json' || type === 'array') {
+      params[name] = []
+    } else if (type === 'number') {
+      params[name] = 1
+    } else if (type === 'boolean') {
+      params[name] = false
+    } else {
+      params[name] = SAFE_ID
+    }
+  }
+  Object.assign(params, fixed)
+  params[paramName] = value
+  return params
+}
+
+function buildUrl(
+  tool: PathTool,
+  paramName: string,
+  value: string,
+  fixed: Record<string, unknown>
+): URL {
+  return new URL(tool.buildUrl(buildParams(tool, paramName, value, fixed)))
+}
+
+/**
+ * Harvests the string literals a URL builder compares against, straight from
+ * its own source.
+ *
+ * Some builders put a path parameter on only one branch of a conditional — a
+ * second identifier that appears only when `action === 'unblock'`, say — and
+ * the discriminating parameter often declares no enum, only prose in its
+ * description. Probing with a single default value never enters that branch, so
+ * the parameter is invisible to discovery and silently untested.
+ *
+ * Reading the comparands out of the function source means every branch is
+ * probed, and a branch added later is picked up without editing any test.
+ */
+function branchLiterals(tool: PathTool): string[] {
+  const source = String(tool.buildUrl)
+  const literals = new Set<string>()
+
+  for (const pattern of [
+    /[=!]==\s*['"`]([^'"`\n]{1,64})['"`]/g,
+    /['"`]([^'"`\n]{1,64})['"`]\s*[=!]==/g,
+    /case\s+['"`]([^'"`\n]{1,64})['"`]/g,
+  ]) {
+    for (const match of source.matchAll(pattern)) literals.add(match[1])
+  }
+
+  return [...literals]
+}
+
+/**
+ * Enumerates every (tool, parameter) pair of a service whose value lands in a
+ * URL **path** segment.
+ *
+ * A parameter that only ever reaches the query string, or a tool with a static
+ * URL, is not in this risk class and is left out — the probe decides that by
+ * looking for the sentinel in `pathname`, never in the full URL.
+ */
+export function discoverPathParams(
+  barrel: Record<string, unknown>,
+  idPrefix: string,
+  fixed: Record<string, unknown> = {}
+): { covered: PathParam[]; unbuildable: UnbuildableTool[] } {
+  const covered: PathParam[] = []
+  const unbuildable: UnbuildableTool[] = []
+
+  for (const exported of Object.values(barrel)) {
+    const tool = asPathTool(exported)
+    if (!tool || !tool.id.startsWith(idPrefix)) continue
+
+    const names = Object.keys(tool.params ?? {}).filter((name) => !(name in fixed))
+
+    /**
+     * Every sibling assignment worth probing: the plain one, then each
+     * parameter pinned to each literal the builder branches on.
+     */
+    const branches: Record<string, unknown>[] = [{}]
+    for (const literal of branchLiterals(tool)) {
+      for (const name of names) branches.push({ [name]: literal })
+    }
+
+    /**
+     * Buildability is decided from an all-safe build, independent of the
+     * per-parameter probes. A probe is *meant* to throw for a guarded
+     * parameter, so treating a failed probe as an unbuildable tool would make
+     * this list noisy; but a tool whose URL will not build at all must never
+     * vanish from coverage silently.
+     */
+    let buildable = false
+    let firstFailure = ''
+    for (const branch of branches) {
+      try {
+        buildUrl(tool, ALL_SAFE, SAFE_ID, { ...fixed, ...branch })
+        buildable = true
+        break
+      } catch (error) {
+        if (!firstFailure) firstFailure = getErrorMessage(error, 'unknown error')
+      }
+    }
+
+    if (!buildable) unbuildable.push({ id: tool.id, reason: firstFailure || 'URL did not build' })
+
+    for (const name of names) {
+      let match: Record<string, unknown> | undefined
+
+      for (const branch of branches) {
+        if (name in branch) continue
+        const context = { ...fixed, ...branch }
+        try {
+          if (buildUrl(tool, name, PROBE_ID, context).pathname.includes(PROBE_ID)) {
+            match = context
+            break
+          }
+        } catch {
+          // A guarded parameter is expected to throw for some probes; another
+          // branch may still reach it, so keep going.
+        }
+      }
+
+      if (match) {
+        covered.push({ label: `${tool.id} :: ${name}`, tool, paramName: name, context: match })
+      }
+    }
+  }
+
+  return { covered, unbuildable }
+}
+
+/**
+ * Normalizes an error message and a parameter name to bare lowercase letters so
+ * a guard can be credited with naming its parameter however it spells it.
+ *
+ * A few parameters are refused by a stricter service-specific validator that
+ * predates these guards and spells the name in prose — Supabase's
+ * `functionName` is reported as *"Invalid function name"*. That is an equally
+ * correct outcome and should still count as naming the offender, so both sides
+ * are stripped of non-letters before the comparison.
+ */
+function namesParam(message: string, paramName: string): boolean {
+  const strip = (text: string) => text.toLowerCase().replaceAll(/[^a-z]/g, '')
+  return strip(message).includes(strip(paramName))
+}
+
+export interface TraversalOptions {
+  origin: string
+  /** The fixed API prefix every route of the service shares. */
+  basePath: string
+}
+
+/** Asserts the traversal invariant for one (tool, parameter) pair. */
+export function itResistsTraversal(
+  { tool, paramName, context }: PathParam,
+  { origin, basePath }: TraversalOptions
+): void {
+  const baselinePath = buildUrl(tool, paramName, PROBE_ID, context).pathname
+  const prefix = baselinePath.split('/').slice(0, baselinePath.split('/').indexOf(PROBE_ID))
+
+  it('stays under the service API prefix', () => {
+    expect(baselinePath.startsWith(basePath)).toBe(true)
+  })
+
+  /**
+   * The rejection assertion, not a shape assertion. A trailing `.` preserves
+   * the shape of the resolved path exactly, so only "did it throw" can see it.
+   */
+  it.each(MUST_REJECT)('rejects %j outright, naming the parameter', (value) => {
+    let message = ''
+    try {
+      buildUrl(tool, paramName, value, context)
+    } catch (error) {
+      message = getErrorMessage(error, 'unknown error')
+    }
+
+    expect(message, `${paramName} accepted ${JSON.stringify(value)}`).not.toBe('')
+    expect(namesParam(message, paramName), `error did not name ${paramName}: ${message}`).toBe(true)
+  })
+
+  it.each(MUST_NOT_RESHAPE)('renders %j inert without reshaping the path', (value) => {
+    let url: URL
+    try {
+      url = buildUrl(tool, paramName, value, context)
+    } catch {
+      return
+    }
+
+    expect(url.origin).toBe(origin)
+    expect(url.pathname.startsWith(basePath)).toBe(true)
+
+    const segments = url.pathname.split('/')
+    expect(segments.slice(0, prefix.length)).toEqual(prefix)
+    expect(segments).not.toContain('..')
+    expect(segments).not.toContain('.')
+    expect(url.searchParams.get('injectedProbe')).toBeNull()
+  })
+
+  /**
+   * Padding must never change which resource is addressed. Refusing it outright
+   * is an equally correct outcome — `validateDatabaseIdentifier` guards
+   * Supabase's `table` and deliberately admits no whitespace at all — so the
+   * assertion is "same path or no path", not "always trims".
+   */
+  it('does not let surrounding whitespace alter the value', () => {
+    let url: URL
+    try {
+      url = buildUrl(tool, paramName, `  ${PROBE_ID}  `, context)
+    } catch {
+      return
+    }
+
+    expect(url.pathname).toBe(baselinePath)
+  })
+}
+
+/**
+ * Asserts that real-world values reach the wire byte-for-byte, so a guard can
+ * never be tightened into breaking legitimate callers.
+ */
+export function itPassesLegitimateValues(
+  { tool, paramName, context }: PathParam,
+  { values, fixed = {} }: { values: readonly string[]; fixed?: Record<string, unknown> }
+): void {
+  const merged = { ...context, ...fixed }
+  const baseline = buildUrl(tool, paramName, PROBE_ID, merged).pathname
+
+  it.each(values)('passes %j through unchanged', (value) => {
+    expect(decodeURIComponent(buildUrl(tool, paramName, value, merged).pathname)).toBe(
+      decodeURIComponent(baseline).split(PROBE_ID).join(value)
+    )
+  })
+}

apps/sim/tools/box/copy_file.ts

@@ -1,4 +1,5 @@
 import type { ToolConfig } from '@/tools/types'
+import { safeUrlPathSegment } from '@/tools/url-path'
 import type { BoxCopyFileParams, BoxUploadFileResponse } from './types'
 import { UPLOAD_FILE_OUTPUT_PROPERTIES } from './types'
 
@@ -41,7 +42,8 @@ export const boxCopyFileTool: ToolConfig<BoxCopyFileParams, BoxUploadFileRespons
   },
 
   request: {
-    url: (params) => `https://api.box.com/2.0/files/${params.fileId.trim()}/copy`,
+    url: (params) =>
+      `https://api.box.com/2.0/files/${safeUrlPathSegment(params.fileId, 'fileId')}/copy`,
     method: 'POST',
     headers: (params) => ({
       Authorization: `Bearer ${params.accessToken}`,