diff --git a/src/commands/issue/list.ts b/src/commands/issue/list.ts index e24c56515..ed337e30e 100644 --- a/src/commands/issue/list.ts +++ b/src/commands/issue/list.ts @@ -19,6 +19,7 @@ import { listIssuesPaginated, listProjects, } from "../../lib/api-client.js"; +import { extractRequiredScopes } from "../../lib/api-scope.js"; import { looksLikeIssueShortId, parseOrgProjectArg, @@ -1111,6 +1112,14 @@ function enrichIssueListError( throw error; } +/** + * Default scopes mentioned when the API response doesn't tell us which + * scope is missing. These are the minimum the issue-list endpoint needs + * — surfaced verbatim from the previous hardcoded message so the + * fallback behavior matches the pre-fix UX. + */ +const DEFAULT_ISSUE_LIST_SCOPES = "org:read, project:read"; + /** * Build an enriched error detail for 403 Forbidden responses. * @@ -1118,21 +1127,38 @@ function enrichIssueListError( * (SENTRY_AUTH_TOKEN / SENTRY_TOKEN) since the regular `sentry auth login` * OAuth flow always grants the required scopes. * + * When the API's detail payload names the required scope(s) explicitly + * (see {@link extractRequiredScopes}) we surface that list instead of + * the hardcoded default — this is the fix for getsentry/cli#785 item #9 + * where a token missing `event:read` was told it might be missing + * `org:read, project:read` (which it actually had). + * * @param originalDetail - The API response detail (may be undefined) * @returns Enhanced detail string with suggestions */ -function build403Detail(originalDetail: string | undefined): string { +function build403Detail(originalDetail: unknown): string { const lines: string[] = []; - if (originalDetail) { + if (typeof originalDetail === "string" && originalDetail) { lines.push(originalDetail, ""); } lines.push("Suggestions:"); if (isEnvTokenActive()) { + const scopes = extractRequiredScopes(originalDetail); + const scopeList = + scopes.length > 0 ? scopes.join(", ") : DEFAULT_ISSUE_LIST_SCOPES; + // When the API was explicit about what's missing, frame the hint + // as a definite statement ("is missing") rather than a hedged + // "may lack" — this is the user-visible payoff of parsing the + // response. + const leader = + scopes.length > 0 + ? `Your ${getActiveEnvVarName()} token is missing the required scope(s)` + : `Your ${getActiveEnvVarName()} token may lack the required scopes`; lines.push( - ` • Your ${getActiveEnvVarName()} token may lack the required scopes (org:read, project:read)`, + ` • ${leader} (${scopeList})`, " • Check token scopes at: https://sentry.io/settings/auth-tokens/" ); } else { diff --git a/src/commands/project/view.ts b/src/commands/project/view.ts index 2430cbc1b..623c5f9d0 100644 --- a/src/commands/project/view.ts +++ b/src/commands/project/view.ts @@ -13,7 +13,7 @@ import { } from "../../lib/arg-parsing.js"; import { openInBrowser } from "../../lib/browser.js"; import { buildCommand } from "../../lib/command.js"; -import { ContextError, withAuthGuard } from "../../lib/errors.js"; +import { AuthError, ContextError, withAuthGuard } from "../../lib/errors.js"; import { divider, formatProjectDetails } from "../../lib/formatters/index.js"; import { CommandOutput } from "../../lib/formatters/output.js"; import { @@ -92,15 +92,16 @@ type ProjectWithDsn = { }; /** - * Fetch project details and keys for a single target. - * Returns null on non-auth errors (e.g., no access to project). - * Rethrows auth errors so they propagate to the user. + * Parallel project + DSN fetch for a single target. + * + * `AuthError` always propagates so the auto-login middleware fires. + * Other API failures rethrow so callers can choose to swallow + * (auto-detect) or surface (explicit/search) them. */ -async function fetchProjectDetails( +async function fetchProjectAndDsn( target: ResolvedTarget -): Promise { +): Promise { const result = await withAuthGuard(async () => { - // Fetch project (skip if already fetched during resolution) and DSN in parallel const [project, dsn] = await Promise.all([ target.projectData ? Promise.resolve(target.projectData) @@ -109,7 +110,40 @@ async function fetchProjectDetails( ]); return { project, dsn }; }); - return result.ok ? result.value : null; + if (result.ok) { + return result.value; + } + throw result.error; +} + +/** + * Fetch details, swallowing non-auth failures (auto-detect mode). + * `AuthError` still propagates for the auto-login middleware. + */ +async function fetchProjectDetails( + target: ResolvedTarget +): Promise { + try { + return await fetchProjectAndDsn(target); + } catch (error) { + if (error instanceof AuthError) { + throw error; + } + return null; + } +} + +/** + * Fetch details, rethrowing API errors verbatim. + * + * Used for explicit/project-search targets: the user named the + * project, so surfacing the real 403/404 is more useful than the + * generic "Could not auto-detect" fallback (getsentry/cli#785 #8). + */ +function fetchProjectDetailsOrThrow( + target: ResolvedTarget +): Promise { + return fetchProjectAndDsn(target); } /** Result of fetching project details for multiple targets */ @@ -120,8 +154,8 @@ type FetchResult = { }; /** - * Fetch project details for all targets in parallel. - * Filters out failed fetches while preserving target association. + * Fetch details for every auto-detected target in parallel, filtering + * out failures while preserving target association. */ async function fetchAllProjectDetails( targets: ResolvedTarget[] @@ -282,9 +316,28 @@ export const viewCommand = buildCommand({ return; } - // Fetch project details for all targets in parallel - const { projects, dsns, targets } = - await fetchAllProjectDetails(resolvedTargets); + // Auto-detect tolerates per-target failures (DSN scans may yield + // inaccessible targets); explicit/search rethrows so the real + // 403/404 surfaces instead of a misleading "not provided" error. + let projects: SentryProject[]; + let dsns: (string | null)[]; + let targets: ResolvedTarget[]; + + if (parsed.type === ProjectSpecificationType.AutoDetect) { + const fetched = await fetchAllProjectDetails(resolvedTargets); + projects = fetched.projects; + dsns = fetched.dsns; + targets = fetched.targets; + } else { + const firstTarget = resolvedTargets[0]; + if (!firstTarget) { + throw buildContextError(); + } + const detail = await fetchProjectDetailsOrThrow(firstTarget); + projects = [detail.project]; + dsns = [detail.dsn]; + targets = [firstTarget]; + } if (projects.length === 0) { throw buildContextError(); diff --git a/src/lib/api-scope.ts b/src/lib/api-scope.ts new file mode 100644 index 000000000..9a59f7a38 --- /dev/null +++ b/src/lib/api-scope.ts @@ -0,0 +1,128 @@ +/** + * Extract Sentry scope identifiers from a 403 response, so we can hint + * at the specific missing scope instead of a hardcoded default + * (getsentry/cli#785 #9). + * + * Sentry's standard 403 path is a DRF `PermissionDenied` with no + * structured scope info, but some endpoints include the scope in the + * free-text `detail`. We also peek at a few plausible structured field + * names (`required` / `requiredScopes` / `scopes`) in case they're + * added later. Empty result → callers fall back to their defaults. + */ + +/** + * Canonical Sentry scopes, mirrored from getsentry/sentry + * `src/sentry/conf/server.py` SENTRY_SCOPES. Excludes OIDC scopes + * (`openid`/`profile`/`email`) and internal-only `org:superuser`. + */ +const SENTRY_SCOPES = [ + "org:read", + "org:write", + "org:admin", + "org:integrations", + "org:ci", + "member:invite", + "member:read", + "member:write", + "member:admin", + "team:read", + "team:write", + "team:admin", + "project:read", + "project:write", + "project:admin", + "project:releases", + "project:distribution", + "event:read", + "event:write", + "event:admin", + "alerts:read", + "alerts:write", +] as const; + +// Explicit alternation (not `:` product) rejects nonexistent +// combinations like `release:write` or `alerts:admin`. `:` is not a +// regex metachar so no escaping needed. +const KNOWN_SCOPE_RE = new RegExp(`\\b(?:${SENTRY_SCOPES.join("|")})\\b`, "gi"); + +const SCOPE_FIELD_NAMES = ["required", "requiredScopes", "scopes"] as const; + +/** + * Extract Sentry scope identifiers from a 403 response detail. + * + * @param detail - ApiError.detail value; string, object, or undefined + * @returns Deduplicated, source-ordered scope identifiers. Empty when none found. + */ +export function extractRequiredScopes(detail: unknown): string[] { + if (!detail) { + return []; + } + if (typeof detail === "object") { + const fromFields = extractFromRecord(detail as Record); + if (fromFields.length > 0) { + return fromFields; + } + // Fall back to scanning the serialized form to catch non-standard keys. + return extractFromText(JSON.stringify(detail)); + } + if (typeof detail === "string") { + return extractFromText(detail); + } + return []; +} + +function extractFromRecord(record: Record): string[] { + for (const field of SCOPE_FIELD_NAMES) { + const value = record[field]; + if (!Array.isArray(value)) { + continue; + } + const scopes = collectScopesFromArray(value); + if (scopes.length > 0) { + return [...new Set(scopes)]; + } + } + return []; +} + +/** Accepts both bare strings and `{scope: "..."}` objects. */ +function collectScopesFromArray(entries: unknown[]): string[] { + const out: string[] = []; + for (const entry of entries) { + const scope = extractScopeCandidate(entry); + if (scope && matchesKnownScope(scope)) { + out.push(scope.toLowerCase()); + } + } + return out; +} + +function extractScopeCandidate(entry: unknown): string | undefined { + if (typeof entry === "string") { + return entry; + } + if ( + entry && + typeof entry === "object" && + "scope" in entry && + typeof (entry as { scope: unknown }).scope === "string" + ) { + return (entry as { scope: string }).scope; + } + return; +} + +/** Tests + resets the shared `g`-flagged regex. */ +function matchesKnownScope(scope: string): boolean { + const matched = KNOWN_SCOPE_RE.test(scope); + KNOWN_SCOPE_RE.lastIndex = 0; + return matched; +} + +function extractFromText(text: string): string[] { + const matches = text.match(KNOWN_SCOPE_RE); + if (!matches) { + return []; + } + return [...new Set(matches.map((m) => m.toLowerCase()))]; +} diff --git a/src/lib/api/organizations.ts b/src/lib/api/organizations.ts index a2ad343ef..778cc9d7e 100644 --- a/src/lib/api/organizations.ts +++ b/src/lib/api/organizations.ts @@ -16,6 +16,7 @@ import { UserRegionsResponseSchema, } from "../../types/index.js"; +import { extractRequiredScopes } from "../api-scope.js"; import { getActiveEnvVarName, isEnvTokenActive } from "../db/auth.js"; import { ApiError, withAuthGuard } from "../errors.js"; import { @@ -69,32 +70,53 @@ export async function listOrganizationsInRegion( // Only mention token scopes when using a custom env-var token — // the regular `sentry auth login` OAuth flow always grants org:read. if (error instanceof ApiError && error.status === 403) { - const lines: string[] = []; - if (error.detail) { - lines.push(error.detail, ""); - } - if (isEnvTokenActive()) { - lines.push( - `Your ${getActiveEnvVarName()} token may lack the required 'org:read' scope.`, - "Check token scopes at: https://sentry.io/settings/auth-tokens/" - ); - } else { - lines.push( - "You may not have access to this organization.", - "Re-authenticate with: sentry auth login" - ); - } - throw new ApiError( - error.message, - error.status, - lines.join("\n "), - error.endpoint - ); + throw enrichListOrgsForbidden(error); } throw error; } } +/** + * Enrich a 403 from the list-organizations endpoint with actionable + * hints. Prefers scope(s) named explicitly in the API response over + * the hardcoded `'org:read'` fallback (getsentry/cli#785 item #9). + */ +function enrichListOrgsForbidden(error: ApiError): ApiError { + const lines: string[] = []; + if (error.detail) { + lines.push(error.detail, ""); + } + if (isEnvTokenActive()) { + lines.push(buildEnvTokenScopeHint(error.detail)); + lines.push( + "Check token scopes at: https://sentry.io/settings/auth-tokens/" + ); + } else { + lines.push( + "You may not have access to this organization.", + "Re-authenticate with: sentry auth login" + ); + } + return new ApiError( + error.message, + error.status, + lines.join("\n "), + error.endpoint + ); +} + +/** + * Build a single-line hint mentioning the scope(s) the env-var token + * is missing, preferring the API-provided list when available. + */ +function buildEnvTokenScopeHint(detail: unknown): string { + const scopes = extractRequiredScopes(detail); + if (scopes.length > 0) { + return `Your ${getActiveEnvVarName()} token is missing the required scope(s) '${scopes.join("', '")}'.`; + } + return `Your ${getActiveEnvVarName()} token may lack the required scope 'org:read'.`; +} + /** * List all organizations, returning cached data when available. * diff --git a/test/commands/project/view.func.test.ts b/test/commands/project/view.func.test.ts index 42e46fb3d..65bd59c8c 100644 --- a/test/commands/project/view.func.test.ts +++ b/test/commands/project/view.func.test.ts @@ -252,23 +252,70 @@ describe("viewCommand.func", () => { } }); - test("non-auth API error is skipped silently", async () => { - getProjectSpy.mockRejectedValue(new Error("404 Not Found")); + test("non-auth API error on explicit target is rethrown verbatim", async () => { + // Previously the error was swallowed and a generic + // "Could not auto-detect organization and project" ContextError + // was raised — confusing because the user provided the target + // explicitly (getsentry/cli#785 item #8). The actual API error + // must bubble up so the user sees the real cause (404/403/etc.). + const apiErr = new Error("404 Not Found"); + getProjectSpy.mockRejectedValue(apiErr); getProjectKeysSpy.mockResolvedValue(sampleKeys); const { context } = createMockContext(); const func = await viewCommand.loader(); - // The project fetch fails with a non-auth error, so it's filtered out. - // With no successful results, buildContextError is thrown. await expect( func.call(context, { json: false, web: false }, "my-org/bad-project") - ).rejects.toThrow(ContextError); + ).rejects.toThrow("404 Not Found"); - // getProject was called (it just failed) expect(getProjectSpy).toHaveBeenCalledWith("my-org", "bad-project"); }); + test("non-auth API error on project-search target is rethrown verbatim", async () => { + resolveProjectBySlugSpy.mockResolvedValue({ + org: "acme", + project: "frontend", + }); + const apiErr = new Error("403 Forbidden"); + getProjectSpy.mockRejectedValue(apiErr); + getProjectKeysSpy.mockResolvedValue(sampleKeys); + + const { context } = createMockContext(); + const func = await viewCommand.loader(); + + await expect( + func.call(context, { json: false, web: false }, "frontend") + ).rejects.toThrow("403 Forbidden"); + }); + + test("non-auth API error on auto-detect target is swallowed (multi-target recovery)", async () => { + // For auto-detect — which may surface multiple DSN-discovered + // targets — per-target failures are still tolerated: one + // inaccessible DSN must not block the rest of the results. When + // all targets fail and the set ends up empty, the original + // ContextError is still the right surface. + resolveAllTargetsSpy.mockResolvedValue({ + targets: [ + { + org: "my-org", + project: "inaccessible", + orgDisplay: "my-org", + projectDisplay: "inaccessible", + }, + ], + }); + getProjectSpy.mockRejectedValue(new Error("403 Forbidden")); + getProjectKeysSpy.mockResolvedValue(sampleKeys); + + const { context } = createMockContext(); + const func = await viewCommand.loader(); + + await expect( + func.call(context, { json: false, web: false }) + ).rejects.toThrow(ContextError); + }); + test("auth error from API is rethrown", async () => { getProjectSpy.mockRejectedValue(new AuthError("not_authenticated")); getProjectKeysSpy.mockResolvedValue(sampleKeys); diff --git a/test/lib/api-scope.test.ts b/test/lib/api-scope.test.ts new file mode 100644 index 000000000..0c9fb4b71 --- /dev/null +++ b/test/lib/api-scope.test.ts @@ -0,0 +1,153 @@ +/** + * Tests for `extractRequiredScopes` — the 403-response scope parser + * that powers the friendly "missing scope: event:read" hint in place + * of the hardcoded "(org:read, project:read)" fallback. + */ + +import { describe, expect, test } from "bun:test"; +import { extractRequiredScopes } from "../../src/lib/api-scope.js"; + +describe("extractRequiredScopes", () => { + test("returns [] for undefined or null detail", () => { + expect(extractRequiredScopes(undefined)).toEqual([]); + expect(extractRequiredScopes(null)).toEqual([]); + }); + + test("returns [] when the detail contains no recognizable scopes", () => { + expect( + extractRequiredScopes( + "You do not have permission to perform this action." + ) + ).toEqual([]); + }); + + test("extracts a single scope from a detail string", () => { + expect( + extractRequiredScopes( + "You do not have the required scope to perform this action. Required scopes: event:read" + ) + ).toEqual(["event:read"]); + }); + + test("extracts multiple scopes from a detail string preserving order", () => { + expect( + extractRequiredScopes( + "Required scopes: event:read, project:write. Got: none." + ) + ).toEqual(["event:read", "project:write"]); + }); + + test("deduplicates repeated scopes", () => { + expect( + extractRequiredScopes( + "event:read is required. Try obtaining event:read scope." + ) + ).toEqual(["event:read"]); + }); + + test("ignores random foo:bar substrings that aren't real scopes", () => { + // The regex is anchored to the known resource namespace so that + // response text mentioning things like `http:localhost` or + // `timestamp:now` doesn't accidentally match. + expect( + extractRequiredScopes("http:localhost timestamp:now category:billing") + ).toEqual([]); + }); + + test("lowercases the matched scope identifier", () => { + expect(extractRequiredScopes("Required: EVENT:READ")).toEqual([ + "event:read", + ]); + }); + + test("pulls scopes from a structured `required` field", () => { + expect( + extractRequiredScopes({ + detail: "You do not have permission to perform this action.", + required: ["event:read"], + }) + ).toEqual(["event:read"]); + }); + + test("pulls scopes from a structured `requiredScopes` field", () => { + expect( + extractRequiredScopes({ + detail: "Missing scopes.", + requiredScopes: ["project:admin", "team:write"], + }) + ).toEqual(["project:admin", "team:write"]); + }); + + test("pulls scopes from a `scopes` field of {scope} objects", () => { + expect( + extractRequiredScopes({ + detail: "Missing scopes.", + scopes: [{ scope: "org:read" }, { scope: "org:write" }], + }) + ).toEqual(["org:read", "org:write"]); + }); + + test("falls back to text scanning when the structured fields are absent", () => { + // An object without the known field names gets serialized and + // scanned — catches responses that carry scope info under a + // non-standard key. + expect( + extractRequiredScopes({ + message: "Your token lacks project:read.", + }) + ).toEqual(["project:read"]); + }); + + test("ignores non-string entries in the required array", () => { + expect( + extractRequiredScopes({ + required: [42, null, "event:read", { unrelated: true }], + }) + ).toEqual(["event:read"]); + }); + + test("matches every scope in the canonical Sentry SENTRY_SCOPES list", () => { + // Canonical list mirrored from getsentry/sentry + // `src/sentry/conf/server.py` SENTRY_SCOPES. Keeping this test here + // makes it easy to spot when the backend adds or removes a scope. + const allScopes = [ + "org:read", + "org:write", + "org:admin", + "org:integrations", + "org:ci", + "member:invite", + "member:read", + "member:write", + "member:admin", + "team:read", + "team:write", + "team:admin", + "project:read", + "project:write", + "project:admin", + "project:releases", + "project:distribution", + "event:read", + "event:write", + "event:admin", + "alerts:read", + "alerts:write", + ]; + expect( + extractRequiredScopes(`Required scopes: ${allScopes.join(", ")}`) + ).toEqual(allScopes); + }); + + test("rejects scope-shaped strings that are not real Sentry scopes", () => { + // Catches regressions where the regex accidentally widens to + // `:` products that Sentry doesn't actually define — + // e.g. `release:read` (no `release` namespace; the real scope is + // `project:releases`) or `alerts:admin` (no admin tier). + expect( + extractRequiredScopes( + "Not real: release:read release:write alerts:admin team:superuser" + ) + ).toEqual([]); + }); +});