From 9e6674034078147f9f1458b7cd48972dbc54aa2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20Beteg=C3=B3n?= Date: Wed, 23 Sep 2026 17:35:26 +0200 Subject: [PATCH] fix(issue): resolve shared links with the current API contract --- packages/cli/src/commands/issue/utils.ts | 81 ++---- packages/cli/src/lib/api/issues.ts | 51 +++- packages/cli/src/lib/sentry-url-parser.ts | 5 + .../cli/test/commands/issue/utils.test.ts | 237 +++++++++--------- .../cli/test/lib/api/shared-issues.test.ts | 111 ++++++++ .../lib/security/custom-headers-leak.test.ts | 6 +- 6 files changed, 313 insertions(+), 178 deletions(-) create mode 100644 packages/cli/test/lib/api/shared-issues.test.ts diff --git a/packages/cli/src/commands/issue/utils.ts b/packages/cli/src/commands/issue/utils.ts index 7afb9fbe32..ea33466cca 100644 --- a/packages/cli/src/commands/issue/utils.ts +++ b/packages/cli/src/commands/issue/utils.ts @@ -20,7 +20,11 @@ import { triggerRootCauseAnalysis, tryGetIssueByShortId, } from "../../lib/api-client.js"; -import { type IssueSelector, parseIssueArg } from "../../lib/arg-parsing.js"; +import { + type IssueSelector, + type ParsedIssueArg, + parseIssueArg, +} from "../../lib/arg-parsing.js"; import { clearCachedIssueOrg, getCachedIssueOrg, @@ -513,61 +517,31 @@ async function resolveSelector( * 1. Call public share API to get numeric group ID * 2. Fetch full issue details via authenticated API * - * When the share URL includes org context (from subdomain), uses org-scoped - * endpoint for proper region routing. Otherwise falls back to the unscoped - * endpoint and extracts org from the response permalink. + * Both requests require organization context, taken from the share URL + * or the usual defaults and DSN resolution. * - * @param shareId - The share ID from the URL - * @param org - Optional organization slug (from share URL subdomain) - * @param baseUrl - The Sentry instance base URL + * @param share - Share URL components, including optional organization context * @param cwd - Current working directory for context resolution + * @param commandHint - Recovery command when organization context is missing */ async function resolveShareIssue( - shareId: string, - org: string | undefined, - baseUrl: string, - cwd: string -): Promise { - const shared = await getSharedIssue(baseUrl, shareId); - const groupId = shared.groupID; - - // Fetch full issue via authenticated API - if (org) { - const resolvedOrg = await resolveEffectiveOrg(org); - const orgScopedIssue = await getIssueInOrg(resolvedOrg, groupId, { - collapse: ISSUE_DETAIL_COLLAPSE, - }); - return { org: resolvedOrg, issue: orgScopedIssue }; + share: Extract, + cwd: string, + commandHint: string +): Promise { + const { shareId, org, baseUrl } = share; + const resolvedOrg = org + ? await resolveEffectiveOrg(org) + : (await resolveOrg({ cwd }))?.org; + if (!resolvedOrg) { + throw new ContextError("Organization", commandHint); } - // No org from URL — try env/DSN context, then the issue-id → org cache, - // then fall back to the unscoped fetch. See resolveNumericIssue for the - // full rationale behind the cache. - const resolvedOrg = await resolveOrg({ cwd }); - const cachedOrg = resolvedOrg ? null : getCachedIssueOrg(groupId); - const { issue, cacheEvicted } = await fetchIssueByNumericId( - groupId, - resolvedOrg?.org, - cachedOrg - ); - // When `cacheEvicted` is true, the cached org was stale (404'd) — do NOT - // let it win the `??` chain; re-derive from the permalink instead. - const effectiveCachedOrg = cacheEvicted ? null : cachedOrg; - const resolvedOrgSlug = - resolvedOrg?.org ?? - effectiveCachedOrg ?? - extractOrgFromPermalink(issue.permalink); - if (resolvedOrgSlug && !resolvedOrg && !effectiveCachedOrg) { - // Best-effort — a broken/read-only DB must not fail a successful lookup. - try { - setCachedIssueOrg(groupId, resolvedOrgSlug); - } catch (cacheErr) { - log.debug( - `Failed to cache issue-org mapping for ${groupId}: ${String(cacheErr)}` - ); - } - } - return { org: resolvedOrgSlug, issue }; + const shared = await getSharedIssue(baseUrl, resolvedOrg, shareId); + const issue = await getIssueInOrg(resolvedOrg, shared.id, { + collapse: ISSUE_DETAIL_COLLAPSE, + }); + return { org: resolvedOrg, issue }; } /** @@ -844,12 +818,7 @@ export async function resolveIssue( case "share": // Share URL — resolve via public share API, then authenticated fetch - result = await resolveShareIssue( - parsed.shareId, - parsed.org, - parsed.baseUrl, - cwd - ); + result = await resolveShareIssue(parsed, cwd, commandHint); break; default: { diff --git a/packages/cli/src/lib/api/issues.ts b/packages/cli/src/lib/api/issues.ts index ec2ac970a8..19c08614bb 100644 --- a/packages/cli/src/lib/api/issues.ts +++ b/packages/cli/src/lib/api/issues.ts @@ -692,20 +692,23 @@ export async function mergeIssues( /** * Resolve a share ID to basic issue data via the public share endpoint. * - * This endpoint does not require authentication and is not org-scoped. - * The response includes the numeric `groupID` needed to fetch full issue + * This org-scoped endpoint does not require authentication. + * The response includes the numeric issue `id` needed to fetch full issue * details via the authenticated API. * * @param baseUrl - The Sentry instance base URL (from the share URL) + * @param orgSlug - The organization that owns the shared issue * @param shareId - The share ID extracted from the share URL - * @returns Object containing the numeric groupID + * @returns Object containing the numeric issue ID * @throws {ApiError} When the share link is expired, disabled, or invalid */ export async function getSharedIssue( baseUrl: string, + orgSlug: string, shareId: string -): Promise<{ groupID: string }> { - const url = `${baseUrl}/api/0/shared/issues/${encodeURIComponent(shareId)}/`; +): Promise<{ id: string }> { + const path = `organizations/${encodeURIComponent(orgSlug)}/shared/issues/${encodeURIComponent(shareId)}/`; + const url = `${baseUrl.replace(TRAILING_SLASH_RE, "")}/api/0/${path}`; const headers = new Headers({ "Content-Type": "application/json" }); // URL-scoped: headers only attach when `url`'s origin matches the trusted // host, so IAP tokens etc. can't leak to an attacker-controlled share URL. @@ -721,7 +724,7 @@ export async function getSharedIssue( "TLS certificate error", 0, buildTlsErrorDetail(error), - `shared/issues/${shareId}` + path ); } throw error; @@ -734,16 +737,46 @@ export async function getSharedIssue( 404, "The share link may have been disabled by the issue owner.\n" + " Ask them to re-enable sharing, or use the issue ID directly.", - `shared/issues/${shareId}` + path ); } throw new ApiError( "Failed to resolve share link", response.status, undefined, - `shared/issues/${shareId}` + path + ); + } + + let data: unknown; + try { + data = await response.json(); + } catch (error) { + if (!(error instanceof SyntaxError)) { + throw error; + } + throw new ApiError( + "Share link returned invalid JSON", + response.status, + undefined, + path + ); + } + + if ( + typeof data !== "object" || + data === null || + !("id" in data) || + typeof data.id !== "string" || + data.id.length === 0 + ) { + throw new ApiError( + "Share link response missing a valid issue ID", + response.status, + undefined, + path ); } - return (await response.json()) as { groupID: string }; + return { id: data.id }; } diff --git a/packages/cli/src/lib/sentry-url-parser.ts b/packages/cli/src/lib/sentry-url-parser.ts index 0ca0b83656..8f0175f2e4 100644 --- a/packages/cli/src/lib/sentry-url-parser.ts +++ b/packages/cli/src/lib/sentry-url-parser.ts @@ -72,6 +72,10 @@ function matchOrganizationsPath( return { baseUrl, org, issueId: segments[3], eventId }; } + if (segments[2] === "share" && segments[3] === "issue" && segments[4]) { + return { baseUrl, org, shareId: segments[4] }; + } + const tracePath = matchTracePath(segments, 2); if (tracePath.status === "detail") { return { baseUrl, org, traceId: tracePath.traceId }; @@ -344,6 +348,7 @@ function matchSharePath( * Recognizes these path patterns (both SaaS and self-hosted): * - `/organizations/{org}/issues/{id}/` * - `/organizations/{org}/issues/{id}/events/{eventId}/` + * - `/organizations/{org}/share/issue/{shareId}/` * - `/settings/{org}/projects/{project}/` * - `/organizations/{org}/explore/traces/trace/{traceId}/` (canonical) * - `/organizations/{org}/traces/{traceId}/` (legacy) diff --git a/packages/cli/test/commands/issue/utils.test.ts b/packages/cli/test/commands/issue/utils.test.ts index 51128fe1da..b6a5991619 100644 --- a/packages/cli/test/commands/issue/utils.test.ts +++ b/packages/cli/test/commands/issue/utils.test.ts @@ -16,8 +16,17 @@ import { DEFAULT_SENTRY_URL } from "../../../src/lib/constants.js"; import { setAuthToken } from "../../../src/lib/db/auth.js"; import { setCachedProject } from "../../../src/lib/db/project-cache.js"; import { setOrgRegion } from "../../../src/lib/db/regions.js"; -import { ApiError, ResolutionError } from "../../../src/lib/errors.js"; -import { mockFetch, useTestConfigDir } from "../../helpers.js"; +import { + ApiError, + ContextError, + ResolutionError, +} from "../../../src/lib/errors.js"; +import { + mockFetch, + resetHostScopingState, + useEnvSandbox, + useTestConfigDir, +} from "../../helpers.js"; describe("buildCommandHint", () => { test("suggests /ID for numeric IDs", () => { @@ -2265,135 +2274,139 @@ describe("resolveIssue: project-search DSN shortcut", () => { }); describe("resolveIssue with share URLs", () => { - const cwd = "/tmp/test-share"; - - test("resolves share URL with org from subdomain", async () => { - setOrgRegion("gibush-kq", DEFAULT_SENTRY_URL); - - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { - const req = new Request(input, init); - const url = req.url; - - // Share API endpoint (public, no auth) - if (url.includes("/shared/issues/f1abd515c51346778384ff25dfb341e5")) { - return new Response(JSON.stringify({ groupID: "99124558" }), { - status: 200, - headers: { "Content-Type": "application/json" }, + useEnvSandbox([ + "SENTRY_AUTH_TOKEN", + "SENTRY_TOKEN", + "SENTRY_HOST", + "SENTRY_URL", + "SENTRY_ORG", + "SENTRY_PROJECT", + "SENTRY_DSN", + ]); + beforeEach(resetHostScopingState); + afterEach(resetHostScopingState); + + const shareId = "aabbccdd11223344aabbccdd11223344"; + + test.each([ + [ + "SaaS subdomain", + "https://test-org.sentry.io", + "test-org", + `/share/issue/${shareId}/`, + ], + [ + "SaaS org path", + "https://sentry.io", + "test-org", + `/organizations/test-org/share/issue/${shareId}/`, + ], + [ + "self-hosted org path", + "https://sentry.example.com", + "self-hosted-org", + `/organizations/self-hosted-org/share/issue/${shareId}/`, + ], + [ + "legacy URL with default org", + "https://sentry.io", + "test-org", + `/share/issue/${shareId}/`, + ], + ])("resolves %s using the shared issue id", async (name, baseUrl, org, path) => { + const { setDefaultOrganization } = await import( + "../../../src/lib/db/defaults.js" + ); + setDefaultOrganization( + name === "legacy URL with default org" ? org : "other-org" + ); + const apiBaseUrl = + baseUrl === "https://sentry.example.com" ? baseUrl : DEFAULT_SENTRY_URL; + setAuthToken("test-token", undefined, undefined, { host: apiBaseUrl }); + setOrgRegion(org, apiBaseUrl); + const requests: Request[] = []; + globalThis.fetch = mockFetch(async (input, init) => { + const request = new Request(input, init); + requests.push(request); + if ( + request.url === + `${baseUrl}/api/0/organizations/${org}/shared/issues/${shareId}/` + ) { + return Response.json({ + id: "12345", + title: "Shared issue", + project: { slug: "backend" }, }); } - - // Authenticated issue fetch - if (url.includes("/organizations/gibush-kq/issues/99124558/")) { - return new Response( - JSON.stringify({ - id: "99124558", - shortId: "BACKEND-A1", - title: "Share Test Issue", - status: "unresolved", - platform: "python", - type: "error", - count: "5", - userCount: 3, - }), - { status: 200, headers: { "Content-Type": "application/json" } } - ); + if ( + new URL(request.url).pathname === + `/api/0/organizations/${org}/issues/12345/` + ) { + return Response.json({ + id: "12345", + shortId: "BACKEND-A1", + title: "Shared issue", + status: "unresolved", + platform: "python", + type: "error", + count: "5", + userCount: 3, + }); } - - return new Response(JSON.stringify({ detail: "Not found" }), { - status: 404, - headers: { "Content-Type": "application/json" }, - }); - }; + return Response.json({ detail: "Not found" }, { status: 404 }); + }); const result = await resolveIssue({ - issueArg: - "https://gibush-kq.sentry.io/share/issue/f1abd515c51346778384ff25dfb341e5/", - cwd, + issueArg: `${baseUrl}${path}`, + cwd: getConfigDir(), command: "view", }); - expect(result.org).toBe("gibush-kq"); - expect(result.issue.id).toBe("99124558"); + expect(result.org).toBe(org); + expect(result.issue.id).toBe("12345"); expect(result.issue.shortId).toBe("BACKEND-A1"); + expect(requests).toHaveLength(2); + expect(requests[0]?.headers.has("Authorization")).toBe(false); + expect(requests[1]?.headers.get("Authorization")).toBe("Bearer test-token"); }); - test("resolves share URL without org via unscoped fetch", async () => { - // @ts-expect-error - partial mock - globalThis.fetch = async (input: RequestInfo | URL, init?: RequestInit) => { - const req = new Request(input, init); - const url = req.url; - - // Share API endpoint - if (url.includes("/shared/issues/aabbccdd11223344aabbccdd11223344")) { - return new Response(JSON.stringify({ groupID: "55555" }), { - status: 200, - headers: { "Content-Type": "application/json" }, - }); - } - - // Unscoped issue fetch - if (url.includes("/issues/55555/")) { - return new Response( - JSON.stringify({ - id: "55555", - shortId: "WEB-B2", - title: "Unscoped Share Issue", - status: "unresolved", - platform: "javascript", - type: "error", - count: "1", - userCount: 1, - permalink: "https://test-org.sentry.io/issues/55555/", - }), - { status: 200, headers: { "Content-Type": "application/json" } } - ); - } - - return new Response(JSON.stringify({ detail: "Not found" }), { - status: 404, - headers: { "Content-Type": "application/json" }, - }); - }; - - const result = await resolveIssue({ - issueArg: - "https://sentry.io/share/issue/aabbccdd11223344aabbccdd11223344/", - cwd, - command: "view", + test("requires organization context before requesting a legacy share URL", async () => { + const requests: string[] = []; + globalThis.fetch = mockFetch(async (input, init) => { + requests.push(new Request(input, init).url); + return Response.json({ detail: "Not found" }, { status: 404 }); }); - expect(result.issue.id).toBe("55555"); - expect(result.org).toBe("test-org"); + await expect( + resolveIssue({ + issueArg: `https://sentry.io/share/issue/${shareId}/`, + cwd: getConfigDir(), + command: "view", + }) + ).rejects.toBeInstanceOf(ContextError); + expect(requests).toEqual([]); }); - test("throws ApiError when share link is expired/disabled", async () => { - // @ts-expect-error - partial mock - globalThis.fetch = async () => - new Response(JSON.stringify({ detail: "Not found" }), { - status: 404, - headers: { "Content-Type": "application/json" }, - }); + test("reports an expired share link without fetching issue details", async () => { + const requests: string[] = []; + globalThis.fetch = mockFetch(async (input, init) => { + requests.push(new Request(input, init).url); + return Response.json({ detail: "Not found" }, { status: 404 }); + }); await expect( resolveIssue({ - issueArg: - "https://sentry.io/share/issue/deadbeefdeadbeefdeadbeefdeadbeef/", - cwd, + issueArg: `https://test-org.sentry.io/share/issue/${shareId}/`, + cwd: getConfigDir(), command: "view", }) - ).rejects.toThrow(ApiError); - - try { - await resolveIssue({ - issueArg: - "https://sentry.io/share/issue/deadbeefdeadbeefdeadbeefdeadbeef/", - cwd, - command: "view", - }); - } catch (error) { - expect(error).toBeInstanceOf(ApiError); - expect((error as ApiError).message).toContain("Share link not found"); - } + ).rejects.toMatchObject({ + name: "ApiError", + message: "Share link not found or expired", + status: 404, + }); + expect(requests).toEqual([ + `https://test-org.sentry.io/api/0/organizations/test-org/shared/issues/${shareId}/`, + ]); }); }); diff --git a/packages/cli/test/lib/api/shared-issues.test.ts b/packages/cli/test/lib/api/shared-issues.test.ts new file mode 100644 index 0000000000..f11c8abe7c --- /dev/null +++ b/packages/cli/test/lib/api/shared-issues.test.ts @@ -0,0 +1,111 @@ +/** + * Public shared-issue response and request-boundary regressions. + */ + +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { getSharedIssue } from "../../../src/lib/api/issues.js"; +import { setAuthToken } from "../../../src/lib/db/auth.js"; +import { ApiError } from "../../../src/lib/errors.js"; +import { mockFetch, useEnvSandbox, useTestConfigDir } from "../../helpers.js"; + +describe("getSharedIssue", () => { + useTestConfigDir("shared-issues-"); + useEnvSandbox([ + "SENTRY_AUTH_TOKEN", + "SENTRY_TOKEN", + "SENTRY_HOST", + "SENTRY_URL", + "SENTRY_CUSTOM_HEADERS", + ]); + + let originalFetch: typeof globalThis.fetch; + + beforeEach(() => { + originalFetch = globalThis.fetch; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + test("reads the backend id from the public org-scoped endpoint without bearer auth", async () => { + setAuthToken("test-token"); + let request: Request | undefined; + globalThis.fetch = mockFetch(async (input, init) => { + request = new Request(input, init); + return Response.json({ + id: "12345", + title: "Example shared issue", + project: { slug: "example-project" }, + }); + }); + + const result = await getSharedIssue( + "https://sentry.io", + "example org", + "share/id" + ); + + expect(result.id).toBe("12345"); + expect(request?.url).toBe( + "https://sentry.io/api/0/organizations/example%20org/shared/issues/share%2Fid/" + ); + expect(request?.method).toBe("GET"); + expect(request?.headers.has("Authorization")).toBe(false); + }); + + test.each([ + ["null body", null], + ["missing id", {}], + ["obsolete groupID field", { groupID: "12345" }], + ["null id", { id: null }], + ["numeric id", { id: 12_345 }], + ["empty id", { id: "" }], + ])("rejects a response with %s", async (_description, body) => { + globalThis.fetch = mockFetch(async () => Response.json(body)); + + await expect( + getSharedIssue("https://sentry.io", "example-org", "share-id") + ).rejects.toBeInstanceOf(ApiError); + }); + + test("reports malformed JSON as an API response error", async () => { + globalThis.fetch = mockFetch(async () => new Response("not JSON")); + + await expect( + getSharedIssue("https://sentry.io", "example-org", "share-id") + ).rejects.toMatchObject({ + name: "ApiError", + message: expect.stringContaining("invalid JSON"), + }); + }); + + test("preserves body read errors", async () => { + const error = new TypeError("Response stream interrupted"); + globalThis.fetch = mockFetch( + async () => + new Response( + new ReadableStream({ + start(controller) { + controller.error(error); + }, + }) + ) + ); + + await expect( + getSharedIssue("https://sentry.io", "example-org", "share-id") + ).rejects.toBe(error); + }); + + test.each([ + [404, "Share link not found or expired"], + [503, "Failed to resolve share link"], + ])("preserves the HTTP %s error", async (status, message) => { + globalThis.fetch = mockFetch(async () => new Response("", { status })); + + await expect( + getSharedIssue("https://sentry.io", "example-org", "share-id") + ).rejects.toMatchObject({ name: "ApiError", status, message }); + }); +}); diff --git a/packages/cli/test/lib/security/custom-headers-leak.test.ts b/packages/cli/test/lib/security/custom-headers-leak.test.ts index c795e543a4..8fc146594c 100644 --- a/packages/cli/test/lib/security/custom-headers-leak.test.ts +++ b/packages/cli/test/lib/security/custom-headers-leak.test.ts @@ -65,7 +65,11 @@ describe("CVE: custom-headers leak (share URL + auth-login bypass)", () => { }) as typeof fetch; try { - await getSharedIssue("https://evil.com", "deadbeef12345678").catch(() => { + await getSharedIssue( + "https://evil.com", + "test-org", + "deadbeef12345678" + ).catch(() => { /* we only care about headers, not the response */ }); expect(capturedHeaders?.get("X-IAP-Token")).toBeNull();