From 9a22d3c542d46ffa09fa58e188551cad9ec4c9d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20Beteg=C3=B3n?= Date: Mon, 28 Sep 2026 12:19:12 +0200 Subject: [PATCH 1/4] fix(auth): reject malformed bearer credentials --- apps/cli-docs/src/fragments/commands/auth.md | 13 + packages/cli/src/lib/api/preprod-artifacts.ts | 5 +- packages/cli/src/lib/auth-header.ts | 18 ++ packages/cli/src/lib/db/auth.ts | 38 ++- packages/cli/src/lib/docs-service.ts | 3 +- packages/cli/src/lib/error-reporting.ts | 7 +- packages/cli/src/lib/init/wizard-runner.ts | 3 +- packages/cli/src/lib/sentry-client.ts | 26 +- packages/cli/test/e2e/auth.test.ts | 14 + packages/cli/test/e2e/library.test.ts | 63 +++- .../test/lib/api/preprod-artifacts.test.ts | 23 +- packages/cli/test/lib/docs-service.test.ts | 20 ++ .../cli/test/lib/init/wizard-runner.test.ts | 17 ++ .../cli/test/lib/sentry-client.auth.test.ts | 282 ++++++++++++++++++ 14 files changed, 498 insertions(+), 34 deletions(-) create mode 100644 packages/cli/src/lib/auth-header.ts create mode 100644 packages/cli/test/lib/sentry-client.auth.test.ts diff --git a/apps/cli-docs/src/fragments/commands/auth.md b/apps/cli-docs/src/fragments/commands/auth.md index ecf464b5c4..ee2c303b3a 100644 --- a/apps/cli-docs/src/fragments/commands/auth.md +++ b/apps/cli-docs/src/fragments/commands/auth.md @@ -131,3 +131,16 @@ override this precedence and force environment tokens to win, set `SENTRY_FORCE_ENV_TOKEN=1`. When a token comes from an environment variable, the CLI skips expiry checks and automatic refresh. + +## Invalid Token Formatting + +Tokens must be a single line of printable ASCII characters, without spaces. +The CLI rejects embedded whitespace, control characters, and non-ASCII +characters before sending an authenticated request. It does not join split +lines or send only the first line of a token. + +If you see "Invalid authentication token", copy the complete token again into +the configuration that supplies it. For environment tokens, check +`SENTRY_AUTH_TOKEN` (or the legacy `SENTRY_TOKEN`). For stored credentials, run +`sentry auth login` to replace them. A token rejected for formatting exits with +code `12` (`AUTH_INVALID`). diff --git a/packages/cli/src/lib/api/preprod-artifacts.ts b/packages/cli/src/lib/api/preprod-artifacts.ts index c6ff7c614a..f8acc968d6 100644 --- a/packages/cli/src/lib/api/preprod-artifacts.ts +++ b/packages/cli/src/lib/api/preprod-artifacts.ts @@ -27,6 +27,7 @@ import { string, tuple, } from "valibot"; +import { formatAuthHeader } from "../auth-header.js"; import { customFetch } from "../custom-ca.js"; import { getAuthToken } from "../db/auth.js"; import { ApiError, TimeoutError, ValidationError } from "../errors.js"; @@ -154,7 +155,7 @@ export async function downloadBuildArtifact( if (isRegionOrigin(url, regionUrl)) { const token = getAuthToken(); if (token) { - headers.Authorization = `Bearer ${token}`; + headers.Authorization = formatAuthHeader(token); } } @@ -565,7 +566,7 @@ export async function openSnapshotArchive( const headers: Record = {}; const token = getAuthToken(); if (token) { - headers.Authorization = `Bearer ${token}`; + headers.Authorization = formatAuthHeader(token); } const response = await customFetch(url, { headers }); if (!response.ok) { diff --git a/packages/cli/src/lib/auth-header.ts b/packages/cli/src/lib/auth-header.ts new file mode 100644 index 0000000000..cef47dd03d --- /dev/null +++ b/packages/cli/src/lib/auth-header.ts @@ -0,0 +1,18 @@ +/** Validated Authorization values for the selected Sentry credential. */ + +import { AuthError } from "./errors.js"; + +/** Bearer tokens are opaque, but cannot contain whitespace or non-ASCII bytes. */ +const INVALID_TOKEN_CHARACTER_PATTERN = /[^\x21-\x7e]/; + +/** Validate a selected credential before constructing its Authorization value. */ +export function formatAuthHeader(token: string): string { + if (!token || INVALID_TOKEN_CHARACTER_PATTERN.test(token)) { + throw new AuthError( + "invalid", + "Invalid authentication token. Copy it again as a single line without spaces or control characters, " + + "or run 'sentry auth login' to replace stored credentials." + ); + } + return `Bearer ${token}`; +} diff --git a/packages/cli/src/lib/db/auth.ts b/packages/cli/src/lib/db/auth.ts index df2164508d..da95a580a0 100644 --- a/packages/cli/src/lib/db/auth.ts +++ b/packages/cli/src/lib/db/auth.ts @@ -3,6 +3,7 @@ */ import { createHash } from "node:crypto"; +import { formatAuthHeader } from "../auth-header.js"; import { DEFAULT_SENTRY_URL, getConfiguredSentryUrl } from "../constants.js"; import { getEnv } from "../env.js"; import { getEnvTokenHost } from "../env-token-host.js"; @@ -609,23 +610,9 @@ async function performTokenRefresh( const { refreshAccessToken } = await import("../oauth.js"); const { AuthError } = await import("../errors.js"); + let tokenResponse: Awaited>; try { - const tokenResponse = await refreshAccessToken(storedRefreshToken); - const now = Date.now(); - const expiresAt = now + tokenResponse.expires_in * 1000; - - await setAuthToken( - tokenResponse.access_token, - tokenResponse.expires_in, - tokenResponse.refresh_token ?? storedRefreshToken - ); - - return { - token: tokenResponse.access_token, - refreshed: true, - expiresAt, - expiresIn: tokenResponse.expires_in, - }; + tokenResponse = await refreshAccessToken(storedRefreshToken); } catch (error) { // Only clear auth on explicit rejection, not network errors if (error instanceof AuthError) { @@ -633,6 +620,25 @@ async function performTokenRefresh( } throw error; } + + // Validate before SQLite can truncate NUL-containing credentials or replace + // the current session with a malformed response. Keep that session intact. + formatAuthHeader(tokenResponse.access_token); + const now = Date.now(); + const expiresAt = now + tokenResponse.expires_in * 1000; + + await setAuthToken( + tokenResponse.access_token, + tokenResponse.expires_in, + tokenResponse.refresh_token ?? storedRefreshToken + ); + + return { + token: tokenResponse.access_token, + refreshed: true, + expiresAt, + expiresIn: tokenResponse.expires_in, + }; } /** Get a valid token, refreshing if needed. Use force=true after 401 responses. */ diff --git a/packages/cli/src/lib/docs-service.ts b/packages/cli/src/lib/docs-service.ts index 6739344bdb..db0dc2b27f 100644 --- a/packages/cli/src/lib/docs-service.ts +++ b/packages/cli/src/lib/docs-service.ts @@ -1,3 +1,4 @@ +import { formatAuthHeader } from "./auth-header.js"; import { customFetch } from "./custom-ca.js"; import { refreshToken } from "./db/auth.js"; import type { DocsProjectContext } from "./docs-context.js"; @@ -25,7 +26,7 @@ async function postDocs( const response = await customFetch(`${MASTRA_API_URL}${path}`, { body: JSON.stringify(body), headers: { - Authorization: `Bearer ${token}`, + Authorization: formatAuthHeader(token), "Content-Type": "application/json", }, method: "POST", diff --git a/packages/cli/src/lib/error-reporting.ts b/packages/cli/src/lib/error-reporting.ts index 62ed1bc27c..69b2448e75 100644 --- a/packages/cli/src/lib/error-reporting.ts +++ b/packages/cli/src/lib/error-reporting.ts @@ -87,10 +87,9 @@ export function classifySilenced(error: unknown): SilenceReason | null { // // All AuthError reasons are expected auth states the user must act on, not // CLI bugs: `not_authenticated` (no token), `expired` (token aged out), and - // `invalid` (a bad/insufficiently-scoped token the user supplied). `invalid` - // is now only thrown for a genuine 401/403 (see auth/login.ts) — transient - // network/server failures no longer masquerade as it — so it is safe to - // silence alongside the others (CLI-19). + // `invalid` (malformed credentials or a bad/insufficiently-scoped token). + // Transient network/server failures do not masquerade as invalid tokens, + // so it is safe to silence these alongside the others (CLI-19). if (error instanceof AuthError) { return "auth_expected"; } diff --git a/packages/cli/src/lib/init/wizard-runner.ts b/packages/cli/src/lib/init/wizard-runner.ts index 9bdbc60e4a..e70f50ccb2 100644 --- a/packages/cli/src/lib/init/wizard-runner.ts +++ b/packages/cli/src/lib/init/wizard-runner.ts @@ -20,6 +20,7 @@ import { getTraceData, setTag, } from "@sentry/node-core/light"; +import { formatAuthHeader } from "../auth-header.js"; import { formatBanner } from "../banner.js"; import { CLI_VERSION } from "../constants.js"; import { customFetch } from "../custom-ca.js"; @@ -1051,7 +1052,7 @@ export async function runWizard(initialOptions: WizardOptions): Promise { const client = new MastraClient({ baseUrl: MASTRA_API_URL, retries: 0, - headers: token ? { Authorization: `Bearer ${token}` } : {}, + headers: token ? { Authorization: formatAuthHeader(token) } : {}, abortSignal: abortController.signal, fetch: ((url, init) => { const traceData = getTraceData(); diff --git a/packages/cli/src/lib/sentry-client.ts b/packages/cli/src/lib/sentry-client.ts index bb8b3653fa..e945bcc9d0 100644 --- a/packages/cli/src/lib/sentry-client.ts +++ b/packages/cli/src/lib/sentry-client.ts @@ -10,6 +10,7 @@ import { setTimeout as sleepMs } from "node:timers/promises"; import { getTraceData } from "@sentry/node-core/light"; +import { formatAuthHeader } from "./auth-header.js"; import { maybeWarnEnvTokenIgnored } from "./auth-hint.js"; import { computeInvalidationPrefixes } from "./cache-keys.js"; import { @@ -25,7 +26,7 @@ import { } from "./custom-ca.js"; import { applyCustomHeaders } from "./custom-headers.js"; import { getAuthToken, refreshToken } from "./db/auth.js"; -import { ApiError, HostScopeError, TimeoutError } from "./errors.js"; +import { ApiError, AuthError, HostScopeError, TimeoutError } from "./errors.js"; import { logger } from "./logger.js"; import { clearLastCacheHitAge, @@ -145,7 +146,7 @@ function prepareHeaders( const sourceHeaders = init?.headers ?? (input instanceof Request ? input.headers : undefined); const headers = new Headers(sourceHeaders); - headers.set("Authorization", `Bearer ${token}`); + headers.set("Authorization", formatAuthHeader(token)); if (!headers.has("User-Agent")) { headers.set("User-Agent", getUserAgent()); } @@ -181,17 +182,26 @@ async function handleUnauthorized(headers: Headers): Promise { // the effective auth source, or returns the env token without refresh when // SENTRY_FORCE_ENV_TOKEN is set. If the token can't be refreshed (env token, // no refresh token), `refreshed` is false and the 401 propagates. + let newToken: string; try { - const { token: newToken, refreshed } = await refreshToken({ force: true }); - if (refreshed) { - headers.set("Authorization", `Bearer ${newToken}`); - headers.set(RETRY_MARKER_HEADER, "1"); - return true; + const result = await refreshToken({ force: true }); + if (!result.refreshed) { + return false; } + newToken = result.token; } catch (error) { + if (error instanceof AuthError && error.reason === "invalid") { + throw error; + } log.debug("Token refresh failed after 401", error); + return false; } - return false; + + // Invalid refreshed credentials must propagate as an auth error, rather + // than being swallowed by the best-effort refresh catch above. + headers.set("Authorization", formatAuthHeader(newToken)); + headers.set(RETRY_MARKER_HEADER, "1"); + return true; } /** Link an external abort signal to an AbortController */ diff --git a/packages/cli/test/e2e/auth.test.ts b/packages/cli/test/e2e/auth.test.ts index 14ccefec0d..ec5f66a834 100644 --- a/packages/cli/test/e2e/auth.test.ts +++ b/packages/cli/test/e2e/auth.test.ts @@ -114,6 +114,20 @@ describe("sentry auth login --token", () => { }); describe("sentry auth whoami", () => { + test("rejects a split stored token without exposing it in JSON mode", async () => { + const token = "sntryu_SYNTHETIC-PREFIX\nSYNTHETIC-SECRET-TAIL"; + await ctx.setAuthToken(token); + + const result = await ctx.run(["auth", "whoami", "--json"]); + const output = result.stdout + result.stderr; + + expect(result.exitCode).toBe(EXIT.AUTH_INVALID); + expect(output).toContain("single line"); + expect(output).not.toContain("SYNTHETIC-PREFIX"); + expect(output).not.toContain("SYNTHETIC-SECRET-TAIL"); + expect(output).not.toContain("Headers.set"); + }); + test("requires authentication", async () => { const result = await ctx.run(["auth", "whoami"]); diff --git a/packages/cli/test/e2e/library.test.ts b/packages/cli/test/e2e/library.test.ts index 3f3b5fa78b..6000e60abb 100644 --- a/packages/cli/test/e2e/library.test.ts +++ b/packages/cli/test/e2e/library.test.ts @@ -10,9 +10,11 @@ import { spawn } from "node:child_process"; import { existsSync } from "node:fs"; -import { readFile } from "node:fs/promises"; +import { readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { beforeAll, describe, expect, test } from "vitest"; +import { EXIT } from "../../src/lib/errors.js"; +import { useTestConfigDir } from "../helpers.js"; import { BUNDLE_INDEX_PATH, BUNDLE_TYPES_PATH, @@ -86,6 +88,8 @@ async function runNodeScriptOk( } describe("library mode (bundled)", () => { + const getConfigDir = useTestConfigDir("e2e-library-"); + beforeAll(async () => { await ensureBundleBuilt(); }, 60_000); @@ -201,6 +205,63 @@ describe("library mode (bundled)", () => { // --- Typed SDK --- + test("sourcemap upload rejects a split token without leaking it or fetching", async () => { + const directory = getConfigDir(); + await writeFile( + join(directory, "app.js"), + "console.log('fixture');\n//# sourceMappingURL=app.js.map\n" + ); + await writeFile( + join(directory, "app.js.map"), + JSON.stringify({ + version: 3, + sources: ["app.ts"], + sourcesContent: ["console.log('fixture');"], + names: [], + mappings: "AAAA", + }) + ); + const claim = Buffer.from( + JSON.stringify({ org: "test-org", url: "http://localhost:9000" }) + ).toString("base64"); + const token = `sntrys_${claim}_ab\nSYNTHETIC-SECRET-TAIL`; + const { stdout, stderr } = await runNodeScriptOk(` + const { createSentrySDK, SentryError } = require('./dist/index.cjs'); + let fetches = 0; + globalThis.fetch = async () => { + fetches++; + throw new Error('Unexpected request'); + }; + const sdk = createSentrySDK({ + token: ${JSON.stringify(token)}, + url: 'http://localhost:9000', + org: 'test-org', + project: 'test-project', + cwd: ${JSON.stringify(directory)}, + }); + sdk.sourcemap.upload({ directory: ${JSON.stringify(directory)} }).then(() => { + console.log(JSON.stringify({ error: false, fetches })); + }).catch(e => { + console.log(JSON.stringify({ + isSentryError: e instanceof SentryError, + exitCode: e.exitCode, + message: e.message, + stderr: e.stderr, + stack: e.stack, + fetches, + })); + }); + `); + const result = JSON.parse(stdout.trim()); + expect(result.isSentryError).toBe(true); + expect(result.exitCode).toBe(EXIT.AUTH_INVALID); + expect(result.message).toContain("single line"); + expect(result.fetches).toBe(0); + expect(stdout + stderr).not.toContain(claim); + expect(stdout + stderr).not.toContain("SYNTHETIC-SECRET-TAIL"); + expect(stdout + stderr).not.toContain("Headers.set"); + }); + test("createSentrySDK() returns object with namespaces", async () => { const { stdout } = await runNodeScriptOk(` const { createSentrySDK } = require('./dist/index.cjs'); diff --git a/packages/cli/test/lib/api/preprod-artifacts.test.ts b/packages/cli/test/lib/api/preprod-artifacts.test.ts index 128e82216a..87ff1bd58b 100644 --- a/packages/cli/test/lib/api/preprod-artifacts.test.ts +++ b/packages/cli/test/lib/api/preprod-artifacts.test.ts @@ -12,7 +12,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { parse, safeParse } from "valibot"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { ApiError, ValidationError } from "../../../src/lib/errors.js"; +import { ApiError, EXIT, ValidationError } from "../../../src/lib/errors.js"; const { customFetchMock, @@ -159,6 +159,27 @@ describe("downloadBuildArtifact", () => { expect(init.headers.Authorization).toBe("Bearer secret-token"); }); + test("rejects a malformed bearer before downloading a build or snapshot", async () => { + getAuthTokenMock.mockReturnValue("synthetic-prefix\nsynthetic-secret-tail"); + for (const download of [ + () => + downloadBuildArtifact( + "https://us.sentry.io", + "https://us.sentry.io/dl/?response_format=ipa", + join(tmpDir, "out.ipa") + ), + () => openSnapshotArchive("my-org", "snap-1"), + ]) { + const error = await download().catch((caught: unknown) => caught); + expect(error).toMatchObject({ + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect(String(error)).not.toContain("synthetic-secret-tail"); + } + expect(customFetchMock).not.toHaveBeenCalled(); + }); + test("does NOT attach the auth token to a cross-origin (signed) URL", async () => { customFetchMock.mockResolvedValue(new Response("X", { status: 200 })); diff --git a/packages/cli/test/lib/docs-service.test.ts b/packages/cli/test/lib/docs-service.test.ts index 73a5218698..6aefae2ba4 100644 --- a/packages/cli/test/lib/docs-service.test.ts +++ b/packages/cli/test/lib/docs-service.test.ts @@ -17,6 +17,26 @@ import { queryDocs } from "../../src/lib/docs-service.js"; import { EXIT, isUserError } from "../../src/lib/errors.js"; describe("queryDocs", () => { + test("rejects a malformed bearer before calling the docs service", async () => { + customFetch.mockClear(); + refreshToken.mockResolvedValue({ + token: "synthetic-prefix\nsynthetic-secret-tail", + }); + + const error = await queryDocs("How do I configure tracing?", { + frameworks: [], + languages: [], + sentryConfigured: false, + }).catch((caught: unknown) => caught); + + expect(error).toMatchObject({ + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect(String(error)).not.toContain("synthetic-secret-tail"); + expect(customFetch).not.toHaveBeenCalled(); + }); + test("explains when Docs AI is unavailable in the service region", async () => { refreshToken.mockResolvedValue({ token: "test-token" }); customFetch.mockResolvedValue({ diff --git a/packages/cli/test/lib/init/wizard-runner.test.ts b/packages/cli/test/lib/init/wizard-runner.test.ts index 3949551cfc..5e2c150a2a 100644 --- a/packages/cli/test/lib/init/wizard-runner.test.ts +++ b/packages/cli/test/lib/init/wizard-runner.test.ts @@ -433,6 +433,23 @@ function hasExpectedInitServiceAuthPolicy(err: unknown): boolean { } describe("runWizard", () => { + test("rejects a malformed bearer before constructing the service client", async () => { + resolveInitContextSpy.mockResolvedValue( + makeContext({ authToken: "synthetic-prefix\nsynthetic-secret-tail" }) + ); + + const error = await runWizard(makeOptions()).catch( + (caught: unknown) => caught + ); + expect(error).toMatchObject({ + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect(String(error)).not.toContain("synthetic-secret-tail"); + expect(capturedClientOptions).toEqual([]); + expect(getWorkflowSpy).not.toHaveBeenCalled(); + }); + test("formats successful results", async () => { await runWizard(makeOptions()); diff --git a/packages/cli/test/lib/sentry-client.auth.test.ts b/packages/cli/test/lib/sentry-client.auth.test.ts new file mode 100644 index 0000000000..96f4ff1a45 --- /dev/null +++ b/packages/cli/test/lib/sentry-client.auth.test.ts @@ -0,0 +1,282 @@ +/** Regression coverage for malformed bearer credentials. */ + +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { shouldAutoAuth } from "../../src/lib/auto-auth.js"; +import { getAuthConfig, setAuthToken } from "../../src/lib/db/auth.js"; +import { setEnv } from "../../src/lib/env.js"; +import { AuthError, EXIT, withAuthGuard } from "../../src/lib/errors.js"; +import { + getSdkConfig, + resetAuthenticatedFetch, +} from "../../src/lib/sentry-client.js"; +import { + extractFetchUrl, + mintSntrysToken, + mockFetch, + resetHostScopingState, + useEnvSandbox, + useTestConfigDir, +} from "../helpers.js"; + +const REGION_URL = "https://us.sentry.io"; +const RESOURCE_URL = `${REGION_URL}/api/0/organizations/synthetic-org/chunk-upload/`; +const ENV_TOKEN_KEYS = ["SENTRY_AUTH_TOKEN", "SENTRY_TOKEN"] as const; +const ORG_TOKEN = mintSntrysToken({ + iat: 1, + url: "https://sentry.io", + org: "synthetic-org", +}); +const MALFORMED_TOKEN = ORG_TOKEN.replace( + "test-secret-tail", + "te\nst-secret-tail" +); + +describe("authenticated fetch bearer validation", () => { + useTestConfigDir("sentry-client-auth-"); + useEnvSandbox([ + ...ENV_TOKEN_KEYS, + "SENTRY_FORCE_ENV_TOKEN", + "SENTRY_HOST", + "SENTRY_URL", + "SENTRY_CLIENT_ID", + ]); + + let originalFetch: typeof globalThis.fetch; + let requests: { url: string; authorization: string | null }[]; + + beforeEach(async () => { + await resetHostScopingState(); + resetAuthenticatedFetch(); + originalFetch = globalThis.fetch; + requests = []; + globalThis.fetch = mockFetch((input, init) => { + requests.push({ + url: extractFetchUrl(input), + authorization: new Headers(init?.headers).get("Authorization"), + }); + return Promise.resolve(new Response("{}", { status: 200 })); + }); + }); + + afterEach(async () => { + setEnv(process.env); + globalThis.fetch = originalFetch; + resetAuthenticatedFetch(); + await resetHostScopingState(); + }); + + function request(): Promise { + return getSdkConfig(REGION_URL).fetch(RESOURCE_URL); + } + + test.each([ + ...ENV_TOKEN_KEYS, + "stored", + ])("rejects an internal LF from %s before any request or auth fallback", async (source) => { + if (source === "stored") { + setAuthToken(MALFORMED_TOKEN); + } else { + process.env[source] = MALFORMED_TOKEN; + } + + const error = await withAuthGuard(request).catch( + (caught: unknown) => caught + ); + expect(error).toBeInstanceOf(AuthError); + const authError = error as AuthError; + expect(authError.reason).toBe("invalid"); + expect(authError.exitCode).toBe(EXIT.AUTH_INVALID); + expect(authError.message).toContain("single line"); + expect(authError.cause).toBeUndefined(); + expect(`${authError.message}\n${authError.stack}`).not.toContain( + MALFORMED_TOKEN + ); + expect(shouldAutoAuth(authError, () => true)).toBe(false); + expect(requests).toEqual([]); + }); + + test.each([ + ["CR", "\r"], + ["NUL", "\0"], + ["tab", "\t"], + ["space", " "], + ["control character", "\x01"], + ["DEL", "\x7f"], + ["non-ASCII byte", "\x80"], + ["non-ByteString character", "\u0100"], + ["surrogate pair", "💥"], + ])("rejects %s in SDK credentials without exposing it", async (_, char) => { + const token = `synthetic-prefix${char}synthetic-secret`; + // SDK options use an in-memory env copy, which preserves even NULs. + // process.env and the SQLite binding truncate NUL-containing strings. + setEnv({ ...process.env, SENTRY_AUTH_TOKEN: token }); + const error = await request().catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(AuthError); + expect(error).toMatchObject({ + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect(String(error)).not.toContain(token); + expect(requests).toEqual([]); + }); + + test.each([ + ["org", ORG_TOKEN], + ["user", `sntryu_${"a".repeat(64)}`], + ["opaque legacy/OAuth", "opaque.legacy_token+with/punctuation=~-"], + ])("preserves a printable %s token", async (_, token) => { + setAuthToken(token); + await request(); + expect(requests).toEqual([ + { url: RESOURCE_URL, authorization: `Bearer ${token}` }, + ]); + }); + + test.each(ENV_TOKEN_KEYS)("preserves outer trimming for %s", async (key) => { + process.env[key] = "\t\n synthetic-token \r\n"; + await request(); + expect(requests[0]?.authorization).toBe("Bearer synthetic-token"); + }); + + test("does not silently trim stored credentials", async () => { + const token = " stored-token "; + setAuthToken(token); + await expect(request()).rejects.toMatchObject({ + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect(requests).toEqual([]); + }); + + test.each( + ENV_TOKEN_KEYS + )("ignores a malformed %s when stored OAuth takes precedence", async (key) => { + process.env[key] = MALFORMED_TOKEN; + setAuthToken("stored-token"); + await request(); + expect(requests[0]?.authorization).toBe("Bearer stored-token"); + }); + + test("rejects forced malformed env credentials instead of using stored OAuth", async () => { + process.env.SENTRY_AUTH_TOKEN = MALFORMED_TOKEN; + process.env.SENTRY_FORCE_ENV_TOKEN = "1"; + setAuthToken("stored-token"); + await expect(request()).rejects.toMatchObject({ + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect(requests).toEqual([]); + }); + + test("retries with a valid refreshed bearer", async () => { + process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; + setAuthToken("stored-token", 3600, "synthetic-refresh-token"); + globalThis.fetch = mockFetch((input, init) => { + const url = extractFetchUrl(input); + const authorization = new Headers(init?.headers).get("Authorization"); + requests.push({ url, authorization }); + if (url.endsWith("/oauth/token/")) { + return Promise.resolve( + Response.json({ + access_token: "refreshed-token", + token_type: "bearer", + expires_in: 3600, + }) + ); + } + return Promise.resolve( + new Response("{}", { + status: authorization === "Bearer stored-token" ? 401 : 200, + }) + ); + }); + + expect((await request()).status).toBe(200); + expect(requests).toEqual([ + { url: RESOURCE_URL, authorization: "Bearer stored-token" }, + { url: "https://sentry.io/oauth/token/", authorization: null }, + { url: RESOURCE_URL, authorization: "Bearer refreshed-token" }, + ]); + }); + + test.each([ + MALFORMED_TOKEN, + "", + "opaque-\0-token", + "opaque-\u0100-token", + ])("rejects malformed refreshed credentials without retrying the request %#", async (token) => { + process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; + setAuthToken("stored-token", 3600, "synthetic-refresh-token"); + globalThis.fetch = mockFetch((input, init) => { + const url = extractFetchUrl(input); + requests.push({ + url, + authorization: new Headers(init?.headers).get("Authorization"), + }); + if (url.endsWith("/oauth/token/")) { + return Promise.resolve( + Response.json({ + access_token: token, + token_type: "bearer", + expires_in: 3600, + refresh_token: "synthetic-refresh-token", + }) + ); + } + return Promise.resolve(new Response("{}", { status: 401 })); + }); + + for (let attempt = 0; attempt < 2; attempt++) { + const error = await request().catch((caught: unknown) => caught); + expect(error).toBeInstanceOf(AuthError); + expect(error).toMatchObject({ + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect((error as Error).cause).toBeUndefined(); + if (token) { + expect(String(error)).not.toContain(token); + } + expect(getAuthConfig()).toMatchObject({ + token: "stored-token", + refreshToken: "synthetic-refresh-token", + }); + } + const refreshAttempt = [ + { url: RESOURCE_URL, authorization: "Bearer stored-token" }, + { url: "https://sentry.io/oauth/token/", authorization: null }, + ]; + expect(requests).toEqual([...refreshAttempt, ...refreshAttempt]); + }); + + test("rejects malformed proactive refresh before storing or using the token", async () => { + process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; + setAuthToken("expired-token", -1, "synthetic-refresh-token"); + globalThis.fetch = mockFetch((input, init) => { + requests.push({ + url: extractFetchUrl(input), + authorization: new Headers(init?.headers).get("Authorization"), + }); + return Promise.resolve( + Response.json({ + access_token: "opaque-\0-secret-tail", + token_type: "bearer", + expires_in: 3600, + refresh_token: "replacement-refresh-token", + }) + ); + }); + + await expect(request()).rejects.toMatchObject({ + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect(getAuthConfig()).toMatchObject({ + token: "expired-token", + refreshToken: "synthetic-refresh-token", + }); + expect(requests).toEqual([ + { url: "https://sentry.io/oauth/token/", authorization: null }, + ]); + }); +}); From e0fdee49a347255bbbb072dfc74baf53ae5998de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20Beteg=C3=B3n?= Date: Mon, 28 Sep 2026 13:38:01 +0200 Subject: [PATCH 2/4] fix(auth): normalize tokens and preserve malformed-token reports --- apps/cli-docs/src/fragments/commands/auth.md | 6 +- packages/cli/src/lib/auth-header.ts | 22 +++--- packages/cli/src/lib/db/auth.ts | 8 +- packages/cli/src/lib/error-reporting.ts | 18 +++-- packages/cli/src/lib/errors.ts | 12 +++ packages/cli/src/lib/sentry-client.ts | 18 +++-- packages/cli/src/lib/telemetry.ts | 4 +- packages/cli/test/e2e/auth.test.ts | 7 +- packages/cli/test/lib/error-reporting.test.ts | 39 +++++++++- .../cli/test/lib/sentry-client.auth.test.ts | 78 +++++++++++++++++-- packages/cli/test/lib/telemetry.test.ts | 41 +++++++++- 11 files changed, 210 insertions(+), 43 deletions(-) diff --git a/apps/cli-docs/src/fragments/commands/auth.md b/apps/cli-docs/src/fragments/commands/auth.md index ee2c303b3a..dc6058eb70 100644 --- a/apps/cli-docs/src/fragments/commands/auth.md +++ b/apps/cli-docs/src/fragments/commands/auth.md @@ -135,9 +135,9 @@ When a token comes from an environment variable, the CLI skips expiry checks and ## Invalid Token Formatting Tokens must be a single line of printable ASCII characters, without spaces. -The CLI rejects embedded whitespace, control characters, and non-ASCII -characters before sending an authenticated request. It does not join split -lines or send only the first line of a token. +The CLI removes surrounding whitespace, then rejects any remaining whitespace, +control characters, and non-ASCII characters before sending an authenticated +request. It does not join split lines or send only the first line of a token. If you see "Invalid authentication token", copy the complete token again into the configuration that supplies it. For environment tokens, check diff --git a/packages/cli/src/lib/auth-header.ts b/packages/cli/src/lib/auth-header.ts index cef47dd03d..84e286629a 100644 --- a/packages/cli/src/lib/auth-header.ts +++ b/packages/cli/src/lib/auth-header.ts @@ -1,18 +1,20 @@ /** Validated Authorization values for the selected Sentry credential. */ -import { AuthError } from "./errors.js"; +import { MalformedAuthTokenError } from "./errors.js"; /** Bearer tokens are opaque, but cannot contain whitespace or non-ASCII bytes. */ const INVALID_TOKEN_CHARACTER_PATTERN = /[^\x21-\x7e]/; -/** Validate a selected credential before constructing its Authorization value. */ -export function formatAuthHeader(token: string): string { - if (!token || INVALID_TOKEN_CHARACTER_PATTERN.test(token)) { - throw new AuthError( - "invalid", - "Invalid authentication token. Copy it again as a single line without spaces or control characters, " + - "or run 'sentry auth login' to replace stored credentials." - ); +/** Remove surrounding whitespace and validate the remaining credential. */ +export function normalizeAuthToken(token: string): string { + const normalized = token.trim(); + if (!normalized || INVALID_TOKEN_CHARACTER_PATTERN.test(normalized)) { + throw new MalformedAuthTokenError(); } - return `Bearer ${token}`; + return normalized; +} + +/** Normalize and validate a credential before constructing its Authorization value. */ +export function formatAuthHeader(token: string): string { + return `Bearer ${normalizeAuthToken(token)}`; } diff --git a/packages/cli/src/lib/db/auth.ts b/packages/cli/src/lib/db/auth.ts index da95a580a0..bc5ecb181b 100644 --- a/packages/cli/src/lib/db/auth.ts +++ b/packages/cli/src/lib/db/auth.ts @@ -3,7 +3,7 @@ */ import { createHash } from "node:crypto"; -import { formatAuthHeader } from "../auth-header.js"; +import { normalizeAuthToken } from "../auth-header.js"; import { DEFAULT_SENTRY_URL, getConfiguredSentryUrl } from "../constants.js"; import { getEnv } from "../env.js"; import { getEnvTokenHost } from "../env-token-host.js"; @@ -623,18 +623,18 @@ async function performTokenRefresh( // Validate before SQLite can truncate NUL-containing credentials or replace // the current session with a malformed response. Keep that session intact. - formatAuthHeader(tokenResponse.access_token); + const token = normalizeAuthToken(tokenResponse.access_token); const now = Date.now(); const expiresAt = now + tokenResponse.expires_in * 1000; await setAuthToken( - tokenResponse.access_token, + token, tokenResponse.expires_in, tokenResponse.refresh_token ?? storedRefreshToken ); return { - token: tokenResponse.access_token, + token, refreshed: true, expiresAt, expiresIn: tokenResponse.expires_in, diff --git a/packages/cli/src/lib/error-reporting.ts b/packages/cli/src/lib/error-reporting.ts index 69b2448e75..99e3a8f2f3 100644 --- a/packages/cli/src/lib/error-reporting.ts +++ b/packages/cli/src/lib/error-reporting.ts @@ -4,7 +4,7 @@ * Provides two things: * * 1. **Silencing rules** — `OutputError`, network failures (offline/DNS/proxy), - * `AuthError` (expected auth states the user must act on), 401–499 `ApiError`, + * `AuthError` (except `MalformedAuthTokenError`), 401–499 `ApiError`, * and 400 `ApiError`s that report an unparseable user search query are not * sent to Sentry as issues. A `cli.error.silenced` metric preserves volume + * user/org context. @@ -13,6 +13,8 @@ * silenced: its volume is the signal driving auto-detection/UX improvements * (e.g. single-org auto-select, the interactive picker), so it must stay * visible (CLI-3B). + * `MalformedAuthTokenError` is also captured to keep token-formatting + * failures visible; its fixed message never includes the rejected token. * * 2. **Grouping tags** — enriches every error event with `cli_error.*` tags * that Sentry's server-side fingerprint rules use for stable grouping. @@ -33,6 +35,7 @@ import { DeviceFlowError, HostScopeError, isNetworkError, + MalformedAuthTokenError, OutputError, ResolutionError, SeerError, @@ -85,12 +88,13 @@ export function classifySilenced(error: unknown): SilenceReason | null { // stays captured (CLI-3B). The accompanying `resolveOrgProjectOrGuide` // changes aim to drive this volume down by helping users succeed instead. // - // All AuthError reasons are expected auth states the user must act on, not - // CLI bugs: `not_authenticated` (no token), `expired` (token aged out), and - // `invalid` (malformed credentials or a bad/insufficiently-scoped token). - // Transient network/server failures do not masquerade as invalid tokens, - // so it is safe to silence these alongside the others (CLI-19). - if (error instanceof AuthError) { + // Missing, expired, and rejected credentials are expected auth states. + // Malformed tokens stay visible to investigate configuration/formatting + // failures, using an error that never contains the credential (CLI-19). + if ( + error instanceof AuthError && + !(error instanceof MalformedAuthTokenError) + ) { return "auth_expected"; } // A ValidationError with field "project.ambiguous_org" means the user diff --git a/packages/cli/src/lib/errors.ts b/packages/cli/src/lib/errors.ts index b21505cd11..92c1ca771a 100644 --- a/packages/cli/src/lib/errors.ts +++ b/packages/cli/src/lib/errors.ts @@ -255,6 +255,18 @@ export class AuthError extends CliError { } } +/** Malformed credentials are reportable without retaining their secret value. */ +export class MalformedAuthTokenError extends AuthError { + constructor() { + super( + "invalid", + "Invalid authentication token. Copy it again as a single line without spaces or control characters, " + + "or run 'sentry auth login' to replace stored credentials." + ); + this.name = "MalformedAuthTokenError"; + } +} + /** * Configuration or DSN errors. * diff --git a/packages/cli/src/lib/sentry-client.ts b/packages/cli/src/lib/sentry-client.ts index e945bcc9d0..cb428b76bc 100644 --- a/packages/cli/src/lib/sentry-client.ts +++ b/packages/cli/src/lib/sentry-client.ts @@ -10,7 +10,7 @@ import { setTimeout as sleepMs } from "node:timers/promises"; import { getTraceData } from "@sentry/node-core/light"; -import { formatAuthHeader } from "./auth-header.js"; +import { formatAuthHeader, normalizeAuthToken } from "./auth-header.js"; import { maybeWarnEnvTokenIgnored } from "./auth-hint.js"; import { computeInvalidationPrefixes } from "./cache-keys.js"; import { @@ -26,7 +26,12 @@ import { } from "./custom-ca.js"; import { applyCustomHeaders } from "./custom-headers.js"; import { getAuthToken, refreshToken } from "./db/auth.js"; -import { ApiError, AuthError, HostScopeError, TimeoutError } from "./errors.js"; +import { + ApiError, + HostScopeError, + MalformedAuthTokenError, + TimeoutError, +} from "./errors.js"; import { logger } from "./logger.js"; import { clearLastCacheHitAge, @@ -131,7 +136,8 @@ function prepareHeaders( // multiple Sentry instances. The claim is unsigned (see token-claims.ts); // fail-open on parse errors. Uses isHostTrustedForClaim so multi-region // fan-out via the control silo's region URLs still works. - const claimUrl = parseSntrysClaim(token)?.url; + const normalizedToken = normalizeAuthToken(token); + const claimUrl = parseSntrysClaim(normalizedToken)?.url; if (claimUrl && !isHostTrustedForClaim(input, claimUrl)) { throw new HostScopeError( "Credentials", @@ -146,7 +152,7 @@ function prepareHeaders( const sourceHeaders = init?.headers ?? (input instanceof Request ? input.headers : undefined); const headers = new Headers(sourceHeaders); - headers.set("Authorization", formatAuthHeader(token)); + headers.set("Authorization", formatAuthHeader(normalizedToken)); if (!headers.has("User-Agent")) { headers.set("User-Agent", getUserAgent()); } @@ -190,7 +196,7 @@ async function handleUnauthorized(headers: Headers): Promise { } newToken = result.token; } catch (error) { - if (error instanceof AuthError && error.reason === "invalid") { + if (error instanceof MalformedAuthTokenError) { throw error; } log.debug("Token refresh failed after 401", error); @@ -466,7 +472,7 @@ async function invalidateAfterMutation( /** Build a `{ authorization }` header map from a bearer token, or `{}` if absent. */ function authHeaders(token: string | undefined): Record { - return token ? { authorization: `Bearer ${token}` } : {}; + return token ? { authorization: `Bearer ${token.trim()}` } : {}; } type AttemptInputFactory = () => { diff --git a/packages/cli/src/lib/telemetry.ts b/packages/cli/src/lib/telemetry.ts index 8b99739adf..18109ed3d9 100644 --- a/packages/cli/src/lib/telemetry.ts +++ b/packages/cli/src/lib/telemetry.ts @@ -231,8 +231,8 @@ export async function withTelemetry( // Route through reportCliError so silencing (OutputError, expected-auth // AuthError, 401–499 ApiError) and fingerprint normalization are applied // consistently. Silenced errors emit a `cli.error.silenced` metric + - // optional structured log instead of creating a Sentry issue. (ContextError - // is intentionally NOT silenced — see classifySilenced.) + // optional structured log instead of creating a Sentry issue. ContextError + // and MalformedAuthTokenError stay captured — see classifySilenced. reportCliError(e); // Only mark the session crashed for genuine, unexpected CLI bugs. This is a // stricter gate than `classifySilenced`: an error can be *captured* to diff --git a/packages/cli/test/e2e/auth.test.ts b/packages/cli/test/e2e/auth.test.ts index ec5f66a834..ba6ed60dbf 100644 --- a/packages/cli/test/e2e/auth.test.ts +++ b/packages/cli/test/e2e/auth.test.ts @@ -136,8 +136,11 @@ describe("sentry auth whoami", () => { expect(result.exitCode).toBe(EXIT.AUTH_NOT_AUTHENTICATED); }); - test("shows current user identity", async () => { - await ctx.setAuthToken(TEST_TOKEN); + test.each([ + TEST_TOKEN, + ` \n${TEST_TOKEN}\r\t`, + ])("shows current user identity with surrounding token whitespace %#", async (token) => { + await ctx.setAuthToken(token); const result = await ctx.run(["auth", "whoami"]); diff --git a/packages/cli/test/lib/error-reporting.test.ts b/packages/cli/test/lib/error-reporting.test.ts index d7603717b5..b212fd39bb 100644 --- a/packages/cli/test/lib/error-reporting.test.ts +++ b/packages/cli/test/lib/error-reporting.test.ts @@ -11,6 +11,7 @@ // biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking import * as Sentry from "@sentry/node-core/light"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { formatAuthHeader } from "../../src/lib/auth-header.js"; import { classifySilenced, enrichEventWithGroupingTags, @@ -26,6 +27,7 @@ import { ConfigError, ContextError, HostScopeError, + MalformedAuthTokenError, OutputError, ResolutionError, SeerError, @@ -225,11 +227,14 @@ describe("classifySilenced", () => { }); test("silences AuthError(invalid)", () => { - // `invalid` is only thrown for a genuine 401/403 (a bad/insufficient token - // the user supplied), so it is an expected auth state like the others. + // Rejected credentials stay silenced; malformed tokens use a subclass. expect(classifySilenced(new AuthError("invalid"))).toBe("auth_expected"); }); + test("does NOT silence MalformedAuthTokenError", () => { + expect(classifySilenced(new MalformedAuthTokenError())).toBeNull(); + }); + test.each([ 401, 403, 404, 429, 418, ])("silences ApiError with status %i", (status) => { @@ -581,6 +586,36 @@ describe("reportCliError integration", () => { ); }); + test("captures malformed-token failures once without retaining credentials", () => { + const firstPart = "sntrys_reporting-secret-first"; + const secondPart = "reporting-secret-second"; + let error: unknown; + try { + formatAuthHeader(`${firstPart}\n${secondPart}`); + } catch (caught) { + error = caught; + } + expect(error).toBeInstanceOf(MalformedAuthTokenError); + + const { tags, contexts } = capturedScopeTags(error); + expect(captureSpy).toHaveBeenCalledExactlyOnceWith(error); + expect(metricSpy).not.toHaveBeenCalled(); + expect(tags).toMatchObject({ + "cli_error.class": "MalformedAuthTokenError", + "cli_error.kind": "invalid", + }); + + const captured = captureSpy.mock.calls[0]?.[0] as Error; + expect(captured.cause).toBeUndefined(); + const diagnostics = JSON.stringify({ + properties: Object.getOwnPropertyDescriptors(captured), + tags, + contexts, + }); + expect(diagnostics).not.toContain(firstPart); + expect(diagnostics).not.toContain(secondPart); + }); + test("captures ApiError(400) with normalized endpoint tag", () => { const err = new ApiError( "failed", diff --git a/packages/cli/test/lib/sentry-client.auth.test.ts b/packages/cli/test/lib/sentry-client.auth.test.ts index 96f4ff1a45..1efe62759c 100644 --- a/packages/cli/test/lib/sentry-client.auth.test.ts +++ b/packages/cli/test/lib/sentry-client.auth.test.ts @@ -4,7 +4,13 @@ import { afterEach, beforeEach, describe, expect, test } from "vitest"; import { shouldAutoAuth } from "../../src/lib/auto-auth.js"; import { getAuthConfig, setAuthToken } from "../../src/lib/db/auth.js"; import { setEnv } from "../../src/lib/env.js"; -import { AuthError, EXIT, withAuthGuard } from "../../src/lib/errors.js"; +import { + AuthError, + EXIT, + HostScopeError, + MalformedAuthTokenError, + withAuthGuard, +} from "../../src/lib/errors.js"; import { getSdkConfig, resetAuthenticatedFetch, @@ -83,6 +89,7 @@ describe("authenticated fetch bearer validation", () => { (caught: unknown) => caught ); expect(error).toBeInstanceOf(AuthError); + expect(error).toBeInstanceOf(MalformedAuthTokenError); const authError = error as AuthError; expect(authError.reason).toBe("invalid"); expect(authError.exitCode).toBe(EXIT.AUTH_INVALID); @@ -138,16 +145,38 @@ describe("authenticated fetch bearer validation", () => { expect(requests[0]?.authorization).toBe("Bearer synthetic-token"); }); - test("does not silently trim stored credentials", async () => { - const token = " stored-token "; + test.each([ + " stored-token ", + "\t\n\u00a0stored-token\r\n", + ])("trims surrounding whitespace from stored credentials %#", async (token) => { setAuthToken(token); + await request(); + expect(requests).toEqual([ + { url: RESOURCE_URL, authorization: "Bearer stored-token" }, + ]); + }); + + test("rejects a whitespace-only stored credential", async () => { + setAuthToken(" \t\r\n "); await expect(request()).rejects.toMatchObject({ + name: "MalformedAuthTokenError", reason: "invalid", exitCode: EXIT.AUTH_INVALID, }); expect(requests).toEqual([]); }); + test("checks token host claims after removing surrounding whitespace", async () => { + const token = mintSntrysToken({ + iat: 1, + url: "https://other-sentry.example.com", + org: "synthetic-org", + }); + setAuthToken(` \n${token}\t `); + await expect(request()).rejects.toBeInstanceOf(HostScopeError); + expect(requests).toEqual([]); + }); + test.each( ENV_TOKEN_KEYS )("ignores a malformed %s when stored OAuth takes precedence", async (key) => { @@ -168,7 +197,10 @@ describe("authenticated fetch bearer validation", () => { expect(requests).toEqual([]); }); - test("retries with a valid refreshed bearer", async () => { + test.each([ + "refreshed-token", + " \nrefreshed-token\r\t", + ])("retries with a normalized valid refreshed bearer %#", async (token) => { process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; setAuthToken("stored-token", 3600, "synthetic-refresh-token"); globalThis.fetch = mockFetch((input, init) => { @@ -178,7 +210,7 @@ describe("authenticated fetch bearer validation", () => { if (url.endsWith("/oauth/token/")) { return Promise.resolve( Response.json({ - access_token: "refreshed-token", + access_token: token, token_type: "bearer", expires_in: 3600, }) @@ -197,11 +229,13 @@ describe("authenticated fetch bearer validation", () => { { url: "https://sentry.io/oauth/token/", authorization: null }, { url: RESOURCE_URL, authorization: "Bearer refreshed-token" }, ]); + expect(getAuthConfig()?.token).toBe("refreshed-token"); }); test.each([ MALFORMED_TOKEN, "", + " \t\n ", "opaque-\0-token", "opaque-\u0100-token", ])("rejects malformed refreshed credentials without retrying the request %#", async (token) => { @@ -228,7 +262,7 @@ describe("authenticated fetch bearer validation", () => { for (let attempt = 0; attempt < 2; attempt++) { const error = await request().catch((caught: unknown) => caught); - expect(error).toBeInstanceOf(AuthError); + expect(error).toBeInstanceOf(MalformedAuthTokenError); expect(error).toMatchObject({ reason: "invalid", exitCode: EXIT.AUTH_INVALID, @@ -249,6 +283,38 @@ describe("authenticated fetch bearer validation", () => { expect(requests).toEqual([...refreshAttempt, ...refreshAttempt]); }); + test("normalizes a proactive refresh before storing and using the token", async () => { + process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; + setAuthToken("expired-token", -1, "synthetic-refresh-token"); + globalThis.fetch = mockFetch((input, init) => { + const url = extractFetchUrl(input); + requests.push({ + url, + authorization: new Headers(init?.headers).get("Authorization"), + }); + return Promise.resolve( + url.endsWith("/oauth/token/") + ? Response.json({ + access_token: " \nrefreshed-token\r\t", + token_type: "bearer", + expires_in: 3600, + refresh_token: "replacement-refresh-token", + }) + : Response.json({}) + ); + }); + + expect((await request()).status).toBe(200); + expect(getAuthConfig()).toMatchObject({ + token: "refreshed-token", + refreshToken: "replacement-refresh-token", + }); + expect(requests).toEqual([ + { url: "https://sentry.io/oauth/token/", authorization: null }, + { url: RESOURCE_URL, authorization: "Bearer refreshed-token" }, + ]); + }); + test("rejects malformed proactive refresh before storing or using the token", async () => { process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; setAuthToken("expired-token", -1, "synthetic-refresh-token"); diff --git a/packages/cli/test/lib/telemetry.test.ts b/packages/cli/test/lib/telemetry.test.ts index f6f2ed213f..fc5e1123d1 100644 --- a/packages/cli/test/lib/telemetry.test.ts +++ b/packages/cli/test/lib/telemetry.test.ts @@ -17,8 +17,14 @@ import { test, vi, } from "vitest"; +import { formatAuthHeader } from "../../src/lib/auth-header.js"; import { Database } from "../../src/lib/db/sqlite.js"; -import { ApiError, AuthError, OutputError } from "../../src/lib/errors.js"; +import { + ApiError, + AuthError, + MalformedAuthTokenError, + OutputError, +} from "../../src/lib/errors.js"; import { createTracedDatabase, createWizardPromptTelemetry, @@ -333,6 +339,39 @@ describe("withTelemetry", () => { currentScopeSpy.mockRestore(); }); + test("captures malformed tokens without marking the session crashed", async () => { + const captureSpy = vi.spyOn(Sentry, "captureException"); + const metricSpy = vi.spyOn(Sentry.metrics, "distribution"); + const session = { status: "ok", errors: 0 }; + const isolationScopeSpy = vi + .spyOn(Sentry, "getIsolationScope") + .mockReturnValue({ + getSession: () => session, + } as unknown as Sentry.Scope); + const currentScopeSpy = vi + .spyOn(Sentry, "getCurrentScope") + .mockReturnValue({ + getSession: () => null, + } as unknown as Sentry.Scope); + try { + await expect( + withTelemetry(() => formatAuthHeader("first-part\nsecond-part")) + ).rejects.toThrow(MalformedAuthTokenError); + expect(captureSpy).toHaveBeenCalledExactlyOnceWith( + expect.any(MalformedAuthTokenError) + ); + expect( + metricSpy.mock.calls.find((c) => c[0] === "cli.error.silenced") + ).toBeUndefined(); + expect(session.status).toBe("ok"); + } finally { + captureSpy.mockRestore(); + metricSpy.mockRestore(); + isolationScopeSpy.mockRestore(); + currentScopeSpy.mockRestore(); + } + }); + test("marks session crashed for a genuine CLI bug", async () => { // A generic Error is neither silenced nor a user error, so the session // should be marked crashed (the counterpart to the ContextError case). From e754fb32850c98417865d6a5cbe6d65882b2d284 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20Beteg=C3=B3n?= Date: Mon, 28 Sep 2026 18:40:16 +0200 Subject: [PATCH 3/4] fix(auth): normalize padding before storing credentials --- apps/cli-docs/src/fragments/commands/auth.md | 7 +- packages/cli/src/commands/auth/login.ts | 9 +- packages/cli/src/lib/auth-header.ts | 21 ++- packages/cli/src/lib/db/auth.ts | 49 +++---- packages/cli/src/lib/db/migration.ts | 40 +++++- packages/cli/src/lib/sentry-client.ts | 10 +- packages/cli/test/commands/auth/login.test.ts | 58 ++++++++ packages/cli/test/e2e/auth.test.ts | 51 ++++++- .../cli/test/lib/auth-header.property.test.ts | 90 ++++++++++++ .../cli/test/lib/db/auth.property.test.ts | 14 +- packages/cli/test/lib/db/auth.test.ts | 60 +++++++- packages/cli/test/lib/db/migration.test.ts | 132 +++++++++++++++++ packages/cli/test/lib/db/model-based.test.ts | 8 +- packages/cli/test/lib/env-token-host.test.ts | 21 ++- .../cli/test/lib/sentry-client.auth.test.ts | 134 +++++++++++++++++- 15 files changed, 642 insertions(+), 62 deletions(-) create mode 100644 packages/cli/test/lib/auth-header.property.test.ts create mode 100644 packages/cli/test/lib/db/migration.test.ts diff --git a/apps/cli-docs/src/fragments/commands/auth.md b/apps/cli-docs/src/fragments/commands/auth.md index dc6058eb70..bc2f3afaff 100644 --- a/apps/cli-docs/src/fragments/commands/auth.md +++ b/apps/cli-docs/src/fragments/commands/auth.md @@ -135,9 +135,10 @@ When a token comes from an environment variable, the CLI skips expiry checks and ## Invalid Token Formatting Tokens must be a single line of printable ASCII characters, without spaces. -The CLI removes surrounding whitespace, then rejects any remaining whitespace, -control characters, and non-ASCII characters before sending an authenticated -request. It does not join split lines or send only the first line of a token. +When preparing an access token for storage or an authenticated request, the CLI +removes surrounding whitespace and ASCII control characters, then rejects any +remaining whitespace, control characters, and non-ASCII characters. It does not +join split lines. If you see "Invalid authentication token", copy the complete token again into the configuration that supplies it. For environment tokens, check diff --git a/packages/cli/src/commands/auth/login.ts b/packages/cli/src/commands/auth/login.ts index 0e66c857be..a9ce8175e7 100644 --- a/packages/cli/src/commands/auth/login.ts +++ b/packages/cli/src/commands/auth/login.ts @@ -5,6 +5,7 @@ import { getUserRegions, listOrganizationsUncached, } from "../../lib/api-client.js"; +import { normalizeAuthToken } from "../../lib/auth-header.js"; import { buildCommand, numberParser } from "../../lib/command.js"; import { normalizeUrl } from "../../lib/constants.js"; import { @@ -459,6 +460,10 @@ export const loginCommand = buildCommand({ // (--token + --read-only/--scope, --read-only + --scope) and invalid // scope values fail fast before any network or DB work. const oauthScope = resolveLoginScope(flags); + // Validate explicit credentials before changing the host or replacing an + // existing session. An empty --token is invalid, not an OAuth request. + const token = + flags.token === undefined ? undefined : normalizeAuthToken(flags.token); // Apply --url first so the device flow / token refresh target the // requested instance. Default URL persistence is deferred until login @@ -490,9 +495,9 @@ export const loginCommand = buildCommand({ // Non-fatal: cache directory may not exist } - if (flags.token) { + if (token !== undefined) { // Save token first (with host scope), then validate by fetching user regions - await setAuthToken(flags.token, undefined, undefined, { + await setAuthToken(token, undefined, undefined, { host: effectiveHost, }); diff --git a/packages/cli/src/lib/auth-header.ts b/packages/cli/src/lib/auth-header.ts index 84e286629a..0a3af0a381 100644 --- a/packages/cli/src/lib/auth-header.ts +++ b/packages/cli/src/lib/auth-header.ts @@ -5,9 +5,26 @@ import { MalformedAuthTokenError } from "./errors.js"; /** Bearer tokens are opaque, but cannot contain whitespace or non-ASCII bytes. */ const INVALID_TOKEN_CHARACTER_PATTERN = /[^\x21-\x7e]/; -/** Remove surrounding whitespace and validate the remaining credential. */ +// biome-ignore lint/suspicious/noControlCharactersInRegex: pasted ASCII controls are the padding this rule removes. +const TOKEN_PADDING_PATTERN = /[\s\x00-\x1f\x7f]/; + +/** Remove surrounding whitespace and ASCII controls without validating a candidate. */ +export function trimAuthToken(token: string): string { + // Scan only the edges; a trailing regex can backtrack over long internal runs. + let start = 0; + let end = token.length; + while (start < end && TOKEN_PADDING_PATTERN.test(token.charAt(start))) { + start += 1; + } + while (end > start && TOKEN_PADDING_PATTERN.test(token.charAt(end - 1))) { + end -= 1; + } + return token.slice(start, end); +} + +/** Trim padding and validate the credential selected for storage or a request. */ export function normalizeAuthToken(token: string): string { - const normalized = token.trim(); + const normalized = trimAuthToken(token); if (!normalized || INVALID_TOKEN_CHARACTER_PATTERN.test(normalized)) { throw new MalformedAuthTokenError(); } diff --git a/packages/cli/src/lib/db/auth.ts b/packages/cli/src/lib/db/auth.ts index bc5ecb181b..891c2d3518 100644 --- a/packages/cli/src/lib/db/auth.ts +++ b/packages/cli/src/lib/db/auth.ts @@ -3,7 +3,7 @@ */ import { createHash } from "node:crypto"; -import { normalizeAuthToken } from "../auth-header.js"; +import { normalizeAuthToken, trimAuthToken } from "../auth-header.js"; import { DEFAULT_SENTRY_URL, getConfiguredSentryUrl } from "../constants.js"; import { getEnv } from "../env.js"; import { getEnvTokenHost } from "../env-token-host.js"; @@ -92,23 +92,11 @@ export type AuthConfig = { }; /** - * Read the raw token string from environment variables, ignoring all filters. - * - * Unlike {@link getEnvToken}, this always returns the env token if set, even - * when stored OAuth credentials would normally take priority. Used by the HTTP - * layer to check "was an env token provided?" independent of whether it's being - * used, and by the per-endpoint permission cache. + * Read the trimmed env token even when stored OAuth takes priority. + * Does not validate credentials that may never be used. */ export function getRawEnvToken(): string | undefined { - const authToken = getEnv().SENTRY_AUTH_TOKEN?.trim(); - if (authToken) { - return authToken; - } - const sentryToken = getEnv().SENTRY_TOKEN?.trim(); - if (sentryToken) { - return sentryToken; - } - return; + return getEnvToken()?.token; } /** @@ -121,13 +109,21 @@ export function getRawEnvToken(): string | undefined { * which check the DB first when `SENTRY_FORCE_ENV_TOKEN` is not set. */ function getEnvToken(): { token: string; source: AuthSource } | undefined { + // Preserve presence rules: whitespace is unset, but control-only credentials + // must remain selected so validation cannot silently fall back to another identity. const authToken = getEnv().SENTRY_AUTH_TOKEN?.trim(); if (authToken) { - return { token: authToken, source: "env:SENTRY_AUTH_TOKEN" }; + return { + token: trimAuthToken(authToken) || authToken, + source: "env:SENTRY_AUTH_TOKEN", + }; } const sentryToken = getEnv().SENTRY_TOKEN?.trim(); if (sentryToken) { - return { token: sentryToken, source: "env:SENTRY_TOKEN" }; + return { + token: trimAuthToken(sentryToken) || sentryToken, + source: "env:SENTRY_TOKEN", + }; } return; } @@ -147,14 +143,9 @@ export function isEnvTokenActive(): boolean { * Falls back to "SENTRY_AUTH_TOKEN" if no env var is set. */ export function getActiveEnvVarName(): string { - // Match getRawEnvToken() priority: SENTRY_AUTH_TOKEN first, then SENTRY_TOKEN - if (getEnv().SENTRY_AUTH_TOKEN?.trim()) { - return "SENTRY_AUTH_TOKEN"; - } - if (getEnv().SENTRY_TOKEN?.trim()) { - return "SENTRY_TOKEN"; - } - return "SENTRY_AUTH_TOKEN"; + return getEnvToken()?.source === "env:SENTRY_TOKEN" + ? "SENTRY_TOKEN" + : "SENTRY_AUTH_TOKEN"; } export function getAuthConfig(): AuthConfig | undefined { @@ -388,12 +379,14 @@ export type SetAuthTokenOptions = { host?: string; }; +/** Normalize an access token before storage; malformed input leaves the auth row unchanged. */ export function setAuthToken( token: string, expiresIn?: number, newRefreshToken?: string, options?: SetAuthTokenOptions ): void { + const normalizedToken = normalizeAuthToken(token); withDbSpan("setAuthToken", () => { const db = getDatabase(); const now = Date.now(); @@ -423,7 +416,7 @@ export function setAuthToken( "auth", { id: 1, - token, + token: normalizedToken, refresh_token: newRefreshToken ?? null, expires_at: expiresAt, issued_at: issuedAt, @@ -622,7 +615,7 @@ async function performTokenRefresh( } // Validate before SQLite can truncate NUL-containing credentials or replace - // the current session with a malformed response. Keep that session intact. + // the stored credentials with a malformed response. Leave those values unchanged. const token = normalizeAuthToken(tokenResponse.access_token); const now = Date.now(); const expiresAt = now + tokenResponse.expires_in * 1000; diff --git a/packages/cli/src/lib/db/migration.ts b/packages/cli/src/lib/db/migration.ts index 380ac3f2a0..6ad40153e7 100644 --- a/packages/cli/src/lib/db/migration.ts +++ b/packages/cli/src/lib/db/migration.ts @@ -8,6 +8,8 @@ import { join } from "node:path"; const _require = createRequire(import.meta.url); +import { normalizeAuthToken } from "../auth-header.js"; +import { MalformedAuthTokenError } from "../errors.js"; import { logger } from "../logger.js"; import { getConfigDir } from "./index.js"; import type { Database } from "./sqlite.js"; @@ -76,7 +78,7 @@ function deleteOldConfig(): boolean { type OldConfig = { auth?: { - token?: string; + token?: unknown; refreshToken?: string; expiresAt?: number; issuedAt?: number; @@ -119,6 +121,7 @@ type OldConfig = { }; }; +/** Migrate once, retaining the original file and skipping auth if its access token is malformed. */ // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: one-time migration export function migrateFromJson(db: Database): void { // Check SQLite metadata first - this is the authoritative source @@ -140,19 +143,38 @@ export function migrateFromJson(db: Database): void { return; } + let token: string | undefined; + let invalidAuthToken = false; + if (oldConfig.auth?.token !== undefined) { + try { + if (typeof oldConfig.auth.token !== "string") { + throw new MalformedAuthTokenError(); + } + token = normalizeAuthToken(oldConfig.auth.token); + } catch (error) { + if (!(error instanceof MalformedAuthTokenError)) { + throw error; + } + // Do not block DB initialization (including login/logout) on a bad + // credential. Preserve the original file instead of binding a token + // that SQLite could truncate at an embedded NUL. + invalidAuthToken = true; + } + } + log.info("Migrating config to SQLite..."); db.exec("BEGIN TRANSACTION"); try { - if (oldConfig.auth?.token) { + if (token && oldConfig.auth) { // Direct write (not via setAuthToken) — safe only because migration // runs during DB bootstrap, before getIdentityFingerprint() memoizes. db.query(` INSERT OR REPLACE INTO auth (id, token, refresh_token, expires_at, issued_at, updated_at) VALUES (1, ?, ?, ?, ?, ?) `).run( - oldConfig.auth.token, + token, oldConfig.auth.refreshToken ?? null, oldConfig.auth.expiresAt ?? null, oldConfig.auth.issuedAt ?? null, @@ -275,9 +297,15 @@ export function migrateFromJson(db: Database): void { markMigrationCompleted(db); db.exec("COMMIT"); - // Best-effort cleanup of old file - if it fails, we're still safe - // because SQLite metadata is the authoritative source - deleteOldConfig(); + if (invalidAuthToken) { + log.warn( + "Malformed authentication credentials were not migrated. The original config.json was kept. " + + "Run 'sentry auth login' to authenticate again, then remove the old file." + ); + } else { + // Best-effort cleanup: SQLite metadata prevents re-import if it fails. + deleteOldConfig(); + } log.success("Migration complete."); } catch (error) { db.exec("ROLLBACK"); diff --git a/packages/cli/src/lib/sentry-client.ts b/packages/cli/src/lib/sentry-client.ts index cb428b76bc..a35c466466 100644 --- a/packages/cli/src/lib/sentry-client.ts +++ b/packages/cli/src/lib/sentry-client.ts @@ -10,7 +10,11 @@ import { setTimeout as sleepMs } from "node:timers/promises"; import { getTraceData } from "@sentry/node-core/light"; -import { formatAuthHeader, normalizeAuthToken } from "./auth-header.js"; +import { + formatAuthHeader, + normalizeAuthToken, + trimAuthToken, +} from "./auth-header.js"; import { maybeWarnEnvTokenIgnored } from "./auth-hint.js"; import { computeInvalidationPrefixes } from "./cache-keys.js"; import { @@ -470,9 +474,9 @@ async function invalidateAfterMutation( } } -/** Build a `{ authorization }` header map from a bearer token, or `{}` if absent. */ +/** Cache metadata must not validate a candidate before OAuth refresh selects a token. */ function authHeaders(token: string | undefined): Record { - return token ? { authorization: `Bearer ${token.trim()}` } : {}; + return token ? { authorization: `Bearer ${trimAuthToken(token)}` } : {}; } type AttemptInputFactory = () => { diff --git a/packages/cli/test/commands/auth/login.test.ts b/packages/cli/test/commands/auth/login.test.ts index 82169b26cb..4f2722ae36 100644 --- a/packages/cli/test/commands/auth/login.test.ts +++ b/packages/cli/test/commands/auth/login.test.ts @@ -131,6 +131,7 @@ import * as dbUser from "../../../src/lib/db/user.js"; import { ApiError, AuthError, + MalformedAuthTokenError, ValidationError, } from "../../../src/lib/errors.js"; @@ -150,11 +151,13 @@ vi.mock("../../../src/lib/interactive-login.js", async (importOriginal) => { // biome-ignore lint/performance/noNamespaceImport: needed for spyOn mocking import * as interactiveLogin from "../../../src/lib/interactive-login.js"; import type { SentryCliRcConfig } from "../../../src/lib/sentryclirc.js"; +import { useEnvSandbox } from "../../helpers.js"; type LoginFlags = { readonly token?: string; readonly timeout: number; readonly force: boolean; + readonly url?: string; readonly "read-only"?: boolean; readonly scope?: readonly string[]; }; @@ -213,6 +216,8 @@ function expectTokenStored( } describe("loginCommand.func --token path", () => { + useEnvSandbox(["SENTRY_HOST", "SENTRY_URL"]); + let isAuthenticatedSpy: ReturnType; let isEnvTokenActiveSpy: ReturnType; let setAuthTokenSpy: ReturnType; @@ -334,6 +339,59 @@ describe("loginCommand.func --token path", () => { expect(out).toContain("Jane Doe"); }); + test.each([ + ["line break", "synthetic-prefix\nsynthetic-suffix"], + ["NUL", "synthetic-prefix\0synthetic-suffix"], + ["empty value", ""], + ["control-only value", "\x01\x7f"], + ])("--force --token rejects %s before changing the session or host", async (_, token) => { + isAuthenticatedSpy.mockReturnValue(true); + process.env.SENTRY_HOST = "https://previous.example.com"; + process.env.SENTRY_URL = "https://previous.example.com"; + const fetchSpy = vi.spyOn(globalThis, "fetch"); + + try { + const { context } = createContext(); + await expect( + func.call(context, { + token, + force: true, + timeout: 900, + url: "https://replacement.example.com", + }) + ).rejects.toBeInstanceOf(MalformedAuthTokenError); + + expect(process.env.SENTRY_HOST).toBe("https://previous.example.com"); + expect(process.env.SENTRY_URL).toBe("https://previous.example.com"); + expect(clearAuthSpy).not.toHaveBeenCalled(); + expect(setAuthTokenSpy).not.toHaveBeenCalled(); + expect(setUserInfoSpy).not.toHaveBeenCalled(); + expect(getUserRegionsSpy).not.toHaveBeenCalled(); + expect(getCurrentUserSpy).not.toHaveBeenCalled(); + expect(runInteractiveLoginSpy).not.toHaveBeenCalled(); + expect(fetchSpy).not.toHaveBeenCalled(); + } finally { + fetchSpy.mockRestore(); + } + }); + + test.each([ + "\t\n\u00a0synthetic-token\r\n", + "\0\u00a0\x01synthetic-token\x7f\ufeff\0", + ])("--token normalizes surrounding whitespace and controls before storage %#", async (token) => { + isAuthenticatedSpy.mockReturnValue(false); + setAuthTokenSpy.mockReturnValue(undefined); + getUserRegionsSpy.mockResolvedValue([]); + getCurrentUserSpy.mockResolvedValue(SAMPLE_USER); + setUserInfoSpy.mockReturnValue(undefined); + + const { context } = createContext(); + await func.call(context, { token, force: false, timeout: 900 }); + + expectTokenStored(setAuthTokenSpy, "synthetic-token"); + expect(runInteractiveLoginSpy).not.toHaveBeenCalled(); + }); + test("--token: null user.name is converted to undefined in setUserInfo", async () => { isAuthenticatedSpy.mockReturnValue(false); setAuthTokenSpy.mockReturnValue(undefined); diff --git a/packages/cli/test/e2e/auth.test.ts b/packages/cli/test/e2e/auth.test.ts index ba6ed60dbf..54281f127e 100644 --- a/packages/cli/test/e2e/auth.test.ts +++ b/packages/cli/test/e2e/auth.test.ts @@ -4,6 +4,8 @@ * Tests for sentry auth login, logout, and status commands. */ +import { readFile, writeFile } from "node:fs/promises"; +import { join } from "node:path"; import { afterAll, afterEach, @@ -13,6 +15,7 @@ import { expect, test, } from "vitest"; +import { Database } from "../../src/lib/db/sqlite.js"; import { EXIT } from "../../src/lib/errors.js"; import { createE2EContext, type E2EContext } from "../fixture.js"; import { cleanupTestDir, createTestConfigDir } from "../helpers.js"; @@ -116,7 +119,14 @@ describe("sentry auth login --token", () => { describe("sentry auth whoami", () => { test("rejects a split stored token without exposing it in JSON mode", async () => { const token = "sntryu_SYNTHETIC-PREFIX\nSYNTHETIC-SECRET-TAIL"; - await ctx.setAuthToken(token); + await ctx.setAuthToken(TEST_TOKEN); + // Model a pre-validation credential; the setter now rejects this input. + const db = new Database(join(ctx.configDir, "cli.db")); + try { + db.query("UPDATE auth SET token = ? WHERE id = 1").run(token); + } finally { + db.close(); + } const result = await ctx.run(["auth", "whoami", "--json"]); const output = result.stdout + result.stderr; @@ -170,6 +180,45 @@ describe("sentry auth whoami", () => { }); }); +describe("auth formatting recovery", () => { + test("forced login rejects a split token without losing the previous login", async () => { + await ctx.setAuthToken(TEST_TOKEN); + const result = await ctx.run([ + "auth", + "login", + "--force", + "--token", + "synthetic-secret-prefix\nsynthetic-secret-tail", + "--url", + "https://different.example.com", + ]); + expect(result.exitCode).toBe(EXIT.AUTH_INVALID); + expect(result.stdout + result.stderr).toContain("single line"); + expect(result.stdout + result.stderr).not.toContain("synthetic-secret"); + + const stored = await ctx.run(["auth", "token"]); + expect(stored.exitCode).toBe(0); + expect(stored.stdout.trim()).toBe(TEST_TOKEN); + // The previous credential must still work against its original host. + expect((await ctx.run(["auth", "whoami"])).exitCode).toBe(0); + }); + + test("malformed legacy JSON does not prevent help or logout", async () => { + const path = join(ctx.configDir, "config.json"); + const contents = JSON.stringify({ + auth: { token: "synthetic-secret-prefix\0synthetic-secret-tail" }, + }); + await writeFile(path, contents); + + for (const args of [["--help"], ["auth", "logout"]]) { + const result = await ctx.run(args); + expect(result.exitCode).toBe(0); + expect(result.stdout + result.stderr).not.toContain("synthetic-secret"); + expect(await readFile(path, "utf8")).toBe(contents); + } + }); +}); + describe("sentry auth logout", () => { test("clears stored auth", { timeout: 15_000 }, async () => { // First login (--url required for non-SaaS mock server) diff --git a/packages/cli/test/lib/auth-header.property.test.ts b/packages/cli/test/lib/auth-header.property.test.ts new file mode 100644 index 0000000000..3b3fa3f10f --- /dev/null +++ b/packages/cli/test/lib/auth-header.property.test.ts @@ -0,0 +1,90 @@ +/** Invariants for opaque bearer credentials and removable edge padding. */ + +import { + array, + constantFrom, + assert as fcAssert, + integer, + property, +} from "fast-check"; +import { describe, expect, test } from "vitest"; +import { + formatAuthHeader, + normalizeAuthToken, + trimAuthToken, +} from "../../src/lib/auth-header.js"; +import { MalformedAuthTokenError } from "../../src/lib/errors.js"; +import { DEFAULT_NUM_RUNS } from "../model-based/helpers.js"; + +const printable = array(integer({ min: 0x21, max: 0x7e }), { + minLength: 1, + maxLength: 80, +}).map((codes) => String.fromCharCode(...codes)); +const paddingCharacter = constantFrom( + ...Array.from({ length: 33 }, (_, code) => String.fromCharCode(code)), + "\x7f", + "\u00a0", + "\ufeff" +); +const padding = array(paddingCharacter, { maxLength: 20 }).map((chars) => + chars.join("") +); + +describe("auth token normalization", () => { + test("preserves every printable credential regardless of surrounding padding", () => { + fcAssert( + property(printable, padding, padding, (token, before, after) => { + const input = before + token + after; + expect(trimAuthToken(input)).toBe(token); + expect(normalizeAuthToken(input)).toBe(token); + expect(normalizeAuthToken(normalizeAuthToken(input))).toBe(token); + expect(formatAuthHeader(input)).toBe(`Bearer ${token}`); + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("rejects padding inside a credential without removing it", () => { + fcAssert( + property( + printable, + paddingCharacter, + printable, + (before, char, after) => { + const input = before + char + after; + expect(trimAuthToken(input)).toBe(input); + expect(() => normalizeAuthToken(input)).toThrow( + MalformedAuthTokenError + ); + } + ), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test("rejects empty or padding-only credentials", () => { + fcAssert( + property(padding, (input) => { + expect(() => normalizeAuthToken(input)).toThrow( + MalformedAuthTokenError + ); + }), + { numRuns: DEFAULT_NUM_RUNS } + ); + }); + + test.each([ + "\x80", + "\x85", + "\u200b", + "é", + "💥", + ])("does not silently remove other Unicode characters %#", (char) => { + expect(() => normalizeAuthToken(`${char}token`)).toThrow( + MalformedAuthTokenError + ); + expect(() => normalizeAuthToken(`token${char}`)).toThrow( + MalformedAuthTokenError + ); + }); +}); diff --git a/packages/cli/test/lib/db/auth.property.test.ts b/packages/cli/test/lib/db/auth.property.test.ts index 5d61385d4d..aafce51296 100644 --- a/packages/cli/test/lib/db/auth.property.test.ts +++ b/packages/cli/test/lib/db/auth.property.test.ts @@ -14,6 +14,7 @@ import { option, property, string, + stringMatching, } from "fast-check"; import { afterEach, beforeEach, describe, expect, test } from "vitest"; import { @@ -36,6 +37,9 @@ const tokenArb = string({ minLength: 1, maxLength: 100 }).filter( (s) => s.trim().length > 0 ); +/** Stored tokens must satisfy the persistence boundary; malformed inputs have separate coverage. */ +const storedTokenArb = stringMatching(/^[\x21-\x7e]{1,100}$/); + /** Save and restore env vars around each test */ let savedAuthToken: string | undefined; let savedSentryToken: string | undefined; @@ -87,7 +91,7 @@ describe("property: env var priority", () => { test("stored OAuth wins over env var (default behavior)", () => { fcAssert( - property(tokenArb, tokenArb, (envToken, storedToken) => { + property(tokenArb, storedTokenArb, (envToken, storedToken) => { resetAuthCaches(); setAuthToken(storedToken); process.env.SENTRY_AUTH_TOKEN = envToken; @@ -102,7 +106,7 @@ describe("property: env var priority", () => { test("SENTRY_FORCE_ENV_TOKEN overrides stored OAuth", () => { fcAssert( - property(tokenArb, tokenArb, (envToken, storedToken) => { + property(tokenArb, storedTokenArb, (envToken, storedToken) => { resetAuthCaches(); setAuthToken(storedToken); process.env.SENTRY_AUTH_TOKEN = envToken; @@ -124,7 +128,7 @@ describe("property: env var priority", () => { test("stored token used when no env vars set", () => { fcAssert( - property(tokenArb, (storedToken) => { + property(storedTokenArb, (storedToken) => { resetAuthCaches(); setAuthToken(storedToken); @@ -171,7 +175,7 @@ describe("property: env tokens never trigger refresh", () => { describe("property: isEnvTokenActive consistency", () => { test("when no env token, getAuthConfig never returns env source", () => { fcAssert( - property(option(tokenArb), (storedTokenOpt) => { + property(option(storedTokenArb), (storedTokenOpt) => { resetAuthCaches(); // Clean slate — no env tokens delete process.env.SENTRY_AUTH_TOKEN; @@ -195,7 +199,7 @@ describe("property: isEnvTokenActive consistency", () => { test("stored OAuth takes priority: getAuthConfig returns oauth even when env token is set", () => { fcAssert( - property(tokenArb, tokenArb, (envToken, storedToken) => { + property(tokenArb, storedTokenArb, (envToken, storedToken) => { resetAuthCaches(); process.env.SENTRY_AUTH_TOKEN = envToken; setAuthToken(storedToken); diff --git a/packages/cli/test/lib/db/auth.test.ts b/packages/cli/test/lib/db/auth.test.ts index be4cf47b39..7df11d0786 100644 --- a/packages/cli/test/lib/db/auth.test.ts +++ b/packages/cli/test/lib/db/auth.test.ts @@ -27,6 +27,7 @@ import { setAuthToken, } from "../../../src/lib/db/auth.js"; import { getDatabase } from "../../../src/lib/db/index.js"; +import { MalformedAuthTokenError } from "../../../src/lib/errors.js"; import { useTestConfigDir } from "../../helpers.js"; useTestConfigDir("auth-env-"); @@ -164,6 +165,38 @@ describe("env var auth: refreshToken edge cases", () => { }); describe("env var auth: getRawEnvToken", () => { + test.each([ + "SENTRY_AUTH_TOKEN", + "SENTRY_TOKEN", + ] as const)("shares normalization and source selection for %s", (source) => { + process.env[source] = "\x01\u00a0synthetic-token\x7f"; + expect(getRawEnvToken()).toBe("synthetic-token"); + expect(getAuthToken()).toBe("synthetic-token"); + expect(getAuthConfig()).toMatchObject({ + token: "synthetic-token", + source: `env:${source}`, + }); + expect(getActiveEnvVarName()).toBe(source); + }); + + test("keeps a control-only primary credential selected over the alias", () => { + process.env.SENTRY_AUTH_TOKEN = "\x01\x7f"; + process.env.SENTRY_TOKEN = "secondary-token"; + expect(getRawEnvToken()).toBe("\x01\x7f"); + expect(getAuthConfig()).toMatchObject({ + token: "\x01\x7f", + source: "env:SENTRY_AUTH_TOKEN", + }); + expect(getActiveEnvVarName()).toBe("SENTRY_AUTH_TOKEN"); + }); + + test("still selects the alias when the primary is only whitespace", () => { + process.env.SENTRY_AUTH_TOKEN = " \t\n\u00a0"; + process.env.SENTRY_TOKEN = "\x01secondary-token\x7f"; + expect(getRawEnvToken()).toBe("secondary-token"); + expect(getActiveEnvVarName()).toBe("SENTRY_TOKEN"); + }); + test("returns SENTRY_TOKEN when SENTRY_AUTH_TOKEN is unset", () => { process.env.SENTRY_TOKEN = "fallback_token"; expect(getRawEnvToken()).toBe("fallback_token"); @@ -174,6 +207,31 @@ describe("env var auth: getRawEnvToken", () => { }); }); +describe("stored credential validation", () => { + test("normalizes before SQLite can truncate surrounding NULs", () => { + setAuthToken("\0\u00a0stored-token\x7f\0", 3600, "refresh-token"); + expect(getAuthConfig()).toMatchObject({ + token: "stored-token", + refreshToken: "refresh-token", + }); + }); + + test.each([ + "prefix\0secret-tail", + "prefix\nsecret-tail", + "", + "\x01\x7f", + ])("rejects a malformed replacement without changing stored credentials %#", (token) => { + setAuthToken("previous-token", 3600, "previous-refresh-token"); + const before = getDatabase().query("SELECT * FROM auth").get(); + expect(() => setAuthToken(token, 60, "new-refresh-token")).toThrow( + MalformedAuthTokenError + ); + expect(getDatabase().query("SELECT * FROM auth").get()).toEqual(before); + expect(getAuthToken()).toBe("previous-token"); + }); +}); + describe("OAuth-preferred auth (#646)", () => { test("getAuthConfig prefers stored OAuth over env token", () => { setAuthToken("stored_oauth", 3600); @@ -316,7 +374,7 @@ describe("getIdentityFingerprint", () => { const fp = getIdentityFingerprint(); // With no DB row, same env token should produce the same fingerprint. - setAuthToken("", -1); + getDatabase().query("DELETE FROM auth").run(); resetIdentityFingerprintCache(); expect(getIdentityFingerprint()).toBe(fp); }); diff --git a/packages/cli/test/lib/db/migration.test.ts b/packages/cli/test/lib/db/migration.test.ts new file mode 100644 index 0000000000..745cf57009 --- /dev/null +++ b/packages/cli/test/lib/db/migration.test.ts @@ -0,0 +1,132 @@ +/** Regression coverage for credentials in legacy JSON configuration. */ + +import { existsSync, readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { + clearAuth, + getAuthConfig, + setAuthToken, +} from "../../../src/lib/db/auth.js"; +import { + getDefaultOrganization, + getDefaultProject, +} from "../../../src/lib/db/defaults.js"; +import { closeDatabase, getDatabase } from "../../../src/lib/db/index.js"; +import { clearMetadata, getMetadata } from "../../../src/lib/db/utils.js"; +import { useEnvSandbox, useTestConfigDir } from "../../helpers.js"; + +const getConfigDir = useTestConfigDir("json-auth-migration-"); +useEnvSandbox(["SENTRY_AUTH_TOKEN", "SENTRY_TOKEN", "SENTRY_FORCE_ENV_TOKEN"]); + +let stderr: ReturnType>; + +beforeEach(() => { + stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); +}); + +afterEach(() => { + stderr.mockRestore(); +}); + +function writeLegacyConfig(token: unknown) { + const path = join(getConfigDir(), "config.json"); + const contents = JSON.stringify({ + auth: { token, refreshToken: "synthetic-legacy-refresh" }, + defaults: { organization: "synthetic-org", project: "synthetic-project" }, + projectCache: { + "synthetic-cache-key": { + orgSlug: "synthetic-org", + orgName: "Synthetic organization", + projectSlug: "synthetic-project", + projectName: "Synthetic project", + cachedAt: Date.now(), + }, + }, + }); + writeFileSync(path, contents); + return { path, contents }; +} + +describe("legacy auth migration", () => { + test("normalizes surrounding ASCII controls before writing credentials", () => { + const { path } = writeLegacyConfig("\0\x01\t synthetic-token \r\n\0"); + + expect(getAuthConfig()).toMatchObject({ + token: "synthetic-token", + refreshToken: "synthetic-legacy-refresh", + }); + expect(existsSync(path)).toBe(false); + }); + + test.each([ + ["internal NUL", "synthetic-secret\0tail"], + ["internal LF", "synthetic-secret\ntail"], + ["empty string", ""], + ["number", 123], + ["null", null], + ["object", { value: "synthetic-secret" }], + ])("preserves the original config and migrates other settings for %s", (_, token) => { + const { path, contents } = writeLegacyConfig(token); + + // Opening the DB must remain possible so login/logout can recover. + const db = getDatabase(); + expect(getAuthConfig()).toBeUndefined(); + expect(db.query("SELECT * FROM auth").get()).toBeNull(); + expect(getDefaultOrganization()).toBe("synthetic-org"); + expect(getDefaultProject()).toBe("synthetic-project"); + expect( + db + .query("SELECT org_slug FROM project_cache WHERE cache_key = ?") + .get("synthetic-cache-key") + ).toEqual({ org_slug: "synthetic-org" }); + expect( + getMetadata(db, ["json_migration_completed"]).get( + "json_migration_completed" + ) + ).toBe("true"); + expect(readFileSync(path, "utf8")).toBe(contents); + + const output = stderr.mock.calls.map(([chunk]) => String(chunk)).join(""); + expect(output).toContain( + "Malformed authentication credentials were not migrated" + ); + expect(output).toContain("config.json was kept"); + expect(output).toContain("sentry auth login"); + expect(output).not.toContain("synthetic-secret"); + expect(output).not.toContain("synthetic-legacy-refresh"); + }); + + test("keeps an existing SQLite session when legacy credentials are invalid", () => { + setAuthToken( + "synthetic-existing-token", + 3600, + "synthetic-existing-refresh" + ); + const before = getAuthConfig(); + const db = getDatabase(); + clearMetadata(db, ["json_migration_completed"]); + closeDatabase(); + const { path, contents } = writeLegacyConfig("synthetic-secret\0tail"); + + expect(getAuthConfig()).toEqual(before); + expect(getDefaultOrganization()).toBe("synthetic-org"); + expect(readFileSync(path, "utf8")).toBe(contents); + }); + + test("does not re-import the retained file after a new login or logout", async () => { + const { path } = writeLegacyConfig("synthetic-secret\0tail"); + expect(getAuthConfig()).toBeUndefined(); + + setAuthToken("synthetic-replacement-token"); + // Even fixing the retained file must not overwrite the new session. + writeLegacyConfig("synthetic-old-token"); + closeDatabase(); + expect(getAuthConfig()?.token).toBe("synthetic-replacement-token"); + + await clearAuth(); + closeDatabase(); + expect(getAuthConfig()).toBeUndefined(); + expect(existsSync(path)).toBe(true); + }); +}); diff --git a/packages/cli/test/lib/db/model-based.test.ts b/packages/cli/test/lib/db/model-based.test.ts index 4e10b9a65d..1091f2b8d7 100644 --- a/packages/cli/test/lib/db/model-based.test.ts +++ b/packages/cli/test/lib/db/model-based.test.ts @@ -27,6 +27,7 @@ import { option, property, string, + stringMatching, tuple, } from "fast-check"; import { describe, expect, test } from "vitest"; @@ -631,8 +632,9 @@ class GetVersionCheckCommand implements AsyncCommand { // Arbitraries (Random Data Generators) -/** Generate valid token strings */ +/** Env/refresh candidates retain their existing domain; stored access tokens must be valid. */ const tokenArb = string({ minLength: 1, maxLength: 64 }); +const storedTokenArb = stringMatching(/^[\x21-\x7e]{1,64}$/); /** Generate org/project slugs (alphanumeric with hyphens) */ const slugChars = "abcdefghijklmnopqrstuvwxyz0123456789"; @@ -677,7 +679,7 @@ const expiresInArb = option(integer({ min: -10, max: 7200 }), { // Command Arbitraries const setAuthTokenCmdArb = tuple( - tokenArb, + storedTokenArb, expiresInArb, option(tokenArb, { nil: undefined }) ).map( @@ -918,7 +920,7 @@ describe("model-based: database layer", () => { test("expired tokens return undefined", () => { fcAssert( - property(tokenArb, (token) => { + property(storedTokenArb, (token) => { const cleanup = createIsolatedDbContext(); const savedAuthToken = process.env.SENTRY_AUTH_TOKEN; delete process.env.SENTRY_AUTH_TOKEN; diff --git a/packages/cli/test/lib/env-token-host.test.ts b/packages/cli/test/lib/env-token-host.test.ts index 2f635f78ec..3d51d66e63 100644 --- a/packages/cli/test/lib/env-token-host.test.ts +++ b/packages/cli/test/lib/env-token-host.test.ts @@ -14,15 +14,32 @@ import { getEnvTokenHost, resetEnvTokenHostForTesting, } from "../../src/lib/env-token-host.js"; -import { useEnvSandbox } from "../helpers.js"; +import { mintSntrysToken, useEnvSandbox } from "../helpers.js"; -const ENV_KEYS = ["SENTRY_HOST", "SENTRY_URL"] as const; +const ENV_KEYS = [ + "SENTRY_HOST", + "SENTRY_URL", + "SENTRY_AUTH_TOKEN", + "SENTRY_TOKEN", +] as const; describe("env-token-host", () => { useEnvSandbox(ENV_KEYS); beforeEach(resetEnvTokenHostForTesting); afterEach(resetEnvTokenHostForTesting); + test("keeps the claim's host authoritative for a token wrapped in controls", () => { + const token = mintSntrysToken({ + iat: 1, + url: "https://self-hosted.example.com", + org: "synthetic-org", + }); + process.env.SENTRY_AUTH_TOKEN = `\x01\u00a0${token}\x7f`; + process.env.SENTRY_HOST = "https://different.example.com"; + captureEnvTokenHost(); + expect(getEnvTokenHost()).toBe("https://self-hosted.example.com"); + }); + test("defaults to SaaS when neither SENTRY_HOST nor SENTRY_URL is set", () => { captureEnvTokenHost(); expect(getEnvTokenHost()).toBe(DEFAULT_SENTRY_URL); diff --git a/packages/cli/test/lib/sentry-client.auth.test.ts b/packages/cli/test/lib/sentry-client.auth.test.ts index 1efe62759c..13c291f6f5 100644 --- a/packages/cli/test/lib/sentry-client.auth.test.ts +++ b/packages/cli/test/lib/sentry-client.auth.test.ts @@ -3,6 +3,7 @@ import { afterEach, beforeEach, describe, expect, test } from "vitest"; import { shouldAutoAuth } from "../../src/lib/auto-auth.js"; import { getAuthConfig, setAuthToken } from "../../src/lib/db/auth.js"; +import { getDatabase } from "../../src/lib/db/index.js"; import { setEnv } from "../../src/lib/env.js"; import { AuthError, @@ -11,6 +12,10 @@ import { MalformedAuthTokenError, withAuthGuard, } from "../../src/lib/errors.js"; +import { + getCachedResponse, + storeCachedResponse, +} from "../../src/lib/response-cache.js"; import { getSdkConfig, resetAuthenticatedFetch, @@ -36,6 +41,9 @@ const MALFORMED_TOKEN = ORG_TOKEN.replace( "test-secret-tail", "te\nst-secret-tail" ); +const EDGE_CONTROLS = `${Array.from({ length: 32 }, (_, code) => + String.fromCharCode(code) +).join("")}\x7f`; describe("authenticated fetch bearer validation", () => { useTestConfigDir("sentry-client-auth-"); @@ -75,12 +83,18 @@ describe("authenticated fetch bearer validation", () => { return getSdkConfig(REGION_URL).fetch(RESOURCE_URL); } + /** Simulate credentials persisted before setAuthToken validated its input. */ + function storeLegacyToken(token: string): void { + setAuthToken("legacy-token"); + getDatabase().query("UPDATE auth SET token = ? WHERE id = 1").run(token); + } + test.each([ ...ENV_TOKEN_KEYS, "stored", ])("rejects an internal LF from %s before any request or auth fallback", async (source) => { if (source === "stored") { - setAuthToken(MALFORMED_TOKEN); + storeLegacyToken(MALFORMED_TOKEN); } else { process.env[source] = MALFORMED_TOKEN; } @@ -145,11 +159,28 @@ describe("authenticated fetch bearer validation", () => { expect(requests[0]?.authorization).toBe("Bearer synthetic-token"); }); + test.each( + ENV_TOKEN_KEYS + )("trims surrounding C0, DEL and whitespace from SDK %s", async (key) => { + // An isolated SDK environment preserves NULs that process.env cannot. + setEnv({ + ...process.env, + [key]: `${EDGE_CONTROLS}\u00a0synthetic-token\ufeff${EDGE_CONTROLS}`, + }); + + await request(); + + expect(requests).toEqual([ + { url: RESOURCE_URL, authorization: "Bearer synthetic-token" }, + ]); + }); + test.each([ " stored-token ", "\t\n\u00a0stored-token\r\n", - ])("trims surrounding whitespace from stored credentials %#", async (token) => { - setAuthToken(token); + "\x1f\u00a0stored-token\ufeff\x7f", + ])("trims surrounding controls and whitespace from legacy credentials %#", async (token) => { + storeLegacyToken(token); await request(); expect(requests).toEqual([ { url: RESOURCE_URL, authorization: "Bearer stored-token" }, @@ -157,7 +188,7 @@ describe("authenticated fetch bearer validation", () => { }); test("rejects a whitespace-only stored credential", async () => { - setAuthToken(" \t\r\n "); + storeLegacyToken(" \t\r\n "); await expect(request()).rejects.toMatchObject({ name: "MalformedAuthTokenError", reason: "invalid", @@ -166,6 +197,63 @@ describe("authenticated fetch bearer validation", () => { expect(requests).toEqual([]); }); + test("looks up Vary: Authorization using the normalized legacy credential", async () => { + storeLegacyToken("\x1fsynthetic-token\x7f"); + await storeCachedResponse( + "GET", + RESOURCE_URL, + { authorization: "Bearer synthetic-token" }, + Response.json( + { source: "cache" }, + { + headers: { + "Cache-Control": "private, max-age=300", + Vary: "Authorization", + }, + } + ) + ); + + expect(await (await request()).json()).toEqual({ source: "cache" }); + expect(requests).toEqual([]); + }); + + test("stores Vary: Authorization with the credential actually sent", async () => { + storeLegacyToken("\x1fsynthetic-token\x7f"); + globalThis.fetch = mockFetch((input, init) => { + requests.push({ + url: extractFetchUrl(input), + authorization: new Headers(init?.headers).get("Authorization"), + }); + return Promise.resolve( + Response.json( + { source: "network" }, + { + headers: { + "Cache-Control": "private, max-age=300", + Vary: "Authorization", + }, + } + ) + ); + }); + + await request(); + + // Cache writes are fire-and-forget; wait for the entry rather than sleeping. + await expect + .poll(async () => { + const cached = await getCachedResponse("GET", RESOURCE_URL, { + authorization: "Bearer synthetic-token", + }); + return cached?.json(); + }) + .toEqual({ source: "network" }); + expect(requests).toEqual([ + { url: RESOURCE_URL, authorization: "Bearer synthetic-token" }, + ]); + }); + test("checks token host claims after removing surrounding whitespace", async () => { const token = mintSntrysToken({ iat: 1, @@ -197,9 +285,37 @@ describe("authenticated fetch bearer validation", () => { expect(requests).toEqual([]); }); + test.each([ + { key: "SENTRY_AUTH_TOKEN", forceEnv: false }, + { key: "SENTRY_AUTH_TOKEN", forceEnv: true }, + { key: "SENTRY_TOKEN", forceEnv: true }, + ])("rejects control-only $key without fallback (force-env=$forceEnv)", async ({ + key, + forceEnv, + }) => { + if (forceEnv) { + setAuthToken("stored-token"); + } + setEnv({ + ...process.env, + SENTRY_AUTH_TOKEN: undefined, + SENTRY_TOKEN: "alias-token", + SENTRY_FORCE_ENV_TOKEN: forceEnv ? "1" : undefined, + [key]: "\0\x01\x7f", + }); + + await expect(request()).rejects.toMatchObject({ + name: "MalformedAuthTokenError", + reason: "invalid", + exitCode: EXIT.AUTH_INVALID, + }); + expect(requests).toEqual([]); + }); + test.each([ "refreshed-token", " \nrefreshed-token\r\t", + "\x1f\nrefreshed-token\r\x7f", ])("retries with a normalized valid refreshed bearer %#", async (token) => { process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; setAuthToken("stored-token", 3600, "synthetic-refresh-token"); @@ -283,8 +399,14 @@ describe("authenticated fetch bearer validation", () => { expect(requests).toEqual([...refreshAttempt, ...refreshAttempt]); }); - test("normalizes a proactive refresh before storing and using the token", async () => { + test.each([ + "none", + ...ENV_TOKEN_KEYS, + ])("normalizes a proactive refresh with malformed env source %s", async (source) => { process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; + if (source !== "none") { + process.env[source] = MALFORMED_TOKEN; + } setAuthToken("expired-token", -1, "synthetic-refresh-token"); globalThis.fetch = mockFetch((input, init) => { const url = extractFetchUrl(input); @@ -295,7 +417,7 @@ describe("authenticated fetch bearer validation", () => { return Promise.resolve( url.endsWith("/oauth/token/") ? Response.json({ - access_token: " \nrefreshed-token\r\t", + access_token: "\x1f \nrefreshed-token\r\t\x7f", token_type: "bearer", expires_in: 3600, refresh_token: "replacement-refresh-token", From 0e53033079101f5b95360c42e65c6b9973d645c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20Beteg=C3=B3n?= Date: Mon, 28 Sep 2026 19:49:40 +0200 Subject: [PATCH 4/4] test(auth): simplify credential regression coverage --- packages/cli/test/e2e/auth.test.ts | 7 +- .../cli/test/lib/db/auth.property.test.ts | 50 +----- packages/cli/test/lib/db/auth.test.ts | 47 +----- .../cli/test/lib/sentry-client.auth.test.ts | 147 +++++++----------- 4 files changed, 70 insertions(+), 181 deletions(-) diff --git a/packages/cli/test/e2e/auth.test.ts b/packages/cli/test/e2e/auth.test.ts index 54281f127e..efd9a2f9a8 100644 --- a/packages/cli/test/e2e/auth.test.ts +++ b/packages/cli/test/e2e/auth.test.ts @@ -146,11 +146,8 @@ describe("sentry auth whoami", () => { expect(result.exitCode).toBe(EXIT.AUTH_NOT_AUTHENTICATED); }); - test.each([ - TEST_TOKEN, - ` \n${TEST_TOKEN}\r\t`, - ])("shows current user identity with surrounding token whitespace %#", async (token) => { - await ctx.setAuthToken(token); + test("shows current user identity", async () => { + await ctx.setAuthToken(TEST_TOKEN); const result = await ctx.run(["auth", "whoami"]); diff --git a/packages/cli/test/lib/db/auth.property.test.ts b/packages/cli/test/lib/db/auth.property.test.ts index aafce51296..db27388ea9 100644 --- a/packages/cli/test/lib/db/auth.property.test.ts +++ b/packages/cli/test/lib/db/auth.property.test.ts @@ -3,7 +3,7 @@ * * Verifies invariants that must hold for any valid token values: * - SENTRY_AUTH_TOKEN always takes priority over SENTRY_TOKEN - * - Env vars always take priority over stored tokens + * - Stored OAuth takes priority unless SENTRY_FORCE_ENV_TOKEN is set * - Env tokens never trigger refresh * - AuthConfig.source correctly identifies the origin */ @@ -16,7 +16,7 @@ import { string, stringMatching, } from "fast-check"; -import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { describe, expect, test } from "vitest"; import { type AuthSource, getAuthConfig, @@ -27,10 +27,11 @@ import { resetAuthTokenCache, setAuthToken, } from "../../../src/lib/db/auth.js"; -import { useTestConfigDir } from "../../helpers.js"; +import { useEnvSandbox, useTestConfigDir } from "../../helpers.js"; import { DEFAULT_NUM_RUNS } from "../../model-based/helpers.js"; useTestConfigDir("auth-prop-"); +useEnvSandbox(["SENTRY_AUTH_TOKEN", "SENTRY_TOKEN", "SENTRY_FORCE_ENV_TOKEN"]); /** Arbitrary for non-empty, trimmed token strings */ const tokenArb = string({ minLength: 1, maxLength: 100 }).filter( @@ -40,32 +41,6 @@ const tokenArb = string({ minLength: 1, maxLength: 100 }).filter( /** Stored tokens must satisfy the persistence boundary; malformed inputs have separate coverage. */ const storedTokenArb = stringMatching(/^[\x21-\x7e]{1,100}$/); -/** Save and restore env vars around each test */ -let savedAuthToken: string | undefined; -let savedSentryToken: string | undefined; - -beforeEach(() => { - savedAuthToken = process.env.SENTRY_AUTH_TOKEN; - savedSentryToken = process.env.SENTRY_TOKEN; - delete process.env.SENTRY_AUTH_TOKEN; - delete process.env.SENTRY_TOKEN; - resetAuthTokenCache(); - resetAuthRowCache(); -}); - -afterEach(() => { - if (savedAuthToken !== undefined) { - process.env.SENTRY_AUTH_TOKEN = savedAuthToken; - } else { - delete process.env.SENTRY_AUTH_TOKEN; - } - if (savedSentryToken !== undefined) { - process.env.SENTRY_TOKEN = savedSentryToken; - } else { - delete process.env.SENTRY_TOKEN; - } -}); - /** Invalidate between property iterations — env-var mutations bypass setAuthToken. */ function resetAuthCaches() { resetAuthTokenCache(); @@ -99,6 +74,7 @@ describe("property: env var priority", () => { // Stored OAuth takes priority — env token is for build tooling expect(getAuthToken()).toBe(storedToken); expect(getAuthConfig()?.source).toBe("oauth" satisfies AuthSource); + expect(isEnvTokenActive()).toBe(true); }), { numRuns: DEFAULT_NUM_RUNS } ); @@ -196,22 +172,6 @@ describe("property: isEnvTokenActive consistency", () => { { numRuns: DEFAULT_NUM_RUNS } ); }); - - test("stored OAuth takes priority: getAuthConfig returns oauth even when env token is set", () => { - fcAssert( - property(tokenArb, storedTokenArb, (envToken, storedToken) => { - resetAuthCaches(); - process.env.SENTRY_AUTH_TOKEN = envToken; - setAuthToken(storedToken); - - const config = getAuthConfig(); - expect(config?.source).toBe("oauth"); - // But isEnvTokenActive is still true (env token exists) - expect(isEnvTokenActive()).toBe(true); - }), - { numRuns: DEFAULT_NUM_RUNS } - ); - }); }); describe("property: source round-trip", () => { diff --git a/packages/cli/test/lib/db/auth.test.ts b/packages/cli/test/lib/db/auth.test.ts index 7df11d0786..a06d48bffa 100644 --- a/packages/cli/test/lib/db/auth.test.ts +++ b/packages/cli/test/lib/db/auth.test.ts @@ -7,7 +7,7 @@ * by property tests (isAuthenticated, getActiveEnvVarName). */ -import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { describe, expect, test } from "vitest"; import { ANON_IDENTITY, clearAuth, @@ -20,7 +20,6 @@ import { isAuthenticated, isEnvTokenActive, refreshToken, - resetAuthRowCache, resetAuthTokenCache, resetHasStoredCredsCache, resetIdentityFingerprintCache, @@ -28,35 +27,10 @@ import { } from "../../../src/lib/db/auth.js"; import { getDatabase } from "../../../src/lib/db/index.js"; import { MalformedAuthTokenError } from "../../../src/lib/errors.js"; -import { useTestConfigDir } from "../../helpers.js"; +import { useEnvSandbox, useTestConfigDir } from "../../helpers.js"; useTestConfigDir("auth-env-"); - -let savedAuthToken: string | undefined; -let savedSentryToken: string | undefined; - -beforeEach(() => { - savedAuthToken = process.env.SENTRY_AUTH_TOKEN; - savedSentryToken = process.env.SENTRY_TOKEN; - delete process.env.SENTRY_AUTH_TOKEN; - delete process.env.SENTRY_TOKEN; - resetIdentityFingerprintCache(); - resetAuthTokenCache(); - resetAuthRowCache(); -}); - -afterEach(() => { - if (savedAuthToken !== undefined) { - process.env.SENTRY_AUTH_TOKEN = savedAuthToken; - } else { - delete process.env.SENTRY_AUTH_TOKEN; - } - if (savedSentryToken !== undefined) { - process.env.SENTRY_TOKEN = savedSentryToken; - } else { - delete process.env.SENTRY_TOKEN; - } -}); +useEnvSandbox(["SENTRY_AUTH_TOKEN", "SENTRY_TOKEN", "SENTRY_FORCE_ENV_TOKEN"]); describe("env var auth: getAuthToken edge cases", () => { test("ignores empty SENTRY_AUTH_TOKEN", () => { @@ -106,16 +80,6 @@ describe("env var auth: isEnvTokenActive edge case", () => { }); describe("env var auth: getActiveEnvVarName", () => { - test("returns SENTRY_AUTH_TOKEN when that var is set", () => { - process.env.SENTRY_AUTH_TOKEN = "test_token"; - expect(getActiveEnvVarName()).toBe("SENTRY_AUTH_TOKEN"); - }); - - test("returns SENTRY_TOKEN when only that var is set", () => { - process.env.SENTRY_TOKEN = "test_token"; - expect(getActiveEnvVarName()).toBe("SENTRY_TOKEN"); - }); - test("prefers SENTRY_AUTH_TOKEN when both are set", () => { process.env.SENTRY_AUTH_TOKEN = "primary"; process.env.SENTRY_TOKEN = "secondary"; @@ -197,11 +161,6 @@ describe("env var auth: getRawEnvToken", () => { expect(getActiveEnvVarName()).toBe("SENTRY_TOKEN"); }); - test("returns SENTRY_TOKEN when SENTRY_AUTH_TOKEN is unset", () => { - process.env.SENTRY_TOKEN = "fallback_token"; - expect(getRawEnvToken()).toBe("fallback_token"); - }); - test("returns undefined when no env var is set", () => { expect(getRawEnvToken()).toBeUndefined(); }); diff --git a/packages/cli/test/lib/sentry-client.auth.test.ts b/packages/cli/test/lib/sentry-client.auth.test.ts index 13c291f6f5..ad50c18e01 100644 --- a/packages/cli/test/lib/sentry-client.auth.test.ts +++ b/packages/cli/test/lib/sentry-client.auth.test.ts @@ -58,18 +58,24 @@ describe("authenticated fetch bearer validation", () => { let originalFetch: typeof globalThis.fetch; let requests: { url: string; authorization: string | null }[]; + /** Mock responses while recording the actual request URL and Authorization. */ + function mockResponses( + respond: (url: string, authorization: string | null) => Response + ): typeof fetch { + return mockFetch((input, init) => { + const url = extractFetchUrl(input); + const authorization = new Headers(init?.headers).get("Authorization"); + requests.push({ url, authorization }); + return Promise.resolve(respond(url, authorization)); + }); + } + beforeEach(async () => { await resetHostScopingState(); resetAuthenticatedFetch(); originalFetch = globalThis.fetch; requests = []; - globalThis.fetch = mockFetch((input, init) => { - requests.push({ - url: extractFetchUrl(input), - authorization: new Headers(init?.headers).get("Authorization"), - }); - return Promise.resolve(new Response("{}", { status: 200 })); - }); + globalThis.fetch = mockResponses(() => new Response("{}", { status: 200 })); }); afterEach(async () => { @@ -220,23 +226,17 @@ describe("authenticated fetch bearer validation", () => { test("stores Vary: Authorization with the credential actually sent", async () => { storeLegacyToken("\x1fsynthetic-token\x7f"); - globalThis.fetch = mockFetch((input, init) => { - requests.push({ - url: extractFetchUrl(input), - authorization: new Headers(init?.headers).get("Authorization"), - }); - return Promise.resolve( - Response.json( - { source: "network" }, - { - headers: { - "Cache-Control": "private, max-age=300", - Vary: "Authorization", - }, - } - ) - ); - }); + globalThis.fetch = mockResponses(() => + Response.json( + { source: "network" }, + { + headers: { + "Cache-Control": "private, max-age=300", + Vary: "Authorization", + }, + } + ) + ); await request(); @@ -260,7 +260,7 @@ describe("authenticated fetch bearer validation", () => { url: "https://other-sentry.example.com", org: "synthetic-org", }); - setAuthToken(` \n${token}\t `); + storeLegacyToken(` \n${token}\t `); await expect(request()).rejects.toBeInstanceOf(HostScopeError); expect(requests).toEqual([]); }); @@ -319,24 +319,17 @@ describe("authenticated fetch bearer validation", () => { ])("retries with a normalized valid refreshed bearer %#", async (token) => { process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; setAuthToken("stored-token", 3600, "synthetic-refresh-token"); - globalThis.fetch = mockFetch((input, init) => { - const url = extractFetchUrl(input); - const authorization = new Headers(init?.headers).get("Authorization"); - requests.push({ url, authorization }); + globalThis.fetch = mockResponses((url, authorization) => { if (url.endsWith("/oauth/token/")) { - return Promise.resolve( - Response.json({ - access_token: token, - token_type: "bearer", - expires_in: 3600, - }) - ); + return Response.json({ + access_token: token, + token_type: "bearer", + expires_in: 3600, + }); } - return Promise.resolve( - new Response("{}", { - status: authorization === "Bearer stored-token" ? 401 : 200, - }) - ); + return new Response("{}", { + status: authorization === "Bearer stored-token" ? 401 : 200, + }); }); expect((await request()).status).toBe(200); @@ -357,23 +350,16 @@ describe("authenticated fetch bearer validation", () => { ])("rejects malformed refreshed credentials without retrying the request %#", async (token) => { process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; setAuthToken("stored-token", 3600, "synthetic-refresh-token"); - globalThis.fetch = mockFetch((input, init) => { - const url = extractFetchUrl(input); - requests.push({ - url, - authorization: new Headers(init?.headers).get("Authorization"), - }); + globalThis.fetch = mockResponses((url) => { if (url.endsWith("/oauth/token/")) { - return Promise.resolve( - Response.json({ - access_token: token, - token_type: "bearer", - expires_in: 3600, - refresh_token: "synthetic-refresh-token", - }) - ); + return Response.json({ + access_token: token, + token_type: "bearer", + expires_in: 3600, + refresh_token: "synthetic-refresh-token", + }); } - return Promise.resolve(new Response("{}", { status: 401 })); + return new Response("{}", { status: 401 }); }); for (let attempt = 0; attempt < 2; attempt++) { @@ -408,23 +394,16 @@ describe("authenticated fetch bearer validation", () => { process.env[source] = MALFORMED_TOKEN; } setAuthToken("expired-token", -1, "synthetic-refresh-token"); - globalThis.fetch = mockFetch((input, init) => { - const url = extractFetchUrl(input); - requests.push({ - url, - authorization: new Headers(init?.headers).get("Authorization"), - }); - return Promise.resolve( - url.endsWith("/oauth/token/") - ? Response.json({ - access_token: "\x1f \nrefreshed-token\r\t\x7f", - token_type: "bearer", - expires_in: 3600, - refresh_token: "replacement-refresh-token", - }) - : Response.json({}) - ); - }); + globalThis.fetch = mockResponses((url) => + url.endsWith("/oauth/token/") + ? Response.json({ + access_token: "\x1f \nrefreshed-token\r\t\x7f", + token_type: "bearer", + expires_in: 3600, + refresh_token: "replacement-refresh-token", + }) + : Response.json({}) + ); expect((await request()).status).toBe(200); expect(getAuthConfig()).toMatchObject({ @@ -440,20 +419,14 @@ describe("authenticated fetch bearer validation", () => { test("rejects malformed proactive refresh before storing or using the token", async () => { process.env.SENTRY_CLIENT_ID = "synthetic-client-id"; setAuthToken("expired-token", -1, "synthetic-refresh-token"); - globalThis.fetch = mockFetch((input, init) => { - requests.push({ - url: extractFetchUrl(input), - authorization: new Headers(init?.headers).get("Authorization"), - }); - return Promise.resolve( - Response.json({ - access_token: "opaque-\0-secret-tail", - token_type: "bearer", - expires_in: 3600, - refresh_token: "replacement-refresh-token", - }) - ); - }); + globalThis.fetch = mockResponses(() => + Response.json({ + access_token: "opaque-\0-secret-tail", + token_type: "bearer", + expires_in: 3600, + refresh_token: "replacement-refresh-token", + }) + ); await expect(request()).rejects.toMatchObject({ reason: "invalid",