fix(github): reject path-traversal values in interpolated URL segments

[waleedlatif1]

The defect

GitHub tools interpolate LLM-writable values — owner, repo, issue_number, pullNumber, path, branch, ref, sha, label name, gist_id, run_id, comment_id, … — straight into the request path. These are visibility: 'user-or-llm', so prompt injection controls them.

A value of .. re-aims an authenticated request, carrying the workspace's GitHub token, at a different resource:

/repos/../../repos/victim/private/contents/x  ->  /repos/victim/private/contents/x

That includes DELETE routes: delete_file, delete_release, delete_branch, delete_comment, delete_milestone, delete_gist. assertRequestUrlMatchesTrust in tools/request-transport.ts only canonicalizes internal /api/ routes, so nothing downstream catches it.

Why encodeURIComponent is not sufficient

This is the question reviewers will ask, so, concretely:

new URL('https://api.github.com/repos/' + encodeURIComponent('..') + '/x/issues').pathname
// => '/x/issues'          the prefix is gone

new URL('https://api.github.com/repos/%2e%2e/x/issues').pathname
// => '/x/issues'          double-encoding does not help either

. and .. are unreserved, so encodeURIComponent('..') returns '..' verbatim. And the WHATWG URL parser that fetch uses removes dot segments after decoding, so the percent-encoded spellings are popped too. No encoding scheme neutralizes a dot segment — only rejecting the value does.

The fix

Every interpolation site now goes through apps/sim/tools/url-path.ts. GitHub has three genuinely different parameter shapes, so this PR adds two helpers alongside the existing safeUrlPathSegment:

helper	for	example
safeUrlPathSegment (existing)	a single path segment	owner, repo, issue_number, release_id
safeUrlPath (new)	a value whose / is real path structure	path, branch, ref, base, head
safeEncodedUrlPathSegment (new)	one path parameter that may itself contain /	label name (area/api must stay %2F)

Applying the single-segment guard to path would reject docs/README.md; promoting label name to a multi-segment path would emit a real / and address a different endpoint. safeUrlPath enforces the traversal rule per segment and restores : after encoding, so compare keeps addressing a cross-fork ref as octocat:feature/my-branch exactly as before. Each split is justified in TSDoc.

Counts: 183 single-segment sites, 12 multi-segment, 2 encoded-single.

Sites deliberately left alone: the GraphQL tools (create_project, list_projects, graphql, review threads, status_check_rollup) post to a fixed https://api.github.com/graphql, and the search_* tools build their URL with URLSearchParams — no value reaches a path segment in either. Human-readable content strings and html_url display fields were left raw so no output text changes.

Two adjacent hardenings in the same files: get_tree / get_file_content interpolated ?ref=${params.ref} into the query without encoding (query injection, not traversal), now encodeURIComponent, matching get_readme.

The test

apps/sim/tools/github/path_safety.test.ts enumerates tools from the barrel and probes every declared parameter to discover which ones reach the path, so a newly added tool with an unguarded parameter fails CI rather than needing to be remembered. Every assertion resolves the built URL with new URL(...) — the same normalization fetch performs — rather than string-matching the template, because string matching is exactly what let this through. The vector list keeps the bare . and .. entries, and LEGITIMATE_IDS / LEGITIMATE_PATHS prove real values (octocat, my-repo, a 40-char sha, v1.2.3, feature/my-branch, docs/README.md, heads/release/2.0) still pass through unchanged.

Verified test-first: the suite was written before any guard and went red against the unmodified tools; reverting one guard on delete_file afterwards turns it red again.

No param visibility, subBlock id, or tool behaviour for legitimate input changed — tool-metadata:check and check-block-registry confirm.

One existing expectation in job_logs.test.ts was tightened: it asserted that owner: '../../orgs/secret' was escaped through as ..%2F..%2Forgs%2Fsecret. That is safe, but the guard now rejects a separator in owner outright, so the test asserts the rejection instead.

Padded identifiers on state-changing requests

owner, repo and the numeric ids were interpolated raw before this branch, so routing them through a trimming guard would have converted a 404 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

No traversal assertion catches that, which is why it needed a sweep rather than inspection. strictUrlPathSegment / strictEncodedUrlPathSegment refuse a padded value instead of trimming it, and are applied to every parameter this branch newly trims on a non-GET route — 37 tools, 101 parameter sites, plus pullRequestUrl in lib/internal/github/operations.ts.

The rule is that a hardening change must never turn a failing request into a succeeding one. So:

• Reads keep the trimming guards; 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.
• safeUrlPath parameters (path, branch, ref, base, head) never trimmed, so they are unaffected.

