From 7c4b484d0a8497c0b5e6e177239148bbb7d65acd Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Wed, 5 Aug 2026 19:46:59 +0530 Subject: [PATCH] fix(rpc): close prototype-chain permission bypass, add server/client/transport tests C1 CRITICAL: implement() looked up procedures/handlers with plain property indexing, so any Object.prototype member name (constructor, toString, etc.) resolved truthy and skipped the permission gate entirely. Fixed with Object.hasOwn checks in packages/rpc/src/server.ts. Defense-in-depth guard added in packages/dev-server/src/rpc-dispatch.ts constraining URL path segments to a safe charset before they reach service/procedure lookups. Added missing direct test coverage for packages/rpc/src/transport.ts, server.ts and client.ts (previously untested), including a prototype-name sweep in both server.test.ts and dev-server's rpc-endpoint.test.ts. Co-Authored-By: Claude Opus 5 --- packages/dev-server/src/rpc-dispatch.ts | 7 +- packages/dev-server/test/rpc-endpoint.test.ts | 25 ++ packages/rpc/src/server.ts | 10 +- packages/rpc/test/client.test.ts | 110 ++++++++ packages/rpc/test/server.test.ts | 243 ++++++++++++++++++ packages/rpc/test/transport.test.ts | 77 ++++++ 6 files changed, 468 insertions(+), 4 deletions(-) create mode 100644 packages/rpc/test/client.test.ts create mode 100644 packages/rpc/test/server.test.ts create mode 100644 packages/rpc/test/transport.test.ts diff --git a/packages/dev-server/src/rpc-dispatch.ts b/packages/dev-server/src/rpc-dispatch.ts index bdb63f01..92f90ae7 100644 --- a/packages/dev-server/src/rpc-dispatch.ts +++ b/packages/dev-server/src/rpc-dispatch.ts @@ -27,9 +27,12 @@ export async function handleRpcRequest( if (!isInternalCaller(req)) return new Response("Not found", { status: 404 }); if (req.method !== "POST") return new Response("Method not allowed", { status: 405 }); const segments = url.pathname.split("/"); - const service = segments[3] ? services.get(segments[3]) : undefined; + const SAFE_SEGMENT = /^[A-Za-z0-9_-]+$/; + const serviceName = segments[3]; const procedure = segments[4]; - if (!service || !procedure || segments.length !== 5) { + const service = + serviceName && SAFE_SEGMENT.test(serviceName) ? services.get(serviceName) : undefined; + if (!service || !procedure || !SAFE_SEGMENT.test(procedure) || segments.length !== 5) { return json({ ok: false, code: "RPC_UNKNOWN", message: "Unknown procedure", retryable: false }); } let payload: unknown; diff --git a/packages/dev-server/test/rpc-endpoint.test.ts b/packages/dev-server/test/rpc-endpoint.test.ts index 272ee439..aee50bce 100644 --- a/packages/dev-server/test/rpc-endpoint.test.ts +++ b/packages/dev-server/test/rpc-endpoint.test.ts @@ -48,4 +48,29 @@ describe("RPC endpoint", () => { expect(isInternalCaller(forwarded)).toBe(false); expect((await handleRpcRequest(forwarded, new URL(forwarded.url), services))!.status).toBe(404); }); + + describe("C1: prototype-chain procedure names cannot bypass the permission gate", () => { + const PROTO_NAMES = [ + "constructor", + "toString", + "valueOf", + "hasOwnProperty", + "__proto__", + "isPrototypeOf", + ]; + + for (const name of PROTO_NAMES) { + test(`"${name}" in the URL path yields RPC_UNKNOWN`, async () => { + const req = request(`/__wrnexus/rpc/demo/${name}`, { "x-wrnexus-internal": "1" }); + const res = await handleRpcRequest(req, new URL(req.url), services); + expect(await res!.json()).toMatchObject({ ok: false, code: "RPC_UNKNOWN" }); + }); + } + + test(`"constructor" as the SERVICE segment also yields RPC_UNKNOWN`, async () => { + const req = request("/__wrnexus/rpc/constructor/add", { "x-wrnexus-internal": "1" }); + const res = await handleRpcRequest(req, new URL(req.url), services); + expect(await res!.json()).toMatchObject({ ok: false, code: "RPC_UNKNOWN" }); + }); + }); }); diff --git a/packages/rpc/src/server.ts b/packages/rpc/src/server.ts index e806537e..adda1c4b 100644 --- a/packages/rpc/src/server.ts +++ b/packages/rpc/src/server.ts @@ -37,8 +37,14 @@ export function implement( return { contract, async invoke(procedureName, payload, identity) { - const definition = contract.procedures[procedureName as keyof Procedures]; - const handler = handlers[procedureName as keyof Procedures]; + // Object.hasOwn, not plain indexing: "constructor", "toString" and every + // other Object.prototype member otherwise resolve as truthy, and a + // prototype member carries no `permission`, so the gate below is skipped + // entirely and an unintended function runs with attacker-controlled input. + const known = + Object.hasOwn(contract.procedures, procedureName) && Object.hasOwn(handlers, procedureName); + const definition = known ? contract.procedures[procedureName as keyof Procedures] : undefined; + const handler = known ? handlers[procedureName as keyof Procedures] : undefined; if (!definition || !handler) return failure(RPC_ERROR_CODES.unknown, "Unknown procedure"); let subject: SubjectContext | undefined; diff --git a/packages/rpc/test/client.test.ts b/packages/rpc/test/client.test.ts new file mode 100644 index 00000000..76136e98 --- /dev/null +++ b/packages/rpc/test/client.test.ts @@ -0,0 +1,110 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import type { Context } from "@wrnexus/core"; +import { v } from "@wrnexus/validation"; +import { serviceClient } from "../src/client.ts"; +import { defineService, procedure } from "../src/contract.ts"; +import { RPC_ERROR_CODES, ServiceError, failure, success } from "../src/errors.ts"; +import type { RpcTarget } from "../src/transport.ts"; + +const original = { ...process.env }; +afterEach(() => { + process.env = { ...original }; +}); + +function configure(appName = "web") { + process.env.WRNEXUS_RPC_SECRET = "test-rpc-secret-at-least-32-chars-long"; + process.env.WRNEXUS_APP_NAME = appName; +} + +const billing = defineService({ + name: "billing", + procedures: { + createInvoice: procedure + .input(v.object({ amountCents: v.number() })) + .output<{ invoiceId: string }>() + .build(), + }, +}); + +describe("serviceClient", () => { + test("returns the handler's value unwrapped", async () => { + const client = serviceClient(billing, { + transport: { call: async () => success({ invoiceId: "inv_1" }) }, + }); + expect(await client.createInvoice({ amountCents: 1 })).toEqual({ invoiceId: "inv_1" }); + }); + + test("throws a ServiceError carrying code and retryable on failure", async () => { + const client = serviceClient(billing, { + transport: { call: async () => failure(RPC_ERROR_CODES.transport, "down") }, + }); + let caught: unknown; + try { + await client.createInvoice({ amountCents: 1 }); + } catch (err) { + caught = err; + } + expect(caught).toBeInstanceOf(ServiceError); + expect(caught).toMatchObject({ code: RPC_ERROR_CODES.transport, retryable: true }); + }); + + test("attaches an identity token when given a context and omits it for an anonymous context", async () => { + configure("web"); + let seenIdentity: string | undefined = "unset"; + const client = serviceClient(billing, { + as: { user: { id: "u1" }, locals: {} } as unknown as Context, + transport: { + call: async (_target, _payload, options) => { + seenIdentity = options.identity; + return success({ invoiceId: "inv_1" }); + }, + }, + }); + await client.createInvoice({ amountCents: 1 }); + expect(seenIdentity).toBeTypeOf("string"); + + let seenAnon: unknown = "unset"; + const anonClient = serviceClient(billing, { + transport: { + call: async (_target, _payload, options) => { + seenAnon = options.identity; + return success({ invoiceId: "inv_1" }); + }, + }, + }); + await anonClient.createInvoice({ amountCents: 1 }); + expect(seenAnon).toBeUndefined(); + }); + + test("calling an undeclared procedure throws rather than issuing a call", async () => { + let called = false; + const client = serviceClient(billing, { + transport: { + call: async () => { + called = true; + return success({}); + }, + }, + }); + const proxy = client as unknown as Record Promise>; + await expect(proxy.deleteEverything!({})).rejects.toMatchObject({ + code: RPC_ERROR_CODES.unknown, + }); + expect(called).toBe(false); + }); + + test("the app defaults to the service name", async () => { + let seenTarget: RpcTarget | undefined; + const client = serviceClient(billing, { + transport: { + call: async (target) => { + seenTarget = target; + return success({ invoiceId: "inv_1" }); + }, + }, + }); + await client.createInvoice({ amountCents: 1 }); + expect(seenTarget?.app).toBe("billing"); + expect(seenTarget?.service).toBe("billing"); + }); +}); diff --git a/packages/rpc/test/server.test.ts b/packages/rpc/test/server.test.ts new file mode 100644 index 00000000..dd23c67a --- /dev/null +++ b/packages/rpc/test/server.test.ts @@ -0,0 +1,243 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { v } from "@wrnexus/validation"; +import { defineService, procedure } from "../src/contract.ts"; +import { RPC_ERROR_CODES } from "../src/errors.ts"; +import { exportSubjectContext } from "../src/identity.ts"; +import { implement } from "../src/server.ts"; + +const original = { ...process.env }; +afterEach(() => { + process.env = { ...original }; +}); + +function configure(appName = "web") { + process.env.WRNEXUS_RPC_SECRET = "test-rpc-secret-at-least-32-chars-long"; + process.env.WRNEXUS_APP_NAME = appName; +} + +const openContract = defineService({ + name: "demo", + procedures: { + add: procedure + .input(v.object({ a: v.number() })) + .output<{ a: number }>() + .build(), + }, +}); + +const guardedContract = defineService({ + name: "billing", + procedures: { + createInvoice: procedure + .input(v.object({ amountCents: v.number() })) + .output<{ invoiceId: string }>() + .permission("invoice:create") + .build(), + }, +}); + +describe("implement()", () => { + test("invokes with validated input", async () => { + const service = implement( + openContract, + { add: async ({ a }) => ({ a: a + 1 }) }, + { selfApp: "demo" }, + ); + const result = await service.invoke("add", { a: 1 }); + expect(result).toEqual({ ok: true, value: { a: 2 } }); + }); + + test("coerces through the schema", async () => { + let received: unknown; + const coercing = defineService({ + name: "demo2", + procedures: { + add: procedure + .input(v.object({ a: v.number() })) + .output<{ a: number }>() + .build(), + }, + }); + const service = implement( + coercing, + { + add: async (input) => { + received = input; + return { a: (input as { a: number }).a }; + }, + }, + { selfApp: "demo2" }, + ); + await service.invoke("add", { a: "3" }); + expect(received).toEqual({ a: 3 }); + }); + + test("rejects schema-invalid input without invoking the handler", async () => { + let called = false; + const service = implement( + openContract, + { + add: async ({ a }) => { + called = true; + return { a }; + }, + }, + { selfApp: "demo" }, + ); + const result = await service.invoke("add", { a: "not-a-number" }); + expect(called).toBe(false); + expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.invalid }); + }); + + test("unknown procedure refused", async () => { + const service = implement(openContract, { add: async ({ a }) => ({ a }) }, { selfApp: "demo" }); + const result = await service.invoke("subtract", {}); + expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.unknown }); + }); + + test("handler throw becomes opaque; a secret in the message does not survive", async () => { + const secret = "sk-super-secret-db-password"; + const service = implement( + openContract, + { + add: async () => { + throw new Error(`db error using ${secret}`); + }, + }, + { selfApp: "demo" }, + ); + const result = await service.invoke("add", { a: 1 }); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.code).toBe(RPC_ERROR_CODES.handler); + expect(result.message).not.toContain(secret); + } + }); + + test("a declared permission is enforced before the handler runs", async () => { + let called = false; + const service = implement( + guardedContract, + { + createInvoice: async ({ amountCents }) => { + called = true; + return { invoiceId: `inv_${amountCents}` }; + }, + }, + { selfApp: "billing", checkPermission: async () => false }, + ); + const result = await service.invoke("createInvoice", { amountCents: 5 }); + expect(called).toBe(false); + expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.denied }); + }); + + test("a permission check that throws denies", async () => { + let called = false; + const service = implement( + guardedContract, + { + createInvoice: async ({ amountCents }) => { + called = true; + return { invoiceId: `inv_${amountCents}` }; + }, + }, + { + selfApp: "billing", + checkPermission: async () => { + throw new Error("permission store unavailable"); + }, + }, + ); + const result = await service.invoke("createInvoice", { amountCents: 5 }); + expect(called).toBe(false); + expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.denied }); + }); + + test("a declared permission with no checkPermission configured denies (fail closed)", async () => { + let called = false; + const service = implement( + guardedContract, + { + createInvoice: async ({ amountCents }) => { + called = true; + return { invoiceId: `inv_${amountCents}` }; + }, + }, + { selfApp: "billing" }, + ); + const result = await service.invoke("createInvoice", { amountCents: 5 }); + expect(called).toBe(false); + expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.denied }); + }); + + test("a valid identity token reaches the handler as a subject", async () => { + configure("web"); + const token = await exportSubjectContext( + { user: { id: "u1" }, locals: {} } as never, + "billing", + ); + const service = implement( + guardedContract, + { + createInvoice: async ({ amountCents }, ctx) => ({ + invoiceId: `${ctx.subject?.subjectId}_${amountCents}`, + }), + }, + { selfApp: "billing", checkPermission: async () => true }, + ); + const result = await service.invoke("createInvoice", { amountCents: 5 }, token); + expect(result).toEqual({ ok: true, value: { invoiceId: "u1_5" } }); + }); + + test("a bad identity token is refused rather than downgraded to anonymous", async () => { + configure("web"); + let called = false; + const service = implement( + openContract, + { + add: async ({ a }, ctx) => { + called = true; + expect(ctx.subject).toBeUndefined(); + return { a }; + }, + }, + { selfApp: "demo" }, + ); + const result = await service.invoke("add", { a: 1 }, "garbage-token"); + expect(called).toBe(false); + expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.identity }); + }); + + describe("C1: prototype-chain procedure names cannot bypass the permission gate", () => { + const PROTO_NAMES = [ + "constructor", + "toString", + "valueOf", + "hasOwnProperty", + "__proto__", + "isPrototypeOf", + ]; + + for (const name of PROTO_NAMES) { + test(`"${name}" resolves as unknown, not as a handler`, async () => { + let permissionChecked = false; + const service = implement( + guardedContract, + { + createInvoice: async ({ amountCents }) => ({ invoiceId: `inv_${amountCents}` }), + }, + { + selfApp: "billing", + checkPermission: async () => { + permissionChecked = true; + return false; + }, + }, + ); + const result = await service.invoke(name, { amountCents: 1 }); + expect(result).toMatchObject({ ok: false, code: RPC_ERROR_CODES.unknown }); + expect(permissionChecked).toBe(false); + }); + } + }); +}); diff --git a/packages/rpc/test/transport.test.ts b/packages/rpc/test/transport.test.ts new file mode 100644 index 00000000..43ad6ed6 --- /dev/null +++ b/packages/rpc/test/transport.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, test } from "bun:test"; +import { RPC_ERROR_CODES, success } from "../src/errors.ts"; +import { inProcessTransport } from "../src/transport.ts"; + +describe("inProcessTransport", () => { + test("routes to the right handler and passes identity through", async () => { + let seenIdentity: string | undefined; + const transport = inProcessTransport({ + "billing/createInvoice": (payload, identity) => { + seenIdentity = identity; + return success({ echoed: payload }); + }, + }); + const result = await transport.call( + { app: "billing", service: "billing", procedure: "createInvoice" }, + { amountCents: 1 }, + { identity: "tok-123" }, + ); + expect(result).toEqual({ ok: true, value: { echoed: { amountCents: 1 } } }); + expect(seenIdentity).toBe("tok-123"); + }); + + test("an unregistered procedure yields non-retryable RPC_UNKNOWN", async () => { + const transport = inProcessTransport({}); + const result = await transport.call( + { app: "billing", service: "billing", procedure: "nope" }, + {}, + {}, + ); + expect(result).toEqual({ + ok: false, + code: RPC_ERROR_CODES.unknown, + message: "Unknown procedure", + retryable: false, + }); + }); + + test("an already-aborted signal fails without invoking the handler", async () => { + let called = false; + const transport = inProcessTransport({ + "billing/createInvoice": () => { + called = true; + return success({}); + }, + }); + const controller = new AbortController(); + controller.abort(); + const result = await transport.call( + { app: "billing", service: "billing", procedure: "createInvoice" }, + {}, + { signal: controller.signal }, + ); + expect(called).toBe(false); + expect(result.ok).toBe(false); + expect(result).toMatchObject({ code: RPC_ERROR_CODES.transport }); + }); + + test("a handler that throws becomes an opaque failure with no leaked message text", async () => { + const secret = "sk-super-secret-database-password-xyz"; + const transport = inProcessTransport({ + "billing/createInvoice": () => { + throw new Error(`connection failed with credential ${secret}`); + }, + }); + const result = await transport.call( + { app: "billing", service: "billing", procedure: "createInvoice" }, + {}, + {}, + ); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.code).toBe(RPC_ERROR_CODES.handler); + expect(result.message).not.toContain(secret); + expect(result.message).not.toContain("connection failed"); + } + }); +});