fix(tools): reject path-traversal ids in Tailscale, Spotify and X tools by waleedlatif1 · Pull Request #7264 · simstudioai/sim

waleedlatif1 · 2026-08-29T03:26:36Z

The defect

Every identifier these tools interpolate into a request path — tailnet, deviceId, userId, keyId, playlistId, albumId, artistId, trackId, showId, episodeId, audiobookId, tweetId, targetUserId, username, woeid — is visibility: 'user-or-llm', so prompt injection controls it.

A value like ../../users/victim escaped its API prefix once fetch normalized the URL, re-aiming the request with the caller's credential attached at a different resource — including on the DELETE routes that remove a Tailscale device or user, unfollow a Spotify playlist, or delete a post on X.

Spotify and X interpolated the raw value with no encoding at all. Tailscale already wrapped it in encodeURIComponent, which is not sufficient:

new URL('https://api.tailscale.com/api/v2/tailnet/' + encodeURIComponent('..') + '/devices').pathname
// => '/api/v2/'   — the tailnet segment popped, `/devices` gone

. and .. are unreserved, so they survive encoding untouched and the URL parser removes them as dot segments after decoding. Only rejecting the value closes it.

The fix

All 71 call sites route through safeUrlPathSegment(value, paramName) from @/tools/url-path.

Service	Call sites	Previous state
Tailscale	28 across 24 tools	`encodeURIComponent(...trim())` — dot segments live
Spotify	24 tools	raw interpolation, no encoding
X	25 across 21 tools	raw `.trim()`, no encoding

No param visibility, subBlock id, or behaviour for legitimate input changed. A dot inside a longer segment is preserved, so a Tailscale tailnet keeps working as an organization name (example.com), an emailish login name (user@example.com), or the literal - default alias — all three verified against the Tailscale API v2 spec and pinned in the tests. Spotify base-62 ids and X numeric snowflakes pass through verbatim.

Dropping the .trim() calls is also a small improvement: an X snowflake arriving as a JSON number previously threw a bare TypeError: params.userId.trim is not a function; it is now accepted, and one too large for a double (already corrupted by JSON.parse) is refused by name rather than silently addressing the wrong id.

Body fields carry the same guard

Several X tools put the same identifier in the path on one branch and in the body on the other — x_manage_block sends targetUserId as a path segment to unblock, and as a body field to block. Guarding only the path left the two branches disagreeing about what a caller may send: a numeric id unblocked fine and threw TypeError: params.targetUserId.trim is not a function when used to block.

The TypeError predates this PR (both branches threw before), but the asymmetry was introduced here, so it is in scope:

export function xIdBodyValue(value: string | number | bigint, paramName: string): string {
  return decodeURIComponent(safeUrlPathSegment(value, paramName))
}

Deferring to the path guard and decoding — rather than restating which value kinds an id may take — makes drift between path and body impossible. Percent-encoding is a path concern; a JSON body carries the raw value. Applied to all six body sites (create_bookmark, manage_block, manage_follow, manage_mute, manage_like, manage_retweet), with a BODY_ID_TOOLS suite pinning that path and body agree per tool.

The tests

path_safety.test.ts per service. Discovery enumerates (tool, parameter) pairs — 79 of them (Tailscale 26, Spotify 24, X 29), matching the guarded call sites exactly — and fuzzes one parameter at a time with every sibling held at a safe value.

That per-pair isolation is the property that makes "a new unguarded parameter fails CI" actually true. An earlier revision of these tests filled every parameter with the same vector and swallowed the throw, so the first guarded parameter ended the case and its siblings went untested. Two demonstrations of the difference, both measured:

Scoped revert	Old harness	This harness
`x_manage_block` → `targetUserId` fully unguarded	622/622 green	15 failures
`tailscale_delete_device` → `deviceId` back to `encodeURIComponent`	2 failures (shape only)	12 failures

Reaching targetUserId at all requires exploring the branch: it appears only on the action === 'unblock' path, and action declares no enum, so the harness harvests the string literals the URL builder compares against from its own source. A new branch value is picked up without editing the tests.

Shape assertions alone are not enough, which the second row shows. https://host/a/. normalizes to https://host/a/ — the segment count and every other segment survive, so a shape-only check passes with the guard removed. Most of these routes end in the guarded id, x_delete_tweet, spotify_get_playlist and tailscale_delete_device among them, and one of those is a DELETE. Under a scoped revert, cannot reshape the path with "." never fails on any of the three. Rejection is therefore asserted directly, per parameter, alongside the shape check: shape catches a value that reshapes the path, rejection catches one that quietly collapses the trailing segment.

