Skip to content

Commit 3d3c6dc

Browse files
waleedlatif1waleedlatif1
committed
fix(attio): build the assert_record query with URLSearchParams
matching_attribute is user-or-llm and was interpolated raw, so a '&' injected extra query params and a '#' truncated the request. A literal '+' also decoded to a space server-side, silently addressing a different attribute.
1 parent 1fd3fb6 commit 3d3c6dc

11 files changed

Lines changed: 582 additions & 38 deletions

File tree

‎apps/sim/app/api/tools/google_drive/download/path-safety.test.ts‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Guards the internal Google Drive download route against path traversal.
5+
*
6+
* This route was invisible to the tools-side traversal sweep, which probed
7+
* each tool's `request.url`. The route has none: the `google_drive_download`
8+
* tool posts to it and the route builds the googleapis URL itself, bare —
9+
* `https://www.googleapis.com/drive/v3/files/${fileId}` — from a body field
10+
* the contract validates only as a non-empty string, and which the tool
11+
* declares `visibility: 'user-or-llm'`.
12+
*
13+
* Every assertion resolves the built URL through `new URL(...)`, the same
14+
* normalization `fetch` performs, and checks the resolved pathname's segment
15+
* count and fixed segments rather than a `startsWith` prefix — which
16+
* `/drive/v3/files/..` would still satisfy before normalization.
17+
*/
18+
import {
19+
createMockRequest,
20+
hybridAuthMockFns,
21+
inputValidationMock,
22+
inputValidationMockFns,
23+
} from '@sim/testing'
24+
import { beforeEach, describe, expect, it, vi } from 'vitest'
25+
26+
vi.mock('@/lib/core/security/input-validation.server', () => inputValidationMock)
27+
28+
import { POST } from '@/app/api/tools/google_drive/download/route'
29+
30+
const { mockValidateUrlWithDNS, mockSecureFetchWithPinnedIP } = inputValidationMockFns
31+
32+
const PINNED_IP = '93.184.216.34'
33+
34+
/** Values no encoding neutralizes; the route must reject them outright. */
35+
const REJECTED = ['..', '.', ' .. ', 'a/b', '..\\..'] as const
36+
37+
/** Values that must not throw but must stay inside one path segment. */
38+
const NEUTRALIZED = ['%2e%2e', 'x?alt=media'] as const
39+
40+
function jsonResponse(body: unknown) {
41+
return {
42+
ok: true,
43+
status: 200,
44+
statusText: '',
45+
headers: new Headers(),
46+
body: null,
47+
text: async () => JSON.stringify(body),
48+
json: async () => body,
49+
arrayBuffer: async () => new ArrayBuffer(0),
50+
}
51+
}
52+
53+
function fileResponse() {
54+
return {
55+
ok: true,
56+
status: 200,
57+
statusText: '',
58+
headers: new Headers(),
59+
body: null,
60+
text: async () => '',
61+
json: async () => ({}),
62+
arrayBuffer: async () => new ArrayBuffer(8),
63+
}
64+
}
65+
66+
function metadataFor(fileId: string) {
67+
return {
68+
id: fileId,
69+
name: 'report.pdf',
70+
mimeType: 'application/pdf',
71+
size: '8',
72+
capabilities: { canReadRevisions: false },
73+
}
74+
}
75+
76+
function requestedUrls(): string[] {
77+
return mockValidateUrlWithDNS.mock.calls.map((call) => String(call[0]))
78+
}
79+
80+
beforeEach(() => {
81+
vi.clearAllMocks()
82+
hybridAuthMockFns.mockCheckInternalAuth.mockResolvedValue({
83+
success: true,
84+
userId: 'user-1',
85+
authType: 'internal_jwt',
86+
})
87+
mockValidateUrlWithDNS.mockResolvedValue({
88+
isValid: true,
89+
resolvedIP: PINNED_IP,
90+
originalHostname: 'www.googleapis.com',
91+
})
92+
})
93+
94+
describe('POST /api/tools/google_drive/download traversal safety', () => {
95+
it.each(REJECTED)('rejects fileId %j with a clean 400 and no outbound request', async (fileId) => {
96+
const response = await POST(createMockRequest('POST', { accessToken: 'token-123', fileId }))
97+
98+
expect(response.status).toBe(400)
99+
const data = (await response.json()) as { success: boolean; error: string }
100+
expect(data.success).toBe(false)
101+
expect(data.error).toMatch(/fileId/)
102+
expect(mockValidateUrlWithDNS).not.toHaveBeenCalled()
103+
expect(mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
104+
})
105+
106+
it.each(NEUTRALIZED)('keeps fileId %j inside a single path segment', async (fileId) => {
107+
mockSecureFetchWithPinnedIP
108+
.mockResolvedValueOnce(jsonResponse(metadataFor('file-abc')))
109+
.mockResolvedValueOnce(fileResponse())
110+
111+
const response = await POST(createMockRequest('POST', { accessToken: 'token-123', fileId }))
112+
expect(response.status).toBe(200)
113+
114+
const metadataUrl = new URL(requestedUrls()[0])
115+
const segments = metadataUrl.pathname.split('/').filter(Boolean)
116+
expect(segments).toHaveLength(4)
117+
expect(segments.slice(0, 3)).toEqual(['drive', 'v3', 'files'])
118+
expect(decodeURIComponent(segments[3])).toBe(fileId)
119+
expect(metadataUrl.searchParams.get('supportsAllDrives')).toBe('true')
120+
})
121+
122+
it('leaves a legitimate file id byte-identical to the pre-guard URL', async () => {
123+
mockSecureFetchWithPinnedIP
124+
.mockResolvedValueOnce(jsonResponse(metadataFor('1a2B3c4D-5e6F_7g8H9i0J')))
125+
.mockResolvedValueOnce(fileResponse())
126+
127+
const response = await POST(
128+
createMockRequest('POST', { accessToken: 'token-123', fileId: '1a2B3c4D-5e6F_7g8H9i0J' })
129+
)
130+
expect(response.status).toBe(200)
131+
132+
const urls = requestedUrls()
133+
expect(urls[0]).toContain('/drive/v3/files/1a2B3c4D-5e6F_7g8H9i0J?fields=')
134+
expect(new URL(urls[1]).pathname).toBe('/drive/v3/files/1a2B3c4D-5e6F_7g8H9i0J')
135+
})
136+
137+
it('preserves a dot inside a longer id and keeps the revisions path intact', async () => {
138+
mockSecureFetchWithPinnedIP
139+
.mockResolvedValueOnce(
140+
jsonResponse({ ...metadataFor('a..b'), capabilities: { canReadRevisions: true } })
141+
)
142+
.mockResolvedValueOnce(fileResponse())
143+
.mockResolvedValueOnce(jsonResponse({ revisions: [] }))
144+
145+
const response = await POST(
146+
createMockRequest('POST', { accessToken: 'token-123', fileId: 'a..b', includeRevisions: true })
147+
)
148+
expect(response.status).toBe(200)
149+
150+
const revisionsUrl = new URL(requestedUrls()[2])
151+
const segments = revisionsUrl.pathname.split('/').filter(Boolean)
152+
expect(segments).toEqual(['drive', 'v3', 'files', 'a..b', 'revisions'])
153+
})
154+
})

‎apps/sim/app/api/tools/google_drive/download/route.ts‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
GOOGLE_WORKSPACE_MIME_TYPES,
2121
VALID_EXPORT_FORMATS,
2222
} from '@/tools/google_drive/utils'
23+
import { safeUrlPathSegment } from '@/tools/url-path'
2324

