fix(supabase): report storage path-guard failures as 400, not 500

cubic found that adding throwing guards to encodeStoragePath and
encodeStorageSegment changed how caller mistakes are attributed. Nothing in
those helpers threw before this branch, so the new rejections fall through to
the generic handler in executeSupabaseStorageUpload and surface as HTTP 500 —
blaming the server for a caller's '..' — while validateSupabaseProjectId one
line above already reports a bad project id as 400.

Maps guard rejections to 400 at both call sites, keeping the guard's named
message. storage-get-public-url surfaces the same rejection through its own
result shape rather than throwing out of the operation.

Pinned with tests over traversal, empty-segment and bad-bucket inputs asserting
400 and that no provider request is made. Verified non-vacuous: removing the
mapping fails all four.

Author: waleedlatif1

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) {