fix(review): address the five findings from the review round

Four are side effects of earlier fixes in this PR:

- the '!= null' guard added so an explicit 0 would survive also admitted
  '', sending a bare 'timeout=' to Apify on a direct tool call
- the pagination coercion turned a whitespace-only input into 0, slipping
  past the tool's own non-negative-integer check
- the dot-segment guard trimmed before comparing, so a GCS object legally
  named ' ..' was rejected even though '%20..' cannot collapse a segment
- the 7MiB media cap threw inside a try whose catch reported success with
  no file, making an over-limit recording look like an empty one; it now
  returns 413, and the other errors that catch handles are unchanged

The fifth is pre-existing, exposed by the new validator: BUILT_IN_PATH is a
plain object literal, so an inherited key like '__proto__' resolved up the
prototype chain and threw a TypeError. Fixed in all three copies -- both
routes and the poller, where 'in' walks the chain the same way.

Author: waleedlatif1

apps/sim/app/api/tools/google_vault/download-export-file/path-safety.test.ts

@@ -36,7 +36,20 @@ import { POST } from '@/app/api/tools/google_vault/download-export-file/route'
 const { mockValidateUrlWithDNS, mockSecureFetchWithPinnedIP } = inputValidationMockFns
 
 const PINNED_IP = '93.184.216.34'
-const REJECTED = ['..', '.', '  ..  '] as const
+const REJECTED = ['..', '.'] as const
+
+/**
+ * Whitespace-padded dot segments. GCS documents any Unicode character as legal
+ * in an object name, so `' ..'` is a real, addressable object — and the server
+ * does not trim, so `' ..'` and `'..'` are *different* objects. Rejecting the
+ * padded form (which trimming before the comparison did) is a false rejection,
+ * and rewriting it would silently address the wrong object. It is safe to allow
+ * because `encodeURIComponent` turns the padding into `%20`, and the WHATWG
+ * parser only removes a segment that is *exactly* `.` or `..` — the same
+ * argument `safeUrlPath`'s `preserveOuterWhitespace` option records for
+ * Supabase Storage keys.
+ */
+const PADDED_DOT_NAMES = [' ..', '.. ', ' .. ', ' . ', '\t..'] as const
 
 function downloadResponse() {
   return {
@@ -143,6 +156,74 @@ describe('POST /api/tools/google_vault/download-export-file traversal safety', (
     ])
   })
 
