mirror of
https://github.com/andwati/zitadel.git
synced 2026-11-13 01:25:11 +00:00
fix(login): improve error classification (#11926)
Closes #11923 # Which Problems Are Solved Client-side gRPC errors (e.g. NotFound, InvalidArgument, PermissionDenied) were being surfaced as HTTP 500 Internal Server Errors, causing false SRE alerts and poor UI feedback. # How the Problems Are Solved - New transport interceptor (`error-classification.ts`): Automatically enriches every ConnectError with httpStatus and isUserError metadata via a `ClassifiedConnectError` wrapper - Route handler protection (`route.ts`, `flow-initiation.ts`): Catches classified errors from `getAuthRequest` / `getSAMLRequest` and returns correct HTTP status codes instead of 500 - Type safety: Replaced all magic number error.code === 9 checks with typed Code.FailedPrecondition + instanceof ConnectError across `oidc.ts`, `saml.ts`, `password.ts`, `zitadel.ts` - Classification-aware logging (`session.ts`): Client/user errors log at warn level, server errors at error level - Observability (`otel.ts`): Spans now include error.is_user_error and http.status_code attributes for alert filtering # Additional Changes Gitignore `next-env.d.ts`: This file is auto-generated by Next.js on every next dev and next build invocation. The two modes write slightly different import paths (.next/dev/types/ vs .next/types/), which causes git diff --exit-code to fail in CI whenever a developer runs next dev locally before committing. Since Next.js regenerates it automatically, there's no need to track it --------- Co-authored-by: Ramon <[email protected]> Co-authored-by: Copilot <[email protected]>
This commit is contained in:
co-authored by
Ramon
Copilot
parent
8e41d288ff
commit
ff4aae7b3f
@@ -0,0 +1,101 @@
|
||||
import { Code, ConnectError } from "@connectrpc/connect";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { ClassifiedConnectError, grpcCodeToHttpStatus, isClassifiedError } from "./error-classification";
|
||||
|
||||
describe("grpcCodeToHttpStatus", () => {
|
||||
const cases: [Code, number][] = [
|
||||
[Code.InvalidArgument, 400],
|
||||
[Code.FailedPrecondition, 400],
|
||||
[Code.OutOfRange, 400],
|
||||
[Code.Unauthenticated, 401],
|
||||
[Code.PermissionDenied, 403],
|
||||
[Code.NotFound, 404],
|
||||
[Code.AlreadyExists, 409],
|
||||
[Code.Aborted, 409],
|
||||
[Code.ResourceExhausted, 429],
|
||||
[Code.Canceled, 499],
|
||||
[Code.Unimplemented, 501],
|
||||
[Code.Unavailable, 503],
|
||||
[Code.DeadlineExceeded, 504],
|
||||
[Code.Internal, 500],
|
||||
[Code.DataLoss, 500],
|
||||
[Code.Unknown, 500],
|
||||
];
|
||||
|
||||
it.each(cases)("maps gRPC code %i to HTTP status %i", (grpcCode, expectedHttp) => {
|
||||
expect(grpcCodeToHttpStatus(grpcCode)).toBe(expectedHttp);
|
||||
});
|
||||
|
||||
it("defaults to 500 for unmapped codes", () => {
|
||||
expect(grpcCodeToHttpStatus(999 as Code)).toBe(500);
|
||||
});
|
||||
});
|
||||
|
||||
describe("ClassifiedConnectError", () => {
|
||||
it("preserves original error properties", () => {
|
||||
const source = new ConnectError("not found", Code.NotFound);
|
||||
const classified = new ClassifiedConnectError(source);
|
||||
|
||||
expect(classified.message).toContain("not found");
|
||||
expect(classified.code).toBe(Code.NotFound);
|
||||
expect(classified.name).toBe("ClassifiedConnectError");
|
||||
});
|
||||
|
||||
it("sets httpStatus from gRPC code", () => {
|
||||
const source = new ConnectError("permission denied", Code.PermissionDenied);
|
||||
const classified = new ClassifiedConnectError(source);
|
||||
|
||||
expect(classified.httpStatus).toBe(403);
|
||||
});
|
||||
|
||||
it("marks client errors correctly", () => {
|
||||
const clientError = new ClassifiedConnectError(new ConnectError("bad input", Code.InvalidArgument));
|
||||
const serverError = new ClassifiedConnectError(new ConnectError("internal", Code.Internal));
|
||||
|
||||
expect(clientError.isUserError).toBe(true);
|
||||
expect(serverError.isUserError).toBe(false);
|
||||
});
|
||||
|
||||
it("is detectable via isClassifiedError type guard", () => {
|
||||
const classified = new ClassifiedConnectError(new ConnectError("test", Code.NotFound));
|
||||
|
||||
expect(isClassifiedError(classified)).toBe(true);
|
||||
});
|
||||
|
||||
it("marks FailedPrecondition as client error", () => {
|
||||
const classified = new ClassifiedConnectError(new ConnectError("precondition", Code.FailedPrecondition));
|
||||
|
||||
expect(classified.isUserError).toBe(true);
|
||||
expect(classified.httpStatus).toBe(400);
|
||||
});
|
||||
|
||||
it("marks Unavailable as server error", () => {
|
||||
const classified = new ClassifiedConnectError(new ConnectError("unavailable", Code.Unavailable));
|
||||
|
||||
expect(classified.isUserError).toBe(false);
|
||||
expect(classified.httpStatus).toBe(503);
|
||||
});
|
||||
});
|
||||
|
||||
describe("isClassifiedError", () => {
|
||||
it("returns true for ClassifiedConnectError", () => {
|
||||
const classified = new ClassifiedConnectError(new ConnectError("test", Code.NotFound));
|
||||
expect(isClassifiedError(classified)).toBe(true);
|
||||
});
|
||||
|
||||
it("returns false for plain ConnectError", () => {
|
||||
const plain = new ConnectError("test", Code.NotFound);
|
||||
expect(isClassifiedError(plain)).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false for plain Error", () => {
|
||||
expect(isClassifiedError(new Error("test"))).toBe(false);
|
||||
});
|
||||
|
||||
it("returns false for non-error values", () => {
|
||||
expect(isClassifiedError(null)).toBe(false);
|
||||
expect(isClassifiedError(undefined)).toBe(false);
|
||||
expect(isClassifiedError("string")).toBe(false);
|
||||
expect(isClassifiedError({ code: 5 })).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,129 @@
|
||||
/**
|
||||
* Error Classification Interceptor
|
||||
*
|
||||
* Enriches ConnectError instances with HTTP status code equivalents and
|
||||
* client/server classification. This interceptor runs at the transport level,
|
||||
* so every gRPC/Connect call automatically gets classified errors so we can properly catch them in client code.
|
||||
*
|
||||
* Purpose:
|
||||
* - Prevent client-side gRPC errors (4xx equivalents) from being surfaced as HTTP 500s
|
||||
* - Provide correct HTTP status codes for route handler error responses
|
||||
*
|
||||
* @see https://cloud.google.com/apis/design/errors#handling_errors
|
||||
*/
|
||||
|
||||
import { Code, ConnectError, Interceptor } from "@connectrpc/connect";
|
||||
|
||||
/** Unique brand symbol for ClassifiedConnectError type guard detection */
|
||||
const CLASSIFIED_BRAND = Symbol.for("ClassifiedConnectError");
|
||||
|
||||
/** Canonical gRPC → HTTP status code mapping */
|
||||
const GRPC_TO_HTTP: Readonly<Record<number, number>> = {
|
||||
[Code.InvalidArgument]: 400,
|
||||
[Code.FailedPrecondition]: 400,
|
||||
[Code.OutOfRange]: 400,
|
||||
[Code.Unauthenticated]: 401,
|
||||
[Code.PermissionDenied]: 403,
|
||||
[Code.NotFound]: 404,
|
||||
[Code.AlreadyExists]: 409,
|
||||
[Code.Aborted]: 409,
|
||||
[Code.ResourceExhausted]: 429,
|
||||
[Code.Canceled]: 499,
|
||||
[Code.Unimplemented]: 501,
|
||||
[Code.Unavailable]: 503,
|
||||
[Code.DeadlineExceeded]: 504,
|
||||
[Code.DataLoss]: 500,
|
||||
[Code.Internal]: 500,
|
||||
[Code.Unknown]: 500,
|
||||
};
|
||||
|
||||
/** gRPC codes that represent user input errors (not genuine server failures) */
|
||||
const CLIENT_ERROR_CODES: ReadonlySet<Code> = new Set([
|
||||
Code.InvalidArgument,
|
||||
Code.FailedPrecondition,
|
||||
Code.OutOfRange,
|
||||
Code.Unauthenticated,
|
||||
Code.PermissionDenied,
|
||||
Code.NotFound,
|
||||
Code.AlreadyExists,
|
||||
Code.Aborted,
|
||||
Code.ResourceExhausted,
|
||||
Code.Canceled,
|
||||
]);
|
||||
|
||||
/**
|
||||
* A ConnectError enriched with HTTP status classification.
|
||||
*
|
||||
* All ConnectErrors thrown by service calls through the classified transport
|
||||
* will be instances of this class, allowing callers to inspect `httpStatus`
|
||||
* and `isUserError` without manual mapping.
|
||||
*/
|
||||
export class ClassifiedConnectError extends ConnectError {
|
||||
/** The equivalent HTTP status code for this gRPC error */
|
||||
readonly httpStatus: number;
|
||||
|
||||
/** Whether this error represents a user input error (true) or a server failure (false) */
|
||||
readonly isUserError: boolean;
|
||||
|
||||
/** @internal Brand symbol for type guard detection */
|
||||
readonly [CLASSIFIED_BRAND] = true as const;
|
||||
|
||||
constructor(source: ConnectError) {
|
||||
super(source.rawMessage, source.code, source.metadata, undefined, source.cause);
|
||||
// ConnectError's constructor resets the prototype chain via Object.setPrototypeOf.
|
||||
// We must restore it so instanceof ClassifiedConnectError works correctly.
|
||||
Object.setPrototypeOf(this, ClassifiedConnectError.prototype);
|
||||
this.name = "ClassifiedConnectError";
|
||||
// Preserve the original stack trace so debugging/alert triage can see the RPC call site.
|
||||
if (source.stack) {
|
||||
this.stack = source.stack;
|
||||
}
|
||||
this.httpStatus = GRPC_TO_HTTP[source.code] ?? 500;
|
||||
this.isUserError = CLIENT_ERROR_CODES.has(source.code);
|
||||
|
||||
// Copy details from the source error (avoids OutgoingDetail/IncomingDetail type mismatch)
|
||||
if (source.details.length > 0) {
|
||||
Object.defineProperty(this, "details", { value: source.details, writable: false });
|
||||
}
|
||||
|
||||
// Preserve the raw message from the original error
|
||||
if ("rawMessage" in source) {
|
||||
Object.defineProperty(this, "rawMessage", { value: source.rawMessage, writable: false });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Type guard for ClassifiedConnectError.
|
||||
* Use this in catch blocks to safely access httpStatus/isUserError.
|
||||
*/
|
||||
export function isClassifiedError(error: unknown): error is ClassifiedConnectError {
|
||||
return error !== null && typeof error === "object" && CLASSIFIED_BRAND in error;
|
||||
}
|
||||
|
||||
/**
|
||||
* Maps a gRPC Code to its HTTP status equivalent.
|
||||
* Useful when you have the code but not a full ClassifiedConnectError instance.
|
||||
*/
|
||||
export function grpcCodeToHttpStatus(code: Code): number {
|
||||
return GRPC_TO_HTTP[code] ?? 500;
|
||||
}
|
||||
|
||||
/**
|
||||
* Transport-level interceptor that catches ConnectError and re-throws
|
||||
* it as a ClassifiedConnectError with httpStatus and isUserError metadata.
|
||||
*
|
||||
* Plug this into the transport's interceptor chain to automatically classify
|
||||
* every error from every service call.
|
||||
*/
|
||||
export const errorClassificationInterceptor: Interceptor = (next) =>
|
||||
async function classifiedCall(req) {
|
||||
try {
|
||||
return await next(req);
|
||||
} catch (err) {
|
||||
if (err instanceof ConnectError) {
|
||||
throw new ClassifiedConnectError(err);
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
};
|
||||
@@ -7,6 +7,7 @@ import { createClientFor } from "@zitadel/client";
|
||||
import { SessionService } from "@zitadel/proto/zitadel/session/v2/session_service_pb";
|
||||
import { UserService } from "@zitadel/proto/zitadel/user/v2/user_service_pb";
|
||||
import { afterAll, beforeEach, describe, expect, it } from "vitest";
|
||||
import { errorClassificationInterceptor } from "./error-classification";
|
||||
import { otelGrpcInterceptor } from "./otel";
|
||||
|
||||
const exporter = new InMemorySpanExporter();
|
||||
@@ -77,7 +78,7 @@ describe("otelGrpcInterceptor", () => {
|
||||
},
|
||||
{
|
||||
transport: {
|
||||
interceptors: [otelGrpcInterceptor],
|
||||
interceptors: [otelGrpcInterceptor, errorClassificationInterceptor],
|
||||
},
|
||||
},
|
||||
);
|
||||
@@ -95,7 +96,11 @@ describe("otelGrpcInterceptor", () => {
|
||||
// code prefix gets added by connect
|
||||
message: "[not_found] Session not found",
|
||||
},
|
||||
attributes: { "rpc.grpc.status_code": 5 },
|
||||
attributes: {
|
||||
"rpc.grpc.status_code": 5,
|
||||
"error.is_user_error": true,
|
||||
"http.status_code": 404,
|
||||
},
|
||||
});
|
||||
expect(spans[0].events).toHaveLength(1);
|
||||
expect(spans[0].events[0].name).toBe("exception");
|
||||
|
||||
@@ -19,8 +19,9 @@
|
||||
* ```
|
||||
*/
|
||||
|
||||
import { ConnectError, Interceptor } from "@connectrpc/connect";
|
||||
import { Interceptor } from "@connectrpc/connect";
|
||||
import { context, propagation, SpanKind, SpanStatusCode, trace } from "@opentelemetry/api";
|
||||
import { isClassifiedError } from "./error-classification";
|
||||
|
||||
const TRACER_NAME = "zitadel-login-grpc" as const;
|
||||
|
||||
@@ -65,14 +66,17 @@ export const otelGrpcInterceptor: Interceptor = (next) =>
|
||||
|
||||
try {
|
||||
const response = await next(req);
|
||||
span.setAttribute("rpc.grpc.status_code", 0); // Code.OK
|
||||
span.setStatus({ code: SpanStatusCode.OK });
|
||||
return response;
|
||||
} catch (err) {
|
||||
const exception = err instanceof Error ? err : new Error(String(err));
|
||||
span.recordException(exception);
|
||||
|
||||
if (exception instanceof ConnectError) {
|
||||
if (isClassifiedError(exception)) {
|
||||
span.setAttribute("rpc.grpc.status_code", exception.code);
|
||||
span.setAttribute("error.is_user_error", exception.isUserError);
|
||||
span.setAttribute("http.status_code", exception.httpStatus);
|
||||
}
|
||||
span.setStatus({
|
||||
code: SpanStatusCode.ERROR,
|
||||
|
||||
Reference in New Issue
Block a user