fix(supabase): reject fragment injection in filters, allow legal storage keys

- a '#' in filter/orderBy silently truncated the query string, dropping
  the row cap on get_row and widening the match on delete/update
- move '&limit=1' ahead of the filter so the cap is out of its reach
- storage paths opt into allowEmptySegments/preserveOuterWhitespace, so
  Supabase's legal keys ('a//b', ' report.csv') survive; dot segments and
  backslashes stay rejected, and all other call sites are unchanged

Author: waleedlatif1

apps/sim/tools/attio/assert_record.ts

@@ -6,6 +6,43 @@ import { RECORD_OUTPUT_PROPERTIES } from './types'
 
 const logger = createLogger('AttioAssertRecord')
 
+/**
+ * Normalizes `matchingAttribute` to a non-empty string before it is handed to
+ * `URLSearchParams`.
+ *
+ * The parameter is declared `type: 'string'`, but it is `visibility:
+ * 'user-or-llm'` and an LLM can emit an attribute id that looks numeric as a
+ * JSON **number**. Calling `.trim()` on the raw value then threw
+ * `params.matchingAttribute.trim is not a function` — an unhandled `TypeError`
+ * surfaced to the caller instead of a named, actionable error.
+ *
+ * `null` and `undefined` are rejected *before* coercion, because
+ * `String(null)` is the truthy `'null'`: coercing first would send a request
+ * matching on an attribute literally named `"null"` rather than reporting the
+ * missing value. This mirrors `toGuardedString` in `@/tools/url-path`, which
+ * solves the same problem for the path zone but is module-private there; the
+ * few lines are duplicated rather than widening that shared module's surface
+ * for a single call site.
+ *
+ * No charset check is applied. Attio documents the value as "the ID or slug of
+ * the attribute" and publishes no pattern for a slug, so any allowlist would be
+ * a guess that silently rejects legitimate attributes. Correct encoding — not
+ * validation — is what confines the value to the query zone.
+ */
+function requiredQueryValue(value: unknown, paramName: string): string {
+  if (value === null || value === undefined) {
+    throw new Error(`${paramName} is required`)
+  }
+
+  const trimmed = String(value).trim()
+
+  if (!trimmed) {
+    throw new Error(`${paramName} is required`)
+  }
+
+  return trimmed
+}
+
 export const attioAssertRecordTool: ToolConfig<AttioAssertRecordParams, AttioAssertRecordResponse> =
   {
     id: 'attio_assert_record',
@@ -49,8 +86,15 @@ export const attioAssertRecordTool: ToolConfig<AttioAssertRecordParams, AttioAss
     },
 
     request: {
-      url: (params) =>
-        `https://api.attio.com/v2/objects/${safeUrlPathSegment(params.objectType, 'objectType')}/records?matching_attribute=${params.matchingAttribute.trim()}`,
+      url: (params) => {
+        const objectType = safeUrlPathSegment(params.objectType, 'objectType')
+        const searchParams = new URLSearchParams()
+        searchParams.set(
+          'matching_attribute',
+          requiredQueryValue(params.matchingAttribute, 'matchingAttribute')
+        )
+        return `https://api.attio.com/v2/objects/${objectType}/records?${searchParams.toString()}`
+      },
       method: 'PUT',
       headers: (params) => ({
         Authorization: `Bearer ${params.accessToken}`,

apps/sim/tools/attio/query_safety.test.ts

@@ -0,0 +1,100 @@
+/**
+ * @vitest-environment node
+ *
+ * Guards the one Attio site that interpolates a parameter into the request
+ * *query* zone: `attio_assert_record`'s `matching_attribute`.
+ *
+ * `matchingAttribute` is `required: true, visibility: 'user-or-llm'`, so
+ * prompt injection controls it. Interpolated raw, a value carrying `&` appends
+ * arbitrary extra query parameters to the request, and a value carrying `#`
+ * truncates the URL at a fragment — both while the caller's Attio OAuth token
+ * is still attached.
+ *
+ * Every assertion resolves the built URL through `new URL(...)` — the same
+ * parse `fetch` performs — and inspects `searchParams`, never the raw string.
+ * A `.includes()` assertion on the template is exactly the weak form that lets
+ * a broken rewrite pass.
+ */
+import { describe, expect, it } from 'vitest'
+import { attioAssertRecordTool } from '@/tools/attio/assert_record'
+
+const ORIGIN = 'https://api.attio.com'
+const PATHNAME = '/v2/objects/people/records'
+
+function buildUrl(matchingAttribute: unknown): URL {
+  return new URL(
+    (attioAssertRecordTool.request!.url as (p: any) => string)({
+      accessToken: 'token',
+      objectType: 'people',
+      matchingAttribute,
+      values: '{}',
+    })
+  )
+}
+
+/** Values a real caller supplies; each must survive verbatim. */
+const LEGITIMATE = [
+  'email_addresses',
+  'domains',
+  'custom.attr-1',
+  'attr+plus',
+  'attr with space',
+  '97052eb9-e65e-443f-a297-f2d9a4a7f795',
+] as const
+
+/** Vectors that reshape the request when interpolated raw. */
+const INJECTIONS = [
+  'email_addresses&limit=1&x=y',
+  'a#b',
+  'email_addresses#',
+  'x&matching_attribute=y',
+  'a=b&c=d',
+] as const
+
+describe('attio_assert_record matching_attribute query safety', () => {
+  it('builds the expected origin and path', () => {
+    const url = buildUrl('email_addresses')
+    expect(url.origin).toBe(ORIGIN)
+    expect(url.pathname).toBe(PATHNAME)
+  })
+
+  it.each(LEGITIMATE)('round-trips %j verbatim', (value) => {
+    const url = buildUrl(value)
+    expect(url.searchParams.get('matching_attribute')).toBe(value)
+    expect([...url.searchParams.keys()]).toEqual(['matching_attribute'])
+  })
+
+  it.each(INJECTIONS)('confines %j to the matching_attribute value', (value) => {
+    const url = buildUrl(value)
+
+    expect(url.origin).toBe(ORIGIN)
+    expect(url.pathname).toBe(PATHNAME)
+    expect(url.hash).toBe('')
+    expect([...url.searchParams.keys()]).toEqual(['matching_attribute'])
+    expect(url.searchParams.get('matching_attribute')).toBe(value)
+  })
+
+  it('does not change the wire bytes for a legitimate slug', () => {
+    const raw = (attioAssertRecordTool.request!.url as (p: any) => string)({
+      accessToken: 'token',
+      objectType: 'people',
+      matchingAttribute: 'email_addresses',
+      values: '{}',
+    })
+    expect(raw).toBe(`${ORIGIN}${PATHNAME}?matching_attribute=email_addresses`)
+  })
+
+  it('still trims surrounding whitespace', () => {
+    expect(buildUrl('  email_addresses  ').searchParams.get('matching_attribute')).toBe(
+      'email_addresses'
+    )
+  })
+
+  it('stringifies a numeric value instead of throwing a TypeError', () => {
+    expect(buildUrl(123).searchParams.get('matching_attribute')).toBe('123')
+  })
+
+  it.each([null, undefined, '', '   '])('rejects %j by name', (value) => {
+    expect(() => buildUrl(value)).toThrow(/matchingAttribute/)
+  })
+})

apps/sim/tools/elasticsearch/not-found.test.ts

@@ -14,6 +14,16 @@ vi.mock('@/lib/core/security/input-validation.server', () => ({
   validateUrlWithDNS: mockValidateUrlWithDNS,
 }))
 
+/**
+ * `vitest.setup.ts` mocks the tool registry to `{}` because the real one pulls
+ * ~5,907 modules. Registering just this tool keeps that saving while giving
+ * `executeTool` the *same object reference* the spy below is attached to.
+ */
+vi.mock('@/tools/registry', async () => {
+  const { getDocumentTool } = await import('@/tools/elasticsearch/get_document')
+  return { tools: { elasticsearch_get_document: getDocumentTool } }
+})
+
 import { getDocumentTool } from '@/tools/elasticsearch/get_document'
 import { executeTool } from '@/tools/index'