Other properties held by the suite:

Resolves every built URL with new URL(...) — the same normalization fetch performs — instead of string-matching the template output. String matching is what let this through.
LEGITIMATE_IDS proves real values reach the wire unchanged.
A tool whose URL cannot be built from all-safe values is surfaced by name rather than silently dropping out of the suite.
No any: the harness narrows barrel exports to a structural PathTool through a type guard.

2653 assertions, all green.

Also hardened during review

Two consequences of widening the guard to accept a numeric id, both surfaced in review and both genuinely introduced here:

Body fields. Six X tools carry the id in the body on one branch and the path on the other. Guarding only the path let a numeric id unblock successfully and throw TypeError when used to block. xIdBodyValue defers to the path guard and decodes, so the two cannot drift.
Echoed outputs. Eight Tailscale tools return the id in a success output declared type: 'string'. Previously a numeric id threw before reaching transformResponse; now it succeeds, so the echo is coerced with String(...) — safe because the URL builder has already rejected every other kind.

Gates

bun run lint, bun run check:audits (39/39), check-block-registry.ts origin/staging, and tool-metadata:generate (no diff — no params changed) all pass. Type-check clean for the three tool directories.

Every identifier these tools interpolate into a request path — `tailnet`, `deviceId`, `userId`, `keyId`, `playlistId`, `albumId`, `artistId`, `trackId`, `showId`, `episodeId`, `audiobookId`, `tweetId`, `targetUserId`, `username`, `woeid` — is `visibility: 'user-or-llm'`, so prompt injection controls it. A value like `../../users/victim` escaped its API prefix once `fetch` normalized the URL, re-aiming the request and the caller's credential at a different resource, including on the DELETE routes that remove a Tailscale device or user, unfollow a Spotify playlist, or delete a post on X. Spotify and X interpolated the value with no encoding at all. Tailscale already wrapped it in `encodeURIComponent`, which is not sufficient: `.` and `..` are unreserved, so they survive encoding untouched and the URL parser then removes them as dot segments, popping a path segment off a fixed host. Only rejecting the value closes it, so all 71 call sites now route through `safeUrlPathSegment` from `@/tools/url-path`. Legitimate input is unchanged. A dot inside a longer segment is preserved, so a Tailscale tailnet keeps working as an organization name (`example.com`), an emailish login name (`user@example.com`), or the `-` default alias, and Spotify base-62 ids and X numeric snowflakes pass through verbatim. Dropping the `.trim()` calls also lets a snowflake arrive as a JSON number, which previously threw a bare `TypeError`. Adds `path_safety.test.ts` per service. Each enumerates its tools from the barrel, so a newly added unguarded path parameter fails CI, and resolves every built URL with `new URL(...)` rather than string-matching the template — string matching is what let this through. Reverting any one guard turns them red.

vercel · 2026-08-29T03:26:41Z

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 5:13am

greptile-apps · 2026-08-29T03:28:44Z

Greptile Summary

The PR routes user- or LLM-controlled identifiers through a shared path-segment guard before constructing Tailscale, Spotify, and X API URLs.

Rejects traversal-capable dot segments and path separators.
Preserves legitimate identifiers while supporting safe numeric X IDs.
Adds per-tool, per-parameter traversal tests and verifies matching X path/body identifier behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
apps/sim/tools/x/path_safety.test.ts	Replaces the previously reported generic type erasure with runtime-validated structural narrowing and adds isolated path and body identifier coverage.
apps/sim/tools/x/types.ts	Adds a body-ID conversion helper that reuses the path guard’s accepted-value contract while returning the unencoded value for JSON bodies.
apps/sim/tools/spotify/path_safety.test.ts	Enumerates path-bearing Spotify parameters and independently verifies rejection, URL shape, origin, and legitimate-ID preservation.
apps/sim/tools/tailscale/path_safety.test.ts	Exercises guarded Tailscale path identifiers, including encoded dot-segment cases that previously survived encodeURIComponent.
apps/sim/tools/url-path.ts	Existing shared path-segment validation is consistently applied by the changed integrations.

_{Reviews (4): Last reviewed commit: "fix(tailscale): keep echoed ids as strin..." | Re-trigger Greptile}

cubic-dev-ai

