From 2d9d08e65de9265b567db1fc89371d5e4553b0cd Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Thu, 30 Jul 2026 15:39:35 -0400 Subject: [PATCH 1/3] fix(api): encode interpolated path segments in API client --- .gitignore | 1 + docs/specs/subpath-constraints.md | 5 +- .../mcp-core/src/api-client/api-path.test.ts | 159 ++++++++ packages/mcp-core/src/api-client/api-path.ts | 46 +++ packages/mcp-core/src/api-client/client.ts | 202 +++++----- packages/mcp-core/src/schema.ts | 10 +- .../src/tools/catalog/get-event-stacktrace.ts | 2 + .../src/tools/catalog/get-issue-details.ts | 4 +- .../catalog/get-issue-tag-values.test.ts | 46 +-- .../src/tools/catalog/get-profile-details.ts | 10 +- .../mcp-core/src/tools/catalog/get-profile.ts | 4 +- .../src/tools/catalog/path-traversal.test.ts | 352 ++++++++++++++++++ .../mcp-core/src/tools/catalog/update-dsn.ts | 2 + .../mcp-core/src/utils/slug-validation.ts | 48 +++ 14 files changed, 772 insertions(+), 119 deletions(-) create mode 100644 packages/mcp-core/src/api-client/api-path.test.ts create mode 100644 packages/mcp-core/src/api-client/api-path.ts create mode 100644 packages/mcp-core/src/tools/catalog/path-traversal.test.ts diff --git a/.gitignore b/.gitignore index 982d02178..a8fb8ea70 100644 --- a/.gitignore +++ b/.gitignore @@ -29,6 +29,7 @@ _.seed node_modules/ jspm_packages/ .npm +.pnpm-store/ .node_repl_history *.tgz .cache diff --git a/docs/specs/subpath-constraints.md b/docs/specs/subpath-constraints.md index 370cc8c7f..5a2808c51 100644 --- a/docs/specs/subpath-constraints.md +++ b/docs/specs/subpath-constraints.md @@ -23,8 +23,9 @@ Examples: ## What you'll experience -- Tools automatically use the constrained organization/project as defaults -- You can still pass explicit `organizationSlug`/`projectSlug` to override defaults per call +- Tools automatically use the constrained organization/project +- Constrained parameters are removed from the tool schemas entirely and injected server-side, so a scoped session cannot override them or reach another organization +- Resource identifiers you pass (event, attachment, replay, profile, and client key IDs) are format-validated and URL-encoded, so they cannot rewrite the scoped path either - If you don't provide a scope, tools work across your accessible organizations when supported - Some tools are filtered when not useful: `find_organizations` is hidden when scoped to an org, and `find_projects` is hidden when scoped to a project diff --git a/packages/mcp-core/src/api-client/api-path.test.ts b/packages/mcp-core/src/api-client/api-path.test.ts new file mode 100644 index 000000000..e8cb74a54 --- /dev/null +++ b/packages/mcp-core/src/api-client/api-path.test.ts @@ -0,0 +1,159 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; +import { apiPath } from "./api-path"; + +/** + * The assertions below resolve each path the way `fetch` does, because dot-segment + * normalization happens during URL parsing rather than in our code. A test that only + * inspected the assembled string would pass even if traversal still worked. + */ +function resolve(path: string): string { + const url = new URL(`https://sentry.io/api/0${path}`); + return url.pathname + url.search; +} + +describe("apiPath", () => { + it("leaves legitimate identifier formats byte for byte", () => { + const cases = [ + "my-org", + "my_project.name", + "1234567890", + "cfe78a5c892d4a64a962d837673398d2", + "7e07485f-12f9-416b-8b14-26260799b51f", + "PROJECT-1Z43", + "latest", + ]; + + for (const value of cases) { + expect(apiPath`/projects/${value}/`).toBe(`/projects/${value}/`); + } + }); + + it("accepts numeric values", () => { + expect(apiPath`/projects/${4511636893859840}/`).toBe( + "/projects/4511636893859840/", + ); + }); + + it("handles templates with no interpolation", () => { + expect(apiPath`/organizations/`).toBe("/organizations/"); + }); + + it("handles adjacent interpolations", () => { + expect(apiPath`/${"a"}${"b"}/`).toBe("/ab/"); + }); + + describe("traversal containment", () => { + it("keeps a traversal payload inside its own segment", () => { + const attack = "../../../victim-org/victim-proj/events/abc"; + const path = apiPath`/projects/my-org/my-proj/events/${attack}/attachments/`; + + expect(resolve(path)).toBe( + "/api/0/projects/my-org/my-proj/events/..%2F..%2F..%2Fvictim-org%2Fvictim-proj%2Fevents%2Fabc/attachments/", + ); + }); + + it("resolves the unencoded equivalent out of scope, proving the test is meaningful", () => { + const attack = "../../../victim-org/victim-proj/events/abc"; + const unsafe = `/projects/my-org/my-proj/events/${attack}/attachments/`; + + expect(resolve(unsafe)).toBe( + "/api/0/projects/victim-org/victim-proj/events/abc/attachments/", + ); + }); + + /** + * VULN-2450 specifically asked that a patch be retested against these variants, + * since encode-only fixes for this bug class have historically been bypassable. + * + * Each payload must survive URL resolution as exactly one inert path segment. + * Asserting the whole resolved path (rather than searching for the payload text, + * which is still present once encoded) is what proves no segment boundary, + * query separator or fragment escaped. + */ + it.each([ + ["plain dot segments", "../../other-org/other-proj"], + ["double encoded dots", "%252e%252e%252fother-org"], + ["single encoded dots", "%2e%2e%2fother-org"], + ["recursive sequences", "....//....//other-org"], + ["backslash separators", "..\\..\\other-org"], + ["mixed separators", "..\\../other-org"], + ["null byte", "abc\u0000/../other-org"], + ["fragment truncation", "abc/#"], + ["query injection", "abc/?download=1&x=2"], + ["scheme relative", "//evil.example.com/x"], + ["absolute path", "/organizations/other-org/members"], + ["trailing dot segment", "abc/.."], + ])("contains %s", (_label, attack) => { + const resolved = resolve( + apiPath`/projects/my-org/my-proj/events/${attack}/`, + ); + + expect(resolved).toBe( + `/api/0/projects/my-org/my-proj/events/${encodeURIComponent(attack)}/`, + ); + }); + + it("cannot redirect the request to another host", () => { + const attack = "//evil.example.com/collect"; + const url = new URL( + `https://sentry.io/api/0${apiPath`/projects/o/p/events/${attack}/`}`, + ); + + expect(url.host).toBe("sentry.io"); + }); + + it("cannot append query parameters to the request", () => { + const url = new URL( + `https://sentry.io/api/0${apiPath`/projects/o/p/events/${"abc/?download=1"}/`}`, + ); + + expect(url.search).toBe(""); + }); + }); +}); + +/** + * The slug-only fix in VULN-848 hardened the parameters that were vulnerable at the + * time and left the resource IDs raw, which is how VULN-2159/2450 happened years + * later. Enumerating parameters does not hold; enforcing the invariant does. + * + * This fails on any newly added request path that interpolates a value without the + * `apiPath` tag, so the class cannot be reintroduced by a tool author who is unaware + * of it. + */ +describe("client.ts request path invariant", () => { + const source = readFileSync(new URL("./client.ts", import.meta.url), "utf8"); + + it("interpolates every request path through apiPath", () => { + const offenders: string[] = []; + + source.split("\n").forEach((line, index) => { + const trimmed = line.trim(); + if (trimmed.startsWith("//") || trimmed.startsWith("*")) return; + + // A template literal opening with a slash is an API request path. + for (let i = 0; i < line.length - 1; i++) { + if (line[i] !== "`" || line[i + 1] !== "/") continue; + const isTagged = line.slice(0, i).endsWith("apiPath"); + const interpolates = /\$\{/.test(line.slice(i)); + if (!isTagged && interpolates) { + offenders.push(`${index + 1}: ${trimmed}`); + } + } + }); + + expect( + offenders, + `Request paths must be built with the apiPath tag so interpolated values cannot escape their segment:\n${offenders.join("\n")}`, + ).toEqual([]); + }); + + it("does not double-encode inside an apiPath template", () => { + const doubleEncoded = [...source.matchAll(/apiPath`[^`]*`/g)] + .map((match) => match[0]) + .filter((template) => template.includes("encodeURIComponent")); + + expect(doubleEncoded).toEqual([]); + }); +}); diff --git a/packages/mcp-core/src/api-client/api-path.ts b/packages/mcp-core/src/api-client/api-path.ts new file mode 100644 index 000000000..2580793ea --- /dev/null +++ b/packages/mcp-core/src/api-client/api-path.ts @@ -0,0 +1,46 @@ +/** + * Safe construction of Sentry API request paths. + * + * Request paths are handed to `fetch`, which resolves `../` dot segments per the + * WHATWG URL spec before the request leaves the process. Interpolating a caller + * supplied value straight into a path therefore lets that value rewrite the path + * above the organization and project the server injected, reaching endpoints the + * session was never scoped to. + * + * `apiPath` encodes every interpolated value so it can only ever occupy the single + * path segment it was written into. Encoding is the structural defence: parameter + * level format validation is applied separately in the tool schemas for better + * error messages, but this layer is what holds for parameters nobody remembered to + * validate. + * + * Query strings must be appended outside the template, because `URLSearchParams` + * has already encoded them and the separators must stay literal: + * + * ```typescript + * const path = `${apiPath`/organizations/${organizationSlug}/issues/`}?${query}`; + * ``` + */ + +/** + * Interpolates values into an API path, percent-encoding each one. + * + * `encodeURIComponent` leaves `A-Za-z0-9`, `-`, `_`, `.`, `!`, `~`, `*`, `'`, `(` + * and `)` untouched, so every legitimate Sentry slug, numeric ID, hex ID, UUID and + * short ID passes through byte for byte. Only separators and traversal sequences + * change, which is exactly the intent. + * + * @param strings Literal portions of the template + * @param values Caller supplied values to encode into single path segments + * @returns The assembled path with every interpolated value encoded + */ +export function apiPath( + strings: TemplateStringsArray, + ...values: (string | number)[] +): string { + return strings.reduce((acc, literal, index) => { + if (index >= values.length) { + return acc + literal; + } + return acc + literal + encodeURIComponent(String(values[index])); + }, ""); +} diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index c067f5afc..9bbfa66ab 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -20,6 +20,7 @@ import { type TraceMetricIdentifier, } from "../utils/url-utils"; import { isNumericId } from "../utils/slug-validation"; +import { apiPath } from "./api-path"; import { isMetricsDataset, isProfilesDataset, @@ -1530,7 +1531,7 @@ export class SentryApiService { queryParams.set("query", params.query); } const queryString = queryParams.toString(); - const path = `/organizations/?${queryString}`; + const path = apiPath`/organizations/` + `?${queryString}`; // For self-hosted instances, the regions endpoint doesn't exist if (!this.isSaas()) { @@ -1587,7 +1588,7 @@ export class SentryApiService { */ async getOrganization(organizationSlug: string, opts?: RequestOptions) { const body = await this.requestJSON( - `/organizations/${organizationSlug}/`, + apiPath`/organizations/${organizationSlug}/`, undefined, opts, ); @@ -1615,7 +1616,8 @@ export class SentryApiService { queryParams.set("query", params.query); } const queryString = queryParams.toString(); - const path = `/organizations/${organizationSlug}/teams/?${queryString}`; + const path = + apiPath`/organizations/${organizationSlug}/teams/` + `?${queryString}`; const body = await this.requestJSON(path, undefined, opts); return TeamListSchema.parse(body); @@ -1642,7 +1644,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/teams/`, + apiPath`/organizations/${organizationSlug}/teams/`, { method: "POST", body: JSON.stringify({ name }), @@ -1673,7 +1675,8 @@ export class SentryApiService { queryParams.set("query", params.query); } const queryString = queryParams.toString(); - const path = `/organizations/${organizationSlug}/projects/?${queryString}`; + const path = + apiPath`/organizations/${organizationSlug}/projects/` + `?${queryString}`; const body = await this.requestJSON(path, undefined, opts); return ProjectListSchema.parse(body); @@ -1708,7 +1711,8 @@ export class SentryApiService { } const response = await this.request( - `/organizations/${organizationSlug}/dashboards/?${queryParams.toString()}`, + apiPath`/organizations/${organizationSlug}/dashboards/` + + `?${queryParams.toString()}`, undefined, opts, ); @@ -1731,7 +1735,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/dashboards/${dashboardId}/`, + apiPath`/organizations/${organizationSlug}/dashboards/${dashboardId}/`, undefined, opts, ); @@ -1758,7 +1762,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlugOrId}/`, + apiPath`/projects/${organizationSlug}/${projectSlugOrId}/`, undefined, opts, ); @@ -1803,7 +1807,7 @@ export class SentryApiService { } const body = await this.requestJSON( - `/teams/${organizationSlug}/${teamSlug}/projects/`, + apiPath`/teams/${organizationSlug}/${teamSlug}/projects/`, { method: "POST", body: JSON.stringify(createData), @@ -1848,7 +1852,7 @@ export class SentryApiService { if (platform) updateData.platform = platform; const body = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlug}/`, + apiPath`/projects/${organizationSlug}/${projectSlug}/`, { method: "PUT", body: JSON.stringify(updateData), @@ -1882,7 +1886,8 @@ export class SentryApiService { } const response = await this.request( - `/organizations/${organizationSlug}/repos/?${params.toString()}`, + apiPath`/organizations/${organizationSlug}/repos/` + + `?${params.toString()}`, { method: "GET" }, opts, ); @@ -1909,7 +1914,7 @@ export class SentryApiService { opts?: RequestOptions, ) { const body = await this.requestJSON( - `/organizations/${organizationSlug}/code-mappings/bulk/`, + apiPath`/organizations/${organizationSlug}/code-mappings/bulk/`, { method: "POST", body: JSON.stringify({ @@ -1979,7 +1984,8 @@ export class SentryApiService { } const response = await this.request( - `/organizations/${organizationSlug}/workflows/?${searchQuery.toString()}`, + apiPath`/organizations/${organizationSlug}/workflows/` + + `?${searchQuery.toString()}`, undefined, opts, ); @@ -2020,7 +2026,8 @@ export class SentryApiService { searchQuery.set("per_page", "1"); const response = await this.request( - `/organizations/${organizationSlug}/workflows/?${searchQuery.toString()}`, + apiPath`/organizations/${organizationSlug}/workflows/` + + `?${searchQuery.toString()}`, undefined, opts, ); @@ -2110,7 +2117,7 @@ export class SentryApiService { } const queryString = searchQuery.toString(); - const path = `/organizations/${organizationSlug}/alert-rules/`; + const path = apiPath`/organizations/${organizationSlug}/alert-rules/`; const response = await this.request( `${path}${queryString ? `?${queryString}` : ""}`, undefined, @@ -2133,7 +2140,7 @@ export class SentryApiService { }, opts?: RequestOptions, ): Promise { - const path = `/organizations/${organizationSlug}/alert-rules/${encodeURIComponent(String(ruleId))}/`; + const path = apiPath`/organizations/${organizationSlug}/alert-rules/${ruleId}/`; const body = await this.requestJSON(path, undefined, opts); return MetricAlertRuleSchema.parse(body); } @@ -2173,7 +2180,8 @@ export class SentryApiService { } const response = await this.request( - `/organizations/${organizationSlug}/combined-rules/?${searchQuery.toString()}`, + apiPath`/organizations/${organizationSlug}/combined-rules/` + + `?${searchQuery.toString()}`, undefined, opts, ); @@ -2213,7 +2221,8 @@ export class SentryApiService { } const response = await this.request( - `/projects/${organizationSlug}/${projectSlug}/teams/?${queryParams.toString()}`, + apiPath`/projects/${organizationSlug}/${projectSlug}/teams/` + + `?${queryParams.toString()}`, undefined, opts, ); @@ -2247,7 +2256,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { await this.request( - `/projects/${organizationSlug}/${projectSlug}/teams/${teamSlug}/`, + apiPath`/projects/${organizationSlug}/${projectSlug}/teams/${teamSlug}/`, { method: "POST", body: JSON.stringify({}), @@ -2278,7 +2287,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { await this.request( - `/projects/${organizationSlug}/${projectSlug}/teams/${teamSlug}/`, + apiPath`/projects/${organizationSlug}/${projectSlug}/teams/${teamSlug}/`, { method: "DELETE", }, @@ -2321,7 +2330,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlug}/keys/`, + apiPath`/projects/${organizationSlug}/${projectSlug}/keys/`, { method: "POST", body: JSON.stringify({ @@ -2383,7 +2392,7 @@ export class SentryApiService { } const body = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlug}/keys/${keyId}/`, + apiPath`/projects/${organizationSlug}/${projectSlug}/keys/${keyId}/`, { method: "PUT", body: JSON.stringify(updateData), @@ -2413,7 +2422,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlug}/keys/`, + apiPath`/projects/${organizationSlug}/${projectSlug}/keys/`, undefined, opts, ); @@ -2467,8 +2476,8 @@ export class SentryApiService { } const path = projectSlug - ? `/projects/${organizationSlug}/${projectSlug}/releases/` - : `/organizations/${organizationSlug}/releases/`; + ? apiPath`/projects/${organizationSlug}/${projectSlug}/releases/` + : apiPath`/organizations/${organizationSlug}/releases/`; const body = await this.requestJSON( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, @@ -2497,10 +2506,9 @@ export class SentryApiService { searchQuery.set("health", "1"); } - const encodedVersion = encodeURIComponent(releaseVersion); const path = projectSlugOrId - ? `/projects/${organizationSlug}/${projectSlugOrId}/releases/${encodedVersion}/` - : `/organizations/${organizationSlug}/releases/${encodedVersion}/`; + ? apiPath`/projects/${organizationSlug}/${projectSlugOrId}/releases/${releaseVersion}/` + : apiPath`/organizations/${organizationSlug}/releases/${releaseVersion}/`; const body = await this.requestJSON( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, undefined, @@ -2534,8 +2542,7 @@ export class SentryApiService { searchQuery.set("per_page", String(limit)); } - const encodedVersion = encodeURIComponent(releaseVersion); - const path = `/organizations/${organizationSlug}/releases/${encodedVersion}/deploys/`; + const path = apiPath`/organizations/${organizationSlug}/releases/${releaseVersion}/deploys/`; const body = await this.requestJSON( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, undefined, @@ -2563,10 +2570,9 @@ export class SentryApiService { searchQuery.set("per_page", String(limit)); } - const encodedVersion = encodeURIComponent(releaseVersion); const path = projectSlugOrId - ? `/projects/${organizationSlug}/${projectSlugOrId}/releases/${encodedVersion}/commits/` - : `/organizations/${organizationSlug}/releases/${encodedVersion}/commits/`; + ? apiPath`/projects/${organizationSlug}/${projectSlugOrId}/releases/${releaseVersion}/commits/` + : apiPath`/organizations/${organizationSlug}/releases/${releaseVersion}/commits/`; const body = await this.requestJSON( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, undefined, @@ -2612,8 +2618,9 @@ export class SentryApiService { const body = await this.requestJSON( searchQuery.toString() - ? `/organizations/${organizationSlug}/monitors/?${searchQuery.toString()}` - : `/organizations/${organizationSlug}/monitors/`, + ? apiPath`/organizations/${organizationSlug}/monitors/` + + `?${searchQuery.toString()}` + : apiPath`/organizations/${organizationSlug}/monitors/`, undefined, opts, ); @@ -2639,10 +2646,9 @@ export class SentryApiService { searchQuery.append("environment", environment); } - const encodedMonitor = encodeURIComponent(monitorSlug); const path = projectSlug - ? `/projects/${organizationSlug}/${projectSlug}/monitors/${encodedMonitor}/` - : `/organizations/${organizationSlug}/monitors/${encodedMonitor}/`; + ? apiPath`/projects/${organizationSlug}/${projectSlug}/monitors/${monitorSlug}/` + : apiPath`/organizations/${organizationSlug}/monitors/${monitorSlug}/`; const body = await this.requestJSON( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, undefined, @@ -2688,10 +2694,9 @@ export class SentryApiService { } this.applyTimeParams(searchQuery, effectiveStatsPeriod, start, end); - const encodedMonitor = encodeURIComponent(monitorSlug); const path = projectSlug - ? `/projects/${organizationSlug}/${projectSlug}/monitors/${encodedMonitor}/checkins/` - : `/organizations/${organizationSlug}/monitors/${encodedMonitor}/checkins/`; + ? apiPath`/projects/${organizationSlug}/${projectSlug}/monitors/${monitorSlug}/checkins/` + : apiPath`/organizations/${organizationSlug}/monitors/${monitorSlug}/checkins/`; const body = await this.requestJSON( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, undefined, @@ -2737,10 +2742,9 @@ export class SentryApiService { rollup, ); - const encodedMonitor = encodeURIComponent(monitorSlug); const path = projectSlug - ? `/projects/${organizationSlug}/${projectSlug}/monitors/${encodedMonitor}/stats/` - : `/organizations/${organizationSlug}/monitors/${encodedMonitor}/stats/`; + ? apiPath`/projects/${organizationSlug}/${projectSlug}/monitors/${monitorSlug}/stats/` + : apiPath`/organizations/${organizationSlug}/monitors/${monitorSlug}/stats/`; const body = await this.requestJSON( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, undefined, @@ -2815,8 +2819,9 @@ export class SentryApiService { const body = await this.requestJSON( searchQuery.toString() - ? `/organizations/${organizationSlug}/tags/?${searchQuery.toString()}` - : `/organizations/${organizationSlug}/tags/`, + ? apiPath`/organizations/${organizationSlug}/tags/` + + `?${searchQuery.toString()}` + : apiPath`/organizations/${organizationSlug}/tags/`, undefined, opts, ); @@ -2880,8 +2885,9 @@ export class SentryApiService { const body = await this.requestJSON( searchQuery.toString() - ? `/organizations/${organizationSlug}/replays/?${searchQuery.toString()}` - : `/organizations/${organizationSlug}/replays/`, + ? apiPath`/organizations/${organizationSlug}/replays/` + + `?${searchQuery.toString()}` + : apiPath`/organizations/${organizationSlug}/replays/`, undefined, opts, ); @@ -2999,7 +3005,8 @@ export class SentryApiService { } const response = await this.request( - `/organizations/${organizationSlug}/events/validate/?${queryParams.toString()}`, + apiPath`/organizations/${organizationSlug}/events/validate/` + + `?${queryParams.toString()}`, undefined, { ...opts, allowStatuses: [400] }, ); @@ -3031,7 +3038,9 @@ export class SentryApiService { } this.applyTimeParams(queryParams, statsPeriod, start, end); - const url = `/organizations/${organizationSlug}/trace-items/attributes/?${queryParams.toString()}`; + const url = + apiPath`/organizations/${organizationSlug}/trace-items/attributes/` + + `?${queryParams.toString()}`; const body = await this.requestJSON(url, undefined, opts); return TraceItemAttributeListSchema.parse(body); @@ -3109,7 +3118,9 @@ export class SentryApiService { } queryParams.append("collapse", "unhandled"); - const apiUrl = `/organizations/${organizationSlug}/issues/?${queryParams.toString()}`; + const apiUrl = + apiPath`/organizations/${organizationSlug}/issues/` + + `?${queryParams.toString()}`; const body = await this.requestJSON(apiUrl, undefined, opts); return IssueListSchema.parse(body); @@ -3126,7 +3137,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/`, undefined, opts, ); @@ -3171,7 +3182,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/tags/${tagKey}/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/tags/${tagKey}/`, undefined, opts, ); @@ -3201,7 +3212,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/external-issues/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/external-issues/`, undefined, opts, ); @@ -3233,7 +3244,7 @@ export class SentryApiService { searchQuery.set("per_page", String(limit)); } - const path = `/organizations/${organizationSlug}/issues/${issueId}/user-reports/`; + const path = apiPath`/organizations/${organizationSlug}/issues/${issueId}/user-reports/`; const response = await this.request( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, undefined, @@ -3259,7 +3270,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/events/${eventId}/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/events/${eventId}/`, undefined, opts, ); @@ -3394,7 +3405,8 @@ export class SentryApiService { if (sdkName) query.set("sdkName", sdkName); const body = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlug}/stacktrace-link/?${query.toString()}`, + apiPath`/projects/${organizationSlug}/${projectSlug}/stacktrace-link/` + + `?${query.toString()}`, { signal }, opts, ); @@ -3454,7 +3466,9 @@ export class SentryApiService { params.append("full", "true"); } - const apiUrl = `/organizations/${organizationSlug}/issues/${issueId}/events/?${params.toString()}`; + const apiUrl = + apiPath`/organizations/${organizationSlug}/issues/${issueId}/events/` + + `?${params.toString()}`; return await this.requestJSON(apiUrl, undefined, opts); } @@ -3471,7 +3485,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlug}/events/${eventId}/attachments/`, + apiPath`/projects/${organizationSlug}/${projectSlug}/events/${eventId}/attachments/`, undefined, opts, ); @@ -3500,7 +3514,7 @@ export class SentryApiService { }> { // Get the attachment metadata first const attachmentsData = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlug}/events/${eventId}/attachments/`, + apiPath`/projects/${organizationSlug}/${projectSlug}/events/${eventId}/attachments/`, undefined, opts, ); @@ -3515,7 +3529,9 @@ export class SentryApiService { } // Download the actual file content - const downloadUrl = `/projects/${organizationSlug}/${projectSlug}/events/${eventId}/attachments/${attachmentId}/?download=1`; + const downloadUrl = + apiPath`/projects/${organizationSlug}/${projectSlug}/events/${eventId}/attachments/${attachmentId}/` + + `?download=1`; const downloadResponse = await this.request( downloadUrl, { method: "GET" }, @@ -3550,7 +3566,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/replays/${replayId}/`, + apiPath`/organizations/${organizationSlug}/replays/${replayId}/`, undefined, opts, ); @@ -3578,7 +3594,8 @@ export class SentryApiService { queryParams.append("project", "-1"); const body = await this.requestJSON( - `/organizations/${organizationSlug}/replay-count/?${queryParams.toString()}`, + apiPath`/organizations/${organizationSlug}/replay-count/` + + `?${queryParams.toString()}`, undefined, opts, ); @@ -3600,7 +3617,8 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/projects/${organizationSlug}/${projectSlugOrId}/replays/${replayId}/recording-segments/?download=true`, + apiPath`/projects/${organizationSlug}/${projectSlugOrId}/replays/${replayId}/recording-segments/` + + `?download=true`, undefined, opts, ); @@ -3658,7 +3676,7 @@ export class SentryApiService { } const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/`, { method: "PUT", body: JSON.stringify(updateData), @@ -3681,7 +3699,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/notes/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/notes/`, { method: "POST", body: JSON.stringify({ text }), @@ -3702,7 +3720,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/activities/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/activities/`, undefined, opts, ); @@ -3726,7 +3744,7 @@ export class SentryApiService { searchQuery.set("per_page", String(limit)); } - const path = `/organizations/${organizationSlug}/issues/${issueId}/notes/`; + const path = apiPath`/organizations/${organizationSlug}/issues/${issueId}/notes/`; const body = await this.requestJSON( searchQuery.toString() ? `${path}?${searchQuery.toString()}` : path, undefined, @@ -3799,7 +3817,9 @@ export class SentryApiService { queryParams.set("query", sentryQuery.join(" ")); // if (projectSlug) queryParams.set("project", projectSlug); - const apiUrl = `/organizations/${organizationSlug}/events/?${queryParams.toString()}`; + const apiUrl = + apiPath`/organizations/${organizationSlug}/events/` + + `?${queryParams.toString()}`; const body = await this.requestJSON(apiUrl, undefined, opts); // TODO(dcramer): If you're using an older version of Sentry this API had a breaking change @@ -3854,7 +3874,9 @@ export class SentryApiService { queryParams.set("query", sentryQuery.join(" ")); // if (projectSlug) queryParams.set("project", projectSlug); - const apiUrl = `/organizations/${organizationSlug}/events/?${queryParams.toString()}`; + const apiUrl = + apiPath`/organizations/${organizationSlug}/events/` + + `?${queryParams.toString()}`; const body = await this.requestJSON(apiUrl, undefined, opts); return SpansSearchResponseSchema.parse(body).data; @@ -4031,7 +4053,9 @@ export class SentryApiService { }); } - const apiUrl = `/organizations/${organizationSlug}/events/?${queryParams.toString()}`; + const apiUrl = + apiPath`/organizations/${organizationSlug}/events/` + + `?${queryParams.toString()}`; return await this.requestJSON(apiUrl, undefined, opts); } @@ -4051,7 +4075,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/autofix/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/autofix/`, { method: "POST", body: JSON.stringify({ @@ -4076,7 +4100,7 @@ export class SentryApiService { opts?: RequestOptions, ): Promise { const body = await this.requestJSON( - `/organizations/${organizationSlug}/issues/${issueId}/autofix/`, + apiPath`/organizations/${organizationSlug}/issues/${issueId}/autofix/`, undefined, opts, ); @@ -4121,7 +4145,8 @@ export class SentryApiService { queryParams.set("statsPeriod", statsPeriod); const body = await this.requestJSON( - `/organizations/${organizationSlug}/trace-meta/${traceId}/?${queryParams.toString()}`, + apiPath`/organizations/${organizationSlug}/trace-meta/${traceId}/` + + `?${queryParams.toString()}`, undefined, opts, ); @@ -4177,7 +4202,8 @@ export class SentryApiService { queryParams.set("statsPeriod", statsPeriod); const body = await this.requestJSON( - `/organizations/${organizationSlug}/trace/${traceId}/?${queryParams.toString()}`, + apiPath`/organizations/${organizationSlug}/trace/${traceId}/` + + `?${queryParams.toString()}`, undefined, opts, ); @@ -4233,7 +4259,8 @@ export class SentryApiService { } const response = await this.request( - `/organizations/${organizationSlug}/ai-conversations/${encodeURIComponent(conversationId)}/?${queryParams.toString()}`, + apiPath`/organizations/${organizationSlug}/ai-conversations/${conversationId}/` + + `?${queryParams.toString()}`, undefined, opts, ); @@ -4314,7 +4341,8 @@ export class SentryApiService { } const response = await this.request( - `/organizations/${organizationSlug}/ai-conversations/?${queryParams.toString()}`, + apiPath`/organizations/${organizationSlug}/ai-conversations/` + + `?${queryParams.toString()}`, undefined, opts, ); @@ -4382,7 +4410,9 @@ export class SentryApiService { ); queryParams.set("statsPeriod", statsPeriod); - const path = `/organizations/${organizationSlug}/profiling/flamegraph/?${queryParams.toString()}`; + const path = + apiPath`/organizations/${organizationSlug}/profiling/flamegraph/` + + `?${queryParams.toString()}`; const body = await this.requestJSON(path, undefined, opts); return FlamegraphSchema.parse(body); } @@ -4399,7 +4429,7 @@ export class SentryApiService { }, opts?: RequestOptions, ): Promise { - const path = `/projects/${organizationSlug}/${projectSlugOrId}/profiling/profiles/${profileId}/`; + const path = apiPath`/projects/${organizationSlug}/${projectSlugOrId}/profiling/profiles/${profileId}/`; const body = await this.requestJSON(path, undefined, opts); return TransactionProfileSchema.parse(body); } @@ -4461,7 +4491,9 @@ export class SentryApiService { queryParams.set("start", start); queryParams.set("end", end); - const path = `/organizations/${organizationSlug}/profiling/chunks/?${queryParams.toString()}`; + const path = + apiPath`/organizations/${organizationSlug}/profiling/chunks/` + + `?${queryParams.toString()}`; const body = await this.requestJSON(path, undefined, opts); // Normalize the Sentry {chunk: ...} wrapper to the internal chunks array. @@ -4504,7 +4536,9 @@ export class SentryApiService { if (compactMetadata) { params.set("compact_metadata", "true"); } - const path = `/organizations/${encodeURIComponent(organizationSlug)}/preprodartifacts/snapshots/${encodeURIComponent(snapshotId)}/?${params.toString()}`; + const path = + apiPath`/organizations/${organizationSlug}/preprodartifacts/snapshots/${snapshotId}/` + + `?${params.toString()}`; return this.requestJSON(path); } @@ -4517,7 +4551,7 @@ export class SentryApiService { snapshotId: string; imageIdentifier: string; }): Promise { - const path = `/organizations/${encodeURIComponent(organizationSlug)}/preprodartifacts/snapshots/${encodeURIComponent(snapshotId)}/images/${encodeURIComponent(imageIdentifier)}/`; + const path = apiPath`/organizations/${organizationSlug}/preprodartifacts/snapshots/${snapshotId}/images/${imageIdentifier}/`; return this.requestJSON(path); } @@ -4566,7 +4600,9 @@ export class SentryApiService { if (project) params.set("project", project); if (projectSlug) params.set("projectSlug", projectSlug); if (compactMetadata) params.set("compact_metadata", "true"); - const path = `/organizations/${encodeURIComponent(organizationSlug)}/preprodartifacts/snapshots/latest-base/?${params.toString()}`; + const path = + apiPath`/organizations/${organizationSlug}/preprodartifacts/snapshots/latest-base/` + + `?${params.toString()}`; return this.requestJSON(path); } } diff --git a/packages/mcp-core/src/schema.ts b/packages/mcp-core/src/schema.ts index d14305b7f..0ae08b134 100644 --- a/packages/mcp-core/src/schema.ts +++ b/packages/mcp-core/src/schema.ts @@ -7,7 +7,7 @@ */ import { z } from "zod"; import { SENTRY_GUIDES } from "./constants"; -import { validateSlug } from "./utils/slug-validation"; +import { validateResourceId, validateSlug } from "./utils/slug-validation"; // Sentry slug lookups can be exact and case-sensitive on legacy instances. // Preserve caller casing for resource slugs; only trim and validate shape. @@ -67,6 +67,7 @@ export const ParamIssueUrl = z export const ParamReplayId = z .string() .trim() + .superRefine(validateResourceId) .describe("The replay ID. e.g. `7e07485f-12f9-416b-8b14-26260799b51f`"); export const ParamReplayUrl = z @@ -237,9 +238,14 @@ export const ParamSentryGuide = z "Use either a platform (e.g., 'javascript', 'python') or platform/guide combination (e.g., 'javascript/nextjs', 'python/django').", ); -export const ParamEventId = z.string().trim().describe("The ID of the event."); +export const ParamEventId = z + .string() + .trim() + .superRefine(validateResourceId) + .describe("The ID of the event."); export const ParamAttachmentId = z .string() .trim() + .superRefine(validateResourceId) .describe("The ID of the attachment to download."); diff --git a/packages/mcp-core/src/tools/catalog/get-event-stacktrace.ts b/packages/mcp-core/src/tools/catalog/get-event-stacktrace.ts index 897414755..0dc90376d 100644 --- a/packages/mcp-core/src/tools/catalog/get-event-stacktrace.ts +++ b/packages/mcp-core/src/tools/catalog/get-event-stacktrace.ts @@ -11,6 +11,7 @@ import { ParamRegionUrl, } from "../../schema"; import type { ServerContext } from "../../types"; +import { validateResourceId } from "../../utils/slug-validation"; import { fetchAndFormatEventStacktrace } from "../support/event-stacktrace"; export default defineTool({ @@ -44,6 +45,7 @@ export default defineTool({ eventId: z .string() .trim() + .superRefine(validateResourceId) .default("latest") .describe("The event ID for the issue. Defaults to `latest`."), thread: z diff --git a/packages/mcp-core/src/tools/catalog/get-issue-details.ts b/packages/mcp-core/src/tools/catalog/get-issue-details.ts index 1060db499..87d6050bb 100644 --- a/packages/mcp-core/src/tools/catalog/get-issue-details.ts +++ b/packages/mcp-core/src/tools/catalog/get-issue-details.ts @@ -1,5 +1,4 @@ import { setTag } from "@sentry/core"; -import { z } from "zod"; import { ApiNotFoundError } from "../../api-client"; import type { SentryApiService } from "../../api-client"; import type { @@ -27,6 +26,7 @@ import { parseIssueParams, } from "../../internal/tool-helpers/issue"; import { + ParamEventId, ParamIssueShortId, ParamIssueUrl, ParamOrganizationSlug, @@ -88,7 +88,7 @@ export default defineTool({ organizationSlug: ParamOrganizationSlug.optional(), regionUrl: ParamRegionUrl.nullable().default(null), issueId: ParamIssueShortId.optional(), - eventId: z.string().trim().describe("The ID of the event.").optional(), + eventId: ParamEventId.optional(), issueUrl: ParamIssueUrl.optional(), }, annotations: { diff --git a/packages/mcp-core/src/tools/catalog/get-issue-tag-values.test.ts b/packages/mcp-core/src/tools/catalog/get-issue-tag-values.test.ts index 4a9b96b43..3ee1c9f1a 100644 --- a/packages/mcp-core/src/tools/catalog/get-issue-tag-values.test.ts +++ b/packages/mcp-core/src/tools/catalog/get-issue-tag-values.test.ts @@ -160,34 +160,28 @@ describe("get_issue_tag_values", () => { ).rejects.toThrow(UserInputError); }); - it("throws error when tagKey contains path traversal characters", async () => { - await expect( - getIssueTagValues.handler( - { - organizationSlug: "sentry-mcp-evals", - issueId: "CLOUDFLARE-MCP-41", - tagKey: "../../../admin", - regionUrl: null, - issueUrl: undefined, - }, - getServerContext(), - ), - ).rejects.toThrow(); + /** + * These assert against the input schema rather than the handler because the + * handler receives already-validated params. Driving the handler directly skips + * Zod, so an earlier version of these tests passed only because MSW rejected the + * unmocked URL, not because the traversal was caught. + */ + it.each([ + ["dot segments", "../../../admin"], + ["slashes", "url/path"], + ["backslashes", "..\\..\\admin"], + ["encoded dot segments", "%2e%2e%2fadmin"], + ["leading dot", ".hidden"], + ])("rejects tagKey containing %s", (_label, tagKey) => { + expect(getIssueTagValues.inputSchema.tagKey.safeParse(tagKey).success).toBe( + false, + ); }); - it("throws error when tagKey contains slashes", async () => { - await expect( - getIssueTagValues.handler( - { - organizationSlug: "sentry-mcp-evals", - issueId: "CLOUDFLARE-MCP-41", - tagKey: "url/path", - regionUrl: null, - issueUrl: undefined, - }, - getServerContext(), - ), - ).rejects.toThrow(); + it("accepts a conventional tag key", () => { + expect(getIssueTagValues.inputSchema.tagKey.safeParse("url").success).toBe( + true, + ); }); it("handles null values in topValues gracefully", async () => { diff --git a/packages/mcp-core/src/tools/catalog/get-profile-details.ts b/packages/mcp-core/src/tools/catalog/get-profile-details.ts index 227ac393d..8aa5309d5 100644 --- a/packages/mcp-core/src/tools/catalog/get-profile-details.ts +++ b/packages/mcp-core/src/tools/catalog/get-profile-details.ts @@ -6,7 +6,11 @@ import { resolveRegionUrlForOrganization } from "../../internal/tool-helpers/res import { UserInputError } from "../../errors"; import type { ServerContext } from "../../types"; import { ParamOrganizationSlug, ParamRegionUrl } from "../../schema"; -import { isNumericId } from "../../utils/slug-validation"; +import { + isNumericId, + validateResourceId, + validateSlugOrId, +} from "../../utils/slug-validation"; import { parseSentryUrl, isProfileUrl } from "../../internal/url-helpers"; import { resolveScopedOrganizationSlug, @@ -234,17 +238,19 @@ export default defineTool({ organizationSlug: ParamOrganizationSlug.optional(), regionUrl: ParamRegionUrl.nullable().default(null), projectSlugOrId: z - .union([z.string(), z.number()]) + .union([z.string().trim().superRefine(validateSlugOrId), z.number()]) .optional() .describe("Project slug or numeric ID"), profileId: z .string() .trim() + .superRefine(validateResourceId) .optional() .describe("Transaction profile ID from a profile flamegraph URL"), profilerId: z .string() .trim() + .superRefine(validateResourceId) .optional() .describe("Continuous profiler session ID"), start: z diff --git a/packages/mcp-core/src/tools/catalog/get-profile.ts b/packages/mcp-core/src/tools/catalog/get-profile.ts index f31c54964..4098d2134 100644 --- a/packages/mcp-core/src/tools/catalog/get-profile.ts +++ b/packages/mcp-core/src/tools/catalog/get-profile.ts @@ -9,7 +9,7 @@ import { ParamPeriod, ParamRegionUrl, } from "../../schema"; -import { isNumericId } from "../../utils/slug-validation"; +import { isNumericId, validateSlugOrId } from "../../utils/slug-validation"; import { formatFlamegraphAnalysis, formatFlamegraphComparison, @@ -152,7 +152,7 @@ export default defineTool({ organizationSlug: ParamOrganizationSlug.optional(), regionUrl: ParamRegionUrl.nullable().default(null), projectSlugOrId: z - .union([z.string(), z.number()]) + .union([z.string().trim().superRefine(validateSlugOrId), z.number()]) .optional() .describe("Project slug or numeric ID"), transactionName: z diff --git a/packages/mcp-core/src/tools/catalog/path-traversal.test.ts b/packages/mcp-core/src/tools/catalog/path-traversal.test.ts new file mode 100644 index 000000000..75f988d95 --- /dev/null +++ b/packages/mcp-core/src/tools/catalog/path-traversal.test.ts @@ -0,0 +1,352 @@ +/** + * Regression tests for the MCP path traversal class (VULN-2159, VULN-2174, + * VULN-2450, VULN-2482). + * + * Unvalidated, unencoded Sentry resource IDs were interpolated into upstream REST + * paths. Because `fetch` resolves `../` dot segments during URL parsing, an ID could + * rewrite the path above the organization and project the server injects from the + * session constraints, reaching other tenants with the granting user's credential. + * + * These tests drive tools through the same pipeline the server uses: the constrained + * keys are stripped from the schema the client sees, the remaining params are parsed + * with Zod, and the constraints are then injected server-side. Asserting on the + * outbound request URL is what makes these meaningful, since the defect was only ever + * observable in the path that actually left the process. + */ +import { mswServer } from "@sentry/mcp-server-mocks"; +import { z } from "zod"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { SentryApiService } from "../../api-client"; +import { + getFilteredInputSchema, + injectConstraintParams, +} from "../catalog-runtime/availability"; +import type { ToolConfig } from "../types"; +import type { ServerContext } from "../../types"; +import { getServerContext } from "../../test-setup.js"; +import getEventAttachment from "./get-event-attachment"; +import getIssueDetails from "./get-issue-details"; +import getEventStacktrace from "./get-event-stacktrace"; +import getProfileDetails from "./get-profile-details"; +import getReplayDetails from "./get-replay-details"; +import updateDsn from "./update-dsn"; + +const SCOPED_ORG = "sentry-mcp-evals"; +const SCOPED_PROJECT = "cloudflare-mcp"; +const VICTIM_ORG = "bbtest-victimorg"; +const VICTIM_PROJECT = "victim-proj"; + +/** + * The exact payload shapes from the reports, plus the bypass variants VULN-2450 + * asked us to retest because encode-only fixes for this class have historically + * been circumvented. + */ +const TRAVERSAL_PAYLOADS = [ + ["cross-project", `../../${VICTIM_PROJECT}/events/abc123`], + ["cross-org", `../../../${VICTIM_ORG}/${VICTIM_PROJECT}/events/abc123`], + [ + "deep cross-org", + `../../../../../organizations/${VICTIM_ORG}/issues/130651143/events/latest`, + ], + ["escape api root", "../../../../../../../auth/config"], + ["double encoded", `%252e%252e%252f%252e%252e%252f${VICTIM_ORG}`], + ["single encoded", `%2e%2e%2f%2e%2e%2f${VICTIM_ORG}`], + ["recursive", `....//....//${VICTIM_ORG}`], + ["backslash", `..\\..\\${VICTIM_ORG}`], + ["null byte", `abc\u0000/../${VICTIM_ORG}`], + ["fragment truncation", "abc/#"], + ["query injection", "abc/?download=1"], +] as const; + +let requestedUrls: string[] = []; + +function recordRequest({ request }: { request: Request }) { + requestedUrls.push(request.url); +} + +beforeEach(() => { + requestedUrls = []; + mswServer.events.on("request:start", recordRequest); +}); + +afterEach(() => { + mswServer.events.removeListener("request:start", recordRequest); +}); + +/** + * Mirrors how the server handles a tool call: constrained keys are absent from the + * schema exposed to the client, the rest is Zod-validated, then constraints are + * injected. Returns the Zod error instead of throwing so callers can assert that + * either validation rejected the input or the request stayed in scope. + */ +async function callToolAsClient( + tool: ToolConfig, + clientParams: Record, + context: ServerContext, +): Promise<{ rejected: boolean }> { + const schema = z.object(getFilteredInputSchema(tool, context)); + const parsed = schema.safeParse(clientParams); + if (!parsed.success) { + return { rejected: true }; + } + + try { + await tool.handler( + injectConstraintParams(parsed.data, tool, context), + context, + ); + } catch { + // An upstream 404 or a formatting failure is irrelevant here. The assertion + // that matters is which URL was requested, which is captured either way. + } + return { rejected: false }; +} + +/** + * Every upstream request must stay within the constrained org and project. + * + * Asserts on decoded path *segments* rather than substrings: an encoded payload + * legitimately still contains the victim slug as literal text, so a substring check + * would flag a contained payload as an escape. + */ +function expectAllRequestsInScope() { + for (const url of requestedUrls) { + // Sentry API paths are /api/0/{projects|organizations}/{org}/... + const segments = new URL(url).pathname.split("/").filter(Boolean); + const anchor = segments.findIndex( + (segment) => segment === "projects" || segment === "organizations", + ); + + expect(anchor).not.toBe(-1); + expect(segments[anchor + 1]).toBe(SCOPED_ORG); + expect(segments).not.toContain(VICTIM_ORG); + expect(segments).not.toContain(VICTIM_PROJECT); + } +} + +describe("path traversal containment", () => { + const scopedContext = () => + getServerContext({ + constraints: { + organizationSlug: SCOPED_ORG, + projectSlug: SCOPED_PROJECT, + }, + }); + + describe("a project-scoped session cannot escape its constraint", () => { + it.each(TRAVERSAL_PAYLOADS)( + "get_event_attachment eventId: %s", + async (_label, payload) => { + await callToolAsClient( + getEventAttachment, + { eventId: payload }, + scopedContext(), + ); + expectAllRequestsInScope(); + }, + ); + + it.each(TRAVERSAL_PAYLOADS)( + "get_event_attachment attachmentId: %s", + async (_label, payload) => { + await callToolAsClient( + getEventAttachment, + { + eventId: "99be789ee5555abf1ad81bd47c2c2e36", + attachmentId: payload, + }, + scopedContext(), + ); + expectAllRequestsInScope(); + }, + ); + + it.each(TRAVERSAL_PAYLOADS)( + "get_issue_details eventId: %s", + async (_label, payload) => { + await callToolAsClient( + getIssueDetails, + { issueId: "CLOUDFLARE-MCP-41", eventId: payload }, + scopedContext(), + ); + expectAllRequestsInScope(); + }, + ); + + it.each(TRAVERSAL_PAYLOADS)( + "get_event_stacktrace eventId: %s", + async (_label, payload) => { + await callToolAsClient( + getEventStacktrace, + { issueId: "CLOUDFLARE-MCP-41", eventId: payload }, + scopedContext(), + ); + expectAllRequestsInScope(); + }, + ); + + it.each(TRAVERSAL_PAYLOADS)( + "get_replay_details replayId: %s", + async (_label, payload) => { + await callToolAsClient( + getReplayDetails, + { replayId: payload }, + scopedContext(), + ); + expectAllRequestsInScope(); + }, + ); + + it.each(TRAVERSAL_PAYLOADS)( + "get_profile_details profileId: %s", + async (_label, payload) => { + await callToolAsClient( + getProfileDetails, + { profileId: payload }, + scopedContext(), + ); + expectAllRequestsInScope(); + }, + ); + + /** + * The write half of VULN-2450: a confined token renamed and disabled DSNs in + * another organization. This is the highest-impact sink in the class. + */ + it.each(TRAVERSAL_PAYLOADS)( + "update_dsn keyId: %s", + async (_label, payload) => { + await callToolAsClient( + updateDsn, + { keyId: payload, name: "XORG-PWN", isActive: false }, + scopedContext(), + ); + expectAllRequestsInScope(); + }, + ); + }); + + describe("an org-only session cannot escape its organization", () => { + const orgOnlyContext = () => + getServerContext({ + constraints: { organizationSlug: SCOPED_ORG, projectSlug: null }, + }); + + it.each(TRAVERSAL_PAYLOADS)( + "get_event_attachment eventId: %s", + async (_label, payload) => { + await callToolAsClient( + getEventAttachment, + { projectSlug: SCOPED_PROJECT, eventId: payload }, + orgOnlyContext(), + ); + expectAllRequestsInScope(); + }, + ); + + it.each(TRAVERSAL_PAYLOADS)( + "update_dsn keyId: %s", + async (_label, payload) => { + await callToolAsClient( + updateDsn, + { + projectSlug: SCOPED_PROJECT, + keyId: payload, + name: "XORG-PWN", + isActive: false, + }, + orgOnlyContext(), + ); + expectAllRequestsInScope(); + }, + ); + }); + + /** + * The schema refinements reject every payload above, so the tool-level tests never + * reach the API client. That makes them a test of layer 2 only. + * + * This block drives the client directly with the same payloads, standing in for a + * parameter that a future tool forgets to validate. It is the actual evidence that + * encoding is load-bearing on its own, which is the reason this class of bug should + * not recur the way it did after the slug-only fix in VULN-848. + */ + describe("the API client contains traversal without any validation layer", () => { + const apiService = () => + new SentryApiService({ accessToken: "access-token", host: "sentry.io" }); + + it.each(TRAVERSAL_PAYLOADS)( + "listEventAttachments eventId: %s", + async (_label, payload) => { + await apiService() + .listEventAttachments({ + organizationSlug: SCOPED_ORG, + projectSlug: SCOPED_PROJECT, + eventId: payload, + }) + .catch(() => {}); + + expect(requestedUrls).toHaveLength(1); + expectAllRequestsInScope(); + }, + ); + + it.each(TRAVERSAL_PAYLOADS)( + "getTransactionProfile profileId: %s", + async (_label, payload) => { + await apiService() + .getTransactionProfile({ + organizationSlug: SCOPED_ORG, + projectSlugOrId: SCOPED_PROJECT, + profileId: payload, + }) + .catch(() => {}); + + expect(requestedUrls).toHaveLength(1); + expectAllRequestsInScope(); + }, + ); + + it.each(TRAVERSAL_PAYLOADS)( + "updateClientKey keyId: %s", + async (_label, payload) => { + await apiService() + .updateClientKey({ + organizationSlug: SCOPED_ORG, + projectSlug: SCOPED_PROJECT, + keyId: payload, + name: "XORG-PWN", + isActive: false, + }) + .catch(() => {}); + + expect(requestedUrls).toHaveLength(1); + expectAllRequestsInScope(); + }, + ); + }); + + describe("constraint keys remain unreachable by the client", () => { + it("strips organizationSlug and projectSlug from the exposed schema", () => { + const schema = getFilteredInputSchema( + getEventAttachment, + scopedContext(), + ); + + expect(Object.keys(schema)).not.toContain("organizationSlug"); + expect(Object.keys(schema)).not.toContain("projectSlug"); + expect(Object.keys(schema)).toContain("eventId"); + }); + + it("ignores a client attempt to override the injected constraints", () => { + const injected = injectConstraintParams( + { organizationSlug: VICTIM_ORG, projectSlug: VICTIM_PROJECT }, + getEventAttachment, + scopedContext(), + ); + + expect(injected.organizationSlug).toBe(SCOPED_ORG); + expect(injected.projectSlug).toBe(SCOPED_PROJECT); + }); + }); +}); diff --git a/packages/mcp-core/src/tools/catalog/update-dsn.ts b/packages/mcp-core/src/tools/catalog/update-dsn.ts index 70ed1b3fc..98db652e4 100644 --- a/packages/mcp-core/src/tools/catalog/update-dsn.ts +++ b/packages/mcp-core/src/tools/catalog/update-dsn.ts @@ -9,6 +9,7 @@ import { ParamRegionUrl, ParamProjectSlug, } from "../../schema"; +import { validateResourceId } from "../../utils/slug-validation"; export default defineTool({ name: "update_dsn", @@ -54,6 +55,7 @@ export default defineTool({ keyId: z .string() .trim() + .superRefine(validateResourceId) .describe( "The ID of the DSN (client key) to update. Use find_dsns() to retrieve this ID first.", ), diff --git a/packages/mcp-core/src/utils/slug-validation.ts b/packages/mcp-core/src/utils/slug-validation.ts index 36bf4d4f7..a72cb82d8 100644 --- a/packages/mcp-core/src/utils/slug-validation.ts +++ b/packages/mcp-core/src/utils/slug-validation.ts @@ -76,6 +76,54 @@ export function validateSlug(val: string, ctx: z.RefinementCtx): void { } } +/** + * Validates a Sentry resource identifier (event, attachment, replay, profile, + * client key, dashboard) to prevent path traversal. + * + * Uses the same character set as slugs, which accommodates every identifier + * format the API returns: numeric IDs, 32-character hex IDs, hyphenated UUIDs, + * `PROJECT-1Z43` short IDs, and the literal `latest`. Rejecting `/`, `\` and `%` + * outright means a traversal or double-encoded payload never reaches the API + * client, and requiring a leading alphanumeric blocks a bare `..`. + * + * The API client percent-encodes path segments regardless (see + * `api-client/api-path.ts`); this layer exists so callers get an actionable + * validation error instead of a confusing upstream 404. + * + * @example + * ```typescript + * const EventId = z.string() + * .trim() + * .superRefine(validateResourceId) + * .describe("The ID of the event"); + * ``` + */ +export function validateResourceId(val: string, ctx: z.RefinementCtx): void { + if (val.length === 0) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "ID cannot be empty", + }); + return; + } + + if (val.length > MAX_SLUG_LENGTH) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `ID exceeds maximum length of ${MAX_SLUG_LENGTH} characters`, + }); + return; + } + + if (!VALID_SLUG_PATTERN.test(val)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: + "ID must contain only alphanumeric characters, hyphens, underscores, and dots, and must start with an alphanumeric character", + }); + } +} + /** * Validates a parameter that can be either a slug or numeric ID. * Designed to be used with Zod's superRefine() method. From f9670c17a26164e3db25ce9c5ab690657a54aea1 Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:02:35 -0400 Subject: [PATCH 2/3] fix(api): reject bare dot segments in apiPath --- .../mcp-core/src/api-client/api-path.test.ts | 78 ++++++++++- packages/mcp-core/src/api-client/api-path.ts | 59 +++++--- packages/mcp-core/src/api-client/client.ts | 10 +- .../src/tools/catalog/path-traversal.test.ts | 126 ++++++++++-------- 4 files changed, 197 insertions(+), 76 deletions(-) diff --git a/packages/mcp-core/src/api-client/api-path.test.ts b/packages/mcp-core/src/api-client/api-path.test.ts index e8cb74a54..0e4ed404b 100644 --- a/packages/mcp-core/src/api-client/api-path.test.ts +++ b/packages/mcp-core/src/api-client/api-path.test.ts @@ -1,5 +1,6 @@ import { readFileSync } from "node:fs"; import { describe, expect, it } from "vitest"; +import { UserInputError } from "../errors"; import { apiPath } from "./api-path"; /** @@ -84,6 +85,9 @@ describe("apiPath", () => { ["scheme relative", "//evil.example.com/x"], ["absolute path", "/organizations/other-org/members"], ["trailing dot segment", "abc/.."], + ["dots with a suffix", "..a"], + ["dots with a prefix", "a.."], + ["three dots", "..."], ])("contains %s", (_label, attack) => { const resolved = resolve( apiPath`/projects/my-org/my-proj/events/${attack}/`, @@ -111,6 +115,51 @@ describe("apiPath", () => { expect(url.search).toBe(""); }); }); + + /** + * A value that is entirely a dot segment carries no separator, so encoding leaves + * it intact and the URL parser still collapses it. It cannot reach another tenant + * (that needs a `/` to name one) but it does consume a preceding segment and move + * the request to a different endpoint, so it has to be rejected rather than encoded. + */ + describe("bare dot segments", () => { + it.each([".", ".."])("rejects %s", (attack) => { + expect(() => apiPath`/projects/o/p/keys/${attack}/`).toThrow( + UserInputError, + ); + }); + + it("would otherwise consume the preceding segment", () => { + // Demonstrates why encoding cannot solve this: the encoded form still collapses. + expect(resolve(`/projects/o/p/keys/${encodeURIComponent("..")}/`)).toBe( + "/api/0/projects/o/p/", + ); + expect(resolve("/projects/o/p/keys/%2e%2e/")).toBe( + "/api/0/projects/o/p/", + ); + }); + + /** + * A percent-encoded spelling arriving as *input* is already inert, because the `%` + * itself gets encoded (`%2e%2e` becomes `%252e%252e`). These must be contained + * rather than rejected, so that a legitimate value is never refused. + */ + it.each(["%2e", "%2E", "%2e%2e", ".%2e", "%2e."])( + "contains rather than rejects %s", + (value) => { + expect(() => apiPath`/projects/o/p/keys/${value}/`).not.toThrow(); + expect(resolve(apiPath`/projects/o/p/keys/${value}/`)).toBe( + `/api/0/projects/o/p/keys/${encodeURIComponent(value)}/`, + ); + }, + ); + + it("still accepts identifiers that merely contain dots", () => { + for (const value of ["1.2.3", "my.project", "...", "..a", "a.."]) { + expect(() => apiPath`/projects/o/p/keys/${value}/`).not.toThrow(); + } + }); + }); }); /** @@ -125,6 +174,27 @@ describe("apiPath", () => { describe("client.ts request path invariant", () => { const source = readFileSync(new URL("./client.ts", import.meta.url), "utf8"); + /** + * Returns the path portion of a template literal body, stopping at the query + * separator. Only interpolation before the `?` needs the `apiPath` tag; query + * values are already encoded by `URLSearchParams` and must keep their separators + * literal. + */ + function pathPortion(body: string): string { + let depth = 0; + for (let i = 0; i < body.length; i++) { + if (body[i] === "$" && body[i + 1] === "{") { + depth++; + i++; + } else if (body[i] === "}" && depth > 0) { + depth--; + } else if (body[i] === "?" && depth === 0) { + return body.slice(0, i); + } + } + return body; + } + it("interpolates every request path through apiPath", () => { const offenders: string[] = []; @@ -135,9 +205,13 @@ describe("client.ts request path invariant", () => { // A template literal opening with a slash is an API request path. for (let i = 0; i < line.length - 1; i++) { if (line[i] !== "`" || line[i + 1] !== "/") continue; + + const closing = line.indexOf("`", i + 1); + const body = line.slice(i + 1, closing === -1 ? undefined : closing); const isTagged = line.slice(0, i).endsWith("apiPath"); - const interpolates = /\$\{/.test(line.slice(i)); - if (!isTagged && interpolates) { + const interpolatesPath = /\$\{/.test(pathPortion(body)); + + if (!isTagged && interpolatesPath) { offenders.push(`${index + 1}: ${trimmed}`); } } diff --git a/packages/mcp-core/src/api-client/api-path.ts b/packages/mcp-core/src/api-client/api-path.ts index 2580793ea..1f47101d4 100644 --- a/packages/mcp-core/src/api-client/api-path.ts +++ b/packages/mcp-core/src/api-client/api-path.ts @@ -1,37 +1,58 @@ /** * Safe construction of Sentry API request paths. * - * Request paths are handed to `fetch`, which resolves `../` dot segments per the - * WHATWG URL spec before the request leaves the process. Interpolating a caller - * supplied value straight into a path therefore lets that value rewrite the path - * above the organization and project the server injected, reaching endpoints the - * session was never scoped to. - * - * `apiPath` encodes every interpolated value so it can only ever occupy the single - * path segment it was written into. Encoding is the structural defence: parameter - * level format validation is applied separately in the tool schemas for better - * error messages, but this layer is what holds for parameters nobody remembered to - * validate. + * Request paths are handed to `fetch`, which resolves dot segments per the WHATWG + * URL spec before the request leaves the process. Interpolating a caller supplied + * value straight into a path therefore lets that value rewrite the path above the + * organization and project the server injected, reaching endpoints the session was + * never scoped to. + * + * `apiPath` closes that in two ways, because encoding alone is not sufficient: + * + * 1. Separators are percent-encoded, so a value cannot introduce new path segments. + * 2. A value that is *entirely* a dot segment is rejected. `encodeURIComponent` + * leaves `.` and `..` untouched, and the URL spec also treats the percent-encoded + * forms (`%2e`, `%2e%2e`) as dot segments, so no amount of encoding neutralises + * them. Such a value would still collapse and consume a preceding segment. + * + * Together these mean an interpolated value occupies exactly the one path segment it + * was written into. Parameter level format validation is applied separately in the + * tool schemas for better error messages, but this layer is what holds for + * parameters nobody remembered to validate. * * Query strings must be appended outside the template, because `URLSearchParams` * has already encoded them and the separators must stay literal: * * ```typescript - * const path = `${apiPath`/organizations/${organizationSlug}/issues/`}?${query}`; + * const path = apiPath`/organizations/${organizationSlug}/issues/`; + * const body = await this.requestJSON(`${path}?${query}`); * ``` */ +import { UserInputError } from "../errors"; + +/** + * Matches a segment the URL parser would treat as a single- or double-dot segment. + * + * Per the WHATWG URL spec these comparisons are ASCII case-insensitive and include + * the percent-encoded spellings of `.`, which is why encoding cannot be used to make + * such a value inert. + */ +const DOT_SEGMENT = /^(?:\.|%2e){1,2}$/i; /** * Interpolates values into an API path, percent-encoding each one. * * `encodeURIComponent` leaves `A-Za-z0-9`, `-`, `_`, `.`, `!`, `~`, `*`, `'`, `(` * and `)` untouched, so every legitimate Sentry slug, numeric ID, hex ID, UUID and - * short ID passes through byte for byte. Only separators and traversal sequences - * change, which is exactly the intent. + * short ID passes through byte for byte. Only separators change, which is exactly + * the intent. * * @param strings Literal portions of the template * @param values Caller supplied values to encode into single path segments * @returns The assembled path with every interpolated value encoded + * @throws {UserInputError} If a value is entirely a dot segment, since such a value + * cannot be contained by encoding and no legitimate Sentry identifier takes that + * form. */ export function apiPath( strings: TemplateStringsArray, @@ -41,6 +62,14 @@ export function apiPath( if (index >= values.length) { return acc + literal; } - return acc + literal + encodeURIComponent(String(values[index])); + + const encoded = encodeURIComponent(String(values[index])); + if (DOT_SEGMENT.test(encoded)) { + throw new UserInputError( + `Invalid identifier "${values[index]}": relative path segments are not allowed.`, + ); + } + + return acc + literal + encoded; }, ""); } diff --git a/packages/mcp-core/src/api-client/client.ts b/packages/mcp-core/src/api-client/client.ts index 9bbfa66ab..51c6e0c27 100644 --- a/packages/mcp-core/src/api-client/client.ts +++ b/packages/mcp-core/src/api-client/client.ts @@ -1531,7 +1531,7 @@ export class SentryApiService { queryParams.set("query", params.query); } const queryString = queryParams.toString(); - const path = apiPath`/organizations/` + `?${queryString}`; + const path = `/organizations/?${queryString}`; // For self-hosted instances, the regions endpoint doesn't exist if (!this.isSaas()) { @@ -1616,8 +1616,8 @@ export class SentryApiService { queryParams.set("query", params.query); } const queryString = queryParams.toString(); - const path = - apiPath`/organizations/${organizationSlug}/teams/` + `?${queryString}`; + const teamsPath = apiPath`/organizations/${organizationSlug}/teams/`; + const path = `${teamsPath}?${queryString}`; const body = await this.requestJSON(path, undefined, opts); return TeamListSchema.parse(body); @@ -1675,8 +1675,8 @@ export class SentryApiService { queryParams.set("query", params.query); } const queryString = queryParams.toString(); - const path = - apiPath`/organizations/${organizationSlug}/projects/` + `?${queryString}`; + const projectsPath = apiPath`/organizations/${organizationSlug}/projects/`; + const path = `${projectsPath}?${queryString}`; const body = await this.requestJSON(path, undefined, opts); return ProjectListSchema.parse(body); diff --git a/packages/mcp-core/src/tools/catalog/path-traversal.test.ts b/packages/mcp-core/src/tools/catalog/path-traversal.test.ts index 75f988d95..f1375855b 100644 --- a/packages/mcp-core/src/tools/catalog/path-traversal.test.ts +++ b/packages/mcp-core/src/tools/catalog/path-traversal.test.ts @@ -17,6 +17,7 @@ import { mswServer } from "@sentry/mcp-server-mocks"; import { z } from "zod"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { SentryApiService } from "../../api-client"; +import { UserInputError } from "../../errors"; import { getFilteredInputSchema, injectConstraintParams, @@ -37,11 +38,14 @@ const VICTIM_ORG = "bbtest-victimorg"; const VICTIM_PROJECT = "victim-proj"; /** - * The exact payload shapes from the reports, plus the bypass variants VULN-2450 - * asked us to retest because encode-only fixes for this class have historically - * been circumvented. + * Payloads that encoding renders inert, so the request is still issued but stays + * within the constrained org and project. + * + * These are the exact shapes from the reports, plus the bypass variants VULN-2450 + * asked us to retest because encode-only fixes for this class have historically been + * circumvented. */ -const TRAVERSAL_PAYLOADS = [ +const CONTAINED_PAYLOADS = [ ["cross-project", `../../${VICTIM_PROJECT}/events/abc123`], ["cross-org", `../../../${VICTIM_ORG}/${VICTIM_PROJECT}/events/abc123`], [ @@ -56,6 +60,24 @@ const TRAVERSAL_PAYLOADS = [ ["null byte", `abc\u0000/../${VICTIM_ORG}`], ["fragment truncation", "abc/#"], ["query injection", "abc/?download=1"], + // Inert once encoded, because the `%` is itself encoded. + ["bare encoded double dot", "%2e%2e"], +] as const; + +/** + * Payloads that are entirely a dot segment. These carry no separator, so encoding + * leaves them intact and the URL parser still collapses them. They cannot name + * another tenant, but they do consume a preceding segment and move the request to a + * different endpoint, so `apiPath` rejects them outright and no request is issued. + */ +const REJECTED_PAYLOADS = [ + ["bare double dot", ".."], + ["bare single dot", "."], +] as const; + +const TRAVERSAL_PAYLOADS = [ + ...CONTAINED_PAYLOADS, + ...REJECTED_PAYLOADS, ] as const; let requestedUrls: string[] = []; @@ -80,7 +102,9 @@ afterEach(() => { * either validation rejected the input or the request stayed in scope. */ async function callToolAsClient( - tool: ToolConfig, + // Matches getFilteredInputSchema and injectConstraintParams, which take + // ToolConfig so they can accept tools with heterogeneous schemas. + tool: ToolConfig, clientParams: Record, context: ServerContext, ): Promise<{ rejected: boolean }> { @@ -268,62 +292,56 @@ describe("path traversal containment", () => { * * This block drives the client directly with the same payloads, standing in for a * parameter that a future tool forgets to validate. It is the actual evidence that - * encoding is load-bearing on its own, which is the reason this class of bug should - * not recur the way it did after the slug-only fix in VULN-848. + * the API client layer is load-bearing on its own, which is the reason this class of + * bug should not recur the way it did after the slug-only fix in VULN-848. */ describe("the API client contains traversal without any validation layer", () => { const apiService = () => new SentryApiService({ accessToken: "access-token", host: "sentry.io" }); - it.each(TRAVERSAL_PAYLOADS)( - "listEventAttachments eventId: %s", - async (_label, payload) => { - await apiService() - .listEventAttachments({ - organizationSlug: SCOPED_ORG, - projectSlug: SCOPED_PROJECT, - eventId: payload, - }) - .catch(() => {}); - - expect(requestedUrls).toHaveLength(1); - expectAllRequestsInScope(); - }, - ); - - it.each(TRAVERSAL_PAYLOADS)( - "getTransactionProfile profileId: %s", - async (_label, payload) => { - await apiService() - .getTransactionProfile({ - organizationSlug: SCOPED_ORG, - projectSlugOrId: SCOPED_PROJECT, - profileId: payload, - }) - .catch(() => {}); - - expect(requestedUrls).toHaveLength(1); - expectAllRequestsInScope(); - }, - ); + const sinks = { + listEventAttachments: (eventId: string) => + apiService().listEventAttachments({ + organizationSlug: SCOPED_ORG, + projectSlug: SCOPED_PROJECT, + eventId, + }), + getTransactionProfile: (profileId: string) => + apiService().getTransactionProfile({ + organizationSlug: SCOPED_ORG, + projectSlugOrId: SCOPED_PROJECT, + profileId, + }), + updateClientKey: (keyId: string) => + apiService().updateClientKey({ + organizationSlug: SCOPED_ORG, + projectSlug: SCOPED_PROJECT, + keyId, + name: "XORG-PWN", + isActive: false, + }), + }; + + describe.each(Object.entries(sinks))("%s", (_name, call) => { + it.each(CONTAINED_PAYLOADS)( + "issues one in-scope request for %s", + async (_label, payload) => { + await call(payload).catch(() => {}); + + expect(requestedUrls).toHaveLength(1); + expectAllRequestsInScope(); + }, + ); - it.each(TRAVERSAL_PAYLOADS)( - "updateClientKey keyId: %s", - async (_label, payload) => { - await apiService() - .updateClientKey({ - organizationSlug: SCOPED_ORG, - projectSlug: SCOPED_PROJECT, - keyId: payload, - name: "XORG-PWN", - isActive: false, - }) - .catch(() => {}); + it.each(REJECTED_PAYLOADS)( + "issues no request at all for %s", + async (_label, payload) => { + await expect(call(payload)).rejects.toThrow(UserInputError); - expect(requestedUrls).toHaveLength(1); - expectAllRequestsInScope(); - }, - ); + expect(requestedUrls).toEqual([]); + }, + ); + }); }); describe("constraint keys remain unreachable by the client", () => { From a463489373619de766f2c583779eaa60fc89f897 Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Fri, 31 Jul 2026 09:24:12 -0400 Subject: [PATCH 3/3] slim down tests fix it: --- .gitignore | 3 + .../mcp-core/src/api-client/api-path.test.ts | 174 +++----- packages/mcp-core/src/api-client/api-path.ts | 66 +-- .../src/tools/catalog/path-traversal.test.ts | 386 +++++------------- 4 files changed, 181 insertions(+), 448 deletions(-) diff --git a/.gitignore b/.gitignore index a8fb8ea70..278906b92 100644 --- a/.gitignore +++ b/.gitignore @@ -75,3 +75,6 @@ packages/mcp-server-evals/eval-results.json # Auto-generated by dotagents — do not commit these files. .agents/.gitignore .warden/ + +# Local investigation notes, never commit (may contain reporter PII) +summary.md diff --git a/packages/mcp-core/src/api-client/api-path.test.ts b/packages/mcp-core/src/api-client/api-path.test.ts index 0e4ed404b..1266b2787 100644 --- a/packages/mcp-core/src/api-client/api-path.test.ts +++ b/packages/mcp-core/src/api-client/api-path.test.ts @@ -4,9 +4,9 @@ import { UserInputError } from "../errors"; import { apiPath } from "./api-path"; /** - * The assertions below resolve each path the way `fetch` does, because dot-segment - * normalization happens during URL parsing rather than in our code. A test that only - * inspected the assembled string would pass even if traversal still worked. + * Resolves a path the way `fetch` does. Dot-segment normalization happens during URL + * parsing rather than in our code, so a test that only inspected the assembled string + * would pass even if traversal still worked. */ function resolve(path: string): string { const url = new URL(`https://sentry.io/api/0${path}`); @@ -15,7 +15,7 @@ function resolve(path: string): string { describe("apiPath", () => { it("leaves legitimate identifier formats byte for byte", () => { - const cases = [ + for (const value of [ "my-org", "my_project.name", "1234567890", @@ -23,104 +23,49 @@ describe("apiPath", () => { "7e07485f-12f9-416b-8b14-26260799b51f", "PROJECT-1Z43", "latest", - ]; - - for (const value of cases) { + ]) { expect(apiPath`/projects/${value}/`).toBe(`/projects/${value}/`); } }); - it("accepts numeric values", () => { - expect(apiPath`/projects/${4511636893859840}/`).toBe( - "/projects/4511636893859840/", - ); - }); - - it("handles templates with no interpolation", () => { - expect(apiPath`/organizations/`).toBe("/organizations/"); - }); + it("resolves an unencoded payload out of scope, proving these tests are meaningful", () => { + const attack = "../../../victim-org/victim-proj/events/abc"; - it("handles adjacent interpolations", () => { - expect(apiPath`/${"a"}${"b"}/`).toBe("/ab/"); + expect(resolve(`/projects/my-org/my-proj/events/${attack}/`)).toBe( + "/api/0/projects/victim-org/victim-proj/events/abc/", + ); }); - describe("traversal containment", () => { - it("keeps a traversal payload inside its own segment", () => { - const attack = "../../../victim-org/victim-proj/events/abc"; - const path = apiPath`/projects/my-org/my-proj/events/${attack}/attachments/`; - - expect(resolve(path)).toBe( - "/api/0/projects/my-org/my-proj/events/..%2F..%2F..%2Fvictim-org%2Fvictim-proj%2Fevents%2Fabc/attachments/", - ); - }); - - it("resolves the unencoded equivalent out of scope, proving the test is meaningful", () => { - const attack = "../../../victim-org/victim-proj/events/abc"; - const unsafe = `/projects/my-org/my-proj/events/${attack}/attachments/`; - - expect(resolve(unsafe)).toBe( - "/api/0/projects/victim-org/victim-proj/events/abc/attachments/", - ); - }); - - /** - * VULN-2450 specifically asked that a patch be retested against these variants, - * since encode-only fixes for this bug class have historically been bypassable. - * - * Each payload must survive URL resolution as exactly one inert path segment. - * Asserting the whole resolved path (rather than searching for the payload text, - * which is still present once encoded) is what proves no segment boundary, - * query separator or fragment escaped. - */ - it.each([ - ["plain dot segments", "../../other-org/other-proj"], - ["double encoded dots", "%252e%252e%252fother-org"], - ["single encoded dots", "%2e%2e%2fother-org"], - ["recursive sequences", "....//....//other-org"], - ["backslash separators", "..\\..\\other-org"], - ["mixed separators", "..\\../other-org"], - ["null byte", "abc\u0000/../other-org"], - ["fragment truncation", "abc/#"], - ["query injection", "abc/?download=1&x=2"], - ["scheme relative", "//evil.example.com/x"], - ["absolute path", "/organizations/other-org/members"], - ["trailing dot segment", "abc/.."], - ["dots with a suffix", "..a"], - ["dots with a prefix", "a.."], - ["three dots", "..."], - ])("contains %s", (_label, attack) => { - const resolved = resolve( - apiPath`/projects/my-org/my-proj/events/${attack}/`, - ); - - expect(resolved).toBe( - `/api/0/projects/my-org/my-proj/events/${encodeURIComponent(attack)}/`, - ); - }); - - it("cannot redirect the request to another host", () => { - const attack = "//evil.example.com/collect"; - const url = new URL( - `https://sentry.io/api/0${apiPath`/projects/o/p/events/${attack}/`}`, - ); - - expect(url.host).toBe("sentry.io"); - }); - - it("cannot append query parameters to the request", () => { - const url = new URL( - `https://sentry.io/api/0${apiPath`/projects/o/p/events/${"abc/?download=1"}/`}`, - ); - - expect(url.search).toBe(""); - }); + /** + * VULN-2450 asked that any patch be retested against these variants, since + * encode-only fixes for this class have historically been bypassable. + * + * Asserting the whole resolved path is what proves containment. Searching for the + * payload text would not, since an encoded payload still contains it as literal text. + */ + it.each([ + ["plain dot segments", "../../other-org"], + ["double encoded dots", "%252e%252e%252fother-org"], + ["single encoded dots", "%2e%2e%2fother-org"], + ["recursive sequences", "....//....//other-org"], + ["backslash separators", "..\\..\\other-org"], + ["null byte", "abc\u0000/../other-org"], + ["fragment truncation", "abc/#"], + ["query injection", "abc/?download=1&x=2"], + ["scheme relative", "//evil.example.com/x"], + ["absolute path", "/organizations/other-org/members"], + ["trailing dot segment", "abc/.."], + ["dots with a suffix", "..a"], + ])("contains %s in a single segment", (_label, attack) => { + expect(resolve(apiPath`/projects/my-org/my-proj/events/${attack}/`)).toBe( + `/api/0/projects/my-org/my-proj/events/${encodeURIComponent(attack)}/`, + ); }); /** - * A value that is entirely a dot segment carries no separator, so encoding leaves - * it intact and the URL parser still collapses it. It cannot reach another tenant - * (that needs a `/` to name one) but it does consume a preceding segment and move - * the request to a different endpoint, so it has to be rejected rather than encoded. + * A bare dot segment carries no separator, so encoding leaves it intact and the URL + * parser still collapses it. It cannot name another tenant, but it does consume a + * preceding segment, so it has to be rejected rather than encoded. */ describe("bare dot segments", () => { it.each([".", ".."])("rejects %s", (attack) => { @@ -130,32 +75,16 @@ describe("apiPath", () => { }); it("would otherwise consume the preceding segment", () => { - // Demonstrates why encoding cannot solve this: the encoded form still collapses. - expect(resolve(`/projects/o/p/keys/${encodeURIComponent("..")}/`)).toBe( - "/api/0/projects/o/p/", - ); + expect(resolve("/projects/o/p/keys/../")).toBe("/api/0/projects/o/p/"); expect(resolve("/projects/o/p/keys/%2e%2e/")).toBe( "/api/0/projects/o/p/", ); }); - /** - * A percent-encoded spelling arriving as *input* is already inert, because the `%` - * itself gets encoded (`%2e%2e` becomes `%252e%252e`). These must be contained - * rather than rejected, so that a legitimate value is never refused. - */ - it.each(["%2e", "%2E", "%2e%2e", ".%2e", "%2e."])( - "contains rather than rejects %s", - (value) => { - expect(() => apiPath`/projects/o/p/keys/${value}/`).not.toThrow(); - expect(resolve(apiPath`/projects/o/p/keys/${value}/`)).toBe( - `/api/0/projects/o/p/keys/${encodeURIComponent(value)}/`, - ); - }, - ); - - it("still accepts identifiers that merely contain dots", () => { - for (const value of ["1.2.3", "my.project", "...", "..a", "a.."]) { + it("accepts values that merely contain dots, including encoded spellings", () => { + // `%2e%2e` as input is inert once the `%` is itself encoded, so rejecting it + // would refuse a value that is already safe. + for (const value of ["1.2.3", "my.project", "...", "%2e%2e"]) { expect(() => apiPath`/projects/o/p/keys/${value}/`).not.toThrow(); } }); @@ -164,21 +93,19 @@ describe("apiPath", () => { /** * The slug-only fix in VULN-848 hardened the parameters that were vulnerable at the - * time and left the resource IDs raw, which is how VULN-2159/2450 happened years + * time and left the resource IDs raw, which is how VULN-2159/2450 happened ten months * later. Enumerating parameters does not hold; enforcing the invariant does. * - * This fails on any newly added request path that interpolates a value without the - * `apiPath` tag, so the class cannot be reintroduced by a tool author who is unaware - * of it. + * This fails on any new request path that interpolates a value without the `apiPath` + * tag, so the class cannot be reintroduced by a tool author unaware of it. */ describe("client.ts request path invariant", () => { const source = readFileSync(new URL("./client.ts", import.meta.url), "utf8"); /** * Returns the path portion of a template literal body, stopping at the query - * separator. Only interpolation before the `?` needs the `apiPath` tag; query - * values are already encoded by `URLSearchParams` and must keep their separators - * literal. + * separator. Only interpolation before the `?` needs the tag; query values are + * already encoded by `URLSearchParams` and must keep their separators literal. */ function pathPortion(body: string): string { let depth = 0; @@ -208,10 +135,11 @@ describe("client.ts request path invariant", () => { const closing = line.indexOf("`", i + 1); const body = line.slice(i + 1, closing === -1 ? undefined : closing); - const isTagged = line.slice(0, i).endsWith("apiPath"); - const interpolatesPath = /\$\{/.test(pathPortion(body)); - if (!isTagged && interpolatesPath) { + if ( + !line.slice(0, i).endsWith("apiPath") && + /\$\{/.test(pathPortion(body)) + ) { offenders.push(`${index + 1}: ${trimmed}`); } } diff --git a/packages/mcp-core/src/api-client/api-path.ts b/packages/mcp-core/src/api-client/api-path.ts index 1f47101d4..2b7aff51b 100644 --- a/packages/mcp-core/src/api-client/api-path.ts +++ b/packages/mcp-core/src/api-client/api-path.ts @@ -1,59 +1,21 @@ +import { UserInputError } from "../errors"; + /** - * Safe construction of Sentry API request paths. - * - * Request paths are handed to `fetch`, which resolves dot segments per the WHATWG - * URL spec before the request leaves the process. Interpolating a caller supplied - * value straight into a path therefore lets that value rewrite the path above the - * organization and project the server injected, reaching endpoints the session was - * never scoped to. - * - * `apiPath` closes that in two ways, because encoding alone is not sufficient: + * Builds an API request path, confining each interpolated value to one path segment. * - * 1. Separators are percent-encoded, so a value cannot introduce new path segments. - * 2. A value that is *entirely* a dot segment is rejected. `encodeURIComponent` - * leaves `.` and `..` untouched, and the URL spec also treats the percent-encoded - * forms (`%2e`, `%2e%2e`) as dot segments, so no amount of encoding neutralises - * them. Such a value would still collapse and consume a preceding segment. + * `fetch` resolves dot segments during URL parsing, so a raw value in a path can + * rewrite it above the org and project the server injected. Encoding stops separators + * but not `.` or `..`, which survive encoding and still collapse (the URL spec treats + * `%2e%2e` as a dot segment too), so those are rejected instead. * - * Together these mean an interpolated value occupies exactly the one path segment it - * was written into. Parameter level format validation is applied separately in the - * tool schemas for better error messages, but this layer is what holds for - * parameters nobody remembered to validate. - * - * Query strings must be appended outside the template, because `URLSearchParams` - * has already encoded them and the separators must stay literal: + * Query strings go outside the template, since `URLSearchParams` has already encoded + * them and their separators must stay literal: * * ```typescript * const path = apiPath`/organizations/${organizationSlug}/issues/`; - * const body = await this.requestJSON(`${path}?${query}`); + * await this.requestJSON(`${path}?${query}`); * ``` */ -import { UserInputError } from "../errors"; - -/** - * Matches a segment the URL parser would treat as a single- or double-dot segment. - * - * Per the WHATWG URL spec these comparisons are ASCII case-insensitive and include - * the percent-encoded spellings of `.`, which is why encoding cannot be used to make - * such a value inert. - */ -const DOT_SEGMENT = /^(?:\.|%2e){1,2}$/i; - -/** - * Interpolates values into an API path, percent-encoding each one. - * - * `encodeURIComponent` leaves `A-Za-z0-9`, `-`, `_`, `.`, `!`, `~`, `*`, `'`, `(` - * and `)` untouched, so every legitimate Sentry slug, numeric ID, hex ID, UUID and - * short ID passes through byte for byte. Only separators change, which is exactly - * the intent. - * - * @param strings Literal portions of the template - * @param values Caller supplied values to encode into single path segments - * @returns The assembled path with every interpolated value encoded - * @throws {UserInputError} If a value is entirely a dot segment, since such a value - * cannot be contained by encoding and no legitimate Sentry identifier takes that - * form. - */ export function apiPath( strings: TemplateStringsArray, ...values: (string | number)[] @@ -63,13 +25,13 @@ export function apiPath( return acc + literal; } - const encoded = encodeURIComponent(String(values[index])); - if (DOT_SEGMENT.test(encoded)) { + const value = String(values[index]); + if (value === "." || value === "..") { throw new UserInputError( - `Invalid identifier "${values[index]}": relative path segments are not allowed.`, + `Invalid identifier "${value}": relative path segments are not allowed.`, ); } - return acc + literal + encoded; + return acc + literal + encodeURIComponent(value); }, ""); } diff --git a/packages/mcp-core/src/tools/catalog/path-traversal.test.ts b/packages/mcp-core/src/tools/catalog/path-traversal.test.ts index f1375855b..8c4c68e6b 100644 --- a/packages/mcp-core/src/tools/catalog/path-traversal.test.ts +++ b/packages/mcp-core/src/tools/catalog/path-traversal.test.ts @@ -7,27 +7,30 @@ * rewrite the path above the organization and project the server injects from the * session constraints, reaching other tenants with the granting user's credential. * - * These tests drive tools through the same pipeline the server uses: the constrained - * keys are stripped from the schema the client sees, the remaining params are parsed - * with Zod, and the constraints are then injected server-side. Asserting on the - * outbound request URL is what makes these meaningful, since the defect was only ever - * observable in the path that actually left the process. + * These drive tools through the same pipeline the server uses (constrained keys + * stripped from the client-visible schema, Zod parse, constraints injected) and assert + * on the outbound request URL, since the defect was only ever observable in the path + * that actually left the process. + * + * The full bypass matrix lives in `api-client/api-path.test.ts`, against the function + * that does the containment. These tests cover the wiring: that each reported sink + * reaches that function. */ import { mswServer } from "@sentry/mcp-server-mocks"; -import { z } from "zod"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { z } from "zod"; import { SentryApiService } from "../../api-client"; import { UserInputError } from "../../errors"; +import { getServerContext } from "../../test-setup.js"; +import type { ServerContext } from "../../types"; import { getFilteredInputSchema, injectConstraintParams, } from "../catalog-runtime/availability"; import type { ToolConfig } from "../types"; -import type { ServerContext } from "../../types"; -import { getServerContext } from "../../test-setup.js"; import getEventAttachment from "./get-event-attachment"; -import getIssueDetails from "./get-issue-details"; import getEventStacktrace from "./get-event-stacktrace"; +import getIssueDetails from "./get-issue-details"; import getProfileDetails from "./get-profile-details"; import getReplayDetails from "./get-replay-details"; import updateDsn from "./update-dsn"; @@ -37,48 +40,8 @@ const SCOPED_PROJECT = "cloudflare-mcp"; const VICTIM_ORG = "bbtest-victimorg"; const VICTIM_PROJECT = "victim-proj"; -/** - * Payloads that encoding renders inert, so the request is still issued but stays - * within the constrained org and project. - * - * These are the exact shapes from the reports, plus the bypass variants VULN-2450 - * asked us to retest because encode-only fixes for this class have historically been - * circumvented. - */ -const CONTAINED_PAYLOADS = [ - ["cross-project", `../../${VICTIM_PROJECT}/events/abc123`], - ["cross-org", `../../../${VICTIM_ORG}/${VICTIM_PROJECT}/events/abc123`], - [ - "deep cross-org", - `../../../../../organizations/${VICTIM_ORG}/issues/130651143/events/latest`, - ], - ["escape api root", "../../../../../../../auth/config"], - ["double encoded", `%252e%252e%252f%252e%252e%252f${VICTIM_ORG}`], - ["single encoded", `%2e%2e%2f%2e%2e%2f${VICTIM_ORG}`], - ["recursive", `....//....//${VICTIM_ORG}`], - ["backslash", `..\\..\\${VICTIM_ORG}`], - ["null byte", `abc\u0000/../${VICTIM_ORG}`], - ["fragment truncation", "abc/#"], - ["query injection", "abc/?download=1"], - // Inert once encoded, because the `%` is itself encoded. - ["bare encoded double dot", "%2e%2e"], -] as const; - -/** - * Payloads that are entirely a dot segment. These carry no separator, so encoding - * leaves them intact and the URL parser still collapses them. They cannot name - * another tenant, but they do consume a preceding segment and move the request to a - * different endpoint, so `apiPath` rejects them outright and no request is issued. - */ -const REJECTED_PAYLOADS = [ - ["bare double dot", ".."], - ["bare single dot", "."], -] as const; - -const TRAVERSAL_PAYLOADS = [ - ...CONTAINED_PAYLOADS, - ...REJECTED_PAYLOADS, -] as const; +/** The payload from VULN-2450 step 3, which reached another organization. */ +const CROSS_ORG = `../../../${VICTIM_ORG}/${VICTIM_PROJECT}/events/abc123`; let requestedUrls: string[] = []; @@ -96,42 +59,30 @@ afterEach(() => { }); /** - * Mirrors how the server handles a tool call: constrained keys are absent from the - * schema exposed to the client, the rest is Zod-validated, then constraints are - * injected. Returns the Zod error instead of throwing so callers can assert that - * either validation rejected the input or the request stayed in scope. + * Mirrors how the server handles a tool call. Swallows handler errors because an + * upstream 404 is irrelevant here; the assertion that matters is which URL was + * requested, which is captured either way. */ async function callToolAsClient( - // Matches getFilteredInputSchema and injectConstraintParams, which take - // ToolConfig so they can accept tools with heterogeneous schemas. + // getFilteredInputSchema and injectConstraintParams take ToolConfig so they + // can accept tools with heterogeneous schemas. tool: ToolConfig, clientParams: Record, context: ServerContext, -): Promise<{ rejected: boolean }> { - const schema = z.object(getFilteredInputSchema(tool, context)); - const parsed = schema.safeParse(clientParams); - if (!parsed.success) { - return { rejected: true }; - } - - try { - await tool.handler( - injectConstraintParams(parsed.data, tool, context), - context, - ); - } catch { - // An upstream 404 or a formatting failure is irrelevant here. The assertion - // that matters is which URL was requested, which is captured either way. - } - return { rejected: false }; +): Promise { + const parsed = z + .object(getFilteredInputSchema(tool, context)) + .safeParse(clientParams); + if (!parsed.success) return; + + await tool + .handler(injectConstraintParams(parsed.data, tool, context), context) + .catch(() => {}); } /** - * Every upstream request must stay within the constrained org and project. - * - * Asserts on decoded path *segments* rather than substrings: an encoded payload - * legitimately still contains the victim slug as literal text, so a substring check - * would flag a contained payload as an escape. + * Asserts on decoded path *segments* rather than substrings, since an encoded payload + * legitimately still contains the victim slug as literal text. */ function expectAllRequestsInScope() { for (const url of requestedUrls) { @@ -157,214 +108,103 @@ describe("path traversal containment", () => { }, }); - describe("a project-scoped session cannot escape its constraint", () => { - it.each(TRAVERSAL_PAYLOADS)( - "get_event_attachment eventId: %s", - async (_label, payload) => { - await callToolAsClient( - getEventAttachment, - { eventId: payload }, - scopedContext(), - ); - expectAllRequestsInScope(); - }, - ); + /** Every ID parameter named across the four reports. */ + const sinks: [ + string, + ToolConfig, + (payload: string) => Record, + ][] = [ + [ + "get_event_attachment eventId", + getEventAttachment, + (p) => ({ eventId: p }), + ], + [ + "get_event_attachment attachmentId", + getEventAttachment, + (p) => ({ + eventId: "99be789ee5555abf1ad81bd47c2c2e36", + attachmentId: p, + }), + ], + [ + "get_issue_details eventId", + getIssueDetails, + (p) => ({ issueId: "CLOUDFLARE-MCP-41", eventId: p }), + ], + [ + "get_event_stacktrace eventId", + getEventStacktrace, + (p) => ({ issueId: "CLOUDFLARE-MCP-41", eventId: p }), + ], + ["get_replay_details replayId", getReplayDetails, (p) => ({ replayId: p })], + [ + "get_profile_details profileId", + getProfileDetails, + (p) => ({ profileId: p }), + ], + // The write half of VULN-2450: a confined token renamed and disabled DSNs in + // another organization. + [ + "update_dsn keyId", + updateDsn, + (p) => ({ keyId: p, name: "XORG-PWN", isActive: false }), + ], + ]; + + it.each(sinks)( + "%s cannot escape the scoped project", + async (_label, tool, params) => { + await callToolAsClient(tool, params(CROSS_ORG), scopedContext()); + expectAllRequestsInScope(); + }, + ); - it.each(TRAVERSAL_PAYLOADS)( - "get_event_attachment attachmentId: %s", - async (_label, payload) => { - await callToolAsClient( - getEventAttachment, - { - eventId: "99be789ee5555abf1ad81bd47c2c2e36", - attachmentId: payload, - }, - scopedContext(), - ); - expectAllRequestsInScope(); - }, - ); - - it.each(TRAVERSAL_PAYLOADS)( - "get_issue_details eventId: %s", - async (_label, payload) => { - await callToolAsClient( - getIssueDetails, - { issueId: "CLOUDFLARE-MCP-41", eventId: payload }, - scopedContext(), - ); - expectAllRequestsInScope(); - }, - ); - - it.each(TRAVERSAL_PAYLOADS)( - "get_event_stacktrace eventId: %s", - async (_label, payload) => { - await callToolAsClient( - getEventStacktrace, - { issueId: "CLOUDFLARE-MCP-41", eventId: payload }, - scopedContext(), - ); - expectAllRequestsInScope(); - }, - ); - - it.each(TRAVERSAL_PAYLOADS)( - "get_replay_details replayId: %s", - async (_label, payload) => { - await callToolAsClient( - getReplayDetails, - { replayId: payload }, - scopedContext(), - ); - expectAllRequestsInScope(); - }, - ); - - it.each(TRAVERSAL_PAYLOADS)( - "get_profile_details profileId: %s", - async (_label, payload) => { - await callToolAsClient( - getProfileDetails, - { profileId: payload }, - scopedContext(), - ); - expectAllRequestsInScope(); - }, - ); - - /** - * The write half of VULN-2450: a confined token renamed and disabled DSNs in - * another organization. This is the highest-impact sink in the class. - */ - it.each(TRAVERSAL_PAYLOADS)( - "update_dsn keyId: %s", - async (_label, payload) => { - await callToolAsClient( - updateDsn, - { keyId: payload, name: "XORG-PWN", isActive: false }, - scopedContext(), - ); - expectAllRequestsInScope(); - }, - ); - }); - - describe("an org-only session cannot escape its organization", () => { - const orgOnlyContext = () => + /** + * VULN-2450 emphasised org-only sessions, where the project-slug ownership guards + * in project-constraints.ts early-return and the path encoding is all that remains. + */ + it("cannot escape an org-only session", async () => { + await callToolAsClient( + updateDsn, + { projectSlug: SCOPED_PROJECT, keyId: CROSS_ORG, name: "XORG-PWN" }, getServerContext({ constraints: { organizationSlug: SCOPED_ORG, projectSlug: null }, - }); - - it.each(TRAVERSAL_PAYLOADS)( - "get_event_attachment eventId: %s", - async (_label, payload) => { - await callToolAsClient( - getEventAttachment, - { projectSlug: SCOPED_PROJECT, eventId: payload }, - orgOnlyContext(), - ); - expectAllRequestsInScope(); - }, + }), ); - it.each(TRAVERSAL_PAYLOADS)( - "update_dsn keyId: %s", - async (_label, payload) => { - await callToolAsClient( - updateDsn, - { - projectSlug: SCOPED_PROJECT, - keyId: payload, - name: "XORG-PWN", - isActive: false, - }, - orgOnlyContext(), - ); - expectAllRequestsInScope(); - }, - ); + expectAllRequestsInScope(); }); /** - * The schema refinements reject every payload above, so the tool-level tests never - * reach the API client. That makes them a test of layer 2 only. - * - * This block drives the client directly with the same payloads, standing in for a - * parameter that a future tool forgets to validate. It is the actual evidence that - * the API client layer is load-bearing on its own, which is the reason this class of - * bug should not recur the way it did after the slug-only fix in VULN-848. + * The schema refinements reject the payloads above, so the tests above never reach + * the API client. This block drives the client directly, standing in for a parameter + * that a future tool forgets to validate. It is the evidence that the client layer + * holds on its own, which is why this class recurred after the slug-only VULN-848 fix. */ - describe("the API client contains traversal without any validation layer", () => { - const apiService = () => - new SentryApiService({ accessToken: "access-token", host: "sentry.io" }); - - const sinks = { - listEventAttachments: (eventId: string) => - apiService().listEventAttachments({ - organizationSlug: SCOPED_ORG, - projectSlug: SCOPED_PROJECT, - eventId, - }), - getTransactionProfile: (profileId: string) => - apiService().getTransactionProfile({ - organizationSlug: SCOPED_ORG, - projectSlugOrId: SCOPED_PROJECT, - profileId, - }), - updateClientKey: (keyId: string) => - apiService().updateClientKey({ - organizationSlug: SCOPED_ORG, - projectSlug: SCOPED_PROJECT, - keyId, - name: "XORG-PWN", - isActive: false, - }), - }; - - describe.each(Object.entries(sinks))("%s", (_name, call) => { - it.each(CONTAINED_PAYLOADS)( - "issues one in-scope request for %s", - async (_label, payload) => { - await call(payload).catch(() => {}); - - expect(requestedUrls).toHaveLength(1); - expectAllRequestsInScope(); - }, - ); - - it.each(REJECTED_PAYLOADS)( - "issues no request at all for %s", - async (_label, payload) => { - await expect(call(payload)).rejects.toThrow(UserInputError); - - expect(requestedUrls).toEqual([]); - }, - ); - }); - }); + describe("the API client contains traversal with no validation layer", () => { + const updateKey = (keyId: string) => + new SentryApiService({ + accessToken: "access-token", + host: "sentry.io", + }).updateClientKey({ + organizationSlug: SCOPED_ORG, + projectSlug: SCOPED_PROJECT, + keyId, + name: "XORG-PWN", + }); - describe("constraint keys remain unreachable by the client", () => { - it("strips organizationSlug and projectSlug from the exposed schema", () => { - const schema = getFilteredInputSchema( - getEventAttachment, - scopedContext(), - ); + it("issues one in-scope request for a traversal payload", async () => { + await updateKey(CROSS_ORG).catch(() => {}); - expect(Object.keys(schema)).not.toContain("organizationSlug"); - expect(Object.keys(schema)).not.toContain("projectSlug"); - expect(Object.keys(schema)).toContain("eventId"); + expect(requestedUrls).toHaveLength(1); + expectAllRequestsInScope(); }); - it("ignores a client attempt to override the injected constraints", () => { - const injected = injectConstraintParams( - { organizationSlug: VICTIM_ORG, projectSlug: VICTIM_PROJECT }, - getEventAttachment, - scopedContext(), - ); + it("issues no request at all for a bare dot segment", async () => { + await expect(updateKey("..")).rejects.toThrow(UserInputError); - expect(injected.organizationSlug).toBe(SCOPED_ORG); - expect(injected.projectSlug).toBe(SCOPED_PROJECT); + expect(requestedUrls).toEqual([]); }); }); });