2425
export const dynamic = 'force-dynamic'
2526

@@ -83,9 +84,22 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
8384
rawExportMimeType && rawExportMimeType !== 'auto' ? rawExportMimeType : null
8485
const authHeader = `Bearer ${accessToken}`
8586

87+
let fileIdSegment: string
88+
try {
89+
fileIdSegment = safeUrlPathSegment(fileId, 'fileId')
90+
} catch (error) {
91+
logger.warn(`[${requestId}] Rejected unsafe fileId`, {
92+
error: getErrorMessage(error, 'Invalid fileId'),
93+
})
94+
return NextResponse.json(
95+
{ success: false, error: getErrorMessage(error, 'Invalid fileId') },
96+
{ status: 400 }
97+
)
98+
}
99+
86100
logger.info(`[${requestId}] Getting file metadata from Google Drive`, { fileId })
87101

88-
const metadataUrl = `https://www.googleapis.com/drive/v3/files/${fileId}?fields=${ALL_FILE_FIELDS}&supportsAllDrives=true`
102+
const metadataUrl = `https://www.googleapis.com/drive/v3/files/${fileIdSegment}?fields=${ALL_FILE_FIELDS}&supportsAllDrives=true`
89103
const metadataUrlValidation = await validateUrlWithDNS(metadataUrl, 'metadataUrl')
90104
if (!metadataUrlValidation.isValid) {
91105
return NextResponse.json(
@@ -150,7 +164,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
150164
exportFormat,
151165
})
152166