2 issues found across 74 files

Confidence score: 3/5

In apps/sim/tools/x/manage_block.ts, numeric targetUserId values pass the guard but .trim() still throws before the POST, so valid block requests can fail at runtime — normalize numeric and string body values consistently before constructing the request.
In apps/sim/tools/spotify/path_safety.test.ts, the broad catch { return } can mark unrelated TypeErrors or crashes as successful traversal checks, weakening regression detection — assert the expected rejection instead of accepting any thrown error.

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/x/manage_block.ts">

<violation number="1" location="apps/sim/tools/x/manage_block.ts:51">
P2: When `action` is `block` and `targetUserId` is a JSON number, this guard permits URL construction, but `request.body` still calls `.trim()` and throws before POST. Normalize the body value with the same numeric-safe conversion.</violation>
</file>

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

<violation number="1" location="apps/sim/tools/spotify/path_safety.test.ts:114">
P3: The `catch { return }` in the "cannot reshape the path" test treats any thrown error as a pass, including TypeErrors or crashes unrelated to traversal, so that it.each test is vacuous for every vector that is rejected (e.g. `..`, `../../users/victim`, `\..\..`). Only the values `safeUrlPathSegment` happens to encode reach the length/shape assertions. Make the catch assert the rejection explicitly (_assert_ the throw is the traversal error) rather than silently passing, or drop the try/catch so an unexpected error fails loudly.

Note: the dedicated `rejects a bare dot-dot segment` and `rejects a bare dot segment` tests do assert the throw with /path traversal/, so the gap is on the separator-containing vectors, but the headline test should not pass on an empty `return`.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

…tion The harness these three files inherited had two blind spots that let a real regression pass. **Every parameter shared one fuzz value.** The assertion swallowed the throw (`try { build() } catch { return }`), so the moment one parameter was guarded the whole vector was skipped and its siblings stopped being tested. Reverting `targetUserId` in `x_manage_block` — leaving it completely unguarded — kept the suite at 622/622 green. Discovery now enumerates (tool, parameter) pairs and fuzzes one at a time with every sibling held safe; that same revert now fails 15 assertions. Finding the second identifier at all needs the branch explored: `targetUserId` only appears on the `action === 'unblock'` path, and `action` declares no enum, so the literals are harvested from the URL builder's own source. That keeps it self-maintaining — a new branch value is picked up without editing the tests. **A shape check cannot see a trailing dot.** `https://host/a/.` normalizes to `https://host/a/`, preserving the segment count and every other segment, and most of these routes end in the guarded id — including DELETE routes. Reverting `tailscale_delete_device` produced no `cannot reshape the path with "."` failure at all. Rejection is now asserted directly per parameter, so that same revert fails 12 assertions instead of 2. Also drops `ToolConfig<any, any>` and `url(fill as any)` for a structural `PathTool` narrowed by a type guard, and surfaces any tool whose URL cannot be built from all-safe values rather than letting it fall out of the suite unseen. 79 pairs (tailscale 26, spotify 24, x 29), matching the guarded call sites exactly. 2653 assertions, all green.

Several X tools carry the same identifier in the path on one branch and in the body on the other: `x_manage_block` sends `targetUserId` as a path segment to unblock and as a body field to block. Guarding only the path left the two branches disagreeing about what a caller may send — a numeric id that unblocked successfully threw a bare `TypeError` from `params.targetUserId.trim()` when used to block. Adds `xIdBodyValue`, which defers to `safeUrlPathSegment` and decodes the result rather than restating which value kinds an id may take, so the path and body rules cannot drift. Percent-encoding is a path concern; a JSON body carries the raw value. Applied to the six body sites across `create_bookmark`, `manage_block`, `manage_follow`, `manage_mute`, `manage_like`, and `manage_retweet`. Also makes the harness's shape assertion non-vacuous: it caught every throw and returned, so an unrelated `TypeError` would have read as a pass. It now asserts the error names the parameter under test before returning.

waleedlatif1 · 2026-08-29T04:55:52Z

@greptile review

waleedlatif1 · 2026-08-29T04:56:03Z

@cubic-dev-ai review

cubic-dev-ai · 2026-08-29T04:56:09Z

@cubic-dev-ai review

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

…ness The body suite reached `request.body` through `as unknown as`, which is the same type erasure the harness had already removed everywhere else. `PathTool` carries a narrowed `body` member, so the accessor reads it directly. Also fixes escaped backticks that leaked into the `xIdBodyValue` TSDoc.