On the duplicate helper: strictUrlPathSegment is defined locally in url-path.ts rather than imported from #7269, which introduces a helper of the same name. That is deliberate. #7269 is rebased onto this branch, so this one merges first, and waiting for the shared helper would mean shipping the regression and fixing it afterwards. When the two meet, this collapses to a one-line import change.

Scope

This is one service of a larger sweep — the same pattern was found at 199 interpolation sites across the codebase, and other services ship separately.

Gates: bun run lint, bun run check:audits (39/39), check-block-registry, and vitest run tools/github lib/internal/github tools/url-path.test.ts (11929 passed) are all green.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Aug 29, 2026 7:08am

[greptile-apps]

Greptile Summary

The PR centralizes GitHub URL-path interpolation behind traversal-safe helpers and distinguishes opaque identifiers, multi-segment repository paths, and encoded single-segment values.

• Applies strict non-trimming guards to identifiers on state-changing requests.
• Preserves legitimate whitespace and slash structure in repository paths while rejecting dot segments.
• Adds discovery-based coverage for GitHub URL builders and maps internal-operation validation failures to client errors.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
apps/sim/tools/url-path.ts	Defines the path-safety helpers, preserves repository-path whitespace, and rejects traversal or padded mutation identifiers as intended.
apps/sim/tools/github/path_safety.test.ts	Enumerates GitHub URL builders, accounts for skipped and pathless tools, and verifies traversal, whitespace, and strict mutation behavior.
apps/sim/lib/internal/github/operations.ts	Guards internal GitHub operation URLs and converts caller-supplied path validation failures into actionable client errors.
apps/sim/tools/url-path.test.ts	Covers whitespace preservation, strict identifier rejection, dot-segment handling, backslashes, and encoded labels.

_{Reviews (6): Last reviewed commit: "fix(github): reject a backslash in safeE..." | Re-trigger Greptile}

[cubic-dev-ai]

3 issues found across 79 files

Confidence score: 3/5

• apps/sim/lib/internal/github/operations.ts lets rejected latest-commit paths escape as a plain Error, causing executeGitHubTool to return 500 instead of a client-facing 400; convert validation failures to GitHubOperationError with status 400.
• apps/sim/tools/github/update_file.ts trims leading or trailing whitespace from valid filename segments, which can update the wrong file or fail to find the target; preserve segment whitespace when constructing safeUrlPath.
• apps/sim/tools/github/path_safety.test.ts can silently omit parameters when baseline construction fails, allowing aggregate discovery to report incomplete coverage; surface the tool and parameter or assert that nothing was skipped.

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/sim/tools/github/update_file.ts">

<violation number="1" location="apps/sim/tools/github/update_file.ts:65">
P2: When `path` contains a legal filename segment with leading or trailing whitespace, `safeUrlPath` trims it before building the request, so the update targets a different file or fails. Preserve segment whitespace while still rejecting raw dot segments.</violation>
</file>

<file name="apps/sim/lib/internal/github/operations.ts">

<violation number="1" location="apps/sim/lib/internal/github/operations.ts:356">
P2: When a latest-commit input contains a rejected path value, this guard throws a plain `Error`, and `executeGitHubTool` returns 500 for it. Convert path-validation failures to `GitHubOperationError` with status 400 so rejected user input is not reported as a server failure.</violation>
</file>

<file name="apps/sim/tools/github/path_safety.test.ts">

<violation number="1" location="apps/sim/tools/github/path_safety.test.ts:171">
P2: Do not silently skip baseline-construction failures during discovery. Surface the tool and parameter that failed, or otherwise assert that no parameters were skipped, so the aggregate count cannot hide missing traversal coverage.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic</sub>

[waleedlatif1]

@greptile All six threads are addressed, replied to individually, and resolved — pushed in 7d4d2e1. Re-review please.

Summary of what changed:

• P1 url-path.ts:250 (segment trimming) — real, and the most important finding here. Reproduced it first: git tracks d/my file .txt and d/ lead.txt verbatim, so trimming made update_file/delete_file act on a different existing file. Rather than delete trimming outright, the behaviour is now split by purpose: safeUrlPathSegment keeps trimming (opaque copy-pasted ids; ~690 call sites depend on it), safeUrlPath trims nowhere and preserves whitespace byte-for-byte. The dot-segment check is unweakened — the URL parser removes %2e%2e but leaves %20..%20 inert. Split explained in TSDoc on both helpers and pinned in both directions.
• P2 discovery skipping — enabling the assertion found a real hole: github_job_logs had silently fallen out of the suite entirely, because a string filler in the sibling job_id parameter aborted the build before owner/repo could be probed. Fixed, and skips are now an explicit asserted ledger.
• P2 ToolConfig<any, any> — replaced with a structural interface plus an unknown type guard. One documented cast remains at the single narrowing point, because the harness intentionally feeds values the tools' own param types forbid, which is the test's whole point.
• cubic P2 operations.ts 500→400 — valid; path-guard failures now raise GitHubOperationError(400). Applied to pullRequestUrl as well, which had the same problem and was not flagged.