153-
const exportUrl = `https://www.googleapis.com/drive/v3/files/${fileId}/export?mimeType=${encodeURIComponent(exportFormat)}&supportsAllDrives=true`
167+
const exportUrl = `https://www.googleapis.com/drive/v3/files/${fileIdSegment}/export?mimeType=${encodeURIComponent(exportFormat)}&supportsAllDrives=true`
154168
const exportUrlValidation = await validateUrlWithDNS(exportUrl, 'exportUrl')
155169
if (!exportUrlValidation.isValid) {
156170
return NextResponse.json(
@@ -194,7 +208,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
194208
}
195209
}
196210

197-
const downloadUrl = `https://www.googleapis.com/drive/v3/files/${fileId}?alt=media&supportsAllDrives=true`
211+
const downloadUrl = `https://www.googleapis.com/drive/v3/files/${fileIdSegment}?alt=media&supportsAllDrives=true`
198212
const downloadUrlValidation = await validateUrlWithDNS(downloadUrl, 'downloadUrl')
199213
if (!downloadUrlValidation.isValid) {
200214
return NextResponse.json(
@@ -230,7 +244,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
230244
const canReadRevisions = metadata.capabilities?.canReadRevisions === true
231245
if (includeRevisions && canReadRevisions) {
232246
try {
233-
const revisionsUrl = `https://www.googleapis.com/drive/v3/files/${fileId}/revisions?fields=revisions(${ALL_REVISION_FIELDS})&pageSize=100`
247+
const revisionsUrl = `https://www.googleapis.com/drive/v3/files/${fileIdSegment}/revisions?fields=revisions(${ALL_REVISION_FIELDS})&pageSize=100`
234248
const revisionsUrlValidation = await validateUrlWithDNS(revisionsUrl, 'revisionsUrl')
235249
if (revisionsUrlValidation.isValid) {
236250
const revisionsResponse = await secureFetchWithPinnedIP(

‎apps/sim/app/api/tools/google_drive/export/path-safety.test.ts‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Guards the internal Google Drive export route against path traversal.
5+
*
6+
* This route already wrapped `fileId` in `encodeURIComponent`, which is not a
7+
* fix: `.` and `..` are unreserved characters, so they survive encoding, and
8+
* the WHATWG parser then removes them as dot segments *after* percent-decoding
9+
* (`/drive/v3/files/%2e%2e/export` resolves to `/drive/v3/export`). Only value
10+
* rejection closes it.
11+
*
12+
* Assertions resolve through `new URL(...)` and check the resolved pathname's
13+
* segment count and fixed segments, never a `startsWith` prefix.
14+
*/
15+
import {
16+
createMockRequest,
17+
hybridAuthMockFns,
18+
inputValidationMock,
19+
inputValidationMockFns,
20+
} from '@sim/testing'
21+
import { beforeEach, describe, expect, it, vi } from 'vitest'
22+
23+
vi.mock('@/lib/core/security/input-validation.server', () => inputValidationMock)
24+
25+
import { POST } from '@/app/api/tools/google_drive/export/route'
26+
27+
const { mockValidateUrlWithDNS, mockSecureFetchWithPinnedIP } = inputValidationMockFns
28+
29+
const PINNED_IP = '93.184.216.34'
30+
const DOC_MIME = 'application/vnd.google-apps.document'
31+
const EXPORT_MIME = 'text/plain'
32+
33+
const REJECTED = ['..', '.', ' .. ', 'a/b', '..\\..'] as const
34+
const NEUTRALIZED = ['%2e%2e', 'x?alt=media'] as const
35+
36+
function metadataResponse(fileId: string) {
37+
const body = { id: fileId, name: 'notes', mimeType: DOC_MIME }
38+
return {
39+
ok: true,
40+
status: 200,
41+
statusText: '',
42+
headers: new Headers(),
43+
body: null,
44+
text: async () => JSON.stringify(body),
45+
json: async () => body,
46+
arrayBuffer: async () => new ArrayBuffer(0),
47+
}
48+
}
49+
50+
function exportResponse() {
51+
return {
52+
ok: true,
53+
status: 200,
54+
statusText: '',
55+
headers: new Headers({ 'content-length': '4' }),
56+
body: null,
57+
text: async () => 'text',
58+
json: async () => ({}),
59+
arrayBuffer: async () => new ArrayBuffer(4),
60+
}
61+
}
62+
63+
function requestedUrls(): string[] {
64+
return mockValidateUrlWithDNS.mock.calls.map((call) => String(call[0]))
65+
}
66+
67+
function bodyFor(fileId: string) {
68+
return { accessToken: 'token-123', fileId, mimeType: EXPORT_MIME }
69+
}
70+
71+
beforeEach(() => {
72+
vi.clearAllMocks()
73+
hybridAuthMockFns.mockCheckInternalAuth.mockResolvedValue({
74+
success: true,
75+
userId: 'user-1',
76+
authType: 'internal_jwt',
77+
})
78+
mockValidateUrlWithDNS.mockResolvedValue({
79+
isValid: true,
80+
resolvedIP: PINNED_IP,
81+
originalHostname: 'www.googleapis.com',
82+
})
83+
})
84+
85+
describe('POST /api/tools/google_drive/export traversal safety', () => {
86+
it.each(REJECTED)('rejects fileId %j with a clean 400 and no outbound request', async (fileId) => {
87+
const response = await POST(createMockRequest('POST', bodyFor(fileId)))
88+
89+
expect(response.status).toBe(400)
90+
const data = (await response.json()) as { success: boolean; error: string }
91+
expect(data.success).toBe(false)
92+
expect(data.error).toMatch(/fileId/)
93+
expect(mockValidateUrlWithDNS).not.toHaveBeenCalled()
94+
expect(mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
95+
})
96+
97+
it.each(NEUTRALIZED)('keeps fileId %j inside a single path segment', async (fileId) => {
98+
mockSecureFetchWithPinnedIP
99+
.mockResolvedValueOnce(metadataResponse('doc-1'))
100+
.mockResolvedValueOnce(exportResponse())
101+
102+
const response = await POST(createMockRequest('POST', bodyFor(fileId)))
103+
expect(response.status).toBe(200)
104+
105+
const metadataSegments = new URL(requestedUrls()[0]).pathname.split('/').filter(Boolean)
106+
expect(metadataSegments).toHaveLength(4)
107+
expect(metadataSegments.slice(0, 3)).toEqual(['drive', 'v3', 'files'])
108+
expect(decodeURIComponent(metadataSegments[3])).toBe(fileId)
109+
110+
const exportSegments = new URL(requestedUrls()[1]).pathname.split('/').filter(Boolean)
111+
expect(exportSegments).toHaveLength(5)
112+
expect(exportSegments.slice(0, 3)).toEqual(['drive', 'v3', 'files'])
113+
expect(decodeURIComponent(exportSegments[3])).toBe(fileId)
114+
expect(exportSegments[4]).toBe('export')
115+
})
116+
117+
it('leaves a legitimate file id byte-identical to the pre-guard URL', async () => {
118+
mockSecureFetchWithPinnedIP
119+
.mockResolvedValueOnce(metadataResponse('1a2B3c4D-5e6F_7g8H9i0J'))
120+
.mockResolvedValueOnce(exportResponse())
121+
122+
const response = await POST(createMockRequest('POST', bodyFor('1a2B3c4D-5e6F_7g8H9i0J')))
123+
expect(response.status).toBe(200)
124+
125+
const urls = requestedUrls()
126+
expect(urls[0]).toContain('/drive/v3/files/1a2B3c4D-5e6F_7g8H9i0J?fields=')
127+
expect(urls[1]).toBe(
128+
'https://www.googleapis.com/drive/v3/files/1a2B3c4D-5e6F_7g8H9i0J/export?mimeType=text%2Fplain'
129+
)
130+
})
131+
132+
it('preserves a dot inside a longer id', async () => {
133+
mockSecureFetchWithPinnedIP
134+
.mockResolvedValueOnce(metadataResponse('a..b'))
135+
.mockResolvedValueOnce(exportResponse())
136+
137+
const response = await POST(createMockRequest('POST', bodyFor('a..b')))
138+
expect(response.status).toBe(200)
139+
140+
const exportSegments = new URL(requestedUrls()[1]).pathname.split('/').filter(Boolean)
141+
expect(exportSegments).toEqual(['drive', 'v3', 'files', 'a..b', 'export'])
142+
})
143+
})

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

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,27 @@ describe('attio_assert_record matching_attribute query safety', () => {
7474
expect(url.searchParams.get('matching_attribute')).toBe(value)
7575
})
7676

77-
it('does not change the wire bytes for a legitimate slug', () => {
78-
const raw = (attioAssertRecordTool.request!.url as (p: any) => string)({
79-
accessToken: 'token',
80-
objectType: 'people',
81-
matchingAttribute: 'email_addresses',
82-
values: '{}',
83-
})
84-
expect(raw).toBe(`${ORIGIN}${PATHNAME}?matching_attribute=email_addresses`)
85-
})
77+
/**
78+
* The realistic slug shapes — word characters, `_`, `-`, `.`, and a UUID —
79+
* are all `application/x-www-form-urlencoded`-safe, so `URLSearchParams`
80+
* emits exactly the bytes raw interpolation did. A value containing a
81+
* literal space or `+` does change on the wire (` ` becomes `+`, `+`
82+
* becomes `%2B`), which is the *correct* form-urlencoded spelling and the
83+
* only spelling that round-trips: raw interpolation sent `+` literally,
84+
* which any form-urlencoded decoder reads back as a space.
85+
*/
86+
it.each(['email_addresses', 'domains', 'custom.attr-1', '97052eb9-e65e-443f-a297-f2d9a4a7f795'])(
87+
'emits byte-identical wire bytes for %j',
88+
(value) => {
89+
const raw = (attioAssertRecordTool.request!.url as (p: any) => string)({
90+
accessToken: 'token',
91+
objectType: 'people',
92+
matchingAttribute: value,
93+
values: '{}',
94+
})
95+
expect(raw).toBe(`${ORIGIN}${PATHNAME}?matching_attribute=${value}`)
96+
}
97+
)
8698

8799
it('still trims surrounding whitespace', () => {
88100
expect(buildUrl(' email_addresses ').searchParams.get('matching_attribute')).toBe(

0 commit comments

Comments
 (0)