waleedlatif1 · 2026-08-29T04:59:13Z

@greptile review

waleedlatif1 · 2026-08-29T04:59:20Z

@cubic-dev-ai review

cubic-dev-ai · 2026-08-29T04:59:24Z

@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 across 75 files

Confidence score: 4/5

apps/sim/tools/tailscale/suspend_user.ts returns a numeric userId unchanged after accepting it, which can violate the declared output shape for JSON tool calls; convert the value to a string before returning it.
apps/sim/tools/tailscale/path_safety.test.ts has documentation that conflicts with the slash-bearing cases in REJECTED_IDS, which could mislead future updates or regression diagnosis; align the comment with the suite’s actual expectations.
apps/sim/tools/spotify/path_safety.test.ts claims to verify discovery of every identifier in multi-ID routes but asserts that no such route exists, weakening confidence in the intended coverage; reconcile the test name, documentation, and assertion.

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/tailscale/suspend_user.ts">

<violation number="1" location="apps/sim/tools/tailscale/suspend_user.ts:48">
P2: When a JSON tool call supplies a numeric `userId`, this guard accepts it but the success response returns the number unchanged. Convert `userId` to a string before returning it so the result matches the declared output contract.</violation>
</file>

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

<violation number="1" location="apps/sim/tools/tailscale/path_safety.test.ts:7">
P3: The doc comment says reverting a guard to encodeURIComponent "turns *only* the `.` and `..` cases red — every slash-bearing vector still passes." That is not what the suite does: `REJECTED_IDS` includes slash-bearing vectors (`../../users/12345`, `device/../../tailnet/victim.com/acl`, `\..\..`) and the `rejects %j outright` test asserts these throw. Encoding does not throw, so under an encode-only regression those cases go red too. Only the shape-based `cannot reshape the path` test is dot-specific. Correct the comment so a maintainer does not conclude the slash-rejection checks are the wrong place to rely on for catching the regression.</violation>
</file>

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

<violation number="1" location="apps/sim/tools/spotify/path_safety.test.ts:285">
P3: This test's name and doc comment say it proves the harness discovers every identifier of a multi-ID route, but it asserts the opposite: `expect(multiId).toHaveLength(0)` requires that no tool have two path-reaching params. Under-discovery of a second id keeps the per-tool count at 1 and the assertion green, so the suite never actually verifies multi-ID discovery — it only fails when a second id is successfully found. Either document the zero-multi-ID invariant (Spotify routes each carry one resource id) or assert that every id present in PATH_PARAMS is exercised, matching the comment's claim.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

cubic-dev-ai

1 issue found and verified against the latest diff

Confidence score: 5/5

In apps/sim/tools/spotify/path_safety.test.ts, the multi-ID route test asserts that no matching Spotify tool exists despite its name and comments, so it may pass without validating identifier discovery; align the expectation with the intended behavior and add a matching fixture if needed.

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/spotify/path_safety.test.ts">

<violation number="1" location="apps/sim/tools/spotify/path_safety.test.ts:285">
P3: The test named "discovers every identifier of a multi-ID route, not just the first" asserts the opposite of what its name and comments claim: `expect(multiId).toHaveLength(0)` passes only when no Spotify tool has more than one path parameter. Every tool's URL here interpolates exactly one id (confirmed across all 24 guarded call sites), so this assertion merely pins the current single-ID state. It never exercises or verifies the multi-ID discovery machinery that `pathParamsOf`/`branchLiteralsOf` and the attached comments advertise, and it retrogrades to red if a legitimate multi-ID tool is ever added and correctly enumerated — failing the right implementation instead of validating it. The assertion and the stated intent (covering a second identifier such as `targetUserId` on a DELETE branch) are mutually contradictory.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

The traversal assertion exempted the slot under test — `if (segment === TARGET) return` — so it proved the path kept its length and its surrounding segments but never checked what landed in the guarded position. A *balanced* vector walks straight through that: `783214/../999999` on an unguarded `x_delete_tweet` yields ["", "2", "tweets", "999999"] vs ["", "2", "tweets", "SAFEID"] Same length, same surrounding segments, only the exempted slot differing — and the request now deletes a different post. The assertion now pins the slot to `encodeURIComponent(value.trim())`, and one balanced vector is added per service so the property is exercised directly. This also closes a second blind spot for free. A bare `.` collapses the slot to an empty segment without changing the count, so `cannot reshape the path with "."` could never fail before; it does now. Scoped reverts, failures per guard: `tailscale_delete_device` 12 to 14, `spotify_get_playlist` 17 to 23, `x_delete_tweet` 22 to 27.

