fix(tools): reject path traversal in Drive, BigQuery, Box, Supabase and Contacts ids

[waleedlatif1]

Depends on #7262 — merge that first. Rebased directly onto ccbf0d2fa2 (fix(github): reject a backslash in safeEncodedUrlPathSegment), so safeUrlPath, strictUrlPathSegment and the rest come from it — this PR defines no path guards of its own. Scope re-verified against that exact tip: zero changed lines in tools/url-path.ts, tools/github/** and lib/internal/github/**.

The defect

Google Drive, BigQuery, Box, Box Sign, Supabase and Google Contacts tools interpolated LLM-writable ids straight into URL path segments. These params are visibility: 'user-or-llm', so prompt injection controls them. A value like ../../files/victim re-aims an authenticated request — carrying the user's OAuth token or the workspace's Supabase service-role key — at a different resource on the same host, including on DELETE.

encodeURIComponent is not a fix. . and .. are unreserved, so they survive encoding untouched, and the WHATWG URL parser removes dot segments after decoding:

new URL('https://api.box.com/2.0/files/' + encodeURIComponent('..') + '/content').pathname
// => '/2.0/content'

Only rejecting the value works.

Highlight: Supabase encodeStoragePath looked sanitised and was not

apps/sim/tools/supabase/utils.ts split the object key on / and ran encodeURIComponent over each segment. Because encodeURIComponent('..') === '..', a key of ../.. passed through byte-for-byte unchanged:

$ node -e "console.log('../..'.split('/').map(s=>encodeURIComponent(s.trim())).join('/'))"
../..

Every Supabase storage tool and both internal storage operations (lib/internal/supabase/operations.ts, operations/storage-get-public-url.ts) went through it, so /storage/v1/object/<bucket>/../../rest/v1/... escaped the storage prefix entirely.

The fix

• Single-segment ids → safeUrlPathSegment (Drive fileId/permissionId/commentId/revisionId, Box fileId/folderId, Box Sign signRequestId, BigQuery projectId/datasetId/tableId/jobId, Supabase functionName and bucket names).
• Genuinely multi-segment values → fix(github): reject path-traversal values in interpolated URL segments #7262's safeUrlPath (Supabase storage keys, Contacts resourceName). An earlier revision added its own near-duplicate helper in a separate file; that has been deleted. There is exactly one multi-segment guard in the tree.

Empty segments are now rejected where the old helper silently emitted them. /storage/v1/object/avatars//folder/x.png and .../avatars/folder/x.png are different paths, so the old behaviour addressed a different object than the caller wrote. Nothing real needs one — executeStorageUploadOperation normalizes its own trailing separator, and a resourceName is always people/<id>.

Storage keys: a pasted key still resolves

Guarding storage paths came within one commit of breaking a real flow, and this is the only place in the PR where that was true. safeUrlPath trims nowhere — correct for a git filename — but the helper it replaced did trim, so a saved workflow whose key field carried a pasted stray space resolved fine:

old  encodeStoragePath("  avatars/photo.png  ")  ->  avatars/photo.png              (found it)
new (before this fix)                            ->  %20%20avatars/photo.png%20%20  (404)

Resolution: trim the whole value, preserve whitespace inside it. The two cases differ in kind — edge padding on the whole value is a paste artifact and never part of the key, while avatars/ photo.png names a component that genuinely starts with a space. So pasted keys resolve exactly as before, and the correctness safeUrlPath exists to provide is kept.

Why trim rather than refuse, when this PR refuses elsewhere? Because the refuse rule is for state-changing requests where being wrong is unrecoverable, and no destructive storage operation routes an object key through this helper:

operation	where the key travels
storage_delete	request body (prefixes) — only the bucket reaches a path guard
storage_move, storage_copy	request body (sourceKey, destinationKey)
storage_download, storage_get_public_url, storage_create_signed_url	path — reads
storage_upload, storage_create_signed_upload_url	path — writes, non-destructive

Upload and download therefore trim identically, so a padded key is never stored padded and the pair cannot disagree about what a key is. That was the original argument for preserving, and it is satisfied here without breaking pasted keys. The no-destructive-caller premise is pinned in path_safety.test.ts, because the reasoning depends on it and it could change silently.

Trimming first also makes this stricter than before in one respect: " .. " is now refused outright, where preserving let it survive as %20%20..%20%20.

⚠️ Compatibility: padded identifiers are now refused, not trimmed (2 irreversible operations)