+  it.each(PADDED_DOT_NAMES)(
+    'accepts the legal padded object name %j and keeps the path shape intact',
+    async (objectName) => {
+      mockSecureFetchWithPinnedIP.mockResolvedValueOnce(downloadResponse())
+
+      const response = await POST(
+        createMockRequest('POST', {
+          accessToken: 'token-123',
+          matterId: 'matter-1',
+          bucketName: 'vault-bucket',
+          objectName,
+        })
+      )
+      expect(response.status).toBe(200)
+
+      const url = new URL(requestedUrl())
+      expect(url.pathname.split('/')).toEqual([
+        '',
+        'storage',
+        'v1',
+        'b',
+        'vault-bucket',
+        'o',
+        encodeURIComponent(objectName),
+      ])
+      expect(decodeURIComponent(url.pathname.split('/')[6])).toBe(objectName)
+    }
+  )
+
+  it.each(PADDED_DOT_NAMES)(
+    'accepts the legal padded bucket name %j and keeps the path shape intact',
+    async (bucketName) => {
+      mockSecureFetchWithPinnedIP.mockResolvedValueOnce(downloadResponse())
+
+      const response = await POST(
+        createMockRequest('POST', {
+          accessToken: 'token-123',
+          matterId: 'matter-1',
+          bucketName,
+          objectName: 'exports/file.zip',
+        })
+      )
+      expect(response.status).toBe(200)
+
+      const url = new URL(requestedUrl())
+      expect(url.pathname.split('/')).toEqual([
+        '',
+        'storage',
+        'v1',
+        'b',
+        encodeURIComponent(bucketName),
+        'o',
+        'exports%2Ffile.zip',
+      ])
+    }
+  )
+
+  it('proves the padded forms cannot pop a segment while the bare forms can', () => {
+    const build = (value: string) =>
+      new URL(`https://storage.googleapis.com/storage/v1/b/bkt/o/${encodeURIComponent(value)}`)
+
+    for (const padded of PADDED_DOT_NAMES) {
+      expect(build(padded).pathname.split('/')).toHaveLength(7)
+    }
+    expect(build('..').pathname.split('/')).toHaveLength(6)
+    expect(build('.').pathname.split('/')[6]).toBe('')
+  })
+
   it('preserves an interior ".." component, which never forms a URL segment', async () => {
     mockSecureFetchWithPinnedIP.mockResolvedValueOnce(downloadResponse())
 

apps/sim/app/api/tools/google_vault/download-export-file/route.ts

@@ -29,13 +29,17 @@ const logger = createLogger('GoogleVaultDownloadExportFileAPI')
  * token attached. Only rejection closes that, and only the whole value can do
  * it: an interior `..` is encoded into the same segment and stays inert.
  *
- * The check trims before comparing but the caller still sends the untrimmed
- * value, so no legitimate name is silently rewritten.
+ * The comparison is against the **raw** value, deliberately not a trimmed one.
+ * GCS documents any Unicode character as legal in an object name and does not
+ * trim, so `' ..'` is a real object distinct from `'..'`; rejecting it is a
+ * false rejection of a legal name. It is also unnecessary — the padding
+ * encodes to `%20`/`%09` and the WHATWG parser removes only a segment that is
+ * *exactly* `.` or `..`, so `%20..` stays inert text. This is the same
+ * reasoning `safeUrlPath` records for its `preserveOuterWhitespace` option.
  */
 function assertNotDotSegment(value: string, paramName: string): void {
-  const trimmed = value.trim()
-  if (trimmed === '.' || trimmed === '..') {
-    throw new Error(`${paramName} cannot be "${trimmed}" (path traversal is not allowed)`)
+  if (value === '.' || value === '..') {
+    throw new Error(`${paramName} cannot be "${value}" (path traversal is not allowed)`)
   }
 }
 

apps/sim/app/api/tools/hubspot/path_safety.test.ts

@@ -53,6 +53,24 @@ const REJECTED = ['..', '.', '%2e%2e', 'a/b', '  ..  ', '..%2f..', '../..'] as c
  */
 const LEGITIMATE = ['p7878787_my_object', '2-123456', '0-1', 'my_object'] as const
 
+/**
+ * Keys that exist on `Object.prototype`, not on `BUILT_IN_PATH`. A plain object
+ * literal resolves them through the prototype chain, so `BUILT_IN_PATH[key]`
+ * returns a function or an object rather than `undefined` — the `?? objectType`
+ * fallback never fires, and `validatePathSegment` then calls `.includes` on a
+ * non-string and throws, turning caller-controlled input into a 500. Every one
+ * of these is a syntactically legal HubSpot custom-object name, so the correct
+ * behavior is to treat it as one: pass it through the validator as the string
+ * it is.
+ */
+const PROTOTYPE_KEYS = [
+  'constructor',
+  '__proto__',
+  'toString',
+  'hasOwnProperty',
+  'valueOf',
+] as const
+
 let fetchMock: ReturnType<typeof vi.fn>
 
 beforeEach(() => {
@@ -111,6 +129,23 @@ describe.each(ROUTES)(
       expect(url.pathname.split('/')).toEqual(['', 'crm', 'v3', collection, objectType])
     })
 
+    it.each(PROTOTYPE_KEYS)(
+      'treats the inherited key %j as a plain object-type string, never a 500',
+      async (objectType) => {
+        const response = await handler(request(name, objectType))
+
+        expect(response.status).not.toBe(500)
+        expect(response.status).toBe(200)
+        expect(outgoingUrl().pathname.split('/')).toEqual([
+          '',
+          'crm',
+          'v3',
+          collection,
+          encodeURIComponent(objectType),
+        ])
+      }
+    )
+
     it.each(REJECTED)(
       'rejects objectType=%j with a 400 and never calls HubSpot',
       async (objectType) => {

apps/sim/app/api/tools/hubspot/pipelines/route.ts

@@ -18,6 +18,14 @@ const logger = createLogger('HubSpotPipelinesAPI')
  * only to a non-empty string. That value lands in a path segment, so it is
  * validated before use — `encodeURIComponent` alone leaves a `.`/`..` segment
  * intact and the WHATWG parser removes it, re-aiming the authenticated request.
+ *
+ * The lookup goes through `Object.hasOwn` rather than plain indexing: on a
+ * plain object literal, `objectType = 'constructor'` (or `'__proto__'`,
+ * `'toString'`, …) resolves through the prototype chain to a function, the
+ * `?? objectType` fallback never fires, and `validatePathSegment` then calls a
+ * string method on a non-string and throws — turning caller-controlled input
+ * into a 500. Those names are legal HubSpot custom-object spellings, so they
+ * must reach the validator as the strings they are.
  */
 const BUILT_IN_PATH: Record<string, string> = {
   contact: 'contacts',
@@ -64,7 +72,9 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
       return NextResponse.json({ error: 'Failed to obtain valid access token' }, { status: 401 })
     }
 
-    const pathSegment = BUILT_IN_PATH[objectType] ?? objectType
+    const pathSegment = Object.hasOwn(BUILT_IN_PATH, objectType)
+      ? BUILT_IN_PATH[objectType]
+      : objectType
     const pathSegmentValidation = validatePathSegment(pathSegment, { paramName: 'objectType' })
     if (!pathSegmentValidation.isValid) {
       logger.warn(`[${requestId}] Invalid objectType: ${pathSegmentValidation.error}`)

apps/sim/app/api/tools/hubspot/properties/route.ts

@@ -18,6 +18,14 @@ const logger = createLogger('HubSpotPropertiesAPI')
  * only to a non-empty string. That value lands in a path segment, so it is
  * validated before use — `encodeURIComponent` alone leaves a `.`/`..` segment
  * intact and the WHATWG parser removes it, re-aiming the authenticated request.
+ *
+ * The lookup goes through `Object.hasOwn` rather than plain indexing: on a
+ * plain object literal, `objectType = 'constructor'` (or `'__proto__'`,
+ * `'toString'`, …) resolves through the prototype chain to a function, the
+ * `?? objectType` fallback never fires, and `validatePathSegment` then calls a
+ * string method on a non-string and throws — turning caller-controlled input
+ * into a 500. Those names are legal HubSpot custom-object spellings, so they
+ * must reach the validator as the strings they are.
  */
 const BUILT_IN_PATH: Record<string, string> = {
   contact: 'contacts',
@@ -67,7 +75,9 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
       return NextResponse.json({ error: 'Failed to obtain valid access token' }, { status: 401 })
     }
 
-    const pathSegment = BUILT_IN_PATH[objectType] ?? objectType
+    const pathSegment = Object.hasOwn(BUILT_IN_PATH, objectType)
+      ? BUILT_IN_PATH[objectType]
+      : objectType
     const pathSegmentValidation = validatePathSegment(pathSegment, { paramName: 'objectType' })
     if (!pathSegmentValidation.isValid) {
       logger.warn(`[${requestId}] Invalid objectType: ${pathSegmentValidation.error}`)

apps/sim/app/api/tools/twilio/get-recording/route.test.ts

@@ -24,6 +24,7 @@ import {
   inputValidationMockFns,
 } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { PayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
 
 vi.mock('@/lib/core/security/input-validation.server', () => inputValidationMock)
 
@@ -206,3 +207,76 @@ describe('POST /api/tools/twilio/get-recording — media size cap', () => {
     })
   })
 })
+
+describe('POST /api/tools/twilio/get-recording — over-limit media', () => {
+  /**
+   * The cap is enforced by `secureFetchWithPinnedIP` while the body streams, so
+   * it surfaces as a rejected promise *inside* the media `try`. A blanket
+   * `catch` there logged a warning and fell through to the success response
+   * with `file` simply absent — indistinguishable, to the caller, from a
+   * recording that has no media yet. An unavailable recording must never look
+   * like a retrieved one.
+   */
+  it('returns an explicit failure, not success-with-no-file, when the media exceeds the cap', async () => {
+    mockSecureFetchWithPinnedIP
+      .mockResolvedValueOnce(jsonResponse(recordingPayload))
+      .mockResolvedValueOnce(jsonResponse({ transcriptions: [] }))
+      .mockRejectedValueOnce(
+        new PayloadSizeLimitError({
+          label: 'response body',
+          maxBytes: MAX_TWILIO_RECORDING_BYTES,
+          observedBytes: MAX_TWILIO_RECORDING_BYTES + 1,
+        })
+      )
+
+    const response = await POST(createMockRequest('POST', baseBody))
+
+    expect(response.status).toBe(413)
+    const data = (await response.json()) as {
+      success: boolean
+      error?: string
+      output?: { file?: unknown }
+    }
+    expect(data.success).toBe(false)
+    expect(data.error).toBeTruthy()
+    expect(data.error).toMatch(new RegExp(String(MAX_TWILIO_RECORDING_BYTES)))
+    expect(data.output?.file).toBeUndefined()
+  })
+
+  it('still degrades to success-without-file for the other media errors that catch exists for', async () => {
+    mockSecureFetchWithPinnedIP
+      .mockResolvedValueOnce(jsonResponse(recordingPayload))
+      .mockResolvedValueOnce(jsonResponse({ transcriptions: [] }))
+      .mockRejectedValueOnce(new Error('socket hang up'))
+
+    const response = await POST(createMockRequest('POST', baseBody))
+
+    expect(response.status).toBe(200)
+    const data = (await response.json()) as {
+      success: boolean
+      output: { file?: unknown; mediaUrl?: string }
+    }
+    expect(data.success).toBe(true)
+    expect(data.output.file).toBeUndefined()
+    expect(data.output.mediaUrl).toBe(
+      `https://api.twilio.com/2010-04-01/Accounts/${ACCOUNT_SID}/Recordings/${RECORDING_SID}`
+    )
+  })
+
+  it('still degrades to success-without-file when media URL validation fails', async () => {
+    mockSecureFetchWithPinnedIP
+      .mockResolvedValueOnce(jsonResponse(recordingPayload))
+      .mockResolvedValueOnce(jsonResponse({ transcriptions: [] }))
+    mockValidateUrlWithDNS
+      .mockResolvedValueOnce({ isValid: true, resolvedIP: PINNED_IP })
+      .mockResolvedValueOnce({ isValid: true, resolvedIP: PINNED_IP })
+      .mockResolvedValueOnce({ isValid: false, error: 'blocked host' })
+
+    const response = await POST(createMockRequest('POST', baseBody))
+
+    expect(response.status).toBe(200)
+    const data = (await response.json()) as { success: boolean; output: { file?: unknown } }
+    expect(data.success).toBe(true)
+    expect(data.output.file).toBeUndefined()
+  })
+})

apps/sim/app/api/tools/twilio/get-recording/route.ts

@@ -9,6 +9,7 @@ import {
   validateUrlWithDNS,
 } from '@/lib/core/security/input-validation.server'
 import { generateRequestId } from '@/lib/core/utils/request'
+import { isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import { getExtensionFromMimeType } from '@/lib/uploads/utils/file-utils'
 
@@ -248,6 +249,29 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
           }
         }
       } catch (error) {
+        /**
+         * The size cap is enforced while the body streams, so it surfaces here
+         * rather than as a non-ok response. It must not share the fall-through
+         * below: that path returns `success: true` with `file` absent, which is
+         * exactly how a recording with no media yet looks. Reporting an
+         * over-limit recording as a successful retrieval that happened to
+         * return nothing is worse than reporting nothing at all.
+         */
+        if (isPayloadSizeLimitError(error)) {
+          logger.warn(`[${requestId}] Twilio recording media exceeds the transportable size cap`, {
+            recordingSid: data.sid ?? recordingSid,
+            maxBytes: MAX_TWILIO_RECORDING_BYTES,
+          })
+          const message = `Recording media exceeds the maximum transportable size of ${MAX_TWILIO_RECORDING_BYTES} bytes and cannot be returned inline. Download it directly from ${mediaUrl}.`
+          return NextResponse.json(
+            {
+              success: false,
+              output: { success: false, error: message, mediaUrl },
+              error: message,
+            },
+            { status: 413 }
+          )
+        }
         logger.warn(`[${requestId}] Failed to download recording media:`, error)
       }
     }