From afaa7c8dec568bae434a0552c47ede6c9a50f0d8 Mon Sep 17 00:00:00 2001 From: stack72 Date: Tue, 5 May 2026 19:40:59 +0100 Subject: [PATCH] fix(validation): close gaps in --global-arg/--input unknown-key check Three follow-ups from the #1312 adversarial review: 1. Prototype-chain bypass: the unknown-key check used `k in shape` which walks Object.prototype, so flag keys like `--input constructor=oops` bypassed the check. Switch to `Object.hasOwn(shape, k)`. 2. ZodEffects not unwrapped: schemas defined with `.refine()`, `.transform()`, or `.pipe()` lack `.strict()` and `.shape` on the outer wrapper, so the previous duck-typed cast silently fell back to strip mode. Introduce `getObjectShape()` that uses the existing `unwrapSchema` helper to find the inner ZodObject's shape, then do an explicit unknown-key check against it. The original schema then handles normal validation, preserving refinements/transforms. 3. Zod v3 vs v4 internals: extensions may import `npm:zod@3` (where `_def.typeName` is e.g. `"ZodObject"` and `_def.shape` is a function) or `npm:zod@4` (where `_def.type` is `"object"` and `_def.shape` is a value). Normalize both in `getSchemaType()` and `readShape()`. Also fixes the CLI UX issue from the #1312 review: the `model create` error no longer shows a leading `: ` prefix because the unknown-key error is now produced explicitly with a clear message naming valid arguments, instead of via Zod's empty-path `unrecognized_keys` issue. Co-Authored-By: Claude Sonnet 4.6 --- src/domain/models/method_execution_service.ts | 21 +++++-- src/domain/models/validation_service.ts | 45 ++++++++++--- src/domain/models/zod_type_coercion.ts | 57 ++++++++++++++--- src/domain/models/zod_type_coercion_test.ts | 62 +++++++++++++++++- src/libswamp/models/create.ts | 31 ++++++--- src/libswamp/models/create_test.ts | 63 +++++++++++++++++++ src/libswamp/models/run.ts | 11 ++-- src/libswamp/models/run_test.ts | 62 ++++++++++++++++++ 8 files changed, 315 insertions(+), 37 deletions(-) diff --git a/src/domain/models/method_execution_service.ts b/src/domain/models/method_execution_service.ts index e88822e0..f669a1a0 100644 --- a/src/domain/models/method_execution_service.ts +++ b/src/domain/models/method_execution_service.ts @@ -40,7 +40,7 @@ import { createFileWriterFactory, createResourceWriter, } from "./data_writer.ts"; -import { coerceMethodArgs } from "./zod_type_coercion.ts"; +import { coerceMethodArgs, getObjectShape } from "./zod_type_coercion.ts"; import { containsExpression, valueContainsExpression, @@ -449,12 +449,21 @@ export class DefaultMethodExecutionService implements MethodExecutionService { modelDef.globalArguments, ); const globalArgsSchema = modelDef.globalArguments; - const strictGlobalArgs = ( - globalArgsSchema as unknown as { - strict?(): typeof globalArgsSchema; + const shape = getObjectShape(globalArgsSchema); + if (shape) { + const unknownKeys = Object.keys(coercedGlobalArgs).filter( + (k) => !Object.hasOwn(shape, k), + ); + if (unknownKeys.length > 0) { + const validKeys = Object.keys(shape).join(", "); + throw new Error( + `Global arguments validation failed: Unknown argument(s): ${ + unknownKeys.join(", ") + }. Valid arguments are: ${validKeys || "none"}`, + ); } - ).strict?.() ?? globalArgsSchema; - const globalArgsResult = strictGlobalArgs.safeParse( + } + const globalArgsResult = globalArgsSchema.safeParse( coercedGlobalArgs, ); if (!globalArgsResult.success) { diff --git a/src/domain/models/validation_service.ts b/src/domain/models/validation_service.ts index 84439b65..9740df5a 100644 --- a/src/domain/models/validation_service.ts +++ b/src/domain/models/validation_service.ts @@ -30,6 +30,7 @@ import { stripExpressionFields, } from "../expressions/expression_parser.ts"; import { detectEnvVarUsageInDefinition } from "./env_var_detector.ts"; +import { getObjectShape } from "./zod_type_coercion.ts"; import { extractEnvReferences, extractPathReferences, @@ -482,14 +483,28 @@ export class DefaultModelValidationService implements ModelValidationService { return Promise.resolve(ValidationResult.pass("Global arguments")); } - // All fields are static, validate with strict mode to reject unknown keys + // All fields are static — reject unknown keys explicitly so the check + // also catches schemas wrapped in .refine()/.transform() (no .strict() + // available on ZodEffects) and is not bypassed by prototype-chain keys. const globalArgsSchema = modelDef.globalArguments; - const strictGlobalArgs = ( - globalArgsSchema as unknown as { strict?(): typeof globalArgsSchema } - ).strict?.() ?? globalArgsSchema; + const shape = getObjectShape(globalArgsSchema); + if (shape) { + const unknownKeys = Object.keys(staticArgs).filter( + (k) => !Object.hasOwn(shape, k), + ); + if (unknownKeys.length > 0) { + const validKeys = Object.keys(shape).join(", "); + return Promise.resolve(ValidationResult.fail( + "Global arguments", + `Unknown global argument(s): ${ + unknownKeys.join(", ") + }. Valid arguments are: ${validKeys || "none"}`, + )); + } + } return this.validateWithSchema( "Global arguments", - strictGlobalArgs, + globalArgsSchema, staticArgs, ); } @@ -515,10 +530,22 @@ export class DefaultModelValidationService implements ModelValidationService { } const methodArgsSchema = methodDef.arguments; - const strictMethodArgs = ( - methodArgsSchema as unknown as { strict?(): typeof methodArgsSchema } - ).strict?.() ?? methodArgsSchema; - const result = strictMethodArgs.safeParse(staticArgs); + const shape = getObjectShape(methodArgsSchema); + if (shape) { + const unknownKeys = Object.keys(staticArgs).filter( + (k) => !Object.hasOwn(shape, k), + ); + if (unknownKeys.length > 0) { + const validKeys = Object.keys(shape).join(", "); + errors.push( + `Method "${methodName}": Unknown argument(s): ${ + unknownKeys.join(", ") + }. Valid arguments are: ${validKeys || "none"}`, + ); + continue; + } + } + const result = methodArgsSchema.safeParse(staticArgs); if (!result.success) { errors.push( `Method "${methodName}": ${formatZodError(result.error)}`, diff --git a/src/domain/models/zod_type_coercion.ts b/src/domain/models/zod_type_coercion.ts index 990876e6..0e34b234 100644 --- a/src/domain/models/zod_type_coercion.ts +++ b/src/domain/models/zod_type_coercion.ts @@ -20,13 +20,20 @@ import type { z } from "zod"; /** - * Internal Zod v4 definition structure for schema introspection. + * Internal Zod definition structure for schema introspection. + * + * Extension authors may import Zod v3 (`npm:zod@3`) or Zod v4 (`npm:zod@4`). + * The two versions name the type field differently (`typeName` in v3, + * `type` in v4) and store object shape differently (`shape()` function in + * v3, `shape` value in v4). Both fields are read here so the helpers work + * regardless of which Zod version the extension imports. */ interface ZodDef { - type: string; + type?: string; + typeName?: string; innerType?: z.ZodTypeAny; schema?: z.ZodTypeAny; - shape?: Record; + shape?: Record | (() => Record); } /** @@ -37,10 +44,27 @@ function getSchemaDef(schema: z.ZodTypeAny): ZodDef { } /** - * Gets the definition type string from a Zod schema. + * Returns a normalized type name ("object", "optional", "effects", ...) for + * a Zod schema. Maps Zod v3 typeName values (e.g. "ZodObject") to Zod v4 + * type values (e.g. "object"). */ function getSchemaType(schema: z.ZodTypeAny): string { - return getSchemaDef(schema)?.type ?? ""; + const def = getSchemaDef(schema); + if (!def) return ""; + if (def.type) return def.type; + if (def.typeName) { + return def.typeName.replace(/^Zod/, "").toLowerCase(); + } + return ""; +} + +/** + * Returns the field shape of a ZodObject, accepting both Zod v3 (where + * `_def.shape` is a function) and Zod v4 (where `_def.shape` is a value). + */ +function readShape(def: ZodDef): Record | undefined { + if (typeof def.shape === "function") return def.shape(); + return def.shape; } /** @@ -83,7 +107,8 @@ export function coerceMethodArgs( } const def = getSchemaDef(unwrapped); - if (!def.shape) { + const shape = readShape(def); + if (!shape) { return args; } @@ -94,7 +119,7 @@ export function coerceMethodArgs( continue; } - const fieldSchema = def.shape[key]; + const fieldSchema = shape[key]; if (!fieldSchema) { continue; } @@ -117,3 +142,21 @@ export function coerceMethodArgs( return result; } + +/** + * Returns the field shape of the inner ZodObject for a schema, unwrapping + * optional/nullable/default/effects wrappers. Returns undefined when the + * schema does not resolve to a ZodObject (e.g. a primitive or a union). + * + * Used to detect unknown keys passed via CLI flags before Zod's default + * strip mode silently discards them. + */ +export function getObjectShape( + schema: z.ZodTypeAny, +): Record | undefined { + const unwrapped = unwrapSchema(schema); + if (getSchemaType(unwrapped) !== "object") { + return undefined; + } + return readShape(getSchemaDef(unwrapped)); +} diff --git a/src/domain/models/zod_type_coercion_test.ts b/src/domain/models/zod_type_coercion_test.ts index b1f03097..3f56d53b 100644 --- a/src/domain/models/zod_type_coercion_test.ts +++ b/src/domain/models/zod_type_coercion_test.ts @@ -19,7 +19,7 @@ import { assertEquals } from "@std/assert"; import { z } from "zod"; -import { coerceMethodArgs } from "./zod_type_coercion.ts"; +import { coerceMethodArgs, getObjectShape } from "./zod_type_coercion.ts"; Deno.test("coerces string 'true' to boolean true", () => { const schema = z.object({ enabled: z.boolean() }); @@ -137,3 +137,63 @@ Deno.test("does not coerce empty string to number", () => { const result = coerceMethodArgs({ count: "" }, schema); assertEquals(result, { count: 0 }); }); + +// ---------- getObjectShape ---------- + +Deno.test("getObjectShape returns shape for plain ZodObject", () => { + const schema = z.object({ name: z.string(), count: z.number() }); + const shape = getObjectShape(schema); + assertEquals(Object.keys(shape ?? {}).sort(), ["count", "name"]); +}); + +Deno.test("getObjectShape returns shape for ZodObject wrapped in .refine()", () => { + const schema = z.object({ name: z.string() }).refine( + (v) => v.name.length > 0, + ); + const shape = getObjectShape(schema); + assertEquals(Object.keys(shape ?? {}), ["name"]); +}); + +Deno.test("getObjectShape returns shape for ZodObject wrapped in .optional()", () => { + const schema = z.object({ name: z.string() }).optional(); + const shape = getObjectShape(schema); + assertEquals(Object.keys(shape ?? {}), ["name"]); +}); + +Deno.test("getObjectShape returns undefined for non-object schema", () => { + const schema = z.string(); + assertEquals(getObjectShape(schema), undefined); +}); + +Deno.test("getObjectShape handles Zod v3 internal structure (typeName + shape function)", () => { + // Extensions may import npm:zod@3, whose ZodObject stores its type as + // `_def.typeName` ("ZodObject") and its shape as a function `_def.shape()`. + // This test simulates that structure to ensure compatibility. + const v3LikeSchema = { + _def: { + typeName: "ZodObject", + shape: () => ({ + name: z.string(), + count: z.number(), + }), + }, + } as unknown as z.ZodTypeAny; + const shape = getObjectShape(v3LikeSchema); + assertEquals(Object.keys(shape ?? {}).sort(), ["count", "name"]); +}); + +Deno.test("getObjectShape handles Zod v3 ZodEffects (typeName + .schema)", () => { + const v3LikeRefined = { + _def: { + typeName: "ZodEffects", + schema: { + _def: { + typeName: "ZodObject", + shape: () => ({ name: z.string() }), + }, + }, + }, + } as unknown as z.ZodTypeAny; + const shape = getObjectShape(v3LikeRefined); + assertEquals(Object.keys(shape ?? {}), ["name"]); +}); diff --git a/src/libswamp/models/create.ts b/src/libswamp/models/create.ts index b0b3d060..bc8a67f5 100644 --- a/src/libswamp/models/create.ts +++ b/src/libswamp/models/create.ts @@ -34,6 +34,7 @@ import { zodToJsonSchema, } from "../types/schema_helpers.ts"; import { coerceInputTypes } from "../../domain/inputs/mod.ts"; +import { getObjectShape } from "../../domain/models/zod_type_coercion.ts"; import { withGeneratorSpan } from "../../infrastructure/tracing/mod.ts"; /** @@ -161,16 +162,30 @@ export async function* modelCreate( jsonSchema as Record, ); const globalArgsSchema = modelDef.globalArguments; - const strictGlobalArgs = ( - globalArgsSchema as unknown as { - strict?(): typeof globalArgsSchema; + const shape = getObjectShape(globalArgsSchema); + if (shape) { + const unknownKeys = Object.keys(globalArguments).filter( + (k) => !Object.hasOwn(shape, k), + ); + if (unknownKeys.length > 0) { + const validKeys = Object.keys(shape).join(", "); + yield { + kind: "error", + error: validationFailed( + `Unknown global argument(s) for type '${modelType.normalized}': ${ + unknownKeys.join(", ") + }. Valid arguments are: ${validKeys || "none"}`, + ), + }; + return; } - ).strict?.() ?? globalArgsSchema; - const result = strictGlobalArgs.safeParse(globalArguments); + } + const result = globalArgsSchema.safeParse(globalArguments); if (!result.success) { - const issues = result.error.issues.map((i) => - ` ${i.path.join(".")}: ${i.message}` - ).join("\n"); + const issues = result.error.issues.map((i) => { + const path = i.path.length > 0 ? `${i.path.join(".")}: ` : ""; + return ` ${path}${i.message}`; + }).join("\n"); yield { kind: "error", error: validationFailed( diff --git a/src/libswamp/models/create_test.ts b/src/libswamp/models/create_test.ts index 6d873d23..9af1d748 100644 --- a/src/libswamp/models/create_test.ts +++ b/src/libswamp/models/create_test.ts @@ -208,6 +208,69 @@ Deno.test("modelCreate: accepts all known global arg keys", async () => { assertEquals(last.kind, "completed"); }); +Deno.test("modelCreate: rejects prototype-chain key like 'constructor' as global arg", async () => { + const globalArgsSchema = z.object({ name: z.string() }); + + const deps = makeDeps({ + getModelDef: () => ({ + type: { normalized: "test/strict" }, + version: "1.0.0", + globalArguments: globalArgsSchema, + methods: {}, + resources: {}, + } as unknown as ModelDefinition), + }); + + const events = await collect( + modelCreate(createLibSwampContext(), deps, { + typeArg: "test/strict", + name: "my-model", + globalArguments: { name: "hello", constructor: "oops" }, + }), + ); + + const last = events[events.length - 1] as Extract< + ModelCreateEvent, + { kind: "error" } + >; + assertEquals(last.kind, "error"); + assertEquals(last.error.code, "validation_failed"); + assertEquals(last.error.message.includes("constructor"), true); +}); + +Deno.test("modelCreate: rejects unknown global arg when schema uses .refine()", async () => { + const globalArgsSchema = z.object({ name: z.string() }).refine( + (v) => v.name.length > 0, + { message: "name cannot be empty" }, + ); + + const deps = makeDeps({ + getModelDef: () => ({ + type: { normalized: "test/refined" }, + version: "1.0.0", + globalArguments: globalArgsSchema, + methods: {}, + resources: {}, + } as unknown as ModelDefinition), + }); + + const events = await collect( + modelCreate(createLibSwampContext(), deps, { + typeArg: "test/refined", + name: "my-model", + globalArguments: { name: "hello", typoKey: "oops" }, + }), + ); + + const last = events[events.length - 1] as Extract< + ModelCreateEvent, + { kind: "error" } + >; + assertEquals(last.kind, "error"); + assertEquals(last.error.code, "validation_failed"); + assertEquals(last.error.message.includes("typoKey"), true); +}); + Deno.test("modelCreate: yields error when name already exists", async () => { const deps = makeDeps({ findByNameGlobal: () => Promise.resolve(true), diff --git a/src/libswamp/models/run.ts b/src/libswamp/models/run.ts index c6e01033..dfd79f71 100644 --- a/src/libswamp/models/run.ts +++ b/src/libswamp/models/run.ts @@ -60,6 +60,7 @@ import { import { extractInputReferences } from "../../domain/expressions/expression_parser.ts"; import { extractSensitiveFieldValues } from "../../domain/models/sensitive_field_extractor.ts"; import { detectEnvVarUsageInDefinition } from "../../domain/models/env_var_detector.ts"; +import { getObjectShape } from "../../domain/models/zod_type_coercion.ts"; import { withEventBridge } from "../../infrastructure/stream/event_bridge.ts"; import type { MethodResult } from "../../domain/models/model.ts"; import { getRunLogger } from "../../infrastructure/logging/logger.ts"; @@ -368,15 +369,13 @@ export async function* modelMethodRun( ), ); if (Object.keys(overrideInputs).length > 0) { - const methodSchema = method.arguments as { - shape?: Record; - }; - if (methodSchema.shape !== undefined) { + const shape = getObjectShape(method.arguments); + if (shape) { const unknownKeys = Object.keys(overrideInputs).filter( - (k) => !(k in methodSchema.shape!), + (k) => !Object.hasOwn(shape, k), ); if (unknownKeys.length > 0) { - const validInputs = Object.keys(methodSchema.shape).join(", "); + const validInputs = Object.keys(shape).join(", "); yield { kind: "error", error: validationFailed( diff --git a/src/libswamp/models/run_test.ts b/src/libswamp/models/run_test.ts index 4781c2e3..b469b5ad 100644 --- a/src/libswamp/models/run_test.ts +++ b/src/libswamp/models/run_test.ts @@ -581,6 +581,68 @@ Deno.test("modelMethodRun: accepts known override input key", async () => { assertEquals(completedEvent?.kind, "completed"); }); +Deno.test("modelMethodRun: rejects prototype-chain key like 'constructor'", async () => { + const definition = Definition.create({ + name: "test-model", + methods: { run: { arguments: {} } }, + }); + const modelDef = createTestModelDef("run"); + const deps = createTestDeps(definition, modelDef); + + const ctx = createLibSwampContext(); + const events = await collect( + modelMethodRun(ctx, deps, { + ...createTestInput("test-model", "run"), + // "constructor" is on Object.prototype but is NOT a method argument + inputs: { constructor: "oops" }, + }), + ); + + const errorEvent = events.find((e) => e.kind === "error"); + assertEquals(errorEvent?.kind, "error"); + if (errorEvent?.kind === "error") { + assertEquals(errorEvent.error.code, "validation_failed"); + assertEquals(errorEvent.error.message.includes("constructor"), true); + } +}); + +Deno.test("modelMethodRun: rejects unknown key when method arguments use .refine()", async () => { + const definition = Definition.create({ + name: "refined-model", + methods: { run: { arguments: {} } }, + }); + const modelDef: ModelDefinition = { + type: TEST_MODEL_TYPE, + version: "1.0.0", + methods: { + run: { + description: "Test method with refined arguments", + arguments: z.object({ key: z.string().optional() }).refine( + (v) => v.key !== "forbidden", + { message: "key cannot be 'forbidden'" }, + ), + execute: () => Promise.resolve({ dataHandles: [] }), + }, + }, + }; + const deps = createTestDeps(definition, modelDef); + + const ctx = createLibSwampContext(); + const events = await collect( + modelMethodRun(ctx, deps, { + ...createTestInput("refined-model", "run"), + inputs: { typoKey: "oops" }, + }), + ); + + const errorEvent = events.find((e) => e.kind === "error"); + assertEquals(errorEvent?.kind, "error"); + if (errorEvent?.kind === "error") { + assertEquals(errorEvent.error.code, "validation_failed"); + assertEquals(errorEvent.error.message.includes("typoKey"), true); + } +}); + // --- Sensitive argument registration tests --- Deno.test("modelMethodRun: registers sensitive string-array method arg values with the redactor", async () => {