diff --git a/docs/architecture/code-mode.md b/docs/architecture/code-mode.md index 220a74abf..0f8368cf8 100644 --- a/docs/architecture/code-mode.md +++ b/docs/architecture/code-mode.md @@ -90,8 +90,10 @@ Before each `sentry.request()`, the broker (`broker.ts`) checks: Then its `ApiAuthorization` (`authorization.ts`, `access.ts`) checks: -3. The path and method are a published operation. HEAD may use a published - GET. Traversal segments, unknown paths and ambiguous paths fail. +3. The path and method are a published operation, matched with the template + matcher shared with the CLI. HEAD may use a published GET, and a missing + trailing slash is added to the path sent. Traversal segments, unknown paths + and ambiguous paths fail. 4. On `/mcp/{org}`, the organization path parameter must be that organization. Other organizations, numeric aliases and global object-ID routes fail; query parameters cannot stand in for a scoped path. diff --git a/packages/cli/src/lib/idempotent-operations.ts b/packages/cli/src/lib/idempotent-operations.ts index 31df37154..fa9460109 100644 --- a/packages/cli/src/lib/idempotent-operations.ts +++ b/packages/cli/src/lib/idempotent-operations.ts @@ -7,6 +7,7 @@ * so the CLI does not hard-code which assemble calls may retry. */ +import { matchApiPathTemplate } from "@sentry/toolkit-core/api-path-template"; import generated from "../generated/idempotent-operations.json"; /** A non-safe operation Sentry marks idempotent. */ @@ -17,24 +18,7 @@ export type IdempotentOperation = { path: string; }; -type Matcher = { method: string; pattern: RegExp }; - -const TEMPLATE_SEGMENT = /^\{[^/]+\}$/; -const REGEXP_SPECIAL = /[.*+?^${}()|[\]\\]/g; - -function toMatcher({ method, path }: IdempotentOperation): Matcher { - const source = path - .split("/") - .map((segment) => - TEMPLATE_SEGMENT.test(segment) - ? "[^/]+" - : segment.replace(REGEXP_SPECIAL, "\\$&"), - ) - .join("/"); - return { method: method.toUpperCase(), pattern: new RegExp(`^${source}$`) }; -} - -let matchers: Matcher[] | undefined; +const operations = generated as IdempotentOperation[]; /** * Whether Sentry marks this request's operation idempotent. @@ -57,9 +41,9 @@ export function isIdempotentOperation(method: string, url: string): boolean { const apiPath = pathname.slice(apiIndex); const normalized = apiPath.endsWith("/") ? apiPath : `${apiPath}/`; const upper = method.toUpperCase(); - - matchers ??= (generated as IdempotentOperation[]).map(toMatcher); - return matchers.some( - (matcher) => matcher.method === upper && matcher.pattern.test(normalized), + return operations.some( + (operation) => + operation.method === upper && + matchApiPathTemplate(operation.path, normalized) !== undefined, ); } diff --git a/packages/mcp-cloudflare/src/code-mode/access.test.ts b/packages/mcp-cloudflare/src/code-mode/access.test.ts index 846315de0..ba275fd52 100644 --- a/packages/mcp-cloudflare/src/code-mode/access.test.ts +++ b/packages/mcp-cloudflare/src/code-mode/access.test.ts @@ -26,7 +26,11 @@ describe("authorizeApiRequest", () => { it("accepts published operations and HEAD on a published GET", () => { expect( request("/api/0/organizations/example/dashboards/?cursor=next", "GET"), - ).toEqual({ pathTemplate: dashboardPath, organizationSlug: "example" }); + ).toEqual({ + pathTemplate: dashboardPath, + pathname: "/api/0/organizations/example/dashboards/", + organizationSlug: "example", + }); expect( request("/api/0/organizations/example/dashboards/", "HEAD").pathTemplate, ).toBe(dashboardPath); @@ -51,6 +55,7 @@ describe("authorizeApiRequest", () => { } expect(request("/api/0/issues/123/", "GET")).toEqual({ pathTemplate: "/api/0/issues/{issue_id}/", + pathname: "/api/0/issues/123/", }); }); @@ -61,8 +66,14 @@ describe("authorizeApiRequest", () => { expect(() => request("/api/0/organizations/example/dashboards/", "DELETE"), ).toThrow("HTTP method"); + }); + + it("adds a missing trailing slash to match and send the canonical path", () => { + expect( + request("/api/0/organizations/example/dashboards", "GET").pathname, + ).toBe("/api/0/organizations/example/dashboards/"); expect(() => - request("/api/0/organizations/example/dashboards", "GET"), + request("/api/0/organizations/example/dashboards//", "GET"), ).toThrow("published API schema"); }); diff --git a/packages/mcp-cloudflare/src/code-mode/access.ts b/packages/mcp-cloudflare/src/code-mode/access.ts index e5d92258f..9f98d3b89 100644 --- a/packages/mcp-cloudflare/src/code-mode/access.ts +++ b/packages/mcp-cloudflare/src/code-mode/access.ts @@ -1,4 +1,8 @@ import type { ApiOperation } from "@sentry/api/operations"; +import { + type ApiPathTemplateMatch, + matchApiPathTemplate, +} from "@sentry/toolkit-core/api-path-template"; export type CatalogOperation = Pick; @@ -6,11 +10,7 @@ export interface CodeModeAccess { organizationSlug?: string | null; } -interface MatchedPath { - pathTemplate: string; - parameters: Record; - literalSegments: number; -} +type MatchedPath = ApiPathTemplateMatch & { pathTemplate: string }; function decodePathSegment(segment: string): string { let decoded: string; @@ -27,44 +27,23 @@ function decodePathSegment(segment: string): string { return decoded; } -/** The most literal published path template matching the URL. */ -function findPath(url: URL, pathTemplates: Iterable): MatchedPath { - const segments = url.pathname.split("/").map(decodePathSegment); +/** The most literal published path template matching a decoded path. */ +function findPath( + path: string, + pathTemplates: Iterable, +): MatchedPath | undefined { const matches: MatchedPath[] = []; for (const pathTemplate of pathTemplates) { - const templateSegments = pathTemplate.split("/"); - if (templateSegments.length !== segments.length) continue; - const parameters: Record = {}; - let literalSegments = 0; - const matchesPath = templateSegments.every((segment, index) => { - const parameter = /^\{([^{}]+)\}$/.exec(segment); - if (parameter) { - if (!segments[index] || Object.hasOwn(parameters, parameter[1])) { - return false; - } - parameters[parameter[1]] = segments[index]; - return true; - } - literalSegments++; - return segment === segments[index]; - }); - if (matchesPath) { - matches.push({ pathTemplate, parameters, literalSegments }); - } + const match = matchApiPathTemplate(pathTemplate, path); + if (match) matches.push({ pathTemplate, ...match }); } matches.sort((a, b) => b.literalSegments - a.literalSegments); - const match = matches[0]; - if (!match) { - throw new Error( - "This API path is not available in the published API schema.", - ); - } - if (matches[1]?.literalSegments === match.literalSegments) { + if (matches[1] && matches[1].literalSegments === matches[0].literalSegments) { throw new Error( "This API path matches multiple operations in the API schema.", ); } - return match; + return matches[0]; } /** @@ -77,11 +56,22 @@ export function authorizeApiRequest( method: string, catalog: readonly CatalogOperation[], access: CodeModeAccess, -): { pathTemplate: string; organizationSlug?: string } { - const { pathTemplate, parameters } = findPath( - url, - new Set(catalog.map(({ path }) => path)), - ); +): { pathTemplate: string; pathname: string; organizationSlug?: string } { + const templates = new Set(catalog.map(({ path }) => path)); + const decoded = url.pathname.split("/").map(decodePathSegment).join("/"); + let pathname = url.pathname; + let match = findPath(decoded, templates); + // Most routes end with a slash. Send the canonical path rather than fail. + if (!match && !pathname.endsWith("/")) { + match = findPath(`${decoded}/`, templates); + pathname = `${pathname}/`; + } + if (!match) { + throw new Error( + "This API path is not available in the published API schema.", + ); + } + const { pathTemplate, parameters } = match; const methods = new Set( catalog.filter(({ path }) => path === pathTemplate).map((op) => op.method), ); @@ -103,5 +93,9 @@ export function authorizeApiRequest( ); } - return { pathTemplate, ...(organizationSlug ? { organizationSlug } : {}) }; + return { + pathTemplate, + pathname, + ...(organizationSlug ? { organizationSlug } : {}), + }; } diff --git a/packages/mcp-cloudflare/src/code-mode/authorization.ts b/packages/mcp-cloudflare/src/code-mode/authorization.ts index d352acdc6..7c57bb364 100644 --- a/packages/mcp-cloudflare/src/code-mode/authorization.ts +++ b/packages/mcp-cloudflare/src/code-mode/authorization.ts @@ -24,8 +24,9 @@ export class ApiAuthorization { const target = new URL(url); let organizationSlug: string | undefined; let pathTemplate: string; + let pathname: string; try { - ({ organizationSlug, pathTemplate } = authorizeApiRequest( + ({ organizationSlug, pathTemplate, pathname } = authorizeApiRequest( target, method, OPERATIONS, @@ -40,7 +41,7 @@ export class ApiAuthorization { await apiServiceForOrganization(this.#context, organizationSlug) ).requestTarget(); return { - url: new URL(`${target.pathname}${target.search}`, origin).toString(), + url: new URL(`${pathname}${target.search}`, origin).toString(), headers, pathTemplate, }; diff --git a/packages/mcp-cloudflare/src/code-mode/tools.integration.test.ts b/packages/mcp-cloudflare/src/code-mode/tools.integration.test.ts index b51ba3c42..759dd34e7 100644 --- a/packages/mcp-cloudflare/src/code-mode/tools.integration.test.ts +++ b/packages/mcp-cloudflare/src/code-mode/tools.integration.test.ts @@ -417,15 +417,16 @@ describe("Code Mode on the authenticated MCP", () => { expect(apiRequests).toHaveLength(0); }); - it("uses the API write grant, regional routing and attribution headers", async () => { + it("uses the API write grant, regional routing, the canonical path and attribution headers", async () => { apiResponse = (request) => Response.json( { saved: JSON.parse(request.body) }, { headers: { ETag: '"saved-monitor"', Link: "; rel=next" } }, ); + // Without the trailing slash: the broker sends the published path. const result = await execute( `async () => sentry.request({ - method: "PUT", path: "/projects/code-mode-org/web/monitors/billing/", body: {name: "Billing"} + method: "PUT", path: "/projects/code-mode-org/web/monitors/billing", body: {name: "Billing"} })`, { props: { grantedSkills: ["api-write"] } }, ); diff --git a/packages/toolkit-core/package.json b/packages/toolkit-core/package.json index 4b3d127db..d8dbd2b03 100644 --- a/packages/toolkit-core/package.json +++ b/packages/toolkit-core/package.json @@ -15,6 +15,10 @@ "types": "./src/api-path-segment.ts", "default": "./src/api-path-segment.ts" }, + "./api-path-template": { + "types": "./src/api-path-template.ts", + "default": "./src/api-path-template.ts" + }, "./auth-token": { "types": "./src/auth-token.ts", "default": "./src/auth-token.ts" diff --git a/packages/toolkit-core/src/api-path-template.test.ts b/packages/toolkit-core/src/api-path-template.test.ts new file mode 100644 index 000000000..44039af1f --- /dev/null +++ b/packages/toolkit-core/src/api-path-template.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from "vitest"; +import { matchApiPathTemplate } from "./api-path-template.js"; + +const issues = "/api/0/organizations/{organization_id_or_slug}/issues/"; + +describe("matchApiPathTemplate", () => { + it("captures parameters and counts literal segments", () => { + expect( + matchApiPathTemplate(issues, "/api/0/organizations/acme/issues/"), + ).toEqual({ + parameters: { organization_id_or_slug: "acme" }, + // "", "api", "0", "organizations", "issues", "" + literalSegments: 6, + }); + }); + + it.each([ + ["a different literal", "/api/0/projects/acme/issues/"], + ["an empty parameter", "/api/0/organizations//issues/"], + ["a missing trailing slash", "/api/0/organizations/acme/issues"], + ["an extra segment", "/api/0/organizations/acme/issues/1/"], + ])("rejects %s", (_, path) => { + expect(matchApiPathTemplate(issues, path)).toBeUndefined(); + }); + + it("ranks a literal route above a parameter at the same position", () => { + const path = "/api/0/organizations/settings/"; + const generic = matchApiPathTemplate( + "/api/0/organizations/{organization_id_or_slug}/", + path, + ); + const literal = matchApiPathTemplate(path, path); + expect(literal!.literalSegments).toBeGreaterThan(generic!.literalSegments); + }); + + it("rejects a template that repeats a parameter name", () => { + expect(matchApiPathTemplate("/{id}/{id}/", "/a/b/")).toBeUndefined(); + }); +}); diff --git a/packages/toolkit-core/src/api-path-template.ts b/packages/toolkit-core/src/api-path-template.ts new file mode 100644 index 000000000..cd67a4e3b --- /dev/null +++ b/packages/toolkit-core/src/api-path-template.ts @@ -0,0 +1,42 @@ +/** A concrete API path matched against an OpenAPI path template. */ +export type ApiPathTemplateMatch = { + /** Path parameter values by name, as they appear in the path. */ + parameters: Record; + /** Segments that matched the template literally, for ranking overlaps. */ + literalSegments: number; +}; + +const TEMPLATE_SEGMENT = /^\{([^{}]+)\}$/; + +/** + * Matches a concrete API path against an OpenAPI path template such as + * `/api/0/organizations/{organization_id_or_slug}/issues/`. + * + * Each `{name}` matches one non-empty segment; every other segment must be + * equal, and the segment counts must match, trailing slash included. Callers + * own decoding and validating segments first and choosing among overlapping + * templates, for which `literalSegments` ranks the most specific. + */ +export function matchApiPathTemplate( + template: string, + path: string, +): ApiPathTemplateMatch | undefined { + const templateSegments = template.split("/"); + const segments = path.split("/"); + if (templateSegments.length !== segments.length) return; + + const parameters: Record = {}; + let literalSegments = 0; + for (const [index, templateSegment] of templateSegments.entries()) { + const segment = segments[index] ?? ""; + const name = TEMPLATE_SEGMENT.exec(templateSegment)?.[1]; + if (name === undefined) { + if (templateSegment !== segment) return; + literalSegments++; + } else { + if (!segment || Object.hasOwn(parameters, name)) return; + parameters[name] = segment; + } + } + return { parameters, literalSegments }; +}