Skip to content

Commit 55c2c2c

Browse files
waleedlatif1waleedlatif1
committed
fix(url-path,github): stop the guards rejecting and rewriting legal values
Three false rejections the guards introduced, all verified live before changing: GitHub label names legitimately contain slashes -- area/apiserver, kind/bug -- and both the literal and encoded forms return 200 with the same label id, with GitHub echoing the literal form as canonical. remove_label was using the single-segment guard, so it hard-threw on a label GitHub accepts. safeUrlPath trimmed each segment, so a Supabase key of 'folder/ report .csv' was silently rewritten to a different object -- 404 or the wrong file, with no error. Supabase's own server regex permits a literal space in both object keys and bucket names. Now only the whole value is trimmed. This cannot re-open traversal: the URL parser pops a segment only when it is exactly '..', and an encoded space keeps it inert. A trailing slash on a GitHub contents path used to work -- GitHub 302s to the slash-free form and fetch follows it -- and get_tree's own description invites a directory path. Stripped at the GitHub callsites via a local helper rather than weakening safeUrlPath for its other ~700 callers. Also: a number reaching a string param became '' and threw 'X is required' even though it was supplied. That regressed the 53 sites whose baseline was a bare interpolation. Now coerced with String(), with null and undefined rejected before coercion so they still throw rather than becoming 'null'.
1 parent ff26be9 commit 55c2c2c

10 files changed

Lines changed: 334 additions & 33 deletions

File tree

‎apps/sim/tools/github/contents_path.ts‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import { safeUrlPath } from '@/tools/url-path'
2+
3+
/**
4+
* Builds a traversal-safe repository file or directory path for the GitHub
5+
* contents API, tolerating a single trailing `/`.
6+
*
7+
* `safeUrlPath` rejects a trailing slash because an empty segment usually means
8+
* the caller's value was malformed. On the contents API it does not: GitHub
9+
* normalizes the slash away itself, answering `/contents/packages/` with a
10+
* `302` whose `Location` is the slash-free `/contents/packages`, which `fetch`
11+
* follows to the same `200` the bare form returns. The two spellings address
12+
* the identical resource, so rejecting one is a false rejection — and these
13+
* params are `visibility: 'user-or-llm'` and documented as directory paths, so
14+
* a model writing `src/components/` is the natural case.
15+
*
16+
* Only one trailing slash is stripped, and only here: `a//b`, a leading `/`,
17+
* and `a///` still reach `safeUrlPath` with an empty segment and are still
18+
* rejected, as are `.` and `..` segments. Stripping at this callsite rather
19+
* than in the helper keeps the provider-specific normalization next to the
20+
* provider that performs it.
21+
*
22+
* @param value - The raw repository path, typically LLM- or user-supplied.
23+
* @param paramName - The parameter name, used to name the offender in errors.
24+
* @returns The encoded path, with `/` preserved between segments.
25+
*/
26+
export function safeGithubContentsPath(value: string, paramName: string): string {
27+
const raw = typeof value === 'string' ? value.trim() : ''
28+
const withoutTrailingSlash = raw.endsWith('/') ? raw.slice(0, -1) : raw
29+
30+
return safeUrlPath(withoutTrailingSlash, paramName)
31+
}

‎apps/sim/tools/github/create_file.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1+
import { safeGithubContentsPath } from '@/tools/github/contents_path'
12
import type { CreateFileParams, FileOperationResponse } from '@/tools/github/types'
23
import type { ToolConfig } from '@/tools/types'
3-
import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path'
4+
import { safeUrlPathSegment } from '@/tools/url-path'
45

