From dab70b2cd47c39d6c42365683f9fbf26fa2c8b0b Mon Sep 17 00:00:00 2001 From: Christian Klotz Date: Wed, 5 Aug 2026 11:06:39 +0300 Subject: [PATCH] fix(server): sanitize service failures (#7644) --- packages/protocol/src/schemas.ts | 2 + packages/protocol/test/protocol.test.ts | 10 +++++ packages/server/src/errors.ts | 20 +++++++++- packages/server/src/server.ts | 16 +++++++- packages/server/test/conformance.test.ts | 48 +++++++++++++++++++++++- 5 files changed, 91 insertions(+), 5 deletions(-) diff --git a/packages/protocol/src/schemas.ts b/packages/protocol/src/schemas.ts index afd61e91b..1e510806c 100644 --- a/packages/protocol/src/schemas.ts +++ b/packages/protocol/src/schemas.ts @@ -269,6 +269,8 @@ export const ProtocolErrorCodeSchema = Type.Union([ Type.Literal("session_locked"), Type.Literal("not_found"), Type.Literal("invalid_request"), + Type.Literal("not_implemented"), + Type.Literal("internal_error"), ]); export const ProtocolErrorSchema = StrictObject({ code: ProtocolErrorCodeSchema, diff --git a/packages/protocol/test/protocol.test.ts b/packages/protocol/test/protocol.test.ts index 30e328ed3..359523a9b 100644 --- a/packages/protocol/test/protocol.test.ts +++ b/packages/protocol/test/protocol.test.ts @@ -100,6 +100,16 @@ describe("protocol validation", () => { expect(parseServerMessage(serverHello)).toEqual(serverHello); }); + test.each(["not_implemented", "internal_error"] as const)("accepts the %s error code", (code) => { + const message: ServerMessage = { + type: "response", + id: "request-1", + ok: false, + error: { code, message: "safe" }, + }; + expect(parseServerMessage(message)).toEqual(message); + }); + test.each([ { type: "hello", diff --git a/packages/server/src/errors.ts b/packages/server/src/errors.ts index 717f5edd8..0822c56ca 100644 --- a/packages/server/src/errors.ts +++ b/packages/server/src/errors.ts @@ -2,9 +2,12 @@ import type { JsonValue, ProtocolErrorCode } from "@earendil-works/pi-protocol"; export type PiServerOperationErrorCode = Extract< ProtocolErrorCode, - "busy" | "session_locked" | "not_found" | "invalid_request" + "busy" | "session_locked" | "not_found" | "invalid_request" | "not_implemented" >; +export const INTERNAL_SERVER_ERROR_MESSAGE = "Internal server error"; +export const NOT_IMPLEMENTED_MESSAGE = "Operation is not implemented"; + /** A service/runtime error that can safely cross the protocol boundary. */ export class PiServerError extends Error { readonly code: PiServerOperationErrorCode; @@ -38,3 +41,18 @@ export class SessionNotFoundError extends PiServerError { this.name = "SessionNotFoundError"; } } + +export class NotImplementedError extends PiServerError { + constructor() { + super("not_implemented", NOT_IMPLEMENTED_MESSAGE); + this.name = "NotImplementedError"; + } +} + +/** An unsafe failure whose cause is retained for reporting but never serialized. */ +export class InternalServerError extends Error { + constructor(cause: unknown) { + super(INTERNAL_SERVER_ERROR_MESSAGE, { cause }); + this.name = "InternalServerError"; + } +} diff --git a/packages/server/src/server.ts b/packages/server/src/server.ts index 9c0532062..f03ac4640 100644 --- a/packages/server/src/server.ts +++ b/packages/server/src/server.ts @@ -21,7 +21,12 @@ import { type ConnectionState, isTerminalConnection, } from "./connection.ts"; -import { PiServerError } from "./errors.ts"; +import { + INTERNAL_SERVER_ERROR_MESSAGE, + InternalServerError, + NOT_IMPLEMENTED_MESSAGE, + PiServerError, +} from "./errors.ts"; import type { PiServerListener } from "./listener.ts"; import { LiveSessionManager } from "./sessions.ts"; import { ServerSnapshotPublisher } from "./snapshots.ts"; @@ -344,7 +349,14 @@ export class PiServer { } private toProtocolError(error: unknown): ProtocolError { + if (error instanceof InternalServerError) { + this.reportError(error.cause); + return { code: "internal_error", message: INTERNAL_SERVER_ERROR_MESSAGE }; + } if (error instanceof PiServerError) { + if (error.code === "not_implemented") { + return { code: "not_implemented", message: NOT_IMPLEMENTED_MESSAGE }; + } return error.details === undefined ? { code: error.code, message: error.message } : { code: error.code, message: error.message, details: error.details }; @@ -353,7 +365,7 @@ export class PiServer { return { code: "invalid_request", message: error.message }; } this.reportError(error); - return { code: "invalid_request", message: "Internal server error" }; + return { code: "internal_error", message: INTERNAL_SERVER_ERROR_MESSAGE }; } private reportError(error: unknown): void { diff --git a/packages/server/test/conformance.test.ts b/packages/server/test/conformance.test.ts index d9e1d006f..17df33363 100644 --- a/packages/server/test/conformance.test.ts +++ b/packages/server/test/conformance.test.ts @@ -3,7 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { encodeClientMessage, encodeFrame, PROTOCOL_VERSION } from "@earendil-works/pi-protocol"; import { afterEach, describe, expect, test } from "vitest"; -import { type PiServer, PiServerError } from "../src/index.ts"; +import { InternalServerError, NotImplementedError, type PiServer, PiServerError } from "../src/index.ts"; import { connectUnixTestClient, Deferred, type ProtocolTestClient, TestServerService } from "../src/testing/index.ts"; import { createUnixServer, type UnixServerOptions } from "../src/transports/unix/index.ts"; @@ -267,11 +267,55 @@ describe("Unix transport conformance", () => { await client.hello(); expect(await client.request({ command: "list" })).toMatchObject({ ok: false, - error: { code: "invalid_request", message: "Internal server error" }, + error: { code: "internal_error", message: "Internal server error" }, }); expect(errors).toContainEqual(expect.objectContaining({ message: "private service detail" })); }); + test("keeps not_implemented stable", async () => { + class IncompleteService extends TestServerService { + private listCount = 0; + + override async listSessions() { + this.listCount += 1; + if (this.listCount > 1) throw new NotImplementedError(); + return super.listSessions(); + } + } + const { server } = await startServer(new IncompleteService()); + const client = await connect(server); + await client.hello(); + expect(await client.request({ command: "list" })).toMatchObject({ + ok: false, + error: { code: "not_implemented", message: "Operation is not implemented" }, + }); + }); + + test("reports wrapped internal causes without exposing them", async () => { + const cause = new Error("private storage detail", { cause: new Error("private root cause") }); + class WrappedFailureService extends TestServerService { + private listCount = 0; + + override async listSessions() { + this.listCount += 1; + if (this.listCount > 1) throw new InternalServerError(cause); + return super.listSessions(); + } + } + const errors: Error[] = []; + const { server } = await startServer(new WrappedFailureService(), { onError: (error) => errors.push(error) }); + const client = await connect(server); + await client.hello(); + const response = await client.request({ command: "list" }); + expect(response).toMatchObject({ + ok: false, + error: { code: "internal_error", message: "Internal server error" }, + }); + expect(JSON.stringify(response)).not.toContain("private"); + expect(errors).toContain(cause); + expect(errors).not.toContainEqual(expect.objectContaining({ name: "InternalServerError" })); + }); + test("can respond out of request order after the handshake", async () => { const service = new TestServerService(); service.seed("first");