fix(rootly,clerk,jira): reject path-traversal ids in URL segments - #7268
fix(rootly,clerk,jira): reject path-traversal ids in URL segments#7268waleedlatif1 wants to merge 2 commits into
Conversation
Rootly, Clerk and Jira tools interpolated LLM-writable identifiers straight into request path segments. A value like `../../users/victim` escapes its API prefix once `fetch` normalizes the URL, re-aiming the request — and the workspace's credential — at a different resource on the same host, including on DELETE. Clerk is the worst case: its credential is a backend API key with full user-management authority. `encodeURIComponent` does not close this. `.` and `..` are unreserved, so they survive encoding verbatim and the WHATWG URL parser removes them as dot segments afterwards. Only rejecting the value works, which is what `safeUrlPathSegment` does. Hardened 69 path-segment sites across 66 tool files, covering both the `request.url` builder and the `transformResponse` fallback that re-issues the call after resolving a Jira `cloudId`. Clerk's `provider` slug is guarded too: it is only prefixed with `oauth_`, so a separator in it still traverses. Jira's `cloudId` is left unguarded on purpose. It is `visibility: 'hidden'`, no Jira block subBlock has ever written it, and its only real source is `resolveAtlassianCloudId`, which returns a UUID from Atlassian's own accessible-resources endpoint. Each service gets a `path_safety.test.ts` that enumerates its tool barrel and, per tool, every parameter that reaches a path segment — fuzzing one parameter at a time so a still-guarded sibling cannot mask an unguarded one. URLs are resolved with `new URL(...)`, the same normalization `fetch` performs, rather than string-matched. A `LEGITIMATE_IDS` list proves real values (`PROJ-123`, `user_2abcDEF`, `org_2abcDEF`, Rootly UUIDs) pass through byte-for-byte.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe PR prevents model-writable Rootly, Clerk, and Jira identifiers from reshaping authenticated request paths by validating each interpolated URL segment.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and the previously reported test-harness typing issue is fixed at the current head.
|
| Filename | Overview |
|---|---|
| apps/sim/tools/clerk/path_safety.test.ts | Adds typed, barrel-driven coverage for Clerk path parameters and removes the previously reported unrestricted any usage. |
| apps/sim/tools/jira/path_safety.test.ts | Covers both primary Jira request URLs and URLs rebuilt after cloud-ID resolution using the typed harness. |
| apps/sim/tools/rootly/path_safety.test.ts | Adds typed traversal and legitimate-identifier coverage for Rootly’s dynamic request paths. |
| apps/sim/tools/clerk/get_user_oauth_token.ts | Validates both the user ID and provider slug before interpolating them into Clerk’s OAuth-token endpoint. |
| apps/sim/tools/jira/delete_comment.ts | Validates the issue key and comment ID in both direct and cloud-ID fallback request paths. |
| apps/sim/tools/rootly/delete_incident.ts | Validates the incident ID before constructing the authenticated destructive request URL. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[User or model supplies identifier] --> B[safeUrlPathSegment validation]
B -->|Valid segment| C[Build provider URL]
B -->|Traversal or delimiter input| D[Reject request]
C --> E[Authenticated Rootly, Clerk, or Jira request]
F[Jira cloud ID resolution] --> G[Fallback request builder]
G --> B
Reviews (2): Last reviewed commit: "test(rootly,clerk,jira): type the path-s..." | Re-trigger Greptile
There was a problem hiding this comment.
1 issue found across 69 files
Confidence score: 4/5
apps/sim/tools/jira/path_safety.test.tsdoes not coverjira_bulk_read.projectIdwhen it is interpolated bytransformResponse, leaving this URL path-safety case unvalidated and allowing a regression to go undetected—include parameters used in generated URLs in the test coverage.
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/jira/path_safety.test.ts">
<violation number="1" location="apps/sim/tools/jira/path_safety.test.ts:139">
P2: PATH_PARAMS only enumerates parameters that land in a path segment of `tool.request.url`. `jira_bulk_read.projectId` is interpolated only into a URL built inside `transformResponse` (its `request.url` is the fixed accessible-resources endpoint), so it is filtered out of PATH_PARAMS and never fuzzed here — contradicting the comment above the suite that it covers "every declared parameter that reaches a path segment." If the `safeUrlPathSegment` guard on that transformResponse-built URL is removed or weakened, this suite will not detect the regression it was written to catch. Extend PATH_PARAMS (or split it) to also exercise the URLs constructed in `transformResponse` after cloudId resolution, matching the second URL construction the PR guards.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…econd URL Two review findings, both real. cubic (jira/path_safety.test.ts): `PATH_PARAMS` enumerates only what `request.url` returns, but a Jira tool invoked without a `cloudId` sends its configured request to the fixed accessible-resources endpoint and builds the real per-site URL inside `transformResponse`. `jira_bulk_read.projectId` reaches a path segment only that way, so discovery could not see it — the guard was there, the test was blind to it. Adds a second suite that runs `transformResponse` with the `cloudId` withheld against a stubbed `fetch` and reads back the URLs the tool asked for. Discovery is probe-driven like the first suite, so it stays total rather than hard-coding the one known case: it finds 24 (tool, param) pairs, a superset of the 23 `request.url` exposes, the extra being `jira_bulk_read :: projectId`. Reverting that guard to the pre-PR `encodeURIComponent` now fails the suite. No additional unguarded parameter surfaced. Assertions there are shape-based — same segment count, same non-probe segments — because a tool may legitimately issue a different number of requests for different inputs, and segment count is what a popped dot segment changes. Greptile (all three files): the harness used `ToolConfig<any, any>` and cast generated params with `as any`, which CLAUDE.md forbids. Replaced with `asPathBuildingTool`, an `unknown` -> narrow type guard returning the slice the suite actually drives (`id`, `params`, `buildUrl`, and on Jira the optional `transformResponse`). No `any` remains in any of the three files. Rootly and Clerk headers now state why one suite is total for them — no tool in either service issues a `fetch` of its own from `transformResponse` — and the Jira header documents the residual limit: a URL a tool builds but never sends is invisible to both mechanisms.
|
Both review threads addressed in cubic P2 — Greptile P2 — harness erased tool types. Correct; CLAUDE.md forbids On Gates re-run on the new commit: 2094 tests pass across the three folders, |
|
Thanks for the follow-up. The generic |
|
@greptile review Both threads are addressed and resolved as of |
|
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 |
The defect
Rootly, Clerk and Jira tools interpolated LLM-writable identifiers (incident id, alert id, user id, organization id, session id, issue key, comment id, worklog id, attachment id, project id, …) straight into request path segments. Those params are
visibility: 'user-or-llm', so prompt injection controls them.A value like
../../users/victimescapes its API prefix oncefetchnormalizes the URL, re-aiming the request — carrying the workspace's credential — at a different resource on the same host, including on DELETE.assertRequestUrlMatchesTrustintools/request-transport.tsonly canonicalizes internal/api/routes, so nothing downstream catches it.Clerk is the worst case: its credential is a backend API key with full user-management authority, and the affected routes include delete/ban/lock.
encodeURIComponentdoes not close this:.and..are unreserved, so they survive encoding verbatim, and the WHATWG URL parser removes dot segments after decoding. Only rejecting the value works.The fix
safeUrlPathSegment(value, paramName)from@/tools/url-pathat 69 path-segment sites across 66 tool files:transformResponsefallbacksJira tools build the URL twice — once in
request.urlwhencloudIdis already present, and again insidetransformResponseafter resolving thecloudId. Both were unguarded; both are guarded now.Clerk's
providerslug is guarded too: it is only prefixed withoauth_, so a/inside it still traverses (provider = "x/../../users/victim").No param
visibility, no subBlock id, and no behaviour for legitimate input changed.bun run tool-metadata:generateproduces no diff.Jira's
cloudId— left unguarded on purposecloudIdreaches a path segment on every Jira tool, but it is not LLM-writable:visibility: 'hidden'on every toolgrep -c cloudId blocks/blocks/jira.tsis0, andgit log --all -S'cloudId' -- blocks/blocks/jira.tsreturns no commits — no revision has ever been able to set itgetJiraCloudId→resolveAtlassianCloudId, which returns a UUID from Atlassian's own accessible-resources endpointGuarding it would assert a threat model that does not exist. The test pins it instead, and says why.
Also checked and left alone:
lib/internal/jira/operations.ts, the server side ofjira_update/jira_write/jira_add_attachment, already runsvalidateJiraIssueKey(avalidatePathSegmentwithallowDots: false) before everyclient.issuePath()interpolation.Tests
path_safety.test.tsunder each of the three tool folders, modelled ontools/vercel/edge_config_path_safety.test.ts:new URL(...), the same normalizationfetchperforms, never string matching.and..vectors, plus' .. ',%2fspellings, backslashes, embedded traversal, and query/fragment injectionLEGITIMATE_IDSlist asserts real values pass through byte-for-byte:PROJ-123,MY_PROJECT-4567,user_2abcDEF,org_2abcDEF,sess_2abcDEF, Rootly UUIDs, and the..foo/foo..edge cases1684 tests pass across the three folders.
Verified the tests can fail: reverted the guard on
rootly_delete_incident.incidentId,clerk_delete_user.userId, andjira_delete_comment.commentIdone at a time and watched exactly that pair go red (10 failures each — the traversal vectors, both bare-dot rejections, and query injection), then restored.Gates
bun run lint,bun run check:audits(39 audits green, includingcheck:tool-request-boundaryandcheck:api-validation:strict),check-block-registry.ts origin/staging,bun run type-checkclean on all touched files,tool-metadata:generateno-diff.