This is the highest-consequence change in the PR and it is on two DELETE tools. Read this one.

projectId was interpolated as encodeURIComponent(params.projectId) before this branch — never trimmed. A padded value became %20%20my-project%20%20, which names no GCP project (ids match [a-z][a-z0-9-]{5,29}, no whitespace), so the request failed cleanly and nothing happened. safeUrlPathSegment trims:

before: /bigquery/v2/projects/%20%20my-project%20%20/datasets/prod_dataset   -> 404, no-op
after:  /bigquery/v2/projects/my-project/datasets/prod_dataset               -> deletes it

On google_bigquery_delete_dataset and google_bigquery_delete_table that silently converts a request that did nothing into one that irreversibly destroys a real dataset or table, from a value the caller never wrote. datasetId on those tools was already .trim()-ed pre-PR, so projectId was the one identifier out of step — the guard hid its own hazard.

Decision: refuse the padded value — on state-changing routes only.

An earlier revision applied the refusal to every tool that newly trimmed an identifier, including reads. #7262 then documented the sharper rule and this branch adopted it: the reason the rule exists is that the harm is asymmetric. On a write, being wrong mutates or destroys a resource the caller never named — unrecoverable, and invisible in review because every traversal assertion still passes. On a read, being wrong returns data from the resource the caller almost certainly did mean, having typed the padded name themselves, while refusing breaks a working paste-with-a-stray-newline flow for no safety gain.

Refuse where being wrong is unrecoverable; tolerate where being wrong is merely unhelpful.

	tools
Strict (refuses padding)	BigQuery delete_dataset, delete_table, create_dataset, create_table, query, insert_rows; Box Sign cancel_request, resend_request
Plain (trims, as before)	BigQuery get_query_results, get_table, list_datasets, list_table_data, list_tables; Box Sign get_request

The rule is narrow and testable: this change must not turn a failing request into a succeeding one. Two facts specific to these values carry it, and neither is an appeal to consistency with the other guarded sites — that argument averages over very different blast radii and would excuse the deletion above:

1. No legitimate BigQuery identifier contains surrounding whitespace, so refusing excludes nothing real.
2. The previous behaviour for these parameters was already a clean failure, so refusing preserves it — and improves on it, replacing an opaque 404 with an error naming the parameter.

Second instance, found by sweeping the other five services

The same class exists in Box Sign, and it was not in the original report — I found it by asking, for every service, which parameters does this PR newly trim, and which of those sit on an irreversible request?

signRequestId was interpolated raw before this branch — not even a .trim():

before: /2.0/sign_requests/%20%20<uuid>%20%20/cancel   -> 404, no-op
after:  /2.0/sign_requests/<uuid>/cancel               -> cancels a real signature request

box_sign_cancel_request is irreversible, so the same fix applies. Box Sign ids are UUIDs, so no legitimate value carries whitespace. Both services use #7262's strictUrlPathSegment. This branch briefly carried its own tools/strict-url-path.ts while the two PRs were in flight; once #7262 landed the same rule, that file was deleted and its consumers repointed, so there is one strict guard in the tree rather than two.

The rest of the sweep is clean, and deliberately unchanged:

service	pre-PR	verdict
google_drive	every path id already ?.trim()	no new resolution
box	every path id already .trim()	no new resolution
google_contacts	resourceName.trim() → now safeUrlPath (no trim)	moves the safe way: a padded value that worked now fails
supabase storage key	trimmed per segment → now preserved	moves the safe way

Deliberately not changed: identifiers already .trim()-ed before this branch — datasetId on the delete tools, tableId on delete_table, jobId on get_query_results. Trimming those is not a change this PR makes, and refusing them would break callers whose stored value works today. That is a real pre-existing hazard on a destructive tool and worth a follow-up, but bundling a behaviour break into a security fix is how the regression above got in.

Pinned by NEWLY_TRIMMED_BY_THIS_CHANGE, which upgrades the generic per-pair whitespace assertion from "same path or no path" to must throw, plus explicit delete_* tests asserting the method really is DELETE, that a padded projectId throws, that the unpadded id still works, and that datasetId is still trimmed. Verified non-vacuous: reverting either guard to a plain trim fails both the generic and the explicit assertions.

Behaviour change: whitespace in a storage key is now preserved, not trimmed

