From 3701acec7ee067223bcff1897d51ed39bd0a961c Mon Sep 17 00:00:00 2001 From: dan sutton Date: Tue, 6 Oct 2026 13:43:51 -0500 Subject: [PATCH 1/5] fix(client): name Metabase's reason for a 403 instead of blaming the API key A 403 means Metabase accepted the key and refused the request. Metabase answers some validation errors that way with a text/plain reason, such as "A table with that name already exists." for a transform whose target table exists, and a permission refusal with "You don't have permissions to do that." Both read as "Invalid or unauthorized API key", which sent people and agents off debugging authentication. A 403's plain-text reason is now the message; a 401, whose body is only "Unauthenticated", and a 403 without one keep the key message. GHY-4738 Co-Authored-By: Claude Opus 5.5 --- packages/client/src/http/errors.test.ts | 17 +++++++++++++++++ packages/client/src/http/errors.ts | 15 +++++++++++++-- tests/e2e/profiles.e2e.test.ts | 2 +- 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/packages/client/src/http/errors.test.ts b/packages/client/src/http/errors.test.ts index 3ddaadeb..55e2d784 100644 --- a/packages/client/src/http/errors.test.ts +++ b/packages/client/src/http/errors.test.ts @@ -157,6 +157,23 @@ describe("HttpError message extraction", () => { ); }); + it("names Metabase's reason for a 403 with a text/plain body instead of blaming the key", () => { + const refused = buildHttpError({ + status: 403, + responseHeaders: textHeaders(), + rawBody: "A table with that name already exists.", + }); + expect(refused.message).toBe("A table with that name already exists."); + expect(refused.kind).toBe("auth"); + }); + + it("keeps the key message for a 401 whose text/plain body is only Unauthenticated", () => { + expect( + buildHttpError({ status: 401, responseHeaders: textHeaders(), rawBody: "Unauthenticated" }) + .message, + ).toBe("Invalid or unauthorized API key (host: example.invalid)."); + }); + it("falls back to status defaults for 408 and 429 with no body", () => { expect(buildHttpError({ status: 408, rawBody: null }).message).toBe( "Metabase timed out responding.", diff --git a/packages/client/src/http/errors.ts b/packages/client/src/http/errors.ts index 26997d30..66e539a2 100644 --- a/packages/client/src/http/errors.ts +++ b/packages/client/src/http/errors.ts @@ -19,6 +19,7 @@ interface StatusClassification { message?: string; } +const FORBIDDEN_STATUS = 403; const NOT_FOUND_STATUS = 404; const TEXT_CONTENT_TYPE = "text/plain"; @@ -256,16 +257,26 @@ function buildUserMessage( if (fromBody !== null) { return fromBody; } + const fromText = plainTextMessage(sanitizedBody, redactedHeaders); if (kind === "auth") { - return `Invalid or unauthorized API key (host: ${hostFromUrl(input.url)}).`; + return authMessage(input, fromText); } - const fromText = plainTextMessage(sanitizedBody, redactedHeaders); if (fromText !== null) { return fromText; } return defaultMessageForStatus(input.status); } +// A 403 means Metabase accepted the key and refused the request, and its plain-text body says why +// ("A table with that name already exists."). A 401 body ("Unauthenticated") says less than the +// key message does. +function authMessage(input: HttpErrorInput, fromText: string | null): string { + if (input.status === FORBIDDEN_STATUS && fromText !== null) { + return fromText; + } + return `Invalid or unauthorized API key (host: ${hostFromUrl(input.url)}).`; +} + // Metabase answers some rejections — a query that fails normalization, for one — with a text/plain // body that is nothing but the message. Only that content type is read as one: an HTML error page // from whatever sits in front of Metabase is never a message. diff --git a/tests/e2e/profiles.e2e.test.ts b/tests/e2e/profiles.e2e.test.ts index 25b197af..00533094 100644 --- a/tests/e2e/profiles.e2e.test.ts +++ b/tests/e2e/profiles.e2e.test.ts @@ -199,7 +199,7 @@ describe("profiles e2e", () => { configHome, }); expect(limitedQuery.exitCode).toBe(1); - expect(limitedQuery.stderr).toContain("Invalid or unauthorized API key"); + expect(limitedQuery.stderr).toContain("You don't have permissions to do that."); expect(limitedQuery.stdout).toBe(""); }); From f70c48a4b01fdbad4f00d2a705b0f2c191841c8f Mon Sep 17 00:00:00 2001 From: dan sutton Date: Tue, 6 Oct 2026 15:45:18 -0500 Subject: [PATCH 2/5] fix(client): let the status, not the body, decide that a 403 is a refusal A 401 is a credential Metabase did not accept; a 403 is a request it refused from a user it identified. A 403 without a plain-text reason still said "Invalid or unauthorized API key", so the right message depended on Metabase sending a body. It now reads as a refusal either way, and a reason, when present, makes it specific. GHY-4738 Co-Authored-By: Claude Opus 5.5 --- packages/client/src/http/errors.test.ts | 4 ++-- packages/client/src/http/errors.ts | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/client/src/http/errors.test.ts b/packages/client/src/http/errors.test.ts index 55e2d784..fcef67ad 100644 --- a/packages/client/src/http/errors.test.ts +++ b/packages/client/src/http/errors.test.ts @@ -151,9 +151,9 @@ describe("HttpError message extraction", () => { ); }); - it("emits an auth message with the host for 403 with no body", () => { + it("calls a 403 with no body a refusal, not a bad key, since Metabase identified the user", () => { expect(buildHttpError({ status: 403, rawBody: null }).message).toBe( - "Invalid or unauthorized API key (host: example.invalid).", + "Metabase refused the request: the API key's user is not allowed to do this.", ); }); diff --git a/packages/client/src/http/errors.ts b/packages/client/src/http/errors.ts index 66e539a2..096382f2 100644 --- a/packages/client/src/http/errors.ts +++ b/packages/client/src/http/errors.ts @@ -267,14 +267,14 @@ function buildUserMessage( return defaultMessageForStatus(input.status); } -// A 403 means Metabase accepted the key and refused the request, and its plain-text body says why -// ("A table with that name already exists."). A 401 body ("Unauthenticated") says less than the -// key message does. +// The status decides: a 401 is a credential Metabase did not accept, a 403 a request it refused +// from a user it did identify, so a 403 never blames the key. A plain-text reason, when Metabase +// sends one, only makes the refusal specific. function authMessage(input: HttpErrorInput, fromText: string | null): string { - if (input.status === FORBIDDEN_STATUS && fromText !== null) { - return fromText; + if (input.status !== FORBIDDEN_STATUS) { + return `Invalid or unauthorized API key (host: ${hostFromUrl(input.url)}).`; } - return `Invalid or unauthorized API key (host: ${hostFromUrl(input.url)}).`; + return fromText ?? "Metabase refused the request: the API key's user is not allowed to do this."; } // Metabase answers some rejections — a query that fails normalization, for one — with a text/plain From 50b68d905c7d0b53eb7ca2e078a6b73d3fa5de10 Mon Sep 17 00:00:00 2001 From: dan sutton Date: Tue, 6 Oct 2026 15:49:17 -0500 Subject: [PATCH 3/5] fix(client): give 403 and 409 their own error kinds 401 stays `auth`: Metabase did not accept the credential, so the message names the API key. 403 is `forbidden`, a request refused for a user Metabase identified, and 409 is `conflict`, a request that clashes with existing state; neither is about the key. Both show the server's reason and otherwise fall back to a default for their status. The dashboard card check treats `forbidden` like `auth`: the card is unreadable. GHY-4738 Co-Authored-By: Claude Opus 5.5 --- packages/client/README.md | 2 +- packages/client/src/http/errors.test.ts | 29 +++++++++++++++-- packages/client/src/http/errors.ts | 36 +++++++++++++--------- packages/client/src/index.test.ts | 6 ++++ packages/client/src/resources/dashboard.ts | 2 +- 5 files changed, 55 insertions(+), 20 deletions(-) diff --git a/packages/client/README.md b/packages/client/README.md index 61869800..d17b542a 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -389,7 +389,7 @@ carries the request context and the Zod issues, and a hand-decoded body or heade `ConfigError` and `InternalError` split blame: the first is input a caller could correct, the second is a caller that violated a function's contract, which is a bug in the calling code. `HttpError` carries the status and response body, redacted of known secrets at construction, plus a `kind` -(`HttpErrorKind`) separating a route Metabase does not serve from a row that is gone; +(`HttpErrorKind`) separating a route Metabase does not serve from a row that is gone, and a credential Metabase did not accept (`auth`, 401) from a request it refused for an identified user (`forbidden`, 403) or that conflicts with existing state (`conflict`, 409); `isHttpNotFound(value)` answers the coarser question of whether a thrown value is an `HttpError` with status 404. `ChainedRequestError` wraps a cause and delegates its category and retryability to it. `toMetabaseError(unknown)` normalizes a thrown value into the taxonomy. diff --git a/packages/client/src/http/errors.test.ts b/packages/client/src/http/errors.test.ts index fcef67ad..38dc6a0d 100644 --- a/packages/client/src/http/errors.test.ts +++ b/packages/client/src/http/errors.test.ts @@ -164,7 +164,7 @@ describe("HttpError message extraction", () => { rawBody: "A table with that name already exists.", }); expect(refused.message).toBe("A table with that name already exists."); - expect(refused.kind).toBe("auth"); + expect(refused.kind).toBe("forbidden"); }); it("keeps the key message for a 401 whose text/plain body is only Unauthenticated", () => { @@ -174,6 +174,22 @@ describe("HttpError message extraction", () => { ).toBe("Invalid or unauthorized API key (host: example.invalid)."); }); + it("names Metabase's reason for a 409 with a text/plain body", () => { + expect( + buildHttpError({ + status: 409, + responseHeaders: textHeaders(), + rawBody: "A table with that name already exists.", + }).message, + ).toBe("A table with that name already exists."); + }); + + it("calls a 409 with no body a conflict", () => { + expect(buildHttpError({ status: 409, rawBody: null }).message).toBe( + "Metabase refused the request: it conflicts with what already exists.", + ); + }); + it("falls back to status defaults for 408 and 429 with no body", () => { expect(buildHttpError({ status: 408, rawBody: null }).message).toBe( "Metabase timed out responding.", @@ -326,9 +342,16 @@ describe("HttpError field errors", () => { }); describe("HttpError kind classification", () => { - it("classifies 401 and 403 as auth", () => { + it("classifies 401 as auth: the credential was not accepted", () => { expect(buildHttpError({ status: 401 }).kind).toBe("auth"); - expect(buildHttpError({ status: 403 }).kind).toBe("auth"); + }); + + it("classifies 403 as forbidden: the user was identified and refused", () => { + expect(buildHttpError({ status: 403 }).kind).toBe("forbidden"); + }); + + it("classifies 409 as conflict", () => { + expect(buildHttpError({ status: 409 }).kind).toBe("conflict"); }); it("classifies 429 as rate-limit", () => { diff --git a/packages/client/src/http/errors.ts b/packages/client/src/http/errors.ts index 096382f2..cd148e8f 100644 --- a/packages/client/src/http/errors.ts +++ b/packages/client/src/http/errors.ts @@ -10,6 +10,8 @@ export type HttpErrorKind = | "route-missing" | "resource-missing" | "auth" + | "forbidden" + | "conflict" | "rate-limit" | "server-error" | "generic"; @@ -19,7 +21,6 @@ interface StatusClassification { message?: string; } -const FORBIDDEN_STATUS = 403; const NOT_FOUND_STATUS = 404; const TEXT_CONTENT_TYPE = "text/plain"; @@ -28,8 +29,15 @@ const RESOURCE_MISSING_LITERAL = "Not found."; const STATUS_CLASSIFICATIONS: Record = { 401: { retryable: false }, - 403: { retryable: false }, + 403: { + retryable: false, + message: "Metabase refused the request: the API key's user is not allowed to do this.", + }, 404: { retryable: false }, + 409: { + retryable: false, + message: "Metabase refused the request: it conflicts with what already exists.", + }, 408: { retryable: true, message: "Metabase timed out responding." }, 425: { retryable: true }, 429: { retryable: true, message: "Metabase rate-limited the request." }, @@ -199,9 +207,17 @@ function classifyKind( sanitizedBody: string | null, redactedHeaders: Record, ): HttpErrorKind { - if (status === 401 || status === 403) { + // 401: Metabase did not accept the credential. 403: it identified the user and refused the + // request. 409: the request conflicts with existing state. None of the last two is about the key. + if (status === 401) { return "auth"; } + if (status === 403) { + return "forbidden"; + } + if (status === 409) { + return "conflict"; + } if (status === NOT_FOUND_STATUS) { return isRouteMissingResponse(sanitizedBody, redactedHeaders) ? "route-missing" @@ -257,26 +273,16 @@ function buildUserMessage( if (fromBody !== null) { return fromBody; } - const fromText = plainTextMessage(sanitizedBody, redactedHeaders); if (kind === "auth") { - return authMessage(input, fromText); + return `Invalid or unauthorized API key (host: ${hostFromUrl(input.url)}).`; } + const fromText = plainTextMessage(sanitizedBody, redactedHeaders); if (fromText !== null) { return fromText; } return defaultMessageForStatus(input.status); } -// The status decides: a 401 is a credential Metabase did not accept, a 403 a request it refused -// from a user it did identify, so a 403 never blames the key. A plain-text reason, when Metabase -// sends one, only makes the refusal specific. -function authMessage(input: HttpErrorInput, fromText: string | null): string { - if (input.status !== FORBIDDEN_STATUS) { - return `Invalid or unauthorized API key (host: ${hostFromUrl(input.url)}).`; - } - return fromText ?? "Metabase refused the request: the API key's user is not allowed to do this."; -} - // Metabase answers some rejections — a query that fails normalization, for one — with a text/plain // body that is nothing but the message. Only that content type is read as one: an HTML error page // from whatever sits in front of Metabase is never a message. diff --git a/packages/client/src/index.test.ts b/packages/client/src/index.test.ts index 608146fe..eabd5ab2 100644 --- a/packages/client/src/index.test.ts +++ b/packages/client/src/index.test.ts @@ -190,6 +190,12 @@ function describeKind(kind: HttpErrorKind): string { case "auth": { return "the credential was rejected"; } + case "forbidden": { + return "the user was identified and the request refused"; + } + case "conflict": { + return "the request conflicts with existing state"; + } case "rate-limit": { return "the caller is sending too fast"; } diff --git a/packages/client/src/resources/dashboard.ts b/packages/client/src/resources/dashboard.ts index 985ff77c..da53dc33 100644 --- a/packages/client/src/resources/dashboard.ts +++ b/packages/client/src/resources/dashboard.ts @@ -237,7 +237,7 @@ export function dashboardResource(transport: Transport) { if (error.kind === "resource-missing") { return { reason: "missing" }; } - if (error.kind === "auth") { + if (error.kind === "auth" || error.kind === "forbidden") { return { reason: "unreadable", detail: error.userMessage }; } throw error; From fa4facead5a088eb9c6279e1a5ff212b5188a6f1 Mon Sep 17 00:00:00 2001 From: dan sutton Date: Tue, 6 Oct 2026 17:04:20 -0500 Subject: [PATCH 4/5] fix(client): word the 403 and 409 defaults for any credential and any responder The 403 default named "the API key's user", which an OAuth login doesn't have, and both defaults said Metabase refused, though a proxy in front of it can answer too. They now say the request was refused, with the status. GHY-4738 Co-Authored-By: Claude Opus 5.5 --- packages/client/src/http/errors.test.ts | 4 ++-- packages/client/src/http/errors.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/client/src/http/errors.test.ts b/packages/client/src/http/errors.test.ts index 38dc6a0d..8f6ee70e 100644 --- a/packages/client/src/http/errors.test.ts +++ b/packages/client/src/http/errors.test.ts @@ -153,7 +153,7 @@ describe("HttpError message extraction", () => { it("calls a 403 with no body a refusal, not a bad key, since Metabase identified the user", () => { expect(buildHttpError({ status: 403, rawBody: null }).message).toBe( - "Metabase refused the request: the API key's user is not allowed to do this.", + "The request was refused (403): the signed-in user is not allowed to do this.", ); }); @@ -186,7 +186,7 @@ describe("HttpError message extraction", () => { it("calls a 409 with no body a conflict", () => { expect(buildHttpError({ status: 409, rawBody: null }).message).toBe( - "Metabase refused the request: it conflicts with what already exists.", + "The request was refused (409): it conflicts with what already exists.", ); }); diff --git a/packages/client/src/http/errors.ts b/packages/client/src/http/errors.ts index cd148e8f..c4589ac8 100644 --- a/packages/client/src/http/errors.ts +++ b/packages/client/src/http/errors.ts @@ -31,12 +31,12 @@ const STATUS_CLASSIFICATIONS: Record = { 401: { retryable: false }, 403: { retryable: false, - message: "Metabase refused the request: the API key's user is not allowed to do this.", + message: "The request was refused (403): the signed-in user is not allowed to do this.", }, 404: { retryable: false }, 409: { retryable: false, - message: "Metabase refused the request: it conflicts with what already exists.", + message: "The request was refused (409): it conflicts with what already exists.", }, 408: { retryable: true, message: "Metabase timed out responding." }, 425: { retryable: true }, From 5bfc4be1e715ad6a6e48d13c4304cc176587608f Mon Sep 17 00:00:00 2001 From: dan sutton Date: Tue, 6 Oct 2026 17:09:09 -0500 Subject: [PATCH 5/5] fix(cli): only a 401 marks a profile auth-failed verify read 401 and 403 from /api/user/current alike as "auth", so `mb auth list` said to update the token. Metabase serves every identified user their own record; a 403 there comes from whatever answered instead. verify now takes the client's own kind: `auth` (401) is a credential problem, anything else a server one. The stored failure kinds don't change. GHY-4738 Co-Authored-By: Claude Opus 5.5 --- packages/cli/src/core/auth/verify.test.ts | 32 +++++++++++++++++++++++ packages/cli/src/core/auth/verify.ts | 4 ++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/core/auth/verify.test.ts b/packages/cli/src/core/auth/verify.test.ts index ffd7186a..dd6cbbe1 100644 --- a/packages/cli/src/core/auth/verify.test.ts +++ b/packages/cli/src/core/auth/verify.test.ts @@ -93,6 +93,38 @@ describe("verifyAndProbe", () => { // A transport failure names no route, and a login that reports only "verification failed // (current user)" leaves a user unable to tell a blocked route from an unreachable host. + it("reads a 401 on the user as a credential problem", async () => { + // The version probe reaches fetch first. + const capture = captureFetch([ + jsonResponse(SESSION_PROPERTIES), + jsonResponse({ message: "Unauthenticated" }, 401), + ]); + vi.stubGlobal("fetch", capture.fetch); + + expect(await verifyAndProbe(BASE_URL, CREDENTIAL)).toMatchObject({ + ok: false, + which: "user", + kind: "auth", + status: 401, + }); + }); + + it("reads a 403 on the user as a server problem, since Metabase serves every identified user", async () => { + // The version probe reaches fetch first. + const capture = captureFetch([ + jsonResponse(SESSION_PROPERTIES), + jsonResponse({ message: "Forbidden" }, 403), + ]); + vi.stubGlobal("fetch", capture.fetch); + + expect(await verifyAndProbe(BASE_URL, CREDENTIAL)).toMatchObject({ + ok: false, + which: "user", + kind: "server", + status: 403, + }); + }); + it("names the request a transport failure never reached", async () => { const unreachable = [new TypeError("fetch failed"), new TypeError("fetch failed")]; const capture = captureFetch(unreachable); diff --git a/packages/cli/src/core/auth/verify.ts b/packages/cli/src/core/auth/verify.ts index acebf142..214d3fd9 100644 --- a/packages/cli/src/core/auth/verify.ts +++ b/packages/cli/src/core/auth/verify.ts @@ -73,7 +73,9 @@ export async function verifyAndProbe( function failure(error: unknown, which: VerifyWhich): VerifyFailure { if (error instanceof HttpError) { - const kind = error.status === 401 || error.status === 403 ? "auth" : "server"; + // Only a 401 is a credential Metabase did not take; a 403 here comes from whatever answered + // instead of Metabase, which serves every identified user their own record. + const kind = error.kind === "auth" ? "auth" : "server"; return { ok: false, which,