From 68808e3fc156d5d054d409a732dab8d9a893b8d5 Mon Sep 17 00:00:00 2001 From: Burak Yigit Kaya Date: Mon, 27 Apr 2026 18:11:49 +0000 Subject: [PATCH] fix(api): guard listOrganizations against non-array SDK responses (CLI-1CQ) The @sentry/api SDK returns an empty object {} (not []) when the response body is empty, has Content-Length: 0, or status 204; and returns a ReadableStream when Content-Type is missing. The unsafe `as unknown as SentryOrganization[]` cast in listOrganizationsInRegion masked the type mismatch, causing `TypeError: l.map is not a function` in the self-hosted fallback path of listOrganizationsUncached. Self-hosted instances behind reverse proxies (nginx, Cloudflare, WAFs) commonly trigger this by stripping bodies or wrapping responses. Add Array.isArray guard after unwrapResult with a descriptive ApiError that names the region URL and the likely cause. --- AGENTS.md | 24 ++++----- src/lib/api/organizations.ts | 8 +++ test/lib/api/organizations.test.ts | 81 ++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 12 deletions(-) create mode 100644 test/lib/api/organizations.test.ts diff --git a/AGENTS.md b/AGENTS.md index 5cb238380..57a946b78 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1010,7 +1010,7 @@ mock.module("./some-module", () => ({ * **Sentry API: events require org+project, issues have legacy global endpoint**: Sentry API scoping/auth quirks: (1) Events require org+project (\`/projects/{org}/{project}/events/{id}/\`); issues use legacy global \`/api/0/issues/{id}/\`; traces need only org. Cross-project search via Discover \`/organizations/{org}/events/?query=id:{eventId}\`. (2) \`/users/me/\` returns 403 for OAuth tokens — use \`/auth/\` instead (all token types, control silo). \`getControlSiloUrl()\` routes; \`SentryUserSchema\` uses \`.passthrough()\` since \`/auth/\` only requires \`id\`. (3) Chunk upload endpoint returns camelCase (\`chunkSize\`, \`chunksPerRequest\`, \`maxRequestSize\`, \`hashAlgorithm\`); \`AssembleResponse\` also camelCase — exception to snake\_case convention. -* **Sentry CLI authenticated fetch architecture with response caching**: Authenticated fetch (\`createAuthenticatedFetch\` in \`src/lib/sentry-client.ts\`): auth headers, 30s \`REQUEST\_TIMEOUT\_MS\` default, retry max 2, 401 refresh, span tracing. Dual input: SDK \`Request\` vs \`(url, init)\`. \*\*Body-reuse:\*\* \`buildAttemptFactory\` yields fresh \`(input, init)\` per attempt. \`Request\` clones per attempt. \`FormData\`/\`Blob\`/\`URLSearchParams\` pass through — fetch re-derives multipart boundary / re-streams per call. Only bare \`ReadableStream\` needs materialization to ArrayBuffer. \*\*Do NOT materialize FormData\*\* — strips auto-negotiated \`Content-Type: multipart/form-data; boundary=...\` and breaks chunk upload. \*\*Timeouts:\*\* internal aborts tagged \`INTERNAL\_TIMEOUT\_MARKER\` Symbol; last attempt throws \`TimeoutError\`. Per-endpoint \`ENDPOINT\_TIMEOUT\_OVERRIDES\` (e.g. \`/autofix/\` 120s). Test hooks \`\_\_resolveRequestTimeoutMsForTests\`, \`\_\_injectTimeoutOverrideForTests\`. Response cache: \`http-cache-semantics\` RFC 7234 at \`~/.sentry/cache/responses/\`; GET 2xx only. +* **Sentry CLI authenticated fetch architecture with response caching**: Authenticated fetch (\`createAuthenticatedFetch\` in \`src/lib/sentry-client.ts\`): auth headers, 30s \`REQUEST\_TIMEOUT\_MS\`, retry max 2, 401 refresh, span tracing. Dual input: SDK \`Request\` vs \`(url, init)\`. Body-reuse: \`buildAttemptFactory\` yields fresh \`(input, init)\` per attempt; \`Request\` clones per attempt; \`FormData\`/\`Blob\`/\`URLSearchParams\` pass through (fetch re-derives multipart boundary). Only bare \`ReadableStream\` needs materialization. Do NOT materialize FormData — strips auto-negotiated \`Content-Type: multipart/form-data; boundary=...\`. Timeouts: internal aborts tagged \`INTERNAL\_TIMEOUT\_MARKER\` Symbol; last attempt throws \`TimeoutError\`. Per-endpoint \`ENDPOINT\_TIMEOUT\_OVERRIDES\` (e.g. \`/autofix/\` 120s). Response cache: \`http-cache-semantics\` RFC 7234 at \`~/.sentry/cache/responses/\`; GET 2xx only. On 4xx/5xx, \`apiRequestToRegion\` attaches allow-listed response headers to Sentry scope as \`api\_response\_headers\` context for empty-detail triage. * **Sentry CLI resolve-target cascade has 5 priority levels with env var support**: resolve-target.ts cascade has 5 priority levels: (1) Explicit CLI flags, (2) SENTRY\_ORG/SENTRY\_PROJECT env vars, (3) SQLite config defaults, (4) DSN auto-detection, (5) Directory name inference. SENTRY\_PROJECT supports combo notation \`org/project\` — when used, SENTRY\_ORG is ignored. If combo parse fails (e.g. \`org/\`), the entire value is discarded. \`resolveFromEnvVars()\` helper is injected into all four resolution functions. @@ -1025,14 +1025,11 @@ mock.module("./some-module", () => ({ ### Gotcha - -* **Bun bytecode: true crashes esbuild→compile ESM bundles (Bun 1.3.11)**: Bun build flags for compiled CLI (\`script/build.ts\`): (1) Do NOT enable \`bytecode: true\` with esbuild→\`Bun.build({ compile })\` pipeline (Bun 1.3.11). Crashes with \`TypeError: Expected CommonJS module to have a function wrapper\`, exit 0, no output. Upstream: oven-sh/bun#21097, #23490. (2) Pass \`autoloadDotenv: false\` and \`autoloadBunfig: false\` — otherwise a user's \`.env\`/\`bunfig.toml\` silently injects into \`process.env\` (e.g., Next.js \`.env.local\` could override stored OAuth token). Shell env vars still work; suggest direnv for dir-scoped vars. - - -* **Bun mock.module() leaks globally across test files in same process**: Bun's mock.module() replaces modules globally and leaks across test files in the same process. Solution: tests using mock.module() must run in a separate \`bun test\` invocation. In package.json, use \`bun run test:unit && bun run test:isolated\` instead of \`bun test\`. The \`test/isolated/\` directory exists for these tests. This was the root cause of ~100 test failures (getsentry/cli#258). + +* **@sentry/api SDK can return non-array data for empty/edge responses**: \`@sentry/api\` SDK (in \`node\_modules/@sentry/api/dist/index.js\`) returns \`data = {}\` (not \`\[]\`) when response body is empty, has \`Content-Length: 0\`, or status 204; and returns a \`ReadableStream\` when \`Content-Type\` is missing. \`unwrapResult\` from \`src/lib/api/infrastructure.ts\` returns \`data\` as-is, and \`as unknown as SentryX\[]\` casts silently lie. Always guard array-typed SDK results with \`Array.isArray(data)\` before \`.map()\` — applied in \`listOrganizationsInRegion\` (CLI-1CQ). Self-hosted instances behind reverse proxies (nginx, Cloudflare, WAFs) commonly trigger this by stripping bodies or wrapping responses. Throw a descriptive \`ApiError\` on mismatch rather than letting \`TypeError: x.map is not a function\` bubble up minified. - -* **Bun test files share globalThis.fetch — mock counters leak across file boundaries**: Bun runs all test files in one process with shared \`globalThis.fetch\`. A test that swaps \`globalThis.fetch = myCountingMock\` and asserts on \`callCount\` can see foreign calls from async work leaked by earlier test files (e.g. the CLI's own Sentry telemetry, or pending retries that outlive their test's \`afterEach\`). CI flake symptom: \`expect(callCount).toBe(2)\` fails with \`Received: 7\`, with debug logs showing stray URLs like \`/api/0/organizations/1/\` and \`/api/0/projects/1/4510776311808000/\` (the CLI's telemetry project ID) between your expected calls. Fix: scope every fetch mock to a per-test URL marker and delegate foreign URLs to the captured \`originalFetch\` (preload.ts blocker). Pattern \`scopedFetchMock(marker, handler)\` lives in \`test/lib/sentry-client.test.ts\`. Reference: PR #832, CI run 24835339085. + +* **Bun bytecode: true crashes esbuild→compile ESM bundles (Bun 1.3.11)**: Bun build flags for compiled CLI (\`script/build.ts\`): (1) Do NOT enable \`bytecode: true\` with esbuild→\`Bun.build({ compile })\` pipeline. Still broken on Bun 1.3.13 — crashes \`TypeError: Expected CommonJS module to have a function wrapper\` at entry.instantiate (esbuild emits ESM; bytecode loader mis-caches as CJS). Exit 0, no output. Upstream: oven-sh/bun#21097, #23490. (2) Pass \`autoloadDotenv: false\` and \`autoloadBunfig: false\` — otherwise user's \`.env\`/\`bunfig.toml\` silently injects into \`process.env\` (e.g. Next.js \`.env.local\` could override stored OAuth token). Shell env vars still work; suggest direnv for dir-scoped vars. * **dist/bin.cjs runtime Node version check must match engines.node**: Node 20 dropped; \`engines.node >=22.12\` matches Astro 6 floor. CI \`Build npm Package\` matrix \`\["22","24"]\`. Docs build jobs pin \`actions/setup-node@v6\` with \`node-version: "24"\` after \`setup-bun\` for astro's node shebang. The npm package's \`dist/bin.cjs\` (from \`script/bundle.ts\`) contains an inline Node guard that MUST match \`engines.node\`. Simple \`parseInt(process.versions.node) < 22\` misses 22.0.0–22.11.x — use explicit major+minor: \`let v=process.versions.node.split('.').map(Number);if(v\[0]<22||(v\[0]===22&\&v\[1]<12)){...}\`. When bumping, update BIN\_WRAPPER string AND error message in lockstep. Without \`engine-strict=true\`, npm only warns — the runtime guard is real enforcement. @@ -1043,6 +1040,9 @@ mock.module("./some-module", () => ({ * **script/generate-api-schema.ts regex is brittle against SDK bundler output changes**: \`script/generate-api-schema.ts\` parses \`node\_modules/@sentry/api/dist/index.js\` with a regex (\`/var (\w+) = \\(options\S\*\\) => \\(options\S\*client \\?\\? client\\)\\.(\w+)\\(/g\`) to map SDK function names to URL+method pairs, producing \`src/generated/api-schema.json\`. If the SDK changes its generator's bundle format (e.g., switches to \`const\`, arrow vs function, different client-selection pattern), schema generation silently produces empty \`fn\` fields. When bumping \`@sentry/api\`, verify \`sentry schema\` output still populates function names. \`src/generated/api-schema.json\` is gitignored — regenerates on every dev/build/typecheck via \`bun run generate:schema\`. + +* **Sentry /auth/ endpoint returns 400 (not 401) for Bearer tokens pre-fix**: Sentry \`/auth/\` endpoint historically returned \*\*400 with empty body\*\* for valid Bearer tokens — \`AuthIndexEndpoint\` excluded \`UserAuthTokenAuthentication\`. Fixed server-side by getsentry/sentry#112853 (release 26.4.1, Apr 22 2026), now fully rolled out to SaaS. Fix adds \`UserAuthTokenAuthentication\` only — \`OrgAuthTokenAuthentication\` was NOT added, and \`UserAuthTokenAuthentication.accepts\_auth\` rejects \`sntrys\_\` prefix. So \`sntrys\_\` org-auth-tokens always fail on \`/auth/\` (and whoami is semantically meaningless for org tokens — no user). CLI short-circuits \`sntrys\_\` via \`classifySentryToken\` before the call, throwing \`ResolutionError\` with \`sentry auth status\` hint. The 400-translation path in \`translateWhoamiApiError\` was dropped post-rollout (PR #841) as vestigial — anomalous 400s now bubble as \`ApiError\` and are diagnosable via the \`api\_response\_headers\` Sentry context. \`sentry auth status\` verifies via \`listOrganizationsUncached()\` so it works with org tokens. + * **Source Map v3 spec allows null entries in sources array**: The Source Map v3 spec allows \`null\` entries in the \`sources\` array, and bundlers like esbuild actually produce them. Any code iterating over \`sources\` and calling string methods (e.g., \`.replaceAll()\`) must guard against null: \`map.sources.map((s) => typeof s === "string" ? s.replaceAll("\\\\", "/") : s)\`. Without the guard, \`null.replaceAll()\` throws \`TypeError\`. This applies to \`src/lib/sourcemap/debug-id.ts\` and any future sourcemap manipulation code. @@ -1060,17 +1060,17 @@ mock.module("./some-module", () => ({ * **Non-essential DB cache writes should be guarded with try-catch**: Non-essential DB cache writes (e.g., \`setUserInfo()\`, \`setInstallInfo()\`) must be wrapped in try-catch so a broken/read-only DB doesn't crash a command whose primary operation succeeded. Pattern: \`try { setInstallInfo(...) } catch { log.debug(...) }\`. In login.ts, \`getCurrentUser()\` failure after token save must not block auth — log warning, continue. In upgrade.ts, \`setInstallInfo\` after legacy detection is guarded same way. Exception: \`getUserRegions()\` failure should \`clearAuth()\` and fail hard (indicates invalid token). This is enforced by BugBot reviews — any \`setInstallInfo\`/\`setUserInfo\` call outside setup.ts's \`bestEffort()\` wrapper needs its own try-catch. - -* **Regenerating @sentry/core + @sentry/node-core patches for SDK version bumps**: Bumping sentry-javascript exact-pinned version: (1) Delete old \`patches/@sentry%2F{core,node-core}@OLD.patch\` and remove \`patchedDependencies\` entries from \`package.json\`. (2) Bump \`@sentry/node-core\` in devDependencies, \`bun install\`. (3) \`bun patch @sentry/node-core\`, edit \`node\_modules/@sentry/node-core/build/{cjs,esm}/light/index.js\` to strip unused exports, \`bun patch --commit\`. (4) Repeat for \`@sentry/core\` editing \`build/{cjs,esm}/index.js\` — strip unused \`require()\`s AND their \`exports.X = Y;\` lines in CJS, strip names from single-line ESM export. (5) Verify with \`bun install && bun run typecheck && bun test\`. \*\*Critical\*\*: before stripping any \`core\` export, grep \`node-core/build/{cjs,esm}/light/sdk.js\` for runtime usage — e.g. 10.50.0+ calls \`spanStreamingIntegration()\` at runtime when \`traceLifecycle === 'stream'\`; stripping causes \`SyntaxError: Export named 'spanStreamingIntegration' not found\` on first \`Sentry.init()\`. Safe to strip from node-core/light re-export surface. \*\*Also\*\*: when running \`bun patch --commit\`, remove any \`.bun-tag-\\` hunks at the top of the generated patch file — they embed a stray empty marker file into every install (\`node\_modules/@sentry/core/.bun-tag-\*\`). Bun creates its own tag on install regardless; the one from the patch is redundant and noisy. - * **Sentry CLI command docs are auto-generated from Stricli route tree with CI freshness check**: Sentry CLI command docs are auto-generated from Stricli route tree: Docs in \`docs/src/content/docs/commands/\*.md\` and skill files in \`plugins/sentry-cli/skills/sentry-cli/references/\*.md\` are generated via \`bun run generate:docs\`. Content between \`\\` markers is regenerated; hand-written examples go in \`docs/src/fragments/commands/\`. CI checks \`check:command-docs\` and \`check:skill\` fail if stale. Run generators after changing command parameters/flags/docs. * **Stricli buildCommand output config injects json flag into func params**: Stricli command gotchas: (1) In \`func()\` handlers use \`this.stdout\`/\`this.stderr\` directly — NOT \`this.process.stdout\`. \`SentryContext\` has \`process\` and \`stdout\`/\`stderr\` as separate top-level properties; test mocks omit full \`process\` so \`this.process.stdout\` throws \`TypeError\` at runtime (TS doesn't catch). (2) \`output: { json: true, human: formatFn }\` auto-injects \`--json\`/\`--fields\` flags — type flags explicitly (\`flags: { json?: boolean }\`). Commands with interactive side effects (prompts, QR codes) should check \`flags.json\` and skip. (3) Route maps with \`defaultCommand\` blend the default command's flags into subcommand completions — completion tests must track \`hasDefaultCommand\` and skip strict subcommand-matching. + +* **Token-type classification via literal prefix match (classifySentryToken)**: \`src/lib/token-type.ts\` \`classifySentryToken(token)\` returns \`'org-auth-token'\` (\`sntrys\_\` prefix), \`'user-auth-token'\` (\`sntryu\_\` prefix), or \`'oauth-or-legacy'\` (OAuth, legacy PATs, JWT-shaped). Case-sensitive literal \`startsWith\` — matches Sentry backend's \`SENTRY\_ORG\_AUTH\_TOKEN\_PREFIX\`. Use to short-circuit commands where a token type is semantically invalid (e.g. \`whoami\` with org token) before a confusing API failure. \`getAuthToken()\` from \`db/auth\` returns the effective token (env + DB fallback). + ### Preference -* **PR workflow: address review comments, resolve threads, wait for CI**: PR workflow: (1) wait for CI; (2) check unresolved review comments via \`gh api repos/.../pulls/N/comments\`; (3) fix in follow-up commits (not amends); (4) reply explaining fix; (5) resolve thread via \`gh api graphql resolveReviewThread\`; (6) push + re-check CI. BugBot/Seer/Warden/Cursor bots post new comments per-commit and frequently catch real bugs in fix commits themselves — always re-check after each push. \*\*Dispatch a subagent review before declaring merge-ready\*\* — self-assessment is unreliable; has caught real backwards-compat and error-path bugs. After applying review fixes, plan for ANOTHER critical review pass on the HEAD commit. Branches: \`fix/\*\` or \`feat/\*\`. Style: \`Array.from(set)\` over spreads; 'allowlist' not 'whitelist'; \`arr.at(-1)\` over index math. Reviewer questions may be inquiries — confirm intent before changing. Multi-fix PRs: split into independent PRs off \`main\`. +* **PR workflow: address review comments, resolve threads, wait for CI**: PR workflow: (1) wait for CI; (2) check unresolved comments via \`gh api repos/.../pulls/N/comments\`; (3) fix in follow-up commits (NEVER amend a pushed commit without explicit user request + force push); (4) reply explaining fix; (5) resolve thread via \`gh api graphql resolveReviewThread\`; (6) push + re-check CI. BugBot/Seer/Warden/Cursor post new comments per-commit and often catch bugs in fix commits — re-check after each push. Dispatch a subagent review before declaring merge-ready; has caught real backwards-compat, error-path, and patch-hygiene bugs. Branches: \`fix/\*\` or \`feat/\*\`. Style: \`Array.from(set)\` over spreads; 'allowlist' not 'whitelist'; \`arr.at(-1)\` over index math. Reviewer questions may be inquiries — confirm intent before changing. diff --git a/src/lib/api/organizations.ts b/src/lib/api/organizations.ts index 778cc9d7e..7b6504a2d 100644 --- a/src/lib/api/organizations.ts +++ b/src/lib/api/organizations.ts @@ -64,6 +64,14 @@ export async function listOrganizationsInRegion( try { const data = unwrapResult(result, "Failed to list organizations"); + if (!Array.isArray(data)) { + throw new ApiError( + "Failed to list organizations: unexpected response format", + 0, + `Expected an array from ${regionUrl}/api/0/organizations/ but received ${typeof data}. ` + + "This may indicate an incompatible self-hosted Sentry version or a proxy interfering with the response." + ); + } return data as unknown as SentryOrganization[]; } catch (error) { // Enrich 403 errors with contextual guidance (CLI-89, 24 users). diff --git a/test/lib/api/organizations.test.ts b/test/lib/api/organizations.test.ts new file mode 100644 index 000000000..597fe9908 --- /dev/null +++ b/test/lib/api/organizations.test.ts @@ -0,0 +1,81 @@ +/** + * Tests for listOrganizationsInRegion — guards against non-array SDK responses. + * + * CLI-1CQ: self-hosted instances can return non-array data from + * GET /api/0/organizations/ when a reverse proxy or WAF interferes. + */ + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { listOrganizationsInRegion } from "../../../src/lib/api/organizations.js"; +import { setAuthToken } from "../../../src/lib/db/auth.js"; +import { ApiError } from "../../../src/lib/errors.js"; +import { mockFetch, useTestConfigDir } from "../../helpers.js"; + +useTestConfigDir("org-api-test-"); + +let originalFetch: typeof globalThis.fetch; + +beforeEach(async () => { + originalFetch = globalThis.fetch; + await setAuthToken("fake-token-for-test", 3600); +}); + +afterEach(() => { + globalThis.fetch = originalFetch; +}); + +describe("listOrganizationsInRegion", () => { + test("returns organizations when API returns a valid array", async () => { + globalThis.fetch = mockFetch( + async () => + new Response( + JSON.stringify([{ id: "1", slug: "test-org", name: "Test Org" }]), + { + status: 200, + headers: { "Content-Type": "application/json" }, + } + ) + ); + + const orgs = await listOrganizationsInRegion("https://sentry.example.com"); + expect(orgs).toHaveLength(1); + expect(orgs[0].slug).toBe("test-org"); + }); + + test("throws ApiError when API returns an empty object instead of array", async () => { + globalThis.fetch = mockFetch( + async () => + new Response(JSON.stringify({}), { + status: 200, + headers: { "Content-Type": "application/json" }, + }) + ); + + await expect( + listOrganizationsInRegion("https://sentry.example.com") + ).rejects.toThrow(ApiError); + + try { + await listOrganizationsInRegion("https://sentry.example.com"); + } catch (error) { + expect(error).toBeInstanceOf(ApiError); + const apiError = error as ApiError; + expect(apiError.message).toContain("unexpected response format"); + expect(apiError.detail).toContain("sentry.example.com"); + } + }); + + test("throws ApiError when API returns empty body", async () => { + globalThis.fetch = mockFetch( + async () => + new Response("", { + status: 200, + headers: { "Content-Type": "application/json" }, + }) + ); + + await expect( + listOrganizationsInRegion("https://sentry.example.com") + ).rejects.toThrow(ApiError); + }); +});