From b0f53c6755205977b85464b76d043a268d57d208 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 20:24:02 -0700 Subject: [PATCH 1/9] fix(cloudflare,discord): reject path traversal in interpolated resource IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zone, account, ruleset, rule, record, tunnel, bucket, guild, channel, message, user, role, webhook, and invite IDs are `visibility: 'user-or-llm'`, so prompt injection controls them. Every one was interpolated straight into the request path, where a value like `../../accounts/victim` escapes the `/client/v4` or `/api/v10` prefix once `fetch` normalizes the URL — re-aiming an authenticated request, with the user's Cloudflare API token or the workspace's Discord bot token still attached, at a different resource. That includes DELETE zone, DELETE bucket, DELETE channel, DELETE role, and the ban routes. `encodeURIComponent` does not close this: `.` and `..` are unreserved, so they survive encoding untouched and the URL parser removes them as dot segments afterwards. The three call sites that already encoded (`bucketName`, `scriptName`, reaction `emoji`) were therefore just as exposed as the raw ones. Only rejecting the value works, so all 133 sites now route through `safeUrlPathSegment`. `get_zone_settings.ts` already rejected dot segments through its own local helper and is left as-is, since its error wording is asserted by an existing test. Nothing about legitimate input changes: no param visibility, no subBlock id, no tool metadata. Discord snowflakes — including ones arriving as JSON numbers or bigints — pass through as before, and one that `JSON.parse` already rounded past `MAX_SAFE_INTEGER` is now refused by name rather than silently addressing a neighbouring resource. Both new suites enumerate their tools from the service barrel, so a newly added tool with an unguarded path param fails CI without editing the test. --- .../cloudflare/create_access_application.ts | 3 +- .../tools/cloudflare/create_access_policy.ts | 3 +- .../cloudflare/create_access_service_token.ts | 3 +- .../sim/tools/cloudflare/create_dns_record.ts | 3 +- apps/sim/tools/cloudflare/create_r2_bucket.ts | 3 +- .../cloudflare/create_rate_limit_rule.ts | 3 +- apps/sim/tools/cloudflare/create_ruleset.ts | 4 +- .../tools/cloudflare/create_ruleset_rule.ts | 3 +- .../cloudflare/delete_access_application.ts | 3 +- .../tools/cloudflare/delete_access_policy.ts | 3 +- .../sim/tools/cloudflare/delete_dns_record.ts | 3 +- apps/sim/tools/cloudflare/delete_r2_bucket.ts | 3 +- .../tools/cloudflare/delete_ruleset_rule.ts | 3 +- apps/sim/tools/cloudflare/delete_zone.ts | 4 +- apps/sim/tools/cloudflare/dns_analytics.ts | 3 +- .../cloudflare/get_access_application.ts | 3 +- apps/sim/tools/cloudflare/get_r2_bucket.ts | 3 +- apps/sim/tools/cloudflare/get_ruleset.ts | 3 +- .../cloudflare/get_ruleset_entrypoint.ts | 3 +- apps/sim/tools/cloudflare/get_tunnel.ts | 3 +- .../cloudflare/get_tunnel_configuration.ts | 3 +- .../cloudflare/get_worker_script_settings.ts | 3 +- apps/sim/tools/cloudflare/get_zone.ts | 4 +- .../cloudflare/list_access_applications.ts | 3 +- .../tools/cloudflare/list_access_groups.ts | 3 +- .../list_access_identity_providers.ts | 3 +- .../tools/cloudflare/list_access_policies.ts | 3 +- .../cloudflare/list_access_service_tokens.ts | 3 +- .../sim/tools/cloudflare/list_certificates.ts | 3 +- apps/sim/tools/cloudflare/list_dns_records.ts | 3 +- .../list_managed_ruleset_overrides.ts | 3 +- apps/sim/tools/cloudflare/list_r2_buckets.ts | 3 +- .../tools/cloudflare/list_rate_limit_rules.ts | 3 +- apps/sim/tools/cloudflare/list_rulesets.ts | 3 +- apps/sim/tools/cloudflare/list_tunnels.ts | 3 +- .../tools/cloudflare/list_worker_routes.ts | 3 +- .../tools/cloudflare/list_worker_scripts.ts | 3 +- apps/sim/tools/cloudflare/path_safety.test.ts | 186 ++++++++++++++ apps/sim/tools/cloudflare/purge_cache.ts | 3 +- .../cloudflare/revoke_access_service_token.ts | 3 +- .../cloudflare/update_access_application.ts | 3 +- .../tools/cloudflare/update_access_policy.ts | 3 +- .../sim/tools/cloudflare/update_dns_record.ts | 3 +- .../cloudflare/update_rate_limit_rule.ts | 3 +- .../tools/cloudflare/update_ruleset_rule.ts | 3 +- .../tools/cloudflare/update_zone_setting.ts | 3 +- apps/sim/tools/discord/add_reaction.ts | 5 +- apps/sim/tools/discord/archive_thread.ts | 3 +- apps/sim/tools/discord/assign_role.ts | 3 +- apps/sim/tools/discord/ban_member.ts | 3 +- .../sim/tools/discord/bulk_delete_messages.ts | 3 +- apps/sim/tools/discord/create_channel.ts | 3 +- apps/sim/tools/discord/create_invite.ts | 3 +- apps/sim/tools/discord/create_role.ts | 3 +- apps/sim/tools/discord/create_thread.ts | 5 +- apps/sim/tools/discord/create_webhook.ts | 3 +- apps/sim/tools/discord/delete_channel.ts | 3 +- apps/sim/tools/discord/delete_invite.ts | 3 +- apps/sim/tools/discord/delete_message.ts | 3 +- apps/sim/tools/discord/delete_role.ts | 3 +- apps/sim/tools/discord/delete_webhook.ts | 3 +- apps/sim/tools/discord/edit_message.ts | 3 +- apps/sim/tools/discord/execute_webhook.ts | 3 +- apps/sim/tools/discord/get_channel.ts | 3 +- apps/sim/tools/discord/get_invite.ts | 3 +- apps/sim/tools/discord/get_member.ts | 3 +- apps/sim/tools/discord/get_messages.ts | 3 +- apps/sim/tools/discord/get_pinned_messages.ts | 3 +- apps/sim/tools/discord/get_server.ts | 3 +- apps/sim/tools/discord/get_user.ts | 3 +- apps/sim/tools/discord/get_webhook.ts | 3 +- apps/sim/tools/discord/join_thread.ts | 3 +- apps/sim/tools/discord/kick_member.ts | 3 +- apps/sim/tools/discord/leave_thread.ts | 3 +- apps/sim/tools/discord/list_channels.ts | 3 +- apps/sim/tools/discord/list_roles.ts | 3 +- apps/sim/tools/discord/path_safety.test.ts | 231 ++++++++++++++++++ apps/sim/tools/discord/pin_message.ts | 3 +- apps/sim/tools/discord/remove_reaction.ts | 7 +- apps/sim/tools/discord/remove_role.ts | 3 +- apps/sim/tools/discord/unban_member.ts | 3 +- apps/sim/tools/discord/unpin_message.ts | 3 +- apps/sim/tools/discord/update_channel.ts | 3 +- apps/sim/tools/discord/update_member.ts | 3 +- apps/sim/tools/discord/update_role.ts | 3 +- 85 files changed, 590 insertions(+), 87 deletions(-) create mode 100644 apps/sim/tools/cloudflare/path_safety.test.ts create mode 100644 apps/sim/tools/discord/path_safety.test.ts diff --git a/apps/sim/tools/cloudflare/create_access_application.ts b/apps/sim/tools/cloudflare/create_access_application.ts index 77550d13061..c295ff0c726 100644 --- a/apps/sim/tools/cloudflare/create_access_application.ts +++ b/apps/sim/tools/cloudflare/create_access_application.ts @@ -12,6 +12,7 @@ import { parseJsonObjectParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createAccessApplicationTool: ToolConfig< CloudflareCreateAccessApplicationParams, @@ -130,7 +131,7 @@ export const createAccessApplicationTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps`, method: 'POST', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/create_access_policy.ts b/apps/sim/tools/cloudflare/create_access_policy.ts index 6cb6d6db8ce..02b804ebfad 100644 --- a/apps/sim/tools/cloudflare/create_access_policy.ts +++ b/apps/sim/tools/cloudflare/create_access_policy.ts @@ -10,6 +10,7 @@ import { parseJsonArrayParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createAccessPolicyTool: ToolConfig< CloudflareCreateAccessPolicyParams, @@ -114,7 +115,7 @@ export const createAccessPolicyTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps/${params.appId.trim()}/policies`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps/${safeUrlPathSegment(params.appId, 'appId')}/policies`, method: 'POST', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/create_access_service_token.ts b/apps/sim/tools/cloudflare/create_access_service_token.ts index dc6487411c7..05b5727346a 100644 --- a/apps/sim/tools/cloudflare/create_access_service_token.ts +++ b/apps/sim/tools/cloudflare/create_access_service_token.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createAccessServiceTokenTool: ToolConfig< CloudflareCreateAccessServiceTokenParams, @@ -45,7 +46,7 @@ export const createAccessServiceTokenTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/service_tokens`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/service_tokens`, method: 'POST', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/create_dns_record.ts b/apps/sim/tools/cloudflare/create_dns_record.ts index 9fb75216ec8..17ecf8355b2 100644 --- a/apps/sim/tools/cloudflare/create_dns_record.ts +++ b/apps/sim/tools/cloudflare/create_dns_record.ts @@ -3,6 +3,7 @@ import type { CloudflareCreateDnsRecordResponse, } from '@/tools/cloudflare/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createDnsRecordTool: ToolConfig< CloudflareCreateDnsRecordParams, @@ -79,7 +80,7 @@ export const createDnsRecordTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/dns_records`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/dns_records`, method: 'POST', headers: (params) => ({ Authorization: `Bearer ${params.apiKey}`, diff --git a/apps/sim/tools/cloudflare/create_r2_bucket.ts b/apps/sim/tools/cloudflare/create_r2_bucket.ts index 66524353208..b9b7850bf1e 100644 --- a/apps/sim/tools/cloudflare/create_r2_bucket.ts +++ b/apps/sim/tools/cloudflare/create_r2_bucket.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createR2BucketTool: ToolConfig< CloudflareCreateR2BucketParams, @@ -58,7 +59,7 @@ export const createR2BucketTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/r2/buckets`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/r2/buckets`, method: 'POST', headers: (params) => { const headers = cloudflareHeaders(params.apiKey) diff --git a/apps/sim/tools/cloudflare/create_rate_limit_rule.ts b/apps/sim/tools/cloudflare/create_rate_limit_rule.ts index d75a3246bde..909d7ee7030 100644 --- a/apps/sim/tools/cloudflare/create_rate_limit_rule.ts +++ b/apps/sim/tools/cloudflare/create_rate_limit_rule.ts @@ -10,6 +10,7 @@ import { parseCsvParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createRateLimitRuleTool: ToolConfig< CloudflareCreateRateLimitRuleParams, @@ -111,7 +112,7 @@ export const createRateLimitRuleTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/${params.rulesetId.trim()}/rules`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/${safeUrlPathSegment(params.rulesetId, 'rulesetId')}/rules`, method: 'POST', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/create_ruleset.ts b/apps/sim/tools/cloudflare/create_ruleset.ts index 0666c44b499..1b03984d02d 100644 --- a/apps/sim/tools/cloudflare/create_ruleset.ts +++ b/apps/sim/tools/cloudflare/create_ruleset.ts @@ -10,6 +10,7 @@ import { parseJsonArrayParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createRulesetTool: ToolConfig< CloudflareCreateRulesetParams, @@ -70,7 +71,8 @@ export const createRulesetTool: ToolConfig< }, request: { - url: (params) => `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets`, + url: (params) => + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets`, method: 'POST', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/create_ruleset_rule.ts b/apps/sim/tools/cloudflare/create_ruleset_rule.ts index 4ff7bb34156..db025400ebe 100644 --- a/apps/sim/tools/cloudflare/create_ruleset_rule.ts +++ b/apps/sim/tools/cloudflare/create_ruleset_rule.ts @@ -10,6 +10,7 @@ import { parseJsonObjectParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const createRulesetRuleTool: ToolConfig< CloudflareCreateRulesetRuleParams, @@ -90,7 +91,7 @@ export const createRulesetRuleTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/${params.rulesetId.trim()}/rules`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/${safeUrlPathSegment(params.rulesetId, 'rulesetId')}/rules`, method: 'POST', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/delete_access_application.ts b/apps/sim/tools/cloudflare/delete_access_application.ts index 00c03ab1258..2f371d64a3e 100644 --- a/apps/sim/tools/cloudflare/delete_access_application.ts +++ b/apps/sim/tools/cloudflare/delete_access_application.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const deleteAccessApplicationTool: ToolConfig< CloudflareDeleteAccessApplicationParams, @@ -38,7 +39,7 @@ export const deleteAccessApplicationTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps/${params.appId.trim()}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps/${safeUrlPathSegment(params.appId, 'appId')}`, method: 'DELETE', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/delete_access_policy.ts b/apps/sim/tools/cloudflare/delete_access_policy.ts index f13889c6a3b..f632242fddb 100644 --- a/apps/sim/tools/cloudflare/delete_access_policy.ts +++ b/apps/sim/tools/cloudflare/delete_access_policy.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const deleteAccessPolicyTool: ToolConfig< CloudflareDeleteAccessPolicyParams, @@ -44,7 +45,7 @@ export const deleteAccessPolicyTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps/${params.appId.trim()}/policies/${params.policyId.trim()}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps/${safeUrlPathSegment(params.appId, 'appId')}/policies/${safeUrlPathSegment(params.policyId, 'policyId')}`, method: 'DELETE', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/delete_dns_record.ts b/apps/sim/tools/cloudflare/delete_dns_record.ts index d333255f682..9e666954aa7 100644 --- a/apps/sim/tools/cloudflare/delete_dns_record.ts +++ b/apps/sim/tools/cloudflare/delete_dns_record.ts @@ -3,6 +3,7 @@ import type { CloudflareDeleteDnsRecordResponse, } from '@/tools/cloudflare/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const deleteDnsRecordTool: ToolConfig< CloudflareDeleteDnsRecordParams, @@ -36,7 +37,7 @@ export const deleteDnsRecordTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/dns_records/${params.recordId.trim()}`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/dns_records/${safeUrlPathSegment(params.recordId, 'recordId')}`, method: 'DELETE', headers: (params) => ({ Authorization: `Bearer ${params.apiKey}`, diff --git a/apps/sim/tools/cloudflare/delete_r2_bucket.ts b/apps/sim/tools/cloudflare/delete_r2_bucket.ts index a239b0f1a35..86c440df5d1 100644 --- a/apps/sim/tools/cloudflare/delete_r2_bucket.ts +++ b/apps/sim/tools/cloudflare/delete_r2_bucket.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const deleteR2BucketTool: ToolConfig< CloudflareDeleteR2BucketParams, @@ -44,7 +45,7 @@ export const deleteR2BucketTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/r2/buckets/${encodeURIComponent(params.bucketName)}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/r2/buckets/${safeUrlPathSegment(params.bucketName, 'bucketName')}`, method: 'DELETE', headers: (params) => { const headers = cloudflareHeaders(params.apiKey) diff --git a/apps/sim/tools/cloudflare/delete_ruleset_rule.ts b/apps/sim/tools/cloudflare/delete_ruleset_rule.ts index 68ec2c45f9b..23f8a188abf 100644 --- a/apps/sim/tools/cloudflare/delete_ruleset_rule.ts +++ b/apps/sim/tools/cloudflare/delete_ruleset_rule.ts @@ -9,6 +9,7 @@ import { mapRuleset, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const deleteRulesetRuleTool: ToolConfig< CloudflareDeleteRulesetRuleParams, @@ -49,7 +50,7 @@ export const deleteRulesetRuleTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/${params.rulesetId.trim()}/rules/${params.ruleId.trim()}`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/${safeUrlPathSegment(params.rulesetId, 'rulesetId')}/rules/${safeUrlPathSegment(params.ruleId, 'ruleId')}`, method: 'DELETE', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/delete_zone.ts b/apps/sim/tools/cloudflare/delete_zone.ts index 06ddf1d42cb..77784cfb6a1 100644 --- a/apps/sim/tools/cloudflare/delete_zone.ts +++ b/apps/sim/tools/cloudflare/delete_zone.ts @@ -3,6 +3,7 @@ import type { CloudflareDeleteZoneResponse, } from '@/tools/cloudflare/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const deleteZoneTool: ToolConfig = { @@ -27,7 +28,8 @@ export const deleteZoneTool: ToolConfig `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}`, + url: (params) => + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}`, method: 'DELETE', headers: (params) => ({ Authorization: `Bearer ${params.apiKey}`, diff --git a/apps/sim/tools/cloudflare/dns_analytics.ts b/apps/sim/tools/cloudflare/dns_analytics.ts index b21fe318265..592ed47d798 100644 --- a/apps/sim/tools/cloudflare/dns_analytics.ts +++ b/apps/sim/tools/cloudflare/dns_analytics.ts @@ -5,6 +5,7 @@ import type { } from '@/tools/cloudflare/types' import { readCloudflareResponse } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const dnsAnalyticsTool: ToolConfig< CloudflareDnsAnalyticsParams, @@ -80,7 +81,7 @@ export const dnsAnalyticsTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/dns_analytics/report` + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/dns_analytics/report` ) if (params.since) url.searchParams.append('since', params.since) if (params.until) url.searchParams.append('until', params.until) diff --git a/apps/sim/tools/cloudflare/get_access_application.ts b/apps/sim/tools/cloudflare/get_access_application.ts index 71f1989c713..d20daae7853 100644 --- a/apps/sim/tools/cloudflare/get_access_application.ts +++ b/apps/sim/tools/cloudflare/get_access_application.ts @@ -9,6 +9,7 @@ import { mapAccessApplication, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getAccessApplicationTool: ToolConfig< CloudflareGetAccessApplicationParams, @@ -43,7 +44,7 @@ export const getAccessApplicationTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps/${params.appId.trim()}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps/${safeUrlPathSegment(params.appId, 'appId')}`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/get_r2_bucket.ts b/apps/sim/tools/cloudflare/get_r2_bucket.ts index f144a34f608..b46357dcd1f 100644 --- a/apps/sim/tools/cloudflare/get_r2_bucket.ts +++ b/apps/sim/tools/cloudflare/get_r2_bucket.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getR2BucketTool: ToolConfig = { @@ -42,7 +43,7 @@ export const getR2BucketTool: ToolConfig - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/r2/buckets/${encodeURIComponent(params.bucketName)}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/r2/buckets/${safeUrlPathSegment(params.bucketName, 'bucketName')}`, method: 'GET', headers: (params) => { const headers = cloudflareHeaders(params.apiKey) diff --git a/apps/sim/tools/cloudflare/get_ruleset.ts b/apps/sim/tools/cloudflare/get_ruleset.ts index badc225f26e..29873788447 100644 --- a/apps/sim/tools/cloudflare/get_ruleset.ts +++ b/apps/sim/tools/cloudflare/get_ruleset.ts @@ -9,6 +9,7 @@ import { mapRuleset, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getRulesetTool: ToolConfig = { id: 'cloudflare_get_ruleset', @@ -40,7 +41,7 @@ export const getRulesetTool: ToolConfig - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/${params.rulesetId.trim()}`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/${safeUrlPathSegment(params.rulesetId, 'rulesetId')}`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/get_ruleset_entrypoint.ts b/apps/sim/tools/cloudflare/get_ruleset_entrypoint.ts index 87e460390a3..0a932144828 100644 --- a/apps/sim/tools/cloudflare/get_ruleset_entrypoint.ts +++ b/apps/sim/tools/cloudflare/get_ruleset_entrypoint.ts @@ -9,6 +9,7 @@ import { mapRuleset, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getRulesetEntrypointTool: ToolConfig< CloudflareGetRulesetEntrypointParams, @@ -44,7 +45,7 @@ export const getRulesetEntrypointTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/phases/${params.phase.trim()}/entrypoint`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/phases/${safeUrlPathSegment(params.phase, 'phase')}/entrypoint`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/get_tunnel.ts b/apps/sim/tools/cloudflare/get_tunnel.ts index cf0faf957f0..d3a9f38857d 100644 --- a/apps/sim/tools/cloudflare/get_tunnel.ts +++ b/apps/sim/tools/cloudflare/get_tunnel.ts @@ -1,6 +1,7 @@ import type { CloudflareGetTunnelParams, CloudflareTunnelResponse } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getTunnelTool: ToolConfig = { id: 'cloudflare_get_tunnel', @@ -32,7 +33,7 @@ export const getTunnelTool: ToolConfig - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/cfd_tunnel/${params.tunnelId.trim()}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/cfd_tunnel/${safeUrlPathSegment(params.tunnelId, 'tunnelId')}`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/get_tunnel_configuration.ts b/apps/sim/tools/cloudflare/get_tunnel_configuration.ts index 8b76c3590a8..55ba4c878bb 100644 --- a/apps/sim/tools/cloudflare/get_tunnel_configuration.ts +++ b/apps/sim/tools/cloudflare/get_tunnel_configuration.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getTunnelConfigurationTool: ToolConfig< CloudflareGetTunnelConfigurationParams, @@ -38,7 +39,7 @@ export const getTunnelConfigurationTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/cfd_tunnel/${params.tunnelId.trim()}/configurations`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/cfd_tunnel/${safeUrlPathSegment(params.tunnelId, 'tunnelId')}/configurations`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/get_worker_script_settings.ts b/apps/sim/tools/cloudflare/get_worker_script_settings.ts index dcc787391cf..d2502a3491e 100644 --- a/apps/sim/tools/cloudflare/get_worker_script_settings.ts +++ b/apps/sim/tools/cloudflare/get_worker_script_settings.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getWorkerScriptSettingsTool: ToolConfig< CloudflareGetWorkerScriptSettingsParams, @@ -38,7 +39,7 @@ export const getWorkerScriptSettingsTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/workers/scripts/${encodeURIComponent(params.scriptName)}/settings`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/workers/scripts/${safeUrlPathSegment(params.scriptName, 'scriptName')}/settings`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/get_zone.ts b/apps/sim/tools/cloudflare/get_zone.ts index 5d58a922991..71cf2edfe88 100644 --- a/apps/sim/tools/cloudflare/get_zone.ts +++ b/apps/sim/tools/cloudflare/get_zone.ts @@ -1,5 +1,6 @@ import type { CloudflareGetZoneParams, CloudflareGetZoneResponse } from '@/tools/cloudflare/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const getZoneTool: ToolConfig = { id: 'cloudflare_get_zone', @@ -23,7 +24,8 @@ export const getZoneTool: ToolConfig `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}`, + url: (params) => + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}`, method: 'GET', headers: (params) => ({ Authorization: `Bearer ${params.apiKey}`, diff --git a/apps/sim/tools/cloudflare/list_access_applications.ts b/apps/sim/tools/cloudflare/list_access_applications.ts index 97938fcd247..0ef94f865fd 100644 --- a/apps/sim/tools/cloudflare/list_access_applications.ts +++ b/apps/sim/tools/cloudflare/list_access_applications.ts @@ -9,6 +9,7 @@ import { mapAccessApplication, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listAccessApplicationsTool: ToolConfig< CloudflareListAccessApplicationsParams, @@ -80,7 +81,7 @@ export const listAccessApplicationsTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps` + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps` ) appendParam(url, 'name', params.name) appendParam(url, 'domain', params.domain) diff --git a/apps/sim/tools/cloudflare/list_access_groups.ts b/apps/sim/tools/cloudflare/list_access_groups.ts index 65ed2b03cb2..3e6f33160f2 100644 --- a/apps/sim/tools/cloudflare/list_access_groups.ts +++ b/apps/sim/tools/cloudflare/list_access_groups.ts @@ -10,6 +10,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listAccessGroupsTool: ToolConfig< CloudflareListAccessGroupsParams, @@ -63,7 +64,7 @@ export const listAccessGroupsTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/groups` + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/groups` ) appendParam(url, 'name', params.name) appendParam(url, 'search', params.search) diff --git a/apps/sim/tools/cloudflare/list_access_identity_providers.ts b/apps/sim/tools/cloudflare/list_access_identity_providers.ts index c6568fcb499..ebcf90a0ea0 100644 --- a/apps/sim/tools/cloudflare/list_access_identity_providers.ts +++ b/apps/sim/tools/cloudflare/list_access_identity_providers.ts @@ -9,6 +9,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listAccessIdentityProvidersTool: ToolConfig< CloudflareListAccessIdentityProvidersParams, @@ -37,7 +38,7 @@ export const listAccessIdentityProvidersTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/identity_providers`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/identity_providers`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/list_access_policies.ts b/apps/sim/tools/cloudflare/list_access_policies.ts index 0ec5e0c645c..fe3006eed71 100644 --- a/apps/sim/tools/cloudflare/list_access_policies.ts +++ b/apps/sim/tools/cloudflare/list_access_policies.ts @@ -9,6 +9,7 @@ import { mapAccessPolicy, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listAccessPoliciesTool: ToolConfig< CloudflareListAccessPoliciesParams, @@ -56,7 +57,7 @@ export const listAccessPoliciesTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps/${params.appId.trim()}/policies` + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps/${safeUrlPathSegment(params.appId, 'appId')}/policies` ) appendParam(url, 'page', params.page) appendParam(url, 'per_page', params.per_page) diff --git a/apps/sim/tools/cloudflare/list_access_service_tokens.ts b/apps/sim/tools/cloudflare/list_access_service_tokens.ts index 75e0737752a..c5c8528d864 100644 --- a/apps/sim/tools/cloudflare/list_access_service_tokens.ts +++ b/apps/sim/tools/cloudflare/list_access_service_tokens.ts @@ -10,6 +10,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listAccessServiceTokensTool: ToolConfig< CloudflareListAccessServiceTokensParams, @@ -63,7 +64,7 @@ export const listAccessServiceTokensTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/service_tokens` + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/service_tokens` ) appendParam(url, 'name', params.name) appendParam(url, 'search', params.search) diff --git a/apps/sim/tools/cloudflare/list_certificates.ts b/apps/sim/tools/cloudflare/list_certificates.ts index 4a8d104b289..bec38fd4a6e 100644 --- a/apps/sim/tools/cloudflare/list_certificates.ts +++ b/apps/sim/tools/cloudflare/list_certificates.ts @@ -5,6 +5,7 @@ import type { } from '@/tools/cloudflare/types' import { readCloudflareResponse } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listCertificatesTool: ToolConfig< CloudflareListCertificatesParams, @@ -58,7 +59,7 @@ export const listCertificatesTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/ssl/certificate_packs` + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/ssl/certificate_packs` ) if (params.status) url.searchParams.append('status', params.status) if (params.page) url.searchParams.append('page', String(params.page)) diff --git a/apps/sim/tools/cloudflare/list_dns_records.ts b/apps/sim/tools/cloudflare/list_dns_records.ts index 1c726d4c990..5f4a3c1700b 100644 --- a/apps/sim/tools/cloudflare/list_dns_records.ts +++ b/apps/sim/tools/cloudflare/list_dns_records.ts @@ -5,6 +5,7 @@ import type { } from '@/tools/cloudflare/types' import { readCloudflareResponse } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listDnsRecordsTool: ToolConfig< CloudflareListDnsRecordsParams, @@ -112,7 +113,7 @@ export const listDnsRecordsTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/dns_records` + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/dns_records` ) if (params.type) url.searchParams.append('type', params.type) if (params.name) url.searchParams.append('name.exact', params.name) diff --git a/apps/sim/tools/cloudflare/list_managed_ruleset_overrides.ts b/apps/sim/tools/cloudflare/list_managed_ruleset_overrides.ts index 603a58d2990..3036ecae482 100644 --- a/apps/sim/tools/cloudflare/list_managed_ruleset_overrides.ts +++ b/apps/sim/tools/cloudflare/list_managed_ruleset_overrides.ts @@ -9,6 +9,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listManagedRulesetOverridesTool: ToolConfig< CloudflareListManagedRulesetOverridesParams, @@ -37,7 +38,7 @@ export const listManagedRulesetOverridesTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/phases/http_request_firewall_managed/entrypoint`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/phases/http_request_firewall_managed/entrypoint`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/list_r2_buckets.ts b/apps/sim/tools/cloudflare/list_r2_buckets.ts index cd73043771d..3e366245f2e 100644 --- a/apps/sim/tools/cloudflare/list_r2_buckets.ts +++ b/apps/sim/tools/cloudflare/list_r2_buckets.ts @@ -10,6 +10,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listR2BucketsTool: ToolConfig< CloudflareListR2BucketsParams, @@ -75,7 +76,7 @@ export const listR2BucketsTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/r2/buckets` + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/r2/buckets` ) appendParam(url, 'name_contains', params.name_contains) appendParam(url, 'start_after', params.start_after) diff --git a/apps/sim/tools/cloudflare/list_rate_limit_rules.ts b/apps/sim/tools/cloudflare/list_rate_limit_rules.ts index 09a723c58ec..a0750f61914 100644 --- a/apps/sim/tools/cloudflare/list_rate_limit_rules.ts +++ b/apps/sim/tools/cloudflare/list_rate_limit_rules.ts @@ -9,6 +9,7 @@ import { mapRuleset, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listRateLimitRulesTool: ToolConfig< CloudflareListRateLimitRulesParams, @@ -37,7 +38,7 @@ export const listRateLimitRulesTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/phases/http_ratelimit/entrypoint`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/phases/http_ratelimit/entrypoint`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/list_rulesets.ts b/apps/sim/tools/cloudflare/list_rulesets.ts index 25cffb0a1ee..db001e2bbba 100644 --- a/apps/sim/tools/cloudflare/list_rulesets.ts +++ b/apps/sim/tools/cloudflare/list_rulesets.ts @@ -10,6 +10,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listRulesetsTool: ToolConfig< CloudflareListRulesetsParams, @@ -52,7 +53,7 @@ export const listRulesetsTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets` + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets` ) appendParam(url, 'per_page', params.per_page) appendParam(url, 'cursor', params.cursor) diff --git a/apps/sim/tools/cloudflare/list_tunnels.ts b/apps/sim/tools/cloudflare/list_tunnels.ts index 14f8821e1da..0644dd2b079 100644 --- a/apps/sim/tools/cloudflare/list_tunnels.ts +++ b/apps/sim/tools/cloudflare/list_tunnels.ts @@ -10,6 +10,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listTunnelsTool: ToolConfig< CloudflareListTunnelsParams, @@ -105,7 +106,7 @@ export const listTunnelsTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/cfd_tunnel` + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/cfd_tunnel` ) appendParam(url, 'name', params.name) appendParam(url, 'status', params.status) diff --git a/apps/sim/tools/cloudflare/list_worker_routes.ts b/apps/sim/tools/cloudflare/list_worker_routes.ts index 96dffef09f9..dabedefc2de 100644 --- a/apps/sim/tools/cloudflare/list_worker_routes.ts +++ b/apps/sim/tools/cloudflare/list_worker_routes.ts @@ -9,6 +9,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listWorkerRoutesTool: ToolConfig< CloudflareListWorkerRoutesParams, @@ -38,7 +39,7 @@ export const listWorkerRoutesTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/workers/routes`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/workers/routes`, method: 'GET', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/list_worker_scripts.ts b/apps/sim/tools/cloudflare/list_worker_scripts.ts index c41bfdd2542..661506215aa 100644 --- a/apps/sim/tools/cloudflare/list_worker_scripts.ts +++ b/apps/sim/tools/cloudflare/list_worker_scripts.ts @@ -10,6 +10,7 @@ import { readCloudflareResponse, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const listWorkerScriptsTool: ToolConfig< CloudflareListWorkerScriptsParams, @@ -46,7 +47,7 @@ export const listWorkerScriptsTool: ToolConfig< request: { url: (params) => { const url = new URL( - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/workers/scripts` + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/workers/scripts` ) appendParam(url, 'tags', params.tags) return url.toString() diff --git a/apps/sim/tools/cloudflare/path_safety.test.ts b/apps/sim/tools/cloudflare/path_safety.test.ts new file mode 100644 index 00000000000..ef7b17528b7 --- /dev/null +++ b/apps/sim/tools/cloudflare/path_safety.test.ts @@ -0,0 +1,186 @@ +/** + * @vitest-environment node + * + * Guards every Cloudflare tool against path traversal through an LLM-writable + * ID that gets interpolated into the request path. + * + * Zone, account, ruleset, rule, record, tunnel, and bucket IDs are + * `visibility: 'user-or-llm'`, so prompt injection controls them. Interpolating + * one raw let a value like `../../accounts/victim` escape its `/client/v4` + * prefix once `fetch` normalized the URL, re-aiming the request — and the + * user's Cloudflare API token — at an arbitrary Cloudflare resource, including + * on the DELETE zone and DELETE bucket routes. + * + * `encodeURIComponent` is NOT enough, which is why the vector list below keeps + * the bare `.` and `..` segments: both are made of unreserved characters, so + * they survive encoding untouched and the URL parser then removes them as dot + * segments, popping one path segment off a fixed host. Every assertion here + * resolves the built URL with `new URL(...)` — the same normalization `fetch` + * performs — rather than string-matching the template output, because string + * matching is exactly what let this through. + * + * The tool list is enumerated from the barrel, so a newly added Cloudflare tool + * that interpolates an unguarded ID fails here without anyone editing this file. + */ +import { describe, expect, it } from 'vitest' +import * as cloudflareTools from '@/tools/cloudflare' +import type { ToolConfig } from '@/tools/types' + +const API_ORIGIN = 'https://api.cloudflare.com' +const API_PREFIX = '/client/v4/' + +/** + * The bare `.` and `..` entries are the whole point: their omission is why an + * `encodeURIComponent`-only fix looks correct while the hole stays live. + */ +const TRAVERSAL_IDS = [ + '..', + '.', + ' .. ', + '../../accounts/victim-account', + '..%2f..%2faccounts/victim-account', + '023e105f4ecef8ad9ca31a8372d0c353/../../accounts/victim-account', + '023e105f4ecef8ad9ca31a8372d0c353?account_id=attacker', + '023e105f4ecef8ad9ca31a8372d0c353#fragment', + '023e105f4ecef8ad9ca31a8372d0c353/dns_records/../../../zones', + '\\..\\..', +] as const + +/** Values a real user legitimately supplies; none may be rejected or altered. */ +const LEGITIMATE_IDS = [ + '023e105f4ecef8ad9ca31a8372d0c353', + 'a1b2c3d4e5f60718293a4b5c6d7e8f90', + 'http_request_firewall_custom', + 'always_use_https', + 'my-bucket', + 'my.bucket.name', + 'worker-script-v2', + '..foo', + 'foo..', + 'v1.2.3', +] as const + +const SAFE_ID = 'SAFEID' + +type AnyTool = ToolConfig + +function isCloudflareTool(value: unknown): value is AnyTool { + return ( + typeof value === 'object' && + value !== null && + typeof (value as AnyTool).id === 'string' && + (value as AnyTool).id.startsWith('cloudflare_') + ) +} + +/** + * Builds a param object for a tool, filling every declared string param with + * `value` so whichever one reaches the path is exercised. + */ +function buildParams(tool: AnyTool, value: string): Record { + const params: Record = { apiKey: 'cf-token' } + for (const [name, def] of Object.entries(tool.params ?? {})) { + if (name === 'apiKey') continue + const type = (def as { type?: string }).type + if (type === 'json' || type === 'array') { + params[name] = [] + } else if (type === 'number') { + params[name] = 1 + } else if (type === 'boolean') { + params[name] = false + } else { + params[name] = value + } + } + return params +} + +function buildUrl(tool: AnyTool, value: string): URL { + const url = tool.request?.url + if (typeof url !== 'function') { + throw new Error(`${tool.id} does not build its URL from params`) + } + return new URL(url(buildParams(tool, value) as any)) +} + +function segmentsOf(pathname: string): string[] { + return pathname.split('/') +} + +const DYNAMIC_PATH_TOOLS = Object.values(cloudflareTools) + .filter(isCloudflareTool) + .filter((tool) => typeof tool.request?.url === 'function') + .filter((tool) => { + try { + return buildUrl(tool, SAFE_ID).pathname.includes(SAFE_ID) + } catch { + return false + } + }) + .map((tool) => ({ name: tool.id, tool })) + +describe('cloudflare path-ID traversal safety', () => { + it('covers every Cloudflare tool that interpolates an ID into its path', () => { + expect(DYNAMIC_PATH_TOOLS.length).toBeGreaterThanOrEqual(40) + }) + + describe.each(DYNAMIC_PATH_TOOLS)('$name', ({ tool }) => { + const baseline = segmentsOf(buildUrl(tool, SAFE_ID).pathname) + + it.each(TRAVERSAL_IDS)('cannot reshape the path with %j', (value) => { + let url: URL + try { + url = buildUrl(tool, value) + } catch { + return + } + + expect(url.origin).toBe(API_ORIGIN) + expect(url.pathname.startsWith(API_PREFIX)).toBe(true) + + const actual = segmentsOf(url.pathname) + expect(actual).toHaveLength(baseline.length) + baseline.forEach((segment, index) => { + if (segment === SAFE_ID) return + expect(actual[index]).toBe(segment) + }) + }) + + it.each(TRAVERSAL_IDS)('never smuggles a query parameter via %j', (value) => { + let url: URL + try { + url = buildUrl(tool, value) + } catch { + return + } + + expect(url.searchParams.get('account_id')).toBeNull() + }) + + it('rejects a bare dot-dot segment instead of silently popping the prefix', () => { + expect(() => buildUrl(tool, '..')).toThrow(/path traversal is not allowed/) + }) + + it('rejects a bare dot segment', () => { + expect(() => buildUrl(tool, '.')).toThrow(/path traversal is not allowed/) + }) + + it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { + const actual = segmentsOf(buildUrl(tool, value).pathname) + + expect(actual).toHaveLength(baseline.length) + baseline.forEach((segment, index) => { + expect(actual[index]).toBe(segment === SAFE_ID ? value : segment) + }) + }) + + it('trims surrounding whitespace off a legitimate ID', () => { + const actual = segmentsOf(buildUrl(tool, ' 023e105f4ecef8ad9ca31a8372d0c353 ').pathname) + + baseline.forEach((segment, index) => { + if (segment !== SAFE_ID) return + expect(actual[index]).toBe('023e105f4ecef8ad9ca31a8372d0c353') + }) + }) + }) +}) diff --git a/apps/sim/tools/cloudflare/purge_cache.ts b/apps/sim/tools/cloudflare/purge_cache.ts index 144628b92b9..73d98810531 100644 --- a/apps/sim/tools/cloudflare/purge_cache.ts +++ b/apps/sim/tools/cloudflare/purge_cache.ts @@ -3,6 +3,7 @@ import type { CloudflarePurgeCacheResponse, } from '@/tools/cloudflare/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const purgeCacheTool: ToolConfig = { @@ -60,7 +61,7 @@ export const purgeCacheTool: ToolConfig - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/purge_cache`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/purge_cache`, method: 'POST', headers: (params) => ({ Authorization: `Bearer ${params.apiKey}`, diff --git a/apps/sim/tools/cloudflare/revoke_access_service_token.ts b/apps/sim/tools/cloudflare/revoke_access_service_token.ts index 2215d8d777a..950f7307e1b 100644 --- a/apps/sim/tools/cloudflare/revoke_access_service_token.ts +++ b/apps/sim/tools/cloudflare/revoke_access_service_token.ts @@ -4,6 +4,7 @@ import type { } from '@/tools/cloudflare/types' import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const revokeAccessServiceTokenTool: ToolConfig< CloudflareRevokeAccessServiceTokenParams, @@ -38,7 +39,7 @@ export const revokeAccessServiceTokenTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/service_tokens/${params.serviceTokenId.trim()}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/service_tokens/${safeUrlPathSegment(params.serviceTokenId, 'serviceTokenId')}`, method: 'DELETE', headers: (params) => cloudflareHeaders(params.apiKey), }, diff --git a/apps/sim/tools/cloudflare/update_access_application.ts b/apps/sim/tools/cloudflare/update_access_application.ts index db78a62f8f4..d3430c85dfe 100644 --- a/apps/sim/tools/cloudflare/update_access_application.ts +++ b/apps/sim/tools/cloudflare/update_access_application.ts @@ -12,6 +12,7 @@ import { parseJsonObjectParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateAccessApplicationTool: ToolConfig< CloudflareUpdateAccessApplicationParams, @@ -135,7 +136,7 @@ export const updateAccessApplicationTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps/${params.appId.trim()}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps/${safeUrlPathSegment(params.appId, 'appId')}`, method: 'PUT', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/update_access_policy.ts b/apps/sim/tools/cloudflare/update_access_policy.ts index 529155a1d05..b6d01530926 100644 --- a/apps/sim/tools/cloudflare/update_access_policy.ts +++ b/apps/sim/tools/cloudflare/update_access_policy.ts @@ -10,6 +10,7 @@ import { parseJsonArrayParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateAccessPolicyTool: ToolConfig< CloudflareUpdateAccessPolicyParams, @@ -119,7 +120,7 @@ export const updateAccessPolicyTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${params.accountId.trim()}/access/apps/${params.appId.trim()}/policies/${params.policyId.trim()}`, + `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/access/apps/${safeUrlPathSegment(params.appId, 'appId')}/policies/${safeUrlPathSegment(params.policyId, 'policyId')}`, method: 'PUT', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/update_dns_record.ts b/apps/sim/tools/cloudflare/update_dns_record.ts index 7f86743d0a0..2ebdc87e695 100644 --- a/apps/sim/tools/cloudflare/update_dns_record.ts +++ b/apps/sim/tools/cloudflare/update_dns_record.ts @@ -3,6 +3,7 @@ import type { CloudflareUpdateDnsRecordResponse, } from '@/tools/cloudflare/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateDnsRecordTool: ToolConfig< CloudflareUpdateDnsRecordParams, @@ -85,7 +86,7 @@ export const updateDnsRecordTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/dns_records/${params.recordId.trim()}`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/dns_records/${safeUrlPathSegment(params.recordId, 'recordId')}`, method: 'PATCH', headers: (params) => ({ Authorization: `Bearer ${params.apiKey}`, diff --git a/apps/sim/tools/cloudflare/update_rate_limit_rule.ts b/apps/sim/tools/cloudflare/update_rate_limit_rule.ts index bedd37d6108..9430fae6798 100644 --- a/apps/sim/tools/cloudflare/update_rate_limit_rule.ts +++ b/apps/sim/tools/cloudflare/update_rate_limit_rule.ts @@ -11,6 +11,7 @@ import { parseJsonObjectParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateRateLimitRuleTool: ToolConfig< CloudflareUpdateRateLimitRuleParams, @@ -137,7 +138,7 @@ export const updateRateLimitRuleTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/${params.rulesetId.trim()}/rules/${params.ruleId.trim()}`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/${safeUrlPathSegment(params.rulesetId, 'rulesetId')}/rules/${safeUrlPathSegment(params.ruleId, 'ruleId')}`, method: 'PATCH', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/update_ruleset_rule.ts b/apps/sim/tools/cloudflare/update_ruleset_rule.ts index 918dd7e557c..779965c6488 100644 --- a/apps/sim/tools/cloudflare/update_ruleset_rule.ts +++ b/apps/sim/tools/cloudflare/update_ruleset_rule.ts @@ -10,6 +10,7 @@ import { parseJsonObjectParam, } from '@/tools/cloudflare/utils' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateRulesetRuleTool: ToolConfig< CloudflareUpdateRulesetRuleParams, @@ -104,7 +105,7 @@ export const updateRulesetRuleTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/rulesets/${params.rulesetId.trim()}/rules/${params.ruleId.trim()}`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/rulesets/${safeUrlPathSegment(params.rulesetId, 'rulesetId')}/rules/${safeUrlPathSegment(params.ruleId, 'ruleId')}`, method: 'PATCH', headers: (params) => cloudflareHeaders(params.apiKey), body: (params) => { diff --git a/apps/sim/tools/cloudflare/update_zone_setting.ts b/apps/sim/tools/cloudflare/update_zone_setting.ts index c7ba09db206..4625a251ed7 100644 --- a/apps/sim/tools/cloudflare/update_zone_setting.ts +++ b/apps/sim/tools/cloudflare/update_zone_setting.ts @@ -3,6 +3,7 @@ import type { CloudflareUpdateZoneSettingResponse, } from '@/tools/cloudflare/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const updateZoneSettingTool: ToolConfig< CloudflareUpdateZoneSettingParams, @@ -44,7 +45,7 @@ export const updateZoneSettingTool: ToolConfig< request: { url: (params) => - `https://api.cloudflare.com/client/v4/zones/${params.zoneId.trim()}/settings/${params.settingId.trim()}`, + `https://api.cloudflare.com/client/v4/zones/${safeUrlPathSegment(params.zoneId, 'zoneId')}/settings/${safeUrlPathSegment(params.settingId, 'settingId')}`, method: 'PATCH', headers: (params) => ({ Authorization: `Bearer ${params.apiKey}`, diff --git a/apps/sim/tools/discord/add_reaction.ts b/apps/sim/tools/discord/add_reaction.ts index 42afc9a7394..bbb565338e9 100644 --- a/apps/sim/tools/discord/add_reaction.ts +++ b/apps/sim/tools/discord/add_reaction.ts @@ -1,5 +1,6 @@ import type { DiscordAddReactionParams, DiscordAddReactionResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordAddReactionTool: ToolConfig< DiscordAddReactionParams, @@ -45,8 +46,8 @@ export const discordAddReactionTool: ToolConfig< request: { url: (params: DiscordAddReactionParams) => { - const encodedEmoji = encodeURIComponent(params.emoji) - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/messages/${params.messageId.trim()}/reactions/${encodedEmoji}/@me` + const encodedEmoji = safeUrlPathSegment(params.emoji, 'emoji') + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(params.messageId, 'messageId')}/reactions/${encodedEmoji}/@me` }, method: 'PUT', headers: (params) => ({ diff --git a/apps/sim/tools/discord/archive_thread.ts b/apps/sim/tools/discord/archive_thread.ts index 67f7b79e01a..e81157fd716 100644 --- a/apps/sim/tools/discord/archive_thread.ts +++ b/apps/sim/tools/discord/archive_thread.ts @@ -3,6 +3,7 @@ import type { DiscordArchiveThreadResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordArchiveThreadTool: ToolConfig< DiscordArchiveThreadParams, @@ -42,7 +43,7 @@ export const discordArchiveThreadTool: ToolConfig< request: { url: (params: DiscordArchiveThreadParams) => { - return `https://discord.com/api/v10/channels/${params.threadId.trim()}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.threadId, 'threadId')}` }, method: 'PATCH', headers: (params) => ({ diff --git a/apps/sim/tools/discord/assign_role.ts b/apps/sim/tools/discord/assign_role.ts index 2fd161707b8..f8eeb0b72d5 100644 --- a/apps/sim/tools/discord/assign_role.ts +++ b/apps/sim/tools/discord/assign_role.ts @@ -1,5 +1,6 @@ import type { DiscordAssignRoleParams, DiscordAssignRoleResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordAssignRoleTool: ToolConfig = { @@ -37,7 +38,7 @@ export const discordAssignRoleTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/members/${params.userId.trim()}/roles/${params.roleId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/members/${safeUrlPathSegment(params.userId, 'userId')}/roles/${safeUrlPathSegment(params.roleId, 'roleId')}` }, method: 'PUT', headers: (params) => ({ diff --git a/apps/sim/tools/discord/ban_member.ts b/apps/sim/tools/discord/ban_member.ts index e998f04cf28..7659b4d7a38 100644 --- a/apps/sim/tools/discord/ban_member.ts +++ b/apps/sim/tools/discord/ban_member.ts @@ -1,5 +1,6 @@ import type { DiscordBanMemberParams, DiscordBanMemberResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordBanMemberTool: ToolConfig = { id: 'discord_ban_member', @@ -42,7 +43,7 @@ export const discordBanMemberTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/bans/${params.userId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/bans/${safeUrlPathSegment(params.userId, 'userId')}` }, method: 'PUT', headers: (params) => { diff --git a/apps/sim/tools/discord/bulk_delete_messages.ts b/apps/sim/tools/discord/bulk_delete_messages.ts index e73065e3a6c..cd2b0081b7d 100644 --- a/apps/sim/tools/discord/bulk_delete_messages.ts +++ b/apps/sim/tools/discord/bulk_delete_messages.ts @@ -3,6 +3,7 @@ import type { DiscordBulkDeleteMessagesResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordBulkDeleteMessagesTool: ToolConfig< DiscordBulkDeleteMessagesParams, @@ -43,7 +44,7 @@ export const discordBulkDeleteMessagesTool: ToolConfig< request: { url: (params: DiscordBulkDeleteMessagesParams) => { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/messages/bulk-delete` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/bulk-delete` }, method: 'POST', headers: (params) => ({ diff --git a/apps/sim/tools/discord/create_channel.ts b/apps/sim/tools/discord/create_channel.ts index 40f8a6cd8b7..13fb10f1cce 100644 --- a/apps/sim/tools/discord/create_channel.ts +++ b/apps/sim/tools/discord/create_channel.ts @@ -3,6 +3,7 @@ import type { DiscordCreateChannelResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordCreateChannelTool: ToolConfig< DiscordCreateChannelParams, @@ -54,7 +55,7 @@ export const discordCreateChannelTool: ToolConfig< request: { url: (params: DiscordCreateChannelParams) => { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/channels` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/channels` }, method: 'POST', headers: (params) => ({ diff --git a/apps/sim/tools/discord/create_invite.ts b/apps/sim/tools/discord/create_invite.ts index 87b945322c3..9d550ba2777 100644 --- a/apps/sim/tools/discord/create_invite.ts +++ b/apps/sim/tools/discord/create_invite.ts @@ -1,5 +1,6 @@ import type { DiscordCreateInviteParams, DiscordCreateInviteResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordCreateInviteTool: ToolConfig< DiscordCreateInviteParams, @@ -51,7 +52,7 @@ export const discordCreateInviteTool: ToolConfig< request: { url: (params: DiscordCreateInviteParams) => { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/invites` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/invites` }, method: 'POST', headers: (params) => ({ diff --git a/apps/sim/tools/discord/create_role.ts b/apps/sim/tools/discord/create_role.ts index bbd558990af..9ad5fa2f9cc 100644 --- a/apps/sim/tools/discord/create_role.ts +++ b/apps/sim/tools/discord/create_role.ts @@ -1,5 +1,6 @@ import type { DiscordCreateRoleParams, DiscordCreateRoleResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordCreateRoleTool: ToolConfig = { @@ -49,7 +50,7 @@ export const discordCreateRoleTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/roles` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/roles` }, method: 'POST', headers: (params) => ({ diff --git a/apps/sim/tools/discord/create_thread.ts b/apps/sim/tools/discord/create_thread.ts index b69fd5318a0..83a2e5462e7 100644 --- a/apps/sim/tools/discord/create_thread.ts +++ b/apps/sim/tools/discord/create_thread.ts @@ -1,5 +1,6 @@ import type { DiscordCreateThreadParams, DiscordCreateThreadResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordCreateThreadTool: ToolConfig< DiscordCreateThreadParams, @@ -61,9 +62,9 @@ export const discordCreateThreadTool: ToolConfig< url: (params: DiscordCreateThreadParams) => { const messageId = params.messageId?.trim() if (messageId) { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/messages/${messageId}/threads` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(messageId, 'messageId')}/threads` } - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/threads` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/threads` }, method: 'POST', headers: (params) => ({ diff --git a/apps/sim/tools/discord/create_webhook.ts b/apps/sim/tools/discord/create_webhook.ts index c0920debff3..ed76386aa2a 100644 --- a/apps/sim/tools/discord/create_webhook.ts +++ b/apps/sim/tools/discord/create_webhook.ts @@ -3,6 +3,7 @@ import type { DiscordCreateWebhookResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordCreateWebhookTool: ToolConfig< DiscordCreateWebhookParams, @@ -42,7 +43,7 @@ export const discordCreateWebhookTool: ToolConfig< request: { url: (params: DiscordCreateWebhookParams) => { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/webhooks` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/webhooks` }, method: 'POST', headers: (params) => ({ diff --git a/apps/sim/tools/discord/delete_channel.ts b/apps/sim/tools/discord/delete_channel.ts index 914e1f09096..ed2cdf26e88 100644 --- a/apps/sim/tools/discord/delete_channel.ts +++ b/apps/sim/tools/discord/delete_channel.ts @@ -3,6 +3,7 @@ import type { DiscordDeleteChannelResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordDeleteChannelTool: ToolConfig< DiscordDeleteChannelParams, @@ -36,7 +37,7 @@ export const discordDeleteChannelTool: ToolConfig< request: { url: (params: DiscordDeleteChannelParams) => { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/delete_invite.ts b/apps/sim/tools/discord/delete_invite.ts index f58be5d5390..606381d1a2a 100644 --- a/apps/sim/tools/discord/delete_invite.ts +++ b/apps/sim/tools/discord/delete_invite.ts @@ -1,5 +1,6 @@ import type { DiscordDeleteInviteParams, DiscordDeleteInviteResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordDeleteInviteTool: ToolConfig< DiscordDeleteInviteParams, @@ -33,7 +34,7 @@ export const discordDeleteInviteTool: ToolConfig< request: { url: (params: DiscordDeleteInviteParams) => { - return `https://discord.com/api/v10/invites/${params.inviteCode.trim()}` + return `https://discord.com/api/v10/invites/${safeUrlPathSegment(params.inviteCode, 'inviteCode')}` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/delete_message.ts b/apps/sim/tools/discord/delete_message.ts index 551335b34e4..1194ae9b09f 100644 --- a/apps/sim/tools/discord/delete_message.ts +++ b/apps/sim/tools/discord/delete_message.ts @@ -3,6 +3,7 @@ import type { DiscordDeleteMessageResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordDeleteMessageTool: ToolConfig< DiscordDeleteMessageParams, @@ -42,7 +43,7 @@ export const discordDeleteMessageTool: ToolConfig< request: { url: (params: DiscordDeleteMessageParams) => { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/messages/${params.messageId.trim()}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(params.messageId, 'messageId')}` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/delete_role.ts b/apps/sim/tools/discord/delete_role.ts index 83b1c190abc..cf4d22db950 100644 --- a/apps/sim/tools/discord/delete_role.ts +++ b/apps/sim/tools/discord/delete_role.ts @@ -1,5 +1,6 @@ import type { DiscordDeleteRoleParams, DiscordDeleteRoleResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordDeleteRoleTool: ToolConfig = { @@ -31,7 +32,7 @@ export const discordDeleteRoleTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/roles/${params.roleId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/roles/${safeUrlPathSegment(params.roleId, 'roleId')}` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/delete_webhook.ts b/apps/sim/tools/discord/delete_webhook.ts index 0b5169999cd..84fe22318c5 100644 --- a/apps/sim/tools/discord/delete_webhook.ts +++ b/apps/sim/tools/discord/delete_webhook.ts @@ -3,6 +3,7 @@ import type { DiscordDeleteWebhookResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordDeleteWebhookTool: ToolConfig< DiscordDeleteWebhookParams, @@ -36,7 +37,7 @@ export const discordDeleteWebhookTool: ToolConfig< request: { url: (params: DiscordDeleteWebhookParams) => { - return `https://discord.com/api/v10/webhooks/${params.webhookId.trim()}` + return `https://discord.com/api/v10/webhooks/${safeUrlPathSegment(params.webhookId, 'webhookId')}` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/edit_message.ts b/apps/sim/tools/discord/edit_message.ts index 82856f13f78..8e03142c134 100644 --- a/apps/sim/tools/discord/edit_message.ts +++ b/apps/sim/tools/discord/edit_message.ts @@ -1,5 +1,6 @@ import type { DiscordEditMessageParams, DiscordEditMessageResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordEditMessageTool: ToolConfig< DiscordEditMessageParams, @@ -45,7 +46,7 @@ export const discordEditMessageTool: ToolConfig< request: { url: (params: DiscordEditMessageParams) => { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/messages/${params.messageId.trim()}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(params.messageId, 'messageId')}` }, method: 'PATCH', headers: (params) => ({ diff --git a/apps/sim/tools/discord/execute_webhook.ts b/apps/sim/tools/discord/execute_webhook.ts index fefd16fa8f3..558bd09e7fb 100644 --- a/apps/sim/tools/discord/execute_webhook.ts +++ b/apps/sim/tools/discord/execute_webhook.ts @@ -3,6 +3,7 @@ import type { DiscordExecuteWebhookResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordExecuteWebhookTool: ToolConfig< DiscordExecuteWebhookParams, @@ -48,7 +49,7 @@ export const discordExecuteWebhookTool: ToolConfig< request: { url: (params: DiscordExecuteWebhookParams) => { - return `https://discord.com/api/v10/webhooks/${params.webhookId.trim()}/${params.webhookToken.trim()}?wait=true` + return `https://discord.com/api/v10/webhooks/${safeUrlPathSegment(params.webhookId, 'webhookId')}/${safeUrlPathSegment(params.webhookToken, 'webhookToken')}?wait=true` }, method: 'POST', headers: () => ({ diff --git a/apps/sim/tools/discord/get_channel.ts b/apps/sim/tools/discord/get_channel.ts index a7d19219006..959e959543c 100644 --- a/apps/sim/tools/discord/get_channel.ts +++ b/apps/sim/tools/discord/get_channel.ts @@ -1,5 +1,6 @@ import type { DiscordGetChannelParams, DiscordGetChannelResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetChannelTool: ToolConfig = { @@ -31,7 +32,7 @@ export const discordGetChannelTool: ToolConfig { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}` }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/discord/get_invite.ts b/apps/sim/tools/discord/get_invite.ts index 54efd3e78de..6b048aa2370 100644 --- a/apps/sim/tools/discord/get_invite.ts +++ b/apps/sim/tools/discord/get_invite.ts @@ -1,5 +1,6 @@ import type { DiscordGetInviteParams, DiscordGetInviteResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetInviteTool: ToolConfig = { id: 'discord_get_invite', @@ -30,7 +31,7 @@ export const discordGetInviteTool: ToolConfig { - return `https://discord.com/api/v10/invites/${params.inviteCode.trim()}?with_counts=true` + return `https://discord.com/api/v10/invites/${safeUrlPathSegment(params.inviteCode, 'inviteCode')}?with_counts=true` }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/discord/get_member.ts b/apps/sim/tools/discord/get_member.ts index 11c9a3bc1e3..6ece635f561 100644 --- a/apps/sim/tools/discord/get_member.ts +++ b/apps/sim/tools/discord/get_member.ts @@ -1,5 +1,6 @@ import type { DiscordGetMemberParams, DiscordGetMemberResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetMemberTool: ToolConfig = { id: 'discord_get_member', @@ -30,7 +31,7 @@ export const discordGetMemberTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/members/${params.userId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/members/${safeUrlPathSegment(params.userId, 'userId')}` }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/discord/get_messages.ts b/apps/sim/tools/discord/get_messages.ts index 29c250df0d1..d4684c4c8c8 100644 --- a/apps/sim/tools/discord/get_messages.ts +++ b/apps/sim/tools/discord/get_messages.ts @@ -1,5 +1,6 @@ import type { DiscordGetMessagesParams, DiscordGetMessagesResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetMessagesTool: ToolConfig< DiscordGetMessagesParams, @@ -34,7 +35,7 @@ export const discordGetMessagesTool: ToolConfig< request: { url: (params: DiscordGetMessagesParams) => { const limit = params.limit ? Number(params.limit) : 10 - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/messages?limit=${Math.min(limit, 100)}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages?limit=${Math.min(limit, 100)}` }, method: 'GET', headers: (params) => { diff --git a/apps/sim/tools/discord/get_pinned_messages.ts b/apps/sim/tools/discord/get_pinned_messages.ts index f7737d77d73..e6fecedde05 100644 --- a/apps/sim/tools/discord/get_pinned_messages.ts +++ b/apps/sim/tools/discord/get_pinned_messages.ts @@ -4,6 +4,7 @@ import type { DiscordMessage, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetPinnedMessagesTool: ToolConfig< DiscordGetPinnedMessagesParams, @@ -49,7 +50,7 @@ export const discordGetPinnedMessagesTool: ToolConfig< if (params.limit) query.set('limit', String(Math.min(Math.max(1, Number(params.limit)), 50))) if (params.before) query.set('before', params.before) const queryString = query.toString() - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/messages/pins${queryString ? `?${queryString}` : ''}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/pins${queryString ? `?${queryString}` : ''}` }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/discord/get_server.ts b/apps/sim/tools/discord/get_server.ts index 8e612654b86..4a424f66d24 100644 --- a/apps/sim/tools/discord/get_server.ts +++ b/apps/sim/tools/discord/get_server.ts @@ -4,6 +4,7 @@ import type { DiscordGuild, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetServerTool: ToolConfig = { id: 'discord_get_server', @@ -28,7 +29,7 @@ export const discordGetServerTool: ToolConfig - `https://discord.com/api/v10/guilds/${params.serverId.trim()}?with_counts=true`, + `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}?with_counts=true`, method: 'GET', headers: (params: DiscordGetServerParams) => { const headers: Record = { diff --git a/apps/sim/tools/discord/get_user.ts b/apps/sim/tools/discord/get_user.ts index 0b42dab90b7..f3a7ef4a2a1 100644 --- a/apps/sim/tools/discord/get_user.ts +++ b/apps/sim/tools/discord/get_user.ts @@ -4,6 +4,7 @@ import type { DiscordUser, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetUserTool: ToolConfig = { id: 'discord_get_user', @@ -28,7 +29,7 @@ export const discordGetUserTool: ToolConfig - `https://discord.com/api/v10/users/${params.userId.trim()}`, + `https://discord.com/api/v10/users/${safeUrlPathSegment(params.userId, 'userId')}`, method: 'GET', headers: (params: DiscordGetUserParams) => { const headers: Record = { diff --git a/apps/sim/tools/discord/get_webhook.ts b/apps/sim/tools/discord/get_webhook.ts index f0b795780a8..a8cd6a6f721 100644 --- a/apps/sim/tools/discord/get_webhook.ts +++ b/apps/sim/tools/discord/get_webhook.ts @@ -1,5 +1,6 @@ import type { DiscordGetWebhookParams, DiscordGetWebhookResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetWebhookTool: ToolConfig = { @@ -31,7 +32,7 @@ export const discordGetWebhookTool: ToolConfig { - return `https://discord.com/api/v10/webhooks/${params.webhookId.trim()}` + return `https://discord.com/api/v10/webhooks/${safeUrlPathSegment(params.webhookId, 'webhookId')}` }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/discord/join_thread.ts b/apps/sim/tools/discord/join_thread.ts index 58caf12693b..971d37be14f 100644 --- a/apps/sim/tools/discord/join_thread.ts +++ b/apps/sim/tools/discord/join_thread.ts @@ -1,5 +1,6 @@ import type { DiscordJoinThreadParams, DiscordJoinThreadResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordJoinThreadTool: ToolConfig = { @@ -31,7 +32,7 @@ export const discordJoinThreadTool: ToolConfig { - return `https://discord.com/api/v10/channels/${params.threadId.trim()}/thread-members/@me` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.threadId, 'threadId')}/thread-members/@me` }, method: 'PUT', headers: (params) => ({ diff --git a/apps/sim/tools/discord/kick_member.ts b/apps/sim/tools/discord/kick_member.ts index 8c865a1e8b6..57ce87d5d57 100644 --- a/apps/sim/tools/discord/kick_member.ts +++ b/apps/sim/tools/discord/kick_member.ts @@ -1,5 +1,6 @@ import type { DiscordKickMemberParams, DiscordKickMemberResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordKickMemberTool: ToolConfig = { @@ -37,7 +38,7 @@ export const discordKickMemberTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/members/${params.userId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/members/${safeUrlPathSegment(params.userId, 'userId')}` }, method: 'DELETE', headers: (params) => { diff --git a/apps/sim/tools/discord/leave_thread.ts b/apps/sim/tools/discord/leave_thread.ts index 3011b408358..d3cba98643d 100644 --- a/apps/sim/tools/discord/leave_thread.ts +++ b/apps/sim/tools/discord/leave_thread.ts @@ -1,5 +1,6 @@ import type { DiscordLeaveThreadParams, DiscordLeaveThreadResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordLeaveThreadTool: ToolConfig< DiscordLeaveThreadParams, @@ -33,7 +34,7 @@ export const discordLeaveThreadTool: ToolConfig< request: { url: (params: DiscordLeaveThreadParams) => { - return `https://discord.com/api/v10/channels/${params.threadId.trim()}/thread-members/@me` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.threadId, 'threadId')}/thread-members/@me` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/list_channels.ts b/apps/sim/tools/discord/list_channels.ts index 0507779a416..62dc22a2d14 100644 --- a/apps/sim/tools/discord/list_channels.ts +++ b/apps/sim/tools/discord/list_channels.ts @@ -1,5 +1,6 @@ import type { DiscordListChannelsParams, DiscordListChannelsResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordListChannelsTool: ToolConfig< DiscordListChannelsParams, @@ -27,7 +28,7 @@ export const discordListChannelsTool: ToolConfig< request: { url: (params: DiscordListChannelsParams) => { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/channels` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/channels` }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/discord/list_roles.ts b/apps/sim/tools/discord/list_roles.ts index dffae85fb4c..db4d5990004 100644 --- a/apps/sim/tools/discord/list_roles.ts +++ b/apps/sim/tools/discord/list_roles.ts @@ -1,5 +1,6 @@ import type { DiscordListRolesParams, DiscordListRolesResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordListRolesTool: ToolConfig = { id: 'discord_list_roles', @@ -24,7 +25,7 @@ export const discordListRolesTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/roles` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/roles` }, method: 'GET', headers: (params) => ({ diff --git a/apps/sim/tools/discord/path_safety.test.ts b/apps/sim/tools/discord/path_safety.test.ts new file mode 100644 index 00000000000..40fb1b3e746 --- /dev/null +++ b/apps/sim/tools/discord/path_safety.test.ts @@ -0,0 +1,231 @@ +/** + * @vitest-environment node + * + * Guards every Discord tool against path traversal through an LLM-writable ID + * that gets interpolated into the request path. + * + * Guild, channel, message, user, role, webhook, and invite IDs are + * `visibility: 'user-or-llm'`, so prompt injection controls them. Interpolating + * one raw let a value like `../../guilds/victim` escape its `/api/v10` prefix + * once `fetch` normalized the URL, re-aiming the request — and the workspace's + * Discord bot token — at an arbitrary Discord resource, including on the DELETE + * channel, DELETE role, and ban routes. + * + * `encodeURIComponent` is NOT enough, which is why the vector list below keeps + * the bare `.` and `..` segments: both are made of unreserved characters, so + * they survive encoding untouched and the URL parser then removes them as dot + * segments, popping one path segment off a fixed host. Every assertion here + * resolves the built URL with `new URL(...)` — the same normalization `fetch` + * performs — rather than string-matching the template output, because string + * matching is exactly what let this through. + * + * The tool list is enumerated from the barrel, so a newly added Discord tool + * that interpolates an unguarded ID fails here without anyone editing this file. + */ +import { describe, expect, it } from 'vitest' +import * as discordTools from '@/tools/discord' +import { discordAssignRoleTool } from '@/tools/discord/assign_role' +import { discordGetMemberTool } from '@/tools/discord/get_member' +import type { ToolConfig } from '@/tools/types' + +const API_ORIGIN = 'https://discord.com' +const API_PREFIX = '/api/v10/' + +/** + * The bare `.` and `..` entries are the whole point: their omission is why an + * `encodeURIComponent`-only fix looks correct while the hole stays live. + */ +const TRAVERSAL_IDS = [ + '..', + '.', + ' .. ', + '../../guilds/987654321098765432', + '..%2f..%2fguilds/987654321098765432', + '123456789012345678/../../../guilds/987654321098765432', + '123456789012345678?with_counts=false', + '123456789012345678#fragment', + '123456789012345678/messages/../../../users/@me', + '\\..\\..', +] as const + +/** + * Values a real user legitimately supplies; none may be rejected or altered. + * + * Discord IDs are snowflakes — 17 to 19 digit decimal strings — so the numeric + * spellings here are the shape that actually reaches these tools in production. + */ +const LEGITIMATE_IDS = [ + '12345678901234567', + '123456789012345678', + '1234567890123456789', + '80351110224678912', + 'general', + 'aBcDeF1', + '..foo', + 'foo..', + 'v1.2.3', +] as const + +const SAFE_ID = 'SAFEID' + +type AnyTool = ToolConfig + +function isDiscordTool(value: unknown): value is AnyTool { + return ( + typeof value === 'object' && + value !== null && + typeof (value as AnyTool).id === 'string' && + (value as AnyTool).id.startsWith('discord_') + ) +} + +/** + * Builds a param object for a tool, filling every declared string param with + * `value` so whichever one reaches the path is exercised. + */ +function buildParams(tool: AnyTool, value: string): Record { + const params: Record = { botToken: 'bot-token' } + for (const [name, def] of Object.entries(tool.params ?? {})) { + if (name === 'botToken') continue + const type = (def as { type?: string }).type + if (type === 'json' || type === 'array' || type === 'file[]') { + params[name] = [] + } else if (type === 'number') { + params[name] = 1 + } else if (type === 'boolean') { + params[name] = false + } else { + params[name] = value + } + } + return params +} + +function buildUrl(tool: AnyTool, value: string): URL { + const url = tool.request?.url + if (typeof url !== 'function') { + throw new Error(`${tool.id} does not build its URL from params`) + } + return new URL(url(buildParams(tool, value) as any)) +} + +function segmentsOf(pathname: string): string[] { + return pathname.split('/') +} + +const DYNAMIC_PATH_TOOLS = Object.values(discordTools) + .filter(isDiscordTool) + .filter((tool) => typeof tool.request?.url === 'function') + .filter((tool) => { + try { + return buildUrl(tool, SAFE_ID).pathname.includes(SAFE_ID) + } catch { + return false + } + }) + .map((tool) => ({ name: tool.id, tool })) + +describe('discord path-ID traversal safety', () => { + it('covers every Discord tool that interpolates an ID into its path', () => { + expect(DYNAMIC_PATH_TOOLS.length).toBeGreaterThanOrEqual(30) + }) + + describe.each(DYNAMIC_PATH_TOOLS)('$name', ({ tool }) => { + const baseline = segmentsOf(buildUrl(tool, SAFE_ID).pathname) + + it.each(TRAVERSAL_IDS)('cannot reshape the path with %j', (value) => { + let url: URL + try { + url = buildUrl(tool, value) + } catch { + return + } + + expect(url.origin).toBe(API_ORIGIN) + expect(url.pathname.startsWith(API_PREFIX)).toBe(true) + + const actual = segmentsOf(url.pathname) + expect(actual).toHaveLength(baseline.length) + baseline.forEach((segment, index) => { + if (segment === SAFE_ID) return + expect(actual[index]).toBe(segment) + }) + }) + + it('rejects a bare dot-dot segment instead of silently popping the prefix', () => { + expect(() => buildUrl(tool, '..')).toThrow(/path traversal is not allowed/) + }) + + it('rejects a bare dot segment', () => { + expect(() => buildUrl(tool, '.')).toThrow(/path traversal is not allowed/) + }) + + it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { + const actual = segmentsOf(buildUrl(tool, value).pathname) + + expect(actual).toHaveLength(baseline.length) + baseline.forEach((segment, index) => { + expect(actual[index]).toBe(segment === SAFE_ID ? value : segment) + }) + }) + + it('trims surrounding whitespace off a snowflake', () => { + const actual = segmentsOf(buildUrl(tool, ' 123456789012345678 ').pathname) + + baseline.forEach((segment, index) => { + if (segment !== SAFE_ID) return + expect(actual[index]).toBe('123456789012345678') + }) + }) + }) +}) + +/** + * An LLM tool call carries JSON, so a snowflake can arrive as a `number` rather + * than the declared `string`. The guard must not turn that into a bogus segment. + * + * A snowflake wider than `Number.MAX_SAFE_INTEGER` has already lost digits by + * the time `JSON.parse` finished, so it is refused by name instead of silently + * addressing a neighbouring resource. A snowflake that survives as a `bigint`, + * and any numeric id inside the safe range, pass through as their decimal text. + */ +describe('discord snowflakes supplied as JSON numbers', () => { + it('accepts a numeric id inside the safe integer range', () => { + const url = new URL( + discordGetMemberTool.request.url({ + botToken: 'bot-token', + serverId: 8035111022467891, + userId: 8035111022467892, + } as any) + ) + + expect(url.pathname).toBe('/api/v10/guilds/8035111022467891/members/8035111022467892') + }) + + it('accepts a full-width snowflake supplied as a bigint', () => { + const url = new URL( + discordGetMemberTool.request.url({ + botToken: 'bot-token', + serverId: 1234567890123456789n, + userId: 987654321098765432n, + } as any) + ) + + expect(url.pathname).toBe('/api/v10/guilds/1234567890123456789/members/987654321098765432') + }) + + it('refuses a snowflake JSON.parse already rounded rather than addressing the wrong resource', () => { + const roundedSnowflake = Number('1234567890123456789') + + expect(roundedSnowflake).not.toBe(1234567890123456789n) + + expect(() => + discordAssignRoleTool.request.url({ + botToken: 'bot-token', + serverId: roundedSnowflake, + userId: '123456789012345678', + roleId: '123456789012345678', + } as any) + ).toThrow(/serverId is too large to be represented exactly/) + }) +}) diff --git a/apps/sim/tools/discord/pin_message.ts b/apps/sim/tools/discord/pin_message.ts index d526b3387d6..d3c1b6189e3 100644 --- a/apps/sim/tools/discord/pin_message.ts +++ b/apps/sim/tools/discord/pin_message.ts @@ -1,5 +1,6 @@ import type { DiscordPinMessageParams, DiscordPinMessageResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordPinMessageTool: ToolConfig = { @@ -37,7 +38,7 @@ export const discordPinMessageTool: ToolConfig { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/pins/${params.messageId.trim()}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/pins/${safeUrlPathSegment(params.messageId, 'messageId')}` }, method: 'PUT', headers: (params) => ({ diff --git a/apps/sim/tools/discord/remove_reaction.ts b/apps/sim/tools/discord/remove_reaction.ts index e03924d16e1..4800a2ee778 100644 --- a/apps/sim/tools/discord/remove_reaction.ts +++ b/apps/sim/tools/discord/remove_reaction.ts @@ -3,6 +3,7 @@ import type { DiscordRemoveReactionResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordRemoveReactionTool: ToolConfig< DiscordRemoveReactionParams, @@ -55,10 +56,10 @@ export const discordRemoveReactionTool: ToolConfig< request: { url: (params: DiscordRemoveReactionParams) => { - const encodedEmoji = encodeURIComponent(params.emoji) + const encodedEmoji = safeUrlPathSegment(params.emoji, 'emoji') const userId = params.userId?.trim() - const userPart = userId ? `/${userId}` : '/@me' - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/messages/${params.messageId.trim()}/reactions/${encodedEmoji}${userPart}` + const userPart = userId ? `/${safeUrlPathSegment(userId, 'userId')}` : '/@me' + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(params.messageId, 'messageId')}/reactions/${encodedEmoji}${userPart}` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/remove_role.ts b/apps/sim/tools/discord/remove_role.ts index 889b86f5283..62ea44a1c68 100644 --- a/apps/sim/tools/discord/remove_role.ts +++ b/apps/sim/tools/discord/remove_role.ts @@ -1,5 +1,6 @@ import type { DiscordRemoveRoleParams, DiscordRemoveRoleResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordRemoveRoleTool: ToolConfig = { @@ -37,7 +38,7 @@ export const discordRemoveRoleTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/members/${params.userId.trim()}/roles/${params.roleId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/members/${safeUrlPathSegment(params.userId, 'userId')}/roles/${safeUrlPathSegment(params.roleId, 'roleId')}` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/unban_member.ts b/apps/sim/tools/discord/unban_member.ts index dc3ed0f3456..fa294501ea8 100644 --- a/apps/sim/tools/discord/unban_member.ts +++ b/apps/sim/tools/discord/unban_member.ts @@ -1,5 +1,6 @@ import type { DiscordUnbanMemberParams, DiscordUnbanMemberResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordUnbanMemberTool: ToolConfig< DiscordUnbanMemberParams, @@ -39,7 +40,7 @@ export const discordUnbanMemberTool: ToolConfig< request: { url: (params: DiscordUnbanMemberParams) => { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/bans/${params.userId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/bans/${safeUrlPathSegment(params.userId, 'userId')}` }, method: 'DELETE', headers: (params) => { diff --git a/apps/sim/tools/discord/unpin_message.ts b/apps/sim/tools/discord/unpin_message.ts index 5ba2b894832..3058c87dea9 100644 --- a/apps/sim/tools/discord/unpin_message.ts +++ b/apps/sim/tools/discord/unpin_message.ts @@ -1,5 +1,6 @@ import type { DiscordUnpinMessageParams, DiscordUnpinMessageResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordUnpinMessageTool: ToolConfig< DiscordUnpinMessageParams, @@ -39,7 +40,7 @@ export const discordUnpinMessageTool: ToolConfig< request: { url: (params: DiscordUnpinMessageParams) => { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}/pins/${params.messageId.trim()}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/pins/${safeUrlPathSegment(params.messageId, 'messageId')}` }, method: 'DELETE', headers: (params) => ({ diff --git a/apps/sim/tools/discord/update_channel.ts b/apps/sim/tools/discord/update_channel.ts index 786f99a45bb..4211db8ba39 100644 --- a/apps/sim/tools/discord/update_channel.ts +++ b/apps/sim/tools/discord/update_channel.ts @@ -3,6 +3,7 @@ import type { DiscordUpdateChannelResponse, } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordUpdateChannelTool: ToolConfig< DiscordUpdateChannelParams, @@ -48,7 +49,7 @@ export const discordUpdateChannelTool: ToolConfig< request: { url: (params: DiscordUpdateChannelParams) => { - return `https://discord.com/api/v10/channels/${params.channelId.trim()}` + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}` }, method: 'PATCH', headers: (params) => ({ diff --git a/apps/sim/tools/discord/update_member.ts b/apps/sim/tools/discord/update_member.ts index 9a24f21a698..f61b3ad3dd7 100644 --- a/apps/sim/tools/discord/update_member.ts +++ b/apps/sim/tools/discord/update_member.ts @@ -1,5 +1,6 @@ import type { DiscordUpdateMemberParams, DiscordUpdateMemberResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordUpdateMemberTool: ToolConfig< DiscordUpdateMemberParams, @@ -51,7 +52,7 @@ export const discordUpdateMemberTool: ToolConfig< request: { url: (params: DiscordUpdateMemberParams) => { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/members/${params.userId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/members/${safeUrlPathSegment(params.userId, 'userId')}` }, method: 'PATCH', headers: (params) => ({ diff --git a/apps/sim/tools/discord/update_role.ts b/apps/sim/tools/discord/update_role.ts index cf45f1e8a91..78ed4d569b3 100644 --- a/apps/sim/tools/discord/update_role.ts +++ b/apps/sim/tools/discord/update_role.ts @@ -1,5 +1,6 @@ import type { DiscordUpdateRoleParams, DiscordUpdateRoleResponse } from '@/tools/discord/types' import type { ToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordUpdateRoleTool: ToolConfig = { @@ -55,7 +56,7 @@ export const discordUpdateRoleTool: ToolConfig { - return `https://discord.com/api/v10/guilds/${params.serverId.trim()}/roles/${params.roleId.trim()}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/roles/${safeUrlPathSegment(params.roleId, 'roleId')}` }, method: 'PATCH', headers: (params) => ({ From 12ffff00f329d15fe2ab4a7cdf81a09165eb8f13 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 21:15:37 -0700 Subject: [PATCH 2/9] test(cloudflare,discord): fuzz path params one at a time, not as a whole object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous suites filled every string param with the same fuzz value and swallowed the throw: try { path = buildPath(tool, value) } catch { return } URL construction is eager, so the first guarded param to throw aborted the whole vector and every sibling param went untested. That inverted the property the suites were written to hold: once a tool had one guard, a *newly unguarded* sibling could no longer fail CI. It is the worst possible shape for these two services, where `channelId` + `messageId`, `serverId` + `roleId`, and `zoneId` + `rulesetId` + `ruleId` share one path. Both suites now enumerate (tool, param) pairs — discovered by probing one param at a time, so a new tool or a new path param appears with no edit here — and fuzz exactly one param while holding every sibling at a safe value. 133 pairs across 84 tools, up from 84 whole-tool cases. The vectors are also split by the outcome they must produce, replacing the tolerant try/catch: MUST_REJECT (dot segments and anything carrying a path separator) asserts a throw naming the offending param, and MUST_NEUTRALIZE (`?`/`#` inside a segment) asserts the segment shape is preserved. Nothing is skipped silently any more. Verified red-first against the tightened suite by reverting one guard on a multi-param tool — `messageId` on discord_delete_message and `ruleId` on cloudflare_delete_ruleset_rule — to an `encodeURIComponent`-only version, leaving their siblings guarded. That is exactly the case the old shape could not see: it produces 16 named failures now, while the old whole-object assertion passed 3/3 green on the identical code. No source change: the pair enumeration confirms every param that reaches a path is already guarded. --- apps/sim/tools/cloudflare/path_safety.test.ts | 146 ++++++++++-------- apps/sim/tools/discord/path_safety.test.ts | 136 +++++++++------- 2 files changed, 168 insertions(+), 114 deletions(-) diff --git a/apps/sim/tools/cloudflare/path_safety.test.ts b/apps/sim/tools/cloudflare/path_safety.test.ts index ef7b17528b7..a5e9bac4632 100644 --- a/apps/sim/tools/cloudflare/path_safety.test.ts +++ b/apps/sim/tools/cloudflare/path_safety.test.ts @@ -19,8 +19,14 @@ * performs — rather than string-matching the template output, because string * matching is exactly what let this through. * - * The tool list is enumerated from the barrel, so a newly added Cloudflare tool - * that interpolates an unguarded ID fails here without anyone editing this file. + * The suite enumerates **(tool, param) pairs** and fuzzes one param at a time, + * holding every sibling at a safe value. Fuzzing all params at once cannot work + * here: the first guard to throw aborts URL construction, so a tool's remaining + * params stop being exercised the moment one of them is fixed. Pair enumeration + * is what makes "a newly unguarded param fails CI" actually true for a tool that + * already has a guarded param — the dominant shape in this service, where + * `accountId` + `appId` + `policyId` and `zoneId` + `rulesetId` + `ruleId` + * share one path. */ import { describe, expect, it } from 'vitest' import * as cloudflareTools from '@/tools/cloudflare' @@ -28,24 +34,34 @@ import type { ToolConfig } from '@/tools/types' const API_ORIGIN = 'https://api.cloudflare.com' const API_PREFIX = '/client/v4/' +const CREDENTIAL_PARAM = 'apiKey' /** + * Vectors the guard must **reject outright**. Each is either a bare dot segment + * or carries a path separator, so encoding it would leave a live traversal. * The bare `.` and `..` entries are the whole point: their omission is why an * `encodeURIComponent`-only fix looks correct while the hole stays live. */ -const TRAVERSAL_IDS = [ +const MUST_REJECT = [ '..', '.', ' .. ', '../../accounts/victim-account', '..%2f..%2faccounts/victim-account', '023e105f4ecef8ad9ca31a8372d0c353/../../accounts/victim-account', - '023e105f4ecef8ad9ca31a8372d0c353?account_id=attacker', - '023e105f4ecef8ad9ca31a8372d0c353#fragment', '023e105f4ecef8ad9ca31a8372d0c353/dns_records/../../../zones', '\\..\\..', ] as const +/** + * Vectors that are not traversals but must not be able to reshape the request: + * a `?` or `#` inside a segment has to stay inside that segment. + */ +const MUST_NEUTRALIZE = [ + '023e105f4ecef8ad9ca31a8372d0c353?account_id=attacker', + '023e105f4ecef8ad9ca31a8372d0c353#frag', +] as const + /** Values a real user legitimately supplies; none may be rejected or altered. */ const LEGITIMATE_IDS = [ '023e105f4ecef8ad9ca31a8372d0c353', @@ -61,6 +77,8 @@ const LEGITIMATE_IDS = [ ] as const const SAFE_ID = 'SAFEID' +const PROBE = 'PROBEVALUE' +const TRIM_SAMPLE = '023e105f4ecef8ad9ca31a8372d0c353' type AnyTool = ToolConfig @@ -74,112 +92,118 @@ function isCloudflareTool(value: unknown): value is AnyTool { } /** - * Builds a param object for a tool, filling every declared string param with - * `value` so whichever one reaches the path is exercised. + * Builds a param object with every string param at a known-safe value, then + * applies one override so exactly one param carries the value under test. */ -function buildParams(tool: AnyTool, value: string): Record { - const params: Record = { apiKey: 'cf-token' } +function buildParams( + tool: AnyTool, + overrides: Record = {} +): Record { + const params: Record = { [CREDENTIAL_PARAM]: 'cf-token' } for (const [name, def] of Object.entries(tool.params ?? {})) { - if (name === 'apiKey') continue + if (name === CREDENTIAL_PARAM) continue const type = (def as { type?: string }).type - if (type === 'json' || type === 'array') { + if (type === 'json' || type === 'array' || type === 'file[]') { params[name] = [] } else if (type === 'number') { params[name] = 1 } else if (type === 'boolean') { params[name] = false } else { - params[name] = value + params[name] = SAFE_ID } } - return params + return { ...params, ...overrides } } -function buildUrl(tool: AnyTool, value: string): URL { +function buildUrl(tool: AnyTool, overrides: Record = {}): URL { const url = tool.request?.url if (typeof url !== 'function') { throw new Error(`${tool.id} does not build its URL from params`) } - return new URL(url(buildParams(tool, value) as any)) + return new URL(url(buildParams(tool, overrides) as any)) } function segmentsOf(pathname: string): string[] { return pathname.split('/') } -const DYNAMIC_PATH_TOOLS = Object.values(cloudflareTools) +const TOOLS = Object.values(cloudflareTools) .filter(isCloudflareTool) .filter((tool) => typeof tool.request?.url === 'function') - .filter((tool) => { - try { - return buildUrl(tool, SAFE_ID).pathname.includes(SAFE_ID) - } catch { - return false - } + +/** + * Every (tool, param) pair where that param alone reaches the request path, + * discovered by probing one param at a time. A newly added tool — or a newly + * added path param on an existing tool — appears here with no edit to this file. + */ +const PATH_PARAM_PAIRS = TOOLS.flatMap((tool) => + Object.keys(tool.params ?? {}) + .filter((param) => param !== CREDENTIAL_PARAM) + .filter((param) => { + try { + return buildUrl(tool, { [param]: PROBE }).pathname.includes(PROBE) + } catch { + return false + } + }) + .map((param) => ({ name: `${tool.id} / ${param}`, tool, param })) +) + +describe('cloudflare path-param traversal safety', () => { + it('finds every (tool, param) pair that reaches the request path', () => { + expect(PATH_PARAM_PAIRS.length).toBeGreaterThanOrEqual(65) }) - .map((tool) => ({ name: tool.id, tool })) -describe('cloudflare path-ID traversal safety', () => { - it('covers every Cloudflare tool that interpolates an ID into its path', () => { - expect(DYNAMIC_PATH_TOOLS.length).toBeGreaterThanOrEqual(40) + it('covers multi-param paths, where whole-object fuzzing goes blind', () => { + const counts = new Map() + for (const { tool } of PATH_PARAM_PAIRS) { + counts.set(tool.id, (counts.get(tool.id) ?? 0) + 1) + } + const multiParamTools = [...counts.values()].filter((count) => count > 1) + + expect(multiParamTools.length).toBeGreaterThanOrEqual(15) }) - describe.each(DYNAMIC_PATH_TOOLS)('$name', ({ tool }) => { - const baseline = segmentsOf(buildUrl(tool, SAFE_ID).pathname) + describe.each(PATH_PARAM_PAIRS)('$name', ({ tool, param }) => { + const baseline = segmentsOf(buildUrl(tool, { [param]: PROBE }).pathname) - it.each(TRAVERSAL_IDS)('cannot reshape the path with %j', (value) => { - let url: URL - try { - url = buildUrl(tool, value) - } catch { - return - } + it.each(MUST_REJECT)('rejects %j outright', (value) => { + expect(() => buildUrl(tool, { [param]: value })).toThrow( + new RegExp(`${param}|path traversal|path separator`) + ) + }) + + it.each(MUST_NEUTRALIZE)('confines %j to a single segment', (value) => { + const url = buildUrl(tool, { [param]: value }) expect(url.origin).toBe(API_ORIGIN) expect(url.pathname.startsWith(API_PREFIX)).toBe(true) + expect(url.searchParams.get('account_id')).toBeNull() + expect(url.hash).toBe('') const actual = segmentsOf(url.pathname) expect(actual).toHaveLength(baseline.length) baseline.forEach((segment, index) => { - if (segment === SAFE_ID) return + if (segment.includes(PROBE)) return expect(actual[index]).toBe(segment) }) }) - it.each(TRAVERSAL_IDS)('never smuggles a query parameter via %j', (value) => { - let url: URL - try { - url = buildUrl(tool, value) - } catch { - return - } - - expect(url.searchParams.get('account_id')).toBeNull() - }) - - it('rejects a bare dot-dot segment instead of silently popping the prefix', () => { - expect(() => buildUrl(tool, '..')).toThrow(/path traversal is not allowed/) - }) - - it('rejects a bare dot segment', () => { - expect(() => buildUrl(tool, '.')).toThrow(/path traversal is not allowed/) - }) - it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { - const actual = segmentsOf(buildUrl(tool, value).pathname) + const actual = segmentsOf(buildUrl(tool, { [param]: value }).pathname) expect(actual).toHaveLength(baseline.length) baseline.forEach((segment, index) => { - expect(actual[index]).toBe(segment === SAFE_ID ? value : segment) + expect(actual[index]).toBe(segment.replaceAll(PROBE, value)) }) }) - it('trims surrounding whitespace off a legitimate ID', () => { - const actual = segmentsOf(buildUrl(tool, ' 023e105f4ecef8ad9ca31a8372d0c353 ').pathname) + it('trims surrounding whitespace off a legitimate value', () => { + const actual = segmentsOf(buildUrl(tool, { [param]: ` ${TRIM_SAMPLE} ` }).pathname) baseline.forEach((segment, index) => { - if (segment !== SAFE_ID) return - expect(actual[index]).toBe('023e105f4ecef8ad9ca31a8372d0c353') + expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) }) }) }) diff --git a/apps/sim/tools/discord/path_safety.test.ts b/apps/sim/tools/discord/path_safety.test.ts index 40fb1b3e746..5af3edaaff6 100644 --- a/apps/sim/tools/discord/path_safety.test.ts +++ b/apps/sim/tools/discord/path_safety.test.ts @@ -6,10 +6,10 @@ * * Guild, channel, message, user, role, webhook, and invite IDs are * `visibility: 'user-or-llm'`, so prompt injection controls them. Interpolating - * one raw let a value like `../../guilds/victim` escape its `/api/v10` prefix - * once `fetch` normalized the URL, re-aiming the request — and the workspace's - * Discord bot token — at an arbitrary Discord resource, including on the DELETE - * channel, DELETE role, and ban routes. + * one raw let a value like `../../guilds/987654321098765432` escape its + * `/api/v10` prefix once `fetch` normalized the URL, re-aiming the request — + * and the workspace's Discord bot token — at an arbitrary Discord resource, + * including on the DELETE channel, DELETE role, and ban routes. * * `encodeURIComponent` is NOT enough, which is why the vector list below keeps * the bare `.` and `..` segments: both are made of unreserved characters, so @@ -19,8 +19,13 @@ * performs — rather than string-matching the template output, because string * matching is exactly what let this through. * - * The tool list is enumerated from the barrel, so a newly added Discord tool - * that interpolates an unguarded ID fails here without anyone editing this file. + * The suite enumerates **(tool, param) pairs** and fuzzes one param at a time, + * holding every sibling at a safe value. Fuzzing all params at once cannot work + * here: the first guard to throw aborts URL construction, so a tool's remaining + * params stop being exercised the moment one of them is fixed. Pair enumeration + * is what makes "a newly unguarded param fails CI" actually true for a tool that + * already has a guarded param — the dominant shape in this service, where + * `channelId` + `messageId` and `serverId` + `roleId` share one path. */ import { describe, expect, it } from 'vitest' import * as discordTools from '@/tools/discord' @@ -30,24 +35,31 @@ import type { ToolConfig } from '@/tools/types' const API_ORIGIN = 'https://discord.com' const API_PREFIX = '/api/v10/' +const CREDENTIAL_PARAM = 'botToken' /** + * Vectors the guard must **reject outright**. Each is either a bare dot segment + * or carries a path separator, so encoding it would leave a live traversal. * The bare `.` and `..` entries are the whole point: their omission is why an * `encodeURIComponent`-only fix looks correct while the hole stays live. */ -const TRAVERSAL_IDS = [ +const MUST_REJECT = [ '..', '.', ' .. ', '../../guilds/987654321098765432', '..%2f..%2fguilds/987654321098765432', '123456789012345678/../../../guilds/987654321098765432', - '123456789012345678?with_counts=false', - '123456789012345678#fragment', '123456789012345678/messages/../../../users/@me', '\\..\\..', ] as const +/** + * Vectors that are not traversals but must not be able to reshape the request: + * a `?` or `#` inside a segment has to stay inside that segment. + */ +const MUST_NEUTRALIZE = ['123456789012345678?with_counts=false', '123456789012345678#frag'] as const + /** * Values a real user legitimately supplies; none may be rejected or altered. * @@ -67,6 +79,8 @@ const LEGITIMATE_IDS = [ ] as const const SAFE_ID = 'SAFEID' +const PROBE = 'PROBEVALUE' +const TRIM_SAMPLE = '123456789012345678' type AnyTool = ToolConfig @@ -80,13 +94,16 @@ function isDiscordTool(value: unknown): value is AnyTool { } /** - * Builds a param object for a tool, filling every declared string param with - * `value` so whichever one reaches the path is exercised. + * Builds a param object with every string param at a known-safe value, then + * applies one override so exactly one param carries the value under test. */ -function buildParams(tool: AnyTool, value: string): Record { - const params: Record = { botToken: 'bot-token' } +function buildParams( + tool: AnyTool, + overrides: Record = {} +): Record { + const params: Record = { [CREDENTIAL_PARAM]: 'bot-token' } for (const [name, def] of Object.entries(tool.params ?? {})) { - if (name === 'botToken') continue + if (name === CREDENTIAL_PARAM) continue const type = (def as { type?: string }).type if (type === 'json' || type === 'array' || type === 'file[]') { params[name] = [] @@ -95,86 +112,99 @@ function buildParams(tool: AnyTool, value: string): Record { } else if (type === 'boolean') { params[name] = false } else { - params[name] = value + params[name] = SAFE_ID } } - return params + return { ...params, ...overrides } } -function buildUrl(tool: AnyTool, value: string): URL { +function buildUrl(tool: AnyTool, overrides: Record = {}): URL { const url = tool.request?.url if (typeof url !== 'function') { throw new Error(`${tool.id} does not build its URL from params`) } - return new URL(url(buildParams(tool, value) as any)) + return new URL(url(buildParams(tool, overrides) as any)) } function segmentsOf(pathname: string): string[] { return pathname.split('/') } -const DYNAMIC_PATH_TOOLS = Object.values(discordTools) +const TOOLS = Object.values(discordTools) .filter(isDiscordTool) .filter((tool) => typeof tool.request?.url === 'function') - .filter((tool) => { - try { - return buildUrl(tool, SAFE_ID).pathname.includes(SAFE_ID) - } catch { - return false - } + +/** + * Every (tool, param) pair where that param alone reaches the request path, + * discovered by probing one param at a time. A newly added tool — or a newly + * added path param on an existing tool — appears here with no edit to this file. + */ +const PATH_PARAM_PAIRS = TOOLS.flatMap((tool) => + Object.keys(tool.params ?? {}) + .filter((param) => param !== CREDENTIAL_PARAM) + .filter((param) => { + try { + return buildUrl(tool, { [param]: PROBE }).pathname.includes(PROBE) + } catch { + return false + } + }) + .map((param) => ({ name: `${tool.id} / ${param}`, tool, param })) +) + +describe('discord path-param traversal safety', () => { + it('finds every (tool, param) pair that reaches the request path', () => { + expect(PATH_PARAM_PAIRS.length).toBeGreaterThanOrEqual(60) }) - .map((tool) => ({ name: tool.id, tool })) -describe('discord path-ID traversal safety', () => { - it('covers every Discord tool that interpolates an ID into its path', () => { - expect(DYNAMIC_PATH_TOOLS.length).toBeGreaterThanOrEqual(30) + it('covers multi-param paths, where whole-object fuzzing goes blind', () => { + const counts = new Map() + for (const { tool } of PATH_PARAM_PAIRS) { + counts.set(tool.id, (counts.get(tool.id) ?? 0) + 1) + } + const multiParamTools = [...counts.values()].filter((count) => count > 1) + + expect(multiParamTools.length).toBeGreaterThanOrEqual(15) }) - describe.each(DYNAMIC_PATH_TOOLS)('$name', ({ tool }) => { - const baseline = segmentsOf(buildUrl(tool, SAFE_ID).pathname) + describe.each(PATH_PARAM_PAIRS)('$name', ({ tool, param }) => { + const baseline = segmentsOf(buildUrl(tool, { [param]: PROBE }).pathname) - it.each(TRAVERSAL_IDS)('cannot reshape the path with %j', (value) => { - let url: URL - try { - url = buildUrl(tool, value) - } catch { - return - } + it.each(MUST_REJECT)('rejects %j outright', (value) => { + expect(() => buildUrl(tool, { [param]: value })).toThrow( + new RegExp(`${param}|path traversal|path separator`) + ) + }) + + it.each(MUST_NEUTRALIZE)('confines %j to a single segment', (value) => { + const url = buildUrl(tool, { [param]: value }) expect(url.origin).toBe(API_ORIGIN) expect(url.pathname.startsWith(API_PREFIX)).toBe(true) + expect(url.hash).toBe('') const actual = segmentsOf(url.pathname) expect(actual).toHaveLength(baseline.length) baseline.forEach((segment, index) => { - if (segment === SAFE_ID) return + if (segment.includes(PROBE)) return expect(actual[index]).toBe(segment) }) }) - it('rejects a bare dot-dot segment instead of silently popping the prefix', () => { - expect(() => buildUrl(tool, '..')).toThrow(/path traversal is not allowed/) - }) - - it('rejects a bare dot segment', () => { - expect(() => buildUrl(tool, '.')).toThrow(/path traversal is not allowed/) - }) - it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { - const actual = segmentsOf(buildUrl(tool, value).pathname) + const actual = segmentsOf(buildUrl(tool, { [param]: value }).pathname) expect(actual).toHaveLength(baseline.length) baseline.forEach((segment, index) => { - expect(actual[index]).toBe(segment === SAFE_ID ? value : segment) + expect(actual[index]).toBe(segment.replaceAll(PROBE, value)) }) }) - it('trims surrounding whitespace off a snowflake', () => { - const actual = segmentsOf(buildUrl(tool, ' 123456789012345678 ').pathname) + it('trims surrounding whitespace off a legitimate value', () => { + const actual = segmentsOf(buildUrl(tool, { [param]: ` ${TRIM_SAMPLE} ` }).pathname) baseline.forEach((segment, index) => { - if (segment !== SAFE_ID) return - expect(actual[index]).toBe('123456789012345678') + expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) }) }) }) From 65a07876aded9525a1e9a2e0fe6b6357ef76468b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 21:32:35 -0700 Subject: [PATCH 3/9] test(cloudflare,discord): probe every branch, drop `any`, name unbuildable tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three refinements to the path-safety suites. No source change: the fix itself was already complete, and all three confirm that rather than alter it. Branch coverage. A param that only appears on ONE branch of a conditional URL builder is invisible to a single all-params probe. Neither service switches on a string literal — the harness now harvests comparison literals from `String(tool.request.url)` so a future `action`-style builder is probed on every branch without editing this file, and it finds none today — but two Discord tools branch on param PRESENCE: `create_thread` picks a different endpoint when `messageId` is absent, and `remove_reaction` falls back to `/@me` when `userId` is. Discovery now probes each param with every optional sibling omitted in turn, which raises the case count 133 -> 137: 4 Discord branch shapes that were never exercised (the `/@me` form and the no-message thread form). The set of (tool, param) pairs is unchanged, so no unguarded param was hiding there; the new cases are previously untested path shapes for params already guarded. A ratchet assertion keeps them covered. No `any`. `ToolConfig` and the `as any` calls are replaced by a structural `ServiceTool`/`PathTool` pair narrowed through `isPathTool` and `pathToolFor`, per CLAUDE.md. Unbuildable tools are named, not swallowed. `SKIPPED_TOOL_IDS` asserts the tools that build no URL from params against an explicit allowlist (`discord_send_message`, `cloudflare_create_zone`, `cloudflare_get_zone_settings`), and `UNBUILDABLE` collects any tool whose URL will not build from all-safe values and asserts empty — a failed probe of a guarded param is still expected and tolerated, but a tool that cannot be exercised at all now fails instead of vanishing from coverage. Verified non-vacuous by dropping an entry and watching it go red. Also adds the ` . ` vector, since a bare dot survives whitespace trimming. Rejection assertions are what carry this. Scoped-reverting the branch-only `userId` guard on discord_remove_reaction plus zoneId on cloudflare_delete_zone produces 19 named failures; a shape-only check would have caught 4. Of the 9 reject vectors, shape sees just 2: `encodeURIComponent` turns `/` into `%2F`, which the URL parser never decodes back into a separator, so every separator-bearing traversal preserves the path shape exactly — and a trailing bare `.` collapses to the parent collection while keeping the segment count. --- apps/sim/tools/cloudflare/path_safety.test.ts | 283 ++++++++++++---- apps/sim/tools/discord/path_safety.test.ts | 304 ++++++++++++++---- 2 files changed, 452 insertions(+), 135 deletions(-) diff --git a/apps/sim/tools/cloudflare/path_safety.test.ts b/apps/sim/tools/cloudflare/path_safety.test.ts index a5e9bac4632..beb17598825 100644 --- a/apps/sim/tools/cloudflare/path_safety.test.ts +++ b/apps/sim/tools/cloudflare/path_safety.test.ts @@ -11,41 +11,44 @@ * user's Cloudflare API token — at an arbitrary Cloudflare resource, including * on the DELETE zone and DELETE bucket routes. * - * `encodeURIComponent` is NOT enough, which is why the vector list below keeps - * the bare `.` and `..` segments: both are made of unreserved characters, so - * they survive encoding untouched and the URL parser then removes them as dot - * segments, popping one path segment off a fixed host. Every assertion here - * resolves the built URL with `new URL(...)` — the same normalization `fetch` - * performs — rather than string-matching the template output, because string - * matching is exactly what let this through. + * `encodeURIComponent` is NOT enough: `.` and `..` are unreserved, so they + * survive encoding untouched and the URL parser then removes them as dot + * segments. Every assertion here resolves the built URL with `new URL(...)` — + * the same normalization `fetch` performs — rather than string-matching the + * template output, because string matching is what let this through. * - * The suite enumerates **(tool, param) pairs** and fuzzes one param at a time, - * holding every sibling at a safe value. Fuzzing all params at once cannot work - * here: the first guard to throw aborts URL construction, so a tool's remaining - * params stop being exercised the moment one of them is fixed. Pair enumeration - * is what makes "a newly unguarded param fails CI" actually true for a tool that - * already has a guarded param — the dominant shape in this service, where - * `accountId` + `appId` + `policyId` and `zoneId` + `rulesetId` + `ruleId` - * share one path. + * Two independent blind spots shape this file, and both are load-bearing: + * + * 1. **Fuzz one param at a time.** URL construction is eager, so filling every + * param with the same vector means the first guard to throw aborts the whole + * case and every sibling goes untested — once a tool has one guard, a newly + * unguarded sibling can no longer fail CI. So the suite enumerates + * (tool, param) pairs and holds every sibling at a safe value. + * 2. **Assert rejection, not just shape.** A bare `.` in the FINAL segment + * collapses invisibly: `/zones/abc/rulesets/.` normalizes to + * `/zones/abc/rulesets/`, which keeps the segment count and every other + * segment intact, so a shape-only check passes with the guard removed. Since + * the guarded id is the last segment on `delete_zone`, `delete_r2_bucket`, + * `delete_ruleset_rule` and friends — all DELETEs — that is exactly where a + * shape check is blindest. `MUST_REJECT` therefore asserts a throw. */ import { describe, expect, it } from 'vitest' import * as cloudflareTools from '@/tools/cloudflare' -import type { ToolConfig } from '@/tools/types' +import { deleteZoneTool } from '@/tools/cloudflare/delete_zone' const API_ORIGIN = 'https://api.cloudflare.com' const API_PREFIX = '/client/v4/' const CREDENTIAL_PARAM = 'apiKey' /** - * Vectors the guard must **reject outright**. Each is either a bare dot segment - * or carries a path separator, so encoding it would leave a live traversal. - * The bare `.` and `..` entries are the whole point: their omission is why an - * `encodeURIComponent`-only fix looks correct while the hole stays live. + * Vectors the guard must **reject outright**. Each is a bare dot segment or + * carries a path separator, so encoding it would leave a live traversal. */ const MUST_REJECT = [ '..', '.', ' .. ', + ' . ', '../../accounts/victim-account', '..%2f..%2faccounts/victim-account', '023e105f4ecef8ad9ca31a8372d0c353/../../accounts/victim-account', @@ -76,38 +79,73 @@ const LEGITIMATE_IDS = [ 'v1.2.3', ] as const +/** + * Tools that legitimately build no URL from params, so the pair enumeration + * cannot reach them: `create_zone` posts to a static collection URL, and + * `get_zone_settings` fans out through an internal operation (its own + * `zoneSettingUrl` already rejects dot segments, asserted in cloudflare.test.ts). + * Named explicitly rather than silently skipped: a tool that loses its URL + * builder must surface here rather than quietly dropping out of coverage. + */ +const TOOLS_WITHOUT_PARAM_BUILT_URLS = [ + 'cloudflare_create_zone', + 'cloudflare_get_zone_settings', +] as const + const SAFE_ID = 'SAFEID' const PROBE = 'PROBEVALUE' const TRIM_SAMPLE = '023e105f4ecef8ad9ca31a8372d0c353' -type AnyTool = ToolConfig +interface PathToolParam { + type?: string + required?: boolean +} + +/** The structural slice of a tool config this suite needs; avoids `any`. */ +interface ServiceTool { + id: string + params?: Record + request?: { url?: unknown } +} + +/** A tool that builds its request URL from params, so it can be probed. */ +type PathTool = ServiceTool & { + request: { url: (params: Record) => string } +} -function isCloudflareTool(value: unknown): value is AnyTool { +function isCloudflareTool(value: unknown): value is ServiceTool { return ( typeof value === 'object' && value !== null && - typeof (value as AnyTool).id === 'string' && - (value as AnyTool).id.startsWith('cloudflare_') + typeof (value as ServiceTool).id === 'string' && + (value as ServiceTool).id.startsWith('cloudflare_') ) } +function isPathTool(tool: ServiceTool): tool is PathTool { + return typeof tool.request?.url === 'function' +} + +function pathToolFor(value: unknown, id: string): PathTool { + if (!isCloudflareTool(value) || !isPathTool(value)) { + throw new Error(`${id} does not build its URL from params`) + } + return value +} + /** * Builds a param object with every string param at a known-safe value, then * applies one override so exactly one param carries the value under test. */ -function buildParams( - tool: AnyTool, - overrides: Record = {} -): Record { +function buildParams(tool: PathTool, overrides: Record): Record { const params: Record = { [CREDENTIAL_PARAM]: 'cf-token' } for (const [name, def] of Object.entries(tool.params ?? {})) { if (name === CREDENTIAL_PARAM) continue - const type = (def as { type?: string }).type - if (type === 'json' || type === 'array' || type === 'file[]') { + if (def.type === 'json' || def.type === 'array' || def.type === 'file[]') { params[name] = [] - } else if (type === 'number') { + } else if (def.type === 'number') { params[name] = 1 - } else if (type === 'boolean') { + } else if (def.type === 'boolean') { params[name] = false } else { params[name] = SAFE_ID @@ -116,43 +154,132 @@ function buildParams( return { ...params, ...overrides } } -function buildUrl(tool: AnyTool, overrides: Record = {}): URL { - const url = tool.request?.url - if (typeof url !== 'function') { - throw new Error(`${tool.id} does not build its URL from params`) - } - return new URL(url(buildParams(tool, overrides) as any)) +function buildUrl(tool: PathTool, overrides: Record = {}): URL { + return new URL(tool.request.url(buildParams(tool, overrides))) } function segmentsOf(pathname: string): string[] { return pathname.split('/') } -const TOOLS = Object.values(cloudflareTools) - .filter(isCloudflareTool) - .filter((tool) => typeof tool.request?.url === 'function') +const ALL_TOOLS = Object.values(cloudflareTools).filter(isCloudflareTool) +const TOOLS = ALL_TOOLS.filter(isPathTool) + +/** Surfaced, not swallowed — a silent skip is the blindness this suite fixes. */ +const SKIPPED_TOOL_IDS = ALL_TOOLS.filter((tool) => !isPathTool(tool)).map((tool) => tool.id) + +/** + * Tools whose URL will not build even from all-safe values. Distinct from a + * probe that throws (probing a *guarded* param is supposed to throw): this + * means the tool cannot be exercised at all, so it would vanish from coverage + * rather than fail. Asserted empty. + */ +const UNBUILDABLE: string[] = [] + +/** + * String literals the URL builder compares against, harvested from its own + * source so a branching builder is probed on every branch without this file + * enumerating them. Neither service switches on a literal today — every match + * set is currently empty — but a tool added later that picks its endpoint from + * an `action`/`operation` param would otherwise hide the identifiers that only + * appear on its non-default branch. + */ +function branchLiterals(tool: PathTool): string[] { + const source = String(tool.request.url) + const matches = [...source.matchAll(/[=!]==\s*'([^']{1,64})'|'([^']{1,64})'\s*[=!]==/g)] + return [...new Set(matches.map((match) => match[1] ?? match[2]).filter(Boolean))] +} + +interface ProbeContext { + label: string + overrides: Record +} /** - * Every (tool, param) pair where that param alone reaches the request path, - * discovered by probing one param at a time. A newly added tool — or a newly - * added path param on an existing tool — appears here with no edit to this file. + * Sibling contexts to probe each param under. A param that only appears on one + * branch of a conditional builder is invisible to a single all-params probe — + * no Cloudflare builder branches on a + * path param today, but probing without each optional param keeps that true as + * tools are added, rather than assuming it. */ -const PATH_PARAM_PAIRS = TOOLS.flatMap((tool) => - Object.keys(tool.params ?? {}) - .filter((param) => param !== CREDENTIAL_PARAM) - .filter((param) => { +function contextsFor(tool: PathTool): ProbeContext[] { + const contexts: ProbeContext[] = [{ label: 'all params', overrides: {} }] + + for (const [name, def] of Object.entries(tool.params ?? {})) { + if (name === CREDENTIAL_PARAM || def.required) continue + contexts.push({ label: `without ${name}`, overrides: { [name]: undefined } }) + } + + for (const literal of branchLiterals(tool)) { + for (const name of Object.keys(tool.params ?? {})) { + if (name === CREDENTIAL_PARAM) continue + contexts.push({ label: `${name}=${literal}`, overrides: { [name]: literal } }) + } + } + + return contexts +} + +interface PathParamCase { + name: string + tool: PathTool + param: string + overrides: Record + baseline: string[] +} + +/** + * Every (tool, param, branch) case where that param alone reaches the request + * path, discovered by probing one param at a time under every sibling context. + * Cases producing an identical path shape are collapsed, so a param guarded the + * same way on both branches is tested once. + */ +const PATH_PARAM_PAIRS: PathParamCase[] = [] +const seenCases = new Set() + +for (const tool of TOOLS) { + for (const context of contextsFor(tool)) { + for (const param of Object.keys(tool.params ?? {})) { + if (param === CREDENTIAL_PARAM || param in context.overrides) continue + + let baseline: string[] try { - return buildUrl(tool, { [param]: PROBE }).pathname.includes(PROBE) - } catch { - return false + baseline = segmentsOf(buildUrl(tool, { ...context.overrides, [param]: PROBE }).pathname) + } catch (error) { + if (context.label === 'all params') { + UNBUILDABLE.push(`${tool.id} / ${param}: ${(error as Error).message}`) + } + continue } - }) - .map((param) => ({ name: `${tool.id} / ${param}`, tool, param })) -) + + if (!baseline.some((segment) => segment.includes(PROBE))) continue + + const key = `${tool.id}|${param}|${baseline.join('/')}` + if (seenCases.has(key)) continue + seenCases.add(key) + + PATH_PARAM_PAIRS.push({ + name: `${tool.id} / ${param}${context.label === 'all params' ? '' : ` (${context.label})`}`, + tool, + param, + overrides: context.overrides, + baseline, + }) + } + } +} describe('cloudflare path-param traversal safety', () => { + it('can build every tool that declares a params-based URL', () => { + expect(UNBUILDABLE).toEqual([]) + }) + + it('accounts for every tool that builds no URL from params', () => { + expect([...SKIPPED_TOOL_IDS].sort()).toEqual([...TOOLS_WITHOUT_PARAM_BUILT_URLS].sort()) + }) + it('finds every (tool, param) pair that reaches the request path', () => { - expect(PATH_PARAM_PAIRS.length).toBeGreaterThanOrEqual(65) + expect(PATH_PARAM_PAIRS.length).toBeGreaterThanOrEqual(70) }) it('covers multi-param paths, where whole-object fuzzing goes blind', () => { @@ -160,22 +287,19 @@ describe('cloudflare path-param traversal safety', () => { for (const { tool } of PATH_PARAM_PAIRS) { counts.set(tool.id, (counts.get(tool.id) ?? 0) + 1) } - const multiParamTools = [...counts.values()].filter((count) => count > 1) - expect(multiParamTools.length).toBeGreaterThanOrEqual(15) + expect([...counts.values()].filter((count) => count > 1).length).toBeGreaterThanOrEqual(15) }) - describe.each(PATH_PARAM_PAIRS)('$name', ({ tool, param }) => { - const baseline = segmentsOf(buildUrl(tool, { [param]: PROBE }).pathname) + describe.each(PATH_PARAM_PAIRS)('$name', ({ tool, param, overrides, baseline }) => { + const withValue = (value: unknown) => buildUrl(tool, { ...overrides, [param]: value }) it.each(MUST_REJECT)('rejects %j outright', (value) => { - expect(() => buildUrl(tool, { [param]: value })).toThrow( - new RegExp(`${param}|path traversal|path separator`) - ) + expect(() => withValue(value)).toThrow(new RegExp(`${param}|path traversal|path separator`)) }) it.each(MUST_NEUTRALIZE)('confines %j to a single segment', (value) => { - const url = buildUrl(tool, { [param]: value }) + const url = withValue(value) expect(url.origin).toBe(API_ORIGIN) expect(url.pathname.startsWith(API_PREFIX)).toBe(true) @@ -191,7 +315,7 @@ describe('cloudflare path-param traversal safety', () => { }) it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { - const actual = segmentsOf(buildUrl(tool, { [param]: value }).pathname) + const actual = segmentsOf(withValue(value).pathname) expect(actual).toHaveLength(baseline.length) baseline.forEach((segment, index) => { @@ -200,7 +324,7 @@ describe('cloudflare path-param traversal safety', () => { }) it('trims surrounding whitespace off a legitimate value', () => { - const actual = segmentsOf(buildUrl(tool, { [param]: ` ${TRIM_SAMPLE} ` }).pathname) + const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) baseline.forEach((segment, index) => { expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) @@ -208,3 +332,32 @@ describe('cloudflare path-param traversal safety', () => { }) }) }) + +/** + * Pins the reason `MUST_REJECT` asserts a throw rather than comparing shape. + * + * `zoneId` is the final segment of `DELETE /zones/{zone_id}`, so a bare `.` + * there addresses the zone *collection* while leaving the segment count and + * every other segment identical. A shape-only assertion cannot see it. If this + * test ever fails because the path stopped ending in the id, the reasoning + * above needs revisiting. + */ +describe('a trailing dot segment is invisible to a shape check', () => { + const tool = pathToolFor(deleteZoneTool, 'cloudflare_delete_zone') + + it('collapses to the parent collection without changing the segment count', () => { + const baseline = segmentsOf( + new URL('https://api.cloudflare.com/client/v4/zones/SAFEID').pathname + ) + const collapsed = segmentsOf( + new URL(`https://api.cloudflare.com/client/v4/zones/${encodeURIComponent('.')}`).pathname + ) + + expect(collapsed).toHaveLength(baseline.length) + expect(collapsed.at(-1)).toBe('') + }) + + it('is caught anyway, because the guard rejects rather than encodes', () => { + expect(() => buildUrl(tool, { zoneId: '.' })).toThrow(/path traversal is not allowed/) + }) +}) diff --git a/apps/sim/tools/discord/path_safety.test.ts b/apps/sim/tools/discord/path_safety.test.ts index 5af3edaaff6..f530f3a1dd2 100644 --- a/apps/sim/tools/discord/path_safety.test.ts +++ b/apps/sim/tools/discord/path_safety.test.ts @@ -11,42 +11,46 @@ * and the workspace's Discord bot token — at an arbitrary Discord resource, * including on the DELETE channel, DELETE role, and ban routes. * - * `encodeURIComponent` is NOT enough, which is why the vector list below keeps - * the bare `.` and `..` segments: both are made of unreserved characters, so - * they survive encoding untouched and the URL parser then removes them as dot - * segments, popping one path segment off a fixed host. Every assertion here - * resolves the built URL with `new URL(...)` — the same normalization `fetch` - * performs — rather than string-matching the template output, because string - * matching is exactly what let this through. + * `encodeURIComponent` is NOT enough: `.` and `..` are unreserved, so they + * survive encoding untouched and the URL parser then removes them as dot + * segments. Every assertion here resolves the built URL with `new URL(...)` — + * the same normalization `fetch` performs — rather than string-matching the + * template output, because string matching is what let this through. * - * The suite enumerates **(tool, param) pairs** and fuzzes one param at a time, - * holding every sibling at a safe value. Fuzzing all params at once cannot work - * here: the first guard to throw aborts URL construction, so a tool's remaining - * params stop being exercised the moment one of them is fixed. Pair enumeration - * is what makes "a newly unguarded param fails CI" actually true for a tool that - * already has a guarded param — the dominant shape in this service, where - * `channelId` + `messageId` and `serverId` + `roleId` share one path. + * Two independent blind spots shape this file, and both are load-bearing: + * + * 1. **Fuzz one param at a time.** URL construction is eager, so filling every + * param with the same vector means the first guard to throw aborts the whole + * case and every sibling goes untested — once a tool has one guard, a newly + * unguarded sibling can no longer fail CI. So the suite enumerates + * (tool, param) pairs and holds every sibling at a safe value. + * 2. **Assert rejection, not just shape.** A bare `.` in the FINAL segment + * collapses invisibly: `/channels/123/messages/.` normalizes to + * `/channels/123/messages/`, which keeps the segment count and every other + * segment intact, so a shape-only check passes with the guard removed. Since + * the guarded id is the last segment on `delete_message`, `delete_channel`, + * `delete_role` and friends — all DELETEs — that is exactly where a shape + * check is blindest. `MUST_REJECT` therefore asserts a throw. */ import { describe, expect, it } from 'vitest' import * as discordTools from '@/tools/discord' import { discordAssignRoleTool } from '@/tools/discord/assign_role' +import { discordDeleteMessageTool } from '@/tools/discord/delete_message' import { discordGetMemberTool } from '@/tools/discord/get_member' -import type { ToolConfig } from '@/tools/types' const API_ORIGIN = 'https://discord.com' const API_PREFIX = '/api/v10/' const CREDENTIAL_PARAM = 'botToken' /** - * Vectors the guard must **reject outright**. Each is either a bare dot segment - * or carries a path separator, so encoding it would leave a live traversal. - * The bare `.` and `..` entries are the whole point: their omission is why an - * `encodeURIComponent`-only fix looks correct while the hole stays live. + * Vectors the guard must **reject outright**. Each is a bare dot segment or + * carries a path separator, so encoding it would leave a live traversal. */ const MUST_REJECT = [ '..', '.', ' .. ', + ' . ', '../../guilds/987654321098765432', '..%2f..%2fguilds/987654321098765432', '123456789012345678/../../../guilds/987654321098765432', @@ -78,38 +82,68 @@ const LEGITIMATE_IDS = [ 'v1.2.3', ] as const +/** + * Tools that legitimately build no URL from params, so the pair enumeration + * cannot reach them. Named explicitly rather than silently skipped: a tool that + * loses its URL builder — or a new operation-dispatched tool — must surface + * here rather than quietly dropping out of coverage. + */ +const TOOLS_WITHOUT_PARAM_BUILT_URLS = ['discord_send_message'] as const + const SAFE_ID = 'SAFEID' const PROBE = 'PROBEVALUE' const TRIM_SAMPLE = '123456789012345678' -type AnyTool = ToolConfig +interface PathToolParam { + type?: string + required?: boolean +} + +/** The structural slice of a tool config this suite needs; avoids `any`. */ +interface ServiceTool { + id: string + params?: Record + request?: { url?: unknown } +} + +/** A tool that builds its request URL from params, so it can be probed. */ +type PathTool = ServiceTool & { + request: { url: (params: Record) => string } +} -function isDiscordTool(value: unknown): value is AnyTool { +function isDiscordTool(value: unknown): value is ServiceTool { return ( typeof value === 'object' && value !== null && - typeof (value as AnyTool).id === 'string' && - (value as AnyTool).id.startsWith('discord_') + typeof (value as ServiceTool).id === 'string' && + (value as ServiceTool).id.startsWith('discord_') ) } +function isPathTool(tool: ServiceTool): tool is PathTool { + return typeof tool.request?.url === 'function' +} + +function pathToolFor(value: unknown, id: string): PathTool { + if (!isDiscordTool(value) || !isPathTool(value)) { + throw new Error(`${id} does not build its URL from params`) + } + return value +} + /** * Builds a param object with every string param at a known-safe value, then * applies one override so exactly one param carries the value under test. */ -function buildParams( - tool: AnyTool, - overrides: Record = {} -): Record { +function buildParams(tool: PathTool, overrides: Record): Record { const params: Record = { [CREDENTIAL_PARAM]: 'bot-token' } for (const [name, def] of Object.entries(tool.params ?? {})) { if (name === CREDENTIAL_PARAM) continue - const type = (def as { type?: string }).type - if (type === 'json' || type === 'array' || type === 'file[]') { + if (def.type === 'json' || def.type === 'array' || def.type === 'file[]') { params[name] = [] - } else if (type === 'number') { + } else if (def.type === 'number') { params[name] = 1 - } else if (type === 'boolean') { + } else if (def.type === 'boolean') { params[name] = false } else { params[name] = SAFE_ID @@ -118,43 +152,143 @@ function buildParams( return { ...params, ...overrides } } -function buildUrl(tool: AnyTool, overrides: Record = {}): URL { - const url = tool.request?.url - if (typeof url !== 'function') { - throw new Error(`${tool.id} does not build its URL from params`) - } - return new URL(url(buildParams(tool, overrides) as any)) +function buildUrl(tool: PathTool, overrides: Record = {}): URL { + return new URL(tool.request.url(buildParams(tool, overrides))) } function segmentsOf(pathname: string): string[] { return pathname.split('/') } -const TOOLS = Object.values(discordTools) - .filter(isDiscordTool) - .filter((tool) => typeof tool.request?.url === 'function') +const ALL_TOOLS = Object.values(discordTools).filter(isDiscordTool) +const TOOLS = ALL_TOOLS.filter(isPathTool) + +/** Surfaced, not swallowed — a silent skip is the blindness this suite fixes. */ +const SKIPPED_TOOL_IDS = ALL_TOOLS.filter((tool) => !isPathTool(tool)).map((tool) => tool.id) + +/** + * Tools whose URL will not build even from all-safe values. Distinct from a + * probe that throws (probing a *guarded* param is supposed to throw): this + * means the tool cannot be exercised at all, so it would vanish from coverage + * rather than fail. Asserted empty. + */ +const UNBUILDABLE: string[] = [] + +/** + * String literals the URL builder compares against, harvested from its own + * source so a branching builder is probed on every branch without this file + * enumerating them. Neither service switches on a literal today — every match + * set is currently empty — but a tool added later that picks its endpoint from + * an `action`/`operation` param would otherwise hide the identifiers that only + * appear on its non-default branch. + */ +function branchLiterals(tool: PathTool): string[] { + const source = String(tool.request.url) + const matches = [...source.matchAll(/[=!]==\s*'([^']{1,64})'|'([^']{1,64})'\s*[=!]==/g)] + return [...new Set(matches.map((match) => match[1] ?? match[2]).filter(Boolean))] +} + +interface ProbeContext { + label: string + overrides: Record +} /** - * Every (tool, param) pair where that param alone reaches the request path, - * discovered by probing one param at a time. A newly added tool — or a newly - * added path param on an existing tool — appears here with no edit to this file. + * Sibling contexts to probe each param under. A param that only appears on one + * branch of a conditional builder is invisible to a single all-params probe — + * `create_thread` switches on whether `messageId` is present, and + * `remove_reaction` on whether `userId` is, so the branch taken when they are + * ABSENT is never reached if discovery always fills them. */ -const PATH_PARAM_PAIRS = TOOLS.flatMap((tool) => - Object.keys(tool.params ?? {}) - .filter((param) => param !== CREDENTIAL_PARAM) - .filter((param) => { +function contextsFor(tool: PathTool): ProbeContext[] { + const contexts: ProbeContext[] = [{ label: 'all params', overrides: {} }] + + for (const [name, def] of Object.entries(tool.params ?? {})) { + if (name === CREDENTIAL_PARAM || def.required) continue + contexts.push({ label: `without ${name}`, overrides: { [name]: undefined } }) + } + + for (const literal of branchLiterals(tool)) { + for (const name of Object.keys(tool.params ?? {})) { + if (name === CREDENTIAL_PARAM) continue + contexts.push({ label: `${name}=${literal}`, overrides: { [name]: literal } }) + } + } + + return contexts +} + +interface PathParamCase { + name: string + tool: PathTool + param: string + overrides: Record + baseline: string[] +} + +/** + * Every (tool, param, branch) case where that param alone reaches the request + * path, discovered by probing one param at a time under every sibling context. + * Cases producing an identical path shape are collapsed, so a param guarded the + * same way on both branches is tested once. + */ +const PATH_PARAM_PAIRS: PathParamCase[] = [] +const seenCases = new Set() + +for (const tool of TOOLS) { + for (const context of contextsFor(tool)) { + for (const param of Object.keys(tool.params ?? {})) { + if (param === CREDENTIAL_PARAM || param in context.overrides) continue + + let baseline: string[] try { - return buildUrl(tool, { [param]: PROBE }).pathname.includes(PROBE) - } catch { - return false + baseline = segmentsOf(buildUrl(tool, { ...context.overrides, [param]: PROBE }).pathname) + } catch (error) { + if (context.label === 'all params') { + UNBUILDABLE.push(`${tool.id} / ${param}: ${(error as Error).message}`) + } + continue } - }) - .map((param) => ({ name: `${tool.id} / ${param}`, tool, param })) -) + + if (!baseline.some((segment) => segment.includes(PROBE))) continue + + const key = `${tool.id}|${param}|${baseline.join('/')}` + if (seenCases.has(key)) continue + seenCases.add(key) + + PATH_PARAM_PAIRS.push({ + name: `${tool.id} / ${param}${context.label === 'all params' ? '' : ` (${context.label})`}`, + tool, + param, + overrides: context.overrides, + baseline, + }) + } + } +} describe('discord path-param traversal safety', () => { + it('can build every tool that declares a params-based URL', () => { + expect(UNBUILDABLE).toEqual([]) + }) + + it('accounts for every tool that builds no URL from params', () => { + expect([...SKIPPED_TOOL_IDS].sort()).toEqual([...TOOLS_WITHOUT_PARAM_BUILT_URLS].sort()) + }) + it('finds every (tool, param) pair that reaches the request path', () => { - expect(PATH_PARAM_PAIRS.length).toBeGreaterThanOrEqual(60) + expect(PATH_PARAM_PAIRS.length).toBeGreaterThanOrEqual(64) + }) + + /** + * Ratchets the branch coverage. `create_thread` and `remove_reaction` pick a + * different path when `messageId` / `userId` are absent; probing only the + * all-params shape left those branches — and the `/@me` form — untested. + */ + it('probes conditional builders on their non-default branch too', () => { + const branchCases = PATH_PARAM_PAIRS.filter((testCase) => testCase.name.includes('(without ')) + + expect(branchCases.length).toBeGreaterThanOrEqual(4) }) it('covers multi-param paths, where whole-object fuzzing goes blind', () => { @@ -162,22 +296,19 @@ describe('discord path-param traversal safety', () => { for (const { tool } of PATH_PARAM_PAIRS) { counts.set(tool.id, (counts.get(tool.id) ?? 0) + 1) } - const multiParamTools = [...counts.values()].filter((count) => count > 1) - expect(multiParamTools.length).toBeGreaterThanOrEqual(15) + expect([...counts.values()].filter((count) => count > 1).length).toBeGreaterThanOrEqual(15) }) - describe.each(PATH_PARAM_PAIRS)('$name', ({ tool, param }) => { - const baseline = segmentsOf(buildUrl(tool, { [param]: PROBE }).pathname) + describe.each(PATH_PARAM_PAIRS)('$name', ({ tool, param, overrides, baseline }) => { + const withValue = (value: unknown) => buildUrl(tool, { ...overrides, [param]: value }) it.each(MUST_REJECT)('rejects %j outright', (value) => { - expect(() => buildUrl(tool, { [param]: value })).toThrow( - new RegExp(`${param}|path traversal|path separator`) - ) + expect(() => withValue(value)).toThrow(new RegExp(`${param}|path traversal|path separator`)) }) it.each(MUST_NEUTRALIZE)('confines %j to a single segment', (value) => { - const url = buildUrl(tool, { [param]: value }) + const url = withValue(value) expect(url.origin).toBe(API_ORIGIN) expect(url.pathname.startsWith(API_PREFIX)).toBe(true) @@ -192,7 +323,7 @@ describe('discord path-param traversal safety', () => { }) it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { - const actual = segmentsOf(buildUrl(tool, { [param]: value }).pathname) + const actual = segmentsOf(withValue(value).pathname) expect(actual).toHaveLength(baseline.length) baseline.forEach((segment, index) => { @@ -201,7 +332,7 @@ describe('discord path-param traversal safety', () => { }) it('trims surrounding whitespace off a legitimate value', () => { - const actual = segmentsOf(buildUrl(tool, { [param]: ` ${TRIM_SAMPLE} ` }).pathname) + const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) baseline.forEach((segment, index) => { expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) @@ -210,6 +341,36 @@ describe('discord path-param traversal safety', () => { }) }) +/** + * Pins the reason `MUST_REJECT` asserts a throw rather than comparing shape. + * + * `messageId` is the final segment of `DELETE /channels/{id}/messages/{id}`, so + * a bare `.` there deletes nothing and instead addresses the *collection* — + * while leaving the segment count and every other segment identical. A + * shape-only assertion cannot see it. If this test ever starts failing because + * the path stopped ending in the id, the reasoning above needs revisiting. + */ +describe('a trailing dot segment is invisible to a shape check', () => { + const tool = pathToolFor(discordDeleteMessageTool, 'discord_delete_message') + + it('collapses to the parent collection without changing the segment count', () => { + const baseline = segmentsOf( + new URL('https://discord.com/api/v10/channels/123/messages/SAFEID').pathname + ) + const collapsed = segmentsOf( + new URL(`https://discord.com/api/v10/channels/123/messages/${encodeURIComponent('.')}`) + .pathname + ) + + expect(collapsed).toHaveLength(baseline.length) + expect(collapsed.at(-1)).toBe('') + }) + + it('is caught anyway, because the guard rejects rather than encodes', () => { + expect(() => buildUrl(tool, { messageId: '.' })).toThrow(/path traversal is not allowed/) + }) +}) + /** * An LLM tool call carries JSON, so a snowflake can arrive as a `number` rather * than the declared `string`. The guard must not turn that into a bogus segment. @@ -220,13 +381,16 @@ describe('discord path-param traversal safety', () => { * and any numeric id inside the safe range, pass through as their decimal text. */ describe('discord snowflakes supplied as JSON numbers', () => { + const getMember = pathToolFor(discordGetMemberTool, 'discord_get_member') + const assignRole = pathToolFor(discordAssignRoleTool, 'discord_assign_role') + it('accepts a numeric id inside the safe integer range', () => { const url = new URL( - discordGetMemberTool.request.url({ + getMember.request.url({ botToken: 'bot-token', serverId: 8035111022467891, userId: 8035111022467892, - } as any) + }) ) expect(url.pathname).toBe('/api/v10/guilds/8035111022467891/members/8035111022467892') @@ -234,11 +398,11 @@ describe('discord snowflakes supplied as JSON numbers', () => { it('accepts a full-width snowflake supplied as a bigint', () => { const url = new URL( - discordGetMemberTool.request.url({ + getMember.request.url({ botToken: 'bot-token', serverId: 1234567890123456789n, userId: 987654321098765432n, - } as any) + }) ) expect(url.pathname).toBe('/api/v10/guilds/1234567890123456789/members/987654321098765432') @@ -250,12 +414,12 @@ describe('discord snowflakes supplied as JSON numbers', () => { expect(roundedSnowflake).not.toBe(1234567890123456789n) expect(() => - discordAssignRoleTool.request.url({ + assignRole.request.url({ botToken: 'bot-token', serverId: roundedSnowflake, userId: '123456789012345678', roleId: '123456789012345678', - } as any) + }) ).toThrow(/serverId is too large to be represented exactly/) }) }) From 71bc9da54dd9efb70142bdcf72c503bb95ef2919 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 21:54:20 -0700 Subject: [PATCH 4/9] fix(discord): stop a pre-trim from rejecting numeric snowflakes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `remove_reaction` and `create_thread` tested an optional param for presence with `params.x?.trim()`. That throws a bare `TypeError: params.userId?.trim is not a function` on a JSON number — naming neither the tool nor the parameter — and it throws BEFORE `safeUrlPathSegment`, so the number and bigint support that helper deliberately provides never applied to those two tools. An LLM tool call can and does deliver a snowflake as a JSON number, so this contradicted a claim made for this PR: that numeric snowflakes work. They worked everywhere the guard was reached directly (`get_member` builds fine from two numeric ids) and failed on exactly the two builders that pre-trimmed. Caught by greptile and cubic independently; both were right. Presence is now tested by `isProvidedParam` in a new `tools/discord/utils.ts` (two call sites, per the utils rule), which does not assume a string and hands the raw value to `safeUrlPathSegment` — the single place that owns kind checking and named errors. A blank or whitespace-only string still counts as absent, so the branch each builder selects is unchanged. The harness missed this because it only ever passed strings. Both suites now assert, for every one of the 137 (tool, param) cases, that a safe-range number and a bigint build the same path as their decimal string — which is what catches a pre-trim anywhere, not just at these two sites. Verified red-first: restoring either pre-trim fails exactly 4 of those assertions, naming `discord_remove_reaction / userId` and `discord_create_thread / messageId`. --- apps/sim/tools/cloudflare/path_safety.test.ts | 27 +++++++++++++++++++ apps/sim/tools/discord/create_thread.ts | 6 ++--- apps/sim/tools/discord/path_safety.test.ts | 27 +++++++++++++++++++ apps/sim/tools/discord/remove_reaction.ts | 6 +++-- apps/sim/tools/discord/utils.ts | 24 +++++++++++++++++ 5 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 apps/sim/tools/discord/utils.ts diff --git a/apps/sim/tools/cloudflare/path_safety.test.ts b/apps/sim/tools/cloudflare/path_safety.test.ts index beb17598825..86f620a82e5 100644 --- a/apps/sim/tools/cloudflare/path_safety.test.ts +++ b/apps/sim/tools/cloudflare/path_safety.test.ts @@ -323,6 +323,33 @@ describe('cloudflare path-param traversal safety', () => { }) }) + /** + * A safe-range numeric id must build the same path as its decimal string. + * + * This is what catches a pre-trim anywhere in a URL builder. A + * `params.x?.trim()` ahead of the guard throws a bare + * `TypeError: params.x?.trim is not a function` on a JSON number, and it + * throws BEFORE `safeUrlPathSegment` — which accepts numbers and bigints — + * ever runs. The first version of this suite passed only strings, so the + * `remove_reaction` and `create_thread` pre-trims survived it; both bots + * caught what the harness could not. + */ + it('accepts a safe-range numeric id identically to its decimal string', () => { + const numeric = 8035111022467891 + + expect(segmentsOf(withValue(numeric).pathname)).toEqual( + segmentsOf(withValue(String(numeric)).pathname) + ) + }) + + it('accepts a bigint id identically to its decimal string', () => { + const snowflake = 1234567890123456789n + + expect(segmentsOf(withValue(snowflake).pathname)).toEqual( + segmentsOf(withValue(snowflake.toString()).pathname) + ) + }) + it('trims surrounding whitespace off a legitimate value', () => { const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) diff --git a/apps/sim/tools/discord/create_thread.ts b/apps/sim/tools/discord/create_thread.ts index 83a2e5462e7..defbac9a9b7 100644 --- a/apps/sim/tools/discord/create_thread.ts +++ b/apps/sim/tools/discord/create_thread.ts @@ -1,4 +1,5 @@ import type { DiscordCreateThreadParams, DiscordCreateThreadResponse } from '@/tools/discord/types' +import { isProvidedParam } from '@/tools/discord/utils' import type { ToolConfig } from '@/tools/types' import { safeUrlPathSegment } from '@/tools/url-path' @@ -60,9 +61,8 @@ export const discordCreateThreadTool: ToolConfig< request: { url: (params: DiscordCreateThreadParams) => { - const messageId = params.messageId?.trim() - if (messageId) { - return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(messageId, 'messageId')}/threads` + if (isProvidedParam(params.messageId)) { + return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(params.messageId, 'messageId')}/threads` } return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/threads` }, diff --git a/apps/sim/tools/discord/path_safety.test.ts b/apps/sim/tools/discord/path_safety.test.ts index f530f3a1dd2..830c30e12a2 100644 --- a/apps/sim/tools/discord/path_safety.test.ts +++ b/apps/sim/tools/discord/path_safety.test.ts @@ -331,6 +331,33 @@ describe('discord path-param traversal safety', () => { }) }) + /** + * A safe-range numeric id must build the same path as its decimal string. + * + * This is what catches a pre-trim anywhere in a URL builder. A + * `params.x?.trim()` ahead of the guard throws a bare + * `TypeError: params.x?.trim is not a function` on a JSON number, and it + * throws BEFORE `safeUrlPathSegment` — which accepts numbers and bigints — + * ever runs. The first version of this suite passed only strings, so the + * `remove_reaction` and `create_thread` pre-trims survived it; both bots + * caught what the harness could not. + */ + it('accepts a safe-range numeric id identically to its decimal string', () => { + const numeric = 8035111022467891 + + expect(segmentsOf(withValue(numeric).pathname)).toEqual( + segmentsOf(withValue(String(numeric)).pathname) + ) + }) + + it('accepts a bigint id identically to its decimal string', () => { + const snowflake = 1234567890123456789n + + expect(segmentsOf(withValue(snowflake).pathname)).toEqual( + segmentsOf(withValue(snowflake.toString()).pathname) + ) + }) + it('trims surrounding whitespace off a legitimate value', () => { const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) diff --git a/apps/sim/tools/discord/remove_reaction.ts b/apps/sim/tools/discord/remove_reaction.ts index 4800a2ee778..c751d90f964 100644 --- a/apps/sim/tools/discord/remove_reaction.ts +++ b/apps/sim/tools/discord/remove_reaction.ts @@ -2,6 +2,7 @@ import type { DiscordRemoveReactionParams, DiscordRemoveReactionResponse, } from '@/tools/discord/types' +import { isProvidedParam } from '@/tools/discord/utils' import type { ToolConfig } from '@/tools/types' import { safeUrlPathSegment } from '@/tools/url-path' @@ -57,8 +58,9 @@ export const discordRemoveReactionTool: ToolConfig< request: { url: (params: DiscordRemoveReactionParams) => { const encodedEmoji = safeUrlPathSegment(params.emoji, 'emoji') - const userId = params.userId?.trim() - const userPart = userId ? `/${safeUrlPathSegment(userId, 'userId')}` : '/@me' + const userPart = isProvidedParam(params.userId) + ? `/${safeUrlPathSegment(params.userId, 'userId')}` + : '/@me' return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(params.messageId, 'messageId')}/reactions/${encodedEmoji}${userPart}` }, method: 'DELETE', diff --git a/apps/sim/tools/discord/utils.ts b/apps/sim/tools/discord/utils.ts new file mode 100644 index 00000000000..255de7ca8c8 --- /dev/null +++ b/apps/sim/tools/discord/utils.ts @@ -0,0 +1,24 @@ +/** + * Whether an optional Discord tool param was supplied. + * + * This exists instead of `params.x?.trim()`. Tool params are declared + * `type: 'string'`, but nothing enforces that before the value reaches a URL + * builder: an LLM tool call can deliver a snowflake as a JSON **number**, and + * stored workflow state can too. Calling `.trim()` on one throws a bare + * `TypeError: params.userId?.trim is not a function` — naming neither the tool + * nor the parameter — and it throws *before* `safeUrlPathSegment` runs, so the + * number and bigint support that helper deliberately provides never applies. + * + * Presence is therefore tested without assuming the value is a string, and the + * raw value is handed to `safeUrlPathSegment`, which owns every kind check and + * reports a named error for the shapes it refuses. + * + * A blank or whitespace-only string counts as absent, matching the `?.trim()` + * truthiness test this replaces, so an omitted param still selects the same + * branch it always did. + */ +export function isProvidedParam(value: T): value is NonNullable { + if (value === null || value === undefined) return false + if (typeof value === 'string') return value.trim() !== '' + return true +} From 48175cd95fc5602ad258bbd83edcf15bca3963cb Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 22:05:50 -0700 Subject: [PATCH 5/9] fix(discord): finish the numeric-id fix in body and operation builders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The URL builder is not the only place a tool touches an id, and fixing only the URL moved the failure one step later rather than removing it. `create_thread` reads `messageId` again in its `body` to decide the thread type (a standalone thread must pin `type` to PUBLIC_THREAD; a message-backed one must not). That read was still `params.messageId?.trim()`, so a numeric messageId now passed the URL and threw a bare TypeError building the body. Caught by greptile on the previous head; it was right. `send_message` had the same class in its `operation.input` mapper. It is not a traversal sink — `executeDiscordSendMessage` validates `channelId` through `validateNumericId` before `lib/internal/discord/client.ts` interpolates it, and that validator explicitly accepts `string | number` — but the mapper's `params.channelId.trim()` threw on a number before the validator that was built to accept one ever ran. It now goes through `safeUrlPathSegment`, which is a no-op for a valid channel id (snowflakes are all digits, and digits are unreserved) while rejecting a precision-lost number by name. Both suites now also assert, for every one of the 137 (tool, param) cases, that `request.body` and `request.headers` build from a numeric id without a TypeError — checking specifically for TypeError so a builder's deliberate domain error does not produce a false failure. That is the assertion the suites lacked: they only ever exercised `request.url`, which is exactly why the `create_thread` body read survived them. Verified red-first — restoring that one `?.trim()` fails it, naming `discord_create_thread / messageId`. --- apps/sim/tools/cloudflare/path_safety.test.ts | 38 ++++++++++++++++++- apps/sim/tools/discord/create_thread.ts | 2 +- apps/sim/tools/discord/path_safety.test.ts | 38 ++++++++++++++++++- apps/sim/tools/discord/send_message.ts | 3 +- 4 files changed, 75 insertions(+), 6 deletions(-) diff --git a/apps/sim/tools/cloudflare/path_safety.test.ts b/apps/sim/tools/cloudflare/path_safety.test.ts index 86f620a82e5..9c5813c46cf 100644 --- a/apps/sim/tools/cloudflare/path_safety.test.ts +++ b/apps/sim/tools/cloudflare/path_safety.test.ts @@ -105,12 +105,16 @@ interface PathToolParam { interface ServiceTool { id: string params?: Record - request?: { url?: unknown } + request?: { url?: unknown; body?: unknown; headers?: unknown } } /** A tool that builds its request URL from params, so it can be probed. */ type PathTool = ServiceTool & { - request: { url: (params: Record) => string } + request: { + url: (params: Record) => string + body?: (params: Record) => unknown + headers?: (params: Record) => unknown + } } function isCloudflareTool(value: unknown): value is ServiceTool { @@ -122,6 +126,21 @@ function isCloudflareTool(value: unknown): value is ServiceTool { ) } +/** + * Whether calling `fn` raises a `TypeError` — the signature of a builder that + * assumed a param was a string (`params.x?.trim is not a function`). Domain + * errors thrown deliberately by a builder are not TypeErrors, so they pass + * through and do not cause a false failure here. + */ +function throwsTypeError(fn: () => unknown): boolean { + try { + fn() + return false + } catch (error) { + return error instanceof TypeError + } +} + function isPathTool(tool: ServiceTool): tool is PathTool { return typeof tool.request?.url === 'function' } @@ -350,6 +369,21 @@ describe('cloudflare path-param traversal safety', () => { ) }) + /** + * The URL is not the only builder that touches an id. `create_thread` also + * reads `messageId` in its `body` to decide the thread type, and a + * `?.trim()` there threw a bare TypeError on a numeric id *after* the URL + * had already accepted it — caught by review, not by this suite, because + * the suite only ever exercised `request.url`. + */ + it('builds body and headers from a numeric id without a TypeError', () => { + const numericParams = buildParams(tool, { ...overrides, [param]: 8035111022467891 }) + + expect(throwsTypeError(() => tool.request.url(numericParams))).toBe(false) + expect(throwsTypeError(() => tool.request.body?.(numericParams))).toBe(false) + expect(throwsTypeError(() => tool.request.headers?.(numericParams))).toBe(false) + }) + it('trims surrounding whitespace off a legitimate value', () => { const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) diff --git a/apps/sim/tools/discord/create_thread.ts b/apps/sim/tools/discord/create_thread.ts index defbac9a9b7..e50ed2a84ac 100644 --- a/apps/sim/tools/discord/create_thread.ts +++ b/apps/sim/tools/discord/create_thread.ts @@ -80,7 +80,7 @@ export const discordCreateThreadTool: ToolConfig< } // Standalone threads (no source message) default to PRIVATE_THREAD per the Discord API // unless `type` is explicitly set, so pin it to PUBLIC_THREAD (11) unless the caller opts out. - if (!params.messageId?.trim()) { + if (!isProvidedParam(params.messageId)) { body.type = params.isPublic === false ? 12 : 11 } return body diff --git a/apps/sim/tools/discord/path_safety.test.ts b/apps/sim/tools/discord/path_safety.test.ts index 830c30e12a2..ae0063a17a6 100644 --- a/apps/sim/tools/discord/path_safety.test.ts +++ b/apps/sim/tools/discord/path_safety.test.ts @@ -103,12 +103,16 @@ interface PathToolParam { interface ServiceTool { id: string params?: Record - request?: { url?: unknown } + request?: { url?: unknown; body?: unknown; headers?: unknown } } /** A tool that builds its request URL from params, so it can be probed. */ type PathTool = ServiceTool & { - request: { url: (params: Record) => string } + request: { + url: (params: Record) => string + body?: (params: Record) => unknown + headers?: (params: Record) => unknown + } } function isDiscordTool(value: unknown): value is ServiceTool { @@ -120,6 +124,21 @@ function isDiscordTool(value: unknown): value is ServiceTool { ) } +/** + * Whether calling `fn` raises a `TypeError` — the signature of a builder that + * assumed a param was a string (`params.x?.trim is not a function`). Domain + * errors thrown deliberately by a builder are not TypeErrors, so they pass + * through and do not cause a false failure here. + */ +function throwsTypeError(fn: () => unknown): boolean { + try { + fn() + return false + } catch (error) { + return error instanceof TypeError + } +} + function isPathTool(tool: ServiceTool): tool is PathTool { return typeof tool.request?.url === 'function' } @@ -358,6 +377,21 @@ describe('discord path-param traversal safety', () => { ) }) + /** + * The URL is not the only builder that touches an id. `create_thread` also + * reads `messageId` in its `body` to decide the thread type, and a + * `?.trim()` there threw a bare TypeError on a numeric id *after* the URL + * had already accepted it — caught by review, not by this suite, because + * the suite only ever exercised `request.url`. + */ + it('builds body and headers from a numeric id without a TypeError', () => { + const numericParams = buildParams(tool, { ...overrides, [param]: 8035111022467891 }) + + expect(throwsTypeError(() => tool.request.url(numericParams))).toBe(false) + expect(throwsTypeError(() => tool.request.body?.(numericParams))).toBe(false) + expect(throwsTypeError(() => tool.request.headers?.(numericParams))).toBe(false) + }) + it('trims surrounding whitespace off a legitimate value', () => { const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) diff --git a/apps/sim/tools/discord/send_message.ts b/apps/sim/tools/discord/send_message.ts index eb522406b5e..e9058892ca5 100644 --- a/apps/sim/tools/discord/send_message.ts +++ b/apps/sim/tools/discord/send_message.ts @@ -1,5 +1,6 @@ import type { DiscordSendMessageParams, DiscordSendMessageResponse } from '@/tools/discord/types' import type { InternalToolConfig } from '@/tools/types' +import { safeUrlPathSegment } from '@/tools/url-path' export const discordSendMessageTool: InternalToolConfig< DiscordSendMessageParams, @@ -47,7 +48,7 @@ export const discordSendMessageTool: InternalToolConfig< input: (params: DiscordSendMessageParams) => { return { botToken: params.botToken.trim(), - channelId: params.channelId.trim(), + channelId: safeUrlPathSegment(params.channelId, 'channelId'), content: params.content || 'Message sent from Sim', files: params.files || null, } From dc7f6478972b2c09ef04f491354a62586cc6ed5c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 22:13:07 -0700 Subject: [PATCH 6/9] fix(discord,cloudflare): guard the send-message sink, pin the query in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings, each verified before acting. **Guard the real sink.** `lib/internal/discord/client.ts` interpolated `channelId` raw into the messages URL. Not exploitable today — `executeDiscordSendMessage` runs `validateNumericId` ahead of both `sendDiscordMessage` call paths — but `sendDiscordMessage` is exported, so the guard lived only in callers rather than at the point of interpolation. It now applies at the sink, which is where the Application Operation Boundary rule puts it. No behaviour change for a valid channel id: snowflakes are all digits, and digits are unreserved, so the encode is the identity function. **Report the bucket that was actually deleted.** `delete_r2_bucket` echoes the requested name because Cloudflare returns an empty body, but it echoed the RAW param while the request addressed the trimmed one — so a padded input deleted `my-bucket` and reported `" my-bucket "`. The output now matches what the path addressed. This is the one place the trim was observable, and it was inconsistent rather than merely cosmetic. **Pin the query string and the probe slot in MUST_NEUTRALIZE.** The test asserted origin, prefix, hash, segment count and every NON-probe segment — and skipped the probe slot. Both gaps mattered, and the second is the more general one: - A raw interpolation of `id?x=y` kept the pathname segment count and every surrounding segment; only `search` showed the id had been torn in half (`?with_counts=false?with_counts=true`). Now asserted equal to the query the tool builds on its own, which is not simply `''` — several Discord tools carry a legitimate query. - Skipping the probe slot would let a balanced traversal such as `id/../../other/victim` pass with the guard removed, since only that slot differs. Every segment is now pinned to the trimmed, percent-encoded value. Verified red-first: reverting `get_server` to raw interpolation now fails 14 assertions including both MUST_NEUTRALIZE cases, which previously passed. --- apps/sim/lib/internal/discord/client.ts | 22 ++- apps/sim/tools/cloudflare/delete_r2_bucket.ts | 11 +- apps/sim/tools/cloudflare/path_safety.test.ts | 171 ++++++++++-------- apps/sim/tools/discord/path_safety.test.ts | 170 +++++++++-------- 4 files changed, 217 insertions(+), 157 deletions(-) diff --git a/apps/sim/lib/internal/discord/client.ts b/apps/sim/lib/internal/discord/client.ts index df86a4a1e15..62475d835ff 100644 --- a/apps/sim/lib/internal/discord/client.ts +++ b/apps/sim/lib/internal/discord/client.ts @@ -2,6 +2,7 @@ import { isRecordLike } from '@sim/utils/object' import { MAX_JSON_API_RESPONSE_BYTES } from '@/lib/core/security/input-validation.server' import { readResponseJsonWithLimit } from '@/lib/core/utils/stream-limits' import { DiscordOperationError } from '@/lib/internal/discord/errors' +import { safeUrlPathSegment } from '@/tools/url-path' export async function sendDiscordMessage( botToken: string, @@ -11,15 +12,18 @@ export async function sendDiscordMessage( signal?: AbortSignal ): Promise> { signal?.throwIfAborted() - const response = await fetch(`https://discord.com/api/v10/channels/${channelId}/messages`, { - method: 'POST', - headers: { - Authorization: `Bot ${botToken}`, - ...(contentType === 'json' ? { 'Content-Type': 'application/json' } : {}), - }, - body, - signal, - }) + const response = await fetch( + `https://discord.com/api/v10/channels/${safeUrlPathSegment(channelId, 'channelId')}/messages`, + { + method: 'POST', + headers: { + Authorization: `Bot ${botToken}`, + ...(contentType === 'json' ? { 'Content-Type': 'application/json' } : {}), + }, + body, + signal, + } + ) let data: unknown try { data = await readResponseJsonWithLimit(response, { diff --git a/apps/sim/tools/cloudflare/delete_r2_bucket.ts b/apps/sim/tools/cloudflare/delete_r2_bucket.ts index 86c440df5d1..b4e2b9bab17 100644 --- a/apps/sim/tools/cloudflare/delete_r2_bucket.ts +++ b/apps/sim/tools/cloudflare/delete_r2_bucket.ts @@ -65,7 +65,16 @@ export const deleteR2BucketTool: ToolConfig< } } - return { success: true, output: { name: params?.bucketName ?? '' } } + /** + * Echo the name the request actually addressed. `safeUrlPathSegment` trims + * before building the path, so reporting the raw param would name a bucket + * that was not the one deleted whenever the input carried whitespace. + */ + const deleted = params?.bucketName + return { + success: true, + output: { name: typeof deleted === 'string' ? deleted.trim() : (deleted ?? '') }, + } }, outputs: { diff --git a/apps/sim/tools/cloudflare/path_safety.test.ts b/apps/sim/tools/cloudflare/path_safety.test.ts index 9c5813c46cf..36860dc94cc 100644 --- a/apps/sim/tools/cloudflare/path_safety.test.ts +++ b/apps/sim/tools/cloudflare/path_safety.test.ts @@ -245,6 +245,8 @@ interface PathParamCase { param: string overrides: Record baseline: string[] + /** The query string the tool builds on its own, with no vector involved. */ + baselineSearch: string } /** @@ -262,8 +264,11 @@ for (const tool of TOOLS) { if (param === CREDENTIAL_PARAM || param in context.overrides) continue let baseline: string[] + let baselineSearch: string try { - baseline = segmentsOf(buildUrl(tool, { ...context.overrides, [param]: PROBE }).pathname) + const probed = buildUrl(tool, { ...context.overrides, [param]: PROBE }) + baseline = segmentsOf(probed.pathname) + baselineSearch = probed.search } catch (error) { if (context.label === 'all params') { UNBUILDABLE.push(`${tool.id} / ${param}: ${(error as Error).message}`) @@ -283,6 +288,7 @@ for (const tool of TOOLS) { param, overrides: context.overrides, baseline, + baselineSearch, }) } } @@ -310,88 +316,105 @@ describe('cloudflare path-param traversal safety', () => { expect([...counts.values()].filter((count) => count > 1).length).toBeGreaterThanOrEqual(15) }) - describe.each(PATH_PARAM_PAIRS)('$name', ({ tool, param, overrides, baseline }) => { - const withValue = (value: unknown) => buildUrl(tool, { ...overrides, [param]: value }) + describe.each(PATH_PARAM_PAIRS)( + '$name', + ({ tool, param, overrides, baseline, baselineSearch }) => { + const withValue = (value: unknown) => buildUrl(tool, { ...overrides, [param]: value }) - it.each(MUST_REJECT)('rejects %j outright', (value) => { - expect(() => withValue(value)).toThrow(new RegExp(`${param}|path traversal|path separator`)) - }) + it.each(MUST_REJECT)('rejects %j outright', (value) => { + expect(() => withValue(value)).toThrow(new RegExp(`${param}|path traversal|path separator`)) + }) + + it.each(MUST_NEUTRALIZE)('confines %j to a single segment', (value) => { + const url = withValue(value) + + expect(url.origin).toBe(API_ORIGIN) + expect(url.pathname.startsWith(API_PREFIX)).toBe(true) + expect(url.hash).toBe('') + + /** + * The query string must be byte-identical to what the tool builds alone. + * Without this, a raw interpolation of `id?x=y` passes every other + * assertion here: the `?` starts a query, so the PATH keeps its segment + * count and every surrounding segment, and only `search` reveals that the + * id was torn in half. Shape alone cannot see it. + */ + expect(url.search).toBe(baselineSearch) + + const actual = segmentsOf(url.pathname) + expect(actual).toHaveLength(baseline.length) + /** + * Every segment is pinned, including the one under test — it must equal + * the trimmed, percent-encoded value. Skipping the probe slot would let a + * balanced traversal (`id/../../other/victim`) pass with the guard gone, + * because only that slot changes. + */ + const expected = encodeURIComponent(value.trim()) + baseline.forEach((segment, index) => { + expect(actual[index]).toBe(segment.replaceAll(PROBE, expected)) + }) + }) - it.each(MUST_NEUTRALIZE)('confines %j to a single segment', (value) => { - const url = withValue(value) + it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { + const actual = segmentsOf(withValue(value).pathname) - expect(url.origin).toBe(API_ORIGIN) - expect(url.pathname.startsWith(API_PREFIX)).toBe(true) - expect(url.searchParams.get('account_id')).toBeNull() - expect(url.hash).toBe('') + expect(actual).toHaveLength(baseline.length) + baseline.forEach((segment, index) => { + expect(actual[index]).toBe(segment.replaceAll(PROBE, value)) + }) + }) - const actual = segmentsOf(url.pathname) - expect(actual).toHaveLength(baseline.length) - baseline.forEach((segment, index) => { - if (segment.includes(PROBE)) return - expect(actual[index]).toBe(segment) + /** + * A safe-range numeric id must build the same path as its decimal string. + * + * This is what catches a pre-trim anywhere in a URL builder. A + * `params.x?.trim()` ahead of the guard throws a bare + * `TypeError: params.x?.trim is not a function` on a JSON number, and it + * throws BEFORE `safeUrlPathSegment` — which accepts numbers and bigints — + * ever runs. The first version of this suite passed only strings, so the + * `remove_reaction` and `create_thread` pre-trims survived it; both bots + * caught what the harness could not. + */ + it('accepts a safe-range numeric id identically to its decimal string', () => { + const numeric = 8035111022467891 + + expect(segmentsOf(withValue(numeric).pathname)).toEqual( + segmentsOf(withValue(String(numeric)).pathname) + ) }) - }) - it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { - const actual = segmentsOf(withValue(value).pathname) + it('accepts a bigint id identically to its decimal string', () => { + const snowflake = 1234567890123456789n - expect(actual).toHaveLength(baseline.length) - baseline.forEach((segment, index) => { - expect(actual[index]).toBe(segment.replaceAll(PROBE, value)) + expect(segmentsOf(withValue(snowflake).pathname)).toEqual( + segmentsOf(withValue(snowflake.toString()).pathname) + ) }) - }) - - /** - * A safe-range numeric id must build the same path as its decimal string. - * - * This is what catches a pre-trim anywhere in a URL builder. A - * `params.x?.trim()` ahead of the guard throws a bare - * `TypeError: params.x?.trim is not a function` on a JSON number, and it - * throws BEFORE `safeUrlPathSegment` — which accepts numbers and bigints — - * ever runs. The first version of this suite passed only strings, so the - * `remove_reaction` and `create_thread` pre-trims survived it; both bots - * caught what the harness could not. - */ - it('accepts a safe-range numeric id identically to its decimal string', () => { - const numeric = 8035111022467891 - - expect(segmentsOf(withValue(numeric).pathname)).toEqual( - segmentsOf(withValue(String(numeric)).pathname) - ) - }) - - it('accepts a bigint id identically to its decimal string', () => { - const snowflake = 1234567890123456789n - - expect(segmentsOf(withValue(snowflake).pathname)).toEqual( - segmentsOf(withValue(snowflake.toString()).pathname) - ) - }) - - /** - * The URL is not the only builder that touches an id. `create_thread` also - * reads `messageId` in its `body` to decide the thread type, and a - * `?.trim()` there threw a bare TypeError on a numeric id *after* the URL - * had already accepted it — caught by review, not by this suite, because - * the suite only ever exercised `request.url`. - */ - it('builds body and headers from a numeric id without a TypeError', () => { - const numericParams = buildParams(tool, { ...overrides, [param]: 8035111022467891 }) - - expect(throwsTypeError(() => tool.request.url(numericParams))).toBe(false) - expect(throwsTypeError(() => tool.request.body?.(numericParams))).toBe(false) - expect(throwsTypeError(() => tool.request.headers?.(numericParams))).toBe(false) - }) - - it('trims surrounding whitespace off a legitimate value', () => { - const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) - - baseline.forEach((segment, index) => { - expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) + + /** + * The URL is not the only builder that touches an id. `create_thread` also + * reads `messageId` in its `body` to decide the thread type, and a + * `?.trim()` there threw a bare TypeError on a numeric id *after* the URL + * had already accepted it — caught by review, not by this suite, because + * the suite only ever exercised `request.url`. + */ + it('builds body and headers from a numeric id without a TypeError', () => { + const numericParams = buildParams(tool, { ...overrides, [param]: 8035111022467891 }) + + expect(throwsTypeError(() => tool.request.url(numericParams))).toBe(false) + expect(throwsTypeError(() => tool.request.body?.(numericParams))).toBe(false) + expect(throwsTypeError(() => tool.request.headers?.(numericParams))).toBe(false) }) - }) - }) + + it('trims surrounding whitespace off a legitimate value', () => { + const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) + + baseline.forEach((segment, index) => { + expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) + }) + }) + } + ) }) /** diff --git a/apps/sim/tools/discord/path_safety.test.ts b/apps/sim/tools/discord/path_safety.test.ts index ae0063a17a6..2e03201286c 100644 --- a/apps/sim/tools/discord/path_safety.test.ts +++ b/apps/sim/tools/discord/path_safety.test.ts @@ -243,6 +243,8 @@ interface PathParamCase { param: string overrides: Record baseline: string[] + /** The query string the tool builds on its own, with no vector involved. */ + baselineSearch: string } /** @@ -260,8 +262,11 @@ for (const tool of TOOLS) { if (param === CREDENTIAL_PARAM || param in context.overrides) continue let baseline: string[] + let baselineSearch: string try { - baseline = segmentsOf(buildUrl(tool, { ...context.overrides, [param]: PROBE }).pathname) + const probed = buildUrl(tool, { ...context.overrides, [param]: PROBE }) + baseline = segmentsOf(probed.pathname) + baselineSearch = probed.search } catch (error) { if (context.label === 'all params') { UNBUILDABLE.push(`${tool.id} / ${param}: ${(error as Error).message}`) @@ -281,6 +286,7 @@ for (const tool of TOOLS) { param, overrides: context.overrides, baseline, + baselineSearch, }) } } @@ -319,87 +325,105 @@ describe('discord path-param traversal safety', () => { expect([...counts.values()].filter((count) => count > 1).length).toBeGreaterThanOrEqual(15) }) - describe.each(PATH_PARAM_PAIRS)('$name', ({ tool, param, overrides, baseline }) => { - const withValue = (value: unknown) => buildUrl(tool, { ...overrides, [param]: value }) + describe.each(PATH_PARAM_PAIRS)( + '$name', + ({ tool, param, overrides, baseline, baselineSearch }) => { + const withValue = (value: unknown) => buildUrl(tool, { ...overrides, [param]: value }) - it.each(MUST_REJECT)('rejects %j outright', (value) => { - expect(() => withValue(value)).toThrow(new RegExp(`${param}|path traversal|path separator`)) - }) + it.each(MUST_REJECT)('rejects %j outright', (value) => { + expect(() => withValue(value)).toThrow(new RegExp(`${param}|path traversal|path separator`)) + }) + + it.each(MUST_NEUTRALIZE)('confines %j to a single segment', (value) => { + const url = withValue(value) + + expect(url.origin).toBe(API_ORIGIN) + expect(url.pathname.startsWith(API_PREFIX)).toBe(true) + expect(url.hash).toBe('') + + /** + * The query string must be byte-identical to what the tool builds alone. + * Without this, a raw interpolation of `id?x=y` passes every other + * assertion here: the `?` starts a query, so the PATH keeps its segment + * count and every surrounding segment, and only `search` reveals that the + * id was torn in half. Shape alone cannot see it. + */ + expect(url.search).toBe(baselineSearch) + + const actual = segmentsOf(url.pathname) + expect(actual).toHaveLength(baseline.length) + /** + * Every segment is pinned, including the one under test — it must equal + * the trimmed, percent-encoded value. Skipping the probe slot would let a + * balanced traversal (`id/../../other/victim`) pass with the guard gone, + * because only that slot changes. + */ + const expected = encodeURIComponent(value.trim()) + baseline.forEach((segment, index) => { + expect(actual[index]).toBe(segment.replaceAll(PROBE, expected)) + }) + }) - it.each(MUST_NEUTRALIZE)('confines %j to a single segment', (value) => { - const url = withValue(value) + it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { + const actual = segmentsOf(withValue(value).pathname) - expect(url.origin).toBe(API_ORIGIN) - expect(url.pathname.startsWith(API_PREFIX)).toBe(true) - expect(url.hash).toBe('') + expect(actual).toHaveLength(baseline.length) + baseline.forEach((segment, index) => { + expect(actual[index]).toBe(segment.replaceAll(PROBE, value)) + }) + }) - const actual = segmentsOf(url.pathname) - expect(actual).toHaveLength(baseline.length) - baseline.forEach((segment, index) => { - if (segment.includes(PROBE)) return - expect(actual[index]).toBe(segment) + /** + * A safe-range numeric id must build the same path as its decimal string. + * + * This is what catches a pre-trim anywhere in a URL builder. A + * `params.x?.trim()` ahead of the guard throws a bare + * `TypeError: params.x?.trim is not a function` on a JSON number, and it + * throws BEFORE `safeUrlPathSegment` — which accepts numbers and bigints — + * ever runs. The first version of this suite passed only strings, so the + * `remove_reaction` and `create_thread` pre-trims survived it; both bots + * caught what the harness could not. + */ + it('accepts a safe-range numeric id identically to its decimal string', () => { + const numeric = 8035111022467891 + + expect(segmentsOf(withValue(numeric).pathname)).toEqual( + segmentsOf(withValue(String(numeric)).pathname) + ) }) - }) - it.each(LEGITIMATE_IDS)('passes %j through unchanged', (value) => { - const actual = segmentsOf(withValue(value).pathname) + it('accepts a bigint id identically to its decimal string', () => { + const snowflake = 1234567890123456789n - expect(actual).toHaveLength(baseline.length) - baseline.forEach((segment, index) => { - expect(actual[index]).toBe(segment.replaceAll(PROBE, value)) + expect(segmentsOf(withValue(snowflake).pathname)).toEqual( + segmentsOf(withValue(snowflake.toString()).pathname) + ) }) - }) - - /** - * A safe-range numeric id must build the same path as its decimal string. - * - * This is what catches a pre-trim anywhere in a URL builder. A - * `params.x?.trim()` ahead of the guard throws a bare - * `TypeError: params.x?.trim is not a function` on a JSON number, and it - * throws BEFORE `safeUrlPathSegment` — which accepts numbers and bigints — - * ever runs. The first version of this suite passed only strings, so the - * `remove_reaction` and `create_thread` pre-trims survived it; both bots - * caught what the harness could not. - */ - it('accepts a safe-range numeric id identically to its decimal string', () => { - const numeric = 8035111022467891 - - expect(segmentsOf(withValue(numeric).pathname)).toEqual( - segmentsOf(withValue(String(numeric)).pathname) - ) - }) - - it('accepts a bigint id identically to its decimal string', () => { - const snowflake = 1234567890123456789n - - expect(segmentsOf(withValue(snowflake).pathname)).toEqual( - segmentsOf(withValue(snowflake.toString()).pathname) - ) - }) - - /** - * The URL is not the only builder that touches an id. `create_thread` also - * reads `messageId` in its `body` to decide the thread type, and a - * `?.trim()` there threw a bare TypeError on a numeric id *after* the URL - * had already accepted it — caught by review, not by this suite, because - * the suite only ever exercised `request.url`. - */ - it('builds body and headers from a numeric id without a TypeError', () => { - const numericParams = buildParams(tool, { ...overrides, [param]: 8035111022467891 }) - - expect(throwsTypeError(() => tool.request.url(numericParams))).toBe(false) - expect(throwsTypeError(() => tool.request.body?.(numericParams))).toBe(false) - expect(throwsTypeError(() => tool.request.headers?.(numericParams))).toBe(false) - }) - - it('trims surrounding whitespace off a legitimate value', () => { - const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) - - baseline.forEach((segment, index) => { - expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) + + /** + * The URL is not the only builder that touches an id. `create_thread` also + * reads `messageId` in its `body` to decide the thread type, and a + * `?.trim()` there threw a bare TypeError on a numeric id *after* the URL + * had already accepted it — caught by review, not by this suite, because + * the suite only ever exercised `request.url`. + */ + it('builds body and headers from a numeric id without a TypeError', () => { + const numericParams = buildParams(tool, { ...overrides, [param]: 8035111022467891 }) + + expect(throwsTypeError(() => tool.request.url(numericParams))).toBe(false) + expect(throwsTypeError(() => tool.request.body?.(numericParams))).toBe(false) + expect(throwsTypeError(() => tool.request.headers?.(numericParams))).toBe(false) }) - }) - }) + + it('trims surrounding whitespace off a legitimate value', () => { + const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) + + baseline.forEach((segment, index) => { + expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) + }) + }) + } + ) }) /** From 39a7df952cef7460d7b189abc710873cd7b787c8 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 22:16:33 -0700 Subject: [PATCH 7/9] fix(discord): preserve the documented @me alias in user-ID slots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discord publishes `@me` as a literal route segment standing in for the current bot: `GET /users/@me` (resources/user), `DELETE .../reactions/{emoji}/@me` (Delete Own Reaction), and `PATCH /guilds/{guild.id}/members/@me`. Before this PR those slots were interpolated raw, so a user who typed `@me` into a user-ID field got a working request. `encodeURIComponent('@me')` is `%40me`, which Discord does not route, so the guards silently turned those calls into 404s. That is a real backwards- compatibility regression on documented routes, and it is the only such regression this PR introduces — found by auditing the tools against Discord's published reference rather than by a failing test, since a 404 is invisible to a unit suite. `discordUserPathSegment` passes `@me` through verbatim and delegates everything else to `safeUrlPathSegment` unchanged. This widens the accepted set by exactly one constant and weakens nothing: `@me` is neither a dot segment nor does it contain `/` or `\`, so it cannot pop or add a path segment. Applied only to the three slots where Discord documents the alias — `get_user`, `remove_reaction`, and `update_member` — not to every user ID. Tests pin the alias (including whitespace-padded), pin that a lookalike such as `@everyone` is still encoded to `%40everyone`, and pin that `..` and `@me/../../guilds/1` are still rejected in the same slot. --- apps/sim/tools/discord/get_user.ts | 4 +- apps/sim/tools/discord/path_safety.test.ts | 60 ++++++++++++++++++++++ apps/sim/tools/discord/remove_reaction.ts | 4 +- apps/sim/tools/discord/update_member.ts | 3 +- apps/sim/tools/discord/utils.ts | 26 ++++++++++ 5 files changed, 92 insertions(+), 5 deletions(-) diff --git a/apps/sim/tools/discord/get_user.ts b/apps/sim/tools/discord/get_user.ts index f3a7ef4a2a1..ca94788c7b9 100644 --- a/apps/sim/tools/discord/get_user.ts +++ b/apps/sim/tools/discord/get_user.ts @@ -3,8 +3,8 @@ import type { DiscordGetUserResponse, DiscordUser, } from '@/tools/discord/types' +import { discordUserPathSegment } from '@/tools/discord/utils' import type { ToolConfig } from '@/tools/types' -import { safeUrlPathSegment } from '@/tools/url-path' export const discordGetUserTool: ToolConfig = { id: 'discord_get_user', @@ -29,7 +29,7 @@ export const discordGetUserTool: ToolConfig - `https://discord.com/api/v10/users/${safeUrlPathSegment(params.userId, 'userId')}`, + `https://discord.com/api/v10/users/${discordUserPathSegment(params.userId, 'userId')}`, method: 'GET', headers: (params: DiscordGetUserParams) => { const headers: Record = { diff --git a/apps/sim/tools/discord/path_safety.test.ts b/apps/sim/tools/discord/path_safety.test.ts index 2e03201286c..fd47b9d8074 100644 --- a/apps/sim/tools/discord/path_safety.test.ts +++ b/apps/sim/tools/discord/path_safety.test.ts @@ -37,6 +37,8 @@ import * as discordTools from '@/tools/discord' import { discordAssignRoleTool } from '@/tools/discord/assign_role' import { discordDeleteMessageTool } from '@/tools/discord/delete_message' import { discordGetMemberTool } from '@/tools/discord/get_member' +import { discordGetUserTool } from '@/tools/discord/get_user' +import { discordUpdateMemberTool } from '@/tools/discord/update_member' const API_ORIGIN = 'https://discord.com' const API_PREFIX = '/api/v10/' @@ -456,6 +458,64 @@ describe('a trailing dot segment is invisible to a shape check', () => { }) }) +/** + * `@me` is a literal route segment Discord publishes for the current bot — + * `GET /users/@me`, `DELETE .../reactions/{emoji}/@me`, + * `PATCH /guilds/{guild.id}/members/@me`. It was interpolated raw before these + * guards, so typing it into a user-ID field worked; `encodeURIComponent('@me')` + * is `%40me`, which Discord does not route. These pin the alias so the + * compatibility fix is not silently undone, and pin that it stays traversal-inert. + */ +describe('the documented @me alias survives the guard', () => { + const cases: ReadonlyArray<[string, PathTool, Record, string]> = [ + [ + 'discord_get_user', + pathToolFor(discordGetUserTool, 'discord_get_user'), + {}, + '/api/v10/users/@me', + ], + [ + 'discord_update_member', + pathToolFor(discordUpdateMemberTool, 'discord_update_member'), + { serverId: '123456789012345678' }, + '/api/v10/guilds/123456789012345678/members/@me', + ], + ] + + it.each(cases)('%s routes @me verbatim', (_name, tool, extra, expected) => { + expect(new URL(tool.request.url({ botToken: 'b', ...extra, userId: '@me' })).pathname).toBe( + expected + ) + }) + + it('accepts @me with surrounding whitespace', () => { + const tool = pathToolFor(discordGetUserTool, 'discord_get_user') + + expect(new URL(tool.request.url({ botToken: 'b', userId: ' @me ' })).pathname).toBe( + '/api/v10/users/@me' + ) + }) + + it('still rejects a traversal in the same slot', () => { + const tool = pathToolFor(discordGetUserTool, 'discord_get_user') + + expect(() => tool.request.url({ botToken: 'b', userId: '..' })).toThrow( + /path traversal is not allowed/ + ) + expect(() => tool.request.url({ botToken: 'b', userId: '@me/../../guilds/1' })).toThrow( + /path separator/ + ) + }) + + it('does not widen the alias to lookalikes', () => { + const tool = pathToolFor(discordGetUserTool, 'discord_get_user') + + expect(new URL(tool.request.url({ botToken: 'b', userId: '@everyone' })).pathname).toBe( + '/api/v10/users/%40everyone' + ) + }) +}) + /** * An LLM tool call carries JSON, so a snowflake can arrive as a `number` rather * than the declared `string`. The guard must not turn that into a bogus segment. diff --git a/apps/sim/tools/discord/remove_reaction.ts b/apps/sim/tools/discord/remove_reaction.ts index c751d90f964..b8579c4ec55 100644 --- a/apps/sim/tools/discord/remove_reaction.ts +++ b/apps/sim/tools/discord/remove_reaction.ts @@ -2,7 +2,7 @@ import type { DiscordRemoveReactionParams, DiscordRemoveReactionResponse, } from '@/tools/discord/types' -import { isProvidedParam } from '@/tools/discord/utils' +import { discordUserPathSegment, isProvidedParam } from '@/tools/discord/utils' import type { ToolConfig } from '@/tools/types' import { safeUrlPathSegment } from '@/tools/url-path' @@ -59,7 +59,7 @@ export const discordRemoveReactionTool: ToolConfig< url: (params: DiscordRemoveReactionParams) => { const encodedEmoji = safeUrlPathSegment(params.emoji, 'emoji') const userPart = isProvidedParam(params.userId) - ? `/${safeUrlPathSegment(params.userId, 'userId')}` + ? `/${discordUserPathSegment(params.userId, 'userId')}` : '/@me' return `https://discord.com/api/v10/channels/${safeUrlPathSegment(params.channelId, 'channelId')}/messages/${safeUrlPathSegment(params.messageId, 'messageId')}/reactions/${encodedEmoji}${userPart}` }, diff --git a/apps/sim/tools/discord/update_member.ts b/apps/sim/tools/discord/update_member.ts index f61b3ad3dd7..1412eb929a8 100644 --- a/apps/sim/tools/discord/update_member.ts +++ b/apps/sim/tools/discord/update_member.ts @@ -1,4 +1,5 @@ import type { DiscordUpdateMemberParams, DiscordUpdateMemberResponse } from '@/tools/discord/types' +import { discordUserPathSegment } from '@/tools/discord/utils' import type { ToolConfig } from '@/tools/types' import { safeUrlPathSegment } from '@/tools/url-path' @@ -52,7 +53,7 @@ export const discordUpdateMemberTool: ToolConfig< request: { url: (params: DiscordUpdateMemberParams) => { - return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/members/${safeUrlPathSegment(params.userId, 'userId')}` + return `https://discord.com/api/v10/guilds/${safeUrlPathSegment(params.serverId, 'serverId')}/members/${discordUserPathSegment(params.userId, 'userId')}` }, method: 'PATCH', headers: (params) => ({ diff --git a/apps/sim/tools/discord/utils.ts b/apps/sim/tools/discord/utils.ts index 255de7ca8c8..da706d63f17 100644 --- a/apps/sim/tools/discord/utils.ts +++ b/apps/sim/tools/discord/utils.ts @@ -1,3 +1,5 @@ +import { safeUrlPathSegment } from '@/tools/url-path' + /** * Whether an optional Discord tool param was supplied. * @@ -22,3 +24,27 @@ export function isProvidedParam(value: T): value is NonNullable { if (typeof value === 'string') return value.trim() !== '' return true } + +/** + * Builds a path segment for a Discord `{user.id}` slot, preserving the + * documented `@me` alias. + * + * Discord publishes `@me` as a literal route segment standing in for the + * current bot — `GET /users/@me`, `DELETE .../reactions/{emoji}/@me`, and + * `PATCH /guilds/{guild.id}/members/@me`. Before these guards existed the value + * was interpolated raw, so a user who typed `@me` into a user-ID field got a + * working request. `encodeURIComponent('@me')` is `%40me`, which Discord does + * not route, so encoding it silently turned those calls into 404s. + * + * `@me` is passed through verbatim because it is traversal-inert: it is + * neither a dot segment nor does it contain `/` or `\`, so it cannot pop or + * add a path segment. Everything else goes through `safeUrlPathSegment` + * unchanged, so this widens the accepted set by exactly one constant and + * weakens nothing. + */ +export function discordUserPathSegment(value: unknown, paramName: string): string { + if (typeof value === 'string' && value.trim() === '@me') { + return '@me' + } + return safeUrlPathSegment(value as string | number | bigint, paramName) +} From c5dd2450e443ef480c12ee5e05b9c7f3f53444d6 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 22:25:37 -0700 Subject: [PATCH 8/9] fix(cloudflare): always report the deleted bucket name as a string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete_r2_bucket` echoes the requested name because Cloudflare returns an empty body for this endpoint. The echo passed a non-string param straight through, so a bucket named `12345` supplied as JSON `12345` produced a NUMBER in `output.name`, contradicting the `type: 'string'` the tool declares. Reachable rather than theoretical: R2's documented rule is `^[a-z0-9][a-z0-9-]*[a-z0-9]`, so a digits-only bucket name is valid, and `safeUrlPathSegment` accepts a number — which is what made the path build succeed and pushed the inconsistency into the output instead of the request. Now `String(value).trim()`, matching the segment the request addressed for every accepted kind. Covered by `r2_output.test.ts`: padded and plain strings, a numeric name (asserting the returned type is `string`), and a missing name. --- apps/sim/tools/cloudflare/delete_r2_bucket.ts | 13 ++++++++---- apps/sim/tools/cloudflare/r2_output.test.ts | 20 +++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) create mode 100644 apps/sim/tools/cloudflare/r2_output.test.ts diff --git a/apps/sim/tools/cloudflare/delete_r2_bucket.ts b/apps/sim/tools/cloudflare/delete_r2_bucket.ts index b4e2b9bab17..e6b02216695 100644 --- a/apps/sim/tools/cloudflare/delete_r2_bucket.ts +++ b/apps/sim/tools/cloudflare/delete_r2_bucket.ts @@ -66,14 +66,19 @@ export const deleteR2BucketTool: ToolConfig< } /** - * Echo the name the request actually addressed. `safeUrlPathSegment` trims - * before building the path, so reporting the raw param would name a bucket - * that was not the one deleted whenever the input carried whitespace. + * Echo the name the request actually addressed, as a string. + * + * `safeUrlPathSegment` trims before building the path, so reporting the raw + * param would name a bucket that was not the one deleted whenever the input + * carried whitespace. It also accepts a number, and a digits-only bucket + * name is valid under R2's `^[a-z0-9][a-z0-9-]*[a-z0-9]` rule, so an id + * supplied as JSON `12345` must still leave here as the string `'12345'` + * that `outputs.name` declares. */ const deleted = params?.bucketName return { success: true, - output: { name: typeof deleted === 'string' ? deleted.trim() : (deleted ?? '') }, + output: { name: deleted === null || deleted === undefined ? '' : String(deleted).trim() }, } }, diff --git a/apps/sim/tools/cloudflare/r2_output.test.ts b/apps/sim/tools/cloudflare/r2_output.test.ts new file mode 100644 index 00000000000..bc2936dbf32 --- /dev/null +++ b/apps/sim/tools/cloudflare/r2_output.test.ts @@ -0,0 +1,20 @@ +import { describe, expect, it } from 'vitest' +import { deleteR2BucketTool as t } from '@/tools/cloudflare/delete_r2_bucket' + +const ok = { json: async () => ({ success: true }) } as unknown as Response +describe('delete_r2_bucket output is always a trimmed string', () => { + it.each([ + [' my-bucket ', 'my-bucket'], + ['my-bucket', 'my-bucket'], + ])('string %s', async (i, e) => { + expect((await t.transformResponse!(ok, { bucketName: i } as any)).output.name).toBe(e) + }) + it('numeric bucket name becomes a string', async () => { + const r = await t.transformResponse!(ok, { bucketName: 12345 } as any) + expect(r.output.name).toBe('12345') + expect(typeof r.output.name).toBe('string') + }) + it('missing bucket name becomes an empty string', async () => { + expect((await t.transformResponse!(ok, {} as any)).output.name).toBe('') + }) +}) From c6968f01870810672cfd4b825aaa2bb6534675ee Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Fri, 28 Aug 2026 22:34:44 -0700 Subject: [PATCH 9/9] fix(cloudflare): refuse a padded bucket name on the irreversible delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reversing my earlier position on this, because the narrower framing is right. I had defended trimming here on consistency: it is `safeUrlPathSegment`'s contract at all 137 sites, and `accountId` on this very line already trimmed before this PR. That argument is weaker than it looked. Only FIVE params in this PR are newly trimmed — the ones that previously went through a bare `encodeURIComponent` — and of those, `delete_r2_bucket / bucketName` is the only one attached to an irreversible request. The rest are two GETs, a PUT, and an emoji. So this is not one of 137 uniform sites; it is the single intersection of "newly trimmed" and "cannot be undone". R2 names are `^[a-z0-9][a-z0-9-]*[a-z0-9]`, so `" prod-data "` names no bucket that can exist. Before this PR that request failed. Trimming turns it into one that destroys `prod-data`. That inference is fine for a read and not worth making on the caller's behalf for a delete, and a stray newline out of a file read or a workflow variable is exactly how a padded name arrives. Rejecting costs nothing legitimate: no valid bucket name has surrounding whitespace to lose. `get_r2_bucket`, a read of the same resource, still trims, and that divergence is asserted rather than assumed. The suite carries the exception explicitly — `REJECTS_SURROUNDING_WHITESPACE` makes the generic per-pair trim assertion demand a throw for this pair instead of silently skipping it. Verified non-vacuous: removing the guard fails 6 assertions across both files. --- apps/sim/tools/cloudflare/delete_r2_bucket.ts | 40 ++++++++++++++++--- apps/sim/tools/cloudflare/path_safety.test.ts | 19 ++++++++- apps/sim/tools/cloudflare/r2_output.test.ts | 40 +++++++++++++++++++ 3 files changed, 92 insertions(+), 7 deletions(-) diff --git a/apps/sim/tools/cloudflare/delete_r2_bucket.ts b/apps/sim/tools/cloudflare/delete_r2_bucket.ts index e6b02216695..0693ceff627 100644 --- a/apps/sim/tools/cloudflare/delete_r2_bucket.ts +++ b/apps/sim/tools/cloudflare/delete_r2_bucket.ts @@ -6,6 +6,35 @@ import { cloudflareErrorMessage, cloudflareHeaders } from '@/tools/cloudflare/ut import type { ToolConfig } from '@/tools/types' import { safeUrlPathSegment } from '@/tools/url-path' +/** + * Refuses a bucket name carrying surrounding whitespace, instead of letting + * `safeUrlPathSegment` trim it. + * + * Trimming is the right default almost everywhere, and this tool is the one + * place it is not. Deleting a bucket is irreversible, and this is the only + * parameter in this PR that is BOTH newly trimmed (it previously went through + * a bare `encodeURIComponent`, so a padded name reached Cloudflare verbatim and + * failed) AND attached to a destructive request. Every other newly-trimmed + * parameter is a GET, a PUT, or an emoji. + * + * R2 names contain only lowercase letters, digits and hyphens + * (`^[a-z0-9][a-z0-9-]*[a-z0-9]`), so `" prod-data "` names no bucket that + * can exist. Trimming it therefore turns a request that used to fail into one + * that destroys `prod-data` — a reasonable inference for a read, but not one + * worth making on the caller's behalf when it cannot be undone. A stray + * newline from a file read or a workflow variable is exactly how that arrives. + * + * Rejecting costs nothing legitimate, because no valid bucket name has + * surrounding whitespace to lose. + */ +function assertExactBucketName(bucketName: unknown): void { + if (typeof bucketName === 'string' && bucketName !== bucketName.trim()) { + throw new Error( + 'bucketName cannot have leading or trailing whitespace: R2 bucket names contain only lowercase letters, digits, and hyphens, so this would delete a different bucket than the one named' + ) + } +} + export const deleteR2BucketTool: ToolConfig< CloudflareDeleteR2BucketParams, CloudflareDeleteR2BucketResponse @@ -44,8 +73,10 @@ export const deleteR2BucketTool: ToolConfig< }, request: { - url: (params) => - `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/r2/buckets/${safeUrlPathSegment(params.bucketName, 'bucketName')}`, + url: (params) => { + assertExactBucketName(params.bucketName) + return `https://api.cloudflare.com/client/v4/accounts/${safeUrlPathSegment(params.accountId, 'accountId')}/r2/buckets/${safeUrlPathSegment(params.bucketName, 'bucketName')}` + }, method: 'DELETE', headers: (params) => { const headers = cloudflareHeaders(params.apiKey) @@ -68,9 +99,8 @@ export const deleteR2BucketTool: ToolConfig< /** * Echo the name the request actually addressed, as a string. * - * `safeUrlPathSegment` trims before building the path, so reporting the raw - * param would name a bucket that was not the one deleted whenever the input - * carried whitespace. It also accepts a number, and a digits-only bucket + * A padded name is refused above, so the echo cannot drift from the path. + * `safeUrlPathSegment` also accepts a number, and a digits-only bucket * name is valid under R2's `^[a-z0-9][a-z0-9-]*[a-z0-9]` rule, so an id * supplied as JSON `12345` must still leave here as the string `'12345'` * that `outputs.name` declares. diff --git a/apps/sim/tools/cloudflare/path_safety.test.ts b/apps/sim/tools/cloudflare/path_safety.test.ts index 36860dc94cc..d28b771dbde 100644 --- a/apps/sim/tools/cloudflare/path_safety.test.ts +++ b/apps/sim/tools/cloudflare/path_safety.test.ts @@ -92,6 +92,15 @@ const TOOLS_WITHOUT_PARAM_BUILT_URLS = [ 'cloudflare_get_zone_settings', ] as const +/** + * (tool, param) pairs that deliberately REJECT surrounding whitespace rather + * than trimming it. Only `delete_r2_bucket` qualifies: it is the sole parameter + * that is both newly trimmed by this PR and attached to an irreversible + * request, so inferring the caller's intent there is not worth the risk. + * Listing it here keeps the divergence explicit rather than implicit. + */ +const REJECTS_SURROUNDING_WHITESPACE = new Set(['cloudflare_delete_r2_bucket/bucketName']) + const SAFE_ID = 'SAFEID' const PROBE = 'PROBEVALUE' const TRIM_SAMPLE = '023e105f4ecef8ad9ca31a8372d0c353' @@ -406,9 +415,15 @@ describe('cloudflare path-param traversal safety', () => { expect(throwsTypeError(() => tool.request.headers?.(numericParams))).toBe(false) }) - it('trims surrounding whitespace off a legitimate value', () => { - const actual = segmentsOf(withValue(` ${TRIM_SAMPLE} `).pathname) + it("handles surrounding whitespace per this parameter's policy", () => { + const padded = ` ${TRIM_SAMPLE} ` + + if (REJECTS_SURROUNDING_WHITESPACE.has(`${tool.id}/${param}`)) { + expect(() => withValue(padded)).toThrow(/leading or trailing whitespace/) + return + } + const actual = segmentsOf(withValue(padded).pathname) baseline.forEach((segment, index) => { expect(actual[index]).toBe(segment.replaceAll(PROBE, TRIM_SAMPLE)) }) diff --git a/apps/sim/tools/cloudflare/r2_output.test.ts b/apps/sim/tools/cloudflare/r2_output.test.ts index bc2936dbf32..091c4405a8f 100644 --- a/apps/sim/tools/cloudflare/r2_output.test.ts +++ b/apps/sim/tools/cloudflare/r2_output.test.ts @@ -1,6 +1,13 @@ import { describe, expect, it } from 'vitest' import { deleteR2BucketTool as t } from '@/tools/cloudflare/delete_r2_bucket' +const buildUrl = (bucketName: unknown) => + (t.request.url as (p: Record) => string)({ + apiKey: 'k', + accountId: 'acc', + bucketName, + }) + const ok = { json: async () => ({ success: true }) } as unknown as Response describe('delete_r2_bucket output is always a trimmed string', () => { it.each([ @@ -18,3 +25,36 @@ describe('delete_r2_bucket output is always a trimmed string', () => { expect((await t.transformResponse!(ok, {} as any)).output.name).toBe('') }) }) + +/** + * Deleting a bucket is irreversible, and `bucketName` is one of only five + * params this PR newly trims, so a padded name is refused rather than + * canonicalized. No valid R2 name has surrounding whitespace to lose. + */ +describe('delete_r2_bucket refuses a padded bucket name', () => { + it.each([' my-bucket ', 'my-bucket ', ' my-bucket', '\tmy-bucket', 'my-bucket\n'])( + 'rejects %j', + (name) => { + expect(() => buildUrl(name)).toThrow(/leading or trailing whitespace/) + } + ) + + it.each(['my-bucket', 'bucket-123', 'abc', '12345'])('still accepts %j', (name) => { + expect(new URL(buildUrl(name)).pathname).toBe(`/client/v4/accounts/acc/r2/buckets/${name}`) + }) + + it('still rejects a traversal', () => { + expect(() => buildUrl('..')).toThrow(/path traversal is not allowed/) + }) + + it('get_r2_bucket, a read of the same resource, still trims', async () => { + const { getR2BucketTool } = await import('@/tools/cloudflare/get_r2_bucket') + const url = (getR2BucketTool.request.url as (p: Record) => string)({ + apiKey: 'k', + accountId: 'acc', + bucketName: ' my-bucket ', + }) + + expect(new URL(url).pathname).toBe('/client/v4/accounts/acc/r2/buckets/my-bucket') + }) +})