From d36e6c8071bfbf08d039d94098c1c89c66bc1449 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 20:25:40 -0700 Subject: [PATCH 1/6] fix(github): reject path-traversal values in interpolated URL segments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub tools interpolate LLM-writable values (owner, repo, issue_number, pullNumber, path, branch, ref, label name, gist_id, ...) straight into the request path. A value of `..` re-aims an authenticated request — carrying the workspace's GitHub token — at a different resource, including on DELETE routes such as delete_file, delete_release and delete_branch. Guards every such site with the helpers in tools/url-path.ts, and adds two new helpers there for the parameter shapes GitHub actually has: - safeUrlPath, for values that legitimately carry `/` as structure (path, branch, ref, base, head) - safeEncodedUrlPathSegment, for a value the provider reads as ONE path parameter that may still contain `/` (a namespaced label such as `area/api`) Adds tools/github/path_safety.test.ts, which enumerates tools from the barrel and probes every parameter that reaches the path, so a new unguarded parameter fails CI. --- apps/sim/lib/internal/github/operations.ts | 9 +- apps/sim/tools/github/add_assignees.ts | 3 +- apps/sim/tools/github/add_labels.ts | 3 +- apps/sim/tools/github/cancel_workflow_run.ts | 3 +- apps/sim/tools/github/check_star.ts | 4 +- apps/sim/tools/github/close_issue.ts | 3 +- apps/sim/tools/github/close_pr.ts | 3 +- apps/sim/tools/github/compare_commits.ts | 3 +- apps/sim/tools/github/create_branch.ts | 4 +- .../tools/github/create_comment_reaction.ts | 3 +- apps/sim/tools/github/create_file.ts | 3 +- apps/sim/tools/github/create_issue.ts | 4 +- .../sim/tools/github/create_issue_reaction.ts | 3 +- apps/sim/tools/github/create_milestone.ts | 4 +- apps/sim/tools/github/create_pr.ts | 4 +- apps/sim/tools/github/create_pr_review.ts | 3 +- apps/sim/tools/github/create_release.ts | 4 +- apps/sim/tools/github/delete_branch.ts | 3 +- apps/sim/tools/github/delete_comment.ts | 3 +- .../tools/github/delete_comment_reaction.ts | 3 +- apps/sim/tools/github/delete_file.ts | 3 +- apps/sim/tools/github/delete_gist.ts | 4 +- .../sim/tools/github/delete_issue_reaction.ts | 3 +- apps/sim/tools/github/delete_milestone.ts | 3 +- apps/sim/tools/github/delete_release.ts | 3 +- apps/sim/tools/github/fork_gist.ts | 4 +- apps/sim/tools/github/fork_repo.ts | 4 +- apps/sim/tools/github/get_branch.ts | 3 +- .../sim/tools/github/get_branch_protection.ts | 3 +- apps/sim/tools/github/get_commit.ts | 3 +- apps/sim/tools/github/get_file_content.ts | 5 +- apps/sim/tools/github/get_gist.ts | 4 +- apps/sim/tools/github/get_issue.ts | 3 +- apps/sim/tools/github/get_latest_release.ts | 4 +- apps/sim/tools/github/get_milestone.ts | 3 +- apps/sim/tools/github/get_pr_files.ts | 3 +- apps/sim/tools/github/get_readme.ts | 3 +- apps/sim/tools/github/get_release.ts | 3 +- apps/sim/tools/github/get_tree.ts | 7 +- apps/sim/tools/github/get_workflow.ts | 3 +- apps/sim/tools/github/get_workflow_run.ts | 3 +- apps/sim/tools/github/issue_comment.ts | 3 +- apps/sim/tools/github/job_logs.test.ts | 16 +- apps/sim/tools/github/job_logs.ts | 3 +- apps/sim/tools/github/list_branches.ts | 3 +- apps/sim/tools/github/list_commits.ts | 5 +- apps/sim/tools/github/list_forks.ts | 5 +- apps/sim/tools/github/list_gists.ts | 3 +- apps/sim/tools/github/list_issue_comments.ts | 3 +- apps/sim/tools/github/list_issues.ts | 5 +- apps/sim/tools/github/list_milestones.ts | 5 +- apps/sim/tools/github/list_pr_comments.ts | 3 +- apps/sim/tools/github/list_prs.ts | 5 +- apps/sim/tools/github/list_releases.ts | 5 +- apps/sim/tools/github/list_stargazers.ts | 5 +- apps/sim/tools/github/list_tags.ts | 5 +- apps/sim/tools/github/list_workflow_runs.ts | 3 +- apps/sim/tools/github/list_workflows.ts | 3 +- apps/sim/tools/github/merge_pr.ts | 3 +- apps/sim/tools/github/path_safety.test.ts | 240 ++++++++++++++++++ apps/sim/tools/github/pr.ts | 5 +- apps/sim/tools/github/remove_label.ts | 3 +- apps/sim/tools/github/repo_info.ts | 4 +- apps/sim/tools/github/request_reviewers.ts | 3 +- apps/sim/tools/github/rerun_workflow.ts | 3 +- apps/sim/tools/github/star_gist.ts | 4 +- apps/sim/tools/github/star_repo.ts | 4 +- apps/sim/tools/github/trigger_workflow.ts | 3 +- apps/sim/tools/github/unstar_gist.ts | 4 +- apps/sim/tools/github/unstar_repo.ts | 4 +- .../tools/github/update_branch_protection.ts | 3 +- apps/sim/tools/github/update_comment.ts | 3 +- apps/sim/tools/github/update_file.ts | 3 +- apps/sim/tools/github/update_gist.ts | 4 +- apps/sim/tools/github/update_issue.ts | 3 +- apps/sim/tools/github/update_milestone.ts | 3 +- apps/sim/tools/github/update_pr.ts | 3 +- apps/sim/tools/github/update_release.ts | 3 +- apps/sim/tools/url-path.ts | 123 +++++++++ 79 files changed, 566 insertions(+), 88 deletions(-) create mode 100644 apps/sim/tools/github/path_safety.test.ts diff --git a/apps/sim/lib/internal/github/operations.ts b/apps/sim/lib/internal/github/operations.ts index 49522552564..d42c476d7a4 100644 --- a/apps/sim/lib/internal/github/operations.ts +++ b/apps/sim/lib/internal/github/operations.ts @@ -19,6 +19,7 @@ import type { } from '@/tools/github/types' import { secureGitHubRequest } from '@/tools/github/utils.server' import type { ToolResponse } from '@/tools/types' +import { safeEncodedUrlPathSegment, safeUrlPathSegment } from '@/tools/url-path' const logger = createLogger('GitHubLatestCommitOperation') const MAX_COMMIT_RESPONSE_BYTES = 10 * 1024 * 1024 @@ -98,7 +99,7 @@ function githubHeaders(apiKey: string): Record { } function pullRequestUrl(params: CreateCommentParams): string { - return `${GITHUB_API_BASE}/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}` + return `${GITHUB_API_BASE}/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}` } function isFileCommentRequest(params: CreateCommentParams): boolean { @@ -352,9 +353,9 @@ export async function getGitHubLatestCommit( context: GitHubOperationContext ): Promise { context.signal?.throwIfAborted() - const owner = encodeURIComponent(input.owner) - const repo = encodeURIComponent(input.repo) - const revision = encodeURIComponent(input.branch || 'HEAD') + const owner = safeUrlPathSegment(input.owner, 'owner') + const repo = safeUrlPathSegment(input.repo, 'repo') + const revision = safeEncodedUrlPathSegment(input.branch || 'HEAD', 'branch') const commitUrl = `https://api.github.com/repos/${owner}/${repo}/commits/${revision}` const validation = await validateUrlWithDNS(commitUrl, 'commitUrl') context.signal?.throwIfAborted() diff --git a/apps/sim/tools/github/add_assignees.ts b/apps/sim/tools/github/add_assignees.ts index 33665a70aa9..074c4bca3e4 100644 --- a/apps/sim/tools/github/add_assignees.ts +++ b/apps/sim/tools/github/add_assignees.ts @@ -1,5 +1,6 @@ import type { AddAssigneesParams, IssueResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const addAssigneesTool: ToolConfig = { id: 'github_add_assignees', @@ -42,7 +43,7 @@ export const addAssigneesTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}/assignees`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/assignees`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/add_labels.ts b/apps/sim/tools/github/add_labels.ts index 6b30b6cb04c..efbfc3ef565 100644 --- a/apps/sim/tools/github/add_labels.ts +++ b/apps/sim/tools/github/add_labels.ts @@ -1,5 +1,6 @@ import type { AddLabelsParams, LabelsResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const addLabelsTool: ToolConfig = { id: 'github_add_labels', @@ -42,7 +43,7 @@ export const addLabelsTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}/labels`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/labels`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/cancel_workflow_run.ts b/apps/sim/tools/github/cancel_workflow_run.ts index e07d4e1b221..acc7269c6f8 100644 --- a/apps/sim/tools/github/cancel_workflow_run.ts +++ b/apps/sim/tools/github/cancel_workflow_run.ts @@ -1,5 +1,6 @@ import type { CancelWorkflowRunParams, CancelWorkflowRunResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const cancelWorkflowRunTool: ToolConfig = { @@ -38,7 +39,7 @@ export const cancelWorkflowRunTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/actions/runs/${params.run_id}/cancel`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/runs/${safeUrlPathSegment(params.run_id, 'run_id')}/cancel`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/check_star.ts b/apps/sim/tools/github/check_star.ts index b269d1e8f2f..2e07dcc1321 100644 --- a/apps/sim/tools/github/check_star.ts +++ b/apps/sim/tools/github/check_star.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface CheckStarParams { owner: string @@ -46,7 +47,8 @@ export const checkStarTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/user/starred/${params.owner}/${params.repo}`, + url: (params) => + `https://api.github.com/user/starred/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/close_issue.ts b/apps/sim/tools/github/close_issue.ts index 1d5a7576683..0d05d096072 100644 --- a/apps/sim/tools/github/close_issue.ts +++ b/apps/sim/tools/github/close_issue.ts @@ -1,6 +1,7 @@ import type { CloseIssueParams, IssueResponse } from '@/tools/github/types' import { ISSUE_OUTPUT_PROPERTIES, LABEL_OUTPUT, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const closeIssueTool: ToolConfig = { id: 'github_close_issue', @@ -43,7 +44,7 @@ export const closeIssueTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/close_pr.ts b/apps/sim/tools/github/close_pr.ts index ae8b15eede2..948b525fb9a 100644 --- a/apps/sim/tools/github/close_pr.ts +++ b/apps/sim/tools/github/close_pr.ts @@ -1,5 +1,6 @@ import type { ClosePRParams, PRResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const closePRTool: ToolConfig = { id: 'github_close_pr', @@ -36,7 +37,7 @@ export const closePRTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/compare_commits.ts b/apps/sim/tools/github/compare_commits.ts index b5c44f563f6..9d601a883e5 100644 --- a/apps/sim/tools/github/compare_commits.ts +++ b/apps/sim/tools/github/compare_commits.ts @@ -4,6 +4,7 @@ import { USER_FULL_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' interface CompareCommitsParams { owner: string @@ -103,7 +104,7 @@ export const compareCommitsTool: ToolConfig { const url = new URL( - `https://api.github.com/repos/${params.owner}/${params.repo}/compare/${params.base}...${params.head}` + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/compare/${safeUrlPath(params.base, 'base')}...${safeUrlPath(params.head, 'head')}` ) if (params.per_page) url.searchParams.append('per_page', String(params.per_page)) if (params.page) url.searchParams.append('page', String(params.page)) diff --git a/apps/sim/tools/github/create_branch.ts b/apps/sim/tools/github/create_branch.ts index c32919e6051..ee8b3a56468 100644 --- a/apps/sim/tools/github/create_branch.ts +++ b/apps/sim/tools/github/create_branch.ts @@ -1,6 +1,7 @@ import type { CreateBranchParams, RefResponse } from '@/tools/github/types' import { GIT_REF_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createBranchTool: ToolConfig = { id: 'github_create_branch', @@ -43,7 +44,8 @@ export const createBranchTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/repos/${params.owner}/${params.repo}/git/refs`, + url: (params) => + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/git/refs`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/create_comment_reaction.ts b/apps/sim/tools/github/create_comment_reaction.ts index b24511818bf..a9dbb599201 100644 --- a/apps/sim/tools/github/create_comment_reaction.ts +++ b/apps/sim/tools/github/create_comment_reaction.ts @@ -1,5 +1,6 @@ import { REACTION_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface CreateCommentReactionParams { owner: string @@ -67,7 +68,7 @@ export const createCommentReactionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/comments/${params.comment_id}/reactions`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}/reactions`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.squirrel-girl-preview+json', diff --git a/apps/sim/tools/github/create_file.ts b/apps/sim/tools/github/create_file.ts index 803c6e3d771..b5ca7b5f7ce 100644 --- a/apps/sim/tools/github/create_file.ts +++ b/apps/sim/tools/github/create_file.ts @@ -1,5 +1,6 @@ import type { CreateFileParams, FileOperationResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const createFileTool: ToolConfig = { id: 'github_create_file', @@ -55,7 +56,7 @@ export const createFileTool: ToolConfig request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/contents/${params.path}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/create_issue.ts b/apps/sim/tools/github/create_issue.ts index 99c405eb022..b49e89b8268 100644 --- a/apps/sim/tools/github/create_issue.ts +++ b/apps/sim/tools/github/create_issue.ts @@ -6,6 +6,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createIssueTool: ToolConfig = { id: 'github_create_issue', @@ -65,7 +66,8 @@ export const createIssueTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/repos/${params.owner}/${params.repo}/issues`, + url: (params) => + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/create_issue_reaction.ts b/apps/sim/tools/github/create_issue_reaction.ts index cec36d0215d..52f3ae0cfd6 100644 --- a/apps/sim/tools/github/create_issue_reaction.ts +++ b/apps/sim/tools/github/create_issue_reaction.ts @@ -1,5 +1,6 @@ import { REACTION_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface CreateIssueReactionParams { owner: string @@ -67,7 +68,7 @@ export const createIssueReactionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}/reactions`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/reactions`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.squirrel-girl-preview+json', diff --git a/apps/sim/tools/github/create_milestone.ts b/apps/sim/tools/github/create_milestone.ts index 5f82e5370e9..458510eea8e 100644 --- a/apps/sim/tools/github/create_milestone.ts +++ b/apps/sim/tools/github/create_milestone.ts @@ -1,5 +1,6 @@ import { MILESTONE_CREATOR_OUTPUT, MILESTONE_V2_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface CreateMilestoneParams { owner: string @@ -83,7 +84,8 @@ export const createMilestoneTool: ToolConfig `https://api.github.com/repos/${params.owner}/${params.repo}/milestones`, + url: (params) => + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/milestones`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/create_pr.ts b/apps/sim/tools/github/create_pr.ts index 8faced4eaa9..a02bea2b783 100644 --- a/apps/sim/tools/github/create_pr.ts +++ b/apps/sim/tools/github/create_pr.ts @@ -1,5 +1,6 @@ import type { CreatePRParams, PRResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createPRTool: ToolConfig = { id: 'github_create_pr', @@ -59,7 +60,8 @@ export const createPRTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/repos/${params.owner}/${params.repo}/pulls`, + url: (params) => + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/create_pr_review.ts b/apps/sim/tools/github/create_pr_review.ts index 09c3335689e..5aafe897741 100644 --- a/apps/sim/tools/github/create_pr_review.ts +++ b/apps/sim/tools/github/create_pr_review.ts @@ -19,6 +19,7 @@ import type { } from '@/tools/github/types' import { USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig, ToolResponse } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' const COMMIT_SHA_PATTERN = /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/i @@ -159,7 +160,7 @@ export const createPRReviewTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}/reviews`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}/reviews`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/create_release.ts b/apps/sim/tools/github/create_release.ts index b10fac70d92..462d3c22caf 100644 --- a/apps/sim/tools/github/create_release.ts +++ b/apps/sim/tools/github/create_release.ts @@ -5,6 +5,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createReleaseTool: ToolConfig = { id: 'github_create_release', @@ -75,7 +76,8 @@ export const createReleaseTool: ToolConfig }, request: { - url: (params) => `https://api.github.com/repos/${params.owner}/${params.repo}/releases`, + url: (params) => + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/delete_branch.ts b/apps/sim/tools/github/delete_branch.ts index 65555453192..783691e9e1b 100644 --- a/apps/sim/tools/github/delete_branch.ts +++ b/apps/sim/tools/github/delete_branch.ts @@ -1,6 +1,7 @@ import type { DeleteBranchParams, DeleteBranchResponse } from '@/tools/github/types' import { DELETE_BRANCH_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const deleteBranchTool: ToolConfig = { id: 'github_delete_branch', @@ -38,7 +39,7 @@ export const deleteBranchTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/git/refs/heads/${params.branch}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/git/refs/heads/${safeUrlPath(params.branch, 'branch')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/delete_comment.ts b/apps/sim/tools/github/delete_comment.ts index 4ed63a16689..00f30ee0921 100644 --- a/apps/sim/tools/github/delete_comment.ts +++ b/apps/sim/tools/github/delete_comment.ts @@ -1,5 +1,6 @@ import type { DeleteCommentParams, DeleteCommentResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const deleteCommentTool: ToolConfig = { id: 'github_delete_comment', @@ -36,7 +37,7 @@ export const deleteCommentTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/comments/${params.comment_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/delete_comment_reaction.ts b/apps/sim/tools/github/delete_comment_reaction.ts index 7708bfe3a13..2845c4bef23 100644 --- a/apps/sim/tools/github/delete_comment_reaction.ts +++ b/apps/sim/tools/github/delete_comment_reaction.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface DeleteCommentReactionParams { owner: string @@ -63,7 +64,7 @@ export const deleteCommentReactionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/comments/${params.comment_id}/reactions/${params.reaction_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}/reactions/${safeUrlPathSegment(params.reaction_id, 'reaction_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.squirrel-girl-preview+json', diff --git a/apps/sim/tools/github/delete_file.ts b/apps/sim/tools/github/delete_file.ts index 1cc72870946..671fa021262 100644 --- a/apps/sim/tools/github/delete_file.ts +++ b/apps/sim/tools/github/delete_file.ts @@ -1,5 +1,6 @@ import type { DeleteFileParams, DeleteFileResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const deleteFileTool: ToolConfig = { id: 'github_delete_file', @@ -55,7 +56,7 @@ export const deleteFileTool: ToolConfig = request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/contents/${params.path}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/delete_gist.ts b/apps/sim/tools/github/delete_gist.ts index da9e884047c..fdebeb8fe16 100644 --- a/apps/sim/tools/github/delete_gist.ts +++ b/apps/sim/tools/github/delete_gist.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface DeleteGistParams { gist_id: string @@ -38,7 +39,8 @@ export const deleteGistTool: ToolConfig = }, request: { - url: (params) => `https://api.github.com/gists/${params.gist_id?.trim()}`, + url: (params) => + `https://api.github.com/gists/${safeUrlPathSegment(params.gist_id, 'gist_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/delete_issue_reaction.ts b/apps/sim/tools/github/delete_issue_reaction.ts index 410d398026e..a048baa473b 100644 --- a/apps/sim/tools/github/delete_issue_reaction.ts +++ b/apps/sim/tools/github/delete_issue_reaction.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface DeleteIssueReactionParams { owner: string @@ -63,7 +64,7 @@ export const deleteIssueReactionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}/reactions/${params.reaction_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/reactions/${safeUrlPathSegment(params.reaction_id, 'reaction_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.squirrel-girl-preview+json', diff --git a/apps/sim/tools/github/delete_milestone.ts b/apps/sim/tools/github/delete_milestone.ts index cbe44f634df..21573c6457d 100644 --- a/apps/sim/tools/github/delete_milestone.ts +++ b/apps/sim/tools/github/delete_milestone.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface DeleteMilestoneParams { owner: string @@ -53,7 +54,7 @@ export const deleteMilestoneTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/milestones/${params.milestone_number}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/milestones/${safeUrlPathSegment(params.milestone_number, 'milestone_number')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/delete_release.ts b/apps/sim/tools/github/delete_release.ts index bc7891d02e9..47ae18fa5a8 100644 --- a/apps/sim/tools/github/delete_release.ts +++ b/apps/sim/tools/github/delete_release.ts @@ -1,5 +1,6 @@ import type { DeleteReleaseParams, DeleteReleaseResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const deleteReleaseTool: ToolConfig = { id: 'github_delete_release', @@ -37,7 +38,7 @@ export const deleteReleaseTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/releases/${params.release_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases/${safeUrlPathSegment(params.release_id, 'release_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/fork_gist.ts b/apps/sim/tools/github/fork_gist.ts index 8ecafafb289..6a5cd7472ef 100644 --- a/apps/sim/tools/github/fork_gist.ts +++ b/apps/sim/tools/github/fork_gist.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface ForkGistParams { gist_id: string @@ -44,7 +45,8 @@ export const forkGistTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/gists/${params.gist_id?.trim()}/forks`, + url: (params) => + `https://api.github.com/gists/${safeUrlPathSegment(params.gist_id, 'gist_id')}/forks`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/fork_repo.ts b/apps/sim/tools/github/fork_repo.ts index 1de0ae638d7..7768f06d5e6 100644 --- a/apps/sim/tools/github/fork_repo.ts +++ b/apps/sim/tools/github/fork_repo.ts @@ -5,6 +5,7 @@ import { USER_FULL_OUTPUT_PROPERTIES, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface ForkRepoParams { owner: string @@ -81,7 +82,8 @@ export const forkRepoTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/repos/${params.owner}/${params.repo}/forks`, + url: (params) => + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/forks`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/get_branch.ts b/apps/sim/tools/github/get_branch.ts index 414534802f6..e9086b019d2 100644 --- a/apps/sim/tools/github/get_branch.ts +++ b/apps/sim/tools/github/get_branch.ts @@ -1,6 +1,7 @@ import type { BranchResponse, GetBranchParams } from '@/tools/github/types' import { BRANCH_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const getBranchTool: ToolConfig = { id: 'github_get_branch', @@ -38,7 +39,7 @@ export const getBranchTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/branches/${params.branch}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/branches/${safeUrlPath(params.branch, 'branch')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/get_branch_protection.ts b/apps/sim/tools/github/get_branch_protection.ts index b80e7905866..742e1b3faf2 100644 --- a/apps/sim/tools/github/get_branch_protection.ts +++ b/apps/sim/tools/github/get_branch_protection.ts @@ -1,6 +1,7 @@ import type { BranchProtectionResponse, GetBranchProtectionParams } from '@/tools/github/types' import { BRANCH_PROTECTION_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const getBranchProtectionTool: ToolConfig< GetBranchProtectionParams, @@ -41,7 +42,7 @@ export const getBranchProtectionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/branches/${params.branch}/protection`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/branches/${safeUrlPath(params.branch, 'branch')}/protection`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/get_commit.ts b/apps/sim/tools/github/get_commit.ts index 1f1ad9530e9..44ad43bf3ed 100644 --- a/apps/sim/tools/github/get_commit.ts +++ b/apps/sim/tools/github/get_commit.ts @@ -7,6 +7,7 @@ import { USER_FULL_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' interface GetCommitParams { owner: string @@ -74,7 +75,7 @@ export const getCommitTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/commits/${params.ref}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/commits/${safeUrlPath(params.ref, 'ref')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/get_file_content.ts b/apps/sim/tools/github/get_file_content.ts index 812b888a4d7..dc8630ef11b 100644 --- a/apps/sim/tools/github/get_file_content.ts +++ b/apps/sim/tools/github/get_file_content.ts @@ -1,6 +1,7 @@ import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils' import type { FileContentResponse, GetFileContentParams } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const getFileContentTool: ToolConfig = { id: 'github_get_file_content', @@ -44,8 +45,8 @@ export const getFileContentTool: ToolConfig { - const baseUrl = `https://api.github.com/repos/${params.owner}/${params.repo}/contents/${params.path}` - return params.ref ? `${baseUrl}?ref=${params.ref}` : baseUrl + const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}` + return params.ref ? `${baseUrl}?ref=${encodeURIComponent(params.ref)}` : baseUrl }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/github/get_gist.ts b/apps/sim/tools/github/get_gist.ts index d5f0bc0a246..e56051b8d22 100644 --- a/apps/sim/tools/github/get_gist.ts +++ b/apps/sim/tools/github/get_gist.ts @@ -1,5 +1,6 @@ import { GIST_FILES_OUTPUT, GIST_OUTPUT_PROPERTIES, GIST_OWNER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface GetGistParams { gist_id: string @@ -53,7 +54,8 @@ export const getGistTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/gists/${params.gist_id?.trim()}`, + url: (params) => + `https://api.github.com/gists/${safeUrlPathSegment(params.gist_id, 'gist_id')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/get_issue.ts b/apps/sim/tools/github/get_issue.ts index 16fbba179fc..6b1c43d81de 100644 --- a/apps/sim/tools/github/get_issue.ts +++ b/apps/sim/tools/github/get_issue.ts @@ -6,6 +6,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getIssueTool: ToolConfig = { id: 'github_get_issue', @@ -42,7 +43,7 @@ export const getIssueTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/get_latest_release.ts b/apps/sim/tools/github/get_latest_release.ts index 697c1f83b5f..42552b45a73 100644 --- a/apps/sim/tools/github/get_latest_release.ts +++ b/apps/sim/tools/github/get_latest_release.ts @@ -5,6 +5,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getLatestReleaseTool: ToolConfig = { id: 'github_get_latest_release', @@ -35,7 +36,8 @@ export const getLatestReleaseTool: ToolConfig `https://api.github.com/repos/${params.owner}/${params.repo}/releases/latest`, + url: (params) => + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases/latest`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/get_milestone.ts b/apps/sim/tools/github/get_milestone.ts index 077657339fc..ad30b47ed16 100644 --- a/apps/sim/tools/github/get_milestone.ts +++ b/apps/sim/tools/github/get_milestone.ts @@ -1,5 +1,6 @@ import { MILESTONE_CREATOR_OUTPUT, MILESTONE_V2_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface GetMilestoneParams { owner: string @@ -64,7 +65,7 @@ export const getMilestoneTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/milestones/${params.milestone_number}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/milestones/${safeUrlPathSegment(params.milestone_number, 'milestone_number')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/get_pr_files.ts b/apps/sim/tools/github/get_pr_files.ts index 492f8d4aee1..31e47dcb558 100644 --- a/apps/sim/tools/github/get_pr_files.ts +++ b/apps/sim/tools/github/get_pr_files.ts @@ -1,6 +1,7 @@ import type { GetPRFilesParams, PRFilesListResponse } from '@/tools/github/types' import { PR_FILE_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getPRFilesTool: ToolConfig = { id: 'github_get_pr_files', @@ -52,7 +53,7 @@ export const getPRFilesTool: ToolConfig = request: { url: (params) => { const url = new URL( - `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}/files` + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}/files` ) if (params.per_page) url.searchParams.append('per_page', Number(params.per_page).toString()) if (params.page) url.searchParams.append('page', Number(params.page).toString()) diff --git a/apps/sim/tools/github/get_readme.ts b/apps/sim/tools/github/get_readme.ts index da38feb5601..3d9b028b0d7 100644 --- a/apps/sim/tools/github/get_readme.ts +++ b/apps/sim/tools/github/get_readme.ts @@ -1,5 +1,6 @@ import type { GetReadmeParams, ReadmeResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getReadmeTool: ToolConfig = { id: 'github_get_readme', @@ -38,7 +39,7 @@ export const getReadmeTool: ToolConfig = { request: { url: (params) => { - const baseUrl = `https://api.github.com/repos/${params.owner}/${params.repo}/readme` + const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/readme` return params.ref ? `${baseUrl}?ref=${encodeURIComponent(params.ref)}` : baseUrl }, method: 'GET', diff --git a/apps/sim/tools/github/get_release.ts b/apps/sim/tools/github/get_release.ts index b8dcbd77e0b..1641cf6cbe5 100644 --- a/apps/sim/tools/github/get_release.ts +++ b/apps/sim/tools/github/get_release.ts @@ -5,6 +5,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getReleaseTool: ToolConfig = { id: 'github_get_release', @@ -42,7 +43,7 @@ export const getReleaseTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/releases/${params.release_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases/${safeUrlPathSegment(params.release_id, 'release_id')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/get_tree.ts b/apps/sim/tools/github/get_tree.ts index 72dd3b1b767..cea88253a93 100644 --- a/apps/sim/tools/github/get_tree.ts +++ b/apps/sim/tools/github/get_tree.ts @@ -1,5 +1,6 @@ import type { GetTreeParams, TreeResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const getTreeTool: ToolConfig = { id: 'github_get_tree', @@ -44,9 +45,9 @@ export const getTreeTool: ToolConfig = { request: { url: (params) => { - const path = params.path || '' - const baseUrl = `https://api.github.com/repos/${params.owner}/${params.repo}/contents/${path}` - return params.ref ? `${baseUrl}?ref=${params.ref}` : baseUrl + const path = params.path ? safeUrlPath(params.path, 'path') : '' + const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${path}` + return params.ref ? `${baseUrl}?ref=${encodeURIComponent(params.ref)}` : baseUrl }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/github/get_workflow.ts b/apps/sim/tools/github/get_workflow.ts index d6a9699008d..f5229416bcf 100644 --- a/apps/sim/tools/github/get_workflow.ts +++ b/apps/sim/tools/github/get_workflow.ts @@ -1,6 +1,7 @@ import type { GetWorkflowParams, WorkflowResponse } from '@/tools/github/types' import { WORKFLOW_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getWorkflowTool: ToolConfig = { id: 'github_get_workflow', @@ -38,7 +39,7 @@ export const getWorkflowTool: ToolConfig = request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/actions/workflows/${params.workflow_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/workflows/${safeUrlPathSegment(params.workflow_id, 'workflow_id')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/get_workflow_run.ts b/apps/sim/tools/github/get_workflow_run.ts index 532505b5aa2..98b8a90fd80 100644 --- a/apps/sim/tools/github/get_workflow_run.ts +++ b/apps/sim/tools/github/get_workflow_run.ts @@ -7,6 +7,7 @@ import { WORKFLOW_RUN_OUTPUT_PROPERTIES, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getWorkflowRunTool: ToolConfig = { id: 'github_get_workflow_run', @@ -44,7 +45,7 @@ export const getWorkflowRunTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/actions/runs/${params.run_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/runs/${safeUrlPathSegment(params.run_id, 'run_id')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/issue_comment.ts b/apps/sim/tools/github/issue_comment.ts index a32fe31c665..d235b46b94d 100644 --- a/apps/sim/tools/github/issue_comment.ts +++ b/apps/sim/tools/github/issue_comment.ts @@ -2,6 +2,7 @@ import { truncate } from '@sim/utils/string' import type { CreateIssueCommentParams, IssueCommentResponse } from '@/tools/github/types' import { COMMENT_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const issueCommentTool: ToolConfig = { id: 'github_issue_comment', @@ -44,7 +45,7 @@ export const issueCommentTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}/comments`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/comments`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/job_logs.test.ts b/apps/sim/tools/github/job_logs.test.ts index cd3d9efd2d1..8f9b7815521 100644 --- a/apps/sim/tools/github/job_logs.test.ts +++ b/apps/sim/tools/github/job_logs.test.ts @@ -34,16 +34,22 @@ describe('github_job_logs', () => { expect(url).toBe('https://api.github.com/repos/octo/demo/actions/jobs/42/logs') }) - it('escapes coordinates so they cannot redirect the authenticated request', () => { + it('rejects coordinates that would redirect the authenticated request', () => { + const url = jobLogsTool.request.url as (params: JobLogsParams) => string + + expect(() => url({ ...BASE_PARAMS, owner: '../../orgs/secret' })).toThrow( + /owner cannot contain a path separator/ + ) + expect(() => url({ ...BASE_PARAMS, owner: '..' })).toThrow(/path traversal is not allowed/) + }) + + it('escapes a coordinate that cannot redirect but carries URL syntax', () => { const url = (jobLogsTool.request.url as (params: JobLogsParams) => string)({ ...BASE_PARAMS, - owner: '../../orgs/secret', repo: 'demo?ref=x', }) - expect(url).toBe( - 'https://api.github.com/repos/..%2F..%2Forgs%2Fsecret/demo%3Fref%3Dx/actions/jobs/42/logs' - ) + expect(url).toBe('https://api.github.com/repos/octo/demo%3Fref%3Dx/actions/jobs/42/logs') }) it('rejects a job id that is not a positive integer', () => { diff --git a/apps/sim/tools/github/job_logs.ts b/apps/sim/tools/github/job_logs.ts index be1ccf46098..bf79eb93db2 100644 --- a/apps/sim/tools/github/job_logs.ts +++ b/apps/sim/tools/github/job_logs.ts @@ -1,5 +1,6 @@ import type { JobLogsParams, JobLogsResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' const DEFAULT_MAX_CHARACTERS = 20_000 const MAX_CHARACTERS_LIMIT = 200_000 @@ -25,7 +26,7 @@ function jobLogsPath(owner: string, repo: string, jobId: number): string { if (!Number.isSafeInteger(jobId) || jobId < 1) { throw new Error('job_id must be a positive integer') } - return `${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/actions/jobs/${jobId}/logs` + return `${safeUrlPathSegment(owner, 'owner')}/${safeUrlPathSegment(repo, 'repo')}/actions/jobs/${jobId}/logs` } /** Byte offsets from a `Content-Range: bytes -/` header. */ diff --git a/apps/sim/tools/github/list_branches.ts b/apps/sim/tools/github/list_branches.ts index 14db5da268d..a6dd76a1464 100644 --- a/apps/sim/tools/github/list_branches.ts +++ b/apps/sim/tools/github/list_branches.ts @@ -1,6 +1,7 @@ import type { BranchListResponse, ListBranchesParams } from '@/tools/github/types' import { BRANCH_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listBranchesTool: ToolConfig = { id: 'github_list_branches', @@ -50,7 +51,7 @@ export const listBranchesTool: ToolConfig { - const baseUrl = `https://api.github.com/repos/${params.owner}/${params.repo}/branches` + const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/branches` const queryParams = new URLSearchParams() if (params.protected !== undefined) { diff --git a/apps/sim/tools/github/list_commits.ts b/apps/sim/tools/github/list_commits.ts index 5e979952c4d..7db81bf5847 100644 --- a/apps/sim/tools/github/list_commits.ts +++ b/apps/sim/tools/github/list_commits.ts @@ -5,6 +5,7 @@ import { USER_FULL_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface ListCommitsParams { owner: string @@ -117,7 +118,9 @@ export const listCommitsTool: ToolConfig request: { url: (params) => { - const url = new URL(`https://api.github.com/repos/${params.owner}/${params.repo}/commits`) + const url = new URL( + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/commits` + ) if (params.sha) url.searchParams.append('sha', params.sha) if (params.path) url.searchParams.append('path', params.path) if (params.author) url.searchParams.append('author', params.author) diff --git a/apps/sim/tools/github/list_forks.ts b/apps/sim/tools/github/list_forks.ts index fad5cb9776e..2da2f52eb12 100644 --- a/apps/sim/tools/github/list_forks.ts +++ b/apps/sim/tools/github/list_forks.ts @@ -1,5 +1,6 @@ import { REPO_FULL_OUTPUT_PROPERTIES, USER_FULL_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface ListForksParams { owner: string @@ -81,7 +82,9 @@ export const listForksTool: ToolConfig = { request: { url: (params) => { - const url = new URL(`https://api.github.com/repos/${params.owner}/${params.repo}/forks`) + const url = new URL( + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/forks` + ) if (params.sort) url.searchParams.append('sort', params.sort) if (params.per_page) url.searchParams.append('per_page', String(params.per_page)) if (params.page) url.searchParams.append('page', String(params.page)) diff --git a/apps/sim/tools/github/list_gists.ts b/apps/sim/tools/github/list_gists.ts index 9f7cee60d35..faf802da11f 100644 --- a/apps/sim/tools/github/list_gists.ts +++ b/apps/sim/tools/github/list_gists.ts @@ -1,5 +1,6 @@ import { GIST_FILES_OUTPUT, GIST_OUTPUT_PROPERTIES, GIST_OWNER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface ListGistsParams { username?: string @@ -74,7 +75,7 @@ export const listGistsTool: ToolConfig = { request: { url: (params) => { const baseUrl = params.username - ? `https://api.github.com/users/${params.username}/gists` + ? `https://api.github.com/users/${safeUrlPathSegment(params.username, 'username')}/gists` : 'https://api.github.com/gists' const url = new URL(baseUrl) if (params.since) url.searchParams.append('since', params.since) diff --git a/apps/sim/tools/github/list_issue_comments.ts b/apps/sim/tools/github/list_issue_comments.ts index c113b107fb0..afdd822270d 100644 --- a/apps/sim/tools/github/list_issue_comments.ts +++ b/apps/sim/tools/github/list_issue_comments.ts @@ -2,6 +2,7 @@ import { truncate } from '@sim/utils/string' import type { CommentsListResponse, ListIssueCommentsParams } from '@/tools/github/types' import { COMMENT_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listIssueCommentsTool: ToolConfig = { id: 'github_list_issue_comments', @@ -58,7 +59,7 @@ export const listIssueCommentsTool: ToolConfig { - const baseUrl = `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}/comments` + const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/comments` const queryParams = new URLSearchParams() if (params.since) queryParams.append('since', params.since) diff --git a/apps/sim/tools/github/list_issues.ts b/apps/sim/tools/github/list_issues.ts index ddd6e0dc08a..2538f0c292e 100644 --- a/apps/sim/tools/github/list_issues.ts +++ b/apps/sim/tools/github/list_issues.ts @@ -6,6 +6,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listIssuesTool: ToolConfig = { id: 'github_list_issues', @@ -90,7 +91,9 @@ export const listIssuesTool: ToolConfig = request: { url: (params) => { - const url = new URL(`https://api.github.com/repos/${params.owner}/${params.repo}/issues`) + const url = new URL( + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues` + ) if (params.state) url.searchParams.append('state', params.state) if (params.assignee) url.searchParams.append('assignee', params.assignee) if (params.creator) url.searchParams.append('creator', params.creator) diff --git a/apps/sim/tools/github/list_milestones.ts b/apps/sim/tools/github/list_milestones.ts index 7ee054dca03..77d43b9631d 100644 --- a/apps/sim/tools/github/list_milestones.ts +++ b/apps/sim/tools/github/list_milestones.ts @@ -1,5 +1,6 @@ import { MILESTONE_CREATOR_OUTPUT, MILESTONE_V2_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface ListMilestonesParams { owner: string @@ -96,7 +97,9 @@ export const listMilestonesTool: ToolConfig { - const url = new URL(`https://api.github.com/repos/${params.owner}/${params.repo}/milestones`) + const url = new URL( + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/milestones` + ) if (params.state) url.searchParams.append('state', params.state) if (params.sort) url.searchParams.append('sort', params.sort) if (params.direction) url.searchParams.append('direction', params.direction) diff --git a/apps/sim/tools/github/list_pr_comments.ts b/apps/sim/tools/github/list_pr_comments.ts index f0b4ba23765..704d798031a 100644 --- a/apps/sim/tools/github/list_pr_comments.ts +++ b/apps/sim/tools/github/list_pr_comments.ts @@ -2,6 +2,7 @@ import { truncate } from '@sim/utils/string' import type { CommentsListResponse, ListPRCommentsParams } from '@/tools/github/types' import { PR_COMMENT_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listPRCommentsTool: ToolConfig = { id: 'github_list_pr_comments', @@ -72,7 +73,7 @@ export const listPRCommentsTool: ToolConfig { - const baseUrl = `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}/comments` + const baseUrl = `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}/comments` const queryParams = new URLSearchParams() if (params.sort) queryParams.append('sort', params.sort) diff --git a/apps/sim/tools/github/list_prs.ts b/apps/sim/tools/github/list_prs.ts index 47e7f72291a..1685d6bf2be 100644 --- a/apps/sim/tools/github/list_prs.ts +++ b/apps/sim/tools/github/list_prs.ts @@ -1,6 +1,7 @@ import type { ListPRsParams, PRListResponse } from '@/tools/github/types' import { BRANCH_REF_OUTPUT, PR_SUMMARY_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listPRsTool: ToolConfig = { id: 'github_list_prs', @@ -79,7 +80,9 @@ export const listPRsTool: ToolConfig = { request: { url: (params) => { - const url = new URL(`https://api.github.com/repos/${params.owner}/${params.repo}/pulls`) + const url = new URL( + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls` + ) if (params.state) url.searchParams.append('state', params.state) if (params.head) url.searchParams.append('head', params.head) if (params.base) url.searchParams.append('base', params.base) diff --git a/apps/sim/tools/github/list_releases.ts b/apps/sim/tools/github/list_releases.ts index 20870fad320..625f73b4566 100644 --- a/apps/sim/tools/github/list_releases.ts +++ b/apps/sim/tools/github/list_releases.ts @@ -5,6 +5,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listReleasesTool: ToolConfig = { id: 'github_list_releases', @@ -50,7 +51,9 @@ export const listReleasesTool: ToolConfig { - const url = new URL(`https://api.github.com/repos/${params.owner}/${params.repo}/releases`) + const url = new URL( + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases` + ) if (params.per_page) { url.searchParams.append('per_page', Number(params.per_page).toString()) } diff --git a/apps/sim/tools/github/list_stargazers.ts b/apps/sim/tools/github/list_stargazers.ts index b562db06903..f05b9a60a49 100644 --- a/apps/sim/tools/github/list_stargazers.ts +++ b/apps/sim/tools/github/list_stargazers.ts @@ -1,5 +1,6 @@ import { USER_FULL_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface ListStargazersParams { owner: string @@ -69,7 +70,9 @@ export const listStargazersTool: ToolConfig { - const url = new URL(`https://api.github.com/repos/${params.owner}/${params.repo}/stargazers`) + const url = new URL( + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/stargazers` + ) if (params.per_page) url.searchParams.append('per_page', String(params.per_page)) if (params.page) url.searchParams.append('page', String(params.page)) return url.toString() diff --git a/apps/sim/tools/github/list_tags.ts b/apps/sim/tools/github/list_tags.ts index bf5f73e173a..44b2d3a8997 100644 --- a/apps/sim/tools/github/list_tags.ts +++ b/apps/sim/tools/github/list_tags.ts @@ -1,5 +1,6 @@ import type { ListTagsParams, TagsListResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listTagsTool: ToolConfig = { id: 'github_list_tags', @@ -45,7 +46,9 @@ export const listTagsTool: ToolConfig = { request: { url: (params) => { - const url = new URL(`https://api.github.com/repos/${params.owner}/${params.repo}/tags`) + const url = new URL( + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/tags` + ) if (params.per_page) { url.searchParams.append('per_page', Number(params.per_page).toString()) } diff --git a/apps/sim/tools/github/list_workflow_runs.ts b/apps/sim/tools/github/list_workflow_runs.ts index c670a71f1e1..5dedd55c804 100644 --- a/apps/sim/tools/github/list_workflow_runs.ts +++ b/apps/sim/tools/github/list_workflow_runs.ts @@ -7,6 +7,7 @@ import { WORKFLOW_RUN_OUTPUT_PROPERTIES, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listWorkflowRunsTool: ToolConfig = { id: 'github_list_workflow_runs', @@ -77,7 +78,7 @@ export const listWorkflowRunsTool: ToolConfig { const url = new URL( - `https://api.github.com/repos/${params.owner}/${params.repo}/actions/runs` + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/runs` ) if (params.actor) { url.searchParams.append('actor', params.actor) diff --git a/apps/sim/tools/github/list_workflows.ts b/apps/sim/tools/github/list_workflows.ts index b281a8cdaf8..553f4214d34 100644 --- a/apps/sim/tools/github/list_workflows.ts +++ b/apps/sim/tools/github/list_workflows.ts @@ -1,6 +1,7 @@ import type { ListWorkflowsParams, ListWorkflowsResponse } from '@/tools/github/types' import { WORKFLOW_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listWorkflowsTool: ToolConfig = { id: 'github_list_workflows', @@ -47,7 +48,7 @@ export const listWorkflowsTool: ToolConfig { const url = new URL( - `https://api.github.com/repos/${params.owner}/${params.repo}/actions/workflows` + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/workflows` ) if (params.per_page) { url.searchParams.append('per_page', Number(params.per_page).toString()) diff --git a/apps/sim/tools/github/merge_pr.ts b/apps/sim/tools/github/merge_pr.ts index 521ec846f36..d94858cb38c 100644 --- a/apps/sim/tools/github/merge_pr.ts +++ b/apps/sim/tools/github/merge_pr.ts @@ -1,5 +1,6 @@ import type { MergePRParams, MergeResultResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const mergePRTool: ToolConfig = { id: 'github_merge_pr', @@ -55,7 +56,7 @@ export const mergePRTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}/merge`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}/merge`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/path_safety.test.ts b/apps/sim/tools/github/path_safety.test.ts new file mode 100644 index 00000000000..281357228e0 --- /dev/null +++ b/apps/sim/tools/github/path_safety.test.ts @@ -0,0 +1,240 @@ +/** + * @vitest-environment node + * + * Guards every GitHub tool against path traversal through an LLM-writable value + * that gets interpolated into the request path. + * + * `owner`, `repo`, `issueNumber`, `pullNumber`, `sha`, `path`, `branch`, `ref` + * and their siblings are `visibility: 'user-or-llm'`, so prompt injection + * controls them. Interpolating one raw let a value like `../../repos/victim/private` + * escape its `/repos/{owner}/{repo}` prefix once `fetch` normalized the URL, + * re-aiming the request — and the workspace's GitHub token — at an arbitrary + * repository, including on DELETE routes such as `delete_file` and + * `delete_release`. `assertRequestUrlMatchesTrust` in `tools/request-transport.ts` + * only canonicalizes internal `/api/` routes, so nothing downstream catches it. + * + * Wrapping the value in `encodeURIComponent` is NOT enough, which is why the + * vector list below keeps the bare `.` and `..` segments: both are made of + * unreserved characters, so they survive encoding untouched, and the URL parser + * then removes them as dot segments — popping a segment off a fixed host. It + * removes the percent-encoded spellings too, so double-encoding is no fix + * either. Only rejecting the value works. + * + * Every assertion resolves the built URL with `new URL(...)` — the same + * normalization `fetch` performs — rather than string-matching the template + * output, because string matching is exactly what let this through. + * + * Tools are enumerated from the barrel rather than listed, so a newly added + * GitHub tool that interpolates an unguarded parameter fails this suite. + */ +import { describe, expect, it } from 'vitest' +import * as githubTools from '@/tools/github/index' +import type { ToolConfig } from '@/tools/types' + +/** + * The bare `.` and `..` entries are the whole point: their omission is why an + * `encodeURIComponent`-only fix looks correct while the hole stays live. + */ +const TRAVERSAL_VALUES = [ + '..', + '.', + ' .. ', + '../../repos/victim/private', + '..%2f..%2frepos/victim/private', + 'octocat/../../../repos/victim/private', + 'octocat?access_token=attacker', + 'octocat#fragment', + 'sim/contents/../../../repos/victim/private', + '\\..\\..', + '../', + './.', +] as const + +/** + * Values a real user legitimately supplies for a single-segment parameter. + * None may be rejected or altered by the guards. + */ +const LEGITIMATE_IDS = [ + 'octocat', + 'my-repo', + 'sim', + 'simstudioai', + '1234', + 'README.md', + 'ci.yml', + 'v1.2.3', + '9d1e0e1a3b8a4c2f6d7e8f9a0b1c2d3e4f5a6b7c', + '..foo', + 'foo..', + 'release-2.0', +] as const + +/** + * Values a real user legitimately supplies for a parameter that addresses a + * location inside a repository. These carry `/`, so a single-segment guard + * would reject every one of them — which is why those parameters use + * `safeUrlPath` instead. + */ +const LEGITIMATE_PATHS = [ + 'feature/my-branch', + 'docs/README.md', + 'apps/sim/tools/github/index.ts', + 'heads/release/2.0', + 'octocat:feature/my-branch', +] as const + +/** + * Parameters GitHub documents as slash-delimited. Every other path parameter + * addresses a single resource and must reject a separator outright. + */ +const MULTI_SEGMENT_PARAMS = new Set(['path', 'branch', 'ref', 'base', 'head']) + +/** + * Parameters the provider reads as one path parameter that may itself contain + * `/` — a namespaced GitHub label such as `area/api`. The separator must + * survive as `%2F`, so these neither reject it nor promote it to a boundary. + */ +const ENCODED_SEGMENT_PARAMS = new Set(['name']) + +const PROBE = 'PROBEVALUE' +const FILLER = 'SAFEID' + +type AnyTool = ToolConfig + +function isGitHubTool(value: unknown): value is AnyTool { + return ( + typeof value === 'object' && + value !== null && + typeof (value as AnyTool).id === 'string' && + (value as AnyTool).id.startsWith('github') + ) +} + +/** + * Builds a param object for a tool with one parameter set to `value` and every + * other string-ish parameter set to a constant, so the assertion isolates the + * parameter under test. + * + * Number-typed parameters are filled with the probe string too. Their declared + * type is not enforced anywhere between the LLM tool call and the URL builder, + * so an `issue_number` of `'..'` reaches the path exactly like a string one. + */ +function buildParams(tool: AnyTool, target: string, value: string): Record { + const params: Record = { apiKey: 'token' } + for (const [name, def] of Object.entries(tool.params ?? {})) { + if (name === 'apiKey') continue + const type = (def as { type?: string }).type + if (type === 'json' || type === 'array') { + params[name] = [] + } else if (type === 'boolean') { + params[name] = false + } else { + params[name] = name === target ? value : FILLER + } + } + return params +} + +function buildUrl(tool: AnyTool, target: string, value: string): URL { + const url = tool.request?.url + if (typeof url !== 'function') { + throw new Error(`${tool.id} does not build its URL from params`) + } + return new URL(url(buildParams(tool, target, value) as any)) +} + +function buildPath(tool: AnyTool, target: string, value: string): string { + return buildUrl(tool, target, value).pathname +} + +interface PathParamCase { + name: string + tool: AnyTool + param: string + baseline: string +} + +/** + * Every (tool, parameter) pair whose value actually reaches the URL path, + * discovered by probing rather than declared, so a new tool is covered the + * moment it lands in the barrel. + */ +const PATH_PARAM_CASES: PathParamCase[] = [] + +for (const tool of Object.values(githubTools).filter(isGitHubTool)) { + if (typeof tool.request?.url !== 'function') continue + for (const param of Object.keys(tool.params ?? {})) { + if (param === 'apiKey') continue + let baseline: string + try { + baseline = buildPath(tool, param, PROBE) + } catch { + continue + } + if (!baseline.includes(PROBE)) continue + PATH_PARAM_CASES.push({ name: `${tool.id} / ${param}`, tool, param, baseline }) + } +} + +describe('github path traversal safety', () => { + it('covers every GitHub tool parameter that reaches the request path', () => { + expect(PATH_PARAM_CASES.length).toBeGreaterThanOrEqual(60) + }) + + it('covers the multi-segment parameters', () => { + const covered = new Set(PATH_PARAM_CASES.map((entry) => entry.param)) + for (const param of MULTI_SEGMENT_PARAMS) { + expect(covered.has(param)).toBe(true) + } + }) + + describe.each(PATH_PARAM_CASES)('$name', ({ tool, param, baseline }) => { + const prefix = baseline.slice(0, baseline.indexOf(PROBE)) + + it.each(TRAVERSAL_VALUES)('cannot escape its path prefix with %j', (value) => { + let url: URL + try { + url = buildUrl(tool, param, value) + } catch { + return + } + + expect(url.origin).toBe('https://api.github.com') + expect(url.pathname.startsWith(prefix)).toBe(true) + expect(url.pathname.split('/')).not.toContain('..') + expect(url.pathname.split('/')).not.toContain('.') + if (!MULTI_SEGMENT_PARAMS.has(param)) { + expect(url.pathname).not.toContain('/victim/') + } + expect(url.searchParams.get('access_token')).toBeNull() + }) + + it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { + expect(buildPath(tool, param, value)).toBe(baseline.replaceAll(PROBE, value)) + }) + + it('rejects a bare dot-dot instead of silently popping its prefix', () => { + expect(() => buildPath(tool, param, '..')).toThrow(new RegExp(param)) + }) + + it('rejects a bare dot', () => { + expect(() => buildPath(tool, param, '.')).toThrow(new RegExp(param)) + }) + + if (MULTI_SEGMENT_PARAMS.has(param)) { + it.each(LEGITIMATE_PATHS)('passes multi-segment %j through unchanged', (value) => { + expect(buildPath(tool, param, value)).toBe(baseline.replaceAll(PROBE, value)) + }) + } else if (ENCODED_SEGMENT_PARAMS.has(param)) { + it.each(LEGITIMATE_PATHS)('keeps multi-segment %j inside one segment', (value) => { + expect(buildPath(tool, param, value)).toBe( + baseline.replaceAll(PROBE, encodeURIComponent(value)) + ) + }) + } else { + it.each(LEGITIMATE_PATHS)('rejects multi-segment %j', (value) => { + expect(() => buildPath(tool, param, value)).toThrow(new RegExp(param)) + }) + } + }) +}) diff --git a/apps/sim/tools/github/pr.ts b/apps/sim/tools/github/pr.ts index 6460911ea2a..f4ac354f717 100644 --- a/apps/sim/tools/github/pr.ts +++ b/apps/sim/tools/github/pr.ts @@ -20,6 +20,7 @@ import type { } from '@/tools/github/types' import { PR_BRANCH_REF_OUTPUT, PR_FILE_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' type GitHubPullRequest = Omit @@ -150,7 +151,7 @@ async function fetchPullRequestFiles( for (let page = 1; page <= maxPages; page += 1) { const response = await fetch( - `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${pullNumber}/files?per_page=${PULL_REQUEST_FILES_PER_PAGE}&page=${page}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${pullNumber}/files?per_page=${PULL_REQUEST_FILES_PER_PAGE}&page=${page}`, { headers: { Accept: 'application/vnd.github+json', @@ -225,7 +226,7 @@ export const prTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/remove_label.ts b/apps/sim/tools/github/remove_label.ts index b31b10602f9..630e0f3b181 100644 --- a/apps/sim/tools/github/remove_label.ts +++ b/apps/sim/tools/github/remove_label.ts @@ -1,5 +1,6 @@ import type { LabelsResponse, RemoveLabelParams } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeEncodedUrlPathSegment, safeUrlPathSegment } from '@/tools/url-path' export const removeLabelTool: ToolConfig = { id: 'github_remove_label', @@ -42,7 +43,7 @@ export const removeLabelTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}/labels/${encodeURIComponent(params.name)}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/labels/${safeEncodedUrlPathSegment(params.name, 'name')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/repo_info.ts b/apps/sim/tools/github/repo_info.ts index 01faee0f6c0..591e1f5ed64 100644 --- a/apps/sim/tools/github/repo_info.ts +++ b/apps/sim/tools/github/repo_info.ts @@ -6,6 +6,7 @@ import { USER_FULL_OUTPUT_PROPERTIES, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const repoInfoTool: ToolConfig = { id: 'github_repo_info', @@ -36,7 +37,8 @@ export const repoInfoTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/repos/${params.owner}/${params.repo}`, + url: (params) => + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}`, method: 'GET', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/request_reviewers.ts b/apps/sim/tools/github/request_reviewers.ts index 6dcd2852d9e..cb8e2123013 100644 --- a/apps/sim/tools/github/request_reviewers.ts +++ b/apps/sim/tools/github/request_reviewers.ts @@ -1,5 +1,6 @@ import type { RequestReviewersParams, ReviewersResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const requestReviewersTool: ToolConfig = { id: 'github_request_reviewers', @@ -49,7 +50,7 @@ export const requestReviewersTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}/requested_reviewers`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}/requested_reviewers`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/rerun_workflow.ts b/apps/sim/tools/github/rerun_workflow.ts index 379d96cb4f0..42a0a6f8904 100644 --- a/apps/sim/tools/github/rerun_workflow.ts +++ b/apps/sim/tools/github/rerun_workflow.ts @@ -1,5 +1,6 @@ import type { RerunWorkflowParams, RerunWorkflowResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const rerunWorkflowTool: ToolConfig = { id: 'github_rerun_workflow', @@ -44,7 +45,7 @@ export const rerunWorkflowTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/actions/runs/${params.run_id}/rerun`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/runs/${safeUrlPathSegment(params.run_id, 'run_id')}/rerun`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/star_gist.ts b/apps/sim/tools/github/star_gist.ts index 0d654cd0efa..6ac4cf0cbfa 100644 --- a/apps/sim/tools/github/star_gist.ts +++ b/apps/sim/tools/github/star_gist.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface StarGistParams { gist_id: string @@ -38,7 +39,8 @@ export const starGistTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/gists/${params.gist_id?.trim()}/star`, + url: (params) => + `https://api.github.com/gists/${safeUrlPathSegment(params.gist_id, 'gist_id')}/star`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/star_repo.ts b/apps/sim/tools/github/star_repo.ts index 1bd65ac1037..d3a3f2e8afe 100644 --- a/apps/sim/tools/github/star_repo.ts +++ b/apps/sim/tools/github/star_repo.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface StarRepoParams { owner: string @@ -46,7 +47,8 @@ export const starRepoTool: ToolConfig = { }, request: { - url: (params) => `https://api.github.com/user/starred/${params.owner}/${params.repo}`, + url: (params) => + `https://api.github.com/user/starred/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/trigger_workflow.ts b/apps/sim/tools/github/trigger_workflow.ts index c382ebf0426..dfc55802594 100644 --- a/apps/sim/tools/github/trigger_workflow.ts +++ b/apps/sim/tools/github/trigger_workflow.ts @@ -1,5 +1,6 @@ import type { TriggerWorkflowParams, TriggerWorkflowResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const triggerWorkflowTool: ToolConfig = { id: 'github_trigger_workflow', @@ -49,7 +50,7 @@ export const triggerWorkflowTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/actions/workflows/${params.workflow_id}/dispatches`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/workflows/${safeUrlPathSegment(params.workflow_id, 'workflow_id')}/dispatches`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/unstar_gist.ts b/apps/sim/tools/github/unstar_gist.ts index b57fa4e9688..317457667f2 100644 --- a/apps/sim/tools/github/unstar_gist.ts +++ b/apps/sim/tools/github/unstar_gist.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface UnstarGistParams { gist_id: string @@ -38,7 +39,8 @@ export const unstarGistTool: ToolConfig = }, request: { - url: (params) => `https://api.github.com/gists/${params.gist_id?.trim()}/star`, + url: (params) => + `https://api.github.com/gists/${safeUrlPathSegment(params.gist_id, 'gist_id')}/star`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/unstar_repo.ts b/apps/sim/tools/github/unstar_repo.ts index 5ae47d7b60c..1edcb1c2601 100644 --- a/apps/sim/tools/github/unstar_repo.ts +++ b/apps/sim/tools/github/unstar_repo.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface UnstarRepoParams { owner: string @@ -46,7 +47,8 @@ export const unstarRepoTool: ToolConfig = }, request: { - url: (params) => `https://api.github.com/user/starred/${params.owner}/${params.repo}`, + url: (params) => + `https://api.github.com/user/starred/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_branch_protection.ts b/apps/sim/tools/github/update_branch_protection.ts index 1bff2c6e8a9..2e8c39017bf 100644 --- a/apps/sim/tools/github/update_branch_protection.ts +++ b/apps/sim/tools/github/update_branch_protection.ts @@ -1,6 +1,7 @@ import type { BranchProtectionResponse, UpdateBranchProtectionParams } from '@/tools/github/types' import { BRANCH_PROTECTION_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const updateBranchProtectionTool: ToolConfig< UpdateBranchProtectionParams, @@ -68,7 +69,7 @@ export const updateBranchProtectionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/branches/${params.branch}/protection`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/branches/${safeUrlPath(params.branch, 'branch')}/protection`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/update_comment.ts b/apps/sim/tools/github/update_comment.ts index ded246aee9d..cd1e5c364fb 100644 --- a/apps/sim/tools/github/update_comment.ts +++ b/apps/sim/tools/github/update_comment.ts @@ -2,6 +2,7 @@ import { truncate } from '@sim/utils/string' import type { IssueCommentResponse, UpdateCommentParams } from '@/tools/github/types' import { COMMENT_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateCommentTool: ToolConfig = { id: 'github_update_comment', @@ -44,7 +45,7 @@ export const updateCommentTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/comments/${params.comment_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/update_file.ts b/apps/sim/tools/github/update_file.ts index 470e1f571b6..e2c4f48ba59 100644 --- a/apps/sim/tools/github/update_file.ts +++ b/apps/sim/tools/github/update_file.ts @@ -1,5 +1,6 @@ import type { FileOperationResponse, UpdateFileParams } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' export const updateFileTool: ToolConfig = { id: 'github_update_file', @@ -61,7 +62,7 @@ export const updateFileTool: ToolConfig request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/contents/${params.path}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/update_gist.ts b/apps/sim/tools/github/update_gist.ts index 0bc4f82dfd0..aac592ef56f 100644 --- a/apps/sim/tools/github/update_gist.ts +++ b/apps/sim/tools/github/update_gist.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface UpdateGistParams { gist_id: string @@ -61,7 +62,8 @@ export const updateGistTool: ToolConfig = }, request: { - url: (params) => `https://api.github.com/gists/${params.gist_id?.trim()}`, + url: (params) => + `https://api.github.com/gists/${safeUrlPathSegment(params.gist_id, 'gist_id')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_issue.ts b/apps/sim/tools/github/update_issue.ts index c5cab74a2f2..96e764ac130 100644 --- a/apps/sim/tools/github/update_issue.ts +++ b/apps/sim/tools/github/update_issue.ts @@ -6,6 +6,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateIssueTool: ToolConfig = { id: 'github_update_issue', @@ -72,7 +73,7 @@ export const updateIssueTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/issues/${params.issue_number}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_milestone.ts b/apps/sim/tools/github/update_milestone.ts index 8b45a309056..5a801f10014 100644 --- a/apps/sim/tools/github/update_milestone.ts +++ b/apps/sim/tools/github/update_milestone.ts @@ -1,4 +1,5 @@ import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' interface UpdateMilestoneParams { owner: string @@ -88,7 +89,7 @@ export const updateMilestoneTool: ToolConfig - `https://api.github.com/repos/${params.owner}/${params.repo}/milestones/${params.milestone_number}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/milestones/${safeUrlPathSegment(params.milestone_number, 'milestone_number')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_pr.ts b/apps/sim/tools/github/update_pr.ts index 1d42062a80a..fdb0dfe7bc7 100644 --- a/apps/sim/tools/github/update_pr.ts +++ b/apps/sim/tools/github/update_pr.ts @@ -1,5 +1,6 @@ import type { PRResponse, UpdatePRParams } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updatePRTool: ToolConfig = { id: 'github_update_pr', @@ -60,7 +61,7 @@ export const updatePRTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/pulls/${params.pullNumber}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_release.ts b/apps/sim/tools/github/update_release.ts index 25bef88d2f1..06eaab91b7c 100644 --- a/apps/sim/tools/github/update_release.ts +++ b/apps/sim/tools/github/update_release.ts @@ -5,6 +5,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateReleaseTool: ToolConfig = { id: 'github_update_release', @@ -78,7 +79,7 @@ export const updateReleaseTool: ToolConfig request: { url: (params) => - `https://api.github.com/repos/${params.owner}/${params.repo}/releases/${params.release_id}`, + `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases/${safeUrlPathSegment(params.release_id, 'release_id')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/url-path.ts b/apps/sim/tools/url-path.ts index 83ef707172f..d42d95204f7 100644 --- a/apps/sim/tools/url-path.ts +++ b/apps/sim/tools/url-path.ts @@ -186,3 +186,126 @@ export function safeUrlPathSegment(value: string | number | bigint, paramName: s return encodeSegment(trimmed, paramName) } + +/** + * Builds a traversal-safe **multi-segment** URL path from a parameter whose + * value legitimately contains `/`. + * + * A few provider parameters address a location *inside* a repository rather + * than a single resource: GitHub's `path` (`docs/README.md`), `branch` + * (`feature/my-branch`), and `ref` (`heads/release/2.0`). Passing these through + * {@link safeUrlPathSegment} would reject every real value, because that guard + * treats a separator as proof the caller supplied the wrong kind of thing. The + * split is therefore deliberate and narrow: use `safeUrlPathSegment` unless the + * provider documents the parameter as a slash-delimited path, and never widen a + * single-segment id to this helper merely to make a separator stop erroring. + * + * Permitting `/` does not weaken the traversal rule, which is enforced per + * segment: the value is split on `/`, and any segment that is `.` or `..` after + * trimming is rejected outright for exactly the reason the module note gives — + * the URL parser removes a dot segment after decoding, so encoding it cannot + * neutralize it. Each surviving segment is percent-encoded individually, which + * is what keeps a `?`, `#`, or `%` inside a filename from re-aiming the request + * or opening a query, while leaving the `/` separators intact. + * + * Empty segments are rejected rather than dropped. A `//` or a leading `/` + * changes what the joined path addresses (a leading `/` in + * `` `${base}/${value}` `` produces a `//` that the parser keeps), and silently + * collapsing it would rewrite the caller's value into a different resource. + * A trailing `/` is rejected on the same ground. + * + * A `:` is restored after encoding. It is a legal `pchar` with no delimiter or + * traversal meaning inside a path segment, and providers use it structurally: + * GitHub's compare endpoint addresses a cross-fork ref as `owner:branch`, which + * `encodeURIComponent` would rewrite to `owner%3Abranch`. Nothing else escaped + * by `encodeURIComponent` is restored. + * + * Backslashes are rejected everywhere in the value. They are not path + * separators for the URL parser, but a value carrying one is a Windows-shaped + * path that the caller did not mean to address literally, and accepting it + * would encode `\..\..` into a segment that reads as traversal to any consumer + * downstream that normalizes it. + * + * @param value - The raw path, typically LLM- or user-supplied. + * @param paramName - The parameter name, used to name the offender in errors. + * @returns The trimmed path with every segment percent-encoded and the `/` + * separators preserved, safe to interpolate. + * @throws If the value is not a string or a usable number, is empty, contains + * an empty or dot segment, contains a backslash, or cannot be encoded. + */ +export function safeUrlPath(value: string | number | bigint, paramName: string): string { + const trimmed = toGuardedString(value, paramName).trim() + + if (!trimmed) { + throw new Error(`${paramName} is required`) + } + + if (trimmed.includes('\\')) { + throw new Error(`${paramName} cannot contain a backslash`) + } + + return trimmed + .split('/') + .map((segment) => { + const trimmedSegment = segment.trim() + + if (!trimmedSegment) { + throw new Error(`${paramName} cannot contain an empty path segment`) + } + + if (trimmedSegment === '.' || trimmedSegment === '..') { + throw new Error( + `${paramName} cannot contain a "${trimmedSegment}" segment (path traversal is not allowed)` + ) + } + + return encodeSegment(trimmedSegment, paramName).replaceAll('%3A', ':') + }) + .join('/') +} + +/** + * Builds a traversal-safe URL path segment from a parameter whose value may + * legitimately contain `/` but which the provider still reads as **one** path + * parameter. + * + * This is the third shape, and the narrowest. {@link safeUrlPathSegment} refuses + * a separator outright; {@link safeUrlPath} keeps separators as structure. Some + * provider parameters are neither: GitHub label names are commonly namespaced + * (`area/api`), and `DELETE /repos/{o}/{r}/issues/{n}/labels/{name}` takes the + * whole label as a single parameter, so the separator must survive as `%2F` + * rather than as a path boundary. Emitting a real `/` there would address a + * different endpoint; rejecting it would break a legitimate label. + * + * Percent-encoding a separator is safe on its own — the URL parser does not + * decode `%2F` before removing dot segments, so `a%2F..%2F..` stays put. The + * one hole encoding cannot close is a value that is *entirely* a dot segment, + * which is why that case is still rejected here rather than encoded, exactly as + * the module note requires. + * + * Prefer `safeUrlPathSegment`. Reach for this helper only when the provider + * documents the parameter as a single value that may itself contain `/`. + * + * @param value - The raw value, typically LLM- or user-supplied. + * @param paramName - The parameter name, used to name the offender in errors. + * @returns The trimmed value percent-encoded as a single segment, separators + * included, safe to interpolate. + * @throws If the value is not a string or a usable number, is empty, is a dot + * segment, or cannot be encoded. + */ +export function safeEncodedUrlPathSegment( + value: string | number | bigint, + paramName: string +): string { + const trimmed = toGuardedString(value, paramName).trim() + + if (!trimmed) { + throw new Error(`${paramName} is required`) + } + + if (trimmed === '.' || trimmed === '..') { + throw new Error(`${paramName} cannot be "${trimmed}" (path traversal is not allowed)`) + } + + return encodeSegment(trimmed, paramName) +} From 64273cfb84c810f59d4e26693c6864a2986a2092 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 21:27:21 -0700 Subject: [PATCH 2/6] fix(github): stop safeUrlPath trimming real filename whitespace Review found a data-integrity bug in the new helper: safeUrlPath trimmed each segment, but a leading or trailing space is a legal filename character that git stores verbatim, so `docs/ draft.md` was silently rewritten to `docs/draft.md` and read, updated, or deleted a different file than the caller named. Splits the behaviour by purpose instead of dropping trimming outright: - safeUrlPathSegment keeps trimming. Its inputs are opaque copy-pasted ids and ~690 call sites depend on it. - safeUrlPath no longer trims anywhere. Whitespace is preserved byte-for-byte and percent-encoded. A whitespace-only segment is still rejected, as are dot segments and backslashes. Not trimming does not weaken the dot check: the URL parser removes %2e%2e but leaves %20..%20 inert. Also from review: - Path-guard failures in lib/internal/github/operations.ts now raise GitHubOperationError(400) instead of a plain Error, which executeGitHubTool mapped to 500 for what is caller-supplied input. - The traversal suite drops ToolConfig and its `as any` cast for a structural interface plus a type guard. - The suite no longer swallows discovery failures. Every skip is recorded and asserted against an explicit expectation, which immediately surfaced that github_job_logs had fallen out of coverage entirely: a string filler in the sibling job_id parameter aborted the build before owner/repo could be probed. Non-target number parameters now get a number, and the 12 genuinely pathless tools are listed rather than inferred. --- .../lib/internal/github/operations.test.ts | 38 ++++ apps/sim/lib/internal/github/operations.ts | 36 +++- apps/sim/tools/github/path_safety.test.ts | 180 +++++++++++++++--- apps/sim/tools/url-path.test.ts | 58 +++++- apps/sim/tools/url-path.ts | 63 ++++-- 5 files changed, 333 insertions(+), 42 deletions(-) diff --git a/apps/sim/lib/internal/github/operations.test.ts b/apps/sim/lib/internal/github/operations.test.ts index e4686f9af3b..87d8c71738e 100644 --- a/apps/sim/lib/internal/github/operations.test.ts +++ b/apps/sim/lib/internal/github/operations.test.ts @@ -13,6 +13,7 @@ vi.mock('@/lib/core/security/input-validation.server', () => ({ validateUrlWithDNS: mocks.validateUrlWithDNS, })) +import { GitHubOperationError } from '@/lib/internal/github/errors' import { getGitHubLatestCommit } from '@/lib/internal/github/operations' describe('getGitHubLatestCommit', () => { @@ -61,3 +62,40 @@ describe('getGitHubLatestCommit', () => { ) }) }) + +/** + * The path guards throw a plain `Error`, which `executeGitHubTool` maps to 500. + * Every value they reject is caller-supplied, so it must surface as a 400 with + * the guard's own named message rather than as a server failure. + */ +describe('getGitHubLatestCommit path validation', () => { + beforeEach(() => { + vi.clearAllMocks() + mocks.validateUrlWithDNS.mockResolvedValue({ isValid: true, resolvedIP: '203.0.113.1' }) + }) + + it.each([ + ['..', 'path traversal is not allowed'], + ['../../orgs/secret', 'cannot contain a path separator'], + ])('reports owner %j as a client error, not a server failure', async (owner, message) => { + const error = await getGitHubLatestCommit( + { owner, repo: 'sim', apiKey: 'token' }, + { requestId: 'request-1' } + ).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(GitHubOperationError) + expect((error as GitHubOperationError).status).toBe(400) + expect((error as GitHubOperationError).message).toContain(message) + expect(mocks.secureFetchWithPinnedIP).not.toHaveBeenCalled() + }) + + it('rejects a branch that is a bare dot segment', async () => { + const error = await getGitHubLatestCommit( + { owner: 'simstudioai', repo: 'sim', branch: '..', apiKey: 'token' }, + { requestId: 'request-1' } + ).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(GitHubOperationError) + expect((error as GitHubOperationError).status).toBe(400) + }) +}) diff --git a/apps/sim/lib/internal/github/operations.ts b/apps/sim/lib/internal/github/operations.ts index d42c476d7a4..3c6adf75eac 100644 --- a/apps/sim/lib/internal/github/operations.ts +++ b/apps/sim/lib/internal/github/operations.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { getErrorMessage } from '@sim/utils/errors' import { isRecordLike } from '@sim/utils/object' import { secureFetchWithPinnedIP, @@ -98,8 +99,31 @@ function githubHeaders(apiKey: string): Record { } } +/** + * Runs the path guards for one provider URL, reporting a rejected value as a + * client error. + * + * The guards in `@/tools/url-path` throw a plain `Error`, and the catch in + * `executeGitHubTool` maps anything that is not a `GitHubOperationError` to + * 500. Every value they reject is caller-supplied — an `owner` of `..`, a + * `branch` carrying a separator — so reporting it as a server failure both + * misattributes the fault and hides the guard's message behind a generic + * status. 400 is the accurate answer, and it keeps the named + * " cannot be ..." text reaching the caller who can act on it. + */ +function buildGuardedUrl(build: () => string): string { + try { + return build() + } catch (error) { + throw new GitHubOperationError(getErrorMessage(error, 'Invalid GitHub request path'), 400) + } +} + function pullRequestUrl(params: CreateCommentParams): string { - return `${GITHUB_API_BASE}/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}` + return buildGuardedUrl( + () => + `${GITHUB_API_BASE}/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}` + ) } function isFileCommentRequest(params: CreateCommentParams): boolean { @@ -353,10 +377,12 @@ export async function getGitHubLatestCommit( context: GitHubOperationContext ): Promise { context.signal?.throwIfAborted() - const owner = safeUrlPathSegment(input.owner, 'owner') - const repo = safeUrlPathSegment(input.repo, 'repo') - const revision = safeEncodedUrlPathSegment(input.branch || 'HEAD', 'branch') - const commitUrl = `https://api.github.com/repos/${owner}/${repo}/commits/${revision}` + const commitUrl = buildGuardedUrl(() => { + const owner = safeUrlPathSegment(input.owner, 'owner') + const repo = safeUrlPathSegment(input.repo, 'repo') + const revision = safeEncodedUrlPathSegment(input.branch || 'HEAD', 'branch') + return `https://api.github.com/repos/${owner}/${repo}/commits/${revision}` + }) const validation = await validateUrlWithDNS(commitUrl, 'commitUrl') context.signal?.throwIfAborted() if (!validation.isValid || !validation.resolvedIP) { diff --git a/apps/sim/tools/github/path_safety.test.ts b/apps/sim/tools/github/path_safety.test.ts index 281357228e0..3d5c9707e16 100644 --- a/apps/sim/tools/github/path_safety.test.ts +++ b/apps/sim/tools/github/path_safety.test.ts @@ -27,9 +27,9 @@ * Tools are enumerated from the barrel rather than listed, so a newly added * GitHub tool that interpolates an unguarded parameter fails this suite. */ +import { getErrorMessage } from '@sim/utils/errors' import { describe, expect, it } from 'vitest' import * as githubTools from '@/tools/github/index' -import type { ToolConfig } from '@/tools/types' /** * The bare `.` and `..` entries are the whole point: their omission is why an @@ -89,6 +89,18 @@ const LEGITIMATE_PATHS = [ */ const MULTI_SEGMENT_PARAMS = new Set(['path', 'branch', 'ref', 'base', 'head']) +/** + * Filenames whose own leading, trailing, or interior spaces are content, not + * padding. Git tracks all three verbatim, so trimming any of them would make + * `update_file` and `delete_file` act on a different file than the caller named + * — a silent wrong-target write, which is why `safeUrlPath` does not trim. + */ +const WHITESPACE_PATHS: ReadonlyArray = [ + ['docs/my file .txt', 'docs/my%20file%20.txt'], + ['docs/ leading.md', 'docs/%20leading.md'], + ['docs/trailing.md ', 'docs/trailing.md%20'], +] + /** * Parameters the provider reads as one path parameter that may itself contain * `/` — a namespaced GitHub label such as `area/api`. The separator must @@ -98,16 +110,45 @@ const ENCODED_SEGMENT_PARAMS = new Set(['name']) const PROBE = 'PROBEVALUE' const FILLER = 'SAFEID' +const NUMBER_FILLER = 7 + +/** + * The shape this suite needs from a tool, declared structurally rather than as + * `ToolConfig`. + * + * The barrel's exports are heterogeneous — `ToolConfig` and `InternalToolConfig` + * over dozens of unrelated param types — so there is no single concrete + * instantiation to name here. Describing only the three members the harness + * touches keeps the boundary typed without `any` and without coupling the suite + * to any tool's param interface. + */ +interface UrlBuildingTool { + readonly id: string + readonly params?: Readonly> + readonly request?: { readonly url?: unknown } +} -type AnyTool = ToolConfig +/** + * A URL builder as this suite calls it. Each tool declares a narrower param + * type, but the harness deliberately feeds values those types forbid — a string + * into a `number` parameter — because that is precisely what an LLM tool call + * can do and what the guards must survive. + */ +type UrlBuilder = (params: Record) => string -function isGitHubTool(value: unknown): value is AnyTool { - return ( - typeof value === 'object' && - value !== null && - typeof (value as AnyTool).id === 'string' && - (value as AnyTool).id.startsWith('github') - ) +function isGitHubTool(value: unknown): value is UrlBuildingTool { + if (typeof value !== 'object' || value === null) return false + const id: unknown = (value as { id?: unknown }).id + return typeof id === 'string' && id.startsWith('github') +} + +/** + * Narrows a tool's `url` to a callable, or `null` when the tool serves a fixed + * URL string (the GraphQL tools) and has no path to exercise. + */ +function urlBuilderOf(tool: UrlBuildingTool): UrlBuilder | null { + const url = tool.request?.url + return typeof url === 'function' ? (url as UrlBuilder) : null } /** @@ -115,41 +156,57 @@ function isGitHubTool(value: unknown): value is AnyTool { * other string-ish parameter set to a constant, so the assertion isolates the * parameter under test. * - * Number-typed parameters are filled with the probe string too. Their declared - * type is not enforced anywhere between the LLM tool call and the URL builder, - * so an `issue_number` of `'..'` reaches the path exactly like a string one. + * The parameter under test always receives the probe *string*, whatever its + * declared type. That declaration is not enforced anywhere between the LLM tool + * call and the URL builder, so an `issue_number` of `'..'` reaches the path + * exactly like a string one, and a suite that only ever fed numbers there would + * miss the whole attack. + * + * Every *other* number parameter gets a real number, so a sibling's own + * validation cannot abort the build and hide the parameter under test. Filling + * them with a string made `job_logs` throw on `job_id` while `owner` was the + * target, silently dropping that tool from the suite entirely — which is what + * the skip ledger below now makes impossible. */ -function buildParams(tool: AnyTool, target: string, value: string): Record { +function buildParams( + tool: UrlBuildingTool, + target: string, + value: string +): Record { const params: Record = { apiKey: 'token' } for (const [name, def] of Object.entries(tool.params ?? {})) { if (name === 'apiKey') continue - const type = (def as { type?: string }).type - if (type === 'json' || type === 'array') { + const type = def.type + if (name === target) { + params[name] = value + } else if (type === 'json' || type === 'array') { params[name] = [] } else if (type === 'boolean') { params[name] = false + } else if (type === 'number') { + params[name] = NUMBER_FILLER } else { - params[name] = name === target ? value : FILLER + params[name] = FILLER } } return params } -function buildUrl(tool: AnyTool, target: string, value: string): URL { - const url = tool.request?.url - if (typeof url !== 'function') { +function buildUrl(tool: UrlBuildingTool, target: string, value: string): URL { + const build = urlBuilderOf(tool) + if (!build) { throw new Error(`${tool.id} does not build its URL from params`) } - return new URL(url(buildParams(tool, target, value) as any)) + return new URL(build(buildParams(tool, target, value))) } -function buildPath(tool: AnyTool, target: string, value: string): string { +function buildPath(tool: UrlBuildingTool, target: string, value: string): string { return buildUrl(tool, target, value).pathname } interface PathParamCase { name: string - tool: AnyTool + tool: UrlBuildingTool param: string baseline: string } @@ -161,14 +218,29 @@ interface PathParamCase { */ const PATH_PARAM_CASES: PathParamCase[] = [] +/** + * Every parameter whose baseline could not be built, with the reason. + * + * A silent `catch`/`continue` here would hide a tool from the suite entirely — + * the same class of blindness as an unguarded parameter, and one the aggregate + * count cannot detect, since a case that never existed cannot fail. So every + * skip is recorded and then asserted against an explicit expectation below. + */ +const SKIPPED: Array<{ id: string; param: string; reason: string }> = [] + for (const tool of Object.values(githubTools).filter(isGitHubTool)) { - if (typeof tool.request?.url !== 'function') continue + if (!urlBuilderOf(tool)) continue for (const param of Object.keys(tool.params ?? {})) { if (param === 'apiKey') continue let baseline: string try { baseline = buildPath(tool, param, PROBE) - } catch { + } catch (error) { + SKIPPED.push({ + id: tool.id, + param, + reason: getErrorMessage(error, 'unknown failure'), + }) continue } if (!baseline.includes(PROBE)) continue @@ -176,11 +248,67 @@ for (const tool of Object.values(githubTools).filter(isGitHubTool)) { } } +/** + * The only parameters allowed to refuse the probe, keyed by the guard that + * refuses them. + * + * `job_id` is validated as a positive integer before it reaches the path, so a + * string probe is rejected outright — which is the stronger outcome and is + * already pinned by `job_logs.test.ts`. Anything else appearing here means a + * tool dropped out of coverage and must be explained or fixed, not tolerated. + */ +const EXPECTED_SKIPS = new Set(['github_job_logs / job_id', 'github_job_logs_v2 / job_id']) + +/** + * Tools that build a URL but put no parameter in its path, so there is nothing + * for this suite to guard. + * + * The `search_*` tools assemble their URL with `URLSearchParams`, and + * `create_gist` posts to a fixed `/gists`. Listing them explicitly rather than + * inferring "no path params, therefore fine" is the point: a future tool that + * loses its coverage — by renaming a parameter, or by building its URL in a way + * the probe cannot see — shows up here as an unexplained entry instead of + * quietly vanishing from the suite. + */ +const PATHLESS_TOOLS = new Set([ + 'github_search_code', + 'github_search_code_v2', + 'github_search_commits', + 'github_search_commits_v2', + 'github_search_issues', + 'github_search_issues_v2', + 'github_search_repos', + 'github_search_repos_v2', + 'github_search_users', + 'github_search_users_v2', + 'github_create_gist', + 'github_create_gist_v2', +]) + describe('github path traversal safety', () => { it('covers every GitHub tool parameter that reaches the request path', () => { expect(PATH_PARAM_CASES.length).toBeGreaterThanOrEqual(60) }) + it('skips no parameter without an accounted-for reason', () => { + const unexplained = SKIPPED.filter( + (entry) => !EXPECTED_SKIPS.has(`${entry.id} / ${entry.param}`) + ) + + expect(unexplained).toEqual([]) + }) + + it('leaves no URL-building tool outside the suite unaccounted for', () => { + const builders = Object.values(githubTools) + .filter(isGitHubTool) + .filter((tool) => urlBuilderOf(tool) !== null) + .map((tool) => tool.id) + const covered = new Set(PATH_PARAM_CASES.map((entry) => entry.tool.id)) + const uncovered = builders.filter((id) => !covered.has(id) && !PATHLESS_TOOLS.has(id)) + + expect(uncovered).toEqual([]) + }) + it('covers the multi-segment parameters', () => { const covered = new Set(PATH_PARAM_CASES.map((entry) => entry.param)) for (const param of MULTI_SEGMENT_PARAMS) { @@ -225,6 +353,10 @@ describe('github path traversal safety', () => { it.each(LEGITIMATE_PATHS)('passes multi-segment %j through unchanged', (value) => { expect(buildPath(tool, param, value)).toBe(baseline.replaceAll(PROBE, value)) }) + + it.each(WHITESPACE_PATHS)('preserves the whitespace in %j', (value, encoded) => { + expect(buildPath(tool, param, value)).toBe(baseline.replaceAll(PROBE, encoded)) + }) } else if (ENCODED_SEGMENT_PARAMS.has(param)) { it.each(LEGITIMATE_PATHS)('keeps multi-segment %j inside one segment', (value) => { expect(buildPath(tool, param, value)).toBe( diff --git a/apps/sim/tools/url-path.test.ts b/apps/sim/tools/url-path.test.ts index 17f8e00b7e8..d32e8823250 100644 --- a/apps/sim/tools/url-path.test.ts +++ b/apps/sim/tools/url-path.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { safeUrlPathSegment } from '@/tools/url-path' +import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' const ORIGIN = 'https://api.example.com' @@ -345,3 +345,59 @@ describe('live call-site values', () => { expect(safeUrlPathSegment(0, 'sandboxId')).toBe('0') }) }) + +/** + * The two helpers take opposite positions on surrounding whitespace, and that + * split is load-bearing in both directions — so both directions are pinned. + * + * An id is an opaque copy-pasted token, so whitespace around it is transport + * noise. A path is content: a leading or trailing space is a legal filename + * character that git stores verbatim, so trimming one silently addresses a + * different file. See the TSDoc on `safeUrlPath` for the full reasoning. + */ +describe('whitespace handling differs by purpose', () => { + it('trims an opaque identifier, because the padding is not part of the id', () => { + expect(safeUrlPathSegment(' ecfg_abc123 ', 'edgeConfigId')).toBe('ecfg_abc123') + }) + + it.each([ + ['docs/my file .txt', 'docs/my%20file%20.txt'], + ['docs/ leading.md', 'docs/%20leading.md'], + ['docs/trailing.md ', 'docs/trailing.md%20'], + [' leading-dir/file.md', '%20leading-dir/file.md'], + ['docs/trailing-dir /file.md', 'docs/trailing-dir%20/file.md'], + ])('preserves %j byte-for-byte as a path', (value, expected) => { + expect(safeUrlPath(value, 'path')).toBe(expected) + }) + + it('round-trips a padded filename through the URL parser unchanged', () => { + const url = new URL(`${ORIGIN}/repos/o/r/contents/${safeUrlPath('docs/my file .txt', 'path')}`) + + expect(url.pathname).toBe('/repos/o/r/contents/docs/my%20file%20.txt') + expect(decodeURIComponent(url.pathname)).toBe('/repos/o/r/contents/docs/my file .txt') + }) + + it('still rejects a segment that is only whitespace', () => { + expect(() => safeUrlPath('docs/ /file.md', 'path')).toThrow(/whitespace-only path segment/) + }) + + it('still rejects a dot segment inside a path', () => { + expect(() => safeUrlPath('docs/../../etc/passwd', 'path')).toThrow( + /path traversal is not allowed/ + ) + }) + + it('leaves a space-wrapped dot segment inert rather than rejecting it', () => { + const url = new URL(`${ORIGIN}/repos/o/r/contents/${safeUrlPath('docs/ .. /x', 'path')}`) + + expect(url.pathname).toBe('/repos/o/r/contents/docs/%20..%20/x') + }) + + it('rejects a backslash anywhere in a path', () => { + expect(() => safeUrlPath('docs\\..\\..', 'path')).toThrow(/cannot contain a backslash/) + }) + + it('keeps a colon so a cross-fork compare ref still addresses its owner', () => { + expect(safeUrlPath('octocat:feature/my-branch', 'base')).toBe('octocat:feature/my-branch') + }) +}) diff --git a/apps/sim/tools/url-path.ts b/apps/sim/tools/url-path.ts index d42d95204f7..b228168a43c 100644 --- a/apps/sim/tools/url-path.ts +++ b/apps/sim/tools/url-path.ts @@ -154,6 +154,13 @@ function encodeSegment(segment: string, paramName: string): string { * Builds a single, traversal-safe URL path segment from an identifier that a * tool interpolates into a request path. * + * The value is trimmed first: these are opaque, copy-pasted identifiers, so + * surrounding whitespace is transport noise rather than part of the id, and + * call sites depend on that. {@link safeUrlPath} deliberately does **not** + * trim, because a path segment's leading and trailing spaces are legal + * filename characters and dropping them would address a different file — see + * the note on that function for the full reasoning. + * * Rejects empty values, dot segments, and any value still carrying a `/` or * `\` separator (defense in depth — encoding already neutralizes those, but a * separator in a single-segment parameter means the caller passed something @@ -214,6 +221,39 @@ export function safeUrlPathSegment(value: string | number | bigint, paramName: s * collapsing it would rewrite the caller's value into a different resource. * A trailing `/` is rejected on the same ground. * + * **This helper does not trim, and that is the deliberate difference from + * {@link safeUrlPathSegment}.** The two take opposite positions because their + * inputs are opposite kinds of thing: + * + * - A single-segment id (`ecfg_abc123`, a repo name, a numeric id) is an opaque + * token that a human copy-pastes, so surrounding whitespace is transport + * noise and `safeUrlPathSegment` strips it. Callers depend on that. + * - A path is *content*. A leading or trailing space is a legal character in a + * filename on every filesystem this addresses, and git stores it verbatim — + * `docs/ draft.md` and `docs/draft.md ` are three distinct files alongside + * `docs/draft.md`. Trimming here would silently rewrite the caller's path and + * read, update, or **delete** a different file than the one requested. That + * is a data-integrity bug, and a worse one than the traversal this module + * exists to stop, because it succeeds instead of failing. + * + * So whitespace inside the value is preserved byte-for-byte and percent-encoded + * (` ` becomes `%20`), including at the very start and end of the whole + * parameter, since those positions belong to the first and last filename just + * as much as any interior one. A caller who pastes a padded path gets a loud + * 404 for a file that does not exist rather than a quiet success against the + * wrong one. + * + * A segment that is *only* whitespace is still rejected: it names nothing the + * caller could have meant, and it is indistinguishable from the `//` case above. + * + * Not trimming also does not weaken the dot-segment check, which compares the + * raw segment. A space-wrapped dot segment needs no rejection because encoding + * it makes it inert — the URL parser removes `%2e%2e` but not `%20..%20`: + * + * ``` + * new URL('https://x/a/b/%20..%20').pathname // => '/a/b/%20..%20' (kept) + * ``` + * * A `:` is restored after encoding. It is a legal `pchar` with no delimiter or * traversal meaning inside a path segment, and providers use it structurally: * GitHub's compare endpoint addresses a cross-fork ref as `owner:branch`, which @@ -231,35 +271,34 @@ export function safeUrlPathSegment(value: string | number | bigint, paramName: s * @returns The trimmed path with every segment percent-encoded and the `/` * separators preserved, safe to interpolate. * @throws If the value is not a string or a usable number, is empty, contains - * an empty or dot segment, contains a backslash, or cannot be encoded. + * an empty or whitespace-only segment, contains a dot segment, contains a + * backslash, or cannot be encoded. */ export function safeUrlPath(value: string | number | bigint, paramName: string): string { - const trimmed = toGuardedString(value, paramName).trim() + const path = toGuardedString(value, paramName) - if (!trimmed) { + if (!path) { throw new Error(`${paramName} is required`) } - if (trimmed.includes('\\')) { + if (path.includes('\\')) { throw new Error(`${paramName} cannot contain a backslash`) } - return trimmed + return path .split('/') .map((segment) => { - const trimmedSegment = segment.trim() - - if (!trimmedSegment) { - throw new Error(`${paramName} cannot contain an empty path segment`) + if (!segment.trim()) { + throw new Error(`${paramName} cannot contain an empty or whitespace-only path segment`) } - if (trimmedSegment === '.' || trimmedSegment === '..') { + if (segment === '.' || segment === '..') { throw new Error( - `${paramName} cannot contain a "${trimmedSegment}" segment (path traversal is not allowed)` + `${paramName} cannot contain a "${segment}" segment (path traversal is not allowed)` ) } - return encodeSegment(trimmedSegment, paramName).replaceAll('%3A', ':') + return encodeSegment(segment, paramName).replaceAll('%3A', ':') }) .join('/') } From 515b9516cc16308041897919404dedb25b24b8a1 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 22:46:17 -0700 Subject: [PATCH 3/6] fix(github): permit a whitespace-only path component safeUrlPath rejected a path component made only of spaces. That check had no security value and a real cost: git tracks both a file and a directory whose entire name is spaces, so a valid GitHub file could not be read, updated, or deleted. A whitespace-only segment is not a dot segment, and the parser never removes it: new URL('https://x/a/%20%20%20/b').pathname => /a/%20%20%20/b (kept) new URL('https://x/a/../b').pathname => /b (removed) Only a truly empty component (a `//`, where the caller wrote no name at all) is rejected now. Dot-segment and backslash rejection are unchanged. safeUrlPathSegment still rejects an all-whitespace value. That asymmetry is correct: it trims opaque ids first, so one made only of spaces has named nothing. The TSDoc records why the check is absent, citing the git paths and the parser behaviour, so it is not restored on aesthetic grounds. --- apps/sim/tools/github/path_safety.test.ts | 7 ++++- apps/sim/tools/url-path.test.ts | 24 ++++++++++++++-- apps/sim/tools/url-path.ts | 35 +++++++++++++++++++---- 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/apps/sim/tools/github/path_safety.test.ts b/apps/sim/tools/github/path_safety.test.ts index 3d5c9707e16..959132c4bf7 100644 --- a/apps/sim/tools/github/path_safety.test.ts +++ b/apps/sim/tools/github/path_safety.test.ts @@ -91,14 +91,19 @@ const MULTI_SEGMENT_PARAMS = new Set(['path', 'branch', 'ref', 'base', 'head']) /** * Filenames whose own leading, trailing, or interior spaces are content, not - * padding. Git tracks all three verbatim, so trimming any of them would make + * padding. Git tracks all of these verbatim, so trimming any of them would make * `update_file` and `delete_file` act on a different file than the caller named * — a silent wrong-target write, which is why `safeUrlPath` does not trim. + * + * The last entry is a directory whose entire name is spaces. It is a legal git + * path and `%20%20%20` is never normalized away, so rejecting it would only + * make a real file unreachable. */ const WHITESPACE_PATHS: ReadonlyArray = [ ['docs/my file .txt', 'docs/my%20file%20.txt'], ['docs/ leading.md', 'docs/%20leading.md'], ['docs/trailing.md ', 'docs/trailing.md%20'], + ['docs/ /file.txt', 'docs/%20%20%20/file.txt'], ] /** diff --git a/apps/sim/tools/url-path.test.ts b/apps/sim/tools/url-path.test.ts index d32e8823250..60175a5d6c1 100644 --- a/apps/sim/tools/url-path.test.ts +++ b/apps/sim/tools/url-path.test.ts @@ -377,8 +377,28 @@ describe('whitespace handling differs by purpose', () => { expect(decodeURIComponent(url.pathname)).toBe('/repos/o/r/contents/docs/my file .txt') }) - it('still rejects a segment that is only whitespace', () => { - expect(() => safeUrlPath('docs/ /file.md', 'path')).toThrow(/whitespace-only path segment/) + it.each([ + ['docs/ /file.txt', 'docs/%20%20%20/file.txt'], + [' ', '%20%20%20'], + ['e/ /f.txt', 'e/%20%20%20/f.txt'], + ['d/ ', 'd/%20%20%20'], + ])('permits the whitespace-only component in %j', (value, expected) => { + expect(safeUrlPath(value, 'path')).toBe(expected) + }) + + it('round-trips a whitespace-only component through the URL parser', () => { + const url = new URL(`${ORIGIN}/repos/o/r/contents/${safeUrlPath('docs/ /file.txt', 'path')}`) + + expect(url.pathname).toBe('/repos/o/r/contents/docs/%20%20%20/file.txt') + expect(decodeURIComponent(url.pathname)).toBe('/repos/o/r/contents/docs/ /file.txt') + }) + + it('rejects only a truly empty component', () => { + expect(() => safeUrlPath('docs//file.txt', 'path')).toThrow(/empty path segment/) + }) + + it('still rejects an opaque id that is only whitespace, since it trims first', () => { + expect(() => safeUrlPathSegment(' ', 'edgeConfigId')).toThrow(/edgeConfigId is required/) }) it('still rejects a dot segment inside a path', () => { diff --git a/apps/sim/tools/url-path.ts b/apps/sim/tools/url-path.ts index b228168a43c..d58c6f01171 100644 --- a/apps/sim/tools/url-path.ts +++ b/apps/sim/tools/url-path.ts @@ -243,8 +243,33 @@ export function safeUrlPathSegment(value: string | number | bigint, paramName: s * 404 for a file that does not exist rather than a quiet success against the * wrong one. * - * A segment that is *only* whitespace is still rejected: it names nothing the - * caller could have meant, and it is indistinguishable from the `//` case above. + * A segment that is only whitespace is **permitted**, for the same reason the + * rest of the value is not trimmed, and the temptation to reject it on the + * grounds that it "names nothing" should be resisted. It names something: git + * tracks a file and a directory whose entire name is spaces, exactly as typed. + * + * ``` + * $ git ls-files | sed -n 'l' # `l` makes the line ends visible + * d/ $ + * e/ /f.txt$ + * ``` + * + * And rejecting it would buy nothing, because a whitespace-only segment is not + * a dot segment and the parser never removes it — the encoded form survives + * intact, where a dot segment does not: + * + * ``` + * new URL('https://x/a/%20%20%20/b').pathname // => '/a/%20%20%20/b' (kept) + * new URL('https://x/a/../b').pathname // => '/b' (removed) + * ``` + * + * So the check would carry no security value and a real cost: a legitimate file + * that could not be read, updated, or deleted. Only a *truly* empty component + * — the `//` case above, where the caller wrote no name at all — is rejected. + * + * {@link safeUrlPathSegment} does still reject an all-whitespace value, and + * that asymmetry is correct rather than an oversight: it trims first, so an + * opaque id of only spaces really has named nothing. * * Not trimming also does not weaken the dot-segment check, which compares the * raw segment. A space-wrapped dot segment needs no rejection because encoding @@ -271,7 +296,7 @@ export function safeUrlPathSegment(value: string | number | bigint, paramName: s * @returns The trimmed path with every segment percent-encoded and the `/` * separators preserved, safe to interpolate. * @throws If the value is not a string or a usable number, is empty, contains - * an empty or whitespace-only segment, contains a dot segment, contains a + * a truly empty segment (a `//`), contains a dot segment, contains a * backslash, or cannot be encoded. */ export function safeUrlPath(value: string | number | bigint, paramName: string): string { @@ -288,8 +313,8 @@ export function safeUrlPath(value: string | number | bigint, paramName: string): return path .split('/') .map((segment) => { - if (!segment.trim()) { - throw new Error(`${paramName} cannot contain an empty or whitespace-only path segment`) + if (!segment) { + throw new Error(`${paramName} cannot contain an empty path segment`) } if (segment === '.' || segment === '..') { From d2c74d74160e1c79cf9c0508dc11a96b018caff5 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 23:37:59 -0700 Subject: [PATCH 4/6] fix(github): refuse a padded identifier on state-changing requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Routing a raw identifier through a trimming guard silently turns a 404 no-op into a real mutation: before: DELETE /repos/%20%20acme%20%20/sim/git/refs/heads/main -> 404 after: DELETE /repos/acme/sim/git/refs/heads/main -> branch gone owner, repo and the numeric ids were interpolated raw before this branch, so every guard added here introduced that trim. No traversal test catches it, which is why it would have shipped unnoticed. Adds strictUrlPathSegment and strictEncodedUrlPathSegment, which refuse a padded value instead of trimming it, and applies them to every parameter this branch newly trims on a request whose method is not GET: 37 tools, 101 parameter sites, found by sweeping rather than by inspection. Reads keep the trimming guards, since their worst case is returning data the caller can ignore. The five gist tools keep trimming gist_id because they already trimmed it before this branch — preserving that is the same rule, not an exception to it. The strict guards live in url-path.ts rather than waiting for the shared helper on the dependent PR, because that PR is rebased onto this branch and therefore merges second. The duplication is deliberate and collapses to an import change when the two meet. --- apps/sim/lib/internal/github/operations.ts | 17 ++- apps/sim/tools/github/add_assignees.ts | 4 +- apps/sim/tools/github/add_labels.ts | 4 +- apps/sim/tools/github/cancel_workflow_run.ts | 4 +- apps/sim/tools/github/close_issue.ts | 4 +- apps/sim/tools/github/close_pr.ts | 4 +- apps/sim/tools/github/create_branch.ts | 4 +- .../tools/github/create_comment_reaction.ts | 4 +- apps/sim/tools/github/create_file.ts | 4 +- apps/sim/tools/github/create_issue.ts | 4 +- .../sim/tools/github/create_issue_reaction.ts | 4 +- apps/sim/tools/github/create_milestone.ts | 4 +- apps/sim/tools/github/create_pr.ts | 4 +- apps/sim/tools/github/create_pr_review.ts | 4 +- apps/sim/tools/github/create_release.ts | 4 +- apps/sim/tools/github/delete_branch.ts | 4 +- apps/sim/tools/github/delete_comment.ts | 4 +- .../tools/github/delete_comment_reaction.ts | 4 +- apps/sim/tools/github/delete_file.ts | 4 +- .../sim/tools/github/delete_issue_reaction.ts | 4 +- apps/sim/tools/github/delete_milestone.ts | 4 +- apps/sim/tools/github/delete_release.ts | 4 +- apps/sim/tools/github/fork_repo.ts | 4 +- apps/sim/tools/github/issue_comment.ts | 4 +- apps/sim/tools/github/merge_pr.ts | 4 +- apps/sim/tools/github/path_safety.test.ts | 127 ++++++++++++++++++ apps/sim/tools/github/remove_label.ts | 4 +- apps/sim/tools/github/request_reviewers.ts | 4 +- apps/sim/tools/github/rerun_workflow.ts | 4 +- apps/sim/tools/github/star_repo.ts | 4 +- apps/sim/tools/github/trigger_workflow.ts | 4 +- apps/sim/tools/github/unstar_repo.ts | 4 +- .../tools/github/update_branch_protection.ts | 4 +- apps/sim/tools/github/update_comment.ts | 4 +- apps/sim/tools/github/update_file.ts | 4 +- apps/sim/tools/github/update_issue.ts | 4 +- apps/sim/tools/github/update_milestone.ts | 4 +- apps/sim/tools/github/update_pr.ts | 4 +- apps/sim/tools/github/update_release.ts | 4 +- apps/sim/tools/url-path.test.ts | 59 +++++++- apps/sim/tools/url-path.ts | 84 ++++++++++++ 41 files changed, 358 insertions(+), 77 deletions(-) diff --git a/apps/sim/lib/internal/github/operations.ts b/apps/sim/lib/internal/github/operations.ts index 3c6adf75eac..32f35c947f6 100644 --- a/apps/sim/lib/internal/github/operations.ts +++ b/apps/sim/lib/internal/github/operations.ts @@ -20,7 +20,11 @@ import type { } from '@/tools/github/types' import { secureGitHubRequest } from '@/tools/github/utils.server' import type { ToolResponse } from '@/tools/types' -import { safeEncodedUrlPathSegment, safeUrlPathSegment } from '@/tools/url-path' +import { + safeEncodedUrlPathSegment, + safeUrlPathSegment, + strictUrlPathSegment, +} from '@/tools/url-path' const logger = createLogger('GitHubLatestCommitOperation') const MAX_COMMIT_RESPONSE_BYTES = 10 * 1024 * 1024 @@ -119,10 +123,19 @@ function buildGuardedUrl(build: () => string): string { } } +/** + * The pull-request URL, and the base for the comment and review URLs built from + * it. + * + * Uses the strict guards even though this same URL is also fetched with a GET + * to read the head SHA: every caller reaches it on the way to creating a + * comment or a review, so the operation as a whole changes state and must not + * have a padded identifier quietly resolved to a real pull request. + */ function pullRequestUrl(params: CreateCommentParams): string { return buildGuardedUrl( () => - `${GITHUB_API_BASE}/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}` + `${GITHUB_API_BASE}/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}` ) } diff --git a/apps/sim/tools/github/add_assignees.ts b/apps/sim/tools/github/add_assignees.ts index 074c4bca3e4..90a3a6720fc 100644 --- a/apps/sim/tools/github/add_assignees.ts +++ b/apps/sim/tools/github/add_assignees.ts @@ -1,6 +1,6 @@ import type { AddAssigneesParams, IssueResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const addAssigneesTool: ToolConfig = { id: 'github_add_assignees', @@ -43,7 +43,7 @@ export const addAssigneesTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/assignees`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}/assignees`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/add_labels.ts b/apps/sim/tools/github/add_labels.ts index efbfc3ef565..c1d1fa28989 100644 --- a/apps/sim/tools/github/add_labels.ts +++ b/apps/sim/tools/github/add_labels.ts @@ -1,6 +1,6 @@ import type { AddLabelsParams, LabelsResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const addLabelsTool: ToolConfig = { id: 'github_add_labels', @@ -43,7 +43,7 @@ export const addLabelsTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/labels`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}/labels`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/cancel_workflow_run.ts b/apps/sim/tools/github/cancel_workflow_run.ts index acc7269c6f8..3db4e0cd39b 100644 --- a/apps/sim/tools/github/cancel_workflow_run.ts +++ b/apps/sim/tools/github/cancel_workflow_run.ts @@ -1,6 +1,6 @@ import type { CancelWorkflowRunParams, CancelWorkflowRunResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const cancelWorkflowRunTool: ToolConfig = { @@ -39,7 +39,7 @@ export const cancelWorkflowRunTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/runs/${safeUrlPathSegment(params.run_id, 'run_id')}/cancel`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/actions/runs/${strictUrlPathSegment(params.run_id, 'run_id')}/cancel`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/close_issue.ts b/apps/sim/tools/github/close_issue.ts index 0d05d096072..6b0ddca9d6d 100644 --- a/apps/sim/tools/github/close_issue.ts +++ b/apps/sim/tools/github/close_issue.ts @@ -1,7 +1,7 @@ import type { CloseIssueParams, IssueResponse } from '@/tools/github/types' import { ISSUE_OUTPUT_PROPERTIES, LABEL_OUTPUT, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const closeIssueTool: ToolConfig = { id: 'github_close_issue', @@ -44,7 +44,7 @@ export const closeIssueTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/close_pr.ts b/apps/sim/tools/github/close_pr.ts index 948b525fb9a..cf9f0f54ead 100644 --- a/apps/sim/tools/github/close_pr.ts +++ b/apps/sim/tools/github/close_pr.ts @@ -1,6 +1,6 @@ import type { ClosePRParams, PRResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const closePRTool: ToolConfig = { id: 'github_close_pr', @@ -37,7 +37,7 @@ export const closePRTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/create_branch.ts b/apps/sim/tools/github/create_branch.ts index ee8b3a56468..d4c01ba0cc9 100644 --- a/apps/sim/tools/github/create_branch.ts +++ b/apps/sim/tools/github/create_branch.ts @@ -1,7 +1,7 @@ import type { CreateBranchParams, RefResponse } from '@/tools/github/types' import { GIT_REF_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const createBranchTool: ToolConfig = { id: 'github_create_branch', @@ -45,7 +45,7 @@ export const createBranchTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/git/refs`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/git/refs`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/create_comment_reaction.ts b/apps/sim/tools/github/create_comment_reaction.ts index a9dbb599201..8a571204996 100644 --- a/apps/sim/tools/github/create_comment_reaction.ts +++ b/apps/sim/tools/github/create_comment_reaction.ts @@ -1,6 +1,6 @@ import { REACTION_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface CreateCommentReactionParams { owner: string @@ -68,7 +68,7 @@ export const createCommentReactionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}/reactions`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/comments/${strictUrlPathSegment(params.comment_id, 'comment_id')}/reactions`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.squirrel-girl-preview+json', diff --git a/apps/sim/tools/github/create_file.ts b/apps/sim/tools/github/create_file.ts index b5ca7b5f7ce..db05b2fa861 100644 --- a/apps/sim/tools/github/create_file.ts +++ b/apps/sim/tools/github/create_file.ts @@ -1,6 +1,6 @@ import type { CreateFileParams, FileOperationResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' +import { safeUrlPath, strictUrlPathSegment } from '@/tools/url-path' export const createFileTool: ToolConfig = { id: 'github_create_file', @@ -56,7 +56,7 @@ export const createFileTool: ToolConfig request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/create_issue.ts b/apps/sim/tools/github/create_issue.ts index b49e89b8268..1c7a6cdf139 100644 --- a/apps/sim/tools/github/create_issue.ts +++ b/apps/sim/tools/github/create_issue.ts @@ -6,7 +6,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const createIssueTool: ToolConfig = { id: 'github_create_issue', @@ -67,7 +67,7 @@ export const createIssueTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/create_issue_reaction.ts b/apps/sim/tools/github/create_issue_reaction.ts index 52f3ae0cfd6..c8b22c04867 100644 --- a/apps/sim/tools/github/create_issue_reaction.ts +++ b/apps/sim/tools/github/create_issue_reaction.ts @@ -1,6 +1,6 @@ import { REACTION_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface CreateIssueReactionParams { owner: string @@ -68,7 +68,7 @@ export const createIssueReactionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/reactions`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}/reactions`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.squirrel-girl-preview+json', diff --git a/apps/sim/tools/github/create_milestone.ts b/apps/sim/tools/github/create_milestone.ts index 458510eea8e..3c12ed244d4 100644 --- a/apps/sim/tools/github/create_milestone.ts +++ b/apps/sim/tools/github/create_milestone.ts @@ -1,6 +1,6 @@ import { MILESTONE_CREATOR_OUTPUT, MILESTONE_V2_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface CreateMilestoneParams { owner: string @@ -85,7 +85,7 @@ export const createMilestoneTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/milestones`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/milestones`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/create_pr.ts b/apps/sim/tools/github/create_pr.ts index a02bea2b783..6b8a78b447c 100644 --- a/apps/sim/tools/github/create_pr.ts +++ b/apps/sim/tools/github/create_pr.ts @@ -1,6 +1,6 @@ import type { CreatePRParams, PRResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const createPRTool: ToolConfig = { id: 'github_create_pr', @@ -61,7 +61,7 @@ export const createPRTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/create_pr_review.ts b/apps/sim/tools/github/create_pr_review.ts index 5aafe897741..22e7d2f623b 100644 --- a/apps/sim/tools/github/create_pr_review.ts +++ b/apps/sim/tools/github/create_pr_review.ts @@ -19,7 +19,7 @@ import type { } from '@/tools/github/types' import { USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig, ToolResponse } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' const COMMIT_SHA_PATTERN = /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/i @@ -160,7 +160,7 @@ export const createPRReviewTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}/reviews`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}/reviews`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/create_release.ts b/apps/sim/tools/github/create_release.ts index 462d3c22caf..131f26be617 100644 --- a/apps/sim/tools/github/create_release.ts +++ b/apps/sim/tools/github/create_release.ts @@ -5,7 +5,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const createReleaseTool: ToolConfig = { id: 'github_create_release', @@ -77,7 +77,7 @@ export const createReleaseTool: ToolConfig request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/releases`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/delete_branch.ts b/apps/sim/tools/github/delete_branch.ts index 783691e9e1b..806180f9dea 100644 --- a/apps/sim/tools/github/delete_branch.ts +++ b/apps/sim/tools/github/delete_branch.ts @@ -1,7 +1,7 @@ import type { DeleteBranchParams, DeleteBranchResponse } from '@/tools/github/types' import { DELETE_BRANCH_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' +import { safeUrlPath, strictUrlPathSegment } from '@/tools/url-path' export const deleteBranchTool: ToolConfig = { id: 'github_delete_branch', @@ -39,7 +39,7 @@ export const deleteBranchTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/git/refs/heads/${safeUrlPath(params.branch, 'branch')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/git/refs/heads/${safeUrlPath(params.branch, 'branch')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/delete_comment.ts b/apps/sim/tools/github/delete_comment.ts index 00f30ee0921..af2e20ae643 100644 --- a/apps/sim/tools/github/delete_comment.ts +++ b/apps/sim/tools/github/delete_comment.ts @@ -1,6 +1,6 @@ import type { DeleteCommentParams, DeleteCommentResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const deleteCommentTool: ToolConfig = { id: 'github_delete_comment', @@ -37,7 +37,7 @@ export const deleteCommentTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/comments/${strictUrlPathSegment(params.comment_id, 'comment_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/delete_comment_reaction.ts b/apps/sim/tools/github/delete_comment_reaction.ts index 2845c4bef23..980d21b9553 100644 --- a/apps/sim/tools/github/delete_comment_reaction.ts +++ b/apps/sim/tools/github/delete_comment_reaction.ts @@ -1,5 +1,5 @@ import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface DeleteCommentReactionParams { owner: string @@ -64,7 +64,7 @@ export const deleteCommentReactionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}/reactions/${safeUrlPathSegment(params.reaction_id, 'reaction_id')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/comments/${strictUrlPathSegment(params.comment_id, 'comment_id')}/reactions/${strictUrlPathSegment(params.reaction_id, 'reaction_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.squirrel-girl-preview+json', diff --git a/apps/sim/tools/github/delete_file.ts b/apps/sim/tools/github/delete_file.ts index 671fa021262..6d05d775f00 100644 --- a/apps/sim/tools/github/delete_file.ts +++ b/apps/sim/tools/github/delete_file.ts @@ -1,6 +1,6 @@ import type { DeleteFileParams, DeleteFileResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' +import { safeUrlPath, strictUrlPathSegment } from '@/tools/url-path' export const deleteFileTool: ToolConfig = { id: 'github_delete_file', @@ -56,7 +56,7 @@ export const deleteFileTool: ToolConfig = request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/delete_issue_reaction.ts b/apps/sim/tools/github/delete_issue_reaction.ts index a048baa473b..4ebdef9fed9 100644 --- a/apps/sim/tools/github/delete_issue_reaction.ts +++ b/apps/sim/tools/github/delete_issue_reaction.ts @@ -1,5 +1,5 @@ import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface DeleteIssueReactionParams { owner: string @@ -64,7 +64,7 @@ export const deleteIssueReactionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/reactions/${safeUrlPathSegment(params.reaction_id, 'reaction_id')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}/reactions/${strictUrlPathSegment(params.reaction_id, 'reaction_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.squirrel-girl-preview+json', diff --git a/apps/sim/tools/github/delete_milestone.ts b/apps/sim/tools/github/delete_milestone.ts index 21573c6457d..eb8d6471957 100644 --- a/apps/sim/tools/github/delete_milestone.ts +++ b/apps/sim/tools/github/delete_milestone.ts @@ -1,5 +1,5 @@ import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface DeleteMilestoneParams { owner: string @@ -54,7 +54,7 @@ export const deleteMilestoneTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/milestones/${safeUrlPathSegment(params.milestone_number, 'milestone_number')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/milestones/${strictUrlPathSegment(params.milestone_number, 'milestone_number')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/delete_release.ts b/apps/sim/tools/github/delete_release.ts index 47ae18fa5a8..45074dc0255 100644 --- a/apps/sim/tools/github/delete_release.ts +++ b/apps/sim/tools/github/delete_release.ts @@ -1,6 +1,6 @@ import type { DeleteReleaseParams, DeleteReleaseResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const deleteReleaseTool: ToolConfig = { id: 'github_delete_release', @@ -38,7 +38,7 @@ export const deleteReleaseTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases/${safeUrlPathSegment(params.release_id, 'release_id')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/releases/${strictUrlPathSegment(params.release_id, 'release_id')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/fork_repo.ts b/apps/sim/tools/github/fork_repo.ts index 7768f06d5e6..fc632da3929 100644 --- a/apps/sim/tools/github/fork_repo.ts +++ b/apps/sim/tools/github/fork_repo.ts @@ -5,7 +5,7 @@ import { USER_FULL_OUTPUT_PROPERTIES, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface ForkRepoParams { owner: string @@ -83,7 +83,7 @@ export const forkRepoTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/forks`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/forks`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/issue_comment.ts b/apps/sim/tools/github/issue_comment.ts index d235b46b94d..389da94e31c 100644 --- a/apps/sim/tools/github/issue_comment.ts +++ b/apps/sim/tools/github/issue_comment.ts @@ -2,7 +2,7 @@ import { truncate } from '@sim/utils/string' import type { CreateIssueCommentParams, IssueCommentResponse } from '@/tools/github/types' import { COMMENT_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const issueCommentTool: ToolConfig = { id: 'github_issue_comment', @@ -45,7 +45,7 @@ export const issueCommentTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/comments`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}/comments`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/merge_pr.ts b/apps/sim/tools/github/merge_pr.ts index d94858cb38c..b606ad206fd 100644 --- a/apps/sim/tools/github/merge_pr.ts +++ b/apps/sim/tools/github/merge_pr.ts @@ -1,6 +1,6 @@ import type { MergePRParams, MergeResultResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const mergePRTool: ToolConfig = { id: 'github_merge_pr', @@ -56,7 +56,7 @@ export const mergePRTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}/merge`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}/merge`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/path_safety.test.ts b/apps/sim/tools/github/path_safety.test.ts index 959132c4bf7..a8340e9190e 100644 --- a/apps/sim/tools/github/path_safety.test.ts +++ b/apps/sim/tools/github/path_safety.test.ts @@ -290,6 +290,133 @@ const PATHLESS_TOOLS = new Set([ 'github_create_gist_v2', ]) +/** + * Every (tool, parameter) pair where this PR newly introduced trimming on a + * request that changes state, derived the same way the fix was: the parameter + * was interpolated raw before the guards landed, and its tool's method is not + * GET. + * + * Before the guards, a padded identifier reached GitHub as `%20%20acme%20%20`, + * matched nothing, and the request was a 404 no-op. A trimming guard would turn + * that no-op into a real mutation — a deleted branch, a closed pull request — + * while every traversal assertion in this file kept passing, which is precisely + * why it needed pinning rather than reasoning. + * + * The list is explicit rather than computed so that removing a guard cannot + * also remove its own assertion. + */ +const MUTATING_STRICT_PARAMS: Readonly> = { + github_delete_branch: ['owner', 'repo'], + github_delete_comment: ['owner', 'repo', 'comment_id'], + github_delete_comment_reaction: ['owner', 'repo', 'comment_id', 'reaction_id'], + github_delete_file: ['owner', 'repo'], + github_delete_issue_reaction: ['owner', 'repo', 'issue_number', 'reaction_id'], + github_delete_milestone: ['owner', 'repo', 'milestone_number'], + github_delete_release: ['owner', 'repo', 'release_id'], + github_remove_label: ['owner', 'repo', 'issue_number', 'name'], + github_unstar_repo: ['owner', 'repo'], + github_close_issue: ['owner', 'repo', 'issue_number'], + github_close_pr: ['owner', 'repo', 'pullNumber'], + github_update_comment: ['owner', 'repo', 'comment_id'], + github_update_issue: ['owner', 'repo', 'issue_number'], + github_update_milestone: ['owner', 'repo', 'milestone_number'], + github_update_pr: ['owner', 'repo', 'pullNumber'], + github_update_release: ['owner', 'repo', 'release_id'], + github_add_assignees: ['owner', 'repo', 'issue_number'], + github_add_labels: ['owner', 'repo', 'issue_number'], + github_cancel_workflow_run: ['owner', 'repo', 'run_id'], + github_create_branch: ['owner', 'repo'], + github_create_comment_reaction: ['owner', 'repo', 'comment_id'], + github_create_issue: ['owner', 'repo'], + github_create_issue_reaction: ['owner', 'repo', 'issue_number'], + github_create_milestone: ['owner', 'repo'], + github_create_pr: ['owner', 'repo'], + github_create_pr_review: ['owner', 'repo', 'pullNumber'], + github_create_release: ['owner', 'repo'], + github_fork_repo: ['owner', 'repo'], + github_issue_comment: ['owner', 'repo', 'issue_number'], + github_request_reviewers: ['owner', 'repo', 'pullNumber'], + github_rerun_workflow: ['owner', 'repo', 'run_id'], + github_trigger_workflow: ['owner', 'repo', 'workflow_id'], + github_create_file: ['owner', 'repo'], + github_merge_pr: ['owner', 'repo', 'pullNumber'], + github_star_repo: ['owner', 'repo'], + github_update_branch_protection: ['owner', 'repo'], + github_update_file: ['owner', 'repo'], +} + +/** Identifiers that already trimmed before this PR, so they must keep trimming. */ +const PRE_TRIMMED_PARAMS: Readonly> = { + github_delete_gist: ['gist_id'], + github_star_gist: ['gist_id'], + github_unstar_gist: ['gist_id'], + github_fork_gist: ['gist_id'], + github_update_gist: ['gist_id'], +} + +const PADDED_VALUES = [' octocat ', 'octocat ', ' octocat', '\toctocat'] as const + +describe('mutating routes refuse a padded identifier', () => { + const entries = Object.entries(MUTATING_STRICT_PARAMS).flatMap(([id, params]) => + params.map((param) => ({ name: `${id} / ${param}`, id, param })) + ) + + it('pins every mutating tool this PR newly trimmed', () => { + expect(entries.length).toBe(101) + }) + + describe.each(entries)('$name', ({ id, param }) => { + const tool = Object.values(githubTools) + .filter(isGitHubTool) + .find((candidate) => candidate.id === id) + + it('is present in the barrel', () => { + expect(tool).toBeDefined() + }) + + it.each(PADDED_VALUES)('throws on %j rather than resolving it', (value) => { + expect(() => buildPath(tool as UrlBuildingTool, param, value)).toThrow( + /must not have leading or trailing whitespace/ + ) + }) + + it('still accepts the same value unpadded', () => { + expect(() => buildPath(tool as UrlBuildingTool, param, 'octocat')).not.toThrow() + }) + }) +}) + +describe('reads and already-trimmed identifiers keep trimming', () => { + const preTrimmed = Object.entries(PRE_TRIMMED_PARAMS).flatMap(([id, params]) => + params.map((param) => ({ name: `${id} / ${param}`, id, param })) + ) + + it.each(preTrimmed)('$name trims, because it trimmed before this PR', ({ id, param }) => { + const tool = Object.values(githubTools) + .filter(isGitHubTool) + .find((candidate) => candidate.id === id) + expect(tool).toBeDefined() + + const padded = buildPath(tool as UrlBuildingTool, param, ' abc123 ') + expect(padded).toBe(buildPath(tool as UrlBuildingTool, param, 'abc123')) + }) + + it.each([ + { id: 'github_get_issue', param: 'owner' }, + { id: 'github_repo_info', param: 'repo' }, + { id: 'github_get_file_content', param: 'owner' }, + ])('$id / $param trims on a read', ({ id, param }) => { + const tool = Object.values(githubTools) + .filter(isGitHubTool) + .find((candidate) => candidate.id === id) + expect(tool).toBeDefined() + + expect(buildPath(tool as UrlBuildingTool, param, ' octocat ')).toBe( + buildPath(tool as UrlBuildingTool, param, 'octocat') + ) + }) +}) + describe('github path traversal safety', () => { it('covers every GitHub tool parameter that reaches the request path', () => { expect(PATH_PARAM_CASES.length).toBeGreaterThanOrEqual(60) diff --git a/apps/sim/tools/github/remove_label.ts b/apps/sim/tools/github/remove_label.ts index 630e0f3b181..00ed8b01960 100644 --- a/apps/sim/tools/github/remove_label.ts +++ b/apps/sim/tools/github/remove_label.ts @@ -1,6 +1,6 @@ import type { LabelsResponse, RemoveLabelParams } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeEncodedUrlPathSegment, safeUrlPathSegment } from '@/tools/url-path' +import { strictEncodedUrlPathSegment, strictUrlPathSegment } from '@/tools/url-path' export const removeLabelTool: ToolConfig = { id: 'github_remove_label', @@ -43,7 +43,7 @@ export const removeLabelTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}/labels/${safeEncodedUrlPathSegment(params.name, 'name')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}/labels/${strictEncodedUrlPathSegment(params.name, 'name')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/request_reviewers.ts b/apps/sim/tools/github/request_reviewers.ts index cb8e2123013..08eb560e6cf 100644 --- a/apps/sim/tools/github/request_reviewers.ts +++ b/apps/sim/tools/github/request_reviewers.ts @@ -1,6 +1,6 @@ import type { RequestReviewersParams, ReviewersResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const requestReviewersTool: ToolConfig = { id: 'github_request_reviewers', @@ -50,7 +50,7 @@ export const requestReviewersTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}/requested_reviewers`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}/requested_reviewers`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/rerun_workflow.ts b/apps/sim/tools/github/rerun_workflow.ts index 42a0a6f8904..98f77a973b3 100644 --- a/apps/sim/tools/github/rerun_workflow.ts +++ b/apps/sim/tools/github/rerun_workflow.ts @@ -1,6 +1,6 @@ import type { RerunWorkflowParams, RerunWorkflowResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const rerunWorkflowTool: ToolConfig = { id: 'github_rerun_workflow', @@ -45,7 +45,7 @@ export const rerunWorkflowTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/runs/${safeUrlPathSegment(params.run_id, 'run_id')}/rerun`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/actions/runs/${strictUrlPathSegment(params.run_id, 'run_id')}/rerun`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/star_repo.ts b/apps/sim/tools/github/star_repo.ts index d3a3f2e8afe..5c085a31e4f 100644 --- a/apps/sim/tools/github/star_repo.ts +++ b/apps/sim/tools/github/star_repo.ts @@ -1,5 +1,5 @@ import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface StarRepoParams { owner: string @@ -48,7 +48,7 @@ export const starRepoTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/user/starred/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}`, + `https://api.github.com/user/starred/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/trigger_workflow.ts b/apps/sim/tools/github/trigger_workflow.ts index dfc55802594..94d1a8ad7f1 100644 --- a/apps/sim/tools/github/trigger_workflow.ts +++ b/apps/sim/tools/github/trigger_workflow.ts @@ -1,6 +1,6 @@ import type { TriggerWorkflowParams, TriggerWorkflowResponse } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const triggerWorkflowTool: ToolConfig = { id: 'github_trigger_workflow', @@ -50,7 +50,7 @@ export const triggerWorkflowTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/workflows/${safeUrlPathSegment(params.workflow_id, 'workflow_id')}/dispatches`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/actions/workflows/${strictUrlPathSegment(params.workflow_id, 'workflow_id')}/dispatches`, method: 'POST', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/unstar_repo.ts b/apps/sim/tools/github/unstar_repo.ts index 1edcb1c2601..37771980917 100644 --- a/apps/sim/tools/github/unstar_repo.ts +++ b/apps/sim/tools/github/unstar_repo.ts @@ -1,5 +1,5 @@ import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface UnstarRepoParams { owner: string @@ -48,7 +48,7 @@ export const unstarRepoTool: ToolConfig = request: { url: (params) => - `https://api.github.com/user/starred/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}`, + `https://api.github.com/user/starred/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}`, method: 'DELETE', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_branch_protection.ts b/apps/sim/tools/github/update_branch_protection.ts index 2e8c39017bf..02190ee0f3d 100644 --- a/apps/sim/tools/github/update_branch_protection.ts +++ b/apps/sim/tools/github/update_branch_protection.ts @@ -1,7 +1,7 @@ import type { BranchProtectionResponse, UpdateBranchProtectionParams } from '@/tools/github/types' import { BRANCH_PROTECTION_OUTPUT_PROPERTIES } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' +import { safeUrlPath, strictUrlPathSegment } from '@/tools/url-path' export const updateBranchProtectionTool: ToolConfig< UpdateBranchProtectionParams, @@ -69,7 +69,7 @@ export const updateBranchProtectionTool: ToolConfig< request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/branches/${safeUrlPath(params.branch, 'branch')}/protection`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/branches/${safeUrlPath(params.branch, 'branch')}/protection`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/update_comment.ts b/apps/sim/tools/github/update_comment.ts index cd1e5c364fb..8166ce5f03e 100644 --- a/apps/sim/tools/github/update_comment.ts +++ b/apps/sim/tools/github/update_comment.ts @@ -2,7 +2,7 @@ import { truncate } from '@sim/utils/string' import type { IssueCommentResponse, UpdateCommentParams } from '@/tools/github/types' import { COMMENT_OUTPUT_PROPERTIES, USER_OUTPUT } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const updateCommentTool: ToolConfig = { id: 'github_update_comment', @@ -45,7 +45,7 @@ export const updateCommentTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/comments/${strictUrlPathSegment(params.comment_id, 'comment_id')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/update_file.ts b/apps/sim/tools/github/update_file.ts index e2c4f48ba59..f41cbfb044a 100644 --- a/apps/sim/tools/github/update_file.ts +++ b/apps/sim/tools/github/update_file.ts @@ -1,6 +1,6 @@ import type { FileOperationResponse, UpdateFileParams } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' +import { safeUrlPath, strictUrlPathSegment } from '@/tools/url-path' export const updateFileTool: ToolConfig = { id: 'github_update_file', @@ -62,7 +62,7 @@ export const updateFileTool: ToolConfig request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`, method: 'PUT', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/github/update_issue.ts b/apps/sim/tools/github/update_issue.ts index 96e764ac130..f494d6f7ab2 100644 --- a/apps/sim/tools/github/update_issue.ts +++ b/apps/sim/tools/github/update_issue.ts @@ -6,7 +6,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const updateIssueTool: ToolConfig = { id: 'github_update_issue', @@ -73,7 +73,7 @@ export const updateIssueTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_milestone.ts b/apps/sim/tools/github/update_milestone.ts index 5a801f10014..0b56487c9df 100644 --- a/apps/sim/tools/github/update_milestone.ts +++ b/apps/sim/tools/github/update_milestone.ts @@ -1,5 +1,5 @@ import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' interface UpdateMilestoneParams { owner: string @@ -89,7 +89,7 @@ export const updateMilestoneTool: ToolConfig - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/milestones/${safeUrlPathSegment(params.milestone_number, 'milestone_number')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/milestones/${strictUrlPathSegment(params.milestone_number, 'milestone_number')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_pr.ts b/apps/sim/tools/github/update_pr.ts index fdb0dfe7bc7..38fd70e83da 100644 --- a/apps/sim/tools/github/update_pr.ts +++ b/apps/sim/tools/github/update_pr.ts @@ -1,6 +1,6 @@ import type { PRResponse, UpdatePRParams } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const updatePRTool: ToolConfig = { id: 'github_update_pr', @@ -61,7 +61,7 @@ export const updatePRTool: ToolConfig = { request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github.v3+json', diff --git a/apps/sim/tools/github/update_release.ts b/apps/sim/tools/github/update_release.ts index 06eaab91b7c..5eab21d6921 100644 --- a/apps/sim/tools/github/update_release.ts +++ b/apps/sim/tools/github/update_release.ts @@ -5,7 +5,7 @@ import { USER_OUTPUT, } from '@/tools/github/types' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' +import { strictUrlPathSegment } from '@/tools/url-path' export const updateReleaseTool: ToolConfig = { id: 'github_update_release', @@ -79,7 +79,7 @@ export const updateReleaseTool: ToolConfig request: { url: (params) => - `https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/releases/${safeUrlPathSegment(params.release_id, 'release_id')}`, + `https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/releases/${strictUrlPathSegment(params.release_id, 'release_id')}`, method: 'PATCH', headers: (params) => ({ Accept: 'application/vnd.github+json', diff --git a/apps/sim/tools/url-path.test.ts b/apps/sim/tools/url-path.test.ts index 60175a5d6c1..1ef8d54e811 100644 --- a/apps/sim/tools/url-path.test.ts +++ b/apps/sim/tools/url-path.test.ts @@ -2,7 +2,12 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { safeUrlPath, safeUrlPathSegment } from '@/tools/url-path' +import { + safeUrlPath, + safeUrlPathSegment, + strictEncodedUrlPathSegment, + strictUrlPathSegment, +} from '@/tools/url-path' const ORIGIN = 'https://api.example.com' @@ -421,3 +426,55 @@ describe('whitespace handling differs by purpose', () => { expect(safeUrlPath('octocat:feature/my-branch', 'base')).toBe('octocat:feature/my-branch') }) }) + +/** + * The strict guards exist because a hardening change must never turn a failing + * request into a succeeding one. Before the guards, a padded identifier reached + * GitHub raw, matched nothing, and the request was a 404 no-op; trimming it + * would silently convert that into a real mutation. + */ +describe('strict guards refuse padding on state-changing requests', () => { + it.each([' acme ', 'acme ', ' acme', '\tacme', 'acme\n'])( + 'strictUrlPathSegment rejects %j', + (value) => { + expect(() => strictUrlPathSegment(value, 'owner')).toThrow( + /owner must not have leading or trailing whitespace/ + ) + } + ) + + it('strictEncodedUrlPathSegment rejects padding too', () => { + expect(() => strictEncodedUrlPathSegment(' area/api ', 'name')).toThrow( + /name must not have leading or trailing whitespace/ + ) + }) + + it.each([ + ['acme', 'acme'], + ['my-repo', 'my-repo'], + ['1234', '1234'], + ])('passes the unpadded value %j through unchanged', (value, expected) => { + expect(strictUrlPathSegment(value, 'owner')).toBe(expected) + }) + + it('keeps a namespaced label encoded as one segment', () => { + expect(strictEncodedUrlPathSegment('area/api', 'name')).toBe('area%2Fapi') + }) + + it('reports an all-whitespace value as missing, not as padded', () => { + expect(() => strictUrlPathSegment(' ', 'owner')).toThrow(/owner is required/) + }) + + it('accepts a number, which cannot be padded', () => { + expect(strictUrlPathSegment(1234, 'issue_number')).toBe('1234') + }) + + it('still rejects traversal, inheriting the safe guard', () => { + expect(() => strictUrlPathSegment('..', 'owner')).toThrow(/path traversal is not allowed/) + expect(() => strictUrlPathSegment('a/b', 'owner')).toThrow(/cannot contain a path separator/) + }) + + it('leaves the non-strict guard trimming, for reads and pre-trimmed ids', () => { + expect(safeUrlPathSegment(' acme ', 'owner')).toBe('acme') + }) +}) diff --git a/apps/sim/tools/url-path.ts b/apps/sim/tools/url-path.ts index d58c6f01171..e284d36333c 100644 --- a/apps/sim/tools/url-path.ts +++ b/apps/sim/tools/url-path.ts @@ -373,3 +373,87 @@ export function safeEncodedUrlPathSegment( return encodeSegment(trimmed, paramName) } + +/** + * Rejects a value whose text is surrounded by whitespace. + * + * Shared by the strict guards below. Only a string can be padded — a number or + * bigint has no surrounding text — and a value that is *entirely* whitespace is + * deliberately allowed through to the wrapped guard, so it reports the more + * accurate "is required" rather than complaining about padding on a value that + * has no content at all. + */ +function assertUnpadded(value: string | number | bigint, paramName: string): void { + if (typeof value !== 'string') return + + const trimmed = value.trim() + if (trimmed && trimmed !== value) { + throw new Error( + `${paramName} must not have leading or trailing whitespace (refusing to guess which resource was meant on a request that changes state)` + ) + } +} + +/** + * {@link safeUrlPathSegment} for an identifier on a request that **changes + * state** — a POST, PUT, PATCH, or DELETE. + * + * Identical in every respect except one: it refuses a padded value instead of + * trimming it. + * + * The reason is a rule about what a security fix is allowed to change. Before + * these guards existed, the GitHub tools interpolated identifiers raw, so a + * padded `owner` reached the provider as `%20%20acme%20%20`, matched no + * repository, and the request was a 404 no-op. Routing that same value through + * a *trimming* guard silently converts the no-op into a real mutation: + * + * ``` + * before: DELETE /repos/%20%20acme%20%20/sim/git/refs/heads/main -> 404, nothing happens + * after: DELETE /repos/acme/sim/git/refs/heads/main -> the branch is gone + * ``` + * + * Nothing about that is traversal, and every traversal test still passes, which + * is exactly why it would ship unnoticed. So the rule these strict guards + * encode is: **a hardening change must never turn a failing request into a + * succeeding one.** + * + * This applies only where the change *introduces* the trim. A parameter that + * already trimmed before the guards landed — the gist tools' `gist_id`, which + * read `params.gist_id?.trim()` — keeps trimming, because preserving its + * behaviour is the same rule, not an exception to it. + * + * Reads deliberately keep {@link safeUrlPathSegment}. Trimming a padded id on a + * GET is the copy-paste convenience that helper exists for, and its worst case + * is returning data the caller can simply ignore — not destroying a branch, + * closing someone's pull request, or filing an issue in a real repository. + * + * @param value - The raw identifier, typically LLM- or user-supplied. + * @param paramName - The parameter name, used to name the offender in errors. + * @returns The percent-encoded segment, safe to interpolate. + * @throws Everything {@link safeUrlPathSegment} throws, plus a padded value. + */ +export function strictUrlPathSegment(value: string | number | bigint, paramName: string): string { + assertUnpadded(value, paramName) + return safeUrlPathSegment(value, paramName) +} + +/** + * {@link safeEncodedUrlPathSegment} for a state-changing request. + * + * Same rule as {@link strictUrlPathSegment}; see that function for why. This + * variant exists because `remove_label` is a DELETE whose label `name` is one + * path parameter that may itself contain `/`, so it needs the encoding + * behaviour of `safeEncodedUrlPathSegment` and the padding refusal together. + * + * @param value - The raw value, typically LLM- or user-supplied. + * @param paramName - The parameter name, used to name the offender in errors. + * @returns The value percent-encoded as a single segment. + * @throws Everything {@link safeEncodedUrlPathSegment} throws, plus a padded value. + */ +export function strictEncodedUrlPathSegment( + value: string | number | bigint, + paramName: string +): string { + assertUnpadded(value, paramName) + return safeEncodedUrlPathSegment(value, paramName) +} From 0c5108ecdf3777ec06adc2c7cab2de0f3713d0a1 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 23:50:06 -0700 Subject: [PATCH 5/6] docs(github): record why the strict guards stop at writes The TSDoc stated the boundary and the harm on a write, but not why reads are deliberately excluded. Without that, the asymmetry reads as an unfinished pass and the next contributor "completes" it, breaking a paste flow that works today for no safety gain. Records both halves of the reasoning: the harm is asymmetric (a write mutates a resource the caller never named, unrecoverably and invisibly, since every traversal assertion still passes; a read returns data from the resource they almost certainly meant), and the cost of refusing runs the other way (a padded id on a read is overwhelmingly a stray newline in a paste). States the principle underneath both guards: refuse where being wrong is unrecoverable, tolerate where being wrong is merely unhelpful. Comment-only; no behaviour change. --- apps/sim/tools/url-path.ts | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/apps/sim/tools/url-path.ts b/apps/sim/tools/url-path.ts index e284d36333c..bc2fa15eb07 100644 --- a/apps/sim/tools/url-path.ts +++ b/apps/sim/tools/url-path.ts @@ -422,10 +422,27 @@ function assertUnpadded(value: string | number | bigint, paramName: string): voi * read `params.gist_id?.trim()` — keeps trimming, because preserving its * behaviour is the same rule, not an exception to it. * - * Reads deliberately keep {@link safeUrlPathSegment}. Trimming a padded id on a - * GET is the copy-paste convenience that helper exists for, and its worst case - * is returning data the caller can simply ignore — not destroying a branch, - * closing someone's pull request, or filing an issue in a real repository. + * **Reads deliberately keep {@link safeUrlPathSegment}, and that asymmetry is + * the point rather than an unfinished pass.** Do not "complete" it by routing + * GET routes through this guard — doing so breaks a flow that works today and + * buys no safety. The reasoning, since this is the first question the boundary + * invites: + * + * The rule above is "never turn a failing request into a succeeding one", but + * the *reason* the rule exists is that the harm is asymmetric. On a write, the + * failure mode is destroying or mutating a resource the caller never named — + * unrecoverable, and invisible in review because every traversal assertion + * still passes. On a read, the failure mode is returning data from the resource + * the caller almost certainly did mean, since they typed the padded name + * themselves; the worst case is data they ignore. + * + * The *cost* of refusing runs the other way. A padded identifier arriving at a + * read is overwhelmingly a paste carrying a stray newline, so rejecting it + * breaks a working flow for no gain. On a write, rejecting costs the caller one + * clear error message and saves a branch. + * + * So the principle underneath both guards is: **refuse where being wrong is + * unrecoverable, tolerate where being wrong is merely unhelpful.** * * @param value - The raw identifier, typically LLM- or user-supplied. * @param paramName - The parameter name, used to name the offender in errors. From ccbf0d2fa2b96d2e89a0327cda40ac9aa9d0dff3 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 29 Aug 2026 00:08:18 -0700 Subject: [PATCH 6/6] fix(github): reject a backslash in safeEncodedUrlPathSegment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was the only one of the three guards that encoded a backslash to %5C instead of refusing it. Encoding is safe on the wire — a raw backslash IS a path separator to the WHATWG parser for a special scheme, so https://x/a/b/..\..\etc resolves to /etc, but the encoded form does not move: new URL('https://x/a/b/..%5C..%5Cetc').pathname => /a/b/..%5C..%5Cetc So this is not a live traversal hole. It is refused anyway for the reason the module already gives for safeUrlPath: a value carrying a backslash is a Windows-shaped path the caller did not mean to address literally, and letting one through leaves a segment that reads as traversal to anything downstream that normalizes it. Neither caller — a GitHub label name, a git ref — can legitimately contain one, so the consistency costs nothing. Pinned, including an assertion that all three guards agree. --- apps/sim/tools/url-path.test.ts | 26 ++++++++++++++++++++++++++ apps/sim/tools/url-path.ts | 23 ++++++++++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/apps/sim/tools/url-path.test.ts b/apps/sim/tools/url-path.test.ts index 1ef8d54e811..df9010e96d0 100644 --- a/apps/sim/tools/url-path.test.ts +++ b/apps/sim/tools/url-path.test.ts @@ -3,6 +3,7 @@ */ import { describe, expect, it } from 'vitest' import { + safeEncodedUrlPathSegment, safeUrlPath, safeUrlPathSegment, strictEncodedUrlPathSegment, @@ -457,6 +458,31 @@ describe('strict guards refuse padding on state-changing requests', () => { expect(strictUrlPathSegment(value, 'owner')).toBe(expected) }) + it.each(['a\\b', '..\\..', 'area\\api', '\\'])( + 'safeEncodedUrlPathSegment rejects the backslash in %j', + (value) => { + expect(() => safeEncodedUrlPathSegment(value, 'name')).toThrow( + /name cannot contain a backslash/ + ) + } + ) + + it('rejects a backslash through the strict wrapper too', () => { + expect(() => strictEncodedUrlPathSegment('a\\b', 'name')).toThrow( + /name cannot contain a backslash/ + ) + }) + + it('all three helpers agree that a backslash is refused, not encoded', () => { + for (const [label, fn] of [ + ['safeUrlPathSegment', safeUrlPathSegment], + ['safeUrlPath', safeUrlPath], + ['safeEncodedUrlPathSegment', safeEncodedUrlPathSegment], + ] as const) { + expect(() => fn('a\\b', label)).toThrow() + } + }) + it('keeps a namespaced label encoded as one segment', () => { expect(strictEncodedUrlPathSegment('area/api', 'name')).toBe('area%2Fapi') }) diff --git a/apps/sim/tools/url-path.ts b/apps/sim/tools/url-path.ts index bc2fa15eb07..7bd65deaf5f 100644 --- a/apps/sim/tools/url-path.ts +++ b/apps/sim/tools/url-path.ts @@ -347,6 +347,23 @@ export function safeUrlPath(value: string | number | bigint, paramName: string): * which is why that case is still rejected here rather than encoded, exactly as * the module note requires. * + * A backslash is rejected rather than encoded, matching both sibling helpers. + * Encoding it to `%5C` would in fact be safe on the wire — a raw `\` *is* a + * path separator to the WHATWG parser for a special scheme, so + * `https://x/a/b/..\..\etc` resolves to `/etc`, but the encoded form does not + * move at all: + * + * ``` + * new URL('https://x/a/b/..%5C..%5Cetc').pathname // => '/a/b/..%5C..%5Cetc' + * ``` + * + * It is refused anyway, for the reason the module note gives for `safeUrlPath`: + * a value carrying a backslash is a Windows-shaped path the caller did not mean + * to address literally, and letting one through would leave a segment that + * reads as traversal to any consumer downstream that normalizes it. Neither + * caller — a GitHub label name, a git ref — can legitimately contain one, so + * the consistency is free. + * * Prefer `safeUrlPathSegment`. Reach for this helper only when the provider * documents the parameter as a single value that may itself contain `/`. * @@ -355,7 +372,7 @@ export function safeUrlPath(value: string | number | bigint, paramName: string): * @returns The trimmed value percent-encoded as a single segment, separators * included, safe to interpolate. * @throws If the value is not a string or a usable number, is empty, is a dot - * segment, or cannot be encoded. + * segment, contains a backslash, or cannot be encoded. */ export function safeEncodedUrlPathSegment( value: string | number | bigint, @@ -371,6 +388,10 @@ export function safeEncodedUrlPathSegment( throw new Error(`${paramName} cannot be "${trimmed}" (path traversal is not allowed)`) } + if (trimmed.includes('\\')) { + throw new Error(`${paramName} cannot contain a backslash`) + } + return encodeSegment(trimmed, paramName) }