This is the change most likely to be noticed, and it is deliberate. safeUrlPath after #7262's fix trims nowhere — not interior segment edges, not the whole key's leading/trailing edge. The encodeStoragePath it replaces did encodeURIComponent(segment.trim()) per segment, so it dropped whitespace at every edge:

key	old encodeStoragePath	now
folder/ file.png	folder/file.png	folder/ file.png
avatars/file.png	avatars/file.png	avatars/file.png
folder/my file .png	unchanged	unchanged

Decision: a padded key 404s rather than being trimmed. In order of force:

1. Upload and download share this helper, so preserve/preserve is the only self-consistent pair. storage_upload builds its key through the same encodeStoragePath. Trimming on read would make a padded key that was legitimately uploaded through this very tool permanently unreachable — you could create an object you can never fetch or delete. That alone settles it.
2. The failure modes are asymmetric. Preserving yields a 404 quoting the key actually sent: loud, self-explanatory, one edit to fix. Trimming yields a successful response against the wrong object, which nothing downstream can detect — and on supabase_storage_delete that deletes the wrong file.
3. A padded key is simply a different object. Supabase object names are opaque bytes; " a/b.png " and "a/b.png" are two keys. Trimming does not clean the input, it addresses something the caller did not name.

path is user-or-llm, which reinforces it: a guard that quietly normalizes model output makes an injection attempt and an honest typo indistinguishable.

Pinned by assertions in supabase/path_safety.test.ts (round-trips %j byte-for-byte, addresses the padded object rather than the unpadded one, encodes the padding so it cannot restructure the URL) so a later change to url-path.ts cannot flip it silently. The previous "equivalence with the replaced helper" assertion is deleted — it is false in both the interior and edge cases now.

A related consequence: ' .. ' is no longer rejected for multi-segment params

This reads alarming, so here is the verification rather than the assertion. A padded dot segment is not traversal:

safeUrlPath('  ..  ', 'path')                      -> '%20%20..%20%20'
new URL(base + '/' + that).pathname                -> '/storage/v1/object/avatars/%20%20..%20%20'

Dot-segment removal matches only an exact . or .. (and their %2e spellings). %20%20..%20%20 is one ordinary segment that the parser never removes, so it names an object literally called " .. " — which is what the caller wrote. The bare .. is still rejected, which is the case that actually matters, and that is asserted in the same test. Single-segment ids still trim-then-reject via safeUrlPathSegment, because whitespace around an id is copy-paste noise rather than data — the harness models this as preservesWhitespace.

Noted, deliberately not fixed here

• google_drive/get_content.ts:108 interpolates encodeURIComponent(exportFormat) into a query parameter, and exportFormat derives from the caller-supplied params.mimeType. A lone UTF-16 surrogate there throws a bare URIError rather than a named error. It is byte-identical in origin/staging, untouched by this diff, and a query param rather than a path segment — outside this PR's contract. Flagged rather than folded in, for the same reason as the pre-existing datasetId trim: quietly widening a security fix into unrelated behaviour is how the padded-projectId regression got in. Unlike that case there is no risk in fixing it, so it is a cheap follow-up if wanted.
• datasetId on the BigQuery delete tools was already .trim()-ed before this branch. Trimming it is therefore not a change made here, and refusing it would break callers whose stored value works today — but it is a real pre-existing hazard on a destructive tool.

The guards this PR adds are not affected by the surrogate case: they delegate to encodeSegment, so a lone surrogate produces projectId contains an unpaired UTF-16 surrogate and cannot be encoded rather than URIError: URI malformed. Verified directly.

Rejected as not-a-risk

• Supabase table (8 REST tools) — already guarded by validateDatabaseIdentifier. I had guarded it, then reverted when tests surfaced the existing validator.
• Supabase invoke_function — already has validateFunctionName.
• app/api/tools/drive/{file,files}/route.ts — already validate with validateAlphanumericId. (The routes named in the brief, google_drive/{download,export}, do not exist.)
• Drive create_folder / get_content inner fetches — the id comes from Google's own API response (data.id, metadata.id), not the caller.

Tests

75 (tool, parameter) pairs across six services, over a shared harness in apps/sim/tools/__tests__/path-safety.ts. Four properties, each added because an earlier revision lacked it:

One parameter at a time. The first harness filled every string param with the same hostile value and catch { return }-ed, so one guarded param masked its siblings. Demonstrated, not assumed: with google_drive_unshare's permissionId fully unguarded and its sibling fileId still guarded, the old harness reported 285/285 green.