Every new pin was verified to fail against the pre-fix code before being committed. Gates: lint, check:audits 39/39, check-block-registry, type-check, and the full GitHub suite (12080 passing) are green.

[waleedlatif1]

Carrying over a review finding from #7269, which is rebased onto this branch. cubic raised it against url-path.ts there, but it is this PR's code, so it belongs here rather than riding in on the rebase.

P2: safeEncodedUrlPathSegment accepts backslashes that the path guards otherwise reject, allowing an encoded Windows-shaped traversal value to reach downstream URL handling. Reject backslashes before encoding, as safeUrlPath does.

My assessment: not a security issue, but a reasonable consistency question.

The backslash is inert here. I checked rather than assuming:

safeEncodedUrlPathSegment(...., name)  ->  %5C..%5C..
new URL(https://api.github.com/repos/o/r/issues/1/labels/ + that).pathname
  ->  /repos/o/r/issues/1/labels/%5C..%5C..

The value survives as one segment. The WHATWG parser removes a dot segment only when the segment is exactly . or .. (or their %2e spellings), and it does not decode %5C first, so %5C..%5C.. never becomes a path boundary. Same reasoning by which ..%2f..%2f is inert: encoding a separator is sufficient, and only the bare dot segment resists encoding — which this helper already rejects.

The asymmetry with safeUrlPath also looks deliberate to me. There, / is structure, so a backslash means a pasted Windows path that would silently address the wrong resource. safeEncodedUrlPathSegment encodes the whole value as one opaque segment, separators included, so there is no structural ambiguity — and a GitHub label literally named a\b is legal, so rejecting would break a valid value to prevent something that cannot happen.

Where cubic has a fair point: the two helpers read inconsistently side by side, and "rejects backslashes" is easier to reason about than "encodes them, and here is why that is safe." If you prefer uniform rejection for reviewability, the only cost is the a\b label case. Your call — flagging it so the decision is made here rather than lost.

[waleedlatif1]

Two more review findings from cubic against #7269 (rebased onto this branch), both in this PR's files. Routing them here rather than fixing them on the downstream PR.

---

1. safeEncodedUrlPathSegment backslash — cubic escalated this to P1

I raised this above as P2 and argued it was inert. cubic re-raised it at P1 with a different argument than the one I answered, and I now think it should just be fixed.

My original analysis still stands for the URL parser: safeEncodedUrlPathSegment('\..\..') yields %5C..%5C.., and the WHATWG parser does not decode %5C before dot-segment removal, so it never becomes a boundary. What I did not address is cubic's actual claim — "downstream path normalizers". Intermediaries are the gap: IIS-style normalization folds \ to /, and some proxies and WAFs do the same before the origin sees the path. %5C..%5C.. decoded and folded becomes /../... That is not the URL parser's behaviour, so my earlier check did not test for it and could not have.

The cost of rejecting is one exotic case — a GitHub label literally containing a backslash — against a defense-in-depth hole in a guard whose entire purpose is to be the last word on traversal. I'd take the rejection, matching safeUrlPath:

if (trimmed.includes('\\')) {
  throw new Error(`${paramName} cannot contain a backslash`)
}

That also removes the reviewability wart of two guards in one module disagreeing about backslashes, which is what drew the finding twice.

---

2. remove_label.ts — response is built from raw params while the request is guarded

Confirmed by reading it; this one is straightforwardly right:

// request path: guarded/trimmed
// transformResponse:
content:  `Label "${params.name}" removed from issue #${params.issue_number}`
html_url: `https://github.com/${params.owner}/${params.repo}/issues/${params.issue_number}`

With owner: ' octocat ' the DELETE correctly hits /repos/octocat/..., but the returned html_url is https://github.com/ octocat /repo/issues/1 — a broken link, and content echoes an untrimmed label name. The operation succeeds and reports itself with values that do not match what it did.

Worth a sweep rather than a one-line fix: any transformResponse in this PR that rebuilds a URL or confirmation string from raw params has the same split. Deriving the response from the same canonical values as the request is the fix.

---

For reference, #7269 hit the same class of bug and the shape of the fix there may be useful: safeUrlPathSegment accepts a numeric id (an LLM can send one as a JSON number), so a .trim() elsewhere in the tool threw a raw TypeError after the guard had passed. Rather than restate the normalization I derived the second use from the guard itself — decodeURIComponent(safeUrlPathSegment(value, name)) — so there is one rule instead of two that can drift.

[waleedlatif1]

Third finding from cubic against #7269, again in this PR's code. This one I think is a genuine bug that defeats a guard this PR just added, so worth prioritising.

executeGitHubTool does not await the comment operations, so buildGuardedUrl's 400 never fires

apps/sim/lib/internal/github/execute-tool.ts:

switch (request.toolId) {
  case 'github_comment':
    return executeToolOperationImplementation(executeGitHubCommentOperation, request)   // not awaited
  case 'github_comment_v2':
    return executeToolOperationImplementation(executeGitHubCommentV2Operation, request) // not awaited
  case 'github_latest_commit': {
    ...
    return Response.json(await getGitHubLatestCommit(...))                              // awaited
  }
}

return promise inside try completes the try block before the promise settles, so the enclosing catch never observes a rejection — the classic missing return await. The github_latest_commit branch already awaits; the two comment branches do not.

That matters specifically because of the mapping this PR introduces:

function buildGuardedUrl(build: () => string): string {
  try { return build() }
  catch (error) { throw new GitHubOperationError(getErrorMessage(error, 'Invalid GitHub request path'), 400) }
}

buildGuardedUrl converts a guard rejection into GitHubOperationError(…, 400) precisely so executeGitHubTool's catch can report it as a client error with the named "<param> cannot be …" message. For github_comment and github_comment_v2 that catch is unreachable, so an owner of .. does not produce the documented 400 — it escapes as an unhandled rejection or surfaces as a generic 500, which is the exact misattribution the TSDoc on buildGuardedUrl says it exists to prevent.

Fix is one keyword on each of the two branches:

return await executeToolOperationImplementation(executeGitHubCommentOperation, request)

Worth a test that a rejected guard value returns 400 rather than 500 through executeGitHubTool, since the current operations.test.ts exercises the guards directly and would not have caught this.

---

That is all three of cubic's findings against this PR's files now routed here: the safeEncodedUrlPathSegment backslash (escalated to P1, and I reversed my earlier position in favour of adding the rejection), remove_label.ts building its response from raw params, and this one. #7269 has none of these files in its diff; all its own findings are fixed and it is green.

[waleedlatif1]

Correction pushed in 515b951, from a finding cubic raised on #7269.

safeUrlPath no longer rejects a whitespace-only path component. I had it reject any segment that was empty after trimming. That check was wrong on both counts, and I verified both rather than taking it on faith.

Git permits it — a file and a directory whose entire name is spaces:

$ git ls-files | sed -n 'l'
d/   $
e/   /f.txt$

And rejecting it bought nothing. 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)

