From e8c472c14c38ea163ad2d3c4506183d2dc710fdc Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 17 Apr 2026 14:24:28 +0200 Subject: [PATCH 01/14] feat(appkit): add Standard Schema body validation to Plugin.route() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends RouteConfig with optional `body` (Standard Schema v1) and `exposeValidationErrors` fields. Plugin.route() wraps handlers with a validation closure when a schema is present; zero-overhead pass-through otherwise. On validation failure, emits a canonical 400 body { error, code: "VALIDATION_ERROR", requestId, issues? } — `issues` is included in development or when the route opts in; production omits it by default. A per-request correlation ID is read from x-request-id or generated, and full issues are logged server-side. Migrates genie plugin's POST /:alias/messages as the canary: a Zod schema replaces the manual truthy check on `content`, and the handler receives a typed `req.body` with no casts. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- docs/docs/api/appkit/Class.Plugin.md | 10 +- docs/docs/plugins/custom-plugins.md | 67 ++++ packages/appkit/package.json | 7 +- packages/appkit/src/plugin/plugin.ts | 114 +++++- .../src/plugin/tests/body-validation.test.ts | 372 ++++++++++++++++++ packages/appkit/src/plugins/genie/genie.ts | 35 +- .../src/plugins/genie/tests/genie.test.ts | 46 ++- packages/appkit/src/plugins/genie/types.ts | 5 - packages/shared/package.json | 1 + packages/shared/src/plugin.ts | 52 ++- pnpm-lock.yaml | 9 + 11 files changed, 688 insertions(+), 30 deletions(-) create mode 100644 packages/appkit/src/plugin/tests/body-validation.test.ts diff --git a/docs/docs/api/appkit/Class.Plugin.md b/docs/docs/api/appkit/Class.Plugin.md index 06e558dcf..cb5eda630 100644 --- a/docs/docs/api/appkit/Class.Plugin.md +++ b/docs/docs/api/appkit/Class.Plugin.md @@ -528,21 +528,21 @@ AuthenticationError in production when no user header is present. ### route() ```ts -protected route<_TResponse>(router: Router, config: RouteConfig): void; +protected route(router: Router, config: RouteConfig): void; ``` #### Type Parameters -| Type Parameter | -| ------ | -| `_TResponse` | +| Type Parameter | Default type | +| ------ | ------ | +| `TBody` | `unknown` | #### Parameters | Parameter | Type | | ------ | ------ | | `router` | `Router` | -| `config` | `RouteConfig` | +| `config` | `RouteConfig`\<`TBody`\> | #### Returns diff --git a/docs/docs/plugins/custom-plugins.md b/docs/docs/plugins/custom-plugins.md index 7b7cf5684..6cfe6eb54 100644 --- a/docs/docs/plugins/custom-plugins.md +++ b/docs/docs/plugins/custom-plugins.md @@ -123,6 +123,73 @@ This pattern allows: - **Static tools** (CLI, docs) to show all possible resources - **Runtime validation** to enforce resources based on actual configuration +## Validating request bodies + +When registering a route via `this.route()`, provide a `body` schema to validate `req.body` before the handler runs. The framework accepts any validator that implements the [Standard Schema](https://standardschema.dev) v1 contract — Zod 3.24+, Valibot, ArkType, and others all work out of the box. Your plugin picks its own validator. + +```typescript +import { Plugin, toPlugin, type PluginManifest } from "@databricks/appkit"; +import { z } from "zod"; + +const sendMessageBody = z.object({ + content: z.string().min(1), + conversationId: z.string().optional(), +}); + +type SendMessageBody = z.infer; + +class MyPlugin extends Plugin { + injectRoutes(router) { + this.route(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: sendMessageBody, + handler: async (req, res) => { + // req.body.content is typed as string. + // req.body.conversationId is typed as string | undefined. + res.json({ ok: true, content: req.body.content }); + }, + }); + } +} +``` + +### Behavior + +- When `body` is absent, the route is a zero-overhead pass-through — your handler runs exactly as if no validation existed. +- When `body` is present, the framework calls `schema["~standard"].validate(req.body)` before invoking the handler. If validation succeeds, `req.body` is replaced with the validated value and narrowed to the schema's output type. If validation fails, the handler is not called and the framework emits a canonical 400 response. +- Validation narrows types; if you need to transform the body (e.g. coerce strings to dates), do it in the handler — transformations performed by the schema will be preserved but are not part of the v1 public contract. + +### Canonical 400 error body + +On validation failure the framework responds with: + +```json +{ + "error": "Invalid request body", + "code": "VALIDATION_ERROR", + "requestId": "req_a3f9c18d", + "issues": [ + { "path": ["content"], "message": "String must contain at least 1 character(s)" } + ] +} +``` + +- `requestId` is taken from the `x-request-id` header when present, otherwise a short random token is generated. The full `issues` array is always logged server-side keyed by `requestId`, so operators can correlate client-visible 400s with detailed logs. +- `issues` is included when `NODE_ENV !== "production"`. In production the `issues` field is omitted by default to avoid leaking schema internals. Set `exposeValidationErrors: true` on the route config to opt into including `issues` in production responses. + +```typescript +this.route(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: sendMessageBody, + exposeValidationErrors: true, // include issues in prod responses + handler: async (req, res) => { /* ... */ }, +}); +``` + ## Key extension points - **Route injection**: Implement `injectRoutes()` to add custom endpoints using [`IAppRouter`](../api/appkit/TypeAlias.IAppRouter.md) diff --git a/packages/appkit/package.json b/packages/appkit/package.json index c658a9e3d..f8ecf7cbc 100644 --- a/packages/appkit/package.json +++ b/packages/appkit/package.json @@ -77,9 +77,14 @@ "semver": "7.7.3", "shared": "workspace:*", "vite": "npm:rolldown-vite@7.1.14", - "ws": "8.18.3" + "ws": "8.18.3", + "zod": "4.1.13" + }, + "peerDependencies": { + "@standard-schema/spec": "^1.0.0" }, "devDependencies": { + "@standard-schema/spec": "1.1.0", "@types/express": "4.17.25", "@types/json-schema": "7.0.15", "@types/pg": "8.16.0", diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 5173cb612..009d5aced 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -1,7 +1,10 @@ +import { randomBytes } from "node:crypto"; +import type { StandardSchemaV1 } from "@standard-schema/spec"; import type express from "express"; import type { BasePlugin, BasePluginConfig, + IAppRequestWithBody, IAppResponse, PluginEndpointMap, PluginExecuteConfig, @@ -55,6 +58,20 @@ function hasHttpStatusCode( ); } +/** + * Resolve a request ID from the `x-request-id` header (if present), falling + * back to a short random token. Used by the body validation wrapper so + * operators can correlate a client-facing 400 with the full server-side + * issue log. + */ +function resolveRequestId(req: express.Request): string { + const headerId = req.header("x-request-id"); + if (headerId && typeof headerId === "string" && headerId.length > 0) { + return headerId; + } + return `req_${randomBytes(4).toString("hex")}`; +} + /** * Methods that should not be proxied by asUser(). * These are lifecycle/internal methods that don't make sense @@ -515,13 +532,25 @@ export abstract class Plugin< this.registeredEndpoints[name] = path; } - protected route<_TResponse>( + protected route( router: express.Router, - config: RouteConfig, + config: RouteConfig, ): void { const { name, method, path, handler } = config; - router[method](path, handler); + // Zero-overhead pass-through when no body schema is provided. + const effectiveHandler = config.body + ? this._wrapHandlerWithBodyValidation( + handler, + config.body, + config.exposeValidationErrors === true, + ) + : (handler as ( + req: express.Request, + res: express.Response, + ) => Promise); + + router[method](path, effectiveHandler); const fullPath = `/api/${this.name}${path}`; this.registerEndpoint(name, fullPath); @@ -531,6 +560,85 @@ export abstract class Plugin< } } + /** + * Wrap a route handler in a pre-validation closure. When the wrapped + * handler runs, the request body is validated against the provided + * Standard Schema before the original handler is invoked. + * + * On validation failure the wrapper emits a canonical 400 response and + * does not call the original handler. On success the request body is + * reassigned to the validated value (preserving any narrowing/coercion) + * and the original handler runs as before. + */ + private _wrapHandlerWithBodyValidation( + handler: RouteConfig["handler"], + schema: StandardSchemaV1, + exposeValidationErrors: boolean, + ): (req: express.Request, res: express.Response) => Promise { + return async (req, res) => { + let result = schema["~standard"].validate(req.body); + if (result instanceof Promise) { + result = await result; + } + + if (result.issues) { + const requestId = resolveRequestId(req); + + // Normalize Standard Schema path segments: spec allows either a + // PropertyKey or an object with a `key` field. Callers expect a + // plain ReadonlyArray. + const normalizedIssues = result.issues.map((issue) => ({ + path: Array.isArray(issue.path) + ? (issue.path.map((segment) => + typeof segment === "object" && segment !== null + ? segment.key + : segment, + ) as ReadonlyArray) + : ([] as ReadonlyArray), + message: issue.message, + })); + + // Always log the full issues server-side for operator correlation, + // even when omitted from the response body in production. + logger.warn("Request body validation failed", { + plugin: this.name, + requestId, + issues: normalizedIssues, + }); + + const isProduction = process.env.NODE_ENV === "production"; + const includeIssues = !isProduction || exposeValidationErrors; + + const body: { + error: string; + code: string; + requestId: string; + issues?: Array<{ + path: ReadonlyArray; + message: string; + }>; + } = { + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId, + }; + if (includeIssues) { + body.issues = normalizedIssues; + } + + res.status(400).json(body); + return; + } + + // Narrow req.body to the validated value. This preserves any + // transformation performed by the schema (e.g. coercion), though + // v1 docs advise against relying on transforms. + (req as { body: unknown }).body = result.value; + + await handler(req as IAppRequestWithBody, res); + }; + } + // build execution options by merging defaults, plugin config, and user overrides private _buildExecutionConfig( options: PluginExecutionSettings, diff --git a/packages/appkit/src/plugin/tests/body-validation.test.ts b/packages/appkit/src/plugin/tests/body-validation.test.ts new file mode 100644 index 000000000..808778a4f --- /dev/null +++ b/packages/appkit/src/plugin/tests/body-validation.test.ts @@ -0,0 +1,372 @@ +import type { StandardSchemaV1 } from "@standard-schema/spec"; +import { + createMockRequest, + createMockResponse, + createMockRouter, + createMockTelemetry, + mockServiceContext, +} from "@tools/test-helpers"; +import type express from "express"; +import type { BasePluginConfig } from "shared"; +import { + afterEach, + assertType, + beforeEach, + describe, + expect, + test, + vi, +} from "vitest"; +import { z } from "zod"; +import { AppManager } from "../../app"; +import { CacheManager } from "../../cache"; +import { ServiceContext } from "../../context/service-context"; +import { StreamManager } from "../../stream"; +import { TelemetryManager, type TelemetryProvider } from "../../telemetry"; +import { Plugin } from "../plugin"; + +vi.mock("../../app"); +vi.mock("../../cache", () => ({ + CacheManager: { + getInstanceSync: vi.fn(), + }, +})); +vi.mock("../../stream"); +vi.mock("../../telemetry", () => ({ + TelemetryManager: { + getProvider: vi.fn(), + }, + normalizeTelemetryOptions: vi.fn((config) => { + if (typeof config === "boolean") { + return { traces: config, metrics: config, logs: config }; + } + return config || { traces: true, metrics: true, logs: true }; + }), +})); + +// Silence logger output during validation-failure tests. +vi.mock("../../logging/logger", () => ({ + createLogger: vi.fn(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + event: vi.fn(), + })), +})); + +class TestPlugin extends Plugin { + // Expose protected route() for testing. + public exposedRoute( + router: express.Router, + config: Parameters>[1], + ) { + // biome-ignore lint/complexity/useLiteralKeys: calling protected member from subclass + return this["route"](router, config); + } +} + +function createTestPlugin(): TestPlugin { + vi.mocked(CacheManager.getInstanceSync).mockReturnValue({ + get: vi.fn(), + set: vi.fn(), + delete: vi.fn(), + } as any); + vi.mocked(AppManager).mockImplementation(() => ({}) as any); + vi.mocked(StreamManager).mockImplementation( + () => + ({ + stream: vi.fn(), + abortAll: vi.fn(), + }) as any, + ); + vi.mocked(TelemetryManager.getProvider).mockReturnValue( + createMockTelemetry() as TelemetryProvider, + ); + return new TestPlugin({ name: "test" }); +} + +describe("route body validation", () => { + let serviceContextMock: Awaited>; + const originalNodeEnv = process.env.NODE_ENV; + + beforeEach(async () => { + ServiceContext.reset(); + serviceContextMock = await mockServiceContext(); + }); + + afterEach(() => { + serviceContextMock?.restore(); + process.env.NODE_ENV = originalNodeEnv; + vi.clearAllMocks(); + }); + + test("calls handler and narrows req.body when validation succeeds", async () => { + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ + content: z.string().min(1), + conversationId: z.string().optional(), + }); + + const handlerSpy = vi.fn(async (_req: any, res: any) => { + res.status(200).json({ ok: true }); + }); + + plugin.exposedRoute>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: { content: "hello", conversationId: "conv-1" }, + }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).toHaveBeenCalledTimes(1); + expect(res.status).toHaveBeenCalledWith(200); + // After validation, req.body is the validated (narrowed) value. + expect(req.body).toEqual({ content: "hello", conversationId: "conv-1" }); + }); + + test("returns canonical 400 with issues in development", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + const handlerSpy = vi.fn(); + + plugin.exposedRoute>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), + issues: expect.arrayContaining([ + expect.objectContaining({ + path: expect.arrayContaining(["content"]), + message: expect.any(String), + }), + ]), + }), + ); + }); + + test("omits issues in production by default", async () => { + process.env.NODE_ENV = "production"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + const handlerSpy = vi.fn(); + + plugin.exposedRoute>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + const calledWith = (res.json as any).mock.calls[0][0]; + expect(calledWith).toMatchObject({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), + }); + expect(calledWith).not.toHaveProperty("issues"); + }); + + test("exposes issues in production when exposeValidationErrors=true", async () => { + process.env.NODE_ENV = "production"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + const handlerSpy = vi.fn(); + + plugin.exposedRoute>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + exposeValidationErrors: true, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + issues: expect.any(Array), + }), + ); + }); + + test("prefers x-request-id header for requestId", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: {}, + headers: { "x-request-id": "trace-abc-123" }, + }); + const res = createMockResponse(); + + await handler(req, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + requestId: "trace-abc-123", + }), + ); + }); + + test("passes the exact handler through when no body schema is provided", async () => { + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + + const postSpy = vi.spyOn(router, "post"); + + const handlerRef = vi.fn(async () => {}); + + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + handler: handlerRef, + }); + + // Without a body schema, `route()` should register the exact + // handler reference (no wrapping). + expect(postSpy).toHaveBeenCalledTimes(1); + const registered = postSpy.mock.calls[0][1]; + expect(registered).toBe(handlerRef); + }); + + test("supports schemas that return a Promise from ~standard.validate", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const asyncSchema: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + validate: async (value) => { + if ( + typeof value === "object" && + value !== null && + "content" in value && + typeof (value as any).content === "string" && + (value as any).content.length > 0 + ) { + return { value: { content: (value as any).content } }; + } + return { + issues: [{ message: "content required", path: ["content"] }], + }; + }, + }, + }; + + const handlerSpy = vi.fn(async (_req, res: any) => { + res.status(200).json({ ok: true }); + }); + + plugin.exposedRoute<{ content: string }>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: asyncSchema, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/messages"); + const goodReq = createMockRequest({ body: { content: "hello" } }); + const goodRes = createMockResponse(); + await handler(goodReq, goodRes); + expect(handlerSpy).toHaveBeenCalledTimes(1); + expect(goodRes.status).toHaveBeenCalledWith(200); + + const badReq = createMockRequest({ body: {} }); + const badRes = createMockResponse(); + await handler(badReq, badRes); + expect(badRes.status).toHaveBeenCalledWith(400); + }); + + // Compile-time check: handler's req.body should be typed as the schema's + // output. If this block type-checks, the generic is threading through. + test("type-level: handler req.body is narrowed to schema output", () => { + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + + const schema = z.object({ + content: z.string().min(1), + conversationId: z.string().optional(), + }); + type Body = z.infer; + + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: async (req, _res) => { + // These assertions fail at compile time if generics regress. + assertType(req.body.content); + assertType(req.body.conversationId); + }, + }); + }); +}); diff --git a/packages/appkit/src/plugins/genie/genie.ts b/packages/appkit/src/plugins/genie/genie.ts index 712aadbf5..893964680 100644 --- a/packages/appkit/src/plugins/genie/genie.ts +++ b/packages/appkit/src/plugins/genie/genie.ts @@ -1,6 +1,11 @@ import { randomUUID } from "node:crypto"; import type express from "express"; -import type { IAppRouter, StreamExecutionSettings } from "shared"; +import type { + IAppRequestWithBody, + IAppRouter, + StreamExecutionSettings, +} from "shared"; +import { z } from "zod"; import { GenieConnector } from "../../connectors"; import { getWorkspaceClient } from "../../context"; import { createLogger } from "../../logging"; @@ -10,11 +15,22 @@ import { genieStreamDefaults } from "./defaults"; import manifest from "./manifest.json"; import type { GenieConversationHistoryResponse, - GenieSendMessageRequest, GenieStreamEvent, IGenieConfig, } from "./types"; +/** + * Request body for POST /:alias/messages. Validated via Standard Schema + * (Zod natively implements `~standard` from v3.24+). The handler sees + * `req.body` typed as `SendMessageBody` once validation passes. + */ +const sendMessageBodySchema = z.object({ + content: z.string().min(1), + conversationId: z.string().optional(), +}); + +type SendMessageBody = z.infer; + const logger = createLogger("genie"); export class GeniePlugin extends Plugin { @@ -48,11 +64,12 @@ export class GeniePlugin extends Plugin { } injectRoutes(router: IAppRouter) { - this.route(router, { + this.route(router, { name: "sendMessage", method: "post", path: "/:alias/messages", - handler: async (req: express.Request, res: express.Response) => { + body: sendMessageBodySchema, + handler: async (req, res) => { await this.asUser(req)._handleSendMessage(req, res); }, }); @@ -77,7 +94,7 @@ export class GeniePlugin extends Plugin { } async _handleSendMessage( - req: express.Request, + req: IAppRequestWithBody, res: express.Response, ): Promise { const { alias } = req.params; @@ -88,12 +105,8 @@ export class GeniePlugin extends Plugin { return; } - const { content, conversationId } = req.body as GenieSendMessageRequest; - - if (!content) { - res.status(400).json({ error: "content is required" }); - return; - } + // Body is validated+narrowed by the framework before this runs. + const { content, conversationId } = req.body; logger.debug( "Sending message to space %s (alias=%s, conversationId=%s)", diff --git a/packages/appkit/src/plugins/genie/tests/genie.test.ts b/packages/appkit/src/plugins/genie/tests/genie.test.ts index 3cf0784d6..a26859f33 100644 --- a/packages/appkit/src/plugins/genie/tests/genie.test.ts +++ b/packages/appkit/src/plugins/genie/tests/genie.test.ts @@ -243,7 +243,7 @@ describe("Genie Plugin", () => { }); describe("validation", () => { - test("should return 400 when content is missing", async () => { + test("should return 400 with canonical shape when content is missing", async () => { const plugin = new GeniePlugin(config); const { router, getHandler } = createMockRouter(); @@ -263,9 +263,49 @@ describe("Genie Plugin", () => { await handler(mockReq, mockRes); expect(mockRes.status).toHaveBeenCalledWith(400); - expect(mockRes.json).toHaveBeenCalledWith({ - error: "content is required", + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), + issues: expect.arrayContaining([ + expect.objectContaining({ + path: expect.arrayContaining(["content"]), + message: expect.any(String), + }), + ]), + }), + ); + // Handler should not have invoked the genie service. + expect(mockGenieService.startConversation).not.toHaveBeenCalled(); + }); + + test("should return 400 when content is empty string", async () => { + const plugin = new GeniePlugin(config); + const { router, getHandler } = createMockRouter(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/messages"); + const mockReq = createMockRequest({ + params: { alias: "myspace" }, + body: { content: "" }, + headers: { + "x-forwarded-access-token": "user-token", + "x-forwarded-user": "user-1", + }, }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); }); }); diff --git a/packages/appkit/src/plugins/genie/types.ts b/packages/appkit/src/plugins/genie/types.ts index 3d73a7b2a..cf6149345 100644 --- a/packages/appkit/src/plugins/genie/types.ts +++ b/packages/appkit/src/plugins/genie/types.ts @@ -10,8 +10,3 @@ export interface IGenieConfig extends BasePluginConfig { /** Genie polling timeout in ms. Set to 0 for indefinite. Default: 120000 (2 min) */ timeout?: number; } - -export interface GenieSendMessageRequest { - content: string; - conversationId?: string; -} diff --git a/packages/shared/package.json b/packages/shared/package.json index 27d268ca3..217c2e554 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -22,6 +22,7 @@ "clean:full": "rm -rf dist node_modules" }, "devDependencies": { + "@standard-schema/spec": "1.1.0", "@types/express": "4.17.23", "@types/json-schema": "7.0.15", "@types/node": "25.2.3", diff --git a/packages/shared/src/plugin.ts b/packages/shared/src/plugin.ts index 9fa8066c0..1156dccd8 100644 --- a/packages/shared/src/plugin.ts +++ b/packages/shared/src/plugin.ts @@ -1,3 +1,4 @@ +import type { StandardSchemaV1 } from "@standard-schema/spec"; import type express from "express"; import type { JSONSchema7 } from "json-schema"; import type { @@ -202,14 +203,61 @@ export type IAppRequest = express.Request; export type HttpMethod = "get" | "post" | "put" | "delete" | "patch" | "head"; -export type RouteConfig = { +/** + * Express request type used by route handlers. When `TBody` is `any` the + * type collapses to the plain `express.Request`, keeping backwards + * compatibility with handlers typed as `(req: express.Request, res: ...)`. + * When a Standard Schema is provided via `body`, `TBody` is narrowed to the + * schema's output type so handlers see a fully-typed `req.body`. + */ +export type IAppRequestWithBody = 0 extends 1 & TBody + ? express.Request + : express.Request< + Record, + unknown, + TBody, + Record + >; + +/** + * Route registration config for `Plugin.route()`. + * + * @typeParam TBody - The validated/narrowed `req.body` type. Defaults to `any` + * so existing route registrations that do not provide a `body` schema keep + * compiling without changes. When a `body` schema is provided, `TBody` is + * inferred as the schema's Standard Schema output type. + */ +export type RouteConfig = { /** Unique name for this endpoint (used for frontend access) */ name: string; method: HttpMethod; path: string; - handler: (req: IAppRequest, res: IAppResponse) => Promise; + handler: ( + req: IAppRequestWithBody, + res: IAppResponse, + ) => Promise; /** When true, the server will skip JSON body parsing for this route (e.g. file uploads). */ skipBodyParsing?: boolean; + /** + * Optional Standard Schema describing the shape of `req.body`. + * + * When present, the framework validates the request body before the handler + * runs. On validation failure it emits a canonical 400 response with shape + * `{ error, code: "VALIDATION_ERROR", requestId, issues? }` and the handler + * is not invoked. On success, `req.body` is narrowed to the schema's output + * type for the handler. Any Standard Schema v1-compatible validator may be + * used (zod 3.24+, valibot, arktype, etc.). + * + * @see https://standardschema.dev + */ + body?: StandardSchemaV1; + /** + * When true, the canonical 400 response includes the full `issues` array + * even in production (default: only included when `NODE_ENV !== "production"`). + * Use sparingly — surfacing detailed validation issues in production can + * leak schema internals to clients. + */ + exposeValidationErrors?: boolean; }; /** Map of endpoint names to their full paths for a plugin */ diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 9ca11b818..289079ced 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -326,7 +326,13 @@ importers: ws: specifier: 8.18.3 version: 8.18.3(bufferutil@4.0.9) + zod: + specifier: 4.1.13 + version: 4.1.13 devDependencies: + '@standard-schema/spec': + specifier: 1.1.0 + version: 1.1.0 '@types/express': specifier: 4.17.25 version: 4.17.25 @@ -540,6 +546,9 @@ importers: specifier: 12.1.0 version: 12.1.0 devDependencies: + '@standard-schema/spec': + specifier: 1.1.0 + version: 1.1.0 '@types/express': specifier: 4.17.23 version: 4.17.23 From 64b2e685789f685eaf8f3f82f0ec38ac47e42e47 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 17 Apr 2026 14:33:13 +0200 Subject: [PATCH 02/14] feat(analytics): validate query body with Standard Schema Replaces the `as IAnalyticsQueryRequest` cast on POST /api/analytics/query/:query_key with a Zod schema covering `parameters?` and `format?` (the JSON default now lives in the schema). The now-unused IAnalyticsQueryRequest, AnalyticsFormat, and AnalyticsQueryResponse types are deleted; appkit-ui has its own AnalyticsFormat definition and no external consumer imports the others. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .../appkit/src/plugins/analytics/analytics.ts | 38 ++++++-- .../plugins/analytics/tests/analytics.test.ts | 95 +++++++++++++++++++ .../appkit/src/plugins/analytics/types.ts | 13 --- 3 files changed, 123 insertions(+), 23 deletions(-) diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index a9c688dac..0406015f3 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -1,11 +1,13 @@ import type { WorkspaceClient } from "@databricks/sdk-experimental"; import type express from "express"; import type { + IAppRequestWithBody, IAppRouter, PluginExecuteConfig, SQLTypeMarker, StreamExecutionSettings, } from "shared"; +import { z } from "zod"; import { SQLWarehouseConnector } from "../../connectors"; import { getWarehouseId, getWorkspaceClient } from "../../context"; import { createLogger } from "../../logging/logger"; @@ -14,11 +16,22 @@ import type { PluginManifest } from "../../registry"; import { queryDefaults } from "./defaults"; import manifest from "./manifest.json"; import { QueryProcessor } from "./query"; -import type { - AnalyticsQueryResponse, - IAnalyticsConfig, - IAnalyticsQueryRequest, -} from "./types"; +import type { IAnalyticsConfig } from "./types"; + +/** + * Request body for POST /query/:query_key. Validated via Standard Schema + * (Zod natively implements `~standard` from v3.24+). The `format` field + * defaults to "JSON" via the schema so the handler sees a fully-populated + * body with no manual fallback needed. `parameters` accepts any value + * shape — per-query parameter schemas are the application's concern, not + * the plugin's. + */ +const queryBodySchema = z.object({ + parameters: z.record(z.string(), z.unknown()).optional(), + format: z.enum(["JSON", "ARROW"]).default("JSON"), +}); + +type QueryBody = z.infer; const logger = createLogger("analytics"); @@ -55,11 +68,12 @@ export class AnalyticsPlugin extends Plugin { }, }); - this.route(router, { + this.route(router, { name: "query", method: "post", path: "/query/:query_key", - handler: async (req: express.Request, res: express.Response) => { + body: queryBodySchema, + handler: async (req, res) => { await this._handleQueryRoute(req, res); }, }); @@ -111,11 +125,13 @@ export class AnalyticsPlugin extends Plugin { * When called via asUser(req), uses the user's Databricks credentials. */ async _handleQueryRoute( - req: express.Request, + req: IAppRequestWithBody, res: express.Response, ): Promise { const { query_key } = req.params; - const { parameters, format = "JSON" } = req.body as IAnalyticsQueryRequest; + // Body is validated+narrowed by the framework before this runs; + // `format` defaults to "JSON" via the schema. + const { parameters, format } = req.body; // Request-scoped logging with WideEvent tracking logger.debug(req, "Executing query: %s (format=%s)", query_key, format); @@ -187,9 +203,11 @@ export class AnalyticsPlugin extends Plugin { await executor.executeStream( res, async (signal) => { + // The body schema validates shape only; per-value SQL type checks + // happen inside QueryProcessor via isSQLTypeMarker. const processedParams = await this.queryProcessor.processQueryParams( query, - parameters, + parameters as Record, ); const result = await executor.query( diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index 9a30440ed..ea998c8ca 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -127,6 +127,101 @@ describe("Analytics Plugin", () => { }); }); + test("/query/:query_key should return canonical 400 when format is invalid", async () => { + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { format: 123 }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), + }), + ); + // Handler body (SQL execution) should never be reached. + expect(executeMock).not.toHaveBeenCalled(); + expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); + }); + + test("/query/:query_key should return canonical 400 when format is unknown string", async () => { + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { format: "PARQUET" }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(executeMock).not.toHaveBeenCalled(); + }); + + test("/query/:query_key should apply format default of JSON when omitted", async () => { + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ + query: "SELECT 1", + isAsUser: false, + }); + + const executeMock = vi.fn().mockResolvedValue({ + result: { data: [{ id: 1 }] }, + }); + (plugin as any).SQLClient.executeStatement = executeMock; + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + // No `format` field — schema should default it to "JSON" + // (i.e. no formatParameters are passed to executeStatement). + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { parameters: {} }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(executeMock).toHaveBeenCalledTimes(1); + // JSON format should NOT set a disposition/format ARROW_STREAM. + const call = executeMock.mock.calls[0][1]; + expect(call).not.toHaveProperty("disposition"); + expect(call).not.toHaveProperty("format"); + }); + test("/query/:query_key should execute as service principal for .sql files (isAsUser: false)", async () => { const plugin = new AnalyticsPlugin(config); const { router, getHandler } = createMockRouter(); diff --git a/packages/appkit/src/plugins/analytics/types.ts b/packages/appkit/src/plugins/analytics/types.ts index c58b6ecfe..ddf086dbc 100644 --- a/packages/appkit/src/plugins/analytics/types.ts +++ b/packages/appkit/src/plugins/analytics/types.ts @@ -3,16 +3,3 @@ import type { BasePluginConfig } from "shared"; export interface IAnalyticsConfig extends BasePluginConfig { timeout?: number; } - -export type AnalyticsFormat = "JSON" | "ARROW"; -export interface IAnalyticsQueryRequest { - parameters?: Record; - format?: AnalyticsFormat; -} - -export interface AnalyticsQueryResponse { - chunk_index: number; - row_offset: number; - row_count: number; - data: any[]; -} From 8e98c2cfc9d9b796be8af669d80f0eabc498b264 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 17 Apr 2026 14:42:17 +0200 Subject: [PATCH 03/14] feat(appkit): validate vector-search bodies with Standard Schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Zod schemas for POST /:alias/query and POST /:alias/next-page, including a cross-field refinement on the query route that enforces at least one of queryText or queryVector is present (issue path ["queryText"] for stable client-side rendering). Replaces the manual truthy checks on both routes. SearchRequest stays — the programmatic `plugin.query(alias, request)` exports still contract on it. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .../vector-search/tests/vector-search.test.ts | 180 ++++++++++++++++++ .../plugins/vector-search/vector-search.ts | 83 ++++++-- 2 files changed, 243 insertions(+), 20 deletions(-) diff --git a/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts b/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts index 104eb1cb6..5f20190f3 100644 --- a/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts +++ b/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts @@ -1,3 +1,8 @@ +import { + createMockRequest, + createMockResponse, + createMockRouter, +} from "@tools/test-helpers"; import { beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("../../../context", () => ({ @@ -327,4 +332,179 @@ describe("VectorSearchPlugin", () => { await expect(plugin.shutdown()).resolves.not.toThrow(); }); }); + + describe("routes — body validation", () => { + function buildPlugin() { + const plugin = new VectorSearchPlugin({ + indexes: { + products: { + indexName: "cat.sch.products", + columns: ["id", "title"], + queryType: "hybrid", + pagination: true, + endpointName: "ep-1", + }, + }, + }); + return plugin; + } + + describe("POST /:alias/query", () => { + it("returns canonical 400 when neither queryText nor queryVector is present", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/query"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: {}, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), + issues: expect.arrayContaining([ + expect.objectContaining({ + path: expect.arrayContaining(["queryText"]), + message: "queryText or queryVector is required", + }), + ]), + }), + ); + // Handler body must not have reached the VS connector. + expect(mockRequest).not.toHaveBeenCalled(); + }); + + it("passes validation and reaches the connector with queryText only", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/query"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: { queryText: "machine learning" }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).not.toHaveBeenCalledWith(400); + expect(mockRequest).toHaveBeenCalledTimes(1); + const callBody = mockRequest.mock.calls[0][0].payload; + expect(callBody.query_text).toBe("machine learning"); + expect(callBody.query_vector).toBeUndefined(); + }); + + it("passes validation and reaches the connector with queryVector only", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/query"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: { queryVector: [0.1, 0.2, 0.3] }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).not.toHaveBeenCalledWith(400); + expect(mockRequest).toHaveBeenCalledTimes(1); + const callBody = mockRequest.mock.calls[0][0].payload; + expect(callBody.query_vector).toEqual([0.1, 0.2, 0.3]); + expect(callBody.query_text).toBeUndefined(); + }); + }); + + describe("POST /:alias/next-page", () => { + it("returns canonical 400 when pageToken is missing", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/next-page"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: {}, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), + issues: expect.arrayContaining([ + expect.objectContaining({ + path: expect.arrayContaining(["pageToken"]), + }), + ]), + }), + ); + expect(mockRequest).not.toHaveBeenCalled(); + }); + + it("returns canonical 400 when pageToken is empty string", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/next-page"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: { pageToken: "" }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(mockRequest).not.toHaveBeenCalled(); + }); + + it("passes validation and reaches the connector with a valid pageToken", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/next-page"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: { pageToken: "token-abc" }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).not.toHaveBeenCalledWith(400); + expect(mockRequest).toHaveBeenCalledTimes(1); + const callBody = mockRequest.mock.calls[0][0].payload; + expect(callBody.page_token).toBe("token-abc"); + }); + }); + }); }); diff --git a/packages/appkit/src/plugins/vector-search/vector-search.ts b/packages/appkit/src/plugins/vector-search/vector-search.ts index fefc3f409..4e75423a6 100644 --- a/packages/appkit/src/plugins/vector-search/vector-search.ts +++ b/packages/appkit/src/plugins/vector-search/vector-search.ts @@ -1,5 +1,10 @@ import type express from "express"; -import type { IAppRouter, PluginExecutionSettings } from "shared"; +import type { + IAppRequestWithBody, + IAppRouter, + PluginExecutionSettings, +} from "shared"; +import { z } from "zod"; import { VectorSearchConnector } from "../../connectors/vector-search/client"; import type { VsRawResponse } from "../../connectors/vector-search/types"; import { getWorkspaceClient } from "../../context"; @@ -15,6 +20,47 @@ import type { SearchResponse, } from "./types"; +/** + * Request body for POST /:alias/query. Validated via Standard Schema + * (Zod natively implements `~standard` from v3.24+). The schema expresses + * both the shape of a Vector Search query and a cross-field refinement: + * at least one of `queryText` or `queryVector` must be present. The + * refinement surfaces its issue on the `queryText` path so clients get a + * single, stable location to render the error. + */ +const searchFiltersSchema = z.record( + z.string(), + z.union([ + z.string(), + z.number(), + z.boolean(), + z.array(z.union([z.string(), z.number()])), + ]), +); + +const queryBodySchema = z + .object({ + queryText: z.string().optional(), + queryVector: z.array(z.number()).optional(), + columns: z.array(z.string()).optional(), + numResults: z.number().optional(), + queryType: z.enum(["ann", "hybrid", "full_text"]).optional(), + filters: searchFiltersSchema.optional(), + reranker: z.boolean().optional(), + }) + .refine((value) => Boolean(value.queryText) || Boolean(value.queryVector), { + message: "queryText or queryVector is required", + path: ["queryText"], + }); + +type QueryBody = z.infer; + +const nextPageBodySchema = z.object({ + pageToken: z.string().min(1), +}); + +type NextPageBody = z.infer; + const logger = createLogger("vector-search"); const querySettings: PluginExecutionSettings = { @@ -67,11 +113,15 @@ export class VectorSearchPlugin extends Plugin { } injectRoutes(router: IAppRouter) { - this.route(router, { + this.route(router, { name: "query", method: "post", path: "/:alias/query", - handler: async (req: express.Request, res: express.Response) => { + body: queryBodySchema, + handler: async ( + req: IAppRequestWithBody, + res: express.Response, + ) => { const indexConfig = this._resolveIndex(req.params.alias); if (!indexConfig) { res.status(404).json({ @@ -81,14 +131,9 @@ export class VectorSearchPlugin extends Plugin { return; } - const body: SearchRequest = req.body; - if (!body.queryText && !body.queryVector) { - res.status(400).json({ - error: "queryText or queryVector is required", - plugin: this.name, - }); - return; - } + // Body validated+narrowed by the framework; cross-field refinement + // guarantees at least one of queryText/queryVector is present. + const body = req.body; try { const prepared = await this._prepareQuery(body, indexConfig); @@ -127,11 +172,15 @@ export class VectorSearchPlugin extends Plugin { }, }); - this.route(router, { + this.route(router, { name: "queryNextPage", method: "post", path: "/:alias/next-page", - handler: async (req: express.Request, res: express.Response) => { + body: nextPageBodySchema, + handler: async ( + req: IAppRequestWithBody, + res: express.Response, + ) => { const indexConfig = this._resolveIndex(req.params.alias); if (!indexConfig) { res.status(404).json({ @@ -157,14 +206,8 @@ export class VectorSearchPlugin extends Plugin { return; } + // Body validated+narrowed by the framework; pageToken is guaranteed non-empty. const { pageToken } = req.body; - if (!pageToken) { - res.status(400).json({ - error: "pageToken is required", - plugin: this.name, - }); - return; - } try { const plugin = From 45b733899cf04aedcf7b3c4e7f375fe6f20edd6b Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 17 Apr 2026 14:49:45 +0200 Subject: [PATCH 04/14] feat(appkit): validate files mkdir body with Standard Schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ports the mkdir-specific path rules (non-empty, max 4096 chars, no null bytes) into a Zod schema refinement on POST /api/files/:volumeKey/mkdir. The manual `typeof req.body?.path` check and the in-handler `_isValidPath` call for mkdir are deleted. `_isValidPath` itself stays intact — it still guards the GET query- based routes and the upload route, which migrate to schema validation in a separate follow-up PR. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/plugins/files/plugin.ts | 48 ++++-- .../src/plugins/files/tests/plugin.test.ts | 162 +++++++++++++++++- 2 files changed, 198 insertions(+), 12 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 9344af85b..a48db22ef 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -2,7 +2,12 @@ import { STATUS_CODES } from "node:http"; import { Readable } from "node:stream"; import { ApiError } from "@databricks/sdk-experimental"; import type express from "express"; -import type { IAppRouter, PluginExecutionSettings } from "shared"; +import type { + IAppRequestWithBody, + IAppRouter, + PluginExecutionSettings, +} from "shared"; +import { z } from "zod"; import { contentTypeFromPath, FilesConnector, @@ -32,6 +37,27 @@ import type { VolumeHandle, } from "./types"; +/** + * Request body for POST /:volumeKey/mkdir. Validated via Standard Schema + * (Zod natively implements `~standard` from v3.24+). The schema mirrors the + * existing `_isValidPath` rules — non-empty, at most 4096 characters, and + * no null bytes — expressed as a `.refine()` so the issue surfaces on the + * `path` field for clients. `_isValidPath` itself stays in place for the + * GET routes that still read `req.query.path`; those migrate separately. + */ +const mkdirBodySchema = z.object({ + path: z + .string() + .min(1) + .max(4096) + .refine((p) => !p.includes("\u0000"), { + message: "path must not contain null bytes", + path: ["path"], + }), +}); + +type MkdirBody = z.infer; + const logger = createLogger("files"); export class FilesPlugin extends Plugin { @@ -304,11 +330,15 @@ export class FilesPlugin extends Plugin { }, }); - this.route(router, { + this.route(router, { name: "mkdir", method: "post", path: "/:volumeKey/mkdir", - handler: async (req: express.Request, res: express.Response) => { + body: mkdirBodySchema, + handler: async ( + req: IAppRequestWithBody, + res: express.Response, + ) => { const { connector, volumeKey } = this._resolveVolume(req, res); if (!connector) return; await this._handleMkdir(req, res, connector, volumeKey); @@ -820,18 +850,14 @@ export class FilesPlugin extends Plugin { } private async _handleMkdir( - req: express.Request, + req: IAppRequestWithBody, res: express.Response, connector: FilesConnector, volumeKey: string, ): Promise { - const dirPath = - typeof req.body?.path === "string" ? req.body.path : undefined; - const valid = this._isValidPath(dirPath); - if (valid !== true) { - res.status(400).json({ error: valid, plugin: this.name }); - return; - } + // Body validated+narrowed by the framework; `path` is guaranteed to be a + // non-empty, null-byte-free string at most 4096 characters long. + const dirPath = req.body.path; try { const userPlugin = this.asUser(req); diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 99e08b8c7..a42daf768 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -1,4 +1,10 @@ -import { mockServiceContext, setupDatabricksEnv } from "@tools/test-helpers"; +import { + createMockRequest, + createMockResponse, + createMockRouter, + mockServiceContext, + setupDatabricksEnv, +} from "@tools/test-helpers"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { ServiceContext } from "../../../context/service-context"; import { AuthenticationError } from "../../../errors"; @@ -1009,4 +1015,158 @@ describe("FilesPlugin", () => { ); }); }); + + describe("POST /:volumeKey/mkdir — body validation", () => { + test("returns canonical 400 when path is missing", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:volumeKey/mkdir"); + const mockReq = createMockRequest({ + params: { volumeKey: "uploads" }, + body: {}, + headers: { + "x-forwarded-access-token": "test-token", + "x-forwarded-user": "test-user", + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), + issues: expect.arrayContaining([ + expect.objectContaining({ + path: expect.arrayContaining(["path"]), + }), + ]), + }), + ); + // Handler body must not have reached the SDK. + expect(mockClient.files.createDirectory).not.toHaveBeenCalled(); + }); + + test("returns canonical 400 when path is empty string", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:volumeKey/mkdir"); + const mockReq = createMockRequest({ + params: { volumeKey: "uploads" }, + body: { path: "" }, + headers: { + "x-forwarded-access-token": "test-token", + "x-forwarded-user": "test-user", + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(mockClient.files.createDirectory).not.toHaveBeenCalled(); + }); + + test("returns canonical 400 when path exceeds 4096 characters", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:volumeKey/mkdir"); + const mockReq = createMockRequest({ + params: { volumeKey: "uploads" }, + body: { path: "a".repeat(4097) }, + headers: { + "x-forwarded-access-token": "test-token", + "x-forwarded-user": "test-user", + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(mockClient.files.createDirectory).not.toHaveBeenCalled(); + }); + + test("returns canonical 400 when path contains a null byte", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:volumeKey/mkdir"); + const mockReq = createMockRequest({ + params: { volumeKey: "uploads" }, + body: { path: "foo\u0000bar" }, + headers: { + "x-forwarded-access-token": "test-token", + "x-forwarded-user": "test-user", + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + issues: expect.arrayContaining([ + expect.objectContaining({ + path: expect.arrayContaining(["path"]), + message: "path must not contain null bytes", + }), + ]), + }), + ); + expect(mockClient.files.createDirectory).not.toHaveBeenCalled(); + }); + + test("passes validation and reaches the SDK with a valid path", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + mockClient.files.createDirectory.mockResolvedValue(undefined); + + const handler = getHandler("POST", "/:volumeKey/mkdir"); + const mockReq = createMockRequest({ + params: { volumeKey: "uploads" }, + body: { path: "/Volumes/catalog/schema/uploads/new-dir" }, + headers: { + "x-forwarded-access-token": "test-token", + "x-forwarded-user": "test-user", + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).not.toHaveBeenCalledWith(400); + expect(mockClient.files.createDirectory).toHaveBeenCalledTimes(1); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ success: true }), + ); + }); + }); }); From 45a6598d5a8b1b2a494262129808ba08fe834996 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 17 Apr 2026 14:52:51 +0200 Subject: [PATCH 05/14] docs(appkit): note serving validation follow-up in source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a JSDoc block above ServingPlugin explaining why serving does not use the new RouteConfig.body mechanism — its body shape is resolved dynamically per-alias via filterRequestBody() against disk-cached endpoint schemas, not statically declarable as a RouteConfig field. The comment includes a TODO(serving-validation-follow-up) marker and a placeholder for the tracking GitHub issue URL (to be filled in once the issue is opened). Co-authored-by: Isaac Signed-off-by: Atila Fassina --- .../appkit/src/plugins/serving/serving.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/appkit/src/plugins/serving/serving.ts b/packages/appkit/src/plugins/serving/serving.ts index c6c952b61..ea7c7d1fc 100644 --- a/packages/appkit/src/plugins/serving/serving.ts +++ b/packages/appkit/src/plugins/serving/serving.ts @@ -39,6 +39,30 @@ interface ResolvedEndpoint { name: string; } +/** + * TODO(serving-validation-follow-up): migrate request-body validation to the + * framework's `RouteConfig.body` mechanism (Standard Schema) once a per-request + * schema resolution story lands — see GitHub issue . + * + * The other AppKit plugins (analytics, genie, vector-search, files/mkdir) all + * declare their request-body schemas statically on `RouteConfig.body` and rely + * on the framework to validate incoming `req.body` against a Standard Schema + * before the handler runs. + * + * The serving plugin is the exception: its body shape is NOT static. Each + * configured alias proxies to a different model-serving endpoint, and every + * endpoint has its own input schema (inferred and cached on disk at + * `.databricks/appkit/.appkit-serving-types-cache.json`). The body therefore + * has to be resolved and filtered per-request via `filterRequestBody()` in + * `_handleInvoke` / `_handleStream` / `invoke`, keyed off the `:alias` route + * param. + * + * Fitting this into `RouteConfig.body` needs its own design conversation — + * most likely a function-form `body: (req) => schema` that resolves the + * Standard Schema per-request based on the alias and the loaded allowlists. + * Until that lands, serving validation intentionally stays inside + * `filterRequestBody()` rather than the framework pipeline. + */ export class ServingPlugin extends Plugin { static manifest = manifest as PluginManifest<"serving">; From 4d6f5d94474aa2cfd1f6a9a4750f4bb6352b55b7 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 17 Apr 2026 16:44:30 +0200 Subject: [PATCH 06/14] fix(appkit): harden body validation per review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses four confirmed blockers and three nits surfaced by the multi-model debate review: - Wrap schema.validate() in try/catch — thrown refinements and async rejections now return a canonical 500 with { error, code: "VALIDATION_INTERNAL_ERROR", requestId } instead of escaping to a stack-leaking 500. Thrown message is never reflected; only errorName is logged server-side. - Add size caps to every Zod schema landed by this PR (genie content/ conversationId, analytics parameters key count, vector-search queryText/queryVector/columns/numResults/filters/pageToken). Closes the DoS surface where a 10MB body would be fully parsed before rejection. - Sanitize x-request-id: enforce /^[A-Za-z0-9_-]{1,100}$/ on the incoming header; discard and regenerate if it fails. Kills the CRLF log-injection (CWE-117) and reflection vectors. - Cap validation issues at 20 during response + logging. Adds `issuesTruncated: true` flag on responses when more existed (dev mode / override only). Server-side logs omit issue.message and only record paths + counts. - nit: remove redundant path:["path"] from files mkdir refinement (field-level refine already emits at that path). - nit: fail-fast schema check at route() registration — bad body schemas now throw at plugin startup, not first request. - nit: switch requestId fallback from randomBytes(8) to randomUUID().slice(0,8) (V8-optimized, no Buffer alloc). Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/plugin/plugin.ts | 102 ++++++- .../src/plugin/tests/body-validation.test.ts | 280 ++++++++++++++++++ .../appkit/src/plugins/analytics/analytics.ts | 7 +- .../plugins/analytics/tests/analytics.test.ts | 34 +++ packages/appkit/src/plugins/files/plugin.ts | 1 - packages/appkit/src/plugins/genie/genie.ts | 4 +- .../src/plugins/genie/tests/genie.test.ts | 29 ++ .../vector-search/tests/vector-search.test.ts | 74 +++++ .../plugins/vector-search/vector-search.ts | 30 +- 9 files changed, 531 insertions(+), 30 deletions(-) diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 009d5aced..8014abecb 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -1,4 +1,4 @@ -import { randomBytes } from "node:crypto"; +import { randomUUID } from "node:crypto"; import type { StandardSchemaV1 } from "@standard-schema/spec"; import type express from "express"; import type { @@ -58,18 +58,49 @@ function hasHttpStatusCode( ); } +/** + * Character allowlist for incoming `x-request-id` headers. Restricts to + * URL-safe ASCII + underscore/hyphen and caps length at 100 characters so + * client-supplied values can never contain CRLF (log-injection / CWE-117) + * or blow up server memory. + */ +const REQUEST_ID_HEADER_PATTERN = /^[A-Za-z0-9_-]{1,100}$/; + /** * Resolve a request ID from the `x-request-id` header (if present), falling - * back to a short random token. Used by the body validation wrapper so - * operators can correlate a client-facing 400 with the full server-side - * issue log. + * back to a freshly generated UUID-derived token. Used by the body + * validation wrapper so operators can correlate a client-facing 400 with + * the full server-side issue log. + * + * The header value is validated against a strict allowlist. Invalid values + * are silently discarded — they are never logged or reflected anywhere — + * and a fresh ID is generated instead. */ function resolveRequestId(req: express.Request): string { const headerId = req.header("x-request-id"); - if (headerId && typeof headerId === "string" && headerId.length > 0) { + if ( + typeof headerId === "string" && + REQUEST_ID_HEADER_PATTERN.test(headerId) + ) { return headerId; } - return `req_${randomBytes(4).toString("hex")}`; + return `req_${randomUUID().slice(0, 8)}`; +} + +/** Maximum number of Standard Schema issues retained on a validation failure. */ +const MAX_VALIDATION_ISSUES = 20; + +/** + * Shallow runtime check that a value looks like a Standard Schema v1 + * compliant validator (object with a `~standard` property exposing a + * `validate` function). Used to surface plugin programmer errors at route + * registration time. + */ +function isStandardSchema(value: unknown): value is StandardSchemaV1 { + if (typeof value !== "object" || value === null) return false; + const standard = (value as { "~standard"?: unknown })["~standard"]; + if (typeof standard !== "object" || standard === null) return false; + return typeof (standard as { validate?: unknown }).validate === "function"; } /** @@ -538,6 +569,14 @@ export abstract class Plugin< ): void { const { name, method, path, handler } = config; + // Fail-fast: catch mis-wired `body` values at registration time so plugin + // programmer errors surface at startup instead of the first request. + if (config.body !== undefined && !isStandardSchema(config.body)) { + throw new Error( + "RouteConfig.body must be a Standard Schema v1 compliant value (e.g., a Zod schema)", + ); + } + // Zero-overhead pass-through when no body schema is provided. const effectiveHandler = config.body ? this._wrapHandlerWithBodyValidation( @@ -569,6 +608,12 @@ export abstract class Plugin< * does not call the original handler. On success the request body is * reassigned to the validated value (preserving any narrowing/coercion) * and the original handler runs as before. + * + * Exceptions thrown from the validator itself (sync throw from a + * user-written `.refine()`, or a rejected Promise from an async + * validate) are caught and converted into a canonical 500 response. The + * thrown error's message is never leaked to the client — only a fixed + * code is returned. */ private _wrapHandlerWithBodyValidation( handler: RouteConfig["handler"], @@ -576,18 +621,41 @@ export abstract class Plugin< exposeValidationErrors: boolean, ): (req: express.Request, res: express.Response) => Promise { return async (req, res) => { - let result = schema["~standard"].validate(req.body); - if (result instanceof Promise) { - result = await result; + let result: StandardSchemaV1.Result; + try { + const maybePromise = schema["~standard"].validate(req.body); + result = + maybePromise instanceof Promise ? await maybePromise : maybePromise; + } catch (error) { + const requestId = resolveRequestId(req); + // Log via AppKitError-compatible path so sensitive values inside + // `context` are redacted. The thrown error's free-form message is + // intentionally omitted to avoid leaking refinement internals. + logger.error("validation schema threw unexpectedly", { + plugin: this.name, + requestId, + errorName: error instanceof Error ? error.name : "UnknownError", + }); + res.status(500).json({ + error: "Internal validation error", + code: "VALIDATION_INTERNAL_ERROR", + requestId, + }); + return; } if (result.issues) { const requestId = resolveRequestId(req); + const totalIssueCount = result.issues.length; + const truncated = totalIssueCount > MAX_VALIDATION_ISSUES; + const retained = truncated + ? result.issues.slice(0, MAX_VALIDATION_ISSUES) + : result.issues; // Normalize Standard Schema path segments: spec allows either a // PropertyKey or an object with a `key` field. Callers expect a // plain ReadonlyArray. - const normalizedIssues = result.issues.map((issue) => ({ + const normalizedIssues = retained.map((issue) => ({ path: Array.isArray(issue.path) ? (issue.path.map((segment) => typeof segment === "object" && segment !== null @@ -598,12 +666,16 @@ export abstract class Plugin< message: issue.message, })); - // Always log the full issues server-side for operator correlation, - // even when omitted from the response body in production. + // Log only path metadata server-side; `issue.message` can contain + // arbitrary refinement text and would not pass through the + // AppKitError redactor. Callers can opt in to full issue content + // via `exposeValidationErrors` in the response. logger.warn("Request body validation failed", { plugin: this.name, requestId, - issues: normalizedIssues, + issueCount: totalIssueCount, + truncated, + paths: normalizedIssues.map((issue) => issue.path), }); const isProduction = process.env.NODE_ENV === "production"; @@ -617,6 +689,7 @@ export abstract class Plugin< path: ReadonlyArray; message: string; }>; + issuesTruncated?: boolean; } = { error: "Invalid request body", code: "VALIDATION_ERROR", @@ -624,6 +697,9 @@ export abstract class Plugin< }; if (includeIssues) { body.issues = normalizedIssues; + if (truncated) { + body.issuesTruncated = true; + } } res.status(400).json(body); diff --git a/packages/appkit/src/plugin/tests/body-validation.test.ts b/packages/appkit/src/plugin/tests/body-validation.test.ts index 808778a4f..5a1ea5944 100644 --- a/packages/appkit/src/plugin/tests/body-validation.test.ts +++ b/packages/appkit/src/plugin/tests/body-validation.test.ts @@ -369,4 +369,284 @@ describe("route body validation", () => { }, }); }); + + test("returns canonical 500 when validator throws synchronously", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + // Zod refinement that throws synchronously. + const exploding = z.object({ + content: z.string().refine(() => { + throw new Error("boom from refine"); + }), + }); + + const handlerSpy = vi.fn(); + + plugin.exposedRoute<{ content: string }>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: exploding, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ body: { content: "anything" } }); + const res = createMockResponse(); + + // Should not throw — wrapper must catch and return canonical 500. + await expect(handler(req, res)).resolves.toBeUndefined(); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(500); + const body = (res.json as any).mock.calls[0][0]; + expect(body).toMatchObject({ + error: "Internal validation error", + code: "VALIDATION_INTERNAL_ERROR", + requestId: expect.any(String), + }); + // Refinement message must not leak to the client. + expect(JSON.stringify(body)).not.toContain("boom from refine"); + }); + + test("returns canonical 500 when async validator rejects", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const rejecting: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + validate: async () => { + throw new Error("async boom"); + }, + }, + }; + + const handlerSpy = vi.fn(); + + plugin.exposedRoute<{ ok: true }>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: rejecting, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await expect(handler(req, res)).resolves.toBeUndefined(); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Internal validation error", + code: "VALIDATION_INTERNAL_ERROR", + requestId: expect.any(String), + }), + ); + }); + + test("discards malformed x-request-id and generates a fresh ID", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + // CRLF injection attempt. + const evilId = "attacker\r\nSet-Cookie: pwn=1"; + const req = createMockRequest({ + body: {}, + headers: { "x-request-id": evilId }, + }); + const res = createMockResponse(); + + await handler(req, res); + + const body = (res.json as any).mock.calls[0][0]; + // Must not reflect the attacker-supplied value. + expect(body.requestId).not.toBe(evilId); + expect(body.requestId).not.toContain("\r"); + expect(body.requestId).not.toContain("\n"); + expect(body.requestId).not.toContain("Set-Cookie"); + // Generated fallback has the `req_` prefix. + expect(body.requestId).toMatch(/^req_[A-Fa-f0-9-]+$/); + }); + + test("discards oversized x-request-id header", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute>(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const longId = "a".repeat(101); + const req = createMockRequest({ + body: {}, + headers: { "x-request-id": longId }, + }); + const res = createMockResponse(); + + await handler(req, res); + + const body = (res.json as any).mock.calls[0][0]; + expect(body.requestId).not.toBe(longId); + expect(body.requestId).toMatch(/^req_[A-Fa-f0-9-]+$/); + }); + + test("truncates the issues array to 20 entries and flags issuesTruncated", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + // A schema that always rejects with 50 issues. + const manyIssues: StandardSchemaV1> = { + "~standard": { + version: 1, + vendor: "test", + validate: () => ({ + issues: Array.from({ length: 50 }, (_, i) => ({ + message: `issue ${i}`, + path: [`field${i}`], + })), + }), + }, + }; + + const handlerSpy = vi.fn(); + + plugin.exposedRoute>(router, { + name: "many", + method: "post", + path: "/many", + body: manyIssues, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/many"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + const body = (res.json as any).mock.calls[0][0]; + expect(body.issues).toHaveLength(20); + expect(body.issuesTruncated).toBe(true); + }); + + test("does not set issuesTruncated when issue count is exactly at limit", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const exactlyTwenty: StandardSchemaV1> = { + "~standard": { + version: 1, + vendor: "test", + validate: () => ({ + issues: Array.from({ length: 20 }, (_, i) => ({ + message: `issue ${i}`, + path: [`field${i}`], + })), + }), + }, + }; + + plugin.exposedRoute>(router, { + name: "exact", + method: "post", + path: "/exact", + body: exactlyTwenty, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/exact"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + const body = (res.json as any).mock.calls[0][0]; + expect(body.issues).toHaveLength(20); + expect(body).not.toHaveProperty("issuesTruncated"); + }); + + test("omits issuesTruncated from response body in production", async () => { + process.env.NODE_ENV = "production"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const manyIssues: StandardSchemaV1> = { + "~standard": { + version: 1, + vendor: "test", + validate: () => ({ + issues: Array.from({ length: 50 }, (_, i) => ({ + message: `issue ${i}`, + path: [`field${i}`], + })), + }), + }, + }; + + plugin.exposedRoute>(router, { + name: "many", + method: "post", + path: "/many", + body: manyIssues, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/many"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + const body = (res.json as any).mock.calls[0][0]; + // Production hides issues altogether by default. + expect(body).not.toHaveProperty("issues"); + expect(body).not.toHaveProperty("issuesTruncated"); + }); + + test("throws at route registration when body is not a Standard Schema", () => { + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + + expect(() => + plugin.exposedRoute(router, { + name: "bad", + method: "post", + path: "/bad", + // Plugin author accidentally passed a plain object. + body: { parse: () => {} } as any, + handler: vi.fn(), + }), + ).toThrow(/Standard Schema v1 compliant/); + }); }); diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index 0406015f3..534486a62 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -27,7 +27,12 @@ import type { IAnalyticsConfig } from "./types"; * the plugin's. */ const queryBodySchema = z.object({ - parameters: z.record(z.string(), z.unknown()).optional(), + parameters: z + .record(z.string(), z.unknown()) + .refine((obj) => Object.keys(obj).length <= 100, { + message: "parameters may not contain more than 100 keys", + }) + .optional(), format: z.enum(["JSON", "ARROW"]).default("JSON"), }); diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index ea998c8ca..9be6c1396 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -188,6 +188,40 @@ describe("Analytics Plugin", () => { expect(executeMock).not.toHaveBeenCalled(); }); + test("/query/:query_key should return canonical 400 when parameters has more than 100 keys", async () => { + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const oversizedParams: Record = {}; + for (let i = 0; i < 101; i++) { + oversizedParams[`p${i}`] = String(i); + } + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { parameters: oversizedParams }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(executeMock).not.toHaveBeenCalled(); + expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); + }); + test("/query/:query_key should apply format default of JSON when omitted", async () => { const plugin = new AnalyticsPlugin(config); const { router, getHandler } = createMockRouter(); diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index a48db22ef..4618fbd46 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -52,7 +52,6 @@ const mkdirBodySchema = z.object({ .max(4096) .refine((p) => !p.includes("\u0000"), { message: "path must not contain null bytes", - path: ["path"], }), }); diff --git a/packages/appkit/src/plugins/genie/genie.ts b/packages/appkit/src/plugins/genie/genie.ts index 893964680..9010ace0e 100644 --- a/packages/appkit/src/plugins/genie/genie.ts +++ b/packages/appkit/src/plugins/genie/genie.ts @@ -25,8 +25,8 @@ import type { * `req.body` typed as `SendMessageBody` once validation passes. */ const sendMessageBodySchema = z.object({ - content: z.string().min(1), - conversationId: z.string().optional(), + content: z.string().min(1).max(32768), + conversationId: z.string().max(128).optional(), }); type SendMessageBody = z.infer; diff --git a/packages/appkit/src/plugins/genie/tests/genie.test.ts b/packages/appkit/src/plugins/genie/tests/genie.test.ts index a26859f33..88105a9fa 100644 --- a/packages/appkit/src/plugins/genie/tests/genie.test.ts +++ b/packages/appkit/src/plugins/genie/tests/genie.test.ts @@ -307,6 +307,35 @@ describe("Genie Plugin", () => { }), ); }); + + test("should return 400 when content exceeds 32768 chars", async () => { + const plugin = new GeniePlugin(config); + const { router, getHandler } = createMockRouter(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/messages"); + const mockReq = createMockRequest({ + params: { alias: "myspace" }, + body: { content: "a".repeat(32769) }, + headers: { + "x-forwarded-access-token": "user-token", + "x-forwarded-user": "user-1", + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(mockGenieService.startConversation).not.toHaveBeenCalled(); + }); }); describe("send message - new conversation", () => { diff --git a/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts b/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts index 5f20190f3..6b659356e 100644 --- a/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts +++ b/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts @@ -428,6 +428,55 @@ describe("VectorSearchPlugin", () => { }); }); + describe("POST /:alias/query size caps", () => { + it("rejects queryVector longer than 4096", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/query"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: { queryVector: new Array(4097).fill(0) }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(mockRequest).not.toHaveBeenCalled(); + }); + + it("rejects columns array longer than 100", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/query"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: { + queryText: "hello", + columns: Array.from({ length: 101 }, (_, i) => `col${i}`), + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRequest).not.toHaveBeenCalled(); + }); + }); + describe("POST /:alias/next-page", () => { it("returns canonical 400 when pageToken is missing", async () => { const plugin = buildPlugin(); @@ -505,6 +554,31 @@ describe("VectorSearchPlugin", () => { const callBody = mockRequest.mock.calls[0][0].payload; expect(callBody.page_token).toBe("token-abc"); }); + + it("rejects pageToken longer than 4096 chars", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/next-page"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: { pageToken: "x".repeat(4097) }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(mockRequest).not.toHaveBeenCalled(); + }); }); }); }); diff --git a/packages/appkit/src/plugins/vector-search/vector-search.ts b/packages/appkit/src/plugins/vector-search/vector-search.ts index 4e75423a6..bfd551991 100644 --- a/packages/appkit/src/plugins/vector-search/vector-search.ts +++ b/packages/appkit/src/plugins/vector-search/vector-search.ts @@ -28,22 +28,26 @@ import type { * refinement surfaces its issue on the `queryText` path so clients get a * single, stable location to render the error. */ -const searchFiltersSchema = z.record( - z.string(), - z.union([ +const searchFiltersSchema = z + .record( z.string(), - z.number(), - z.boolean(), - z.array(z.union([z.string(), z.number()])), - ]), -); + z.union([ + z.string(), + z.number(), + z.boolean(), + z.array(z.union([z.string(), z.number()])), + ]), + ) + .refine((obj) => Object.keys(obj).length <= 50, { + message: "filters may not contain more than 50 keys", + }); const queryBodySchema = z .object({ - queryText: z.string().optional(), - queryVector: z.array(z.number()).optional(), - columns: z.array(z.string()).optional(), - numResults: z.number().optional(), + queryText: z.string().max(10000).optional(), + queryVector: z.array(z.number()).max(4096).optional(), + columns: z.array(z.string().max(255)).max(100).optional(), + numResults: z.number().int().min(1).max(1000).optional(), queryType: z.enum(["ann", "hybrid", "full_text"]).optional(), filters: searchFiltersSchema.optional(), reranker: z.boolean().optional(), @@ -56,7 +60,7 @@ const queryBodySchema = z type QueryBody = z.infer; const nextPageBodySchema = z.object({ - pageToken: z.string().min(1), + pageToken: z.string().min(1).max(4096), }); type NextPageBody = z.infer; From 2aa8505281daf024a4ea77d0b199d8f1c499f91e Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 17 Apr 2026 17:16:22 +0200 Subject: [PATCH 07/14] refactor(appkit): overload route() to derive TBody from body schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two overloads on Plugin.route(): - When `body: TSchema` is present, TBody is inferred from StandardSchemaV1.InferOutput. Plugin authors no longer write `` explicitly — the compiler derives it from the schema. - When `body` is absent, TBody is `unknown`. This closes the type hole where a plugin could declare `this.route<{ foo: string }>({ body: z.object({ bar: z.number() }) })` and have the declared TBody silently diverge from the schema's actual output type. Runtime was always safe; now the compile-time types are too. Removes the now-redundant explicit `` at all four migrated plugin call sites (genie, analytics, vector-search query + next-page, files/mkdir). The RouteConfig default stays as-is — it's load-bearing for backward compat with handlers typed as plain express.Request. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- docs/docs/api/appkit/Class.Plugin.md | 77 +++++++++++++++-- packages/appkit/src/plugin/plugin.ts | 37 +++++++- .../src/plugin/tests/body-validation.test.ts | 85 +++++++++++++------ .../appkit/src/plugins/analytics/analytics.ts | 2 +- packages/appkit/src/plugins/files/plugin.ts | 2 +- packages/appkit/src/plugins/genie/genie.ts | 2 +- .../plugins/vector-search/vector-search.ts | 4 +- 7 files changed, 170 insertions(+), 39 deletions(-) diff --git a/docs/docs/api/appkit/Class.Plugin.md b/docs/docs/api/appkit/Class.Plugin.md index cb5eda630..41e065164 100644 --- a/docs/docs/api/appkit/Class.Plugin.md +++ b/docs/docs/api/appkit/Class.Plugin.md @@ -527,24 +527,87 @@ AuthenticationError in production when no user header is present. ### route() +#### Call Signature + ```ts -protected route(router: Router, config: RouteConfig): void; +protected route(router: Router, config: RouteConfig> & { + body: TSchema; +}): void; ``` -#### Type Parameters +Bind a handler to a router path with optional `req.body` validation. -| Type Parameter | Default type | +Overloads exist because `RouteConfig` carries two independent +uses of `TBody` — `body: StandardSchemaV1` and +`handler: (req: IAppRequestWithBody, …)`. Without overloads, +plugin authors can pass a schema whose output diverges from the +declared handler body type; the compiler stays quiet and runtime +narrowing silently disagrees. The overloads below tie `TBody` to the +schema's `InferOutput` when `body` is present, so the handler always +sees the schema's real output type. When `body` is absent, `TBody` +resolves to `unknown` so handlers must narrow before use. + +Note: `RouteConfig` default is load-bearing for backward +compat with handlers typed as plain `express.Request`. DO NOT "fix" +it to `unknown` — cascades into mass typecheck breakage. + +If you're confused why this needs overloads: TypeScript cannot otherwise +enforce that `TBody` equals `StandardSchemaV1.InferOutput` +when both are separate type parameters on the same function. + +##### Type Parameters + +| Type Parameter | +| ------ | +| `TSchema` *extends* `StandardSchemaV1`\<`unknown`, `any`\> | + +##### Parameters + +| Parameter | Type | | ------ | ------ | -| `TBody` | `unknown` | +| `router` | `Router` | +| `config` | `RouteConfig`\<`InferOutput`\<`TSchema`\>\> & \{ `body`: `TSchema`; \} | -#### Parameters +##### Returns + +`void` + +#### Call Signature + +```ts +protected route(router: Router, config: Omit, "body"> & { + body?: undefined; +}): void; +``` + +Bind a handler to a router path with optional `req.body` validation. + +Overloads exist because `RouteConfig` carries two independent +uses of `TBody` — `body: StandardSchemaV1` and +`handler: (req: IAppRequestWithBody, …)`. Without overloads, +plugin authors can pass a schema whose output diverges from the +declared handler body type; the compiler stays quiet and runtime +narrowing silently disagrees. The overloads below tie `TBody` to the +schema's `InferOutput` when `body` is present, so the handler always +sees the schema's real output type. When `body` is absent, `TBody` +resolves to `unknown` so handlers must narrow before use. + +Note: `RouteConfig` default is load-bearing for backward +compat with handlers typed as plain `express.Request`. DO NOT "fix" +it to `unknown` — cascades into mass typecheck breakage. + +If you're confused why this needs overloads: TypeScript cannot otherwise +enforce that `TBody` equals `StandardSchemaV1.InferOutput` +when both are separate type parameters on the same function. + +##### Parameters | Parameter | Type | | ------ | ------ | | `router` | `Router` | -| `config` | `RouteConfig`\<`TBody`\> | +| `config` | `Omit`\<`RouteConfig`\<`unknown`\>, `"body"`\> & \{ `body?`: `undefined`; \} | -#### Returns +##### Returns `void` diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 8014abecb..5a0248431 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -563,9 +563,42 @@ export abstract class Plugin< this.registeredEndpoints[name] = path; } - protected route( + /** + * Bind a handler to a router path with optional `req.body` validation. + * + * Overloads exist because `RouteConfig` carries two independent + * uses of `TBody` — `body: StandardSchemaV1` and + * `handler: (req: IAppRequestWithBody, …)`. Without overloads, + * plugin authors can pass a schema whose output diverges from the + * declared handler body type; the compiler stays quiet and runtime + * narrowing silently disagrees. The overloads below tie `TBody` to the + * schema's `InferOutput` when `body` is present, so the handler always + * sees the schema's real output type. When `body` is absent, `TBody` + * resolves to `unknown` so handlers must narrow before use. + * + * Note: `RouteConfig` default is load-bearing for backward + * compat with handlers typed as plain `express.Request`. DO NOT "fix" + * it to `unknown` — cascades into mass typecheck breakage. + * + * If you're confused why this needs overloads: TypeScript cannot otherwise + * enforce that `TBody` equals `StandardSchemaV1.InferOutput` + * when both are separate type parameters on the same function. + */ + protected route>( + router: express.Router, + config: RouteConfig> & { + body: TSchema; + }, + ): void; + protected route( + router: express.Router, + config: Omit, "body"> & { body?: undefined }, + ): void; + protected route( router: express.Router, - config: RouteConfig, + config: + | (RouteConfig & { body: StandardSchemaV1 }) + | (Omit, "body"> & { body?: undefined }), ): void { const { name, method, path, handler } = config; diff --git a/packages/appkit/src/plugin/tests/body-validation.test.ts b/packages/appkit/src/plugin/tests/body-validation.test.ts index 5a1ea5944..b334a9fed 100644 --- a/packages/appkit/src/plugin/tests/body-validation.test.ts +++ b/packages/appkit/src/plugin/tests/body-validation.test.ts @@ -7,7 +7,7 @@ import { mockServiceContext, } from "@tools/test-helpers"; import type express from "express"; -import type { BasePluginConfig } from "shared"; +import type { BasePluginConfig, RouteConfig } from "shared"; import { afterEach, assertType, @@ -56,13 +56,27 @@ vi.mock("../../logging/logger", () => ({ })); class TestPlugin extends Plugin { - // Expose protected route() for testing. - public exposedRoute( + // Expose protected route() for testing. Overload signatures mirror the + // real `route()` so callers get the same TBody inference rules as + // plugin authors see in production. + public exposedRoute>( router: express.Router, - config: Parameters>[1], - ) { + config: RouteConfig> & { + body: TSchema; + }, + ): void; + public exposedRoute( + router: express.Router, + config: Omit, "body"> & { body?: undefined }, + ): void; + public exposedRoute( + router: express.Router, + config: + | (RouteConfig & { body: StandardSchemaV1 }) + | (Omit, "body"> & { body?: undefined }), + ): void { // biome-ignore lint/complexity/useLiteralKeys: calling protected member from subclass - return this["route"](router, config); + this["route"](router, config as any); } } @@ -114,7 +128,7 @@ describe("route body validation", () => { res.status(200).json({ ok: true }); }); - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -144,7 +158,7 @@ describe("route body validation", () => { const schema = z.object({ content: z.string().min(1) }); const handlerSpy = vi.fn(); - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -183,7 +197,7 @@ describe("route body validation", () => { const schema = z.object({ content: z.string().min(1) }); const handlerSpy = vi.fn(); - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -216,7 +230,7 @@ describe("route body validation", () => { const schema = z.object({ content: z.string().min(1) }); const handlerSpy = vi.fn(); - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -248,7 +262,7 @@ describe("route body validation", () => { const { router, getHandler } = createMockRouter(); const schema = z.object({ content: z.string().min(1) }); - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -324,7 +338,7 @@ describe("route body validation", () => { res.status(200).json({ ok: true }); }); - plugin.exposedRoute<{ content: string }>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -345,8 +359,11 @@ describe("route body validation", () => { expect(badRes.status).toHaveBeenCalledWith(400); }); - // Compile-time check: handler's req.body should be typed as the schema's - // output. If this block type-checks, the generic is threading through. + // Compile-time checks: the overload on `route()` must derive `TBody` + // from `body`'s schema output when present, and fall back to `unknown` + // when absent. Plugin authors should never need to write `` + // explicitly — if these blocks type-check without explicit generics, + // the overload is doing its job. test("type-level: handler req.body is narrowed to schema output", () => { const plugin = createTestPlugin(); const { router } = createMockRouter(); @@ -355,21 +372,39 @@ describe("route body validation", () => { content: z.string().min(1), conversationId: z.string().optional(), }); - type Body = z.infer; - plugin.exposedRoute(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", body: schema, handler: async (req, _res) => { - // These assertions fail at compile time if generics regress. + // These assertions fail at compile time if the overload regresses. assertType(req.body.content); assertType(req.body.conversationId); }, }); }); + test("type-level: handler req.body defaults to unknown when body is absent", () => { + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + + plugin.exposedRoute(router, { + name: "noBody", + method: "post", + path: "/no-body", + handler: async (req, _res) => { + // Without a schema, `req.body` has no compile-time shape. Reading + // any property off `unknown` is a compile error, so this @ts-expect-error + // proves the overload falls back to `unknown` (not `any`). + // @ts-expect-error unknown is not indexable + req.body.anyProperty; + assertType(req.body); + }, + }); + }); + test("returns canonical 500 when validator throws synchronously", async () => { process.env.NODE_ENV = "development"; const plugin = createTestPlugin(); @@ -384,7 +419,7 @@ describe("route body validation", () => { const handlerSpy = vi.fn(); - plugin.exposedRoute<{ content: string }>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -428,7 +463,7 @@ describe("route body validation", () => { const handlerSpy = vi.fn(); - plugin.exposedRoute<{ ok: true }>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -459,7 +494,7 @@ describe("route body validation", () => { const { router, getHandler } = createMockRouter(); const schema = z.object({ content: z.string().min(1) }); - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -494,7 +529,7 @@ describe("route body validation", () => { const { router, getHandler } = createMockRouter(); const schema = z.object({ content: z.string().min(1) }); - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "sendMessage", method: "post", path: "/messages", @@ -538,7 +573,7 @@ describe("route body validation", () => { const handlerSpy = vi.fn(); - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "many", method: "post", path: "/many", @@ -577,7 +612,7 @@ describe("route body validation", () => { }, }; - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "exact", method: "post", path: "/exact", @@ -614,7 +649,7 @@ describe("route body validation", () => { }, }; - plugin.exposedRoute>(router, { + plugin.exposedRoute(router, { name: "many", method: "post", path: "/many", @@ -639,7 +674,7 @@ describe("route body validation", () => { const { router } = createMockRouter(); expect(() => - plugin.exposedRoute(router, { + plugin.exposedRoute(router, { name: "bad", method: "post", path: "/bad", diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index 534486a62..55ceae323 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -73,7 +73,7 @@ export class AnalyticsPlugin extends Plugin { }, }); - this.route(router, { + this.route(router, { name: "query", method: "post", path: "/query/:query_key", diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 4618fbd46..4e9498629 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -329,7 +329,7 @@ export class FilesPlugin extends Plugin { }, }); - this.route(router, { + this.route(router, { name: "mkdir", method: "post", path: "/:volumeKey/mkdir", diff --git a/packages/appkit/src/plugins/genie/genie.ts b/packages/appkit/src/plugins/genie/genie.ts index 9010ace0e..9a33463de 100644 --- a/packages/appkit/src/plugins/genie/genie.ts +++ b/packages/appkit/src/plugins/genie/genie.ts @@ -64,7 +64,7 @@ export class GeniePlugin extends Plugin { } injectRoutes(router: IAppRouter) { - this.route(router, { + this.route(router, { name: "sendMessage", method: "post", path: "/:alias/messages", diff --git a/packages/appkit/src/plugins/vector-search/vector-search.ts b/packages/appkit/src/plugins/vector-search/vector-search.ts index bfd551991..183851070 100644 --- a/packages/appkit/src/plugins/vector-search/vector-search.ts +++ b/packages/appkit/src/plugins/vector-search/vector-search.ts @@ -117,7 +117,7 @@ export class VectorSearchPlugin extends Plugin { } injectRoutes(router: IAppRouter) { - this.route(router, { + this.route(router, { name: "query", method: "post", path: "/:alias/query", @@ -176,7 +176,7 @@ export class VectorSearchPlugin extends Plugin { }, }); - this.route(router, { + this.route(router, { name: "queryNextPage", method: "post", path: "/:alias/next-page", From d20c4f45fafe2bd957a9ff1241b89f9ba4736515 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Mon, 20 Apr 2026 16:42:47 +0200 Subject: [PATCH 08/14] feat: skills for creating, reviewing, and auditing an AppKit plugin (#257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs: add plugin best-practices reference guide Extracts patterns from the 5 core plugins (analytics, genie, files, lakebase, server) into a structured reference with 9 sections and three severity tiers (NEVER/MUST/SHOULD). Signed-off-by: Atila Fassina * docs: integrate best-practices reference into create-core-plugin skill Adds a "Load Best Practices Reference" step before scaffolding decisions so guidelines are applied during plugin creation. Signed-off-by: Atila Fassina * fix: correct inaccuracies in plugin best-practices reference Fixes 3 issues found during dry-run validation against analytics and files plugins: - Plugin extends Plugin (not Plugin) - @internal goes on toPlugin export, not the class - isInUserContext() patterns clarified per actual usage Signed-off-by: Atila Fassina * docs: add review-core-plugin and audit-core-plugin skills Signed-off-by: Atila Fassina * fix: address dry-run findings in audit and review skills - Add cacheKey two-stage pattern guidance to prevent false positives - Add N/A status option for non-applicable scorecard categories - Clarify connector scope in structural completeness check - Add deduplication guidance for cross-category findings - Add recovery hint to plugin-not-found error message Signed-off-by: Atila Fassina * fix: address review feedback on plugin skills - Remove trailing slash from route prefix example in best-practices - Use deterministic directory-existence check instead of name-pattern heuristic for plugin vs branch disambiguation in review-core-plugin - Downgrade defaults.ts from MUST to SHOULD in audit-core-plugin since not all plugins (e.g. server, lakebase) require it Co-authored-by: Isaac Signed-off-by: Atila Fassina * chore: address code review * refactor: extract shared plugin review guidance to reduce duplication Move category index, severity ordering, deduplication rules, and cache-key tracing instructions into a shared reference file so audit and review skills stay in sync from a single source of truth. Co-authored-by: Isaac Signed-off-by: Atila Fassina * docs: clarify getResourceRequirements patterns in plugin best practices Split the rule into config-gated flip (optional→required) and dynamic discovery (required placeholder + dynamic emission) variants. Reference the custom-plugins.md caching example for the flip pattern and FilesPlugin/ServingPlugin for the discovery pattern. Co-authored-by: Isaac Signed-off-by: Atila Fassina * docs: clarify scorecard numbering in audit-core-plugin Add a note that category 0 (Structural Completeness) is a pre-check with no matching section in plugin-best-practices.md, while 1–9 mirror the best-practices sections. Co-authored-by: Isaac Signed-off-by: Atila Fassina --------- Signed-off-by: Atila Fassina --- .claude/commands/audit-core-plugin.md | 143 ++++++++++++++ .claude/commands/create-core-plugin.md | 32 ++-- .claude/commands/review-core-plugin.md | 125 ++++++++++++ .claude/references/plugin-best-practices.md | 188 +++++++++++++++++++ .claude/references/plugin-review-guidance.md | 40 ++++ 5 files changed, 512 insertions(+), 16 deletions(-) create mode 100644 .claude/commands/audit-core-plugin.md create mode 100644 .claude/commands/review-core-plugin.md create mode 100644 .claude/references/plugin-best-practices.md create mode 100644 .claude/references/plugin-review-guidance.md diff --git a/.claude/commands/audit-core-plugin.md b/.claude/commands/audit-core-plugin.md new file mode 100644 index 000000000..4fd1027be --- /dev/null +++ b/.claude/commands/audit-core-plugin.md @@ -0,0 +1,143 @@ +--- +description: Full audit of a core plugin against all best-practices categories with scorecard +argument-hint: +--- + +# Audit Core Plugin + +Perform a full audit of the named plugin against all AppKit plugin best-practices categories and produce a scorecard. + +**Plugin name:** $ARGUMENTS + +## Step 1: Validate Input + +If `$ARGUMENTS` is empty or missing, stop and output: + +> Usage: /audit-core-plugin + +Otherwise, set `PLUGIN_NAME` to the value of `$ARGUMENTS` (trimmed, kebab-case). + +Check whether the plugin directory exists at either of these paths: + +- `packages/appkit/src/plugins/{PLUGIN_NAME}/` +- `packages/appkit/src/connectors/{PLUGIN_NAME}/` + +If **neither** path exists, stop and output: + +> Error: Plugin "{PLUGIN_NAME}" not found. Checked: +> - `packages/appkit/src/plugins/{PLUGIN_NAME}/` +> - `packages/appkit/src/connectors/{PLUGIN_NAME}/` +> +> Available plugins can be listed with: `ls packages/appkit/src/plugins/` + +If at least one path exists, proceed. + +## Step 2: Load Best Practices Reference + +Read the full contents of: + +``` +.claude/references/plugin-best-practices.md +``` + +This defines the 9 audit categories and their NEVER/MUST/SHOULD guidelines. You will evaluate the plugin against every guideline in every category. + +## Step 3: File Discovery + +Read **all** files under: + +- `packages/appkit/src/plugins/{PLUGIN_NAME}/` (recursively, including `tests/` subdirectory) +- `packages/appkit/src/connectors/{PLUGIN_NAME}/` (recursively, if this directory exists) + +Collect the full contents of every file. You need the complete source to evaluate all 9 categories. + +## Step 4: Structural Completeness Check + +If `packages/appkit/src/plugins/{PLUGIN_NAME}/` does not exist (connector-only package), mark Structural Completeness as **N/A** in the scorecard and proceed to Step 5. + +Otherwise, verify the following expected files exist inside `packages/appkit/src/plugins/{PLUGIN_NAME}/`: + +| Expected file | Required? | +|---|---| +| `manifest.json` | MUST | +| Main plugin class file (any `.ts` file containing a class extending `Plugin`) | MUST | +| `types.ts` | MUST | +| `defaults.ts` | SHOULD | +| `index.ts` | MUST | +| `tests/` directory with at least one `.test.ts` file | MUST | + +Treat each missing `MUST` file as a **MUST**-severity finding under the "Structural Completeness" category. Treat a missing `SHOULD` file as a **SHOULD**-severity finding. + +`defaults.ts` is not universally required for every plugin. It should be present when the plugin exposes execution settings or defines behavior that depends on `execute()` / `executeStream()` defaults, but its absence alone should not be reported as a MUST failure for plugins that do not use those defaults. + +> **Note:** The structural completeness check applies only to the `plugins/{PLUGIN_NAME}/` directory. Connector directories (`connectors/{PLUGIN_NAME}/`) serve a different architectural role and are read as supporting context for the best-practices review, not audited for structural completeness. + +## Step 5: Full Best-Practices Review + +Before evaluating, read the shared review rules in `.claude/references/plugin-review-guidance.md` and apply them throughout this step (deduplication, cache-key tracing). + +Evaluate the plugin code against **all 9 categories** from the Category Index in `plugin-review-guidance.md`. Check each category's NEVER/MUST/SHOULD rules from the best-practices reference. + +For each guideline in each category, determine whether the plugin **passes**, **violates**, or is **not applicable** (e.g., SSE rules for a non-streaming plugin). Record findings with: + +- **Severity**: NEVER, MUST, or SHOULD (from the guideline prefix) +- **Category**: Which of the 9 categories +- **Description**: What the guideline requires and how the plugin violates it +- **Location**: Specific `file:line` reference(s) + +A category with no findings is a pass. A category with only SHOULD findings is a warn. A category with any MUST or NEVER finding is a fail. + +## Step 6: Produce Output + +### Scorecard Table (output first) + +``` +## Scorecard + +| # | Category | Status | Findings | +|---|----------|--------|----------| +| 0 | Structural Completeness | {status} | {count} | +| 1 | Manifest Design | {status} | {count} | +| 2 | Plugin Class Structure | {status} | {count} | +| 3 | Route Design | {status} | {count} | +| 4 | Interceptor Usage | {status} | {count} | +| 5 | asUser / OBO Patterns | {status} | {count} | +| 6 | Client Config | {status} | {count} | +| 7 | SSE Streaming | {status} | {count} | +| 8 | Testing Expectations | {status} | {count} | +| 9 | Type Safety | {status} | {count} | +``` + +> Category 0 (Structural Completeness) is a file-layout pre-check from Step 4 and has no corresponding section in `plugin-best-practices.md`. Categories 1–9 mirror sections 1–9 of the best-practices reference. + +Where `{status}` is one of: +- Pass — no findings +- Warn — SHOULD-only findings +- Fail — any NEVER or MUST findings +- N/A — category does not apply to this plugin (e.g., SSE Streaming for a non-streaming plugin) + +And `{count}` is the number of findings (0 if pass). + +### Detailed Findings (output second, severity-first) + +Group all findings across all categories and sort by severity per the Severity Ordering rule in `plugin-review-guidance.md`. + +For each finding, output: + +``` +### [{severity}] {category}: {short description} + +**File:** `{file_path}:{line_number}` + +{Explanation of what the guideline requires, what the code does wrong, and how to fix it.} +``` + +If there are zero findings across all categories, output: + +> All checks passed. No findings. + +### Summary (output last) + +End with a one-line summary: + +> **Audit result: {total_findings} findings ({never_count} NEVER, {must_count} MUST, {should_count} SHOULD) across {failing_categories} failing and {warning_categories} warning categories.** diff --git a/.claude/commands/create-core-plugin.md b/.claude/commands/create-core-plugin.md index bb2344717..a72c6e194 100644 --- a/.claude/commands/create-core-plugin.md +++ b/.claude/commands/create-core-plugin.md @@ -2,6 +2,16 @@ User input: $ARGUMENTS +## 0. Load Best Practices Reference + +Before making any scaffolding decisions, read the plugin best-practices reference: + +``` +.claude/references/plugin-best-practices.md +``` + +This document defines NEVER/MUST/SHOULD guidelines for manifest design, plugin class structure, route design, interceptor usage, asUser/OBO patterns, client config, SSE streaming, testing, and type safety. Apply these guidelines throughout the scaffolding process — especially when deciding interceptor defaults, cache key scoping, OBO enforcement, and route registration patterns. + ## 1. Gather Requirements The user may have provided a plugin name and/or description above in `$ARGUMENTS`. Parse what was given. @@ -65,6 +75,8 @@ connectors/{name}/ ## 4. Plugin Patterns to Follow +> The scaffolding templates below implement the guidelines from `.claude/references/plugin-best-practices.md`. Refer to that document for the full NEVER/MUST/SHOULD rules and rationale behind each pattern. + ### 4a. Config Interface (`types.ts`) Extend `BasePluginConfig` from `shared`: @@ -166,7 +178,7 @@ const logger = createLogger("{name}"); export class {PascalName}Plugin extends Plugin { name = "{name}"; - static manifest = manifest as PluginManifest; + static manifest = manifest as PluginManifest<"{name}">; protected declare config: I{PascalName}Config; private connector: {PascalName}Connector; @@ -320,15 +332,7 @@ export * from "./{name}"; ## 6. Key Conventions -### Route Registration -Routes mount at `/api/{pluginName}/...`. Use `this.route(router, { name, method, path, handler })`. - -### User-Scoped Execution (OBO) -`this.asUser(req)` returns a proxy where all method calls use the user's Databricks credentials. Use it in route handlers for user-scoped operations. Inside the proxied method, `getWorkspaceClient()` automatically returns the user-scoped client. **Important:** any plugin using `asUser()` must declare the appropriate `user_api_scopes` in the bundle config — see section 4f. - -### Execution Methods -- `this.execute(fn, options)` — single request-response with interceptors (telemetry, timeout, retry, cache) -- `this.executeStream(res, fn, options)` — SSE streaming with interceptors +For full NEVER/MUST/SHOULD rules on routes, interceptors, OBO, streaming, and type safety, see `.claude/references/plugin-best-practices.md`. ### Context Utilities Import from `../../context`: @@ -339,12 +343,8 @@ Import from `../../context`: ### Logging Use `createLogger("plugin-name")` from `../../logging/logger`. -### Setup/Teardown -- Override `setup()` for async init (e.g., connection pools). Called by AppKit during startup. -- Override `abortActiveOperations()` for cleanup. Call `super.abortActiveOperations()` first. - -### Plugin Phases -Set `static phase: PluginPhase` if needed: `"core"` (first), `"normal"` (default), `"deferred"` (last). +### OBO Scopes Reminder +Any plugin using `this.asUser(req)` must declare `user_api_scopes` in the bundle config — see section 4f. ## 7. After Scaffolding diff --git a/.claude/commands/review-core-plugin.md b/.claude/commands/review-core-plugin.md new file mode 100644 index 000000000..6db26038c --- /dev/null +++ b/.claude/commands/review-core-plugin.md @@ -0,0 +1,125 @@ +--- +description: Review plugin changes against AppKit best practices (composes with review-pr) +argument-hint: [plugin-name or base-branch] +--- + +# Review Core Plugin Changes + +User input: $ARGUMENTS + +## Step 0: Parse Input + +Parse `$ARGUMENTS` deterministically: + +- If `$ARGUMENTS` is empty: + - Use no plugin name filter. + - Use `origin/main` as the base branch. +- Otherwise, check whether either of these paths exists: + - `packages/appkit/src/plugins/$ARGUMENTS` + - `packages/appkit/src/connectors/$ARGUMENTS` +- If either path exists: + - Treat `$ARGUMENTS` as the **plugin name filter**. + - Use `origin/main` as the base branch. +- Otherwise: + - Treat `$ARGUMENTS` as the **base branch**. + - Use no plugin name filter. + +Do not use a name-pattern heuristic such as "kebab-case with no slashes" to decide whether `$ARGUMENTS` is a plugin name, because common branch names like `feature-x` and `bugfix-foo` would be ambiguous. + +## Step 1: Core Principles Review + +First, invoke the `review-pr` skill to run the standard Core Principles review. Pass the base branch as the argument (not the plugin name). + +Use the Skill tool: +- skill: `review-pr` +- args: the base branch determined in Step 0 (or empty to use default) + +Wait for this review to complete before continuing. + +## Step 2: Diff Analysis + +Run `git diff ...HEAD --name-only` to get all changed files. + +Filter the file list to plugin-relevant paths: +- `packages/appkit/src/plugins/**` +- `packages/appkit/src/connectors/**` + +If a specific plugin name was provided in Step 0, further filter to only files matching that plugin name in the path. + +**If no plugin files are found in the diff**, output: + +> No plugin files were changed in this branch. Plugin best-practices review is not applicable. Only the Core Principles review above applies. + +Then stop. Do not continue to subsequent steps. + +## Step 3: Multi-Plugin Detection + +If no specific plugin name was provided, detect all distinct plugins touched in the diff by extracting the plugin directory name from each changed path: +- From `packages/appkit/src/plugins/{name}/...` extract `{name}` +- From `packages/appkit/src/connectors/{name}/...` extract `{name}` + +Deduplicate the list. You will run Steps 4-6 for **each** detected plugin. + +## Step 4: Category Scoping + +For each plugin being reviewed, use the Category Index from `.claude/references/plugin-review-guidance.md` as the canonical list of categories. Map changed files to relevant categories using the "What to check" column as a guide — match file names and code patterns (e.g., `this.route(` → Route Design, `asUser(` → asUser / OBO Patterns). A single file may trigger multiple categories. + +Read the actual changed file contents with `git diff ...HEAD -- ` to determine which patterns are present. + +Record which of the 9 categories are **relevant** (at least one changed file maps to them) and which are **skipped** (no changed files map to them). + +## Step 5: Load Best Practices Reference + +Read the file `.claude/references/plugin-best-practices.md`. + +For each **relevant** category identified in Step 4, extract all NEVER, MUST, and SHOULD guidelines from that category section. + +## Step 6: Best-Practices Review + +Before evaluating, read the shared review rules in `.claude/references/plugin-review-guidance.md` and apply them throughout this step (deduplication, cache-key tracing). + +For each plugin detected in Step 3, review the changed code against the scoped guidelines from Step 5. + +For each finding: +- Identify the **severity** (NEVER, MUST, or SHOULD) +- Identify the **category** (e.g., "Manifest Design", "Route Design") +- Cite the specific guideline being violated or satisfied +- Reference the exact file and line(s) involved +- Provide a concrete fix if it is a violation + +## Step 7: Output + +### Format + +For each plugin reviewed, output a section with the plugin name as the heading. + +Order findings by severity per the Severity Ordering rule in `plugin-review-guidance.md`. + +Each finding should follow this format: + +``` +### [SEVERITY] Category Name: Brief description +- **File:** `path/to/file.ts:L42` +- **Guideline:** +- **Finding:** +- **Fix:** +``` + +If a plugin has **no findings** (all scoped guidelines are satisfied), state that explicitly. + +### Skipped Categories + +At the end of the output (after all plugin reviews), list the categories that were **not relevant** to this diff: + +``` +### Skipped Categories (not relevant to this diff) +- Category N: — no changed files matched this category +- ... +``` + +### Summary + +End with an overall summary: +- Total findings by severity (e.g., "0 NEVER, 2 MUST, 3 SHOULD") +- Whether the changes are ready to merge from a plugin best-practices perspective +- Any categories that deserve attention even though they were skipped (e.g., "No tests were changed — consider adding tests for the new route") diff --git a/.claude/references/plugin-best-practices.md b/.claude/references/plugin-best-practices.md new file mode 100644 index 000000000..6b6c494ab --- /dev/null +++ b/.claude/references/plugin-best-practices.md @@ -0,0 +1,188 @@ +# Plugin Best Practices + +Reference guide for building AppKit plugins. Every guideline is prefixed with a severity tier: + +- **NEVER** — Security or breakage blocker. Violating this will cause data leaks, crashes, or silent corruption. +- **MUST** — Correctness requirement. Violating this produces bugs, broken APIs, or inconsistent behavior. +- **SHOULD** — Quality recommendation. Violating this degrades DX, performance, or maintainability. + +> **Scope:** These guidelines target **core plugins within the AppKit monorepo** (`packages/appkit/src/plugins/`). Custom plugins built outside the monorepo have more flexibility — see `docs/docs/plugins/custom-plugins.md` for lighter-weight patterns (inline manifests, camelCase names, etc.). + +--- + +## 1. Manifest Design + +**MUST** include all four required top-level fields: `name`, `displayName`, `description`, `resources`. + +**MUST** use lowercase kebab-case for `name` (pattern: `^[a-z][a-z0-9-]*$`). This becomes the route prefix (`/api/{name}`) and the key on the AppKit instance. (This is a monorepo convention; custom plugins may use camelCase per the official docs.) + +**MUST** declare both `resources.required` and `resources.optional` arrays, even if empty. + +**MUST** give every resource a unique `resourceKey` (lowercase kebab-case). The `alias` is for display only; `resourceKey` drives deduplication, env naming, and bundle config. + +**MUST** use the correct permission enum per resource type (e.g. `CAN_USE` for `sql_warehouse`, `WRITE_VOLUME` for `volume`). The schema validates this with `allOf`/`if-then` rules. + +**SHOULD** add `fields` with `env` entries so that `appkit plugin sync` and `appkit init` can auto-generate `.env` templates and `app.yaml` resource blocks. + +**SHOULD** set `hidden: true` on infrastructure plugins (like `server`) that should not appear in the template manifest. + +**MUST** use `getResourceRequirements(config)` for resources that depend on runtime config. Two variants: + +- **Config-gated flip** — when a resource is only needed if a config flag is set: declare it as `optional` in the manifest (so CLI and docs can see it) and return it with `required: true` from the static method when the flag is on. See the "Config-dependent resources" example in `docs/docs/plugins/custom-plugins.md`. +- **Dynamic discovery** — when concrete resources can't be enumerated statically (one per env var, one per config entry, etc.): keep a single required placeholder in the manifest so `apps init` can prompt for at least one, and emit the full dynamic set from `getResourceRequirements`. See `FilesPlugin` and `ServingPlugin`. + +--- + +## 2. Plugin Class Structure + +**MUST** extend `Plugin` (the base class accepts an optional generic but core plugins use the plain form). + +**MUST** declare a static `manifest` property. Core plugins import `manifest.json` as a JSON module, which requires `as` (JSON imports cannot use `satisfies`): + +```ts +static manifest = manifest as PluginManifest<"my-plugin">; +``` + +> **Note:** The `<"my-plugin">` generic is a **SHOULD** (see Section 9: Type Safety). Inline manifests — as shown in the custom-plugins docs — should use `satisfies PluginManifest<"name">` instead, which is stricter and catches structural errors at compile time. + +**MUST** export a `toPlugin`-wrapped factory as the public API. Mark the factory (not the class) as `@internal`: + +```ts +export class MyPlugin extends Plugin { ... } + +/** @internal */ +export const myPlugin = toPlugin(MyPlugin); +``` + +**MUST** re-declare config with `protected declare config: IMyConfig` if extending the base config type. Call `super(config)` first, then assign `this.config = config`. + +**SHOULD** keep the barrel `index.ts` minimal: re-export the plugin factory and types only. + +**SHOULD** use `static phase: PluginPhase` only when initialization order matters. Use `"core"` for config-only plugins, `"deferred"` for server (starts last). Default is `"normal"`. + +**MUST** implement `shutdown()` and call `this.streamManager.abortAll()` if the plugin uses streaming or long-lived connections. + +--- + +## 3. Route Design + +**MUST** register routes via `this.route(router, config)` inside `injectRoutes()`. This auto-registers endpoints under `/api/{pluginName}{path}` and tracks them for the client config endpoint map. + +**MUST** include a `name` in every route config. This becomes the key in `getEndpoints()` and is used by the frontend to discover URLs. + +**NEVER** register routes directly on `router.get(...)` without going through `this.route()` — the endpoint will be invisible to the server plugin's endpoint map and client config. + +**MUST** set `skipBodyParsing: true` on routes that receive raw streams (e.g. file uploads). The server plugin uses this to bypass `express.json()` for those paths. + +**SHOULD** use RESTful conventions: `GET` for reads, `POST` for mutations and queries with bodies, `DELETE` for deletions. + +**SHOULD** validate required params early and return `400` before doing any work. + +--- + +## 4. Interceptor Usage + +**SHOULD** define execution defaults in a separate `defaults.ts` file as `PluginExecuteConfig` constants. This keeps route handlers clean and makes defaults testable. (Required when the plugin uses `execute()` or `executeStream()` with non-trivial settings; not needed for plugins that don't use execution interceptors.) + +**MUST** pass defaults via the `default` key in `PluginExecutionSettings`. User overrides go in `user`. The merge order is: method defaults <- plugin config <- user override. + +**NEVER** enable cache without providing a `cacheKey` array. The cache interceptor silently skips caching when `cacheKey` is empty/missing, so you get no caching and no error. + +**MUST** scope cache keys to include the plugin name, operation, and all varying parameters: + +```ts +cache: { + cacheKey: ["analytics:query", queryKey, JSON.stringify(params), executorKey], +} +``` + +**SHOULD** disable retry for non-idempotent operations (mutations, chat messages, stream creation). See `genieStreamDefaults` for the pattern. + +**SHOULD** disable cache for write operations and streaming downloads. See `FILES_WRITE_DEFAULTS` and `FILES_DOWNLOAD_DEFAULTS`. + +**SHOULD** set appropriate timeouts: short for reads (5-30s), long for writes/uploads (600s), very long for streaming conversations (120s+). + +--- + +## 5. asUser / OBO Patterns + +**MUST** call `this.asUser(req)` to execute operations with user credentials. The returned proxy wraps every method call in `runInUserContext(userContext, ...)`. + +**NEVER** call `asUser()` on lifecycle methods (`setup`, `shutdown`, `injectRoutes`). These are excluded from the proxy via `EXCLUDED_FROM_PROXY`. + +**MUST** use `.obo.sql` file suffix for analytics queries that should execute as the user. The `_handleQueryRoute` method checks `isAsUser` and conditionally wraps with `this.asUser(req)`. + +**SHOULD** use `isInUserContext()` in the programmatic API to detect whether the call is running in a user context. Two patterns exist: + +- **File-naming convention** (analytics): Use `.obo.sql` suffix to determine whether a query runs as user or service principal. No runtime `isInUserContext()` check needed in route handlers. +- **Runtime enforcement** (files programmatic API): Call `isInUserContext()` to warn or throw: + +```ts +// Warn pattern (read operations in route handlers): +if (!isInUserContext()) { logger.warn("..."); } + +// Enforce pattern (programmatic API, e.g. createVolumeAPI): +if (!isInUserContext()) { throw new Error("...use OBO..."); } +``` + +**MUST** scope cache keys differently for OBO vs service principal. Use `getCurrentUserId()` for OBO and `"global"` for service principal to avoid cross-user cache pollution. + +**SHOULD** prefer strict enforcement (`throwIfNoUserContext`) for write operations. Use warn-only (`warnIfNoUserContext`) for read operations where service principal fallback is acceptable. + +--- + +## 6. Client Config + +**MUST** return only JSON-serializable plain data from `clientConfig()`. No functions, Dates, classes, Maps, Sets, BigInts, or circular references. + +**NEVER** expose secrets, tokens, or internal URLs in `clientConfig()`. The server plugin runs `sanitizeClientConfig()` which redacts values matching non-public env vars, but defense in depth means not returning them at all. + +**SHOULD** return an empty object `{}` (the default) if the plugin has no client-facing config. Only override `clientConfig()` when the frontend needs server-side values at boot time. + +**SHOULD** keep client config minimal: feature flags, resource IDs, available volume keys. Avoid large payloads. + +--- + +## 7. SSE Streaming + +**MUST** use `this.executeStream(res, handler, settings)` for SSE responses. This wires up the interceptor chain, stream management, abort signals, and reconnection support. + +**MUST** return an `AsyncGenerator` from the stream handler for chunked event delivery. Non-generator return values are auto-wrapped in a single yield. + +**SHOULD** pass a stable `streamId` (e.g. from `requestId` query param or `randomUUID()`) in `stream.streamId` to support client reconnection via `Last-Event-ID`. + +**SHOULD** configure `stream.bufferSize` for event replay on reconnection. Default is fine for most cases; increase for high-throughput streams. + +**MUST** call `this.streamManager.abortAll()` in `shutdown()` to cancel all active streams during graceful shutdown. + +--- + +## 8. Testing Expectations + +**MUST** co-locate tests in a `tests/` directory inside the plugin folder (e.g. `plugins/analytics/tests/analytics.test.ts`). + +**MUST** mock external connectors and the Databricks SDK. Use `vi.mock()` for connector classes. Never make real API calls in unit tests. + +**SHOULD** write both unit tests (`.test.ts`) and integration tests (`.integration.test.ts`). Integration tests exercise the full interceptor chain with mocked connectors. + +**SHOULD** test error paths: missing params (400), not-found (404), upstream failures (500), auth errors (401). + +**SHOULD** test cache key scoping: verify that different users, different params, and different query keys produce distinct cache entries. + +**SHOULD** test `asUser` proxy behavior: verify that route handlers correctly delegate to `this.asUser(req)` for OBO endpoints. + +--- + +## 9. Type Safety + +**MUST** type the config interface extending `BasePluginConfig` and export it from `types.ts`. + +**MUST** type `exports()` with an explicit return type or interface when the public API is non-trivial. This ensures the registry type generation produces accurate types. + +**MUST** type route handler request/response bodies. Use `req.body as IMyRequest` with a defined interface, not `any`. + +**SHOULD** use `PluginManifest<"name">` generic to tie the manifest type to the plugin name literal. This enables type-safe access on the AppKit instance. + +**SHOULD** use `protected declare config: IMyConfig` instead of redefining the property. The `declare` keyword preserves the base class field while narrowing the type. + +**NEVER** use `any` for connector responses or SDK return types without documenting why. Prefer `unknown` and narrow with type guards, or define response interfaces. diff --git a/.claude/references/plugin-review-guidance.md b/.claude/references/plugin-review-guidance.md new file mode 100644 index 000000000..89c0d2551 --- /dev/null +++ b/.claude/references/plugin-review-guidance.md @@ -0,0 +1,40 @@ +# Plugin Review Guidance + +Shared evaluation rules for any skill that reviews plugin code against the best-practices categories. + +## Category Index + +The canonical list of best-practices categories (defined in `plugin-best-practices.md`): + +| # | Category | What to check | +|---|----------|---------------| +| 1 | Manifest Design | `manifest.json` fields, resource declarations, naming | +| 2 | Plugin Class Structure | Base class, static manifest, factory export, config, shutdown | +| 3 | Route Design | `injectRoutes()`, `this.route()`, endpoint naming, body parsing | +| 4 | Interceptor Usage | `execute()`/`executeStream()` calls, `defaults.ts`, cache keys | +| 5 | asUser / OBO Patterns | `asUser(req)`, `.obo.sql`, cache key scoping, context enforcement | +| 6 | Client Config | `clientConfig()` return value, serialization, secret exposure | +| 7 | SSE Streaming | `executeStream()`, `AsyncGenerator`, `streamManager`, shutdown | +| 8 | Testing Expectations | Co-located tests, mocking, error paths, cache key coverage | +| 9 | Type Safety | Config interface, `exports()` return type, `any` usage | + +## Severity Ordering + +Always order findings by severity, not by category: + +1. **NEVER** findings first — security or breakage blockers +2. **MUST** findings second — correctness requirements +3. **SHOULD** findings last — quality recommendations + +## Deduplication + +If the same code issue is covered by guidelines in multiple categories, report it once under the most specific category and note that it also relates to the other category. Do not count it as separate findings in each category. + +## Cache-Key Tracing + +When evaluating cache configuration (Interceptor Usage, Category 4), the codebase uses a two-stage pattern: + +1. `defaults.ts` defines partial cache config (e.g., `enabled: true, ttl: 3600`) **without** a `cacheKey`. +2. The `cacheKey` is injected at the `execute()` / `executeStream()` call site in the plugin class. + +Before flagging a NEVER violation for missing `cacheKey`, trace the cache config through to every call site. Only flag if no `cacheKey` is provided at any point in the execution path. From 4b3b398e223d8d929eaae6b4726f057f716f89f1 Mon Sep 17 00:00:00 2001 From: Pawel Kosiec Date: Mon, 20 Apr 2026 19:38:33 +0200 Subject: [PATCH 09/14] fix: add AST extraction to serving type generator and move types to shared/ (#279) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add AST extraction to serving type generator and move types to shared/ The CLI generate-types command was falling back to the DATABRICKS_SERVING_ENDPOINT_NAME env var instead of extracting endpoint aliases from server.ts, producing a single "default" key instead of the actual aliases (e.g., "first", "second", "third"). This causes build failures when client code references typed endpoint aliases. Additionally, generated type declarations are moved from client/src/appkit-types/ to shared/appkit-types/ so both server and client tsconfigs can see the ServingEndpointRegistry module augmentation (server tsconfig already includes shared/**/*). Co-authored-by: Isaac * fix: update serving vite-plugin test for shared/ output path The outFile path changed from client/src/ to shared/ but the test assertion was not updated. Signed-off-by: Pawel Kosiec * fix: update gitignore for shared/ types and add tsc to template server build - Move serving types gitignore from client/src/ to shared/ (dev-playground + template) - Add old file cleanup in generator to prevent duplicate module augmentation - Add tsc -b to template build:server for type-checking parity with build:client - Simplify template typecheck script to use tsc -b consistently Signed-off-by: Pawel Kosiec * fix: add old file cleanup for analytics types too Mirror the serving types cleanup — remove stale client/src/appkit-types/analytics.d.ts after generating at the new shared/ location. Signed-off-by: Pawel Kosiec * fix: deduplicate migration cleanup, add error handling, remove dead code - Extract removeOldGeneratedTypes into migration.ts to avoid duplication and circular dependency between index.ts and serving/generator.ts - Add try-catch to resolveEndpointsFromServerFile so AST parse failures fall back gracefully to env var - Remove unused root variable in analytics vite-plugin - Clean up template tsconfig.server.json: remove dead emit options (outDir, declaration, declarationMap, sourceMap) since noEmit is set Signed-off-by: Pawel Kosiec * feat: make serving alias optional for single-endpoint registries ServingFactory now adapts based on endpoint count: - Empty registry: alias optional, untyped - Single key: alias optional, fully typed (defaults to the only endpoint) - Multiple keys: alias required for disambiguation Signed-off-by: Pawel Kosiec * fix: register named routes in unnamed serving mode for type-generated clients When serving() is called without explicit endpoints (unnamed mode), the type generator still creates a "default" alias. Client hooks then construct /api/serving/default/invoke, but the server only had /invoke registered — causing a 404. Now unnamed mode registers both /invoke and /:alias/invoke (same for stream) so both URL patterns work. Signed-off-by: Pawel Kosiec * feat: auto-migrate project configs for shared/ types output path When type generation runs, automatically patch existing apps' config files to match the new shared/appkit-types/ output location: - tsconfig.client.json: add "shared/appkit-types" to include - tsconfig.server.json: switch from emit mode to noEmit - package.json: update build:server and typecheck scripts Each sub-migration is idempotent (content-based detection, no marker files). Opt-out via "appkit": { "autoMigrate": false } in package.json. Vite plugins now pass projectRoot explicitly to generators. Signed-off-by: Pawel Kosiec * fix: improve migration robustness, add type tests, fix stale CLI help - Log warnings on migration sub-function failures instead of silent catch - Track migrated projects per-root (Set) instead of global boolean flag - Validate resolveProjectRoot output by checking for package.json - Add compile-time type tests for IsUnion and ServingFactory conditional - Update CLI help text examples to reference shared/appkit-types/ paths Signed-off-by: Pawel Kosiec * fix: clean up pre-release type files (appKitTypes.d.ts, appKitServingTypes.d.ts) Target the actually shipped filenames for old type cleanup instead of the intermediate appkit-types/ directory that was never released. Fix outdated JSDoc comments referencing old paths. Signed-off-by: Pawel Kosiec * docs: update stale type file path references to shared/appkit-types/ Signed-off-by: Pawel Kosiec --------- Signed-off-by: Pawel Kosiec --- apps/dev-playground/.gitignore | 5 +- apps/dev-playground/client/.gitignore | 3 - .../client/src/routes/type-safety.route.tsx | 4 +- apps/dev-playground/client/tsconfig.app.json | 2 +- .../appkit-types/analytics.d.ts | 0 .../api/appkit/TypeAlias.ServingFactory.md | 19 +- docs/docs/development/type-generation.md | 9 +- docs/docs/plugins/analytics.md | 2 +- packages/appkit-ui/src/react/hooks/types.ts | 2 +- .../appkit/src/plugins/serving/serving.ts | 40 +- .../src/plugins/serving/tests/serving.test.ts | 10 + .../src/plugins/serving/tests/types.test.ts | 86 ++++ packages/appkit/src/plugins/serving/types.ts | 47 ++- packages/appkit/src/type-generator/index.ts | 10 + .../appkit/src/type-generator/migration.ts | 257 +++++++++++ .../src/type-generator/serving/generator.ts | 41 +- .../serving/tests/vite-plugin.test.ts | 4 +- .../src/type-generator/serving/vite-plugin.ts | 6 +- .../type-generator/tests/migration.test.ts | 399 ++++++++++++++++++ .../appkit/src/type-generator/vite-plugin.ts | 7 +- .../shared/src/cli/commands/generate-types.ts | 10 +- template/_gitignore | 3 + template/package.json | 4 +- template/tsconfig.client.json | 2 +- template/tsconfig.server.json | 9 +- 25 files changed, 906 insertions(+), 75 deletions(-) rename apps/dev-playground/{client/src => shared}/appkit-types/analytics.d.ts (100%) create mode 100644 packages/appkit/src/plugins/serving/tests/types.test.ts create mode 100644 packages/appkit/src/type-generator/migration.ts create mode 100644 packages/appkit/src/type-generator/tests/migration.test.ts diff --git a/apps/dev-playground/.gitignore b/apps/dev-playground/.gitignore index 5c957fcd6..1f4745f52 100644 --- a/apps/dev-playground/.gitignore +++ b/apps/dev-playground/.gitignore @@ -1,3 +1,6 @@ # Playwright test-results/ -playwright-report/ \ No newline at end of file +playwright-report/ + +# Auto-generated types (endpoint-specific, varies per developer) +shared/appkit-types/serving.d.ts \ No newline at end of file diff --git a/apps/dev-playground/client/.gitignore b/apps/dev-playground/client/.gitignore index 19067f126..a547bf36d 100644 --- a/apps/dev-playground/client/.gitignore +++ b/apps/dev-playground/client/.gitignore @@ -12,9 +12,6 @@ dist dist-ssr *.local -# Auto-generated types (endpoint-specific, varies per developer) -src/appkit-types/serving.d.ts - # Editor directories and files .vscode/* !.vscode/extensions.json diff --git a/apps/dev-playground/client/src/routes/type-safety.route.tsx b/apps/dev-playground/client/src/routes/type-safety.route.tsx index 07084c534..6962d5d51 100644 --- a/apps/dev-playground/client/src/routes/type-safety.route.tsx +++ b/apps/dev-playground/client/src/routes/type-safety.route.tsx @@ -454,7 +454,7 @@ function TypeSafetyRoute() { 2. Generated Types - Vite plugin or npx command generates appKitTypes.d.ts at build + Vite plugin or npx command generates analytics types at build time @@ -507,7 +507,7 @@ export default defineConfig({ plugins: [ appKitTypesPlugin(), // Optional: appKitTypesPlugin({ - // outputFile: 'src/appKitTypes.d.ts', + // outFile: 'shared/appkit-types/analytics.d.ts', // watchFolders: ['../config/queries'], // }), ], diff --git a/apps/dev-playground/client/tsconfig.app.json b/apps/dev-playground/client/tsconfig.app.json index 5fab448d7..6e2a3b464 100644 --- a/apps/dev-playground/client/tsconfig.app.json +++ b/apps/dev-playground/client/tsconfig.app.json @@ -29,5 +29,5 @@ "@/*": ["./src/*"] } }, - "include": ["src"] + "include": ["src", "../shared/appkit-types"] } diff --git a/apps/dev-playground/client/src/appkit-types/analytics.d.ts b/apps/dev-playground/shared/appkit-types/analytics.d.ts similarity index 100% rename from apps/dev-playground/client/src/appkit-types/analytics.d.ts rename to apps/dev-playground/shared/appkit-types/analytics.d.ts diff --git a/docs/docs/api/appkit/TypeAlias.ServingFactory.md b/docs/docs/api/appkit/TypeAlias.ServingFactory.md index e32ffd648..bb2b04d28 100644 --- a/docs/docs/api/appkit/TypeAlias.ServingFactory.md +++ b/docs/docs/api/appkit/TypeAlias.ServingFactory.md @@ -1,19 +1,18 @@ # Type Alias: ServingFactory ```ts -type ServingFactory = keyof ServingEndpointRegistry extends never ? (alias?: string) => ServingEndpointHandle : (alias: K) => ServingEndpointHandle; +type ServingFactory = keyof ServingEndpointRegistry extends never ? (alias?: string) => ServingEndpointHandle : true extends IsUnion ? (alias: K) => ServingEndpointHandle : { + (alias: K): ServingEndpointHandle; + (): ServingEndpointHandle; +}; ``` Factory function returned by `AppKit.serving`. -This is a conditional type that adapts based on whether `ServingEndpointRegistry` -has been populated via module augmentation (generated by `appKitServingTypesPlugin()`): +Adapts based on the `ServingEndpointRegistry` state: -- **Registry empty (default):** `(alias?: string) => ServingEndpointHandle` — - accepts any alias string with untyped request/response. -- **Registry populated:** `(alias: K) => ServingEndpointHandle<...>` — - restricts `alias` to known endpoint keys and infers typed request/response - from the registry entry. +- **Empty (default):** `(alias?: string) => ServingEndpointHandle` — any string, untyped. +- **Single key:** alias optional — `serving()` returns the typed handle for the only endpoint. +- **Multiple keys:** alias required — must specify which endpoint. -Run `appKitServingTypesPlugin()` in your Vite config to generate the registry -augmentation and enable full type safety. +Run `npx appkit generate-types` or start the dev server to generate the registry. diff --git a/docs/docs/development/type-generation.md b/docs/docs/development/type-generation.md index fd71808ae..c433bb5a9 100644 --- a/docs/docs/development/type-generation.md +++ b/docs/docs/development/type-generation.md @@ -10,7 +10,7 @@ AppKit can automatically generate TypeScript types for your SQL queries, providi Generate type-safe TypeScript declarations for query keys, parameters, and result rows. -All generated files live in `client/src/appkit-types/`, one per plugin (e.g. `analytics.d.ts`). They use [`declare module`](https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation) to augment existing interfaces, so the types apply globally — you never need to import them. TypeScript auto-discovers them through `"include": ["src"]` in your tsconfig. +All generated files live in `shared/appkit-types/`, one per plugin (e.g. `analytics.d.ts`). They use [`declare module`](https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation) to augment existing interfaces, so the types apply globally — you never need to import them. TypeScript auto-discovers them through `"include": ["shared/appkit-types"]` in your tsconfig. ## Vite plugin: `appKitTypesPlugin` @@ -18,7 +18,7 @@ The recommended approach is to use the Vite plugin, which watches your SQL files ### Configuration -- `outFile?: string` - Output file path (default: `src/appkit-types/analytics.d.ts`) +- `outFile?: string` - Output file path (default: `shared/appkit-types/analytics.d.ts`) - `watchFolders?: string[]` - Folders to watch for SQL files (default: `["../config/queries"]`) ### Example @@ -33,7 +33,6 @@ export default defineConfig({ plugins: [ react(), appKitTypesPlugin({ - outFile: "src/appkit-types/analytics.d.ts", watchFolders: ["../config/queries"], }), ], @@ -58,13 +57,13 @@ npx @databricks/appkit generate-types [rootDir] [outFile] [warehouseId] - Generate types using warehouse ID from environment ```bash - npx @databricks/appkit generate-types . client/src/appkit-types/analytics.d.ts + npx @databricks/appkit generate-types . shared/appkit-types/analytics.d.ts ``` - Generate types using warehouse ID explicitly ```bash - npx @databricks/appkit generate-types . client/src/appkit-types/analytics.d.ts abc123... + npx @databricks/appkit generate-types . shared/appkit-types/analytics.d.ts abc123... ``` - Force regeneration (skip cache) diff --git a/docs/docs/plugins/analytics.md b/docs/docs/plugins/analytics.md index 187bf4349..22204f529 100644 --- a/docs/docs/plugins/analytics.md +++ b/docs/docs/plugins/analytics.md @@ -136,7 +136,7 @@ function SpendTable() { Augment the `QueryRegistry` interface to get full type inference on parameters and results: ```ts -// client/src/appkit-types/analytics.d.ts +// shared/appkit-types/analytics.d.ts declare module "@databricks/appkit-ui/react" { interface QueryRegistry { spend_summary: { diff --git a/packages/appkit-ui/src/react/hooks/types.ts b/packages/appkit-ui/src/react/hooks/types.ts index bd5a7dc2e..03e943e2a 100644 --- a/packages/appkit-ui/src/react/hooks/types.ts +++ b/packages/appkit-ui/src/react/hooks/types.ts @@ -59,7 +59,7 @@ export interface UseAnalyticsQueryResult { * * @example * ```typescript - * // config/appKitTypes.d.ts + * // shared/appkit-types/analytics.d.ts * declare module "@databricks/appkit-ui/react" { * interface QueryRegistry { * apps_list: { diff --git a/packages/appkit/src/plugins/serving/serving.ts b/packages/appkit/src/plugins/serving/serving.ts index ea7c7d1fc..28c13d31d 100644 --- a/packages/appkit/src/plugins/serving/serving.ts +++ b/packages/appkit/src/plugins/serving/serving.ts @@ -175,24 +175,46 @@ export class ServingPlugin extends Plugin { }, }); } else { + // Unnamed mode: register both /invoke and /:alias/invoke patterns. + // The type generator creates a "default" alias, so clients may use either URL. + const invokeHandler = async ( + req: express.Request, + res: express.Response, + ) => { + req.params.alias ??= "default"; + await this.asUser(req)._handleInvoke(req, res); + }; + const streamHandler = async ( + req: express.Request, + res: express.Response, + ) => { + req.params.alias ??= "default"; + await this.asUser(req)._handleStream(req, res); + }; + this.route(router, { name: "invoke", method: "post", path: "/invoke", - handler: async (req: express.Request, res: express.Response) => { - req.params.alias = "default"; - await this.asUser(req)._handleInvoke(req, res); - }, + handler: invokeHandler, + }); + this.route(router, { + name: "invoke-named", + method: "post", + path: "/:alias/invoke", + handler: invokeHandler, }); - this.route(router, { name: "stream", method: "post", path: "/stream", - handler: async (req: express.Request, res: express.Response) => { - req.params.alias = "default"; - await this.asUser(req)._handleStream(req, res); - }, + handler: streamHandler, + }); + this.route(router, { + name: "stream-named", + method: "post", + path: "/:alias/stream", + handler: streamHandler, }); } } diff --git a/packages/appkit/src/plugins/serving/tests/serving.test.ts b/packages/appkit/src/plugins/serving/tests/serving.test.ts index 6f9750255..2f2be9574 100644 --- a/packages/appkit/src/plugins/serving/tests/serving.test.ts +++ b/packages/appkit/src/plugins/serving/tests/serving.test.ts @@ -90,6 +90,16 @@ describe("Serving Plugin", () => { expect(handlers["POST:/stream"]).toBeDefined(); }); + test("also registers /:alias/invoke and /:alias/stream for type-generated clients", () => { + const plugin = new ServingPlugin({}); + const { router, handlers } = createMockRouter(); + + plugin.injectRoutes(router); + + expect(handlers["POST:/:alias/invoke"]).toBeDefined(); + expect(handlers["POST:/:alias/stream"]).toBeDefined(); + }); + test("exports returns a factory that provides invoke", () => { const plugin = new ServingPlugin({}); const factory = plugin.exports() as any; diff --git a/packages/appkit/src/plugins/serving/tests/types.test.ts b/packages/appkit/src/plugins/serving/tests/types.test.ts new file mode 100644 index 000000000..e99430cc2 --- /dev/null +++ b/packages/appkit/src/plugins/serving/tests/types.test.ts @@ -0,0 +1,86 @@ +import { assertType, describe, expectTypeOf, test } from "vitest"; +import type { ServingEndpointHandle } from "../types"; + +/** + * Compile-time type tests for the serving type system. + * These tests verify that the IsUnion utility type and the 3-way ServingFactory + * conditional produce correct signatures for different registry shapes. + * + * Tests use expectTypeOf (pure type-level, no runtime calls). + */ + +// Mirror IsUnion from types.ts (not exported, so re-declared here for testing) +type IsUnion = T extends C ? ([C] extends [T] ? false : true) : never; + +// ── IsUnion ───────────────────────────────────────────────────────────── + +describe("IsUnion", () => { + test("single literal is not a union", () => { + assertType(false as IsUnion<"a">); + }); + + test("two-member union is detected", () => { + assertType(true as IsUnion<"a" | "b">); + }); + + test("three-member union is detected", () => { + assertType(true as IsUnion<"a" | "b" | "c">); + }); +}); + +// ── ServingFactory-equivalent patterns ────────────────────────────────── +// We can't augment ServingEndpointRegistry differently per test, so we +// test the conditional logic using equivalent local types. + +interface SingleKeyRegistry { + default: { + request: { prompt: string }; + response: { text: string }; + }; +} + +interface MultiKeyRegistry { + llm: { + request: { prompt: string }; + response: { text: string }; + }; + embedder: { + request: { text: string }; + response: number[]; + }; +} + +// Factory type mirroring ServingFactory but parameterised by registry +type TestFactory = keyof R extends never + ? (alias?: string) => ServingEndpointHandle + : true extends IsUnion + ? (alias: K) => ServingEndpointHandle + : { + (alias: K): ServingEndpointHandle; + (): ServingEndpointHandle; + }; + +describe("ServingFactory conditional", () => { + test("empty registry: produces function with optional string param", () => { + type F = TestFactory>; + expectTypeOf().toBeFunction(); + // Alias is optional — should accept (alias?: string) + expectTypeOf().parameter(0).toEqualTypeOf(); + }); + + test("single-key registry: has call signatures including no-arg", () => { + type F = TestFactory; + // Should be callable (it's an object with call signatures) + expectTypeOf().toBeCallableWith("default"); + expectTypeOf().toBeCallableWith(); + }); + + test("multi-key registry: alias is required", () => { + type F = TestFactory; + expectTypeOf().toBeCallableWith("llm"); + expectTypeOf().toBeCallableWith("embedder"); + // No-arg call should NOT be valid — verified via @ts-expect-error + // @ts-expect-error - calling with no args should fail for multi-key + expectTypeOf().toBeCallableWith(); + }); +}); diff --git a/packages/appkit/src/plugins/serving/types.ts b/packages/appkit/src/plugins/serving/types.ts index 2da1f2554..9ba4616fd 100644 --- a/packages/appkit/src/plugins/serving/types.ts +++ b/packages/appkit/src/plugins/serving/types.ts @@ -49,26 +49,41 @@ export type ServingEndpointHandle< ) => ServingEndpointMethods; }; +/** True when T is a union of 2+ members; false for a single literal type. */ +type IsUnion = T extends C ? ([C] extends [T] ? false : true) : never; + /** * Factory function returned by `AppKit.serving`. * - * This is a conditional type that adapts based on whether `ServingEndpointRegistry` - * has been populated via module augmentation (generated by `appKitServingTypesPlugin()`): + * Adapts based on the `ServingEndpointRegistry` state: * - * - **Registry empty (default):** `(alias?: string) => ServingEndpointHandle` — - * accepts any alias string with untyped request/response. - * - **Registry populated:** `(alias: K) => ServingEndpointHandle<...>` — - * restricts `alias` to known endpoint keys and infers typed request/response - * from the registry entry. + * - **Empty (default):** `(alias?: string) => ServingEndpointHandle` — any string, untyped. + * - **Single key:** alias optional — `serving()` returns the typed handle for the only endpoint. + * - **Multiple keys:** alias required — must specify which endpoint. * - * Run `appKitServingTypesPlugin()` in your Vite config to generate the registry - * augmentation and enable full type safety. + * Run `npx appkit generate-types` or start the dev server to generate the registry. */ export type ServingFactory = keyof ServingEndpointRegistry extends never - ? (alias?: string) => ServingEndpointHandle - : ( - alias: K, - ) => ServingEndpointHandle< - ServingEndpointRegistry[K]["request"], - ServingEndpointRegistry[K]["response"] - >; + ? // Empty registry: accept any string, alias optional + (alias?: string) => ServingEndpointHandle + : true extends IsUnion + ? // Multiple keys: alias REQUIRED for disambiguation + ( + alias: K, + ) => ServingEndpointHandle< + ServingEndpointRegistry[K]["request"], + ServingEndpointRegistry[K]["response"] + > + : // Single key: alias optional (runtime defaults to "default") + { + ( + alias: K, + ): ServingEndpointHandle< + ServingEndpointRegistry[K]["request"], + ServingEndpointRegistry[K]["response"] + >; + (): ServingEndpointHandle< + ServingEndpointRegistry[keyof ServingEndpointRegistry]["request"], + ServingEndpointRegistry[keyof ServingEndpointRegistry]["response"] + >; + }; diff --git a/packages/appkit/src/type-generator/index.ts b/packages/appkit/src/type-generator/index.ts index a1e04025a..c9a528fe7 100644 --- a/packages/appkit/src/type-generator/index.ts +++ b/packages/appkit/src/type-generator/index.ts @@ -2,6 +2,11 @@ import fs from "node:fs/promises"; import path from "node:path"; import dotenv from "dotenv"; import { createLogger } from "../logging/logger"; +import { + migrateProjectConfig, + removeOldGeneratedTypes, + resolveProjectRoot, +} from "./migration"; import { generateQueriesFromDescribe } from "./query-registry"; import { generateServingTypes as generateServingTypesImpl } from "./serving/generator"; import type { QuerySchema } from "./types"; @@ -54,6 +59,7 @@ export async function generateFromEntryPoint(options: { noCache?: boolean; }) { const { outFile, queryFolder, warehouseId, noCache } = options; + const projectRoot = resolveProjectRoot(outFile); logger.debug("Starting type generation..."); @@ -87,6 +93,10 @@ export async function generateFromEntryPoint(options: { await fs.mkdir(path.dirname(outFile), { recursive: true }); await fs.writeFile(outFile, typeDeclarations, "utf-8"); + // One-time migration: remove old generated file and patch project configs + await removeOldGeneratedTypes(projectRoot, "appKitTypes.d.ts"); + await migrateProjectConfig(projectRoot); + logger.debug("Type generation complete!"); } diff --git a/packages/appkit/src/type-generator/migration.ts b/packages/appkit/src/type-generator/migration.ts new file mode 100644 index 000000000..f44e722a6 --- /dev/null +++ b/packages/appkit/src/type-generator/migration.ts @@ -0,0 +1,257 @@ +import fsSync from "node:fs"; +import fs from "node:fs/promises"; +import path from "node:path"; +import pc from "picocolors"; +import { createLogger } from "../logging/logger"; + +const logger = createLogger("type-generator:migration"); + +/** + * Derive project root from an outFile path. + * outFile is always `/shared/appkit-types/` — both the Vite plugins + * and the CLI construct it this way, so going up two levels is safe. + * + * Validates that the resolved root contains a package.json — if not, logs a warning + * so custom outFile paths don't silently operate on the wrong directory. + */ +export function resolveProjectRoot(outFile: string): string { + const root = path.resolve(path.dirname(outFile), "..", ".."); + if (!fsSync.existsSync(path.join(root, "package.json"))) { + logger.warn( + "Resolved project root %s has no package.json — migration may target the wrong directory. " + + "Check your outFile path: %s", + root, + outFile, + ); + } + return root; +} + +/** + * Remove old generated types from client/src/ (pre-shared/ location). + * Best-effort: silently ignores missing files. + */ +export async function removeOldGeneratedTypes( + projectRoot: string, + filename: string, +): Promise { + const oldFile = path.join(projectRoot, "client", "src", filename); + try { + await fs.unlink(oldFile); + logger.debug("Removed old types at %s", oldFile); + } catch { + // File doesn't exist — nothing to clean up + } +} + +// ── Project config migration ──────────────────────────────────────────── + +const migratedProjects = new Set(); + +/** + * One-time config migration: update tsconfig and package.json for shared/ types output. + * Idempotent — each sub-migration checks current file state and skips if already migrated. + * Deduplicates per project root so monorepo builds migrate each app independently. + * Opt-out: set `"appkit": { "autoMigrate": false }` in package.json. + */ +export async function migrateProjectConfig(projectRoot: string): Promise { + const resolved = path.resolve(projectRoot); + if (migratedProjects.has(resolved)) return; + migratedProjects.add(resolved); + + if (await isAutoMigrateDisabled(projectRoot)) { + logger.debug("Auto-migration disabled via package.json appkit.autoMigrate"); + return; + } + + const results: Array<{ file: string; action: string }> = []; + + results.push(...(await migrateTsconfigClient(projectRoot))); + results.push(...(await migrateTsconfigServer(projectRoot))); + results.push(...(await migratePackageJsonScripts(projectRoot))); + + if (results.length > 0) { + printMigrationSummary(results); + } +} + +/** Exported for testing only. */ +export function _resetMigrationState(): void { + migratedProjects.clear(); +} + +// ── Helpers ───────────────────────────────────────────────────────────── + +async function isAutoMigrateDisabled(projectRoot: string): Promise { + try { + const raw = await fs.readFile( + path.join(projectRoot, "package.json"), + "utf-8", + ); + const parsed = JSON.parse(raw); + return parsed.appkit?.autoMigrate === false; + } catch { + return false; + } +} + +/** Strip JSONC comments (block and line) so JSON.parse can handle tsconfig files. */ +function stripJsonComments(text: string): string { + // Match strings (to skip them) or comments (to remove them). + // Strings must be matched first to avoid stripping comment-like patterns inside string values + // (e.g. "server/**/*" contains /* which looks like a block comment start). + return text.replace(/"(?:[^"\\]|\\.)*"|\/\*[\s\S]*?\*\/|\/\/.*/g, (match) => + match.startsWith('"') ? match : "", + ); +} + +type MigrationResult = Array<{ file: string; action: string }>; + +// ── tsconfig.client.json ──────────────────────────────────────────────── + +async function migrateTsconfigClient( + projectRoot: string, +): Promise { + const results: MigrationResult = []; + const filePath = path.join(projectRoot, "tsconfig.client.json"); + + try { + const raw = await fs.readFile(filePath, "utf-8"); + const parsed = JSON.parse(stripJsonComments(raw)); + + if (!Array.isArray(parsed.include)) return results; + if (parsed.include.includes("shared/appkit-types")) return results; + + parsed.include.push("shared/appkit-types"); + await fs.writeFile( + filePath, + `${JSON.stringify(parsed, null, 2)}\n`, + "utf-8", + ); + results.push({ + file: "tsconfig.client.json", + action: 'added "shared/appkit-types" to include', + }); + } catch (err) { + logger.warn( + "Failed to migrate tsconfig.client.json: %s", + (err as Error).message, + ); + } + + return results; +} + +// ── tsconfig.server.json ──────────────────────────────────────────────── + +async function migrateTsconfigServer( + projectRoot: string, +): Promise { + const results: MigrationResult = []; + const filePath = path.join(projectRoot, "tsconfig.server.json"); + + try { + const raw = await fs.readFile(filePath, "utf-8"); + const parsed = JSON.parse(stripJsonComments(raw)); + const opts = parsed.compilerOptions; + + if (!opts || !opts.outDir) return results; // already migrated or non-standard + + delete opts.outDir; + delete opts.declaration; + delete opts.declarationMap; + delete opts.sourceMap; + opts.noEmit = true; + + await fs.writeFile( + filePath, + `${JSON.stringify(parsed, null, 2)}\n`, + "utf-8", + ); + results.push({ + file: "tsconfig.server.json", + action: "switched to noEmit mode", + }); + } catch (err) { + logger.warn( + "Failed to migrate tsconfig.server.json: %s", + (err as Error).message, + ); + } + + return results; +} + +// ── package.json ──────────────────────────────────────────────────────── + +const SCRIPT_MIGRATIONS: Record = { + "build:server": { + old: "tsdown -c tsdown.server.config.ts", + new: "tsc -b tsconfig.server.json && tsdown -c tsdown.server.config.ts", + }, + typecheck: { + old: "tsc -p ./tsconfig.server.json --noEmit && tsc -p ./tsconfig.client.json --noEmit", + new: "tsc -b tsconfig.server.json && tsc -b tsconfig.client.json", + }, +}; + +async function migratePackageJsonScripts( + projectRoot: string, +): Promise { + const results: MigrationResult = []; + const filePath = path.join(projectRoot, "package.json"); + + try { + const raw = await fs.readFile(filePath, "utf-8"); + const parsed = JSON.parse(raw); + const scripts = parsed.scripts; + if (!scripts) return results; + + const updated: string[] = []; + + for (const [name, { old, new: replacement }] of Object.entries( + SCRIPT_MIGRATIONS, + )) { + if (scripts[name] === old) { + scripts[name] = replacement; + updated.push(name); + } + } + + if (updated.length === 0) return results; + + const indent = raw.match(/^\s+/m)?.[0]?.length === 4 ? 4 : 2; + await fs.writeFile( + filePath, + `${JSON.stringify(parsed, null, indent)}\n`, + "utf-8", + ); + results.push({ + file: "package.json", + action: `updated ${updated.join(" and ")} scripts`, + }); + } catch (err) { + logger.warn( + "Failed to migrate package.json scripts: %s", + (err as Error).message, + ); + } + + return results; +} + +// ── Summary ───────────────────────────────────────────────────────────── + +function printMigrationSummary( + results: Array<{ file: string; action: string }>, +) { + const separator = pc.dim("─".repeat(50)); + console.log(""); + console.log(` ${pc.bold("Typegen Migration")}`); + console.log(` ${separator}`); + for (const { file, action } of results) { + console.log(` ${pc.green("✓")} ${file.padEnd(24)} ${pc.dim(action)}`); + } + console.log(` ${separator}`); + console.log(""); +} diff --git a/packages/appkit/src/type-generator/serving/generator.ts b/packages/appkit/src/type-generator/serving/generator.ts index 6adf4a28c..b377c6597 100644 --- a/packages/appkit/src/type-generator/serving/generator.ts +++ b/packages/appkit/src/type-generator/serving/generator.ts @@ -4,6 +4,11 @@ import { WorkspaceClient } from "@databricks/sdk-experimental"; import pc from "picocolors"; import { createLogger } from "../../logging/logger"; import type { EndpointConfig } from "../../plugins/serving/types"; +import { + migrateProjectConfig, + removeOldGeneratedTypes, + resolveProjectRoot, +} from "../migration"; import { CACHE_VERSION, hashSchema, @@ -18,6 +23,10 @@ import { extractRequestKeys, } from "./converter"; import { fetchOpenApiSchema } from "./fetcher"; +import { + extractServingEndpoints, + findServerFile, +} from "./server-file-extractor"; const logger = createLogger("type-generator:serving"); @@ -34,14 +43,22 @@ interface GenerateServingTypesOptions { /** * Generates TypeScript type declarations for serving endpoints * by fetching their OpenAPI schemas and converting to TypeScript. + * + * Endpoint discovery order (when `endpoints` is not provided): + * 1. AST extraction from server file (server/index.ts or server/server.ts) + * 2. DATABRICKS_SERVING_ENDPOINT_NAME env var (single default endpoint) */ export async function generateServingTypes( options: GenerateServingTypesOptions, ): Promise { const { outFile, noCache } = options; + const projectRoot = resolveProjectRoot(outFile); - // Resolve endpoints from config or env - const endpoints = options.endpoints ?? resolveDefaultEndpoints(); + // Resolve endpoints: explicit > AST extraction from server file > env var fallback + const endpoints = + options.endpoints ?? + resolveEndpointsFromServerFile() ?? + resolveDefaultEndpoints(); if (Object.keys(endpoints).length === 0) { logger.debug("No serving endpoints configured, skipping type generation"); return; @@ -77,6 +94,10 @@ export async function generateServingTypes( await fs.mkdir(path.dirname(outFile), { recursive: true }); await fs.writeFile(outFile, output, "utf-8"); + // One-time migration: remove old generated file and patch project configs + await removeOldGeneratedTypes(projectRoot, "appKitServingTypes.d.ts"); + await migrateProjectConfig(projectRoot); + if (registryEntries.length === 0) { logger.debug( "Wrote empty serving types to %s (no endpoints resolved)", @@ -227,6 +248,22 @@ function printLogTable( console.log(""); } +function resolveEndpointsFromServerFile(): + | Record + | undefined { + try { + const serverFile = findServerFile(process.cwd()); + if (!serverFile) return undefined; + return extractServingEndpoints(serverFile) ?? undefined; + } catch (error) { + logger.debug( + "Failed to extract endpoints from server file: %s", + (error as Error).message, + ); + return undefined; + } +} + function resolveDefaultEndpoints(): Record { if (process.env.DATABRICKS_SERVING_ENDPOINT_NAME) { return { default: { env: "DATABRICKS_SERVING_ENDPOINT_NAME" } }; diff --git a/packages/appkit/src/type-generator/serving/tests/vite-plugin.test.ts b/packages/appkit/src/type-generator/serving/tests/vite-plugin.test.ts index 889bee5b6..1d4ad6653 100644 --- a/packages/appkit/src/type-generator/serving/tests/vite-plugin.test.ts +++ b/packages/appkit/src/type-generator/serving/tests/vite-plugin.test.ts @@ -69,7 +69,7 @@ describe("appKitServingTypesPlugin", () => { }); describe("configResolved()", () => { - test("resolves outFile relative to config.root", async () => { + test("resolves outFile relative to project root", async () => { const plugin = appKitServingTypesPlugin({ endpoints: { llm: { env: "LLM" } }, }); @@ -79,7 +79,7 @@ describe("appKitServingTypesPlugin", () => { expect(mockGenerateServingTypes).toHaveBeenCalledWith( expect.objectContaining({ outFile: expect.stringContaining( - "/app/client/src/appkit-types/serving.d.ts", + "/app/shared/appkit-types/serving.d.ts", ), }), ); diff --git a/packages/appkit/src/type-generator/serving/vite-plugin.ts b/packages/appkit/src/type-generator/serving/vite-plugin.ts index 3e3e2d8f8..5c2a3b172 100644 --- a/packages/appkit/src/type-generator/serving/vite-plugin.ts +++ b/packages/appkit/src/type-generator/serving/vite-plugin.ts @@ -11,7 +11,7 @@ import { const logger = createLogger("type-generator:serving:vite-plugin"); interface AppKitServingTypesPluginOptions { - /** Path to the output .d.ts file (relative to client root). Default: "src/appKitServingTypes.d.ts" */ + /** Path to the output .d.ts file (relative to project root). */ outFile?: string; /** Endpoint config override. If omitted, auto-discovers from the server file or falls back to DATABRICKS_SERVING_ENDPOINT_NAME env var. */ endpoints?: Record; @@ -95,8 +95,8 @@ export function appKitServingTypesPlugin( // - pnpm build: process.cwd() is client/ (cd client && vite build), config.root is client/ projectRoot = path.resolve(config.root, ".."); outFile = path.resolve( - config.root, - options?.outFile ?? `src/${TYPES_DIR}/${SERVING_TYPES_FILE}`, + projectRoot, + options?.outFile ?? `shared/${TYPES_DIR}/${SERVING_TYPES_FILE}`, ); }, diff --git a/packages/appkit/src/type-generator/tests/migration.test.ts b/packages/appkit/src/type-generator/tests/migration.test.ts new file mode 100644 index 000000000..8b9ee82c2 --- /dev/null +++ b/packages/appkit/src/type-generator/tests/migration.test.ts @@ -0,0 +1,399 @@ +import fsp from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { _resetMigrationState, migrateProjectConfig } from "../migration"; + +let tmpDir: string; + +beforeEach(async () => { + tmpDir = await fsp.mkdtemp(path.join(os.tmpdir(), "appkit-migration-test-")); + _resetMigrationState(); +}); + +afterEach(async () => { + await fsp.rm(tmpDir, { recursive: true, force: true }); +}); + +function writeFile(name: string, content: string) { + return fsp.writeFile(path.join(tmpDir, name), content, "utf-8"); +} + +function readFile(name: string) { + return fsp.readFile(path.join(tmpDir, name), "utf-8"); +} + +describe("migrateProjectConfig", () => { + // ── tsconfig.client.json ──────────────────────────────────────────── + + describe("tsconfig.client.json", () => { + test("adds shared/appkit-types to include", async () => { + await writeFile( + "tsconfig.client.json", + JSON.stringify({ include: ["client/src"] }, null, 2), + ); + + await migrateProjectConfig(tmpDir); + + const result = JSON.parse(await readFile("tsconfig.client.json")); + expect(result.include).toEqual(["client/src", "shared/appkit-types"]); + }); + + test("no-op if shared/appkit-types already present", async () => { + const original = JSON.stringify( + { include: ["client/src", "shared/appkit-types"] }, + null, + 2, + ); + await writeFile("tsconfig.client.json", original); + + await migrateProjectConfig(tmpDir); + + const result = await readFile("tsconfig.client.json"); + expect(result).toBe(original); + }); + + test("handles JSONC block comments", async () => { + await writeFile( + "tsconfig.client.json", + `{ + /* Bundler mode */ + "compilerOptions": {}, + "include": ["client/src"] +}`, + ); + + await migrateProjectConfig(tmpDir); + + const result = JSON.parse(await readFile("tsconfig.client.json")); + expect(result.include).toEqual(["client/src", "shared/appkit-types"]); + }); + + test("handles JSONC line comments", async () => { + _resetMigrationState(); + await writeFile( + "tsconfig.client.json", + `{ + "compilerOptions": { + "target": "ES2022" // running on modern node + }, + "include": ["client/src"] +}`, + ); + + await migrateProjectConfig(tmpDir); + + const result = JSON.parse(await readFile("tsconfig.client.json")); + expect(result.include).toEqual(["client/src", "shared/appkit-types"]); + }); + + test("skips if include is not an array", async () => { + const original = JSON.stringify({ compilerOptions: {} }, null, 2); + await writeFile("tsconfig.client.json", original); + + await migrateProjectConfig(tmpDir); + + const result = await readFile("tsconfig.client.json"); + expect(result).toBe(original); + }); + }); + + // ── tsconfig.server.json ──────────────────────────────────────────── + + describe("tsconfig.server.json", () => { + test("removes emit config and adds noEmit", async () => { + await writeFile( + "tsconfig.server.json", + JSON.stringify( + { + compilerOptions: { + tsBuildInfoFile: + "./node_modules/.tmp/tsconfig.server.tsbuildinfo", + target: "ES2020", + lib: ["ES2020"], + outDir: "./dist", + rootDir: "./", + declaration: true, + declarationMap: true, + sourceMap: true, + }, + }, + null, + 2, + ), + ); + + await migrateProjectConfig(tmpDir); + + const result = JSON.parse(await readFile("tsconfig.server.json")); + expect(result.compilerOptions.outDir).toBeUndefined(); + expect(result.compilerOptions.declaration).toBeUndefined(); + expect(result.compilerOptions.declarationMap).toBeUndefined(); + expect(result.compilerOptions.sourceMap).toBeUndefined(); + expect(result.compilerOptions.noEmit).toBe(true); + expect(result.compilerOptions.rootDir).toBe("./"); + expect(result.compilerOptions.target).toBe("ES2020"); + }); + + test("no-op if already using noEmit (no outDir)", async () => { + const original = JSON.stringify( + { compilerOptions: { noEmit: true, rootDir: "./" } }, + null, + 2, + ); + await writeFile("tsconfig.server.json", original); + + await migrateProjectConfig(tmpDir); + + const result = await readFile("tsconfig.server.json"); + expect(result).toBe(original); + }); + + test("handles JSONC comments", async () => { + await writeFile( + "tsconfig.server.json", + `{ + "compilerOptions": { + "target": "ES2020", + /* Emit */ + "outDir": "./dist", + "declaration": true + } +}`, + ); + + await migrateProjectConfig(tmpDir); + + const result = JSON.parse(await readFile("tsconfig.server.json")); + expect(result.compilerOptions.outDir).toBeUndefined(); + expect(result.compilerOptions.noEmit).toBe(true); + }); + + test("preserves glob patterns in include paths", async () => { + _resetMigrationState(); + await writeFile( + "tsconfig.server.json", + `{ + "extends": "./tsconfig.shared.json", + "compilerOptions": { + "target": "ES2020", + "lib": ["ES2020"], + + /* Emit */ + "outDir": "./dist", + "rootDir": "./", + "declaration": true, + "declarationMap": true, + "sourceMap": true + }, + "include": ["server/**/*", "shared/**/*", "config/**/*"], + "exclude": ["node_modules", "dist", "client"] +}`, + ); + + await migrateProjectConfig(tmpDir); + + const result = JSON.parse(await readFile("tsconfig.server.json")); + expect(result.include).toEqual([ + "server/**/*", + "shared/**/*", + "config/**/*", + ]); + expect(result.compilerOptions.outDir).toBeUndefined(); + expect(result.compilerOptions.noEmit).toBe(true); + }); + }); + + // ── package.json ──────────────────────────────────────────────────── + + describe("package.json", () => { + test("replaces old build:server and typecheck scripts", async () => { + await writeFile( + "package.json", + JSON.stringify( + { + name: "test-app", + scripts: { + "build:server": "tsdown -c tsdown.server.config.ts", + typecheck: + "tsc -p ./tsconfig.server.json --noEmit && tsc -p ./tsconfig.client.json --noEmit", + }, + }, + null, + 2, + ), + ); + + await migrateProjectConfig(tmpDir); + + const result = JSON.parse(await readFile("package.json")); + expect(result.scripts["build:server"]).toBe( + "tsc -b tsconfig.server.json && tsdown -c tsdown.server.config.ts", + ); + expect(result.scripts.typecheck).toBe( + "tsc -b tsconfig.server.json && tsc -b tsconfig.client.json", + ); + }); + + test("no-op if scripts already match new values", async () => { + const original = JSON.stringify( + { + name: "test-app", + scripts: { + "build:server": + "tsc -b tsconfig.server.json && tsdown -c tsdown.server.config.ts", + typecheck: + "tsc -b tsconfig.server.json && tsc -b tsconfig.client.json", + }, + }, + null, + 2, + ); + await writeFile("package.json", original); + + await migrateProjectConfig(tmpDir); + + const result = await readFile("package.json"); + expect(result).toBe(original); + }); + + test("skips custom scripts that don't match old values", async () => { + const original = JSON.stringify( + { + name: "test-app", + scripts: { + "build:server": "my-custom-build-script", + typecheck: "my-custom-typecheck", + }, + }, + null, + 2, + ); + await writeFile("package.json", original); + + await migrateProjectConfig(tmpDir); + + const result = await readFile("package.json"); + expect(result).toBe(original); + }); + + test("preserves 4-space indent", async () => { + await writeFile( + "package.json", + JSON.stringify( + { + name: "test-app", + scripts: { + "build:server": "tsdown -c tsdown.server.config.ts", + }, + }, + null, + 4, + ), + ); + + await migrateProjectConfig(tmpDir); + + const raw = await readFile("package.json"); + // Should use 4-space indent + expect(raw).toContain(' "name"'); + }); + }); + + // ── Edge cases ────────────────────────────────────────────────────── + + test("does not crash when no config files exist", async () => { + await expect(migrateProjectConfig(tmpDir)).resolves.not.toThrow(); + }); + + test("runs only once per project root (dedup)", async () => { + await writeFile( + "tsconfig.client.json", + JSON.stringify({ include: ["client/src"] }, null, 2), + ); + + await migrateProjectConfig(tmpDir); + + // First call should have migrated + let result = JSON.parse(await readFile("tsconfig.client.json")); + expect(result.include).toContain("shared/appkit-types"); + + // Revert the file manually + await writeFile( + "tsconfig.client.json", + JSON.stringify({ include: ["client/src"] }, null, 2), + ); + + // Second call with same projectRoot should be a no-op (already migrated) + await migrateProjectConfig(tmpDir); + result = JSON.parse(await readFile("tsconfig.client.json")); + expect(result.include).not.toContain("shared/appkit-types"); + }); + + test("migrates different project roots independently", async () => { + const tmpDir2 = await fsp.mkdtemp( + path.join(os.tmpdir(), "appkit-migration-test2-"), + ); + + try { + // Set up both projects + await writeFile( + "tsconfig.client.json", + JSON.stringify({ include: ["client/src"] }, null, 2), + ); + await fsp.writeFile( + path.join(tmpDir2, "tsconfig.client.json"), + JSON.stringify({ include: ["client/src"] }, null, 2), + "utf-8", + ); + + // Migrate first project + await migrateProjectConfig(tmpDir); + + // Second project should still migrate independently + await migrateProjectConfig(tmpDir2); + + const result1 = JSON.parse(await readFile("tsconfig.client.json")); + const result2 = JSON.parse( + await fsp.readFile(path.join(tmpDir2, "tsconfig.client.json"), "utf-8"), + ); + expect(result1.include).toContain("shared/appkit-types"); + expect(result2.include).toContain("shared/appkit-types"); + } finally { + await fsp.rm(tmpDir2, { recursive: true, force: true }); + } + }); + + test("respects appkit.autoMigrate: false opt-out", async () => { + await writeFile( + "package.json", + JSON.stringify( + { + name: "test-app", + appkit: { autoMigrate: false }, + scripts: { + "build:server": "tsdown -c tsdown.server.config.ts", + }, + }, + null, + 2, + ), + ); + await writeFile( + "tsconfig.client.json", + JSON.stringify({ include: ["client/src"] }, null, 2), + ); + + await migrateProjectConfig(tmpDir); + + // tsconfig should NOT be modified + const tsconfig = JSON.parse(await readFile("tsconfig.client.json")); + expect(tsconfig.include).toEqual(["client/src"]); + + // package.json scripts should NOT be modified + const pkg = JSON.parse(await readFile("package.json")); + expect(pkg.scripts["build:server"]).toBe( + "tsdown -c tsdown.server.config.ts", + ); + }); +}); diff --git a/packages/appkit/src/type-generator/vite-plugin.ts b/packages/appkit/src/type-generator/vite-plugin.ts index 3eccb8ad4..5f4a0d4b1 100644 --- a/packages/appkit/src/type-generator/vite-plugin.ts +++ b/packages/appkit/src/type-generator/vite-plugin.ts @@ -27,7 +27,6 @@ interface AppKitTypesPluginOptions { * @returns Vite plugin to generate types for AppKit queries. */ export function appKitTypesPlugin(options?: AppKitTypesPluginOptions): Plugin { - let root: string; let outFile: string; let watchFolders: string[]; @@ -74,10 +73,10 @@ export function appKitTypesPlugin(options?: AppKitTypesPluginOptions): Plugin { }, configResolved(config) { - root = config.root; + const projectRoot = path.resolve(config.root, ".."); outFile = path.resolve( - root, - options?.outFile ?? `src/${TYPES_DIR}/${ANALYTICS_TYPES_FILE}`, + projectRoot, + options?.outFile ?? `shared/${TYPES_DIR}/${ANALYTICS_TYPES_FILE}`, ); watchFolders = options?.watchFolders ?? [ path.join(process.cwd(), "config", "queries"), diff --git a/packages/shared/src/cli/commands/generate-types.ts b/packages/shared/src/cli/commands/generate-types.ts index 6bb5a8bba..1be7c7e2e 100644 --- a/packages/shared/src/cli/commands/generate-types.ts +++ b/packages/shared/src/cli/commands/generate-types.ts @@ -24,7 +24,7 @@ async function runGenerateTypes( if (resolvedWarehouseId) { const resolvedOutFile = outFile || - path.join(process.cwd(), "client/src/appkit-types/analytics.d.ts"); + path.join(process.cwd(), "shared/appkit-types/analytics.d.ts"); const queryFolder = path.join(resolvedRootDir, "config/queries"); if (fs.existsSync(queryFolder)) { @@ -45,7 +45,7 @@ async function runGenerateTypes( // Generate serving endpoint types (no warehouse required) const servingOutFile = path.join( process.cwd(), - "client/src/appkit-types/serving.d.ts", + "shared/appkit-types/serving.d.ts", ); await typeGen.generateServingTypes({ outFile: servingOutFile, @@ -73,7 +73,7 @@ export const generateTypesCommand = new Command("generate-types") .argument( "[outFile]", "Output file path", - path.join(process.cwd(), "client/src/appkit-types/analytics.d.ts"), + path.join(process.cwd(), "shared/appkit-types/analytics.d.ts"), ) .argument("[warehouseId]", "Databricks warehouse ID") .option("--no-cache", "Disable caching for type generation") @@ -82,8 +82,8 @@ export const generateTypesCommand = new Command("generate-types") ` Examples: $ appkit generate-types - $ appkit generate-types . client/src/types.d.ts - $ appkit generate-types . client/src/types.d.ts my-warehouse-id + $ appkit generate-types . shared/appkit-types/analytics.d.ts + $ appkit generate-types . shared/appkit-types/analytics.d.ts my-warehouse-id $ appkit generate-types --no-cache`, ) .action(runGenerateTypes); diff --git a/template/_gitignore b/template/_gitignore index f2abc3263..23adbc24e 100644 --- a/template/_gitignore +++ b/template/_gitignore @@ -8,3 +8,6 @@ build/ .smoke-test/ test-results/ playwright-report/ + +# Auto-generated types (endpoint-specific, varies per developer) +shared/appkit-types/serving.d.ts diff --git a/template/package.json b/template/package.json index 136897dd8..584299afd 100644 --- a/template/package.json +++ b/template/package.json @@ -7,9 +7,9 @@ "start": "NODE_ENV=production node --env-file-if-exists=./.env ./dist/server.js", "dev": "NODE_ENV=development tsx watch --tsconfig ./tsconfig.server.json --env-file-if-exists=./.env ./server/server.ts", "build:client": "tsc -b tsconfig.client.json && vite build --config client/vite.config.ts", - "build:server": "tsdown -c tsdown.server.config.ts", + "build:server": "tsc -b tsconfig.server.json && tsdown -c tsdown.server.config.ts", "build": "npm run build:server && npm run build:client", - "typecheck": "tsc -p ./tsconfig.server.json --noEmit && tsc -p ./tsconfig.client.json --noEmit", + "typecheck": "tsc -b tsconfig.server.json && tsc -b tsconfig.client.json", "lint": "eslint .", "lint:fix": "eslint . --fix", "lint:ast-grep": "appkit lint", diff --git a/template/tsconfig.client.json b/template/tsconfig.client.json index 8732ba46b..a183fcac5 100644 --- a/template/tsconfig.client.json +++ b/template/tsconfig.client.json @@ -24,5 +24,5 @@ "@/*": ["./src/*"] } }, - "include": ["client/src"] + "include": ["client/src", "shared/appkit-types"] } diff --git a/template/tsconfig.server.json b/template/tsconfig.server.json index 8cdada22c..8e6ff1e5b 100644 --- a/template/tsconfig.server.json +++ b/template/tsconfig.server.json @@ -4,13 +4,8 @@ "tsBuildInfoFile": "./node_modules/.tmp/tsconfig.server.tsbuildinfo", "target": "ES2020", "lib": ["ES2020"], - - /* Emit */ - "outDir": "./dist", - "rootDir": "./", - "declaration": true, - "declarationMap": true, - "sourceMap": true + "noEmit": true, + "rootDir": "./" }, "include": ["server/**/*", "shared/**/*", "config/**/*"], "exclude": ["node_modules", "dist", "client"] From e0f1900d20e71284d629f1d67bfaef3ee001c10c Mon Sep 17 00:00:00 2001 From: "databricks-appkit[bot]" Date: Tue, 21 Apr 2026 11:30:56 +0000 Subject: [PATCH 10/14] chore: release v0.24.0 [skip ci] Signed-off-by: databricks-appkit[bot] --- CHANGELOG.md | 13 +++++++++++++ packages/appkit-ui/package.json | 2 +- packages/appkit/package.json | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87757a2f9..03a1e9216 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ All notable changes to this project will be documented in this file. # Changelog +# Changelog + +## [0.24.0](https://github.com/databricks/appkit/compare/v0.23.0...v0.24.0) (2026-04-20) + +* add AST extraction to serving type generator and move types to shared/ ([#279](https://github.com/databricks/appkit/issues/279)) ([422afb3](https://github.com/databricks/appkit/commit/422afb38aa73f8adb94e091225dc3381bd92cfcd)) +* ci on main ([#277](https://github.com/databricks/appkit/issues/277)) ([30a0a24](https://github.com/databricks/appkit/commit/30a0a248e0ab6bac50e8698e33eb5c46ca957aea)) +* add Vector Search plugin ([#200](https://github.com/databricks/appkit/issues/200)) ([279954e](https://github.com/databricks/appkit/commit/279954eca9a82c02af639aa006aa9f968bd60517)) + +### cli + +* **cli:** add non-interactive flags and help examples for agent readiness ([#252](https://github.com/databricks/appkit/issues/252)) ([f803a8b](https://github.com/databricks/appkit/commit/f803a8b445fba043186407af96f7402b57e8ff6e)) + + ## [0.23.0](https://github.com/databricks/appkit/compare/v0.22.0...v0.23.0) (2026-04-14) ### appkit diff --git a/packages/appkit-ui/package.json b/packages/appkit-ui/package.json index 793cf4d24..fa2953b1c 100644 --- a/packages/appkit-ui/package.json +++ b/packages/appkit-ui/package.json @@ -1,7 +1,7 @@ { "name": "@databricks/appkit-ui", "type": "module", - "version": "0.23.0", + "version": "0.24.0", "license": "Apache-2.0", "repository": { "type": "git", diff --git a/packages/appkit/package.json b/packages/appkit/package.json index f8ecf7cbc..4689791d3 100644 --- a/packages/appkit/package.json +++ b/packages/appkit/package.json @@ -1,7 +1,7 @@ { "name": "@databricks/appkit", "type": "module", - "version": "0.23.0", + "version": "0.24.0", "main": "./dist/index.js", "types": "./dist/index.d.ts", "packageManager": "pnpm@10.21.0", From 714401e54cca04687386922ebee23631692646a9 Mon Sep 17 00:00:00 2001 From: "databricks-appkit[bot]" Date: Tue, 21 Apr 2026 11:32:56 +0000 Subject: [PATCH 11/14] chore: sync template to v0.24.0 [skip ci] Signed-off-by: databricks-appkit[bot] --- template/package-lock.json | 16 ++++++++-------- template/package.json | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/template/package-lock.json b/template/package-lock.json index 3b22cc50a..6cee76d75 100644 --- a/template/package-lock.json +++ b/template/package-lock.json @@ -10,8 +10,8 @@ "hasInstallScript": true, "license": "Unlicensed", "dependencies": { - "@databricks/appkit": "0.23.0", - "@databricks/appkit-ui": "0.23.0", + "@databricks/appkit": "0.24.0", + "@databricks/appkit-ui": "0.24.0", "@databricks/sdk-experimental": "0.14.2", "clsx": "2.1.1", "embla-carousel-react": "8.6.0", @@ -558,9 +558,9 @@ } }, "node_modules/@databricks/appkit": { - "version": "0.23.0", - "resolved": "https://registry.npmjs.org/@databricks/appkit/-/appkit-0.23.0.tgz", - "integrity": "sha512-tm6TSiX9Qv1Sgw9x6Bjxs996l5VPJyUmfvL53mIR6ncs5Dm2I6QFMVXSrjEeW6BCIwTToajld2vnr9FJMgMg/w==", + "version": "0.24.0", + "resolved": "https://registry.npmjs.org/@databricks/appkit/-/appkit-0.24.0.tgz", + "integrity": "sha512-GvQFUbp6FPo0CVNHnHTyZOyAhnRbwkw1oTIB82TczGH4XawIT06TS66QezdEq/Lt+3d3XBCmBdQwa3Zp/93+BA==", "hasInstallScript": true, "license": "Apache-2.0", "dependencies": { @@ -601,9 +601,9 @@ } }, "node_modules/@databricks/appkit-ui": { - "version": "0.23.0", - "resolved": "https://registry.npmjs.org/@databricks/appkit-ui/-/appkit-ui-0.23.0.tgz", - "integrity": "sha512-UyA6su1dek+F65GIORQFMUVkidd6opddPQKgOOZ/CYn1CsezPPHCSX7XXqTnSGssnHP0yjVUs5qb1pxaT5KRrg==", + "version": "0.24.0", + "resolved": "https://registry.npmjs.org/@databricks/appkit-ui/-/appkit-ui-0.24.0.tgz", + "integrity": "sha512-UovTw4LF2n/+WLRxecSzZsI34Z6/iLOMIu51IUFEfJzLYOaON+OTFUMfxyWOlwWNjZsJIUdzld9rTRgaTckXhg==", "hasInstallScript": true, "license": "Apache-2.0", "dependencies": { diff --git a/template/package.json b/template/package.json index 584299afd..2ca08581b 100644 --- a/template/package.json +++ b/template/package.json @@ -32,8 +32,8 @@ "license": "Unlicensed", "description": "{{.appDescription}}", "dependencies": { - "@databricks/appkit": "0.23.0", - "@databricks/appkit-ui": "0.23.0", + "@databricks/appkit": "0.24.0", + "@databricks/appkit-ui": "0.24.0", "@databricks/sdk-experimental": "0.14.2", "clsx": "2.1.1", "embla-carousel-react": "8.6.0", From 86dedddf8d813b0b6fa919299235ec8e1caf58ec Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Mon, 27 Apr 2026 09:54:52 +0200 Subject: [PATCH 12/14] fix(appkit): address round-2 review blockers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six blockers from the round-2 multi-model debate: - Restore body-less generic route() overload — apps/dev-playground/ server/reconnect-plugin.ts uses `this.route(...)` with no body schema. Adding a `route(router, config & { body?: undefined })` overload (ordered first so explicit generics route there) lets the legacy call form keep compiling. - Strict discriminated-union check on validator results. A validator returning `{ issues: undefined }` or `{}` no longer falls through to the success path with `req.body = undefined`; it routes to the canonical 500 (VALIDATION_INTERNAL_ERROR) without calling the handler. - Bound vector-search filter sub-structures: keys ≤255, string values ≤1024, nested arrays ≤100 entries with each element bounded. Closes the round-1-deeper-layer DoS surface where 50-key filters could smuggle a 99KB string value past the schema cap. - Tighten analytics `parameters` value type to a constrained union of JSON primitives + the SQLTypeMarker shape. The marker schema is `.strict()` so callers can't smuggle extra fields; `value` is capped at 4096 chars. Both `sql.*(...)` markers and bare primitives are accepted; arbitrary nested objects are rejected. - Strong JSDoc warning on RouteConfig.exposeValidationErrors explaining that body validation runs BEFORE plugin-level auth (which lives inside the handler via asUser(req)), so enabling the flag in production exposes schema structure to anonymous attackers. Plus a new "When to opt into exposeValidationErrors" subsection in the plugin authoring docs. - Fix docs examples that used the now-redundant `` generic on this.route() — the schema-bearing overload infers TBody automatically. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- docs/docs/api/appkit/Class.Plugin.md | 44 ++-- docs/docs/plugins/custom-plugins.md | 24 +- packages/appkit/src/plugin/plugin.ts | 87 +++++-- .../src/plugin/tests/body-validation.test.ts | 180 +++++++++++++- .../appkit/src/plugins/analytics/analytics.ts | 46 +++- .../plugins/analytics/tests/analytics.test.ts | 229 ++++++++++++++++++ .../vector-search/tests/vector-search.test.ts | 28 +++ .../plugins/vector-search/vector-search.ts | 10 +- packages/shared/src/plugin.ts | 19 +- 9 files changed, 617 insertions(+), 50 deletions(-) diff --git a/docs/docs/api/appkit/Class.Plugin.md b/docs/docs/api/appkit/Class.Plugin.md index 41e065164..15505fb30 100644 --- a/docs/docs/api/appkit/Class.Plugin.md +++ b/docs/docs/api/appkit/Class.Plugin.md @@ -530,8 +530,8 @@ AuthenticationError in production when no user header is present. #### Call Signature ```ts -protected route(router: Router, config: RouteConfig> & { - body: TSchema; +protected route(router: Router, config: Omit, "body"> & { + body?: undefined; }): void; ``` @@ -542,10 +542,14 @@ uses of `TBody` — `body: StandardSchemaV1` and `handler: (req: IAppRequestWithBody, …)`. Without overloads, plugin authors can pass a schema whose output diverges from the declared handler body type; the compiler stays quiet and runtime -narrowing silently disagrees. The overloads below tie `TBody` to the -schema's `InferOutput` when `body` is present, so the handler always -sees the schema's real output type. When `body` is absent, `TBody` -resolves to `unknown` so handlers must narrow before use. +narrowing silently disagrees. + +The schema-bearing overload ties `TBody` to the schema's `InferOutput` +when `body` is present, so the handler always sees the schema's real +output type. The body-less overload keeps a free `TBody` generic for +backward compatibility with call sites that threaded a generic through +`route(...)` (historically that slot was abused for response-side +typing — `RouteConfig` default keeps this harmless). Note: `RouteConfig` default is load-bearing for backward compat with handlers typed as plain `express.Request`. DO NOT "fix" @@ -559,14 +563,14 @@ when both are separate type parameters on the same function. | Type Parameter | | ------ | -| `TSchema` *extends* `StandardSchemaV1`\<`unknown`, `any`\> | +| `TBody` | ##### Parameters | Parameter | Type | | ------ | ------ | | `router` | `Router` | -| `config` | `RouteConfig`\<`InferOutput`\<`TSchema`\>\> & \{ `body`: `TSchema`; \} | +| `config` | `Omit`\<`RouteConfig`\<`TBody`\>, `"body"`\> & \{ `body?`: `undefined`; \} | ##### Returns @@ -575,8 +579,8 @@ when both are separate type parameters on the same function. #### Call Signature ```ts -protected route(router: Router, config: Omit, "body"> & { - body?: undefined; +protected route(router: Router, config: RouteConfig> & { + body: TSchema; }): void; ``` @@ -587,10 +591,14 @@ uses of `TBody` — `body: StandardSchemaV1` and `handler: (req: IAppRequestWithBody, …)`. Without overloads, plugin authors can pass a schema whose output diverges from the declared handler body type; the compiler stays quiet and runtime -narrowing silently disagrees. The overloads below tie `TBody` to the -schema's `InferOutput` when `body` is present, so the handler always -sees the schema's real output type. When `body` is absent, `TBody` -resolves to `unknown` so handlers must narrow before use. +narrowing silently disagrees. + +The schema-bearing overload ties `TBody` to the schema's `InferOutput` +when `body` is present, so the handler always sees the schema's real +output type. The body-less overload keeps a free `TBody` generic for +backward compatibility with call sites that threaded a generic through +`route(...)` (historically that slot was abused for response-side +typing — `RouteConfig` default keeps this harmless). Note: `RouteConfig` default is load-bearing for backward compat with handlers typed as plain `express.Request`. DO NOT "fix" @@ -600,12 +608,18 @@ If you're confused why this needs overloads: TypeScript cannot otherwise enforce that `TBody` equals `StandardSchemaV1.InferOutput` when both are separate type parameters on the same function. +##### Type Parameters + +| Type Parameter | +| ------ | +| `TSchema` *extends* `StandardSchemaV1`\<`unknown`, `any`\> | + ##### Parameters | Parameter | Type | | ------ | ------ | | `router` | `Router` | -| `config` | `Omit`\<`RouteConfig`\<`unknown`\>, `"body"`\> & \{ `body?`: `undefined`; \} | +| `config` | `RouteConfig`\<`InferOutput`\<`TSchema`\>\> & \{ `body`: `TSchema`; \} | ##### Returns diff --git a/docs/docs/plugins/custom-plugins.md b/docs/docs/plugins/custom-plugins.md index 6cfe6eb54..4921775c1 100644 --- a/docs/docs/plugins/custom-plugins.md +++ b/docs/docs/plugins/custom-plugins.md @@ -140,7 +140,9 @@ type SendMessageBody = z.infer; class MyPlugin extends Plugin { injectRoutes(router) { - this.route(router, { + // TypeScript infers the body type from `sendMessageBody`'s output, + // so the explicit generic is unnecessary. + this.route(router, { name: "sendMessage", method: "post", path: "/messages", @@ -180,7 +182,7 @@ On validation failure the framework responds with: - `issues` is included when `NODE_ENV !== "production"`. In production the `issues` field is omitted by default to avoid leaking schema internals. Set `exposeValidationErrors: true` on the route config to opt into including `issues` in production responses. ```typescript -this.route(router, { +this.route(router, { name: "sendMessage", method: "post", path: "/messages", @@ -190,6 +192,24 @@ this.route(router, { }); ``` +### When to opt into `exposeValidationErrors` + +`exposeValidationErrors: true` surfaces the full Standard Schema `issues` array to clients in every environment, including production. That array contains field names, types, constraint messages, and any refinement text your schema carries. + +**Security caveat — read before enabling.** Body validation runs BEFORE plugin-level authentication. Plugins typically authenticate inside the handler (for example with `this.asUser(req)`), but the route's `body` schema is evaluated before the handler is ever invoked. If you set `exposeValidationErrors: true` on an otherwise-protected route, **anonymous attackers in production can enumerate your schema** by submitting crafted payloads and reading the `issues` field of the resulting 400s. They can discover: + +- every field name in your schema, +- the constraints on each field (`min(1)`, `max(4096)`, enum values, etc.), +- the exact message text you configured in refinements (often useful reconnaissance in its own right). + +For most server-side routes the right default is to leave `exposeValidationErrors` unset. The server-side log is always populated with the full `issues` array keyed by `requestId`, so operators retain full diagnostics even when clients receive only the canonical error envelope. + +Opt in only when: + +- The route is intentionally public and pre-auth (for example a newsletter sign-up or a status-page feedback form), and +- Field-level feedback is valuable to end users (clients can't easily guess how to fix their payload without it), and +- The schema itself is not sensitive (no internal-only field names, no validation messages that hint at backend internals). + ## Key extension points - **Route injection**: Implement `injectRoutes()` to add custom endpoints using [`IAppRouter`](../api/appkit/TypeAlias.IAppRouter.md) diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 5a0248431..7db55bd98 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -571,10 +571,14 @@ export abstract class Plugin< * `handler: (req: IAppRequestWithBody, …)`. Without overloads, * plugin authors can pass a schema whose output diverges from the * declared handler body type; the compiler stays quiet and runtime - * narrowing silently disagrees. The overloads below tie `TBody` to the - * schema's `InferOutput` when `body` is present, so the handler always - * sees the schema's real output type. When `body` is absent, `TBody` - * resolves to `unknown` so handlers must narrow before use. + * narrowing silently disagrees. + * + * The schema-bearing overload ties `TBody` to the schema's `InferOutput` + * when `body` is present, so the handler always sees the schema's real + * output type. The body-less overload keeps a free `TBody` generic for + * backward compatibility with call sites that threaded a generic through + * `route(...)` (historically that slot was abused for response-side + * typing — `RouteConfig` default keeps this harmless). * * Note: `RouteConfig` default is load-bearing for backward * compat with handlers typed as plain `express.Request`. DO NOT "fix" @@ -584,21 +588,21 @@ export abstract class Plugin< * enforce that `TBody` equals `StandardSchemaV1.InferOutput` * when both are separate type parameters on the same function. */ + protected route( + router: express.Router, + config: Omit, "body"> & { body?: undefined }, + ): void; protected route>( router: express.Router, config: RouteConfig> & { body: TSchema; }, ): void; - protected route( - router: express.Router, - config: Omit, "body"> & { body?: undefined }, - ): void; protected route( router: express.Router, config: | (RouteConfig & { body: StandardSchemaV1 }) - | (Omit, "body"> & { body?: undefined }), + | (Omit, "body"> & { body?: undefined }), ): void { const { name, method, path, handler } = config; @@ -677,13 +681,40 @@ export abstract class Plugin< return; } - if (result.issues) { + // Strict discriminated-union shape check per the Standard Schema + // spec: only `{ value }` (with no issues or an empty issues array) + // is success; only `{ issues: [nonEmpty] }` is failure; anything + // else is a validator programmer error (malformed shape) that must + // route to a canonical 500 — never the success path, which would + // otherwise let a `result.value === undefined` crash the handler. + const resultObject = + typeof result === "object" && result !== null + ? (result as { value?: unknown; issues?: unknown }) + : undefined; + const issuesField = resultObject?.issues; + const issuesArray: ReadonlyArray | undefined = + Array.isArray(issuesField) + ? (issuesField as ReadonlyArray) + : undefined; + const hasNonEmptyIssues = + issuesArray !== undefined && issuesArray.length > 0; + const hasValueField = + resultObject !== undefined && "value" in resultObject; + const isValidationFailure = hasNonEmptyIssues; + // Empty issues array counts as success per the review spec, even + // though no `value` is promised by the validator in that case. + const isSuccess = + !hasNonEmptyIssues && + (hasValueField || + (issuesArray !== undefined && issuesArray.length === 0)); + + if (isValidationFailure && issuesArray !== undefined) { const requestId = resolveRequestId(req); - const totalIssueCount = result.issues.length; + const totalIssueCount = issuesArray.length; const truncated = totalIssueCount > MAX_VALIDATION_ISSUES; const retained = truncated - ? result.issues.slice(0, MAX_VALIDATION_ISSUES) - : result.issues; + ? issuesArray.slice(0, MAX_VALIDATION_ISSUES) + : issuesArray; // Normalize Standard Schema path segments: spec allows either a // PropertyKey or an object with a `key` field. Callers expect a @@ -739,10 +770,36 @@ export abstract class Plugin< return; } - // Narrow req.body to the validated value. This preserves any + if (!isSuccess) { + // Validator returned a shape that is neither `{ value }` nor a + // non-empty `{ issues: [...] }` — e.g. `{ issues: undefined }`, + // `{}`, or `null`. Treat it as a validator programmer error and + // fail closed with a 500. Never invoke the handler; never mutate + // `req.body`. + const requestId = resolveRequestId(req); + logger.error("validation schema returned malformed result", { + plugin: this.name, + requestId, + }); + res.status(500).json({ + error: "Internal validation error", + code: "VALIDATION_INTERNAL_ERROR", + requestId, + }); + return; + } + + // Success. Narrow req.body to the validated value when the result + // carries one; for the rare `{ issues: [] }` shape (empty issues = + // no issues found, per the review spec), leave `req.body` as-is so + // the handler sees the original body. This preserves any // transformation performed by the schema (e.g. coercion), though // v1 docs advise against relying on transforms. - (req as { body: unknown }).body = result.value; + if (hasValueField) { + (req as { body: unknown }).body = ( + resultObject as { value: TBody } + ).value; + } await handler(req as IAppRequestWithBody, res); }; diff --git a/packages/appkit/src/plugin/tests/body-validation.test.ts b/packages/appkit/src/plugin/tests/body-validation.test.ts index b334a9fed..a5844a421 100644 --- a/packages/appkit/src/plugin/tests/body-validation.test.ts +++ b/packages/appkit/src/plugin/tests/body-validation.test.ts @@ -59,21 +59,21 @@ class TestPlugin extends Plugin { // Expose protected route() for testing. Overload signatures mirror the // real `route()` so callers get the same TBody inference rules as // plugin authors see in production. + public exposedRoute( + router: express.Router, + config: Omit, "body"> & { body?: undefined }, + ): void; public exposedRoute>( router: express.Router, config: RouteConfig> & { body: TSchema; }, ): void; - public exposedRoute( - router: express.Router, - config: Omit, "body"> & { body?: undefined }, - ): void; public exposedRoute( router: express.Router, config: | (RouteConfig & { body: StandardSchemaV1 }) - | (Omit, "body"> & { body?: undefined }), + | (Omit, "body"> & { body?: undefined }), ): void { // biome-ignore lint/complexity/useLiteralKeys: calling protected member from subclass this["route"](router, config as any); @@ -684,4 +684,174 @@ describe("route body validation", () => { }), ).toThrow(/Standard Schema v1 compliant/); }); + + // Malformed validator results must not slip into the success path. + // These tests cover schemas that return shapes outside the Standard + // Schema discriminated union (neither a clean `{ value }` success nor a + // `{ issues: [nonEmpty] }` failure). A loose `if (result.issues)` check + // would misroute these and let `result.value === undefined` reach the + // handler; the wrapper must fail closed with a canonical 500. + test("malformed: returns canonical 500 when validator returns { issues: undefined }", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const malformed: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + // Intentionally malformed: `issues` present but undefined, no `value`. + validate: () => ({ issues: undefined }) as any, + }, + }; + + const handlerSpy = vi.fn(); + + plugin.exposedRoute(router, { + name: "bad", + method: "post", + path: "/bad", + body: malformed, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/bad"); + const originalBody = { sentinel: "should-not-be-mutated" }; + const req = createMockRequest({ body: originalBody }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + // req.body must not be mutated into the malformed value. + expect(req.body).toBe(originalBody); + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Internal validation error", + code: "VALIDATION_INTERNAL_ERROR", + requestId: expect.any(String), + }), + ); + }); + + test("malformed: returns canonical 500 when validator returns {}", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const malformed: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + // Intentionally malformed: no `value`, no `issues`. + validate: () => ({}) as any, + }, + }; + + const handlerSpy = vi.fn(); + + plugin.exposedRoute(router, { + name: "bad", + method: "post", + path: "/bad", + body: malformed, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/bad"); + const originalBody = { sentinel: "should-not-be-mutated" }; + const req = createMockRequest({ body: originalBody }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(req.body).toBe(originalBody); + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Internal validation error", + code: "VALIDATION_INTERNAL_ERROR", + requestId: expect.any(String), + }), + ); + }); + + test("empty issues array counts as success: handler is called", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const emptyIssues: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + // Empty issues array is treated as success — "no issues found". + validate: () => ({ issues: [] }) as any, + }, + }; + + const handlerSpy = vi.fn(async (_req: any, res: any) => { + res.status(200).json({ ok: true }); + }); + + plugin.exposedRoute(router, { + name: "good", + method: "post", + path: "/good", + body: emptyIssues, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/good"); + const req = createMockRequest({ body: { any: "thing" } }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).toHaveBeenCalledTimes(1); + expect(res.status).toHaveBeenCalledWith(200); + }); + + test("non-empty issues array is a validation failure (canonical 400)", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const failing: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + validate: () => ({ + issues: [{ message: "nope", path: ["field"] }], + }), + }, + }; + + const handlerSpy = vi.fn(); + + plugin.exposedRoute(router, { + name: "fail", + method: "post", + path: "/fail", + body: failing, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/fail"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + }); }); diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index 55ceae323..6b38276f6 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -22,13 +22,40 @@ import type { IAnalyticsConfig } from "./types"; * Request body for POST /query/:query_key. Validated via Standard Schema * (Zod natively implements `~standard` from v3.24+). The `format` field * defaults to "JSON" via the schema so the handler sees a fully-populated - * body with no manual fallback needed. `parameters` accepts any value - * shape — per-query parameter schemas are the application's concern, not - * the plugin's. + * body with no manual fallback needed. + * + * `parameters` accepts both JSON primitives (string, number, boolean, + * null) AND `SQLTypeMarker` objects produced by `sql.string()`, + * `sql.number()`, `sql.date()`, `sql.timestamp()`, `sql.boolean()`. The + * marker shape is `{ __sql_type, value }` and its `value` field is capped + * at 4096 characters. `.strict()` on the marker schema rejects unknown + * fields so callers can't smuggle additional keys past validation. + * + * Per-query parameter shape validation remains the application's concern; + * this schema is the minimum safety net the plugin enforces for every + * route — it caps total key count, value sizes, and rejects malformed + * markers up front so megabyte-scale payloads never reach the query + * processor. */ +const sqlTypeMarkerSchema = z + .object({ + __sql_type: z.enum(["STRING", "NUMERIC", "BOOLEAN", "DATE", "TIMESTAMP"]), + value: z.string().max(4096), + }) + .strict(); + const queryBodySchema = z.object({ parameters: z - .record(z.string(), z.unknown()) + .record( + z.string().max(255), + z.union([ + z.string().max(4096), + z.number(), + z.boolean(), + z.null(), + sqlTypeMarkerSchema, + ]), + ) .refine((obj) => Object.keys(obj).length <= 100, { message: "parameters may not contain more than 100 keys", }) @@ -208,8 +235,15 @@ export class AnalyticsPlugin extends Plugin { await executor.executeStream( res, async (signal) => { - // The body schema validates shape only; per-value SQL type checks - // happen inside QueryProcessor via isSQLTypeMarker. + // Body schema accepts string | number | boolean | null | + // SQLTypeMarker. `QueryProcessor.processQueryParams` is typed + // narrower — `Record` + // — so we cast the validated input down to that shape. The + // processor's `isSQLTypeMarker` guard re-validates each value + // before trusting it: primitives reaching the processor today + // surface as a runtime ValidationError there. Bridging primitives + // to markers (or rejecting them at the handler) is a separate + // concern; see the corresponding test. const processedParams = await this.queryProcessor.processQueryParams( query, parameters as Record, diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index 9be6c1396..4c296b1b4 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -222,6 +222,235 @@ describe("Analytics Plugin", () => { expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); }); + test("/query/:query_key should return canonical 400 when a parameter value is an array", async () => { + // The body schema restricts parameter values to JSON primitives + // (string, number, boolean, null) or SQLTypeMarker objects. Arrays + // are neither — they have to be rejected at the HTTP boundary so + // oversized nested payloads never reach the query processor. + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { parameters: { key: ["array", "value"] } }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(executeMock).not.toHaveBeenCalled(); + expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); + }); + + test("/query/:query_key should accept SQLTypeMarker values in parameters", async () => { + // The body schema accepts SQLTypeMarker objects (sql.string(), + // sql.number(), etc.) at value position alongside JSON primitives. + // The handler must run, the marker must pass through to the query + // processor, and the SQL parameter list must reflect the marker's + // type. + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ + query: "SELECT * FROM users WHERE id = :user_id", + isAsUser: false, + }); + + const executeMock = vi.fn().mockResolvedValue({ + result: { data: [{ id: 5 }] }, + }); + (plugin as any).SQLClient.executeStatement = executeMock; + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "user_lookup" }, + body: { parameters: { user_id: sql.number(5) } }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + // Handler reaches SQL execution — marker passes through validation. + expect(executeMock).toHaveBeenCalledTimes(1); + expect(executeMock).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + statement: "SELECT * FROM users WHERE id = :user_id", + parameters: expect.arrayContaining([ + expect.objectContaining({ + name: "user_id", + value: "5", + type: "NUMERIC", + }), + ]), + }), + expect.any(AbortSignal), + ); + }); + + test("/query/:query_key should return canonical 400 for malformed marker (unknown __sql_type)", async () => { + // Markers with an unknown discriminant value must be rejected by + // the schema. The handler body must never run. + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { + parameters: { user_id: { __sql_type: "NOT_A_TYPE", value: "5" } }, + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(executeMock).not.toHaveBeenCalled(); + expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); + }); + + test("/query/:query_key should return canonical 400 for marker with extra unknown field", async () => { + // The marker schema is `.strict()` — unknown fields must be + // rejected so callers can't smuggle additional keys past + // validation. + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { + parameters: { + user_id: { __sql_type: "STRING", value: "x", evil: true }, + }, + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(executeMock).not.toHaveBeenCalled(); + expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); + }); + + test("/query/:query_key should return canonical 400 for oversized marker value", async () => { + // Marker `value` is capped at 4096 characters. Oversized payloads + // must be rejected at the HTTP boundary before the query processor + // ever sees them. + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { + parameters: { + big: { __sql_type: "STRING", value: "a".repeat(4097) }, + }, + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(executeMock).not.toHaveBeenCalled(); + expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); + }); + + test("/query/:query_key should accept primitive values in parameters", async () => { + // JSON primitives (string, number, boolean, null) must continue to + // pass through validation alongside SQLTypeMarker shapes. + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + (plugin as any).app.getAppQuery = vi.fn().mockResolvedValue({ + query: "SELECT 1", + isAsUser: false, + }); + + const executeMock = vi.fn().mockResolvedValue({ + result: { data: [{ id: 1 }] }, + }); + (plugin as any).SQLClient.executeStatement = executeMock; + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { + parameters: { + str: "hello", + num: 42, + bool: true, + nullish: null, + }, + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + // Validation lets the request through; the handler runs and tries + // to dispatch to executeStream. + expect((plugin as any).app.getAppQuery).toHaveBeenCalledTimes(1); + }); + test("/query/:query_key should apply format default of JSON when omitted", async () => { const plugin = new AnalyticsPlugin(config); const { router, getHandler } = createMockRouter(); diff --git a/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts b/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts index 6b659356e..00e5ec848 100644 --- a/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts +++ b/packages/appkit/src/plugins/vector-search/tests/vector-search.test.ts @@ -475,6 +475,34 @@ describe("VectorSearchPlugin", () => { expect(mockRes.status).toHaveBeenCalledWith(400); expect(mockRequest).not.toHaveBeenCalled(); }); + + it("rejects filter string value longer than 1024 characters", async () => { + const plugin = buildPlugin(); + await plugin.setup(); + const { router, getHandler } = createMockRouter(); + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/:alias/query"); + const mockReq = createMockRequest({ + params: { alias: "products" }, + body: { + queryText: "hello", + filters: { category: "a".repeat(1025) }, + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(mockRequest).not.toHaveBeenCalled(); + }); }); describe("POST /:alias/next-page", () => { diff --git a/packages/appkit/src/plugins/vector-search/vector-search.ts b/packages/appkit/src/plugins/vector-search/vector-search.ts index 183851070..770be97e6 100644 --- a/packages/appkit/src/plugins/vector-search/vector-search.ts +++ b/packages/appkit/src/plugins/vector-search/vector-search.ts @@ -30,12 +30,16 @@ import type { */ const searchFiltersSchema = z .record( - z.string(), + // Bound keys and string/array values so a client cannot send a + // megabyte-scale filter payload or balloon a single string entry. + z + .string() + .max(255), z.union([ - z.string(), + z.string().max(1024), z.number(), z.boolean(), - z.array(z.union([z.string(), z.number()])), + z.array(z.union([z.string().max(1024), z.number()])).max(100), ]), ) .refine((obj) => Object.keys(obj).length <= 50, { diff --git a/packages/shared/src/plugin.ts b/packages/shared/src/plugin.ts index 1156dccd8..495896b9f 100644 --- a/packages/shared/src/plugin.ts +++ b/packages/shared/src/plugin.ts @@ -252,10 +252,21 @@ export type RouteConfig = { */ body?: StandardSchemaV1; /** - * When true, the canonical 400 response includes the full `issues` array - * even in production (default: only included when `NODE_ENV !== "production"`). - * Use sparingly — surfacing detailed validation issues in production can - * leak schema internals to clients. + * When `true`, validation-failure responses include the Standard Schema + * `issues` array in all environments (including production). Default: + * issues are only included when `NODE_ENV !== "production"`. + * + * Security warning: Body validation runs BEFORE plugin-level + * authentication (which typically lives inside the handler via + * `asUser(req)`). Setting this flag to `true` therefore exposes your + * schema structure — field names, types, constraint messages, + * refinement text — to anonymous callers in production. Attackers can + * submit crafted payloads pre-auth and enumerate the schema shape from + * the responses. + * + * Only enable on routes that are intentionally public, or where the + * schema shape is not sensitive (e.g. a public contact form). When in + * doubt, leave unset. */ exposeValidationErrors?: boolean; }; From 0e72fecfe93019be92473a60a06b0a2a92952c06 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Mon, 27 Apr 2026 10:01:10 +0200 Subject: [PATCH 13/14] refactor(appkit): unify requestId sanitizer and tighten allowlist Three small cleanups from the round-2 review's "judgment call" tier, plus a sanitizer-sharing refactor that fixes a real correlation drift: - Lift `sanitizeRequestId` into a shared module (`packages/appkit/src/logging/request-id.ts`). Both `Plugin.route()`'s `resolveRequestId` and the wide-event logger now share one allowlist + cap. Previously the two disagreed on what counted as a valid `x-request-id`, so a client-visible requestId from a 400 response could not be matched against the server-side wide-event log. - Unified regex: `/^[A-Za-z0-9][A-Za-z0-9_.-]{0,127}$/`. Accepts dots (W3C traceparent compatibility), requires alphanumeric start (preventing `--rf`-style values from being misinterpreted as flags if a requestId ever flows into a shell pipeline), max length 128. - Widen fallback ID entropy from 32 bits (`randomUUID().slice(0, 8)`) to 64 bits (`.slice(0, 16)`). Birthday-collision boundary moves from ~65k IDs to ~4B. - Delete the misleading "AppKitError-compatible path so sensitive values inside context are redacted" comment in the validator-throw catch path. The logger has no redactor; only `errorName` is logged today, but the comment was a trap for future maintainers. Replaced with "no redactor; do not pass user-supplied values." Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/logging/logger.ts | 22 ++-- packages/appkit/src/logging/request-id.ts | 43 ++++++ .../src/logging/tests/request-id.test.ts | 64 +++++++++ packages/appkit/src/plugin/plugin.ts | 37 +++--- .../src/plugin/tests/body-validation.test.ts | 123 +++++++++++++++++- 5 files changed, 256 insertions(+), 33 deletions(-) create mode 100644 packages/appkit/src/logging/request-id.ts create mode 100644 packages/appkit/src/logging/tests/request-id.test.ts diff --git a/packages/appkit/src/logging/logger.ts b/packages/appkit/src/logging/logger.ts index 33aeecaf0..7902502ca 100644 --- a/packages/appkit/src/logging/logger.ts +++ b/packages/appkit/src/logging/logger.ts @@ -3,6 +3,7 @@ import { format } from "node:util"; import { trace } from "@opentelemetry/api"; import type { NextFunction, Request, Response } from "express"; import { createDebug as createObug } from "obug"; +import { sanitizeRequestId } from "./request-id"; import { DEFAULT_SAMPLING_CONFIG, shouldSample } from "./sampling"; import { WideEvent } from "./wide-event"; import { WideEventEmitter } from "./wide-event-emitter"; @@ -40,18 +41,15 @@ const eventsByRequest = new WeakMap(); // Global emitter instance const emitter = new WideEventEmitter(); -const MAX_REQUEST_ID_LENGTH = 128; - -/** - * Sanitize a request ID from user headers - */ -function sanitizeRequestId(id: string): string { - const sanitized = id.replace(/[^a-zA-Z0-9_.-]/g, ""); - return sanitized.slice(0, MAX_REQUEST_ID_LENGTH); -} - /** - * Generate a request ID from the request + * Generate a request ID from the request. + * + * Prefers a client-supplied correlation header (`x-request-id`, + * `x-correlation-id`, or `x-amzn-trace-id`) when it passes the shared + * {@link sanitizeRequestId} allowlist; otherwise generates a fresh + * fallback. The same allowlist is used by `plugin.ts`'s + * `resolveRequestId` so the wide-event log's `request_id` matches the + * value echoed in any canonical 4xx response. */ function generateRequestId(req: Request): string { const existingId = @@ -61,7 +59,7 @@ function generateRequestId(req: Request): string { if (existingId && typeof existingId === "string" && existingId.length > 0) { const sanitized = sanitizeRequestId(existingId); - if (sanitized.length > 0) { + if (sanitized !== undefined) { return sanitized; } } diff --git a/packages/appkit/src/logging/request-id.ts b/packages/appkit/src/logging/request-id.ts new file mode 100644 index 000000000..9b3d4dbc2 --- /dev/null +++ b/packages/appkit/src/logging/request-id.ts @@ -0,0 +1,43 @@ +/** + * Shared request ID helpers used by both the plugin body-validation + * wrapper (`plugin.ts`) and the wide-event logger (`logger.ts`) so they + * agree on what a "valid" client-supplied `x-request-id` header looks + * like. + * + * A single allowlist keeps the two paths in sync: when a request hits a + * validation failure, the `requestId` echoed in the canonical 400 + * response matches the `request_id` recorded in the wide-event log, + * letting operators correlate a client-visible 4xx with the full + * server-side issue trace. + */ + +/** Maximum length of a client-supplied request ID after sanitization. */ +export const MAX_REQUEST_ID_LENGTH = 128; + +/** + * Strict allowlist for incoming `x-request-id` (and equivalent + * correlation) headers. The first character must be alphanumeric so + * values starting with `-`, `_`, or `.` cannot be misinterpreted as + * flags if the requestId ever flows into a shell pipeline an operator + * runs. Subsequent characters allow URL-safe ASCII plus underscore, + * dot, and hyphen. Capped at {@link MAX_REQUEST_ID_LENGTH} characters + * total so client-supplied values can never contain CRLF + * (log-injection / CWE-117) or blow up server memory. + */ +export const REQUEST_ID_PATTERN = /^[A-Za-z0-9][A-Za-z0-9_.-]{0,127}$/; + +/** + * Validate a client-supplied request ID against the canonical allowlist. + * + * Returns the value unchanged if it matches; returns `undefined` + * otherwise. Callers should generate a fresh fallback when this + * returns `undefined` rather than attempting any normalization — the + * helper is intentionally fail-closed so that malformed or + * attacker-controlled values never leak into logs or response bodies. + */ +export function sanitizeRequestId(id: string): string | undefined { + if (REQUEST_ID_PATTERN.test(id)) { + return id; + } + return undefined; +} diff --git a/packages/appkit/src/logging/tests/request-id.test.ts b/packages/appkit/src/logging/tests/request-id.test.ts new file mode 100644 index 000000000..a9dcb00e7 --- /dev/null +++ b/packages/appkit/src/logging/tests/request-id.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, test } from "vitest"; +import { + MAX_REQUEST_ID_LENGTH, + REQUEST_ID_PATTERN, + sanitizeRequestId, +} from "../request-id"; + +describe("sanitizeRequestId", () => { + test("accepts simple alphanumeric IDs", () => { + expect(sanitizeRequestId("abc123")).toBe("abc123"); + }); + + test("accepts IDs with internal hyphens, underscores, and dots", () => { + expect(sanitizeRequestId("trace.abc-123_xyz")).toBe("trace.abc-123_xyz"); + }); + + test("accepts IDs at the maximum length", () => { + const id = "a".repeat(MAX_REQUEST_ID_LENGTH); + expect(sanitizeRequestId(id)).toBe(id); + }); + + test("rejects IDs over the maximum length", () => { + expect(sanitizeRequestId("a".repeat(MAX_REQUEST_ID_LENGTH + 1))).toBe( + undefined, + ); + }); + + test("rejects IDs starting with a dash (potential shell-flag confusion)", () => { + expect(sanitizeRequestId("-rf")).toBe(undefined); + expect(sanitizeRequestId("--help")).toBe(undefined); + }); + + test("rejects IDs starting with an underscore", () => { + expect(sanitizeRequestId("_internal")).toBe(undefined); + }); + + test("rejects IDs starting with a dot", () => { + expect(sanitizeRequestId(".bad")).toBe(undefined); + }); + + test("rejects empty string", () => { + expect(sanitizeRequestId("")).toBe(undefined); + }); + + test("rejects values with characters outside the allowlist", () => { + expect(sanitizeRequestId("abc def")).toBe(undefined); // space + expect(sanitizeRequestId("abc/def")).toBe(undefined); // slash + expect(sanitizeRequestId("abc:def")).toBe(undefined); // colon + }); + + test("rejects CRLF-injection attempts (CWE-117)", () => { + expect(sanitizeRequestId("attacker\r\nSet-Cookie: pwn=1")).toBe(undefined); + expect(sanitizeRequestId("a\nb")).toBe(undefined); + expect(sanitizeRequestId("a\rb")).toBe(undefined); + }); + + test("REQUEST_ID_PATTERN is exported and matches sanitizeRequestId", () => { + expect(REQUEST_ID_PATTERN.test("abc123")).toBe(true); + expect(REQUEST_ID_PATTERN.test(".bad")).toBe(false); + expect(REQUEST_ID_PATTERN.test("a".repeat(MAX_REQUEST_ID_LENGTH + 1))).toBe( + false, + ); + }); +}); diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 7db55bd98..33bb1b95a 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -24,6 +24,7 @@ import { } from "../context"; import { AppKitError, AuthenticationError } from "../errors"; import { createLogger } from "../logging/logger"; +import { sanitizeRequestId } from "../logging/request-id"; import { StreamManager } from "../stream"; import { type ITelemetry, @@ -58,33 +59,29 @@ function hasHttpStatusCode( ); } -/** - * Character allowlist for incoming `x-request-id` headers. Restricts to - * URL-safe ASCII + underscore/hyphen and caps length at 100 characters so - * client-supplied values can never contain CRLF (log-injection / CWE-117) - * or blow up server memory. - */ -const REQUEST_ID_HEADER_PATTERN = /^[A-Za-z0-9_-]{1,100}$/; - /** * Resolve a request ID from the `x-request-id` header (if present), falling * back to a freshly generated UUID-derived token. Used by the body * validation wrapper so operators can correlate a client-facing 400 with * the full server-side issue log. * - * The header value is validated against a strict allowlist. Invalid values - * are silently discarded — they are never logged or reflected anywhere — - * and a fresh ID is generated instead. + * The header value is validated against the shared {@link sanitizeRequestId} + * allowlist (also used by the wide-event logger), so a value accepted here + * matches the `request_id` recorded server-side. Invalid values are silently + * discarded — they are never logged or reflected anywhere — and a fresh ID + * is generated instead. The fallback uses 16 hex characters (~64 bits) so + * birthday collisions stay astronomically unlikely while keeping IDs short + * enough to skim in logs. */ function resolveRequestId(req: express.Request): string { const headerId = req.header("x-request-id"); - if ( - typeof headerId === "string" && - REQUEST_ID_HEADER_PATTERN.test(headerId) - ) { - return headerId; + if (typeof headerId === "string") { + const sanitized = sanitizeRequestId(headerId); + if (sanitized !== undefined) { + return sanitized; + } } - return `req_${randomUUID().slice(0, 8)}`; + return `req_${randomUUID().slice(0, 16)}`; } /** Maximum number of Standard Schema issues retained on a validation failure. */ @@ -665,9 +662,9 @@ export abstract class Plugin< maybePromise instanceof Promise ? await maybePromise : maybePromise; } catch (error) { const requestId = resolveRequestId(req); - // Log via AppKitError-compatible path so sensitive values inside - // `context` are redacted. The thrown error's free-form message is - // intentionally omitted to avoid leaking refinement internals. + // No redactor; do not pass user-supplied values. The thrown + // error's free-form message is intentionally omitted to avoid + // leaking refinement internals. logger.error("validation schema threw unexpectedly", { plugin: this.name, requestId, diff --git a/packages/appkit/src/plugin/tests/body-validation.test.ts b/packages/appkit/src/plugin/tests/body-validation.test.ts index a5844a421..f9b40f874 100644 --- a/packages/appkit/src/plugin/tests/body-validation.test.ts +++ b/packages/appkit/src/plugin/tests/body-validation.test.ts @@ -538,7 +538,8 @@ describe("route body validation", () => { }); const handler = getHandler("POST", "/messages"); - const longId = "a".repeat(101); + // 129 chars — one over the 128-char cap. + const longId = "a".repeat(129); const req = createMockRequest({ body: {}, headers: { "x-request-id": longId }, @@ -552,6 +553,126 @@ describe("route body validation", () => { expect(body.requestId).toMatch(/^req_[A-Fa-f0-9-]+$/); }); + test("accepts x-request-id with dots (unified allowlist)", async () => { + // Wide-event logger has always allowed dots; the validator wrapper + // now agrees so operators can grep both wide-events and 4xx + // responses with the same correlation token. + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: {}, + headers: { "x-request-id": "trace.abc-123" }, + }); + const res = createMockResponse(); + + await handler(req, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + requestId: "trace.abc-123", + }), + ); + }); + + test("rejects x-request-id starting with a dash", async () => { + // Leading `-`/`_`/`.` could be misinterpreted as a flag if the + // requestId ever flows into a shell pipeline an operator runs. + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: {}, + headers: { "x-request-id": "--rf" }, + }); + const res = createMockResponse(); + + await handler(req, res); + + const body = (res.json as any).mock.calls[0][0]; + expect(body.requestId).not.toBe("--rf"); + expect(body.requestId).toMatch(/^req_[A-Fa-f0-9-]+$/); + }); + + test("rejects x-request-id starting with a dot", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: {}, + headers: { "x-request-id": ".bad" }, + }); + const res = createMockResponse(); + + await handler(req, res); + + const body = (res.json as any).mock.calls[0][0]; + expect(body.requestId).not.toBe(".bad"); + expect(body.requestId).toMatch(/^req_[A-Fa-f0-9-]+$/); + }); + + test("generated fallback requestId has 16 hex chars of entropy", async () => { + // 64-bit entropy keeps birthday collisions astronomically unlikely + // (~4 billion IDs) while staying short enough to skim in logs. + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + // No x-request-id header → wrapper generates the fallback. + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + const body = (res.json as any).mock.calls[0][0]; + // randomUUID().slice(0, 16) is `xxxxxxxx-xxxx-xx` — 14 hex chars + // plus 2 hyphens, all matching `[a-f0-9-]`, total length 16. + expect(body.requestId).toMatch(/^req_[a-f0-9-]{16}$/); + }); + test("truncates the issues array to 20 entries and flags issuesTruncated", async () => { process.env.NODE_ENV = "development"; const plugin = createTestPlugin(); From 02c5daaedec05a1072836a03fa2c37dca8871ccb Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Mon, 4 May 2026 23:13:43 +0200 Subject: [PATCH 14/14] fix(appkit): address round-3 review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the round-3 multi-model debate, plus follow-up hardening from a round-4 review. - Strict spec-compliant validator-result branching: any array-valued `issues` field is now classified as failure (per Standard Schema v1 spec — `FailureResult` permits empty `issues`). Round-2 incorrectly treated `{ issues: [] }` as success; this is now a canonical 400 with an empty `issues` array in dev / override responses. Mixed shapes `{ value, issues: [] }` also route to failure (issues present wins). Malformed shapes (`{}`, `{ issues: undefined }`, non-objects, AND `{ value, issues: }`) route to canonical 500 — the 500-gate is `hasIssuesField || !hasValueField` so a present-but- non-array `issues` cannot slip past `Array.isArray` into the success path. - Drop JSON primitives from analytics `parameters` schema. Primitives passed schema validation but failed late inside QueryProcessor via SSE — clients got an opaque stream error instead of the canonical 400. Schema now accepts only `SQLTypeMarker | null`. Callers must wrap raw values with `sql.string()`, `sql.number()`, etc. Cast on the validated body simplified accordingly. - Runtime warning at route registration when `exposeValidationErrors` is true and `NODE_ENV === "production"`. Operators see the schema-disclosure misconfiguration on first deploy instead of relying on JSDoc alone. - Shared `resolveRequestId(req)` helper in `logging/request-id.ts`, consulting headers in order: `x-request-id`, `x-correlation-id`, `x-amzn-trace-id`. Both `Plugin.route()` and the wide-event logger now use the same helper. The resolver memoizes per-request via a `WeakMap` so the headerless fallback path returns the same ID across the four call sites (logger.ts plus three branches in plugin.ts) — without memoization each call generated a fresh fallback and the wide-event `request_id` no longer matched the response `requestId`. The `x-amzn-trace-id` header now parses the `Root=` segment out of the AWS X-Ray format (`Root=1-...;Parent=...;Sampled=1`) before sanitizing, so AWS callers actually benefit from the consultation instead of falling through to a generated fallback. Generated fallbacks use `randomBytes(8).toString("hex")` for exactly 16 hex chars (~64 bits) — no embedded UUID hyphens, single allocation. Co-authored-by: Isaac Signed-off-by: Atila Fassina --- packages/appkit/src/logging/logger.ts | 30 +- packages/appkit/src/logging/request-id.ts | 127 ++++- .../logging/tests/logger-with-event.test.ts | 80 ++- .../src/logging/tests/request-id.test.ts | 193 +++++++ packages/appkit/src/plugin/plugin.ts | 121 ++--- .../src/plugin/tests/body-validation.test.ts | 497 +++++++++++++++++- .../appkit/src/plugins/analytics/analytics.ts | 46 +- .../plugins/analytics/tests/analytics.test.ts | 104 +++- 8 files changed, 1032 insertions(+), 166 deletions(-) diff --git a/packages/appkit/src/logging/logger.ts b/packages/appkit/src/logging/logger.ts index 7902502ca..31fe6b922 100644 --- a/packages/appkit/src/logging/logger.ts +++ b/packages/appkit/src/logging/logger.ts @@ -3,7 +3,7 @@ import { format } from "node:util"; import { trace } from "@opentelemetry/api"; import type { NextFunction, Request, Response } from "express"; import { createDebug as createObug } from "obug"; -import { sanitizeRequestId } from "./request-id"; +import { resolveRequestId } from "./request-id"; import { DEFAULT_SAMPLING_CONFIG, shouldSample } from "./sampling"; import { WideEvent } from "./wide-event"; import { WideEventEmitter } from "./wide-event-emitter"; @@ -41,37 +41,11 @@ const eventsByRequest = new WeakMap(); // Global emitter instance const emitter = new WideEventEmitter(); -/** - * Generate a request ID from the request. - * - * Prefers a client-supplied correlation header (`x-request-id`, - * `x-correlation-id`, or `x-amzn-trace-id`) when it passes the shared - * {@link sanitizeRequestId} allowlist; otherwise generates a fresh - * fallback. The same allowlist is used by `plugin.ts`'s - * `resolveRequestId` so the wide-event log's `request_id` matches the - * value echoed in any canonical 4xx response. - */ -function generateRequestId(req: Request): string { - const existingId = - req.headers["x-request-id"] || - req.headers["x-correlation-id"] || - req.headers["x-amzn-trace-id"]; - - if (existingId && typeof existingId === "string" && existingId.length > 0) { - const sanitized = sanitizeRequestId(existingId); - if (sanitized !== undefined) { - return sanitized; - } - } - - return `req_${Date.now()}_${Math.random().toString(36).substring(2, 9)}`; -} - /** * Create a WideEvent for a request */ function createEventForRequest(req: Request): WideEvent { - const requestId = generateRequestId(req); + const requestId = resolveRequestId(req); const wideEvent = new WideEvent(requestId); // extract path from request (strip query string) diff --git a/packages/appkit/src/logging/request-id.ts b/packages/appkit/src/logging/request-id.ts index 9b3d4dbc2..53e2c35f8 100644 --- a/packages/appkit/src/logging/request-id.ts +++ b/packages/appkit/src/logging/request-id.ts @@ -1,15 +1,54 @@ /** * Shared request ID helpers used by both the plugin body-validation * wrapper (`plugin.ts`) and the wide-event logger (`logger.ts`) so they - * agree on what a "valid" client-supplied `x-request-id` header looks - * like. - * - * A single allowlist keeps the two paths in sync: when a request hits a - * validation failure, the `requestId` echoed in the canonical 400 - * response matches the `request_id` recorded in the wide-event log, - * letting operators correlate a client-visible 4xx with the full - * server-side issue trace. + * agree on what a "valid" client-supplied correlation header looks + * like and which header(s) to consult. + * + * A single allowlist plus a single header lookup keeps the two paths in + * sync: when a request hits a validation failure, the `requestId` + * echoed in the canonical 400 response matches the `request_id` + * recorded in the wide-event log, letting operators correlate a + * client-visible 4xx with the full server-side issue trace. + */ + +import { randomBytes } from "node:crypto"; + +/** + * Per-request memoization of the resolved correlation ID. + * + * Multiple call sites (the wide-event logger and the body-validation + * wrapper's 400/500 paths) all call {@link resolveRequestId} on the same + * request. When no valid correlation header is present each call would + * otherwise generate a new random fallback — defeating the unification + * promised by this module. A `WeakMap` keyed on the request object + * caches the first resolution so all subsequent calls return the same + * ID. The map is `WeakMap`-keyed so entries are reclaimed when the + * request is garbage-collected. */ +const resolvedIdByRequest = new WeakMap(); + +/** + * Extract the `Root=...` segment from an AWS X-Ray header value. + * + * Real AWS X-Amzn-Trace-Id values look like + * `Root=1-5759e988-bd862e3fe1be46a994272793;Parent=...;Sampled=1`. The + * raw value contains `=` and `;` which are intentionally rejected by + * {@link REQUEST_ID_PATTERN}, so without this extractor the + * x-amzn-trace-id consultation in {@link resolveRequestId} would never + * succeed for production AWS traffic. The Root segment alone is + * allowlist-safe (alphanumeric + `-`). + * + * Returns `undefined` if no Root segment is found. + */ +function extractAwsTraceRoot(raw: string): string | undefined { + for (const segment of raw.split(";")) { + const trimmed = segment.trim(); + if (trimmed.startsWith("Root=")) { + return trimmed.slice("Root=".length); + } + } + return undefined; +} /** Maximum length of a client-supplied request ID after sanitization. */ export const MAX_REQUEST_ID_LENGTH = 128; @@ -26,6 +65,21 @@ export const MAX_REQUEST_ID_LENGTH = 128; */ export const REQUEST_ID_PATTERN = /^[A-Za-z0-9][A-Za-z0-9_.-]{0,127}$/; +/** + * Ordered list of headers consulted to derive a request correlation ID. + * + * The order matches the historical wide-event logger lookup so that any + * value the logger would have used for `request_id` is the same value + * the body-validation wrapper echoes back in canonical 4xx/5xx + * responses. Adding new headers here updates both call sites + * simultaneously. + */ +export const REQUEST_ID_HEADERS: ReadonlyArray = [ + "x-request-id", + "x-correlation-id", + "x-amzn-trace-id", +]; + /** * Validate a client-supplied request ID against the canonical allowlist. * @@ -41,3 +95,60 @@ export function sanitizeRequestId(id: string): string | undefined { } return undefined; } + +/** + * Resolve a request correlation ID by consulting + * {@link REQUEST_ID_HEADERS} in order, taking the first value that + * passes {@link sanitizeRequestId}. Returns a freshly generated fallback + * (`req_<16 hex chars>`) when no header is present or all values are + * malformed. + * + * The fallback uses 16 hex characters (~64 bits of entropy) so birthday + * collisions stay astronomically unlikely while keeping IDs short + * enough to skim in logs. Invalid header values are silently discarded + * — they are never logged or reflected anywhere — so attacker-supplied + * payloads cannot leak via this path. + * + * The result is memoized per-request: repeat calls on the same request + * object always return the same ID, so the wide-event logger's + * `request_id` and the body-validation wrapper's response `requestId` + * stay in lockstep even when no correlation header is supplied. + * + * The `x-amzn-trace-id` header is special-cased: real AWS values + * include `=` and `;`, which are rejected by the strict allowlist + * {@link REQUEST_ID_PATTERN}. The `Root=` segment is parsed out before + * sanitization so AWS-deployed callers actually benefit from the + * x-amzn-trace-id consultation. + */ +export function resolveRequestId(req: { + header(name: string): string | undefined; +}): string { + const cached = resolvedIdByRequest.get(req); + if (cached !== undefined) { + return cached; + } + + let resolved: string | undefined; + for (const headerName of REQUEST_ID_HEADERS) { + const raw = req.header(headerName); + if (typeof raw !== "string" || raw.length === 0) { + continue; + } + const candidate = + headerName === "x-amzn-trace-id" + ? (extractAwsTraceRoot(raw) ?? raw) + : raw; + const sanitized = sanitizeRequestId(candidate); + if (sanitized !== undefined) { + resolved = sanitized; + break; + } + } + + // 16 hex chars = 8 random bytes = ~64 bits of entropy. Generated + // directly so the result is exactly 16 hex chars (no embedded UUID + // hyphens) and avoids the allocate-full-UUID-then-slice waste. + const id = resolved ?? `req_${randomBytes(8).toString("hex")}`; + resolvedIdByRequest.set(req, id); + return id; +} diff --git a/packages/appkit/src/logging/tests/logger-with-event.test.ts b/packages/appkit/src/logging/tests/logger-with-event.test.ts index 022419e2a..c08e84299 100644 --- a/packages/appkit/src/logging/tests/logger-with-event.test.ts +++ b/packages/appkit/src/logging/tests/logger-with-event.test.ts @@ -1,6 +1,7 @@ import type { Request, Response } from "express"; import { beforeEach, describe, expect, test, vi } from "vitest"; import { createLogger } from "../logger"; +import { resolveRequestId } from "../request-id"; import { WideEvent } from "../wide-event"; describe("Logger with WideEvent Integration", () => { @@ -19,14 +20,19 @@ describe("Logger with WideEvent Integration", () => { }), }; + const headers: Record = { + "user-agent": "test-agent", + "x-forwarded-for": "127.0.0.1", + }; mockReq = { method: "POST", path: "/api/query", url: "/api/query", - headers: { - "user-agent": "test-agent", - "x-forwarded-for": "127.0.0.1", - }, + headers, + // The shared resolveRequestId helper consults req.header(name); the + // wide-event logger no longer uses raw headers[] for request-id + // lookup, so the mock must mirror what Express sets up at runtime. + header: vi.fn((name: string) => headers[name.toLowerCase()]) as any, res: mockRes as Response, }; }); @@ -228,4 +234,70 @@ describe("Logger with WideEvent Integration", () => { infoSpy.mockRestore(); }); }); + + // The wide-event logger and the body-validation wrapper must agree on + // the correlation ID for any single request. They share the same + // resolver helper so this is now a property check. + describe("Shared requestId resolver", () => { + test("WideEvent.request_id matches resolveRequestId() for the same request", () => { + const logger = createLogger("analytics"); + const headers: Record = { + "user-agent": "test-agent", + "x-amzn-trace-id": "Root.1-abc-def", + }; + const req: Partial = { + method: "POST", + path: "/api/query", + url: "/api/query", + headers, + header: vi.fn((name: string) => headers[name.toLowerCase()]) as any, + res: mockRes as Response, + }; + + const event = logger.event(req as Request); + // Both call sites must derive the same ID, so a 400 echoed + // requestId can be grepped against the wide-event log. + expect(event?.data.request_id).toBe(resolveRequestId(req as Request)); + expect(event?.data.request_id).toBe("Root.1-abc-def"); + }); + + test("falls back to x-amzn-trace-id when x-request-id is absent", () => { + const logger = createLogger("analytics"); + const headers: Record = { + "x-amzn-trace-id": "Root.1-abc-def", + }; + const req: Partial = { + method: "POST", + path: "/api/query", + url: "/api/query", + headers, + header: vi.fn((name: string) => headers[name.toLowerCase()]) as any, + res: mockRes as Response, + }; + + const event = logger.event(req as Request); + expect(event?.data.request_id).toBe("Root.1-abc-def"); + }); + + test("generated fallback uses 16 hex chars (~64 bits of entropy)", () => { + const logger = createLogger("analytics"); + const headers: Record = {}; + const req: Partial = { + method: "POST", + path: "/api/query", + url: "/api/query", + headers, + header: vi.fn((name: string) => headers[name.toLowerCase()]) as any, + res: mockRes as Response, + }; + + const event = logger.event(req as Request); + // The fallback formula was unified across plugin.ts and logger.ts — + // both now use `req_<16 hex chars>` generated via + // `randomBytes(8).toString("hex")` so the result is exactly 16 + // hex characters with no embedded UUID hyphens. The legacy + // `req__` form must no longer appear. + expect(event?.data.request_id).toMatch(/^req_[a-f0-9]{16}$/); + }); + }); }); diff --git a/packages/appkit/src/logging/tests/request-id.test.ts b/packages/appkit/src/logging/tests/request-id.test.ts index a9dcb00e7..dcdb8c120 100644 --- a/packages/appkit/src/logging/tests/request-id.test.ts +++ b/packages/appkit/src/logging/tests/request-id.test.ts @@ -1,10 +1,30 @@ import { describe, expect, test } from "vitest"; import { MAX_REQUEST_ID_LENGTH, + REQUEST_ID_HEADERS, REQUEST_ID_PATTERN, + resolveRequestId, sanitizeRequestId, } from "../request-id"; +/** + * Build a minimal stand-in for an Express Request that exposes the + * `header(name)` accessor `resolveRequestId` consults. Header lookup is + * case-insensitive (matching Express semantics) so callers can pass + * either canonical lowercase or arbitrary casing. + */ +function makeReq(headers: Record) { + const lower: Record = {}; + for (const [k, v] of Object.entries(headers)) { + lower[k.toLowerCase()] = v; + } + return { + header(name: string): string | undefined { + return lower[name.toLowerCase()]; + }, + }; +} + describe("sanitizeRequestId", () => { test("accepts simple alphanumeric IDs", () => { expect(sanitizeRequestId("abc123")).toBe("abc123"); @@ -62,3 +82,176 @@ describe("sanitizeRequestId", () => { ); }); }); + +describe("resolveRequestId", () => { + test("REQUEST_ID_HEADERS lists the canonical header order", () => { + // The order is load-bearing: it must match the wide-event logger's + // historical lookup so a single request produces one request_id + // across both the canonical 4xx response and the wide-event log. + expect(REQUEST_ID_HEADERS).toEqual([ + "x-request-id", + "x-correlation-id", + "x-amzn-trace-id", + ]); + }); + + test("returns sanitized x-request-id when present", () => { + const id = resolveRequestId(makeReq({ "x-request-id": "abc-123" })); + expect(id).toBe("abc-123"); + }); + + test("falls back to x-correlation-id when x-request-id is absent", () => { + const id = resolveRequestId(makeReq({ "x-correlation-id": "corr-1" })); + expect(id).toBe("corr-1"); + }); + + test("falls back to x-amzn-trace-id when earlier headers are absent", () => { + const id = resolveRequestId( + makeReq({ "x-amzn-trace-id": "Root.1-abc-def" }), + ); + expect(id).toBe("Root.1-abc-def"); + }); + + test("prefers x-request-id over later candidate headers", () => { + const id = resolveRequestId( + makeReq({ + "x-request-id": "primary-id", + "x-correlation-id": "secondary-id", + "x-amzn-trace-id": "tertiary-id", + }), + ); + expect(id).toBe("primary-id"); + }); + + test("skips a malformed earlier header and uses a valid later one", () => { + const id = resolveRequestId( + makeReq({ + "x-request-id": "-bad", + "x-correlation-id": "fallback-ok", + }), + ); + expect(id).toBe("fallback-ok"); + }); + + test("generates a fallback when no candidate headers are present", () => { + const id = resolveRequestId(makeReq({})); + expect(id).toMatch(/^req_[a-f0-9]{16}$/); + }); + + test("generates a fallback when all candidate headers are malformed", () => { + const id = resolveRequestId( + makeReq({ + "x-request-id": "-bad", + "x-correlation-id": ".also-bad", + "x-amzn-trace-id": "_nope", + }), + ); + expect(id).toMatch(/^req_[a-f0-9]{16}$/); + }); + + test("ignores empty header values", () => { + const id = resolveRequestId( + makeReq({ + "x-request-id": "", + "x-correlation-id": "fallback-ok", + }), + ); + expect(id).toBe("fallback-ok"); + }); + + // The wide-event logger and the body-validation wrapper both call + // `resolveRequestId(req)` independently on the same request. When no + // valid correlation header is present the fallback used to be + // generated fresh on each call, producing two different IDs and + // breaking log/response correlation. The resolver memoizes per + // request object so all callers see the same value. + describe("memoization", () => { + test("returns the same fallback ID across repeated calls on one request", () => { + const req = makeReq({}); + const a = resolveRequestId(req); + const b = resolveRequestId(req); + const c = resolveRequestId(req); + expect(a).toMatch(/^req_[a-f0-9]{16}$/); + expect(b).toBe(a); + expect(c).toBe(a); + }); + + test("returns the same header-derived ID across repeated calls", () => { + const req = makeReq({ "x-request-id": "abc-123" }); + expect(resolveRequestId(req)).toBe("abc-123"); + expect(resolveRequestId(req)).toBe("abc-123"); + }); + + test("different request objects get different fallback IDs", () => { + const r1 = makeReq({}); + const r2 = makeReq({}); + const id1 = resolveRequestId(r1); + const id2 = resolveRequestId(r2); + expect(id1).toMatch(/^req_[a-f0-9]{16}$/); + expect(id2).toMatch(/^req_[a-f0-9]{16}$/); + // Two independent requests with no correlation headers must be + // distinguishable in logs — the fallback is per-request, not + // process-global. + expect(id1).not.toBe(id2); + }); + }); + + // AWS X-Ray traffic carries `X-Amzn-Trace-Id: Root=1-...;Parent=...; + // Sampled=1`. The strict allowlist rejects `=` and `;`, so without a + // pre-parser the x-amzn-trace-id consultation never triggers in + // production AWS deployments. The resolver special-cases + // x-amzn-trace-id to extract the `Root=` segment before sanitizing. + describe("AWS X-Ray Root= parsing", () => { + test("extracts Root segment from full AWS X-Ray header", () => { + const id = resolveRequestId( + makeReq({ + "x-amzn-trace-id": + "Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8;Sampled=1", + }), + ); + expect(id).toBe("1-5759e988-bd862e3fe1be46a994272793"); + }); + + test("extracts Root segment when it is not the first part", () => { + const id = resolveRequestId( + makeReq({ + "x-amzn-trace-id": + "Sampled=1;Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8", + }), + ); + expect(id).toBe("1-5759e988-bd862e3fe1be46a994272793"); + }); + + test("falls back to fresh ID when Root= segment is missing", () => { + // Header is non-empty but has no Root= segment — the parser + // returns undefined, the raw value still fails the allowlist + // (contains `=`), so the resolver falls through to a generated + // ID. + const id = resolveRequestId( + makeReq({ "x-amzn-trace-id": "Parent=abc;Sampled=1" }), + ); + expect(id).toMatch(/^req_[a-f0-9]{16}$/); + }); + + test("falls back when Root= segment value fails the allowlist", () => { + // A Root segment containing a disallowed character (e.g. space) + // is rejected by sanitizeRequestId, not silently accepted. + const id = resolveRequestId( + makeReq({ "x-amzn-trace-id": "Root=has space;Parent=x" }), + ); + expect(id).toMatch(/^req_[a-f0-9]{16}$/); + }); + + test("only x-amzn-trace-id triggers Root= parsing", () => { + // The x-request-id and x-correlation-id headers do not carry + // X-Ray syntax and must not be mangled by the parser. + const id = resolveRequestId( + makeReq({ "x-request-id": "Root=should-not-be-extracted" }), + ); + // Raw value contains `=` which the allowlist rejects, so the + // resolver must NOT silently accept "should-not-be-extracted" + // as if Root= parsing applied here. + expect(id).toMatch(/^req_[a-f0-9]{16}$/); + }); + }); +}); diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index 33bb1b95a..ec86a8439 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -1,4 +1,3 @@ -import { randomUUID } from "node:crypto"; import type { StandardSchemaV1 } from "@standard-schema/spec"; import type express from "express"; import type { @@ -24,7 +23,7 @@ import { } from "../context"; import { AppKitError, AuthenticationError } from "../errors"; import { createLogger } from "../logging/logger"; -import { sanitizeRequestId } from "../logging/request-id"; +import { resolveRequestId } from "../logging/request-id"; import { StreamManager } from "../stream"; import { type ITelemetry, @@ -59,31 +58,6 @@ function hasHttpStatusCode( ); } -/** - * Resolve a request ID from the `x-request-id` header (if present), falling - * back to a freshly generated UUID-derived token. Used by the body - * validation wrapper so operators can correlate a client-facing 400 with - * the full server-side issue log. - * - * The header value is validated against the shared {@link sanitizeRequestId} - * allowlist (also used by the wide-event logger), so a value accepted here - * matches the `request_id` recorded server-side. Invalid values are silently - * discarded — they are never logged or reflected anywhere — and a fresh ID - * is generated instead. The fallback uses 16 hex characters (~64 bits) so - * birthday collisions stay astronomically unlikely while keeping IDs short - * enough to skim in logs. - */ -function resolveRequestId(req: express.Request): string { - const headerId = req.header("x-request-id"); - if (typeof headerId === "string") { - const sanitized = sanitizeRequestId(headerId); - if (sanitized !== undefined) { - return sanitized; - } - } - return `req_${randomUUID().slice(0, 16)}`; -} - /** Maximum number of Standard Schema issues retained on a validation failure. */ const MAX_VALIDATION_ISSUES = 20; @@ -611,6 +585,24 @@ export abstract class Plugin< ); } + // Surface a one-time warning at route registration when a route + // exposes raw schema details (field names, refinement messages, + // constraint metadata) to anonymous callers in production. Body + // validation runs BEFORE plugin-level authentication, so the 400 + // payload reaches every caller — only public routes should opt in. + if ( + config.exposeValidationErrors === true && + process.env.NODE_ENV === "production" + ) { + logger.warn( + "Route registered with exposeValidationErrors=true in production. " + + "This exposes schema details (field names, constraints, refinement messages) " + + "to anonymous callers in 400 responses. Body validation runs BEFORE plugin-level " + + "authentication. Only use on routes that are intentionally public.", + { method, path, plugin: this.name }, + ); + } + // Zero-overhead pass-through when no body schema is provided. const effectiveHandler = config.body ? this._wrapHandlerWithBodyValidation( @@ -679,33 +671,33 @@ export abstract class Plugin< } // Strict discriminated-union shape check per the Standard Schema - // spec: only `{ value }` (with no issues or an empty issues array) - // is success; only `{ issues: [nonEmpty] }` is failure; anything - // else is a validator programmer error (malformed shape) that must - // route to a canonical 500 — never the success path, which would - // otherwise let a `result.value === undefined` crash the handler. + // v1 spec. The spec defines `FailureResult` as + // `{ readonly issues: ReadonlyArray }` with no minimum + // cardinality, so any array-valued `issues` field — including an + // empty array — is a failure shape. Only a result that is missing + // `issues` and has `value` qualifies as success; anything else + // (e.g. `{ issues: undefined }`, `{}`, `null`) is a validator + // programmer error and must route to a canonical 500. + // + // Mixed shapes such as `{ value, issues: [] }` route to failure + // because `'issues' in result && Array.isArray(...)` matches — + // this matches the spec's "any array-valued issues = failure" + // rule and avoids treating buggy validators as accidentally + // passing. const resultObject = typeof result === "object" && result !== null ? (result as { value?: unknown; issues?: unknown }) : undefined; - const issuesField = resultObject?.issues; + const hasIssuesField = + resultObject !== undefined && "issues" in resultObject; const issuesArray: ReadonlyArray | undefined = - Array.isArray(issuesField) - ? (issuesField as ReadonlyArray) + hasIssuesField && Array.isArray(resultObject?.issues) + ? (resultObject?.issues as ReadonlyArray) : undefined; - const hasNonEmptyIssues = - issuesArray !== undefined && issuesArray.length > 0; const hasValueField = resultObject !== undefined && "value" in resultObject; - const isValidationFailure = hasNonEmptyIssues; - // Empty issues array counts as success per the review spec, even - // though no `value` is promised by the validator in that case. - const isSuccess = - !hasNonEmptyIssues && - (hasValueField || - (issuesArray !== undefined && issuesArray.length === 0)); - - if (isValidationFailure && issuesArray !== undefined) { + + if (issuesArray !== undefined) { const requestId = resolveRequestId(req); const totalIssueCount = issuesArray.length; const truncated = totalIssueCount > MAX_VALIDATION_ISSUES; @@ -767,12 +759,19 @@ export abstract class Plugin< return; } - if (!isSuccess) { - // Validator returned a shape that is neither `{ value }` nor a - // non-empty `{ issues: [...] }` — e.g. `{ issues: undefined }`, - // `{}`, or `null`. Treat it as a validator programmer error and - // fail closed with a 500. Never invoke the handler; never mutate - // `req.body`. + if (hasIssuesField || !hasValueField) { + // Validator returned a shape that is neither `{ value }` (clean + // success) nor `{ issues: [array] }` (which the 400 path above + // already caught). Examples that land here: + // - `{ issues: undefined }` / `{ issues: "string" }` — `issues` + // present but not an array (`hasIssuesField && !arrayValued`) + // - `{ value, issues: undefined }` — same; the 400 path missed + // it because `Array.isArray(undefined) === false`, and the + // success path would otherwise mutate `req.body` with a + // value the validator clearly meant to reject. + // - `{}`, `null`, non-objects — `!hasValueField` + // All are validator programmer errors; fail closed with a 500. + // Never invoke the handler; never mutate `req.body`. const requestId = resolveRequestId(req); logger.error("validation schema returned malformed result", { plugin: this.name, @@ -786,17 +785,13 @@ export abstract class Plugin< return; } - // Success. Narrow req.body to the validated value when the result - // carries one; for the rare `{ issues: [] }` shape (empty issues = - // no issues found, per the review spec), leave `req.body` as-is so - // the handler sees the original body. This preserves any - // transformation performed by the schema (e.g. coercion), though - // v1 docs advise against relying on transforms. - if (hasValueField) { - (req as { body: unknown }).body = ( - resultObject as { value: TBody } - ).value; - } + // Success. The result is `{ value }` with no `issues` field + // (per the strict spec reading above). Narrow req.body to the + // validated value, preserving any transformation performed by + // the schema (e.g. coercion). + (req as { body: unknown }).body = ( + resultObject as { value: TBody } + ).value; await handler(req as IAppRequestWithBody, res); }; diff --git a/packages/appkit/src/plugin/tests/body-validation.test.ts b/packages/appkit/src/plugin/tests/body-validation.test.ts index f9b40f874..34b48c3e7 100644 --- a/packages/appkit/src/plugin/tests/body-validation.test.ts +++ b/packages/appkit/src/plugin/tests/body-validation.test.ts @@ -44,15 +44,18 @@ vi.mock("../../telemetry", () => ({ }), })); -// Silence logger output during validation-failure tests. +// Silence logger output during validation-failure tests, but expose a +// stable shared mock so individual tests can introspect warn/error +// calls (e.g. the exposeValidationErrors-in-production warning). +const loggerMock = vi.hoisted(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + event: vi.fn(), +})); vi.mock("../../logging/logger", () => ({ - createLogger: vi.fn(() => ({ - debug: vi.fn(), - info: vi.fn(), - warn: vi.fn(), - error: vi.fn(), - event: vi.fn(), - })), + createLogger: vi.fn(() => loggerMock), })); class TestPlugin extends Plugin { @@ -668,9 +671,10 @@ describe("route body validation", () => { await handler(req, res); const body = (res.json as any).mock.calls[0][0]; - // randomUUID().slice(0, 16) is `xxxxxxxx-xxxx-xx` — 14 hex chars - // plus 2 hyphens, all matching `[a-f0-9-]`, total length 16. - expect(body.requestId).toMatch(/^req_[a-f0-9-]{16}$/); + // `randomBytes(8).toString("hex")` produces exactly 16 hex chars + // — no embedded hyphens (the previous `randomUUID().slice(0, 16)` + // form contained 2 hyphens and only 14 hex chars). + expect(body.requestId).toMatch(/^req_[a-f0-9]{16}$/); }); test("truncates the issues array to 20 entries and flags issuesTruncated", async () => { @@ -899,7 +903,103 @@ describe("route body validation", () => { ); }); - test("empty issues array counts as success: handler is called", async () => { + // Regression: `{ value, issues: }` is a malformed shape + // (the spec requires `issues` to be a `ReadonlyArray`). The + // earlier 400 gate only fires when `Array.isArray(issues)` is true, + // so without the tightened 500 gate this result would slip through + // to the success path with a possibly-invalid `value`. Both cases + // (string `issues`, `undefined` `issues`) must route to 500. + test("malformed: returns canonical 500 when validator returns { value, issues: 'string' }", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const malformed: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + validate: () => + ({ value: { ok: true }, issues: "not-an-array" }) as any, + }, + }; + + const handlerSpy = vi.fn(); + + plugin.exposedRoute(router, { + name: "bad", + method: "post", + path: "/bad", + body: malformed, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/bad"); + const originalBody = { sentinel: "should-not-be-mutated" }; + const req = createMockRequest({ body: originalBody }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(req.body).toBe(originalBody); + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Internal validation error", + code: "VALIDATION_INTERNAL_ERROR", + requestId: expect.any(String), + }), + ); + }); + + test("malformed: returns canonical 500 when validator returns { value, issues: undefined }", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const malformed: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + validate: () => ({ value: { ok: true }, issues: undefined }) as any, + }, + }; + + const handlerSpy = vi.fn(); + + plugin.exposedRoute(router, { + name: "bad", + method: "post", + path: "/bad", + body: malformed, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/bad"); + const originalBody = { sentinel: "should-not-be-mutated" }; + const req = createMockRequest({ body: originalBody }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(req.body).toBe(originalBody); + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Internal validation error", + code: "VALIDATION_INTERNAL_ERROR", + }), + ); + }); + + test("empty issues array routes to canonical 400 (any array-valued issues = failure)", async () => { + // Standard Schema v1's `FailureResult` is defined as + // `{ readonly issues: ReadonlyArray }` with no minimum + // cardinality, so any array-valued `issues` field — including an + // empty array — is a failure shape. The handler must NOT run; the + // wrapper emits a canonical 400 with `issues: []` in the response + // body in development / override mode. process.env.NODE_ENV = "development"; const plugin = createTestPlugin(); const { router, getHandler } = createMockRouter(); @@ -908,14 +1008,57 @@ describe("route body validation", () => { "~standard": { version: 1, vendor: "test", - // Empty issues array is treated as success — "no issues found". validate: () => ({ issues: [] }) as any, }, }; - const handlerSpy = vi.fn(async (_req: any, res: any) => { - res.status(200).json({ ok: true }); + const handlerSpy = vi.fn(); + + plugin.exposedRoute(router, { + name: "good", + method: "post", + path: "/good", + body: emptyIssues, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/good"); + const originalBody = { any: "thing" }; + const req = createMockRequest({ body: originalBody }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + // req.body must not be mutated when validation fails. + expect(req.body).toBe(originalBody); + expect(res.status).toHaveBeenCalledWith(400); + const body = (res.json as any).mock.calls[0][0]; + expect(body).toMatchObject({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), }); + // Empty issues array is reflected in the response body in dev mode. + expect(body.issues).toEqual([]); + expect(body).not.toHaveProperty("issuesTruncated"); + }); + + test("empty issues array in production omits issues from response", async () => { + // Production hides issues by default regardless of count. + process.env.NODE_ENV = "production"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const emptyIssues: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + validate: () => ({ issues: [] }) as any, + }, + }; + + const handlerSpy = vi.fn(); plugin.exposedRoute(router, { name: "good", @@ -926,13 +1069,64 @@ describe("route body validation", () => { }); const handler = getHandler("POST", "/good"); - const req = createMockRequest({ body: { any: "thing" } }); + const req = createMockRequest({ body: {} }); const res = createMockResponse(); await handler(req, res); - expect(handlerSpy).toHaveBeenCalledTimes(1); - expect(res.status).toHaveBeenCalledWith(200); + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + const body = (res.json as any).mock.calls[0][0]; + expect(body).toMatchObject({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + requestId: expect.any(String), + }); + expect(body).not.toHaveProperty("issues"); + }); + + test("mixed shape { value, issues: [] } routes to validation failure", async () => { + // Per the strict spec reading, the discriminator is the presence + // of an array-valued `issues` field — not its length, and not the + // absence of `value`. A validator that returns both `value` and an + // array `issues` is buggy; we route it to failure (not success) so + // the handler never sees a possibly-invalid body. + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const mixed: StandardSchemaV1 = { + "~standard": { + version: 1, + vendor: "test", + validate: () => ({ value: { any: "thing" }, issues: [] }) as any, + }, + }; + + const handlerSpy = vi.fn(); + + plugin.exposedRoute(router, { + name: "mixed", + method: "post", + path: "/mixed", + body: mixed, + handler: handlerSpy, + }); + + const handler = getHandler("POST", "/mixed"); + const req = createMockRequest({ body: {} }); + const res = createMockResponse(); + + await handler(req, res); + + expect(handlerSpy).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); }); test("non-empty issues array is a validation failure (canonical 400)", async () => { @@ -975,4 +1169,271 @@ describe("route body validation", () => { }), ); }); + + // Item 3: route() must surface a one-time warning at registration time + // when a route opts in to exposing schema details to anonymous callers + // in production. Body validation runs BEFORE plugin-level + // authentication, so the 400 payload is reachable by any client. + describe("exposeValidationErrors production warning", () => { + test("warns at route registration when exposeValidationErrors=true in production", () => { + process.env.NODE_ENV = "production"; + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + loggerMock.warn.mockClear(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "public", + method: "post", + path: "/public", + body: schema, + exposeValidationErrors: true, + handler: vi.fn(), + }); + + expect(loggerMock.warn).toHaveBeenCalledTimes(1); + const [message, context] = loggerMock.warn.mock.calls[0]; + expect(message).toMatch(/exposeValidationErrors/i); + expect(message).toMatch(/production/i); + expect(message).toMatch(/before plugin-level/i); + expect(context).toEqual( + expect.objectContaining({ + method: "post", + path: "/public", + plugin: "test", + }), + ); + }); + + test("does not warn when exposeValidationErrors is undefined", () => { + process.env.NODE_ENV = "production"; + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + loggerMock.warn.mockClear(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "internal", + method: "post", + path: "/internal", + body: schema, + handler: vi.fn(), + }); + + expect(loggerMock.warn).not.toHaveBeenCalled(); + }); + + test("does not warn when exposeValidationErrors=false in production", () => { + process.env.NODE_ENV = "production"; + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + loggerMock.warn.mockClear(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "internal", + method: "post", + path: "/internal", + body: schema, + exposeValidationErrors: false, + handler: vi.fn(), + }); + + expect(loggerMock.warn).not.toHaveBeenCalled(); + }); + + test("does not warn when NODE_ENV is development", () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + loggerMock.warn.mockClear(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "public", + method: "post", + path: "/public", + body: schema, + exposeValidationErrors: true, + handler: vi.fn(), + }); + + expect(loggerMock.warn).not.toHaveBeenCalled(); + }); + + test("does not warn when NODE_ENV is test", () => { + process.env.NODE_ENV = "test"; + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + loggerMock.warn.mockClear(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "public", + method: "post", + path: "/public", + body: schema, + exposeValidationErrors: true, + handler: vi.fn(), + }); + + expect(loggerMock.warn).not.toHaveBeenCalled(); + }); + + test("does not warn when route has no body schema", () => { + process.env.NODE_ENV = "production"; + const plugin = createTestPlugin(); + const { router } = createMockRouter(); + loggerMock.warn.mockClear(); + + // exposeValidationErrors is meaningless without a body schema, but + // even if a plugin author sets it, no warning fires because the + // wrapper is never attached. + plugin.exposedRoute(router, { + name: "noBody", + method: "post", + path: "/no-body", + handler: vi.fn(), + }); + + expect(loggerMock.warn).not.toHaveBeenCalled(); + }); + }); + + // Item 4: the validation wrapper consults the same correlation + // headers as the wide-event logger (in the same order) so the + // canonical 4xx requestId matches the wide-event log's request_id. + describe("requestId header precedence (shared resolver)", () => { + test("falls back to x-amzn-trace-id when x-request-id is absent", async () => { + // Without the shared resolver, the validation wrapper only saw + // x-request-id while the wide-event logger reads x-amzn-trace-id + // — operators got two different IDs for the same request and + // could not correlate. + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: {}, + headers: { "x-amzn-trace-id": "Root.1-abc-def" }, + }); + const res = createMockResponse(); + + await handler(req, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + requestId: "Root.1-abc-def", + }), + ); + }); + + test("falls back to x-correlation-id when x-request-id is absent", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: {}, + headers: { "x-correlation-id": "corr-abc-123" }, + }); + const res = createMockResponse(); + + await handler(req, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + requestId: "corr-abc-123", + }), + ); + }); + + test("prefers x-request-id over later headers", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: {}, + headers: { + "x-request-id": "primary-id", + "x-correlation-id": "secondary-id", + "x-amzn-trace-id": "tertiary-id", + }, + }); + const res = createMockResponse(); + + await handler(req, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + requestId: "primary-id", + }), + ); + }); + + test("skips a malformed earlier header and uses a valid later one", async () => { + process.env.NODE_ENV = "development"; + const plugin = createTestPlugin(); + const { router, getHandler } = createMockRouter(); + + const schema = z.object({ content: z.string().min(1) }); + plugin.exposedRoute(router, { + name: "sendMessage", + method: "post", + path: "/messages", + body: schema, + handler: vi.fn(), + }); + + const handler = getHandler("POST", "/messages"); + const req = createMockRequest({ + body: {}, + headers: { + // Malformed: starts with a dash (rejected by sanitizer). + "x-request-id": "-rf", + "x-correlation-id": "fallback-ok", + }, + }); + const res = createMockResponse(); + + await handler(req, res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + requestId: "fallback-ok", + }), + ); + }); + }); }); diff --git a/packages/appkit/src/plugins/analytics/analytics.ts b/packages/appkit/src/plugins/analytics/analytics.ts index 6b38276f6..c5ada6493 100644 --- a/packages/appkit/src/plugins/analytics/analytics.ts +++ b/packages/appkit/src/plugins/analytics/analytics.ts @@ -24,12 +24,19 @@ import type { IAnalyticsConfig } from "./types"; * defaults to "JSON" via the schema so the handler sees a fully-populated * body with no manual fallback needed. * - * `parameters` accepts both JSON primitives (string, number, boolean, - * null) AND `SQLTypeMarker` objects produced by `sql.string()`, - * `sql.number()`, `sql.date()`, `sql.timestamp()`, `sql.boolean()`. The - * marker shape is `{ __sql_type, value }` and its `value` field is capped - * at 4096 characters. `.strict()` on the marker schema rejects unknown - * fields so callers can't smuggle additional keys past validation. + * `parameters` accepts only `SQLTypeMarker` objects produced by + * `sql.string()`, `sql.number()`, `sql.date()`, `sql.timestamp()`, + * `sql.boolean()` (or explicit `null`). Raw JSON primitives are rejected + * here at the HTTP boundary because `QueryProcessor._createParameter` + * accepts only markers and `null`; if primitives passed schema + * validation, `executeStream()` would have already flipped the response + * to SSE before the processor's `isSQLTypeMarker` guard ran, leaving the + * client with a stream error instead of the canonical 400. Callers must + * wrap raw values with `sql.string()`, `sql.number()`, etc., before + * sending. The marker shape is `{ __sql_type, value }` and its `value` + * field is capped at 4096 characters. `.strict()` on the marker schema + * rejects unknown fields so callers can't smuggle additional keys past + * validation. * * Per-query parameter shape validation remains the application's concern; * this schema is the minimum safety net the plugin enforces for every @@ -46,16 +53,7 @@ const sqlTypeMarkerSchema = z const queryBodySchema = z.object({ parameters: z - .record( - z.string().max(255), - z.union([ - z.string().max(4096), - z.number(), - z.boolean(), - z.null(), - sqlTypeMarkerSchema, - ]), - ) + .record(z.string().max(255), z.union([sqlTypeMarkerSchema, z.null()])) .refine((obj) => Object.keys(obj).length <= 100, { message: "parameters may not contain more than 100 keys", }) @@ -235,18 +233,14 @@ export class AnalyticsPlugin extends Plugin { await executor.executeStream( res, async (signal) => { - // Body schema accepts string | number | boolean | null | - // SQLTypeMarker. `QueryProcessor.processQueryParams` is typed - // narrower — `Record` - // — so we cast the validated input down to that shape. The - // processor's `isSQLTypeMarker` guard re-validates each value - // before trusting it: primitives reaching the processor today - // surface as a runtime ValidationError there. Bridging primitives - // to markers (or rejecting them at the handler) is a separate - // concern; see the corresponding test. + // Body schema restricts parameter values to `SQLTypeMarker | null` + // (raw JSON primitives are rejected at the HTTP boundary — see the + // `queryBodySchema` JSDoc). `processQueryParams` accepts + // `Record`, which is + // type-compatible with the validated input. const processedParams = await this.queryProcessor.processQueryParams( query, - parameters as Record, + parameters, ); const result = await executor.query( diff --git a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts index 4c296b1b4..d45a8990a 100644 --- a/packages/appkit/src/plugins/analytics/tests/analytics.test.ts +++ b/packages/appkit/src/plugins/analytics/tests/analytics.test.ts @@ -223,10 +223,10 @@ describe("Analytics Plugin", () => { }); test("/query/:query_key should return canonical 400 when a parameter value is an array", async () => { - // The body schema restricts parameter values to JSON primitives - // (string, number, boolean, null) or SQLTypeMarker objects. Arrays - // are neither — they have to be rejected at the HTTP boundary so - // oversized nested payloads never reach the query processor. + // The body schema restricts parameter values to SQLTypeMarker + // objects or `null`. Arrays are neither, so they have to be + // rejected at the HTTP boundary; otherwise oversized nested + // payloads would reach the query processor. const plugin = new AnalyticsPlugin(config); const { router, getHandler } = createMockRouter(); @@ -258,10 +258,9 @@ describe("Analytics Plugin", () => { test("/query/:query_key should accept SQLTypeMarker values in parameters", async () => { // The body schema accepts SQLTypeMarker objects (sql.string(), - // sql.number(), etc.) at value position alongside JSON primitives. - // The handler must run, the marker must pass through to the query - // processor, and the SQL parameter list must reflect the marker's - // type. + // sql.number(), etc.) at value position; the handler must run, + // the marker must pass through to the query processor, and the + // SQL parameter list must reflect the marker's type. const plugin = new AnalyticsPlugin(config); const { router, getHandler } = createMockRouter(); @@ -412,9 +411,83 @@ describe("Analytics Plugin", () => { expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); }); - test("/query/:query_key should accept primitive values in parameters", async () => { - // JSON primitives (string, number, boolean, null) must continue to - // pass through validation alongside SQLTypeMarker shapes. + test("/query/:query_key should reject raw primitive values in parameters", async () => { + // Raw JSON primitives (string, number, boolean) are rejected at the + // HTTP boundary so the handler never starts SSE streaming with + // values the query processor would later refuse. Callers must wrap + // primitives with `sql.string()`, `sql.number()`, etc. + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { + parameters: { + str: "hello", + num: 42, + bool: true, + }, + }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(executeMock).not.toHaveBeenCalled(); + expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); + }); + + test("/query/:query_key should reject a raw number parameter", async () => { + // Specific regression for the late-failure-via-SSE concern: a raw + // number (not a marker) must produce VALIDATION_ERROR before the + // handler dispatches to executeStream. + const plugin = new AnalyticsPlugin(config); + const { router, getHandler } = createMockRouter(); + + const executeMock = vi.fn(); + (plugin as any).SQLClient.executeStatement = executeMock; + (plugin as any).app.getAppQuery = vi.fn(); + + plugin.injectRoutes(router); + + const handler = getHandler("POST", "/query/:query_key"); + const mockReq = createMockRequest({ + params: { query_key: "test_query" }, + body: { parameters: { user_id: 123 } }, + }); + const mockRes = createMockResponse(); + + await handler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(400); + expect(mockRes.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Invalid request body", + code: "VALIDATION_ERROR", + }), + ); + expect(executeMock).not.toHaveBeenCalled(); + expect((plugin as any).app.getAppQuery).not.toHaveBeenCalled(); + }); + + test("/query/:query_key should accept null parameter values", async () => { + // `null` is the only non-marker value that passes the schema — + // the query processor's `_createParameter` returns null for null + // entries (effectively skipping them). const plugin = new AnalyticsPlugin(config); const { router, getHandler } = createMockRouter(); @@ -433,14 +506,7 @@ describe("Analytics Plugin", () => { const handler = getHandler("POST", "/query/:query_key"); const mockReq = createMockRequest({ params: { query_key: "test_query" }, - body: { - parameters: { - str: "hello", - num: 42, - bool: true, - nullish: null, - }, - }, + body: { parameters: { nullish: null } }, }); const mockRes = createMockResponse();