Widening the guard to accept a numeric id made an existing echo reachable. Eight Tailscale tools return the id they were given straight back in their success output, declared `type: 'string'`. Before this PR a numeric `userId` threw from `params.userId.trim()` in the URL builder and never got that far; now the request succeeds and the echo emitted a number, turning a hard failure into a quiet contract violation. `String(params?.id ?? '')` is sufficient rather than a second guard call: the URL builder has already run and rejected every kind but string, number, and bigint by the time `transformResponse` executes, so the coercion cannot invent a value. `ECHOED_ID_TOOLS` pins it, and reverting one site turns it red. Also corrects two things the tests claimed but did not do: - The Tailscale header said reverting a guard to `encodeURIComponent` leaves "every slash-bearing vector still passing". That stopped being true once `REJECTED_IDS` began asserting those vectors are refused outright; the claim now describes the shape assertions specifically, which is where it holds. - `discovers every identifier of a multi-ID route` asserted a pinned count, which for Spotify is zero — a name that read as the opposite of the assertion. Renamed to say what it pins.

waleedlatif1 · 2026-08-29T05:14:32Z

@greptile review

waleedlatif1 · 2026-08-29T05:14:39Z

@cubic-dev-ai review

cubic-dev-ai · 2026-08-29T05:14:44Z

@cubic-dev-ai review

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

cubic-dev-ai

No issues found across 75 files

Confidence score: 5/5

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

_{Re-trigger cubic}

waleedlatif1 · 2026-08-29T07:15:54Z

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/tailscale-spotify-x-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.

vercel Bot temporarily deployed to Preview August 29, 2026 03:26 Inactive

greptile-apps Bot reviewed Aug 29, 2026

View reviewed changes

Comment thread apps/sim/tools/x/path_safety.test.ts Outdated

cubic-dev-ai Bot reviewed Aug 29, 2026

View reviewed changes

Comment thread apps/sim/tools/x/manage_block.ts

Comment thread apps/sim/tools/spotify/path_safety.test.ts Outdated

vercel Bot temporarily deployed to Preview August 29, 2026 04:24 Inactive

vercel Bot temporarily deployed to Preview August 29, 2026 04:54 Inactive

vercel Bot temporarily deployed to Preview August 29, 2026 04:59 Inactive

cubic-dev-ai Bot reviewed Aug 29, 2026

View reviewed changes

Comment thread apps/sim/tools/tailscale/suspend_user.ts

Comment thread apps/sim/tools/tailscale/path_safety.test.ts

Comment thread apps/sim/tools/spotify/path_safety.test.ts

cubic-dev-ai Bot reviewed Aug 29, 2026

View reviewed changes

Comment thread apps/sim/tools/spotify/path_safety.test.ts

vercel Bot temporarily deployed to Preview August 29, 2026 05:10 Inactive

vercel Bot temporarily deployed to Preview August 29, 2026 05:13 Inactive

cubic-dev-ai Bot reviewed Aug 29, 2026

View reviewed changes

waleedlatif1 closed this Aug 29, 2026

waleedlatif1 deleted the fix/tailscale-spotify-x-path-safety branch August 29, 2026 07:17

Uh oh!

Conversation

waleedlatif1 commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The defect

The fix

Body fields carry the same guard

The tests

Also hardened during review

Gates

Uh oh!

vercel Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented Aug 29, 2026

Uh oh!

waleedlatif1 commented Aug 29, 2026

Uh oh!

cubic-dev-ai Bot commented Aug 29, 2026

Uh oh!

waleedlatif1 commented Aug 29, 2026

Uh oh!

waleedlatif1 commented Aug 29, 2026

Uh oh!

cubic-dev-ai Bot commented Aug 29, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

waleedlatif1 commented Aug 29, 2026

Uh oh!

waleedlatif1 commented Aug 29, 2026

Uh oh!

cubic-dev-ai Bot commented Aug 29, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

waleedlatif1 commented Aug 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

waleedlatif1 commented Aug 29, 2026 •

edited

Loading

vercel Bot commented Aug 29, 2026 •

edited

Loading

greptile-apps Bot commented Aug 29, 2026 •

edited

Loading