So the guard had no security value and a real cost: update_file threw before the PUT and a legitimate file became unreachable.

Now only a truly empty component is rejected — a //, where the caller wrote no name at all. ./.., backslashes, and every other guard are unchanged.

safeUrlPathSegment is untouched and still rejects an all-whitespace value. That asymmetry is deliberate, not an oversight: it trims opaque ids first, so one made only of spaces really has named nothing. Both halves are pinned, and the TSDoc now records why the check is absent — citing the git paths and the parser behaviour — so it is not reinstated later on aesthetic grounds.

This is the same error as the trimming bug one level down: reasoning about what a path segment ought to look like instead of what the filesystem and the URL parser actually do. Worth stating plainly since this helper is now shared.

New pins: docs/ /file.txt, a bare , e/ /f.txt, and d/ round-trip byte-for-byte, plus docs//file.txt still throws. I confirmed 29 tests go red if I restore the old check.

---

Dependency note for reviewers: #7269 branches off this PR and will need a rebase once this lands — it carries the older safeUrlPath and its own copy of this fix will conflict.

Gates re-run green: lint, check:audits 39/39, check-block-registry, type-check, and the full GitHub suite (12110 passing).

[waleedlatif1]

@greptile review

[waleedlatif1]

@cubic review

[cubic-dev-ai]

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

[waleedlatif1]

Fourth finding from cubic against #7269, again in this PR's code — and this one is a consequence of the whitespace fix itself, so worth deciding deliberately.

safeUrlPath rejects a whitespace-only path component

if (!segment.trim()) {
  throw new Error(`${paramName} cannot contain an empty or whitespace-only path segment`)
}

cubic's case: apps/sim/tools/github/update_file.ts, a path whose component is made only of spaces — docs/ /notes.md — now throws, so a valid GitHub file cannot be updated.

I think this is right, and it is the one inconsistency left in an otherwise coherent rule. The whitespace fix on this PR deliberately made the helper treat whitespace as data: it encodes the raw segment so docs/ draft.md is not silently rewritten to docs/draft.md, on the grounds that a leading or trailing space is a legal filename character git stores verbatim. A component that is entirely spaces is the same kind of value by the same argument — legal, storable, and the caller's own — yet it is now the single shape the helper refuses.

