fix(box_sign): refuse a padded signRequestId instead of silently resolving it

Sweeping the six services for the same class found in BigQuery turned up a
second instance nobody had flagged. signRequestId was interpolated raw before
this branch — not even a .trim() — so a padded id was percent-encoded to
%20%20<uuid>%20%20, matched no sign request, and the call failed:

  before: /2.0/sign_requests/%20%20<uuid>%20%20/cancel  -> 404, no-op
  after:  /2.0/sign_requests/<uuid>/cancel              -> cancels it

box_sign_cancel_request is irreversible, so trimming would have converted a
request that did nothing into one that cancels a real signature request.

Extracts the rule shared with BigQuery into strictUrlPathSegment, now that two
services need it, and applies it to all three box_sign tools. Box Sign ids are
UUIDs, so no legitimate value carries whitespace.

The rest of the sweep is clean and deliberately unchanged: google_drive and box
already trimmed every path id before this branch, so nothing there is newly
resolved; google_contacts and the Supabase storage key move the other way,
since safeUrlPath no longer trims at all, which can only turn a previously
working value into a clean failure.

Pinned the same way as BigQuery and verified non-vacuous: reverting the guard
fails both the generic per-pair assertion and the explicit cancel test.

Author: waleedlatif1

apps/sim/tools/box_sign/cancel_request.ts

@@ -1,5 +1,5 @@
+import { strictUrlPathSegment } from '@/tools/strict-url-path'
 import type { ToolConfig } from '@/tools/types'
-import { safeUrlPathSegment } from '@/tools/url-path'
 import type { BoxSignCancelRequestParams, BoxSignResponse } from './types'
 import { SIGN_REQUEST_OUTPUT_PROPERTIES } from './types'
 
