Skip to content

Commit d2c74d7

Browse files
waleedlatif1waleedlatif1
committed
fix(github): refuse a padded identifier on state-changing requests
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.
1 parent 515b951 commit d2c74d7

41 files changed

Lines changed: 358 additions & 77 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎apps/sim/lib/internal/github/operations.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,11 @@ import type {
2020
} from '@/tools/github/types'
2121
import { secureGitHubRequest } from '@/tools/github/utils.server'
2222
import type { ToolResponse } from '@/tools/types'
23-
import { safeEncodedUrlPathSegment, safeUrlPathSegment } from '@/tools/url-path'
23+
import {
24+
safeEncodedUrlPathSegment,
25+
safeUrlPathSegment,
26+
strictUrlPathSegment,
27+
} from '@/tools/url-path'
2428

2529
const logger = createLogger('GitHubLatestCommitOperation')
2630
const MAX_COMMIT_RESPONSE_BYTES = 10 * 1024 * 1024
@@ -119,10 +123,19 @@ function buildGuardedUrl(build: () => string): string {
119123
}
120124
}
121125

126+
/**
127+
* The pull-request URL, and the base for the comment and review URLs built from
128+
* it.
129+
*
130+
* Uses the strict guards even though this same URL is also fetched with a GET
131+
* to read the head SHA: every caller reaches it on the way to creating a
132+
* comment or a review, so the operation as a whole changes state and must not
133+
* have a padded identifier quietly resolved to a real pull request.
134+
*/
122135
function pullRequestUrl(params: CreateCommentParams): string {
123136
return buildGuardedUrl(
124137
() =>
125-
`${GITHUB_API_BASE}/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}`
138+
`${GITHUB_API_BASE}/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}`
126139
)
127140
}
128141

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

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

55
export const addAssigneesTool: ToolConfig<AddAssigneesParams, IssueResponse> = {
66
id: 'github_add_assignees',
@@ -43,7 +43,7 @@ export const addAssigneesTool: ToolConfig<AddAssigneesParams, IssueResponse> = {
4343

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

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

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

55
export const addLabelsTool: ToolConfig<AddLabelsParams, LabelsResponse> = {
66
id: 'github_add_labels',
@@ -43,7 +43,7 @@ export const addLabelsTool: ToolConfig<AddLabelsParams, LabelsResponse> = {
4343

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

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

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

55
export const cancelWorkflowRunTool: ToolConfig<CancelWorkflowRunParams, CancelWorkflowRunResponse> =
66
{
@@ -39,7 +39,7 @@ export const cancelWorkflowRunTool: ToolConfig<CancelWorkflowRunParams, CancelWo
3939

4040
request: {
4141
url: (params) =>
42-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/actions/runs/${safeUrlPathSegment(params.run_id, 'run_id')}/cancel`,
42+
`https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/actions/runs/${strictUrlPathSegment(params.run_id, 'run_id')}/cancel`,
4343
method: 'POST',
4444
headers: (params) => ({
4545
Accept: 'application/vnd.github+json',

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { CloseIssueParams, IssueResponse } from '@/tools/github/types'
22
import { ISSUE_OUTPUT_PROPERTIES, LABEL_OUTPUT, USER_OUTPUT } from '@/tools/github/types'
33
import type { ToolConfig } from '@/tools/types'
4-
import { safeUrlPathSegment } from '@/tools/url-path'
4+
import { strictUrlPathSegment } from '@/tools/url-path'
55

66
export const closeIssueTool: ToolConfig<CloseIssueParams, IssueResponse> = {
77
id: 'github_close_issue',
@@ -44,7 +44,7 @@ export const closeIssueTool: ToolConfig<CloseIssueParams, IssueResponse> = {
4444

4545
request: {
4646
url: (params) =>
47-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/${safeUrlPathSegment(params.issue_number, 'issue_number')}`,
47+
`https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/${strictUrlPathSegment(params.issue_number, 'issue_number')}`,
4848
method: 'PATCH',
4949
headers: (params) => ({
5050
Accept: 'application/vnd.github.v3+json',

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

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

55
export const closePRTool: ToolConfig<ClosePRParams, PRResponse> = {
66
id: 'github_close_pr',
@@ -37,7 +37,7 @@ export const closePRTool: ToolConfig<ClosePRParams, PRResponse> = {
3737

3838
request: {
3939
url: (params) =>
40-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/pulls/${safeUrlPathSegment(params.pullNumber, 'pullNumber')}`,
40+
`https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/pulls/${strictUrlPathSegment(params.pullNumber, 'pullNumber')}`,
4141
method: 'PATCH',
4242
headers: (params) => ({
4343
Accept: 'application/vnd.github.v3+json',

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { CreateBranchParams, RefResponse } from '@/tools/github/types'
22
import { GIT_REF_OUTPUT_PROPERTIES } from '@/tools/github/types'
33
import type { ToolConfig } from '@/tools/types'
4-
import { safeUrlPathSegment } from '@/tools/url-path'
4+
import { strictUrlPathSegment } from '@/tools/url-path'
55

66
export const createBranchTool: ToolConfig<CreateBranchParams, RefResponse> = {
77
id: 'github_create_branch',
@@ -45,7 +45,7 @@ export const createBranchTool: ToolConfig<CreateBranchParams, RefResponse> = {
4545

4646
request: {
4747
url: (params) =>
48-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/git/refs`,
48+
`https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/git/refs`,
4949
method: 'POST',
5050
headers: (params) => ({
5151
Accept: 'application/vnd.github+json',

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

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

55
interface CreateCommentReactionParams {
66
owner: string
@@ -68,7 +68,7 @@ export const createCommentReactionTool: ToolConfig<
6868

6969
request: {
7070
url: (params) =>
71-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues/comments/${safeUrlPathSegment(params.comment_id, 'comment_id')}/reactions`,
71+
`https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues/comments/${strictUrlPathSegment(params.comment_id, 'comment_id')}/reactions`,
7272
method: 'POST',
7373
headers: (params) => ({
7474
Accept: 'application/vnd.github.squirrel-girl-preview+json',

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

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

55
export const createFileTool: ToolConfig<CreateFileParams, FileOperationResponse> = {
66
id: 'github_create_file',
@@ -56,7 +56,7 @@ export const createFileTool: ToolConfig<CreateFileParams, FileOperationResponse>
5656

5757
request: {
5858
url: (params) =>
59-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`,
59+
`https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/contents/${safeUrlPath(params.path, 'path')}`,
6060
method: 'PUT',
6161
headers: (params) => ({
6262
Accept: 'application/vnd.github+json',

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
USER_OUTPUT,
77
} from '@/tools/github/types'
88
import type { ToolConfig } from '@/tools/types'
9-
import { safeUrlPathSegment } from '@/tools/url-path'
9+
import { strictUrlPathSegment } from '@/tools/url-path'
1010

1111
export const createIssueTool: ToolConfig<CreateIssueParams, IssueResponse> = {
1212
id: 'github_create_issue',
@@ -67,7 +67,7 @@ export const createIssueTool: ToolConfig<CreateIssueParams, IssueResponse> = {
6767

6868
request: {
6969
url: (params) =>
70-
`https://api.github.com/repos/${safeUrlPathSegment(params.owner, 'owner')}/${safeUrlPathSegment(params.repo, 'repo')}/issues`,
70+
`https://api.github.com/repos/${strictUrlPathSegment(params.owner, 'owner')}/${strictUrlPathSegment(params.repo, 'repo')}/issues`,
7171
method: 'POST',
7272
headers: (params) => ({
7373
Accept: 'application/vnd.github.v3+json',

0 commit comments

Comments
 (0)