Rejection, not shape. https://x/a/. normalizes to https://x/a/ — same segment count, every other segment intact — so a shape-only assertion is blind to a dot segment in the final position, which is where the Drive delete_* family and box_sign_get_request put their guarded id. Every value encoding cannot neutralize is asserted to throw and name its parameter.

Every branch. Discovery harvests the literals a builder compares against from String(tool.request.url), so a param reachable only on one branch cannot hide. Measured: 75 pairs before and after, no pair requiring a branch literal — the 29 conditionals here are all query-string only.

No tool can silently leave the suite. Sibling params are filled from their declared type (1 for number, false for boolean, [] for json/array) so an early type check on a sibling cannot remove a tool, and each suite pins both UNBUILDABLE (empty) and the exact set of tools contributing no path parameter.

cubic caught that this pin was initially vacuous: the inventory was enumerated through asPathTool, which requires request.url to be a function, so a tool declaring url as a constant string (box_create_folder) or an InternalToolConfig with no request (box_upload_file) was invisible to the covered pairs and the pinned set — toEqual(['box_search']) passed precisely because they could not be seen. The real extent was 11 tools across four services, including supabase_storage_upload and supabase_storage_get_public_url, which genuinely do build storage paths via encodeStoragePath. The inventory now walks every export carrying the service id prefix whatever shape its request takes. Verified non-vacuous: dropping the two Box tools back out now fails the assertion, where before it changed nothing.

Internal tools have no request for this suite to drive, so pinning them proves they are accounted for, not that they are traversal-safe; their real coverage is the direct encodeStoragePath / encodeStorageSegment unit tests on the exact helper lib/internal/supabase/operations.ts calls. The TSDoc says so rather than overstating the guarantee a second time.

Balanced traversal. A value that pops exactly as many segments as it adds keeps the baseline segment count, so a count-only check cannot see it. id/../../other/victim is in the throw-asserted set and verified red against an unguarded trailing parameter. The inert-value shape check also previously pinned only the prefix ahead of the guarded slot, which would let a value that expands its own slot (a/b/../c) through on a single-segment param; that case now pins the whole shape — count plus every slot but the guarded one.

Plus: a structural PathTool narrowed by asPathTool(value: unknown) instead of ToolConfig<any, any> (zero any), and getErrorMessage from @sim/utils/errors throughout — check:audits enforces that inside test files too.

LEGITIMATE_IDS proves real values reach the wire unchanged: Drive ids with -/_, Box's root folder id 0, people/c12345, folder/sub/file.png, folder/my file .png, and my-project.my_dataset.my_table.

Review findings fixed on this branch

Both bots ran against the head SHA and found real defects, all fixed and each verified red first:

Finding	Raised by	Fix
Traversal tests masked later parameters	Greptile + cubic	(tool, param) pair enumeration; proven with a 285/285-green demo of the hole
Harness exposed ToolConfig<any, any>	Greptile	structural PathTool narrowed from unknown
multi-segment-url-path.ts trimmed each segment	cubic	file deleted; consolidated onto #7262's safeUrlPath
STATIC_URL_TOOLS pin was vacuous	cubic	inventory no longer filtered through asPathTool; 11 invisible tools surfaced
BigQuery projectId differed between URL and body	cubic	normalized in create_dataset, create_table, query; pinned by test
Dotted ids listed as per-parameter legitimate values	cubic	removed — no BigQuery path param accepts a dot; the guard-level property is already pinned in url-path.test.ts
box_sign header comment contradicted itself	cubic	my bulk perl substitution had rewritten the pre-fix template quoted inside the comment; reworded to past tense
BigQuery body .trim() threw on a numeric project id	cubic	a regression from the fix above: safeUrlPathSegment accepts a JSON number, .trim() does not, so the path built and the body threw a raw TypeError. Body identifiers now derive from the path guard itself
Branch probing pinned only one sibling at a time	cubic	a parameter behind two simultaneous conditions was never probed while the docstring claimed "every branch is probed"; added pair probing, capped, with the depth-2 bound stated
Supabase guard failures returned 500, not 400	cubic	nothing in encodeStoragePath threw before this branch, so the new rejections fell through to the generic handler and blamed the server for a caller's ..; mapped at both call sites, with tests asserting no provider request is made
MUST_NOT_RESHAPE swallowed an over-tightening	cubic	catch { return } meant none of the assertions ran, so a guard that rejected a value it should have encoded passed silently; a throw now fails unless the parameter is named in strictlyValidated
preservesWhitespace branch swallowed a rejection	cubic	same class — the branch asserts padding survives, so a throw contradicts it and must fail
Guard errors echoed the rejected value	cubic	these params are user-or-llm and the error returns as a tool result the model reads, so quoting the input copied attacker-chosen text (U+2028/U+2029 included) back into the prompt; value dropped, parameter name kept
Per-parameter discovery probe swallowed a throw	self-audit	a parameter failing every probe left coverage while its siblings kept the tool covered, so neither UNBUILDABLE nor STATIC_URL_TOOLS noticed; discovery now reports it and each suite pins the set empty