Suggested change, keeping the two rules distinct:

if (segment === '') {
  throw new Error(`${paramName} cannot contain an empty path segment`)
}

The important caveat: do not collapse this into "allow anything non-empty after trimming". The empty-segment rule exists for a different reason than the traversal rule — a genuinely empty component (a//b, a leading or trailing /) addresses a different object than the caller wrote, which is why #7269 adopted the rejection for Supabase storage keys. "a/ /b" and "a//b" must stay distinguishable; only the first should become legal.

I flagged this deviation when consolidating #7269 onto safeUrlPath and said then that if whitespace-only components should be preserved, the change belongs here rather than forked downstream. cubic has now produced the concrete GitHub case, so it is yours to take.

---

Running list of findings routed to this PR from #7269, all verified there:

1. safeEncodedUrlPathSegment accepts backslashes — escalated to P1; I reversed my earlier "inert" position, since the risk is downstream normalizers folding \ to /, not the WHATWG parser.
2. remove_label.ts builds html_url and content from raw params while the request path is guarded — a successful removal returns a dead link. Worth a sweep of every transformResponse that rebuilds a URL from raw params.
3. executeGitHubTool does not await the comment operations, so this PR's own buildGuardedUrl 400-mapping is unreachable for github_comment — a rejected owner surfaces as 500 rather than the documented 400.
4. This one.

For what it is worth, #7269 hit the same class as (3) on its own side — encodeStoragePath throwing into a generic 500 handler — and I fixed it there by mapping guard rejections to 400 at the call sites, with tests asserting no provider request is made.

[cubic-dev-ai]

No issues found across 81 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

_{Re-trigger cubic}

[waleedlatif1]

Fifth finding routed from #7269 — and this one is the data-loss class that #7269 just had to fix twice, so flagging it with the pattern rather than just the instance.

safeEncodedUrlPathSegment trims, so a padded label name now removes a different label

safeEncodedUrlPathSegment does toGuardedString(value, paramName).trim(). GitHub label names were previously interpolated without a trim, so:

before: DELETE /repos/o/r/issues/1/labels/%20%20bug%20%20   -> 404, no-op
after:  DELETE /repos/o/r/issues/1/labels/bug               -> removes the real "bug" label

Two independent reasons this is wrong here:

1. A label name can legitimately contain surrounding whitespace. Unlike a UUID or a GCP project id, " bug " is a nameable GitHub label, so trimming addresses a different one. cubic's framing — preserve the untrimmed name, reject only the dot-segment spellings, percent-encode the rest — is right on plain correctness grounds.
2. It converts a failing request into a succeeding destructive one. Pre-PR the padded name matched nothing and the DELETE was a no-op.

The pattern, because it generalises past this one call site

#7269 hit reason (2) twice, and both were invisible until specifically looked for:

• BigQuery projectId on delete_dataset / delete_table: previously encodeURIComponent(params.projectId), never trimmed. " my-project " named no project and 404'd; trimming resolved it to the real project and irreversibly deleted a dataset or table. datasetId on the same tools was already trimmed, so projectId was the one identifier out of step — the guard hid its own hazard.
• Box Sign signRequestId on cancel_request: interpolated raw, so a padded id 404'd; trimming cancels a real signature request.

The rule that resolved both: guarding a path must not turn a failing request into a succeeding one. Operationally — for each parameter, ask whether this PR newly trims it, and whether it sits on an irreversible request. Refuse padding on the newly-trimmed ones; leave the already-trimmed ones alone, since refusing those breaks callers whose stored value works today.

Worth running that question across every parameter this PR guards, not just remove_label. Any identifier that was previously raw or bare-encodeURIComponent and now flows through safeUrlPathSegment or safeEncodedUrlPathSegment is in scope; the DELETE routes (delete_file, delete_release, delete_branch) are where it costs the most.

Label removal is reversible, so this is lower priority than the two above — but it is the same mechanism, and the sweep is the part I would not skip.

[waleedlatif1]

Three more from cubic against #7269, all in this PR's files — plus one that applies to your guards but was found on mine.

1. close_pr.ts — the data-loss class again, on owner/repo (P2)

When owner or repo has surrounding whitespace, this helper trims it and closes the unpadded repository's PR; before this change, GitHub received the padded path and returned 404.

This is the third instance of the pattern I described in my previous comment, and cubic is now explicitly recommending the fix #7269 landed. Pre-PR a padded owner produced %20%20octocat%20%20, matched no repository, and the PATCH was a no-op. safeUrlPathSegment trims, so it now closes a PR in the real repository.

strictUrlPathSegment in apps/sim/tools/strict-url-path.ts (on #7269, so it will exist once that merges) does exactly this — refuse padding on identifiers the change newly trims, leave already-trimmed ones alone.

2. get_branch_protection.ts — branch with / sent as extra path segments (P2)

safeUrlPath keeps / as structure, but branch protection takes the branch as one path parameter, so feature/foo becomes /branches/feature/foo/protection and 404s. This is what safeEncodedUrlPathSegment is for — worth auditing every branch/ref site for the same mix-up, since the two helpers are easy to swap by accident and the failure is a silent 404 rather than an error.

3. get_tree.ts — falsy 0 path skips the guard (P3)

params.path ? safeUrlPath(...) : treats the numeric 0 as absent, so a directory named 0 cannot be listed and malformed input silently becomes a root request. Needs an explicit "omitted or empty string" check rather than a truthiness test.

---

Also worth carrying over: guard errors echo the rejected value

cubic raised this on #7269 against my strictUrlPathSegment, and it applies to the guards here too — safeUrlPathSegment, safeUrlPath and safeEncodedUrlPathSegment all quote the offending value:

throw new Error(`${paramName} cannot be "${trimmed}" (path traversal is not allowed)`)

These parameters are visibility: 'user-or-llm' and buildGuardedUrl maps the throw into a tool result the model reads. So the message copies attacker-chosen text back into the prompt — from a value that arrived precisely because it was hostile enough to be rejected. U+2028/U+2029 is the concrete edge: they terminate a line for some parsers, so a rejected id can break out of whatever framing wraps the error.

I dropped the value from #7269's guard and kept only the parameter name, which is the actionable part since the caller already knows what it sent. Pinned with a payload shaped like the risk (' ignore previous instructions ') rather than a neutral string. Same change looks right here.

---

Running total routed from #7269: safeEncodedUrlPathSegment backslashes (P1), remove_label.ts raw-params response, executeGitHubTool missing await, safeUrlPath rejecting whitespace-only components, remove_label.ts trimming label names, and now these three plus the error-echo. Happy to take any of them as a follow-up PR against this branch if that is easier than folding them in.

[waleedlatif1]

Worked through all four. One I'm pushing back on, one I'm correcting the scope of, one is pre-existing, one isn't a defect. No code changes — this branch stays at 5/5.

---

Error-echo: does not apply to these guards

I verified this rather than taking it on faith, and I agree it does not apply here. The right question isn't "does the guard echo the value?" but "can the echoed value carry attacker-controlled text?" Every echoing site in url-path.ts is constrained to a value that cannot:

site	interpolation	why it's inert
:187 safeUrlPathSegment	cannot be "${trimmed}"	only reachable under trimmed === '.' || trimmed === '..' — one of two constants
:321 safeUrlPath	cannot contain a "${segment}"	same guard, same two constants
:371 safeEncodedUrlPathSegment	cannot be "${trimmed}"	same guard, same two constants
:125 toGuardedString	but ${stringified} is exponential	inside typeof value === 'number', so it is String(<number>) — e.g. 1e+21. A number has no text to inject

Every other throw names only paramName, which is a literal from the call site, never caller data.

Note :371 — that's a fourth echo site your list didn't include (safeEncodedUrlPathSegment). Same constant-only pattern, so the conclusion is unchanged, but the audit list should be complete or the next person will think one was missed.

I proved it rather than reasoning about it: a fuzz over 18 injection payloads (IGNORE PREVIOUS INSTRUCTIONS…, </result><system>…, {{prompt}}, a lone surrogate, U+2028, embedded newlines, a 200-char blob) plus 12 non-string/numeric shapes, across all three helpers — 90 cases. For every message produced, every quoted run is one of . or .., and no free-form payload appears anywhere in the text. U+2028 specifically cannot reach a message: a string containing it isn't a dot segment, so it exits via cannot contain a path separator or is encoded and returned.

The distinction matters in the other direction too: strictUrlPathSegment's echo is genuinely unsafe and your fix on #7269 is right, because it quotes free-form padded input. A guard that names a constant it just matched on is fine; one that quotes what the caller actually sent is not.

---

1. close_pr.ts — real, but it's 21 tools, not the third instance

The finding is correct: owner/repo were raw pre-PR, so I do newly introduce trimming, and a padded value that used to 404 now hits the real repository. That's the rule you stated, and it's met.

But scoping it to close_pr understates it. Every mutating GitHub tool had raw interpolation pre-PR, so all of them newly trim:

DELETE  delete_branch, delete_comment, delete_comment_reaction, delete_file,
        delete_issue_reaction, delete_milestone, delete_release, remove_label, unstar_repo
PATCH   close_issue, close_pr, update_comment, update_issue,
        update_milestone, update_pr, update_release
PUT     create_file, merge_pr, star_repo, update_branch_protection, update_file

21 tools. delete_gist is the one exception — it already had gist_id?.trim() pre-PR, so per your rule it isn't a change I'm making.

One nuance that shrinks the blast radius: the exposure is only via safeUrlPathSegment on owner/repo/ids. The path parameter on create_file/update_file/delete_file goes through safeUrlPath, which no longer trims at all — so the highest-stakes value in the destructive set is already immune as of 515b951.

I'm not fixing this here, deliberately. strictUrlPathSegment lands with #7269; hand-rolling a second copy on this branch would create two competing helpers and guarantee a conflict with the PR that introduces the real one — and it would fix 1 of 21. This wants one systematic pass over the whole table after #7269 merges. Filed as a follow-up.

2. get_branch_protection.ts — pre-existing, byte-identical, needs live-API verification

Not introduced by this PR. Pre-PR was raw ${params.branch}; safeUrlPath emits feature/foo unchanged — that's exactly why I chose it over safeEncodedUrlPathSegment for this parameter. Proven by the passing LEGITIMATE_PATHS assertion, which pins feature/my-branch through unchanged for every multi-segment param including this one. Same for get_branch, delete_branch, update_branch_protection, get_commit(ref), compare_commits(base/head).

So if /branches/feature/foo/protection 404s today, it 404'd identically before this PR.

Switching to %2F is a wire change on a currently-clean branch, and I'd want it verified against the live API first — GitHub's router is greedy for /branches/* on the plain endpoint, and whether that holds with a /protection suffix is the actual open question, not something to guess at. Worth noting it's also the mirror image of finding 1: there the objection is that a fix turns a failing request into a succeeding one. Follow-up, with verification.

3. get_tree.ts falsy 0 — not a defect

Pre-PR: params.path || ''. Post-PR: params.path ? safeUrlPath(...) : ''. Identical falsy set — 0 produced '' before and produces '' now. Nothing changed, so there's no regression to fix here.

Nor is the skipped guard a hole: every falsy value ('', 0, NaN, null, undefined, false) yields '', giving /contents/ — a repo-root listing. There is no value that skips the guard and reaches a path segment, so nothing traversable slips through. path is declared type: 'string' and optional here, which is why the truthiness test is the right shape; get_file_content guards unconditionally because its path is required. I'd rather not add a typeof/length dance to a branch at 5/5 for a case with no behavioural difference.

---

Happy to take the follow-ups — the 21-tool strictUrlPathSegment pass is the substantive one and should go in after #7269 so there's a single helper to call.

[waleedlatif1]

Tenth finding routed from #7269 — cubic, apps/sim/tools/github/get_tree.ts:

P2/P3: When ref contains a lone UTF-16 surrogate from a malformed JSON/LLM value, encodeURIComponent throws a bare URIError before the request can be prepared. Guard this encoding like encodeSegment so the tool returns a named ref input error instead of an opaque request failure.

Valid, and the fix already exists in this PR — it just is not used at that call site. encodeSegment in url-path.ts wraps exactly this:

function encodeSegment(segment: string, paramName: string): string {
  try {
    return encodeURIComponent(segment)
  } catch {
    throw new Error(`${paramName} contains an unpaired UTF-16 surrogate and cannot be encoded`)
  }
}

I verified the difference on the downstream branch with a lone high surrogate:

via encodeSegment (safeUrlPathSegment):  Error: projectId contains an unpaired UTF-16 surrogate and cannot be encoded
raw encodeURIComponent:                  URIError: URI malformed

So the guards that go through encodeSegment are already correct; the gap is anywhere this PR still calls encodeURIComponent directly. Worth grepping for that rather than patching get_tree.ts alone — the same shape will exist at any other direct call.

For symmetry with what I found downstream: #7269 has one such site, google_drive/get_content.ts interpolating encodeURIComponent(exportFormat) into a query parameter. I left it alone there because it is byte-identical in origin/staging, untouched by that diff, and a query param rather than a path segment — outside that PR's contract. Same judgement may or may not apply here depending on whether the get_tree.ts site is one this PR introduced.

---

Running list routed from #7269 (all verified there, none fixed downstream since they are this PR's files):

1. safeEncodedUrlPathSegment accepts backslashes — P1, I reversed my initial "inert" position; the risk is downstream normalizers folding \ to /.
2. remove_label.ts builds html_url/content from raw params — successful removal returns a dead link.
3. executeGitHubTool does not await its comment operations — makes this PR's own buildGuardedUrl 400-mapping unreachable.
4. safeUrlPath rejects a whitespace-only path component — over-rejects since the whitespace fix.
5. remove_label.ts trims label names — data-loss class; a label may legitimately carry whitespace.
6. close_pr.ts trims owner/repo — data-loss class; padded owner went from 404 no-op to closing a real PR.
7. get_branch_protection.ts splits a branch containing / — silent 404; wants safeEncodedUrlPathSegment.
8. get_tree.ts skips the guard for a falsy 0 path.
9. Guard errors echo the rejected value into a model-facing tool result — prompt-injection surface.
10. This one.

Offer stands: happy to take any subset as a follow-up PR against this branch if that is easier than folding them in.

[cubic-dev-ai]

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

No issues found across 81 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

_{Re-trigger cubic}

[waleedlatif1]

Adopted 0c5108ecdf downstream, and it corrected a call I had got wrong.

Your rule is sharper than the one I had been applying. I used "refuse padding wherever this change newly trims an identifier" — which is why #7269 had strictUrlPathSegment on six GET routes (box_sign_get_request, and BigQuery get_query_results, get_table, list_datasets, list_table_data, list_tables). Your doc states the reason the rule exists rather than the rule itself:

refuse where being wrong is unrecoverable, tolerate where being wrong is merely unhelpful

Applied to a read, my version was refusing a padded id whose only consequence would have been returning the resource the caller almost certainly meant — while breaking a working paste-with-a-stray-newline flow to do it. That is cost with no safety gain, and the explicit "do not complete the asymmetry" note is well placed, because completing it is exactly what I had done.

So #7269 now reverts those six to safeUrlPathSegment and keeps the strict guard on its eight state-changing routes (BigQuery delete_dataset, delete_table, create_dataset, create_table, query, insert_rows; Box Sign cancel_request, resend_request). Its test pins follow the same split — the newly-trimmed map lists writes only.

Two smaller notes from the adoption:

1. assertUnpadded's trimmed && trimmed !== value is better than what I had written locally. An all-whitespace value falls through to safeUrlPathSegment and reports "<param> is required", which names the real problem, where my version reported a padding error.
2. My local tools/strict-url-path.ts is deleted and its four consumers import from @/tools/url-path, so there is one strict guard in the tree. The error text differing (must not have vs cannot have) surfaced as four failing assertions on rebase rather than silently — the exact-text pinning earning its keep for the third time.

Nothing outstanding from me on this PR. The routed list is fully addressed as far as I can see: the strict guards cover the destructive routes, the whitespace-only component is permitted, and safeEncodedUrlPathSegment now has a state-changing variant for remove_label.

[waleedlatif1]

@greptile review

[waleedlatif1]

@cubic review

[cubic-dev-ai]

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

All reported issues were addressed across 81 files

<sub>Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic</sub>

[waleedlatif1]

@greptile review

[waleedlatif1]

@cubic review

[cubic-dev-ai]

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

[waleedlatif1]

Confirming ccbf0d2fa2 — the backslash rejection in safeEncodedUrlPathSegment. That closes the finding I originally argued was inert and then reversed on: my check was against the WHATWG parser, which never decodes %5C, but the risk was a downstream normalizer folding \ to /. Good to see it land as a rejection rather than a special case.

No effect on #7269 — it has zero uses of safeEncodedUrlPathSegment or strictEncodedUrlPathSegment (its multi-segment values go through safeUrlPath, single-segment through safeUrlPathSegment / strictUrlPathSegment). Rebased onto it, 14,305 tests green, scope still zero changed lines in tools/url-path.ts, tools/github/** and lib/internal/github/**.

One thing worth flagging back, because it is the mirror image of a decision on this PR. safeUrlPath trimming nowhere is right for a git filename, but downstream it came within a commit of breaking a real flow — Supabase storage keys used to be trimmed, so a saved workflow with a pasted stray space resolved fine and would have started 404ing:

old  encodeStoragePath("  avatars/photo.png  ")  ->  avatars/photo.png
new                                              ->  %20%20avatars/photo.png%20%20

#7269 resolves it by trimming the whole value while preserving whitespace inside it — edge padding on the whole value is a paste artifact and never part of the key, whereas docs/ draft.md names a component that really does start with a space. That keeps your correctness fix intact and restores what pasted keys relied on.

Not suggesting a change here: for GitHub path the same distinction may or may not apply, and you are better placed to judge whether a pasted docs/ draft.md with trailing space is a realistic saved value. Raising it only because the trimming question landed differently on each side of the dependency, and the reasoning is worth having in one place.

[cubic-dev-ai]

No issues found across 81 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

_{Re-trigger cubic}

[waleedlatif1]

Closing for now — not because of a defect. This batch grew to 17 PRs across ~700 changed call sites, and we would rather revisit it as smaller, independently testable pieces than merge this much at once.

Nothing here is lost: the branch fix/github-path-safety is preserved and this PR can be reopened. Review state, the reasoning on every thread, and the red-first verification all stay attached.