Skip to content

Commit 16ce3ec

Browse files
waleedlatif1waleedlatif1
committed
fix(google_drive,google_vault): guard the internal routes, cap get_content reads
The traversal sweep probed request.url, so it could not see tools whose provider URL is built inside an internal route. Drive's download route interpolated a user-or-llm fileId bare; export encoded it, which does not neutralize a dot segment. - guard both routes with safeUrlPathSegment, matching the tools side, and return 400 instead of letting the throw surface as a 500 - Vault keeps its %2F encoding (GCS object names contain slashes) and only rejects a whole value of '.'/'..' - get_content buffered an entire Drive file with no cap; bound it to the 10MB the shared transport already applies to its first hop
1 parent 67e3cab commit 16ce3ec

6 files changed

Lines changed: 247 additions & 30 deletions

File tree

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

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -92,16 +92,19 @@ beforeEach(() => {
9292
})
9393

9494
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-
})
95+
it.each(REJECTED)(
96+
'rejects fileId %j with a clean 400 and no outbound request',
97+
async (fileId) => {
98+
const response = await POST(createMockRequest('POST', { accessToken: 'token-123', fileId }))
99+
100+
expect(response.status).toBe(400)
101+
const data = (await response.json()) as { success: boolean; error: string }
102+
expect(data.success).toBe(false)
103+
expect(data.error).toMatch(/fileId/)
104+
expect(mockValidateUrlWithDNS).not.toHaveBeenCalled()
105+
expect(mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
106+
}
107+
)
105108

106109
it.each(NEUTRALIZED)('keeps fileId %j inside a single path segment', async (fileId) => {
107110
mockSecureFetchWithPinnedIP
@@ -143,7 +146,11 @@ describe('POST /api/tools/google_drive/download traversal safety', () => {
143146
.mockResolvedValueOnce(jsonResponse({ revisions: [] }))
144147

145148
const response = await POST(
146-
createMockRequest('POST', { accessToken: 'token-123', fileId: 'a..b', includeRevisions: true })
149+
createMockRequest('POST', {
150+
accessToken: 'token-123',
151+
fileId: 'a..b',
152+
includeRevisions: true,
153+
})
147154
)
148155
expect(response.status).toBe(200)
149156

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

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -83,20 +83,23 @@ beforeEach(() => {
8383
})
8484

8585
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-
mockSecureFetchWithPinnedIP
88-
.mockResolvedValueOnce(metadataResponse('doc-1'))
89-
.mockResolvedValueOnce(exportResponse())
90-
91-
const response = await POST(createMockRequest('POST', bodyFor(fileId)))
92-
93-
expect(response.status).toBe(400)
94-
const data = (await response.json()) as { success: boolean; error: string }
95-
expect(data.success).toBe(false)
96-
expect(data.error).toMatch(/fileId/)
97-
expect(mockValidateUrlWithDNS).not.toHaveBeenCalled()
98-
expect(mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
99-
})
86+
it.each(REJECTED)(
87+
'rejects fileId %j with a clean 400 and no outbound request',
88+
async (fileId) => {
89+
mockSecureFetchWithPinnedIP
90+
.mockResolvedValueOnce(metadataResponse('doc-1'))
91+
.mockResolvedValueOnce(exportResponse())
92+
93+
const response = await POST(createMockRequest('POST', bodyFor(fileId)))
94+
95+
expect(response.status).toBe(400)
96+
const data = (await response.json()) as { success: boolean; error: string }
97+
expect(data.success).toBe(false)
98+
expect(data.error).toMatch(/fileId/)
99+
expect(mockValidateUrlWithDNS).not.toHaveBeenCalled()
100+
expect(mockSecureFetchWithPinnedIP).not.toHaveBeenCalled()
101+
}
102+
)
100103

101104
it.each(NEUTRALIZED)('keeps fileId %j inside a single path segment', async (fileId) => {
102105
mockSecureFetchWithPinnedIP

‎apps/sim/app/api/tools/jira/add-attachment/route.test.ts‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* `add-attachment` is an internal Next.js route, so the reflective
5+
* `request.url` probe in `tools/jira/path_safety.test.ts` cannot see it: the
6+
* provider URL is assembled *here*, from body fields, not by the tool config.
7+
*
8+
* Both `cloudId` and `issueKey` land in a path segment on a POST that carries
9+
* the caller's OAuth bearer token and a multipart body. `encodeURIComponent`
10+
* would not help — `.` and `..` are unreserved and the WHATWG parser removes
11+
* them after decoding — so the sibling routes reject instead, and so must this
12+
* one. Every assertion resolves the outgoing URL through `new URL(...)`, the
13+
* same normalization `fetch` applies.
14+
*/
15+
import { createMockRequest, hybridAuthMockFns } from '@sim/testing'
16+
import { beforeEach, describe, expect, it, vi } from 'vitest'
17+
18+
const { mockProcessFilesToUserFiles, mockDownload, mockAssertToolFileAccess, mockGetJiraCloudId } =
19+
vi.hoisted(() => ({
20+
mockProcessFilesToUserFiles: vi.fn(),
21+
mockDownload: vi.fn(),
22+
mockAssertToolFileAccess: vi.fn(),
23+
mockGetJiraCloudId: vi.fn(),
24+
}))
25+
26+
vi.mock('@/lib/uploads/utils/file-utils', () => ({
27+
processFilesToUserFiles: mockProcessFilesToUserFiles,
28+
isInternalFileUrl: () => true,
29+
}))
30+
31+
vi.mock('@/lib/uploads/utils/file-utils.server', () => ({
32+
downloadServableFileFromStorage: mockDownload,
33+
}))
34+
35+
vi.mock('@/app/api/files/authorization', () => ({
36+
assertToolFileAccess: mockAssertToolFileAccess,
37+
}))
38+
39+
vi.mock('@/lib/uploads/utils/servable-file-response', () => ({
40+
docNotReadyResponse: () => null,
41+
}))
42+
43+
vi.mock('@/tools/jira/utils', () => ({
44+
getJiraCloudId: mockGetJiraCloudId,
45+
parseAtlassianErrorMessage: (status: number) => `Jira error ${status}`,
46+
}))
47+
48+
import { POST } from '@/app/api/tools/jira/add-attachment/route'
49+
50+
const CLOUD_ID = '1324a887-45db-1bf4-1e99-ef0ff456d421'
51+
const ORIGIN = 'https://api.atlassian.com'
52+
53+
const FILE = { key: 'workspace/u1/report.pdf', name: 'report.pdf', size: 12, type: 'application/pdf' }
54+
55+
function body(overrides: Record<string, unknown> = {}) {
56+
return {
57+
accessToken: 'inert-token',
58+
domain: 'example.atlassian.net',
59+
issueKey: 'PROJ-123',
60+
cloudId: CLOUD_ID,
61+
files: [FILE],
62+
...overrides,
63+
}
64+
}
65+
66+
let fetchMock: ReturnType<typeof vi.fn>
67+
68+
beforeEach(() => {
69+
vi.clearAllMocks()
70+
hybridAuthMockFns.mockCheckInternalAuth.mockResolvedValue({
71+
success: true,
72+
userId: 'user-1',
73+
authType: 'internal_jwt',
74+
})
75+
mockProcessFilesToUserFiles.mockReturnValue([FILE])
76+
mockAssertToolFileAccess.mockResolvedValue(null)
77+
mockDownload.mockResolvedValue({
78+
buffer: Buffer.from('pdf'),
79+
contentType: 'application/pdf',
80+
})
81+
mockGetJiraCloudId.mockResolvedValue(CLOUD_ID)
82+
fetchMock = vi.fn().mockResolvedValue({
83+
ok: true,
84+
status: 200,
85+
statusText: 'OK',
86+
json: async () => [{ id: '1', filename: 'report.pdf', mimeType: 'application/pdf', size: 3 }],
87+
text: async () => '',
88+
})
89+
vi.stubGlobal('fetch', fetchMock)
90+
})
91+
92+
function outgoingUrl(): URL {
93+
expect(fetchMock).toHaveBeenCalledTimes(1)
94+
return new URL(fetchMock.mock.calls[0][0] as string)
95+
}
96+
97+
describe('POST /api/tools/jira/add-attachment path safety', () => {
98+
/** The exact shape a legitimate call produces, asserted segment by segment. */
99+
it('builds the documented attachments path for legitimate values', async () => {
100+
const response = await POST(createMockRequest('POST', body()))
101+
expect(response.status).toBe(200)
102+
103+
const url = outgoingUrl()
104+
expect(url.origin).toBe(ORIGIN)
105+
expect(url.pathname.split('/')).toEqual([
106+
'',
107+
'ex',
108+
'jira',
109+
CLOUD_ID,
110+
'rest',
111+
'api',
112+
'3',
113+
'issue',
114+
'PROJ-123',
115+
'attachments',
116+
])
117+
expect([...url.searchParams.keys()]).toEqual([])
118+
})
119+
120+
it.each(['..', '.', 'a/../b', '%2e%2e', '..%2f..'])(
121+
'rejects issueKey=%j with a 400 and never calls Jira',
122+
async (issueKey) => {
123+
const response = await POST(createMockRequest('POST', body({ issueKey })))
124+
125+
expect(response.status).toBe(400)
126+
expect((await response.json()).error).toMatch(/issueKey/)
127+
expect(fetchMock).not.toHaveBeenCalled()
128+
}
129+
)
130+
131+
it.each(['..', '.', 'a/../b', '%2e%2e', '..%2f..'])(
132+
'rejects body-supplied cloudId=%j with a 400 and never calls Jira',
133+
async (cloudId) => {
134+
const response = await POST(createMockRequest('POST', body({ cloudId })))
135+
136+
expect(response.status).toBe(400)
137+
expect((await response.json()).error).toMatch(/cloudId/)
138+
expect(fetchMock).not.toHaveBeenCalled()
139+
}
140+
)
141+
142+
/** cloudId sits earlier in the path, so it is validated first, like the siblings. */
143+
it('reports cloudId before issueKey when both are hostile', async () => {
144+
const response = await POST(
145+
createMockRequest('POST', body({ cloudId: '..', issueKey: '..' }))
146+
)
147+
148+
expect(response.status).toBe(400)
149+
expect((await response.json()).error).toMatch(/cloudId/)
150+
})
151+
152+
/** A discovered cloudId is not caller-controlled, but it still shares the guard. */
153+
it('rejects a discovered cloudId that is a dot segment', async () => {
154+
mockGetJiraCloudId.mockResolvedValue('..')
155+
156+
const response = await POST(createMockRequest('POST', body({ cloudId: undefined })))
157+
158+
expect(response.status).toBe(400)
159+
expect(fetchMock).not.toHaveBeenCalled()
160+
})
161+
162+
it('still resolves the cloudId from the domain when the body omits it', async () => {
163+
const response = await POST(createMockRequest('POST', body({ cloudId: undefined })))
164+
165+
expect(response.status).toBe(200)
166+
expect(mockGetJiraCloudId).toHaveBeenCalledWith('example.atlassian.net', 'inert-token')
167+
expect(outgoingUrl().pathname.split('/')[3]).toBe(CLOUD_ID)
168+
})
169+
})

‎apps/sim/app/api/tools/jira/add-attachment/route.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { type NextRequest, NextResponse } from 'next/server'
44
import { jiraAddAttachmentContract } from '@/lib/api/contracts/selectors/jira'
55
import { parseRequest } from '@/lib/api/server'
66
import { checkInternalAuth } from '@/lib/auth/hybrid'
7+
import { validateJiraCloudId, validateJiraIssueKey } from '@/lib/core/security/input-validation'
78
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
89
import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types'
910
import { processFilesToUserFiles } from '@/lib/uploads/utils/file-utils'
@@ -44,6 +45,16 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
4445
validatedData.cloudId ||
4546
(await getJiraCloudId(validatedData.domain, validatedData.accessToken))
4647

48+
const cloudIdValidation = validateJiraCloudId(cloudId, 'cloudId')
49+
if (!cloudIdValidation.isValid) {
50+
return NextResponse.json({ error: cloudIdValidation.error }, { status: 400 })
51+
}
52+
53+
const issueKeyValidation = validateJiraIssueKey(validatedData.issueKey, 'issueKey')
54+
if (!issueKeyValidation.isValid) {
55+
return NextResponse.json({ error: issueKeyValidation.error }, { status: 400 })
56+
}
57+
4758
const formData = new FormData()
4859
// Every attachment lands in the same multipart body, so the ceiling covers the
4960
// set rather than each file on its own.

‎apps/sim/tools/google_drive/get_content.test.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,14 @@ function oversizedStreamedResponse(): Response {
5959
return new Response(stream, { status: 200 })
6060
}
6161

62+
/** Resolves to the rejection value, so a failure prints a boolean, not 10 MB. */
63+
async function rejectionOf(mimeType: string): Promise<unknown> {
64+
return run(mimeType).then(
65+
() => new Error('expected the content read to be rejected'),
66+
(error) => error
67+
)
68+
}
69+
6270
async function run(mimeType: string) {
6371
return getContentTool.transformResponse!(metadataResponse(mimeType), {
6472
accessToken: 'token-123',
@@ -76,19 +84,19 @@ describe('google_drive_get_content content cap', () => {
7684
it('rejects an export whose declared size exceeds the cap', async () => {
7785
mockFetch.mockResolvedValueOnce(oversizedDeclaredResponse())
7886

79-
await expect(run(DOC_MIME)).rejects.toSatisfy(isPayloadSizeLimitError)
87+
expect(isPayloadSizeLimitError(await rejectionOf(DOC_MIME))).toBe(true)
8088
})
8189

8290
it('rejects a download whose declared size exceeds the cap', async () => {
8391
mockFetch.mockResolvedValueOnce(oversizedDeclaredResponse())
8492

85-
await expect(run('application/pdf')).rejects.toSatisfy(isPayloadSizeLimitError)
93+
expect(isPayloadSizeLimitError(await rejectionOf('application/pdf'))).toBe(true)
8694
})
8795

8896
it('aborts a streamed download that grows past the cap with no content-length', async () => {
8997
mockFetch.mockResolvedValueOnce(oversizedStreamedResponse())
9098

91-
await expect(run('application/pdf')).rejects.toSatisfy(isPayloadSizeLimitError)
99+
expect(isPayloadSizeLimitError(await rejectionOf('application/pdf'))).toBe(true)
92100
})
93101

94102
it('returns content under the cap unchanged', async () => {

‎apps/sim/tools/google_drive/get_content.ts‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { createLogger } from '@sim/logger'
2+
import { readResponseTextWithLimit } from '@/lib/core/utils/stream-limits'
23
import type {
34
GoogleDriveFile,
45
GoogleDriveGetContentResponse,
@@ -10,12 +11,24 @@ import {
1011
ALL_REVISION_FIELDS,
1112
DEFAULT_EXPORT_FORMATS,
1213
GOOGLE_WORKSPACE_MIME_TYPES,
14+
MAX_EXPORT_BYTES,
1315
} from '@/tools/google_drive/utils'
1416
import type { ToolConfig } from '@/tools/types'
1517
import { safeUrlPathSegment } from '@/tools/url-path'
1618

1719
const logger = createLogger('GoogleDriveGetContentTool')
1820

21+
/**
22+
* Both content fetches below run inside `transformResponse` on global `fetch`,
23+
* outside the shared tool transport, so the executor's own response-body bound
24+
* never reaches them. `MAX_EXPORT_BYTES` (10 MB) is reused rather than a new
25+
* number: it is the transport's own per-tool response ceiling and the limit the
26+
* sibling `/api/tools/google_drive/export` route already enforces, so the two
27+
* hops of this one tool stop contradicting each other. The download route's
28+
* 100 MB `MAX_FILE_SIZE` is deliberately *not* copied — that route base64s
29+
* bytes into a file artifact, whereas this tool decodes to a UTF-8 string that
30+
* lands in a workflow variable and an LLM context, where 100 MB is unusable.
31+
*/
1932
export const getContentTool: ToolConfig<GoogleDriveToolParams, GoogleDriveGetContentResponse> = {
2033
id: 'google_drive_get_content',
2134
name: 'Get Content from Google Drive',
@@ -110,7 +123,10 @@ export const getContentTool: ToolConfig<GoogleDriveToolParams, GoogleDriveGetCon
110123
throw new Error(exportError.error?.message || 'Failed to export Google Workspace file')
111124
}
112125

113-
content = await exportResponse.text()
126+
content = await readResponseTextWithLimit(exportResponse, {
127+
maxBytes: MAX_EXPORT_BYTES,
128+
label: `Google Drive export of file ${fileId}`,
129+
})
114130
} else {
115131
logger.info('Downloading regular file', {
116132
fileId,
@@ -136,7 +152,10 @@ export const getContentTool: ToolConfig<GoogleDriveToolParams, GoogleDriveGetCon
136152
throw new Error(downloadError.error?.message || 'Failed to download file')
137153
}
138154

139-
content = await downloadResponse.text()
155+
content = await readResponseTextWithLimit(downloadResponse, {
156+
maxBytes: MAX_EXPORT_BYTES,
157+
label: `Google Drive file ${fileId}`,
158+
})
140159
}
141160

142161
const includeRevisions = params?.includeRevisions !== false

0 commit comments

Comments
 (0)