Nine findings landed on files this PR does not touch — tools/url-path.ts, tools/github/** and lib/internal/github/**, all added by #7262, which this branch is rebased onto so safeUrlPath is importable (that is also why cubic reports 130 files). All are verified and routed to #7262 rather than fixed here, since fixing them would put changes to that PR's code in this PR's history and conflict on merge:

• safeEncodedUrlPathSegment accepts backslashes. I first argued this was inert and I answered the wrong question — my check was against the WHATWG URL parser, which does not decode %5C before dot-segment removal. cubic's claim was about downstream normalizers, which my check could not catch: IIS-style folding turns \ into /, so %5C..%5C.. can become /../.. at a proxy. I reversed my position and recommended the rejection on fix(github): reject path-traversal values in interpolated URL segments #7262.
• remove_label.ts builds its response from raw params. Confirmed by reading it: the DELETE path is guarded but html_url and content are rebuilt from untrimmed params, so a successful removal returns a dead link. Flagged there as a sweep, since any transformResponse rebuilding a URL from raw params has the same split.
• executeGitHubTool does not await the comment operations. The most consequential: return promise inside try completes the block before it settles, so the enclosing catch never sees a rejection. That makes fix(github): reject path-traversal values in interpolated URL segments #7262's own buildGuardedUrl 400-mapping unreachable for github_comment, so a rejected owner surfaces as a generic 500 instead of the documented client error — the exact misattribution its TSDoc says it prevents.
• safeUrlPath rejects a whitespace-only path component. Over-rejects: since fix(github): reject path-traversal values in interpolated URL segments #7262's own whitespace fix that helper treats whitespace as data, so docs/ /notes.md is a legal path it now refuses. Flagged with the caveat that the empty-segment rule must stay — a/ /b and a//b address different objects.
• remove_label.ts trims label names. The data-loss class again: a padded label previously 404'd, and trimming now removes the real one. A label may legitimately carry surrounding whitespace, so it is also wrong on plain correctness grounds.
• close_pr.ts trims owner/repo. Third instance of the data-loss class, and cubic explicitly recommends the fix that landed here — a padded owner went from a 404 no-op to closing a PR in the real repository. fix(github): reject path-traversal values in interpolated URL segments #7262 has since fixed this itself in d2c74d7416, along with delete_milestone, delete_branch, update_pr and delete_file — the fix landed there rather than as a follow-up, because this PR is rebased onto fix(github): reject path-traversal values in interpolated URL segments #7262, so "after fix(tools): reject path traversal in Drive, BigQuery, Box, Supabase and Contacts ids #7269" would have meant "after the regression shipped".
• get_branch_protection.ts splits a branch containing / across segments. feature/foo becomes /branches/feature/foo/protection and 404s silently; wants safeEncodedUrlPathSegment. Worth auditing every branch/ref site, since the two helpers are easy to swap and nothing fails loudly.
• get_tree.ts skips the guard for a falsy 0. params.path ? … : '' is a truthiness test doing an is-it-absent job, so a directory named 0 cannot be listed — and 0 is a value the guard's own type contract accepts.
• The error-echo finding applies to fix(github): reject path-traversal values in interpolated URL segments #7262's three guards too. They quote the rejected value into a message that buildGuardedUrl turns into a model-facing tool result, so the same prompt-injection surface exists there.

A recurring defect in my own tests, worth naming

Nine separate assertions on this PR could not fail. Review found six; three I found by auditing for the pattern myself:

#	Instance	Found by
1	Sibling-masking swallow — suite reported 285/285 green with a parameter fully unguarded	Greptile + cubic
2	STATIC_URL_TOOLS pin vacuous — 11 tools invisible to it	cubic
3	preservesWhitespace branch swallowed a rejection	cubic
4	MUST_NOT_RESHAPE swallowed an over-tightening	cubic
5	Per-parameter discovery probe swallowed a throw	self-audit
6	describe.each over a derived group that could silently empty	self-audit
7	namesParam substring match accepted a message naming the wrong parameter	cubic
8	if (serialized?.includes(…)) guarded the body assertion out of existence	self-audit
9	A negative assertion kept after its fixture stopped supplying the value it checked for	cubic

The last one was the most instructive: namesParam powered every "rejects … naming the parameter" assertion across all six suites, and its substring match accepted "projectId cannot …" as naming the parameter id. A guard naming the wrong identifier satisfied the check designed to catch exactly that. It now matches whole tokens (joining adjacent ones so "Invalid function name" still names functionName), and has its own contract test, because a weakness there is invisible from every suite it serves.

The ninth added a variant worth naming separately: it was not a tolerance at all but a literal that outlived its fixture. expect(serialized).not.toContain(' my-project ') was meaningful while the test supplied a padded id; when the strict guard made padding throw, the fixture became unpadded and the assertion could no longer fail. Changing a fixture silently defangs assertions written for the old one — a shape that auditing catch blocks and filters does not surface.

The common cause was a blanket tolerance quietly turning "acceptable here" into "untested everywhere". The fix in each case was to name the exceptions — strictlyValidated, rejectsSurroundingWhitespace, STATIC_URL_TOOLS, UNDISCOVERABLE — so an unexpected one fails rather than passes. Every assertion added after this was verified red before being kept.

Red-first verification

harness	failures with all guards reverted
leaky, shape-only	322
pair-based, shape-only	464
pair-based + rejection	642
+ coverage pin, balanced vector, strict-padding guards	652+

Scoped reverts also verified individually: google_drive_unshare.permissionId (sibling-masking) and google_drive_delete.fileId (trailing-position, where all 9 rejection vectors fire and a shape check sees nothing). 14,292 tests pass with the guards in place.

Gates

bun run lint, bun run check:audits (39/39), check-block-registry.ts, and tsc --noEmit all clean.

[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:11am

[greptile-apps]

Greptile Summary

This PR rejects path traversal in provider identifiers and multi-segment resource paths before authenticated requests are built.

• Applies single-segment guards to Drive, BigQuery, Box, Box Sign, and Supabase identifiers.
• Applies multi-segment validation to Supabase storage keys and Google Contacts resource names.
• Adds strict handling where whitespace normalization could retarget state-changing operations.
• Expands path-safety coverage with independently exercised tool-parameter pairs and explicit coverage inventories.
• Maps rejected caller inputs to actionable client-error responses in internal operations.

Confidence Score: 5/5

The PR appears safe to merge because the previously reported harness masking and type-safety issues are fixed and no blocking failure remains.

No blocking failure remains.

Important Files Changed

Filename	Overview
apps/sim/tools/tests/path-safety.ts	Replaces the masking-prone traversal harness with independently driven tool-parameter pairs, typed structural narrowing, branch probing, and pinned coverage inventories.
apps/sim/tools/github/path_safety.test.ts	Adds direct GitHub path-guard assertions and coverage for guarded identifiers without leaving an actionable follow-up defect.
apps/sim/tools/url-path.ts	Defines the shared single- and multi-segment path validation behavior used by the affected provider tools.
apps/sim/tools/supabase/utils.ts	Routes storage buckets, object keys, and function names through the appropriate shared path guards.
apps/sim/lib/internal/supabase/operations.ts	Guards internal storage upload paths and reports rejected caller inputs as client errors before issuing provider requests.
apps/sim/lib/internal/github/operations.ts	Guards internal GitHub URL components and maps path validation failures to client errors.
apps/sim/tools/google_bigquery/path_safety.test.ts	Verifies traversal rejection and strict whitespace behavior for BigQuery path identifiers, including state-changing operations.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[User or model supplied identifier] --> B{Path value type}
  B -->|Single segment| C[safe or strict segment guard]
  B -->|Multi segment| D[safe path guard]
  C --> E[Encoded provider URL]
  D --> E
  E --> F[Authenticated provider request]
  C -->|Rejected| G[Named client error]
  D -->|Rejected| G

Loading

_{Reviews (22): Last reviewed commit: "fix(supabase): trim a pasted storage key..." | Re-trigger Greptile}

[cubic-dev-ai]

2 issues found across 50 files

Confidence score: 3/5

• apps/sim/tools/multi-segment-url-path.ts trims each Supabase object-key segment before encoding, so keys with surrounding whitespace—or whitespace-only segments—can resolve to a different object or fail; preserve the raw segments when constructing the request path.
• apps/sim/tools/__tests__/path-safety.ts does not isolate each path parameter under test, allowing an earlier guarded identifier to fail first and mask traversal through a later unguarded parameter; pin the other parameters to safe values in independent cases.

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/multi-segment-url-path.ts">

<violation number="1" location="apps/sim/tools/multi-segment-url-path.ts:72">
P2: Supabase object keys can legitimately contain surrounding whitespace, but this trims each segment before encoding, so the request addresses a different object and whitespace-only segments are lost. Preserve each raw non-dot segment and reject only exact `.` or `..` before encoding.</violation>
</file>

<file name="apps/sim/tools/__tests__/path-safety.ts">

<violation number="1" location="apps/sim/tools/__tests__/path-safety.ts:73">
P2: Exercise each path parameter independently while pinning the other string parameters to safe values. Otherwise an earlier guarded identifier can throw first, allowing a later unguarded path parameter to escape traversal-test coverage.</violation>
</file>

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

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

[waleedlatif1]

@greptile @cubic-dev-ai review

Re-requesting review against the current head 6a506bf — the earlier review was against the first commit bea017e, and three commits of substantive change have landed since. All 4 threads from that review are addressed and resolved with individual replies.

What changed since bea017e:

1. Sibling masking (raised independently by both of you) — the harness filled every string param with the same hostile value and swallowed the throw, so a guarded param masked its siblings. Proven, not assumed: with google_drive_unshare's permissionId fully unguarded and fileId still guarded, the old harness reported 285/285 green. Now enumerates 75 (tool, parameter) pairs and fuzzes one at a time with type-appropriate safe siblings.

2. ToolConfig<any, any> removed — replaced by a structural PathTool narrowed from unknown via asPathTool. No casts, no any.

3. multi-segment-url-path.ts deleted — it was a near-duplicate of fix(github): reject path-traversal values in interpolated URL segments #7262's safeUrlPath; both callers now use that one helper, so there is a single multi-segment guard in the tree.

4. Whitespace in a storage key is now preserved, not trimmed — cubic's finding, fixed upstream in fix(github): reject path-traversal values in interpolated URL segments #7262 and adopted here. This is a deliberate behaviour change vs. pre-PR and is documented in the PR body.

5. Rejection assertions added — a shape-only check is blind to a dot segment in the final path position (https://x/a/. normalizes to https://x/a/ with the segment count intact), which is where the Drive delete_* family puts its id.

6. STATIC_URL_TOOLS pinned per suite — the exact set of tools contributing no path parameter, so a tool cannot silently leave coverage.

Red-first verification went 322 → 652 failures with all guards reverted; 13,423 pass with them in place.

Two points most likely to draw fire, with the reasoning up front:

• Padded storage keys now 404 instead of being trimmed. The decisive argument is self-consistency: storage_upload builds its key through the same encodeStoragePath, so trimming on read would make a padded key legitimately uploaded through this very tool permanently unreachable.
• ' .. ' is no longer rejected for multi-segment params. Verified inert: it encodes to %20%20..%20%20, one ordinary segment the URL parser never removes. Dot-segment removal matches only an exact ./... The bare .. is still rejected.

Note this PR depends on #7262 and must merge after it.

[cubic-dev-ai]

@greptile @cubic-dev-ai review

Re-requesting review against the current head 6a506bf — the earlier review was against the first commit bea017e, and three commits of substantive change have landed since. All 4 threads from that review are addressed and resolved with individual replies.

What changed since bea017e:
...

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

[waleedlatif1]

@greptile review

[waleedlatif1]

@cubic-dev-ai review

[cubic-dev-ai]

@cubic-dev-ai 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 130 files

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

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

[waleedlatif1]

@greptile review

[waleedlatif1]

@cubic-dev-ai review

[cubic-dev-ai]

@cubic-dev-ai review

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

[cubic-dev-ai]

3 issues found and verified against the latest diff

Confidence score: 3/5

• apps/sim/tools/google_bigquery/create_table.ts and apps/sim/tools/google_bigquery/query.ts can send different project IDs in the URL and JSON body when projectId includes surrounding whitespace, potentially targeting inconsistent BigQuery resources—normalize projectId once and reuse it everywhere.
• apps/sim/tools/google_bigquery/path_safety.test.ts treats dotted dataset/table identifiers as valid single-segment IDs, weakening the path-safety test coverage for these inputs—separate legitimate ID fixtures from dotted resource references and add focused cases.

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/google_bigquery/create_table.ts">

<violation number="1" location="apps/sim/tools/google_bigquery/create_table.ts:70">
P2: When `projectId` has surrounding whitespace, the new URL normalizes it but `tableReference.projectId` still sends the raw value. Normalize the project ID once and use the same value in both the path and JSON body, otherwise BigQuery receives inconsistent identifiers and the create request can fail.</violation>
</file>

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

<violation number="1" location="apps/sim/tools/google_bigquery/path_safety.test.ts:34">
P3: LEGITIMATE_IDS is applied verbatim to every single-segment BigQuery id (projectId, datasetId, tableId, jobId), but the dotted values `my_dataset.my_table`/`my-project.my_dataset.my_table` are not legitimate values for any one of those parameters — BigQuery resource IDs never contain dots, since `project.dataset.table` dots are the separators between distinct path parameters on the REST API. Pinning them as legitimate forces the guard to accept interior dots in single-segment ids and would spuriously fail (and block) a future, still-correct tightening that rejects them. Move the dotted qualified names out of the shared pool and use per-parameter legitimate sets.</violation>
</file>

<file name="apps/sim/tools/google_bigquery/query.ts">

<violation number="1" location="apps/sim/tools/google_bigquery/query.ts:69">
P2: When `projectId` has surrounding whitespace and `defaultDatasetId` is supplied, this call trims the URL project ID while the request body keeps the raw value. Normalize `projectId` once and use the same normalized value in both the URL and `defaultDataset.projectId`.</violation>
</file>

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

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

[waleedlatif1]

@greptile review

[waleedlatif1]

@cubic-dev-ai review

[cubic-dev-ai]

@cubic-dev-ai review

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

[cubic-dev-ai]

3 issues found and verified against the latest diff

Confidence score: 3/5

• apps/sim/tools/google_bigquery/create_table.ts can send different project IDs in the URL and tableReference when input has surrounding whitespace, risking a failed or inconsistent table creation request — normalize projectId once and reuse it.
• apps/sim/tools/google_bigquery/query.ts can mismatch the URL project ID and body.defaultDataset.projectId, so queries using defaultDatasetId may be rejected by BigQuery — use one normalized project ID throughout.
• apps/sim/tools/google_bigquery/create_dataset.ts trims only the URL project ID while retaining the raw value in datasetReference.projectId, which can make datasets.insert fail for padded input — normalize the value before building both request parts.

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/google_bigquery/query.ts">

<violation number="1" location="apps/sim/tools/google_bigquery/query.ts:69">
P2: When `projectId` contains surrounding whitespace and `defaultDatasetId` is provided, this URL uses the trimmed project ID while `body.defaultDataset.projectId` remains raw, so BigQuery receives inconsistent project references and rejects the query. Normalize `projectId` once and reuse that value for both the URL and request body.</violation>
</file>

<file name="apps/sim/tools/google_bigquery/create_table.ts">

<violation number="1" location="apps/sim/tools/google_bigquery/create_table.ts:70">
P2: When `projectId` has surrounding whitespace, this guard trims it in the URL but the request body still sends the padded value. Normalize `projectId` once and use the same value for both the URL and `tableReference`, or reject padded IDs before building the request.</violation>
</file>

<file name="apps/sim/tools/google_bigquery/create_dataset.ts">

<violation number="1" location="apps/sim/tools/google_bigquery/create_dataset.ts:63">
P2: When `projectId` has surrounding whitespace, this guard trims it for the URL while `body` still sends the raw value. Normalize once and use the same project ID in `datasetReference.projectId` so `datasets.insert` does not receive mismatched identifiers.</violation>
</file>

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

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

[waleedlatif1]

@greptile review

[waleedlatif1]

@cubic-dev-ai review

[cubic-dev-ai]

@cubic-dev-ai review

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

[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/storage-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.

[cubic-dev-ai]

1 existing issue remains and no new issues found across 135 files

Confidence score: 3/5

• In apps/sim/tools/supabase/utils.ts, whitespace is normalized before addressing Supabase object keys, so legitimate keys with leading or trailing whitespace can silently resolve to a different object and conflict with the intended rejection rule; pass the raw path while rejecting only bare ./.. segments.

<sub>Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.

Re-trigger cubic</sub>