@@ -31,7 +31,7 @@ export const boxSignCancelRequestTool: ToolConfig<BoxSignCancelRequestParams, Bo
 
   request: {
     url: (params) =>
-      `https://api.box.com/2.0/sign_requests/${safeUrlPathSegment(params.signRequestId, 'signRequestId')}/cancel`,
+      `https://api.box.com/2.0/sign_requests/${strictUrlPathSegment(params.signRequestId, 'signRequestId')}/cancel`,
     method: 'POST',
     headers: (params) => ({
       Authorization: `Bearer ${params.accessToken}`,

apps/sim/tools/box_sign/get_request.ts

@@ -1,5 +1,5 @@
+import { strictUrlPathSegment } from '@/tools/strict-url-path'
 import type { ToolConfig } from '@/tools/types'
-import { safeUrlPathSegment } from '@/tools/url-path'
 import type { BoxSignGetRequestParams, BoxSignResponse } from './types'
 import { SIGN_REQUEST_OUTPUT_PROPERTIES } from './types'
 
@@ -31,7 +31,7 @@ export const boxSignGetRequestTool: ToolConfig<BoxSignGetRequestParams, BoxSignR
 
   request: {
     url: (params) =>
-      `https://api.box.com/2.0/sign_requests/${safeUrlPathSegment(params.signRequestId, 'signRequestId')}`,
+      `https://api.box.com/2.0/sign_requests/${strictUrlPathSegment(params.signRequestId, 'signRequestId')}`,
     method: 'GET',
     headers: (params) => ({
       Authorization: `Bearer ${params.accessToken}`,

apps/sim/tools/box_sign/path_safety.test.ts

@@ -59,7 +59,58 @@ describe('box sign path-id traversal safety', () => {
   })
 
   describe.each(PATH_PARAMS)('$label', (param) => {
-    itResistsTraversal(param, { origin: ORIGIN, basePath: BASE_PATH })
+    itResistsTraversal(param, {
+      origin: ORIGIN,
+      basePath: BASE_PATH,
+      rejectsSurroundingWhitespace: ['signRequestId'],
+    })
     itPassesLegitimateValues(param, { values: LEGITIMATE_IDS })
   })
 })
+
+/**
+ * A padded `signRequestId` must not become a successful cancellation.
+ *
+ * `signRequestId` was interpolated raw before this branch — not even a
+ * `.trim()` — so a padded id was percent-encoded to
+ * `%20%20<uuid>%20%20`, matched no sign request, and the call failed:
+ *
+ * ```
+ * before: /2.0/sign_requests/%20%2012345678-…-123456789012%20%20/cancel
+ * after:  /2.0/sign_requests/12345678-…-123456789012/cancel
+ * ```
+ *
+ * Had the guard simply trimmed, that POST would have stopped failing and
+ * started **cancelling a real signature request** — irreversible, from a value
+ * the caller never wrote. Box Sign ids are UUIDs, so no legitimate value
+ * carries whitespace and refusing costs nothing.
+ */
+describe('a padded signRequestId cannot become a successful cancellation', () => {
+  const PADDED = '  12345678-1234-1234-1234-123456789012  '
+  const CLEAN = '12345678-1234-1234-1234-123456789012'
+
+  const STATE_CHANGING = [
+    { name: 'box_sign_cancel_request', tool: boxSignTools.boxSignCancelRequestTool },
+    { name: 'box_sign_resend_request', tool: boxSignTools.boxSignResendRequestTool },
+  ]
+
+  it.each(STATE_CHANGING)('$name refuses a padded signRequestId', ({ tool }) => {
+    expect(() =>
+      (tool.request?.url as (p: Record<string, unknown>) => string)({
+        accessToken: 't',
+        signRequestId: PADDED,
+      })
+    ).toThrow(/signRequestId cannot have leading or trailing whitespace/)
+  })
+
+  it.each(STATE_CHANGING)('$name still accepts the unpadded id', ({ tool }) => {
+    const url = new URL(
+      (tool.request?.url as (p: Record<string, unknown>) => string)({
+        accessToken: 't',
+        signRequestId: CLEAN,
+      })
+    )
+
+    expect(url.pathname).toContain(`/2.0/sign_requests/${CLEAN}`)
+  })
+})

apps/sim/tools/box_sign/resend_request.ts

@@ -1,5 +1,5 @@
+import { strictUrlPathSegment } from '@/tools/strict-url-path'
 import type { ToolConfig, ToolResponse } from '@/tools/types'
-import { safeUrlPathSegment } from '@/tools/url-path'
 import type { BoxSignResendRequestParams } from './types'
 
 export const boxSignResendRequestTool: ToolConfig<BoxSignResendRequestParams, ToolResponse> = {
@@ -30,7 +30,7 @@ export const boxSignResendRequestTool: ToolConfig<BoxSignResendRequestParams, To
 
   request: {
     url: (params) =>
-      `https://api.box.com/2.0/sign_requests/${safeUrlPathSegment(params.signRequestId, 'signRequestId')}/resend`,
+      `https://api.box.com/2.0/sign_requests/${strictUrlPathSegment(params.signRequestId, 'signRequestId')}/resend`,
     method: 'POST',
     headers: (params) => ({
       Authorization: `Bearer ${params.accessToken}`,

apps/sim/tools/google_bigquery/utils.ts

@@ -1,63 +1,6 @@
+import { assertNoSurroundingWhitespace, strictUrlPathSegment } from '@/tools/strict-url-path'
 import { safeUrlPathSegment } from '@/tools/url-path'
 
-/**
- * Refuses an identifier carrying leading or trailing whitespace.
- *
- * This exists because trimming is not a neutral convenience on an identifier
- * that was **not** trimmed before. Every BigQuery path identifier here was
- * previously interpolated as `encodeURIComponent(params.projectId)`, so a padded
- * value became `%20%20my-project%20%20`, which names no project — GCP project
- * ids match `[a-z][a-z0-9-]{5,29}` and cannot contain whitespace — and the
- * request failed cleanly. Guarding the path with `safeUrlPathSegment` trims,
- * which silently resolves that same value to the **real** `my-project`:
- *
- * ```
- * before: /bigquery/v2/projects/%20%20my-project%20%20/datasets/prod_dataset  -> 404
- * after:  /bigquery/v2/projects/my-project/datasets/prod_dataset              -> deletes it
- * ```
- *
- * On `google_bigquery_delete_dataset` and `google_bigquery_delete_table` that
- * converts a request that did nothing into one that destroys a real dataset or
- * table, irreversibly. The rule this encodes is therefore narrow and testable:
- * **this change must not turn a failing request into a succeeding one.**
- *
- * Rejection rather than trimming is not a consistency argument — that reasoning
- * averages over sites with very different blast radii and is exactly what would
- * excuse the deletion above. It stands on two facts specific to these values:
- * no legitimate BigQuery identifier contains surrounding whitespace, so nothing
- * real is refused; and the pre-existing behaviour for these particular
- * parameters was already a clean failure, so refusing preserves it while adding
- * an error that names the offending parameter instead of an opaque 404.
- *
- * Identifiers that this PR did **not** newly trim keep `safeUrlPathSegment`.
- * `datasetId` on the two delete tools, for instance, was already
- * `.trim()`-ed before this branch, so trimming it is not a change made here and
- * refusing it would break callers whose stored value works today. That is a
- * real pre-existing hazard, but it is not this change's to introduce or to
- * silently alter.
- */
-function assertNoSurroundingWhitespace(value: string | number | bigint, paramName: string): void {
-  if (typeof value === 'string' && value !== value.trim()) {
-    throw new Error(
-      `${paramName} cannot have leading or trailing whitespace (received ${JSON.stringify(value)})`
-    )
-  }
-}
-
-/**
- * Path-segment guard for an identifier this change newly began trimming.
- *
- * See {@link assertNoSurroundingWhitespace} for why padding is refused here
- * rather than trimmed away.
- */
-export function strictBigQueryPathSegment(
-  value: string | number | bigint,
-  paramName: string
-): string {
-  assertNoSurroundingWhitespace(value, paramName)
-  return safeUrlPathSegment(value, paramName)
-}
-
 /**
  * Returns the canonical, unencoded form of an identifier that appears in both
  * the request path and the request body.
@@ -97,3 +40,9 @@ export function strictCanonicalBigQueryId(
   assertNoSurroundingWhitespace(value, paramName)
   return canonicalBigQueryId(value, paramName)
 }
+
+/**
+ * Path-segment guard for a BigQuery identifier this change newly began
+ * trimming. See `strictUrlPathSegment` for why padding is refused.
+ */
+export const strictBigQueryPathSegment = strictUrlPathSegment

apps/sim/tools/strict-url-path.ts

@@ -0,0 +1,60 @@
+import { safeUrlPathSegment } from '@/tools/url-path'
+
+/**
+ * Guards a path identifier that this change **newly began trimming**, refusing
+ * surrounding whitespace instead of silently removing it.
+ *
+ * Trimming is not a neutral convenience when it is new. These identifiers were
+ * previously interpolated raw or through a bare `encodeURIComponent`, so a
+ * padded value was percent-encoded and named nothing:
+ *
+ * ```
+ * before: /2.0/sign_requests/%20%20<uuid>%20%20/cancel                       -> 404, no-op
+ * after:  /2.0/sign_requests/<uuid>/cancel                                   -> cancels it
+ *
+ * before: /bigquery/v2/projects/%20%20my-project%20%20/datasets/prod_dataset -> 404, no-op
+ * after:  /bigquery/v2/projects/my-project/datasets/prod_dataset             -> deletes it
+ * ```
+ *
+ * On `box_sign_cancel_request` and `google_bigquery_delete_*` that converts a
+ * request which did nothing into one with an **irreversible** effect, driven by
+ * a value the caller never wrote. The rule this encodes is therefore narrow and
+ * testable: *guarding a path must not turn a failing request into a succeeding
+ * one.*
+ *
+ * Rejection is deliberately **not** argued from consistency with the other
+ * guarded sites. That reasoning averages over parameters with very different
+ * blast radii and would excuse the deletion above. It rests on two facts
+ * specific to these values:
+ *
+ * 1. None of them can legitimately carry surrounding whitespace — a Box Sign id
+ *    is a UUID, a GCP project id matches `[a-z][a-z0-9-]{5,29}` — so refusing
+ *    excludes nothing a caller could really mean.
+ * 2. Their previous behaviour was already a clean failure, so refusing
+ *    preserves it, and improves on it by replacing an opaque provider 404 with
+ *    an error naming the parameter.
+ *
+ * Identifiers that were **already** trimmed before this change keep plain
+ * {@link safeUrlPathSegment}: trimming those is not a change made here, and
+ * refusing them would break callers whose stored value works today.
+ */
+export function strictUrlPathSegment(value: string | number | bigint, paramName: string): string {
+  assertNoSurroundingWhitespace(value, paramName)
+  return safeUrlPathSegment(value, paramName)
+}
+
+/**
+ * Shared precondition behind {@link strictUrlPathSegment} and its body-value
+ * counterparts, so a padded value is refused identically wherever the same
+ * identifier is rendered.
+ */
+export function assertNoSurroundingWhitespace(
+  value: string | number | bigint,
+  paramName: string
+): void {
+  if (typeof value === 'string' && value !== value.trim()) {
+    throw new Error(
+      `${paramName} cannot have leading or trailing whitespace (received ${JSON.stringify(value)})`
+    )
+  }
+}