56
export const createFileTool: ToolConfig<CreateFileParams, FileOperationResponse> = {
67
id: 'github_create_file',
@@ -56,7 +57,7 @@ export const createFileTool: ToolConfig<CreateFileParams, FileOperationResponse>
5657

5758
request: {
5859
url: (params) =>
59-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`,
60+
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeGithubContentsPath(params.path, 'path')}`,
6061
method: 'PUT',
6162
headers: (params) => ({
6263
Accept: 'application/vnd.github+json',

‎apps/sim/tools/github/delete_file.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1+
import { safeGithubContentsPath } from '@/tools/github/contents_path'
12
import type { DeleteFileParams, DeleteFileResponse } from '@/tools/github/types'
23
import type { ToolConfig } from '@/tools/types'
3-
import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path'
4+
import { safeUrlPathSegment } from '@/tools/url-path'
45

56
export const deleteFileTool: ToolConfig<DeleteFileParams, DeleteFileResponse> = {
67
id: 'github_delete_file',
@@ -56,7 +57,7 @@ export const deleteFileTool: ToolConfig<DeleteFileParams, DeleteFileResponse> =
5657

5758
request: {
5859
url: (params) =>
59-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`,
60+
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeGithubContentsPath(params.path, 'path')}`,
6061
method: 'DELETE',
6162
headers: (params) => ({
6263
Accept: 'application/vnd.github+json',

‎apps/sim/tools/github/get_file_content.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils'
2+
import { safeGithubContentsPath } from '@/tools/github/contents_path'
23
import type { FileContentResponse, GetFileContentParams } from '@/tools/github/types'
34
import type { ToolConfig } from '@/tools/types'
4-
import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path'
5+
import { safeUrlPathSegment } from '@/tools/url-path'
56

67
export const getFileContentTool: ToolConfig<GetFileContentParams, FileContentResponse> = {
78
id: 'github_get_file_content',
@@ -45,7 +46,7 @@ export const getFileContentTool: ToolConfig<GetFileContentParams, FileContentRes
4546

4647
request: {
4748
url: (params) => {
48-
const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`
49+
const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeGithubContentsPath(params.path, 'path')}`
4950
return params.ref ? `${baseUrl}?ref=${params.ref}` : baseUrl
5051
},
5152
method: 'GET',

‎apps/sim/tools/github/get_tree.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1+
import { safeGithubContentsPath } from '@/tools/github/contents_path'
12
import type { GetTreeParams, TreeResponse } from '@/tools/github/types'
23
import type { ToolConfig } from '@/tools/types'
3-
import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path'
4+
import { safeUrlPathSegment } from '@/tools/url-path'
45

56
export const getTreeTool: ToolConfig<GetTreeParams, TreeResponse> = {
67
id: 'github_get_tree',
@@ -45,7 +46,9 @@ export const getTreeTool: ToolConfig<GetTreeParams, TreeResponse> = {
4546

4647
request: {
4748
url: (params) => {
48-
const path = params.path ? safeUrlPath(params.path, 'path') : ''
49+
const rawPath = (params.path ?? '').trim()
50+
const addressesRoot = rawPath === '' || rawPath === '/'
51+
const path = addressesRoot ? '' : safeGithubContentsPath(rawPath, 'path')
4952
const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${path}`
5053
return params.ref ? `${baseUrl}?ref=${params.ref}` : baseUrl
5154
},

‎apps/sim/tools/github/path_safety.test.ts‎

Lines changed: 93 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,29 @@ import type { ToolConfig } from '@/tools/types'
2828

2929
type AnyTool = ToolConfig<any, any>
3030

31-
/** Parameters that legitimately span several path segments. */
32-
const MULTI_SEGMENT_PARAMS = new Set(['path', 'branch', 'ref'])
31+
/**
32+
* Parameters that legitimately span several path segments.
33+
*
34+
* `name` is `remove_label`'s label name. GitHub places no character
35+
* restriction on a label name and slashes are conventional in the wild
36+
* (`area/apiserver`, `kind/bug`, `sig/network`). Verified live against
37+
* `kubernetes/kubernetes`: `labels/area/apiserver` and `labels/area%2Fapiserver`
38+
* both return `200` for the same label id, and GitHub echoes the *literal*
39+
* slash form as the label's canonical `url`. A single-segment guard rejected
40+
* every such label.
41+
*/
42+
const MULTI_SEGMENT_PARAMS = new Set(['path', 'branch', 'ref', 'name'])
43+
44+
/**
45+
* Parameters whose provider normalizes a single trailing `/` away, so the
46+
* tool strips it rather than rejecting the value.
47+
*
48+
* Only the GitHub contents `path`. Verified live against `vercel/next.js`:
49+
* `contents/packages/` returns `302` with `Location: .../contents/packages`,
50+
* which `fetch` follows to the same `200` the bare form returns. See
51+
* `@/tools/github/contents_path`.
52+
*/
53+
const TRAILING_SLASH_TOLERANT_PARAMS = new Set(['path'])
3354

3455
/**
3556
* `compare_commits` interpolates `base` and `head` into `{base}...{head}`.
@@ -44,7 +65,10 @@ const UNVERIFIED_SITES = new Set(['github_compare_commits:base', 'github_compare
4465

4566
const REJECTED_ANYWHERE = ['..', '.', ' .. ', '\\..\\..'] as const
4667
const REJECTED_SINGLE_ONLY = ['a/../../b', 'a/b'] as const
47-
const REJECTED_MULTI_ONLY = ['/leading', 'trailing/', 'a//b', 'a/../b'] as const
68+
const REJECTED_MULTI_ONLY = ['/leading', 'a//b', 'a/../b'] as const
69+
70+
/** Rejected on multi-segment params that their provider does NOT normalize. */
71+
const REJECTED_TRAILING_SLASH = ['trailing/'] as const
4872

4973
/** Must NOT throw — encoding already neutralizes them — but must not reshape the path. */
5074
const NEUTRALIZED = ['%2e%2e', '..%2f..', 'x?foo=attacker'] as const
@@ -142,9 +166,11 @@ describe('github path-parameter traversal safety', () => {
142166

143167
describe('guards every path param independently', () => {
144168
describe.each(SITES)('$name', (site) => {
169+
const tolerantOfTrailingSlash = site.multi && TRAILING_SLASH_TOLERANT_PARAMS.has(site.param)
145170
const rejected = [
146171
...REJECTED_ANYWHERE,
147172
...(site.multi ? REJECTED_MULTI_ONLY : REJECTED_SINGLE_ONLY),
173+
...(tolerantOfTrailingSlash ? [] : site.multi ? REJECTED_TRAILING_SLASH : []),
148174
]
149175

150176
it.each(rejected)('rejects %j', (value) => {
@@ -173,6 +199,70 @@ describe('github path-parameter traversal safety', () => {
173199
expect(url.pathname).not.toContain('%2f')
174200
}
175201
)
202+
203+
if (tolerantOfTrailingSlash) {
204+
it.each(['src/components', 'packages'] as const)(
205+
'resolves %j identically with and without a trailing slash',
206+
(value) => {
207+
const bare = buildUrl(site.tool, { [site.param]: value })
208+
const trailing = buildUrl(site.tool, { [site.param]: `${value}/` })
209+
210+
expect(trailing.href).toBe(bare.href)
211+
expect(segmentsOf(trailing)).toEqual([
212+
...site.prefix,
213+
...value.split('/'),
214+
...site.suffix,
215+
])
216+
}
217+
)
218+
219+
it('still rejects a doubled trailing slash', () => {
220+
expect(() => buildUrl(site.tool, { [site.param]: 'src/components//' })).toThrow(
221+
new RegExp(site.param)
222+
)
223+
})
224+
}
176225
})
177226
})
178227
})
228+
229+
describe('github label names carry slashes', () => {
230+
it('builds the literal-slash label URL GitHub returns as canonical', () => {
231+
const url = new URL(
232+
(githubTools.githubRemoveLabelTool.request!.url as (p: any) => string)({
233+
owner: 'kubernetes',
234+
repo: 'kubernetes',
235+
issue_number: 1,
236+
name: 'area/apiserver',
237+
})
238+
)
239+
240+
expect(url.pathname).toBe('/repos/kubernetes/kubernetes/issues/1/labels/area/apiserver')
241+
expect(url.pathname).not.toContain('%2F')
242+
})
243+
244+
it.each(['kind/bug', 'sig/network', 'priority/important-soon'] as const)(
245+
'accepts the conventional label name %j',
246+
(name) => {
247+
expect(() =>
248+
(githubTools.githubRemoveLabelTool.request!.url as (p: any) => string)({
249+
owner: 'o',
250+
repo: 'r',
251+
issue_number: 1,
252+
name,
253+
})
254+
).not.toThrow()
255+
}
256+
)
257+
258+
it.each(['..', '.', 'a/../b'] as const)('still rejects the traversal label %j', (name) => {
259+
expect(() =>
260+
(githubTools.githubRemoveLabelTool.request!.url as (p: any) => string)({
261+
owner: 'o',
262+
repo: 'r',
263+
issue_number: 1,
264+
name,
265+
})
266+
).toThrow(/name/)
267+
})
268+
})

‎apps/sim/tools/github/remove_label.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { LabelsResponse, RemoveLabelParams } from '@/tools/github/types'
22
import type { ToolConfig } from '@/tools/types'
3-
import { safeUrlPathSegment } from '@/tools/url-path'
3+
import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path'
44

55
export const removeLabelTool: ToolConfig<RemoveLabelParams, LabelsResponse> = {
66
id: 'github_remove_label',
@@ -43,7 +43,7 @@ export const removeLabelTool: ToolConfig<RemoveLabelParams, LabelsResponse> = {
4343

4444
request: {
4545
url: (params) =>
46-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(String(params.issue_number), 'issue_number')}/labels/${safeUrlPathSegment(params.name, 'name')}`,
46+
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(String(params.issue_number), 'issue_number')}/labels/${safeUrlPath(params.name, 'name')}`,
4747
method: 'DELETE',
4848
headers: (params) => ({
4949
Accept: 'application/vnd.github.v3+json',

‎apps/sim/tools/github/update_file.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1+
import { safeGithubContentsPath } from '@/tools/github/contents_path'
12
import type { FileOperationResponse, UpdateFileParams } from '@/tools/github/types'
23
import type { ToolConfig } from '@/tools/types'
3-
import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path'
4+
import { safeUrlPathSegment } from '@/tools/url-path'
45

56
export const updateFileTool: ToolConfig<UpdateFileParams, FileOperationResponse> = {
67
id: 'github_update_file',
@@ -62,7 +63,7 @@ export const updateFileTool: ToolConfig<UpdateFileParams, FileOperationResponse>
6263

6364
request: {
6465
url: (params) =>
65-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`,
66+
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeGithubContentsPath(params.path, 'path')}`,
6667
method: 'PUT',
6768
headers: (params) => ({
6869
Accept: 'application/vnd.github+json',

0 commit comments

Comments
 (0)