From a581480f14ad282315a09270bb7fec162f3ecef3 Mon Sep 17 00:00:00 2001 From: David J Parrott Date: Sun, 8 Nov 2026 12:02:51 -0500 Subject: [PATCH 1/2] Map unmapped 4xx statuses to bad_request error_codes.forStatus derives the envelope code for a bare status (route ctx.fail, typed-route jsonError, auth-method Resolution.fail). Every status without a dedicated code fell back to `internal`, so a deliberate client error such as 422 reached clients labelled as a server fault. In auth-method dispatch, `internal` also triggers redaction: the method's 4xx message was logged and replaced with a generic 500. Unmapped 4xx statuses now map to `bad_request`; unmapped 5xx and other statuses stay `internal`. Auth-method failures with a named 4xx status therefore return their message to the anonymous caller with their own status. This is intentional: Resolution.fail and the framework guide now state that these messages are public explanations. Messages that cannot be sent as a client error stay operator-only. A 5xx, non-error, or transport-unnamed status (such as 432, which response normalization would replace with a 500 and whose message would otherwise be lost) is logged and redacted to the generic internal response. http.transportStatus, isFinalStatus and isErrorStatus centralize this status validity rule, and server response normalization now uses the same helpers. The `internal` and `bad_request` registry explanations (shown by `explain-code`) and the bare-ApiError guard comment now describe the new split; `bad_request` notes that retryability follows the HTTP status rather than the code. A dispatch test sends 401, 422 and 451 through response normalization and checks that 432, 500, 503 and 200 are redacted. The redaction log drops to warn under test, as webauthn and postgres TLS already do, because the test runner fails the build on any logged error. The allocator allowlist counts the test. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HTAHrkfe1L2hfpLeP1CQAS --- changelog.d/record-hook-client-errors.md | 2 + docs/framework.md | 8 +++ scripts/allocator-allowlist.txt | 2 +- src/api/auth_methods.zig | 64 +++++++++++++++++++++--- src/api/error.zig | 9 ++-- src/auth/method.zig | 4 ++ src/ctx.zig | 2 +- src/error_codes.zig | 26 ++++++---- src/http.zig | 28 +++++++++++ src/server.zig | 12 ++--- 10 files changed, 127 insertions(+), 30 deletions(-) create mode 100644 changelog.d/record-hook-client-errors.md diff --git a/changelog.d/record-hook-client-errors.md b/changelog.d/record-hook-client-errors.md new file mode 100644 index 00000000..a6c30d6d --- /dev/null +++ b/changelog.d/record-hook-client-errors.md @@ -0,0 +1,2 @@ +### Fixes +- Unmapped 4xx statuses use the generic `bad_request` code consistently in typed routes, route `ctx.fail`, and auth-method failures. Auth-method failure messages with a named 4xx status are deliberately client-facing, including 422; these previously became a generic 500 for statuses without dedicated codes. Auth-method 5xx and unnamed-status (such as 432) messages remain operator-only: logged and redacted from client responses. diff --git a/docs/framework.md b/docs/framework.md index a052f394..221c6672 100644 --- a/docs/framework.md +++ b/docs/framework.md @@ -2648,6 +2648,14 @@ ZigBase ships with a **pluggable auth-method system** that lets every auth colle 1. **Initiate** — challenge delivery (email a link/code, return WebAuthn options, or a no-op for API-key flows). 2. **Complete** — proof verification. On success the method returns a `Resolution.record(record_id)` and the framework mints the session via the one seam (`issueSession`+`emitAuth`), firing `onAuth`. The method never mints sessions itself. +Auth-method `Resolution.fail` messages for the transport's named 4xx statuses +(`http.isErrorStatus`) are intentionally public and returned to the anonymous +caller. A status without a dedicated machine code (such as 422) uses +`bad_request`. Write these as client explanations, without operator secrets. +Server-error and unnamed-status (such as 432) failure messages remain +operator-only: they are logged and replaced with the generic 500 `internal` +response. + There are three tiers: | Tier | How | When | diff --git a/scripts/allocator-allowlist.txt b/scripts/allocator-allowlist.txt index fd7ee22f..789f5cdf 100644 --- a/scripts/allocator-allowlist.txt +++ b/scripts/allocator-allowlist.txt @@ -2,7 +2,7 @@ # A test may wrap testing.allocator in an arena ONLY if the code under test takes # RequestArena (contract 4). Every line here is a promise to remove it or justify it. src/api/auth.zig 41 3 non-handler outliers un-masked: a dead unused arena in the gcExpiredSessions test; the backlog test whose arena only held transient session-id strings; and the "findByIdentity nocase" test (now that records.zig's create/error-path partial-record leak is fixed, #334, it frees col via Collection.deinit, the applyCreate graphs via freeProvisioned, the record via freeRecord, and each findByIdentity hit — a duped id string — after its compare). The rehash-on-login tests un-mask two more: "verifyPasswordUpgrading verifies a legacy hash…" and "…leaves an argon2 hash untouched" call verifyPasswordUpgrading directly (no RequestArena/Response graph), so they run on raw std.testing.allocator via the seedRawHash helper, which owns and frees every intermediate (fetched Collection via Collection.deinit, the applyCreate/records.create graphs, the UPDATE SQL, the returned record id). 41 remain: genuine handler tests holding a RequestArena/Response graph (contract-4), including the require_verified login test (email_not_verified) and "#80/legacy-upgrade: a hook-free epoch login over a legacy hash acquires the writer EXACTLY once", both of which drive authWithPassword through a real RequestArena — the exact production shape. -src/api/auth_methods.zig 11 contract-4: all 11 tests drive public auth-method dispatch through env.ctx with a real RequestArena, including the two-factor gate for every primary-method resolution. Response bodies and cookies are reclaimed at request end. +src/api/auth_methods.zig 12 contract-4: all 12 tests drive public auth-method dispatch through env.ctx with a real RequestArena, including the two-factor gate for every primary-method resolution and normalized client-failure envelopes. Response bodies and cookies are reclaimed at request end. src/api/two_factor.zig 1 contract-4: the integration test drives RequestCtx handlers through password login, factor completion, and refresh; response bodies, cookies, and policy graphs use the RequestArena lifetime required by those production APIs. Primitive storage and cryptography tests use the raw testing allocator. src/api/collections.zig 9 contract-4: all 9 tests drive the collection create/get/list/update/delete handlers through ctxFor(env, RequestArena.from(&arena), ...) — a real http.RequestCtx whose .allocator is a RequestArena, the exact production shape; the handlers allocate their whole http.Response graph (serialized collection JSON, field-error bodies) on ctx.allocator.a, reclaimed wholesale at request end. src/api/features.zig 4 contract-4: all 4 tests drive the GET /api/features handler through ctxFor(env, RequestArena.from(&arena), ...) — a real http.RequestCtx whose .allocator is a RequestArena; the response graph (declared flags/experiments JSON incl. override values) is allocated on ctx.allocator.a, the exact production shape. diff --git a/src/api/auth_methods.zig b/src/api/auth_methods.zig index 1f7f1053..a7766d3b 100644 --- a/src/api/auth_methods.zig +++ b/src/api/auth_methods.zig @@ -154,12 +154,18 @@ fn dispatch(ctx: *http.RequestCtx, phase: DispatchPhase) anyerror!http.Response // code of its own — derive one from the status, same as route_types.zig's // jsonError does for typed routes. const derived = codes.forStatus(f.status); - // `internal` promises in the frozen registry that no detail is - // leaked, so a 5xx must NOT forward the method's own message — - // that text is written for operators, not for an anonymous caller. - // Log it (the operator still needs it) and answer generically. - if (derived == .internal) { - std.log.err("auth method failed with {d}: {s}", .{ f.status, f.message }); + // A failure message with a named 4xx status is intentionally + // client-facing, including statuses without a dedicated code + // (422 -> bad_request). `internal` promises no detail is leaked: + // messages attached to 5xx, non-error, or transport-unnamed statuses + // (e.g. 432, which response normalization would replace with a 500) + // are operator-only. Log and redact them. + if (derived == .internal or !http.isErrorStatus(f.status)) { + // warn under test: the test runner fails the build on any logged error. + if (@import("builtin").is_test) + std.log.warn("auth method failed with {d}: {s}", .{ f.status, f.message }) + else + std.log.err("auth method failed with {d}: {s}", .{ f.status, f.message }); return ApiError.internal().toResponse(ctx.allocator.a); } return (ApiError{ .status = f.status, .code = codes.s(derived), .message = f.message }).toResponse(ctx.allocator.a); @@ -288,6 +294,52 @@ test "every primary method resolution enters the second-factor gate" { // Tests // --------------------------------------------------------------------------- +test "auth-method client failures preserve their public message without a dedicated status code" { + const env = try auth.TestEnv.initAuth("users"); + defer env.deinit(); + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const Failure = struct { + status: u16, + fn initiate(_: *anyopaque, _: *method_mod.AuthCtx) anyerror!method_mod.InitiateResult { + return .{}; + } + fn complete(ptr: *anyopaque, _: *method_mod.AuthCtx) anyerror!method_mod.Resolution { + const self: *@This() = @ptrCast(@alignCast(ptr)); + return .{ .fail = .{ .status = self.status, .message = "Choose an allowed account option." } }; + } + const vt = method_mod.AuthMethod.VTable{ .initiate = @This().initiate, .complete = @This().complete }; + }; + var failure = Failure{ .status = 422 }; + const methods = [_]method_mod.AuthMethod{.{ .slug = "password", .ctx = &failure, .vtable = &Failure.vt }}; + var registry = registry_mod.Registry{ .methods = &methods }; + env.app.auth_methods = @ptrCast(®istry); + const params = [_]http.Param{ .{ .key = "col", .value = "users" }, .{ .key = "method", .value = "password" } }; + for ([_]u16{ 401, 422, 451 }) |status| { + failure.status = status; + var ctx = env.ctx(RequestArena.from(&arena), .POST, "{}", ¶ms); + const response = try complete(&ctx); + const normalized = try @import("../server.zig").normalizeResponse(&ctx, response); + try std.testing.expectEqual(status, normalized.status); + const envelope = (try std.json.parseFromSlice(std.json.Value, arena.allocator(), normalized.body, .{})).value.object; + try std.testing.expectEqualStrings(if (status == 401) "unauthorized" else "bad_request", envelope.get("code").?.string); + try std.testing.expectEqualStrings("Choose an allowed account option.", envelope.get("message").?.string); + try std.testing.expectEqual(@as(i64, status), envelope.get("status").?.integer); + try std.testing.expectEqual(@as(usize, 0), response.cookies.len); + } + // 432 has no transport name; its message must be redacted like a 5xx, not lost to normalization. + for ([_]u16{ 432, 500, 503, 200 }) |status| { + failure.status = status; + var ctx = env.ctx(RequestArena.from(&arena), .POST, "{}", ¶ms); + const response = try complete(&ctx); + const normalized = try @import("../server.zig").normalizeResponse(&ctx, response); + try std.testing.expectEqual(@as(u16, 500), normalized.status); + const envelope = (try std.json.parseFromSlice(std.json.Value, arena.allocator(), normalized.body, .{})).value.object; + try std.testing.expectEqualStrings("internal", envelope.get("code").?.string); + try std.testing.expect(std.mem.indexOf(u8, normalized.body, "Choose an allowed") == null); + } +} + test "auth-method dispatch: password complete succeeds + 2 cookies + onAuth fires" { const api_auth = @import("auth.zig"); diff --git a/src/api/error.zig b/src/api/error.zig index d02ecc7d..3ba57c1c 100644 --- a/src/api/error.zig +++ b/src/api/error.zig @@ -184,14 +184,15 @@ fn parseLiteralStatus(span: []const u8) ?u16 { // Regression guard (SP-1 Task 2 follow-up): walks every `src/**/*.zig` file at test // time and flags a bare `ApiError{ .status = N, .message = … }` struct-literal // construction that omits `.code` while `N` is a status `error_codes.forStatus` maps -// to something OTHER than `.internal` (400/401/403/404/409/410/429 today, read live -// off `forStatus` so this guard tracks it automatically). Such a literal silently +// to something OTHER than `.internal` (every 4xx today: the dedicated codes for +// 400/401/403/404/409/410/413/429 and `bad_request` for the rest, read live off +// `forStatus` so this guard tracks it automatically). Such a literal silently // falls back to `ApiError`'s default `code: []const u8 = codes.s(.internal)` even // though a specific registered code exists for its status — the exact defect this // task fixed at ~30 call sites (see task-2-report.md, "Concern 2"). // -// Literals at 500, or any other status `forStatus` does not specifically map, are -// NOT flagged: `internal` is the documented, correct default there. +// Literals at 5xx or other non-4xx statuses, which `forStatus` maps to `internal`, +// are NOT flagged: `internal` is the documented, correct default there. // // Limits (stated, not hidden): this is a textual brace-balancing scan, not a Zig // parser. It only understands a single `ApiError{ ... }` literal per match (nested diff --git a/src/auth/method.zig b/src/auth/method.zig index 6bf16226..39158b6d 100644 --- a/src/auth/method.zig +++ b/src/auth/method.zig @@ -5,6 +5,10 @@ pub const InitiateResult = struct { status: u16 = 200, body: ?[]const u8 = null pub const Resolution = union(enum) { record: []const u8, // resolved record id → framework mints session + onAuth + /// Messages for the transport's named 4xx statuses are public explanations for + /// the anonymous caller, including statuses without a dedicated error code. + /// Server-error and unnamed-status messages are logged for operators and + /// replaced with the generic internal error response. fail: struct { status: u16, message: []const u8 }, }; diff --git a/src/ctx.zig b/src/ctx.zig index 3e02bb72..abf43100 100644 --- a/src/ctx.zig +++ b/src/ctx.zig @@ -2189,7 +2189,7 @@ test "errorResponse maps known errors to status codes, unknown to 500" { try std.testing.expectEqual(@as(u16, 500), ctx.errorResponse(error.SomethingWeird).status); // ctx.fail stashes a custom message that errorResponse(error.Handled) renders. The code is - // derived from status (422 has no specific mapping, so it falls back to "internal" — see + // derived from status (422 has no specific mapping, so it falls back to "bad_request" — see // the next case for a status that DOES have a specific code). const e = ctx.fail(422, "nope"); try std.testing.expectError(error.Handled, @as(error{Handled}!void, e)); diff --git a/src/error_codes.zig b/src/error_codes.zig index 195818fe..75919860 100644 --- a/src/error_codes.zig +++ b/src/error_codes.zig @@ -96,11 +96,16 @@ pub fn info(c: Code) Info { .explanation = \\A catch-all for requests that fail validation ZigBase doesn't attach to a \\specific field — malformed query parameters, an unparseable filter/sort - \\expression, a bad cursor, or another request-shape problem the handler - \\rejected before doing any work. + \\expression, a bad cursor, or another request-shape problem. \\ - \\Read `message` for the specific cause and fix the request; this code is - \\not retried automatically. + \\Also the generic code for any 4xx status without a dedicated code (see + \\`forStatus`), e.g. a `ctx.fail(422, …)` from a route or hook, which may + \\reject the request after work was done and rolled back. There the HTTP + \\`status` carries the specific meaning. + \\ + \\Read `message` for the specific cause. ZigBase does not retry this code + \\automatically; whether a retry can succeed follows the HTTP `status` + \\(e.g. 408, 423 and 425 are retryable, 400 and 422 need a changed request). , }, .collections_frozen => .{ @@ -173,8 +178,9 @@ pub fn info(c: Code) Info { \\a bug. `message` is deliberately generic so internals never leak into \\the response. \\ - \\Also the default for any HTTP status this registry doesn't otherwise - \\map (see `forStatus`) — receiving `internal` for a request that "should" + \\Also the default for any 5xx or non-error HTTP status this registry + \\doesn't otherwise map (see `forStatus`; unmapped 4xx statuses use + \\`bad_request`) — receiving `internal` for a request that "should" \\have a specific code is itself a signal to check the server logs. , }, @@ -500,7 +506,7 @@ pub fn info(c: Code) Info { /// The envelope code for a bare HTTP status, used where an error reaches the /// renderer without one (e.g. `ctx.fail(status, message)`). Deliberately -/// conservative: anything unmapped is `internal`, never an invented code. +/// conservative: unmapped 4xx use `bad_request`; other unmapped statuses use `internal`. pub fn forStatus(status: u16) Code { return switch (status) { 400 => .bad_request, @@ -511,7 +517,7 @@ pub fn forStatus(status: u16) Code { 410 => .gone, 413 => .payload_too_large, 429 => .too_many_requests, - else => .internal, + else => if (status >= 400 and status < 500) .bad_request else .internal, }; } @@ -628,7 +634,7 @@ test "error_codes: forStatus maps every constructor's status" { // agent into a pointless retry loop. Regression guard for that exact conflation. try std.testing.expectEqual(Code.payload_too_large, forStatus(413)); try std.testing.expectEqual(Code.too_many_requests, forStatus(429)); - // Anything unmapped — including every 5xx — is `internal`, never a guess. + // Unmapped server errors stay internal; unmapped client errors stay client errors. try std.testing.expectEqual(Code.internal, forStatus(500)); - try std.testing.expectEqual(Code.internal, forStatus(418)); + try std.testing.expectEqual(Code.bad_request, forStatus(418)); } diff --git a/src/http.zig b/src/http.zig index 89ad00b0..646f3e22 100644 --- a/src/http.zig +++ b/src/http.zig @@ -4,6 +4,34 @@ const App = @import("app.zig").App; pub const Method = enum { GET, POST, PUT, PATCH, DELETE, OPTIONS, HEAD, UNKNOWN }; +/// Map only the transport's named statuses. Its non-exhaustive enum also accepts +/// unnamed integers, which the socket adapter cannot preserve as API responses. +pub fn transportStatus(status: u16) ?@import("zap").http.StatusCode { + const StatusCode = @import("zap").http.StatusCode; + inline for (std.meta.fields(StatusCode)) |field| { + if (field.value == status) return @field(StatusCode, field.name); + } + return null; +} + +/// Whether the transport can send this completed application response. +/// Conditional 304 method restrictions are enforced by response normalization. +pub fn isFinalStatus(status: u16) bool { + return status >= 200 and transportStatus(status) != null; +} + +/// Explicit failures must be final, transport-supported 4xx or 5xx responses. +pub fn isErrorStatus(status: u16) bool { + return status >= 400 and status < 600 and isFinalStatus(status); +} + +test "transportStatus accepts named client errors and rejects unnamed statuses" { + for ([_]u16{ 400, 409, 418, 421, 422, 431, 451, 500 }) |status| + try std.testing.expect(transportStatus(status) != null); + for ([_]u16{ 419, 420, 427, 430, 432, 499, 799 }) |status| + try std.testing.expectEqual(null, transportStatus(status)); +} + pub const Param = struct { key: []const u8, value: []const u8 }; pub const RequestCtx = struct { diff --git a/src/server.zig b/src/server.zig index 4c9d39db..96a533b0 100644 --- a/src/server.zig +++ b/src/server.zig @@ -846,10 +846,7 @@ fn zapHeaderSet(ctx: *anyopaque, name: []const u8, value: []const u8) void { /// turns omitted-but-valid codes (e.g. 401 auth rejections, 307 magic-link redirects, /// 405, 415) into 500s. Anything zap doesn't name falls back to 500. fn statusToCode(status: u16) zap.http.StatusCode { - inline for (std.meta.fields(zap.http.StatusCode)) |field| { - if (field.value == status) return @field(zap.http.StatusCode, field.name); - } - return .internal_server_error; + return http.transportStatus(status) orelse .internal_server_error; } test "statusToCode maps named codes and falls back to 500 for unknown" { @@ -1955,9 +1952,8 @@ pub fn normalizeResponse(ctx: *http.RequestCtx, response: http.Response) !http.R // reach here. Likewise, 304 is only meaningful for GET/HEAD, and zap cannot put an // unnamed status on the wire. Turn every invalid application status into one coherent // final response so the wire, body, and access log all agree. - const invalid_status = (response.status >= 100 and response.status < 200) or - (response.status == 304 and ctx.method != .GET and ctx.method != .HEAD) or - (response.status != 500 and statusToCode(response.status) == .internal_server_error); + const invalid_status = !http.isFinalStatus(response.status) or + (response.status == 304 and ctx.method != .GET and ctx.method != .HEAD); var normalized = response; if (invalid_status) { reportInvalidResponseStatus(ctx, response.status); @@ -2252,7 +2248,7 @@ test "normalizeResponse maps invalid final statuses to one framed 500" { .allocator = RequestArena.from(&arena), .app = null, }; - for ([_]u16{ 100, 103, 199, 799 }) |status| { + for ([_]u16{ 100, 103, 199, 432, 499, 799 }) |status| { const normalized = try normalizeResponse(&ctx, .{ .status = status, .body = "wrong" }); try std.testing.expectEqual(@as(u16, 500), normalized.status); try std.testing.expectEqualStrings("application/json", normalized.content_type); From c8f4a4a04469d994af1276ec7b8f769f342a51e9 Mon Sep 17 00:00:00 2001 From: David J Parrott Date: Sun, 8 Nov 2026 12:02:51 -0500 Subject: [PATCH 2/2] Preserve record before-hook error responses A record before-hook that rejected a write with ctx.fail or ctx.invalid always produced a generic 400 "Request rejected by a hook.", discarding the status, message and field details the hook chose. Custom routes already render those via Ctx.errorResponse. The HTTP create/update/delete paths now render the hook's failure with Ctx.errorResponse while the hook Ctx is still alive, for any transport- supported 4xx or 5xx status (http.isErrorStatus). Any other error, a bare error.Handled, or an unsupported status keeps the generic 400. The primary write and the hook's side-writes still roll back together. ctx.fail and ctx.invalid now copy the message and every field string into Ctx-owned memory, so hooks and routes may pass stack buffers. The record dispatcher clears any stored failure before each handler, so a failure a handler caught and recovered from cannot be reported by a later bare error.Handled. Allocation failure falls back to a generic internal error. Rendered 5xx failures invoke onError and the reporter backstop once, with phase `before_hook`, after rollback and writer release; the callback can therefore write through the writer. Deliberate 4xx failures stay silent. Each report's message is ": ", built in the request arena while the Ctx owns the failure. Reporter dedup keys on (message, phase), so distinct failures inside one window are each delivered and a repeat of the same failure is still suppressed. The event's err is error.OutOfMemory when the response is the allocation fallback, and error.Handled otherwise. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HTAHrkfe1L2hfpLeP1CQAS --- changelog.d/record-hook-client-errors.md | 1 + docs/framework.md | 34 +- docs/recipes.md | 7 +- scripts/allocator-allowlist.txt | 2 +- src/api/records.zig | 513 ++++++++++++++++++++--- src/ctx.zig | 118 +++++- src/events.zig | 1 + 7 files changed, 618 insertions(+), 58 deletions(-) diff --git a/changelog.d/record-hook-client-errors.md b/changelog.d/record-hook-client-errors.md index a6c30d6d..4f7de31d 100644 --- a/changelog.d/record-hook-client-errors.md +++ b/changelog.d/record-hook-client-errors.md @@ -1,2 +1,3 @@ ### Fixes +- Record before-create, before-update, and before-delete hooks preserve explicit `ctx.fail` 4xx and 5xx statuses supported by the HTTP transport and their messages, and `ctx.invalid` field details. Stack-backed error messages and fields are copied before hooks return; recovered failures cannot leak into later handlers. A supported status without a dedicated code, such as 422, uses `bad_request`. Other hook failures, including unnamed 4xx statuses such as 432, retain the generic 400 response, and rejected writes still roll back atomically. Hook 5xx responses invoke `onError` and the reporter backstop with phase `before_hook`, including allocation-failure fallback responses; each report's message names the collection, status, and hook message. - Unmapped 4xx statuses use the generic `bad_request` code consistently in typed routes, route `ctx.fail`, and auth-method failures. Auth-method failure messages with a named 4xx status are deliberately client-facing, including 422; these previously became a generic 500 for statuses without dedicated codes. Auth-method 5xx and unnamed-status (such as 432) messages remain operator-only: logged and redacted from client responses. diff --git a/docs/framework.md b/docs/framework.md index 221c6672..30489406 100644 --- a/docs/framework.md +++ b/docs/framework.md @@ -304,6 +304,32 @@ The six valid phase fields are `beforeCreate`, `afterCreate`, `beforeUpdate`, `any` group runs first, then the collection-specific group; only the field matching the current phase runs. +On HTTP record create, update, and delete, a before-hook can return +`ctx.fail(status, message)` to reject the write with a **supported error** +status and message in the standard error envelope. Supported statuses are +the transport’s named 4xx and 5xx statuses (`http.isErrorStatus`). The machine `code` follows the same +status mapping as other `ctx.fail` callers (`409` → `conflict`; a supported status +without a specific code uses `bad_request` for 4xx and `internal` for 5xx). `ctx.invalid(fields)` returns a 400 validation envelope +with the supplied field details. The primary write and the hook's database +side-writes roll back together. After rollback and writer release, rendered 5xx +failures invoke `onError` and its reporter backstop with phase `before_hook`; +deliberate 4xx failures do not. The event's `message` is +`: `, so reporter dedup keeps distinct failures +apart. Its `err` is `error.Handled`, or `error.OutOfMemory` when the response is the +allocation-failure fallback. +For example: + +```zig +return ctx.fail(409, "That item is already queued."); +``` + +Any other returned error, `error.Handled` without a stashed error, or an +unsupported `ctx.fail` status (including unnamed 4xx values such as 432 or 499) +retains the generic 400 response +(`bad_request`, `Request rejected by a hook.`). Internal error names are never +returned to the client. After-hook errors keep their existing error-reporting +behavior and do not change the committed write's response. + Every hook has the signature: ```zig @@ -332,7 +358,8 @@ Need the app itself (e.g. `app.allocator`, `app.io`)? Use the hook's `ctx.app` ### Semantics - **`before*` hooks** may MUTATE `ev.record` and may return an error to REJECT - the write (the request fails with `400`). Early locked-rule and tenant gates + the write (explicit supported `ctx.fail` statuses and `ctx.invalid` details + are preserved; other errors return generic `400`). Early locked-rule and tenant gates run first; conditional create/update predicates are checked in the write transaction after the hook. A later denial rolls back its database side-writes. A before-hook is not proof of final authorization: keep external side effects @@ -1732,7 +1759,10 @@ fn confirmBooking(ctx: *zigbase.Ctx) anyerror!zigbase.http.Response { } ``` -`ctx.fail(status, message)` stashes the error and returns `error.Handled`. +`ctx.fail(status, message)` copies the error and returns `error.Handled`. +`ctx.fail` and `ctx.invalid` copy all supplied strings and field descriptors +before returning, so stack buffers are safe. The `Ctx` owns these copies until +`deinit` or the next handler invocation; a rendered response owns its separate body. `ctx.errorResponse(err)` maps any error to a `{code, message, data}` JSON response: | `anyerror` | HTTP status | diff --git a/docs/recipes.md b/docs/recipes.md index 52a5b0d7..63d9ceef 100644 --- a/docs/recipes.md +++ b/docs/recipes.md @@ -449,8 +449,11 @@ to control built-in record operations. A route guard does not replace record rul A `beforeCreate` hook runs after early locked-rule/tenant gates and **before** the write. A conditional create rule is checked afterward, inside the same transaction; a denial rolls back the hook's database side-writes. Put external side effects in -after-hooks. The hook may mutate `ev.record` and may **return an error to reject** the write (the request -fails with `400`). This example, on `bookings`, (a) rejects a booking that overlaps +after-hooks. The hook may mutate `ev.record` and may **return an error to reject** the write. +Use `ctx.fail(status, message)` with a [supported error status](framework.md#4-record-hooks-hooks) +(for example, 409) for a specific status and explanation, or +`ctx.invalid(fields)` for field validation details; other errors return a generic +`400`. This example, on `bookings`, (a) rejects a booking that overlaps an existing confirmed booking for the same listing, and (b) computes a derived `duration_minutes` field. diff --git a/scripts/allocator-allowlist.txt b/scripts/allocator-allowlist.txt index 789f5cdf..386160f5 100644 --- a/scripts/allocator-allowlist.txt +++ b/scripts/allocator-allowlist.txt @@ -11,7 +11,7 @@ src/api/health.zig 1 contract-4: the single test drives the health handler throu src/api/magic_link_consume.zig 7 contract-4: 6 tests drive the public consume GET handler through consumeCtx(...) — a real http.RequestCtx whose .allocator is a RequestArena; the handler's 302/400/404 Response graphs (Location header, session Set-Cookies, confirm bodies) live on ctx.allocator.a, the exact production shape. The 7th is setupConsumeEnv's setup-only arena around collections.create provisioning scaffolding (never touched by the SUT). src/api/mail_unsubscribe.zig 6 contract-4: all 6 tests drive the public post()/get() handlers through a real http.RequestCtx whose .allocator is a RequestArena (RequestArena.from(&arena)) — the exact production shape. The handler allocates its whole response graph on ctx.allocator.a (the badRequest/notFound/429 responses, the confirm-page HTML, the one_click: source string, and unsubscribe.verify's owned Parts) and never individually frees it, exactly as a request-arena consumer should. Reclaimed wholesale at request end. src/api/oauth.zig 6 The "deleting an auth record removes its external-auth links" outlier is un-masked (seedOAuthCollection now self-frees its encrypted-secret blob + created collection; the test frees the fetched collection via Collection.deinit) and runs under std.testing.allocator. 6 remain: genuine handler tests holding a RequestArena/Response graph (contract-4), including the independently named cross-collection OAuth regression using the resolver through a real RequestCtx. -src/api/records.zig 38 contract-4: 37 tests drive the records create/list/view/update/delete + multipart handlers through ctxFor/formCtx — a real http.RequestCtx whose .allocator is a RequestArena; each handler's http.Response graph is allocated on ctx.allocator.a and reclaimed wholesale at request end. Includes upload and durable cleanup job Ctx coverage, including renamed physical namespaces. The 38th is TestEnv.init's setup-only arena around collections.create provisioning scaffolding. +src/api/records.zig 39 contract-4: 37 tests drive the records create/list/view/update/delete + multipart handlers through ctxFor/formCtx — a real http.RequestCtx whose .allocator is a RequestArena; each handler's http.Response graph is allocated on ctx.allocator.a and reclaimed wholesale at request end. Includes upload and durable cleanup job Ctx coverage, including renamed physical namespaces. The 38th is TestEnv.init's setup-only arena around collections.create provisioning scaffolding. The onError-after-rollback test drives real create/update/delete handlers with a RequestArena response graph. src/api/sessions.zig 6 contract-4: all 6 tests drive the sessions REST handlers (list/get/revoke/revoke-all, plus login via api/auth to mint tokens) through bearerCtx/login/env.ctx — real http.RequestCtx whose .allocator is a RequestArena; the {items} pages, minted tokens and cookie-bearing Response graphs live on ctx.allocator.a, the exact production shape. src/api/settings.zig 2 contract-4: both tests drive the settings GET/PUT/DELETE handlers through ctxFor(env, RequestArena.from(&arena), ...) — a real http.RequestCtx whose .allocator is a RequestArena; response bodies live on ctx.allocator.a, the exact production shape. src/api/state.zig 5 contract-4: all 5 tests drive the public GET /api/state handler through a real http.RequestCtx whose .allocator is a RequestArena (incl. the #230 cache-probe helper that takes a RequestArena directly); the resolved flags/experiments JSON is allocated on ctx.allocator.a, the exact production shape. diff --git a/src/api/records.zig b/src/api/records.zig index 2a5e9501..7d91b8b2 100644 --- a/src/api/records.zig +++ b/src/api/records.zig @@ -38,15 +38,30 @@ pub fn emitRecord( value: *std.json.Value, phase: events.RecordPhase, ) !void { - const d = app.dispatch orelse return; - const handler = d.record orelse return; + _ = try emitRecordResponse(app, rctx, arena, conn, col_name, value, phase, false); +} + +/// HTTP callers render via Ctx while it owns the copied failure payload. +/// Non-HTTP callers retain error propagation through emitRecord. +fn emitRecordResponse( + app: *app_mod.App, + rctx: *const request.RequestContext, + arena: RequestArena, + conn: *db.Db, + col_name: []const u8, + value: *std.json.Value, + phase: events.RecordPhase, + render_failure: bool, +) !?HookRejection { + const d = app.dispatch orelse return null; + const handler = d.record orelse return null; const is_before = switch (phase) { .before_create, .before_update, .before_delete => true, else => false, }; // before-hooks observe the raw request body; skip non-object bodies so a hook's // `ev.record.object` access can't panic. records.* will reject non-objects with NotObject (400). - if (is_before and value.* != .object) return; + if (is_before and value.* != .object) return null; var ev = events.RecordEvent{ .rctx = rctx, .arena = arena, @@ -61,13 +76,25 @@ pub fn emitRecord( var ctx = Ctx{ .app = app, .arena = arena, .rctx = rctx.*, .bound_conn = conn }; defer ctx.deinit(); if (is_before) { - try handler(&ctx, &ev); + handler(&ctx, &ev) catch |err| { + if (render_failure) { + if (err == error.Handled) if (ctx.handled) |handled| { + if (http.isErrorStatus(handled.status)) { + const response = ctx.errorResponse(err); + return .{ .response = response, .incident = hookIncident(&ctx, arena, col_name, handled, response) }; + } + }; + return .{ .response = try ApiError.badRequest("Request rejected by a hook.").toResponse(arena.a) }; + } + return err; + }; } else { handler(&ctx, &ev) catch |e| { var err_ev = events.ErrorEvent{ .app = app, .ctx = rctx, .err = e, .phase = .after_hook, .message = @errorName(e) }; events.dispatchError(app, app.dispatch, &err_ev); }; } + return null; } /// Fire file.afterUpload once per written file. After-style: never propagates. @@ -130,10 +157,54 @@ fn forbidden(ctx: *http.RequestCtx) !http.Response { return ApiError.forbidden().toResponse(ctx.allocator.a); } -fn hookRejected(ctx: *http.RequestCtx) anyerror!http.Response { - return ApiError.badRequest("Request rejected by a hook.").toResponse(ctx.allocator.a); +fn emitBeforeRecord(ctx: *http.RequestCtx, rctx: *const request.RequestContext, conn: *db.Db, col_name: []const u8, value: *std.json.Value, phase: events.RecordPhase) !?HookRejection { + return emitRecordResponse(ctx.app.?, rctx, ctx.allocator, conn, col_name, value, phase, true); } +/// The rendered response for a rejecting before-hook, plus what to report if it is a 5xx. +const HookRejection = struct { + response: http.Response, + incident: ?HookIncident = null, +}; + +const HookIncident = struct { + err: anyerror, + /// Request-arena owned, or `hook_incident_fallback_message`. + message: []const u8, +}; + +const hook_incident_fallback_message = "record before-hook failed; report allocation failed"; + +// Built while the hook Ctx still owns `handled`. Reporter dedup keys on this text, +// so it must tell apart failures from different collections, statuses and messages. +fn hookIncident(ctx: *const Ctx, arena: RequestArena, col_name: []const u8, handled: ApiError, response: http.Response) ?HookIncident { + if (response.status < 500) return null; + return .{ + .err = if (ctx.usedAllocationFallback(response)) error.OutOfMemory else error.Handled, + .message = std.fmt.allocPrint(arena.a, "{s}: {d} {s}", .{ col_name, handled.status, handled.message }) catch hook_incident_fallback_message, + }; +} + +// Register this defer before acquiring the writer. A consumer onError callback +// may write to the database; it must run after rollback and writer release. +const HookErrorReport = struct { + app: *app_mod.App, + pending: ?Pending = null, + + const Pending = struct { rctx: request.RequestContext, incident: HookIncident }; + + fn capture(self: *HookErrorReport, rejection: HookRejection, rctx: request.RequestContext) void { + if (rejection.incident) |incident| self.pending = .{ .rctx = rctx, .incident = incident }; + } + + fn dispatch(self: *HookErrorReport) void { + if (self.pending) |*pending| { + var ev = events.ErrorEvent{ .app = self.app, .ctx = &pending.rctx, .err = pending.incident.err, .phase = .before_hook, .message = pending.incident.message }; + events.dispatchError(self.app, self.app.dispatch, &ev); + } + } +}; + /// For auth collections, transform request data through auth.applyCreate/applyUpdate /// (hash password, gen/rotate tokenKey, strip plaintext, force verified=false on create). /// For non-auth collections, returns `data` unchanged. Maps a bad/short/missing password to BadPassword. @@ -390,6 +461,8 @@ pub fn create(ctx: *http.RequestCtx) anyerror!http.Response { writeUploads(ctx, col, &upload_id, all.writes) catch return ApiError.internal().toResponse(ctx.allocator.a); + var hook_error = HookErrorReport{ .app = app }; + defer hook_error.dispatch(); const w = app.pool.acquireWriter(); defer app.pool.releaseWriter(); const rctx = buildContext(ctx, w, data); @@ -421,10 +494,11 @@ pub fn create(ctx: *http.RequestCtx) anyerror!http.Response { // local; downstream MUST read the `_mut` binding (here and for rec_mut/ur_mut/ex_mut // below), not the original const — the binding is the grow-capturing reference. var data_mut = data2; - emitRecord(app, &rctx, ctx.allocator, w, col.name, &data_mut, .before_create) catch { + if (try emitBeforeRecord(ctx, &rctx, w, col.name, &data_mut, .before_create)) |rejection| { + hook_error.capture(rejection, rctx); w.rollback() catch {}; - return hookRejected(ctx); - }; + return rejection.response; + } // #98: auth-collection registration hook. Fires INSIDE the create transaction, after the // generic before_create hook and before the INSERT. An abort rolls back the whole create // (fail closed) and returns the Ctx-mapped response. `data_mut` is the writable record. @@ -602,6 +676,8 @@ fn updateImpl(ctx: *http.RequestCtx, comptime is_resumable: bool, resumable: if if (upload_plan) |p| writeUploads(ctx, upload_lease.col.?, rid, p.writes) catch return ApiError.internal().toResponse(ctx.allocator.a); + var hook_error = HookErrorReport{ .app = app }; + defer hook_error.dispatch(); const w = app.pool.acquireWriter(); defer app.pool.releaseWriter(); var txn_open = false; @@ -696,9 +772,10 @@ fn updateImpl(ctx: *http.RequestCtx, comptime is_resumable: bool, resumable: if const old_files = if (cleanup_files) (try records.getFilesPhysical(ctx.allocator.a, w, col, rid)).? else null; var data_mut = data2; - emitRecord(app, &rctx, ctx.allocator, w, col.name, &data_mut, .before_update) catch { - return hookRejected(ctx); - }; + if (try emitBeforeRecord(ctx, &rctx, w, col.name, &data_mut, .before_update)) |rejection| { + hook_error.capture(rejection, rctx); + return rejection.response; + } // #98 on the PATCH path (spec §F1): `beforePasswordChange` fires ONLY when the prepared // data contains a password change, INSIDE the update transaction, with a READ-ONLY @@ -807,6 +884,8 @@ pub fn delete(ctx: *http.RequestCtx) anyerror!http.Response { return ApiError.badRequest("REST idempotency is not configured.").toResponse(ctx.allocator.a); } const app = ctx.app.?; + var hook_error = HookErrorReport{ .app = app }; + defer hook_error.dispatch(); const w = app.pool.acquireWriter(); defer app.pool.releaseWriter(); var col_lease = try resolveCollection(ctx, w); @@ -834,9 +913,10 @@ pub fn delete(ctx: *http.RequestCtx) anyerror!http.Response { return ApiError.conflict("Record changed before mutation; reload and retry.").toResponse(ctx.allocator.a); var ex_mut = existing; - emitRecord(app, &rctx, ctx.allocator, w, col.name, &ex_mut, .before_delete) catch { - return hookRejected(ctx); - }; + if (try emitBeforeRecord(ctx, &rctx, w, col.name, &ex_mut, .before_delete)) |rejection| { + hook_error.capture(rejection, rctx); + return rejection.response; + } // Realtime delete prep (#159, PR-6/PR-6b), INSIDE this transaction so a Postgres side-table // snapshot commits atomically with the delete. Returns the AT-REST (ciphertext) snapshot used // for delete authz — so LOCAL authz matches the cross-instance REMOTE path and the live @@ -2375,21 +2455,6 @@ test "creating an auth record without a password is a 400" { // the `audit` row survived (this test FAILS there) — proof the abort is atomic. // --------------------------------------------------------------------------- -fn auditThenRejectHook(ctx: *Ctx, ev: *events.RecordEvent) anyerror!void { - if (ev.phase != .before_create) return; - if (!std.mem.eql(u8, ev.collection, "posts")) return; // act only on the primary write - // Side-write into `audit` on the hook's IN-TRANSACTION connection (ctx.bound_conn). - // We call records.create directly with ev.arena rather than ctx.records().create — the - // latter would allocate the throwaway record on app.allocator and trip the test's - // leak detector; the connection (and thus the shared transaction) is identical. - const conn = ctx.bound_conn.?; - const acol = (try collections.get(ev.arena.a, conn, "audit")) orelse return error.AuditMissing; - var obj: std.json.ObjectMap = .empty; - try obj.put(ev.arena.a, "title", .{ .string = "audit-side-write" }); - _ = try records.create(ev.arena.a, ctx.app.io, conn, acol, .{ .object = obj }); - return error.HookRejected; -} - fn countRows(env: *TestEnv, table: []const u8) !i64 { var r = try env.pool.acquireReader(); defer env.pool.releaseReader(&r); @@ -2403,26 +2468,380 @@ fn countRows(env: *TestEnv, table: []const u8) !i64 { return st.columnInt(0); } -test "before-hook side-write rolls back with the triggering write on hook error" { - var env = try TestEnv.init(); - defer env.deinit(); - // A second collection the hook writes into. - try seedRuled(env, "audit", "@public", "@public", "@public"); +const RejectionHooks = struct { + fn reject(ctx: *Ctx, ev: *events.RecordEvent) anyerror!void { + const conn = ctx.bound_conn.?; + const audit = (try collections.get(ev.arena.a, conn, "audit")).?; + var obj: std.json.ObjectMap = .empty; + try obj.put(ev.arena.a, "title", .{ .string = "must roll back" }); + _ = try records.create(ev.arena.a, ctx.app.io, conn, audit, .{ .object = obj }); - const disp = events.Dispatch{ .record = auditThenRejectHook }; - env.app.dispatch = &disp; - defer env.app.dispatch = null; + return fail(ctx, ev); + } - var arena = std.heap.ArenaAllocator.init(std.testing.allocator); - defer arena.deinit(); - const col_param = [_]http.Param{.{ .key = "col", .value = "posts" }}; - var cctx = ctxFor(env, RequestArena.from(&arena), .POST, "{\"title\":\"hi\"}", &col_param); - const res = try create(&cctx); + fn fail(ctx: *Ctx, ev: *events.RecordEvent) anyerror!void { + const mode = ev.record.object.get("title").?.string; + if (std.mem.eql(u8, mode, "raw")) return error.PrivateHookFailure; + if (std.mem.eql(u8, mode, "stale")) { + try std.testing.expectEqual(error.Handled, ctx.fail(409, "Discarded failure.")); + return error.PrivateHookFailure; + } + if (std.mem.eql(u8, mode, "unhandled")) return error.Handled; + if (std.mem.eql(u8, mode, "invalid")) { + var name = "title".*; + var code = "validation_required".*; + var message = "Choose a title.".*; + var fields = [_]FieldError{.{ .field = &name, .code = &code, .message = &message }}; + return ctx.invalid(&fields); + } + var buffer: [64]u8 = undefined; + const message = try std.fmt.bufPrint(&buffer, "{s}", .{"That item is already queued."}); + return ctx.fail(try std.fmt.parseInt(u16, mode, 10), message); + } + const Case = struct { mode: []const u8, status: u16 = 400, code: []const u8 = "bad_request", message: []const u8 = "Request rejected by a hook." }; + const cases = [_]Case{ + .{ .mode = "400", .code = "bad_request", .message = "That item is already queued." }, + .{ .mode = "403", .status = 403, .code = "forbidden", .message = "That item is already queued." }, + .{ .mode = "409", .status = 409, .code = "conflict", .message = "That item is already queued." }, + .{ .mode = "418", .status = 418, .code = "bad_request", .message = "That item is already queued." }, + .{ .mode = "421", .status = 421, .code = "bad_request", .message = "That item is already queued." }, + .{ .mode = "422", .status = 422, .code = "bad_request", .message = "That item is already queued." }, + .{ .mode = "431", .status = 431, .code = "bad_request", .message = "That item is already queued." }, + .{ .mode = "451", .status = 451, .code = "bad_request", .message = "That item is already queued." }, + .{ .mode = "432" }, + .{ .mode = "499" }, + .{ .mode = "200" }, + .{ .mode = "302" }, + .{ .mode = "399" }, + .{ .mode = "500", .status = 500, .code = "internal", .message = "That item is already queued." }, + .{ .mode = "503", .status = 503, .code = "internal", .message = "That item is already queued." }, + .{ .mode = "65535" }, + .{ .mode = "raw" }, + .{ .mode = "stale" }, + .{ .mode = "unhandled" }, + .{ .mode = "invalid", .code = "validation_failed", .message = "Failed to validate the request." }, + }; - // The hook rejected the write -> 400, and BOTH writes rolled back atomically. - try std.testing.expectEqual(@as(u16, 400), res.status); - try std.testing.expectEqual(@as(i64, 0), try countRows(env, "posts")); - try std.testing.expectEqual(@as(i64, 0), try countRows(env, "audit")); + fn caseForMode(comptime mode: []const u8) Case { + inline for (cases) |case| { + if (comptime std.mem.eql(u8, case.mode, mode)) return case; + } + @compileError("Unknown rejection mode: " ++ mode); + } + + fn expectResponse(response: http.Response, case: Case) !void { + try std.testing.expectEqual(case.status, response.status); + const parsed = try std.json.parseFromSlice(std.json.Value, std.testing.allocator, response.body, .{}); + defer parsed.deinit(); + const envelope = parsed.value.object; + try std.testing.expectEqual(@as(i64, case.status), envelope.get("status").?.integer); + try std.testing.expectEqualStrings(case.code, envelope.get("code").?.string); + try std.testing.expectEqualStrings(case.message, envelope.get("message").?.string); + const data = envelope.get("data").?.object; + if (std.mem.eql(u8, case.mode, "invalid")) { + try std.testing.expectEqual(@as(usize, 1), data.count()); + const title = data.get("title").?.object; + try std.testing.expectEqualStrings("validation_required", title.get("code").?.string); + try std.testing.expectEqualStrings("Choose a title.", title.get("message").?.string); + } else try std.testing.expectEqual(@as(usize, 0), data.count()); + } +}; + +test "record before-hook response ownership is leak-checked without an arena" { + const a = std.testing.allocator; + const dispatch = events.Dispatch{ .record = RejectionHooks.fail }; + var app = app_mod.App{ .allocator = a, .io = std.testing.io, .pool = undefined, .dispatch = &dispatch }; + // This hook only rejects; it never touches the supplied connection. + var conn: db.Db = undefined; + const rctx = request.RequestContext{ .method = "POST" }; + inline for (.{ .before_create, .before_update, .before_delete }) |phase| { + for (RejectionHooks.cases) |case| { + var object: std.json.ObjectMap = .empty; + defer object.deinit(a); + try object.put(a, "title", .{ .string = case.mode }); + var value = std.json.Value{ .object = object }; + var ctx = http.RequestCtx{ .method = .POST, .path = "/", .allocator = RequestArena.forTest(a), .app = &app }; + const rejection = (try emitBeforeRecord(&ctx, &rctx, &conn, "posts", &value, phase)) orelse return error.ExpectedRejection; + const response = rejection.response; + defer a.free(response.body); + defer if (rejection.incident) |owned| a.free(owned.message); + const normalized = try @import("../server.zig").normalizeResponse(&ctx, response); + try RejectionHooks.expectResponse(normalized, case); + } + } +} + +test "before-hook server failures reach onError and the reporter, including allocation fallback" { + const reporter = @import("../report/reporter.zig"); + const Reports = struct { + var consumer_calls: usize = 0; + var reporter_calls: usize = 0; + var context_matches: bool = true; + var report_matches: bool = true; + var expected_err: anyerror = error.Handled; + var expected_message: []const u8 = ""; + fn reject(ctx: *Ctx, ev: *events.RecordEvent) anyerror!void { + return ctx.fail(if (std.mem.eql(u8, ev.collection, "client")) 409 else 503, "Hook rejection."); + } + fn onError(ev: *events.ErrorEvent) void { + consumer_calls += 1; + context_matches = context_matches and ev.phase == .before_hook and ev.err == expected_err and + std.mem.eql(u8, ev.message, expected_message) and + ev.ctx != null and std.mem.eql(u8, ev.ctx.?.method, "POST"); + } + fn report(_: *anyopaque, _: std.Io, _: std.mem.Allocator, value: reporter.Report) anyerror!void { + report_matches = report_matches and std.mem.eql(u8, "before_hook", value.phase) and + std.mem.eql(u8, @errorName(expected_err), value.err_name) and + std.mem.eql(u8, expected_message, value.message); + reporter_calls += 1; + } + const vt = reporter.Reporter.VTable{ .report = report }; + }; + Reports.consumer_calls = 0; + Reports.reporter_calls = 0; + Reports.context_matches = true; + Reports.report_matches = true; + var dummy: u8 = 0; + var report_interface = reporter.Reporter{ .ptr = &dummy, .vtable = &Reports.vt }; + const dispatch = events.Dispatch{ .record = Reports.reject, .on_error = Reports.onError }; + const a = std.testing.allocator; + var app = app_mod.App{ .allocator = a, .io = std.testing.io, .pool = undefined, .dispatch = &dispatch, .reporter = &report_interface }; + var conn: db.Db = undefined; + var value = std.json.Value{ .object = .empty }; + const rctx = request.RequestContext{ .method = "POST" }; + inline for (.{ .before_create, .before_update, .before_delete }) |phase| { + for ([_][]const u8{ "client", "server", "allocation_failure" }) |mode| { + var failing = std.testing.FailingAllocator.init(a, .{ .fail_index = 0 }); + const out_of_memory = std.mem.eql(u8, mode, "allocation_failure"); + var ctx = http.RequestCtx{ .method = .POST, .path = "/", .allocator = RequestArena.forTest(if (out_of_memory) failing.allocator() else a), .app = &app }; + const before_consumer = Reports.consumer_calls; + const before_reporter = Reports.reporter_calls; + Reports.expected_err = if (out_of_memory) error.OutOfMemory else error.Handled; + Reports.expected_message = if (out_of_memory) hook_incident_fallback_message else "server: 503 Hook rejection."; + const rejection = (try emitBeforeRecord(&ctx, &rctx, &conn, mode, &value, phase)).?; + const response = rejection.response; + var hook_error = HookErrorReport{ .app = &app }; + hook_error.capture(rejection, rctx); + hook_error.dispatch(); + // OOM uses Ctx's allocation-free static internal envelope and report text. + defer if (!out_of_memory) a.free(response.body); + defer if (!out_of_memory) if (rejection.incident) |owned| a.free(owned.message); + const incident: usize = @intFromBool(!std.mem.eql(u8, mode, "client")); + try std.testing.expectEqual(before_consumer + incident, Reports.consumer_calls); + try std.testing.expectEqual(before_reporter + incident, Reports.reporter_calls); + try std.testing.expectEqual(@as(u16, if (out_of_memory) 500 else if (incident == 1) 503 else 409), response.status); + } + } + try std.testing.expect(Reports.context_matches); + try std.testing.expect(Reports.report_matches); +} + +test "before-hook incident reports keep distinct failures apart inside the dedup window" { + const reporter = @import("../report/reporter.zig"); + const Dedup = @import("../report/dedup.zig").Dedup; + const Reports = struct { + const expected = [_][]const u8{ "posts: 503 payment provider down", "orders: 500 ledger mismatch" }; + var consumer_calls: usize = 0; + var reporter_calls: usize = 0; + var messages_match: bool = true; + fn reject(ctx: *Ctx, ev: *events.RecordEvent) anyerror!void { + if (std.mem.eql(u8, ev.collection, "posts")) return ctx.fail(503, "payment provider down"); + return ctx.fail(500, "ledger mismatch"); + } + fn onError(_: *events.ErrorEvent) void { + consumer_calls += 1; + } + fn report(_: *anyopaque, _: std.Io, _: std.mem.Allocator, value: reporter.Report) anyerror!void { + messages_match = messages_match and reporter_calls < expected.len and + std.mem.eql(u8, expected[reporter_calls], value.message); + reporter_calls += 1; + } + const vt = reporter.Reporter.VTable{ .report = report }; + }; + Reports.consumer_calls = 0; + Reports.reporter_calls = 0; + Reports.messages_match = true; + var dummy: u8 = 0; + var report_interface = reporter.Reporter{ .ptr = &dummy, .vtable = &Reports.vt }; + const dispatch = events.Dispatch{ .record = Reports.reject, .on_error = Reports.onError }; + const a = std.testing.allocator; + var dedup = Dedup.init(a, 60); + defer dedup.deinit(); + var app = app_mod.App{ .allocator = a, .io = std.testing.io, .pool = undefined, .dispatch = &dispatch, .reporter = &report_interface }; + app.report_dedup = &dedup; + var conn: db.Db = undefined; + var value = std.json.Value{ .object = .empty }; + const rctx = request.RequestContext{ .method = "POST" }; + // The repeated posts failure is the one the window should suppress. + for ([_][]const u8{ "posts", "orders", "posts" }) |collection| { + var ctx = http.RequestCtx{ .method = .POST, .path = "/", .allocator = RequestArena.forTest(a), .app = &app }; + const rejection = (try emitBeforeRecord(&ctx, &rctx, &conn, collection, &value, .before_create)).?; + defer a.free(rejection.response.body); + defer if (rejection.incident) |owned| a.free(owned.message); + var hook_error = HookErrorReport{ .app = &app }; + hook_error.capture(rejection, rctx); + hook_error.dispatch(); + } + try std.testing.expectEqual(@as(usize, 3), Reports.consumer_calls); + try std.testing.expectEqual(@as(usize, 2), Reports.reporter_calls); + try std.testing.expect(Reports.messages_match); +} + +test "before-hook onError can write after the rejected transaction releases its writer" { + const report_log = @import("../report/log.zig"); + const Observer = struct { + var calls: usize = 0; + var reports: usize = 0; + var writer_available: bool = true; + var transaction_closed: bool = true; + var wrote: bool = true; + fn onError(ev: *events.ErrorEvent) void { + calls += 1; + const pool = ev.app.pool; + // Inspect before acquiring: a regression must fail, never deadlock the suite. + const available = if (comptime @import("build_options").postgres) + switch (pool.impl) { + inline else => |*backend| backend.writer_mutex.state.load(.acquire) == .unlocked, + } + else + pool.writer_mutex.state.load(.acquire) == .unlocked; + writer_available = writer_available and available; + if (!available) return; + const w = pool.acquireWriter(); + defer pool.releaseWriter(); + transaction_closed = transaction_closed and !w.inTransaction(); + w.exec("INSERT INTO audit(id,created,updated,title) VALUES ('operator','t','t','reported');") catch { + wrote = false; + }; + } + fn sink(_: []const u8) void { + reports += 1; + } + }; + report_log.log_sink = Observer.sink; + defer report_log.log_sink = null; + inline for (.{ .before_create, .before_update, .before_delete }) |phase| { + Observer.calls = 0; + Observer.reports = 0; + Observer.writer_available = true; + Observer.transaction_closed = true; + Observer.wrote = true; + var env = try TestEnv.init(); + defer env.deinit(); + try seedRuled(env, "audit", "@public", "@public", "@public"); + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const a = arena.allocator(); + const col_params = [_]http.Param{.{ .key = "col", .value = "posts" }}; + const body = "{\"title\":\"503\"}"; + var rid: []const u8 = ""; + if (phase != .before_create) { + var ctx = ctxFor(env, RequestArena.from(&arena), .POST, body, &col_params); + const created = try create(&ctx); + try std.testing.expectEqual(@as(u16, 201), created.status); + rid = (try std.json.parseFromSlice(std.json.Value, a, created.body, .{})).value.object.get("id").?.string; + } + const dispatch = events.Dispatch{ .record = RejectionHooks.reject, .on_error = Observer.onError }; + env.app.dispatch = &dispatch; + const row_params = [_]http.Param{ .{ .key = "col", .value = "posts" }, .{ .key = "id", .value = rid } }; + var ctx = ctxFor(env, RequestArena.from(&arena), .POST, body, if (phase == .before_create) &col_params else &row_params); + const response = try switch (phase) { + .before_create => create(&ctx), + .before_update => update(&ctx), + .before_delete => delete(&ctx), + else => unreachable, + }; + try std.testing.expectEqual(@as(u16, 503), response.status); + try std.testing.expectEqual(@as(usize, 1), Observer.calls); + try std.testing.expectEqual(@as(usize, 1), Observer.reports); + try std.testing.expect(Observer.writer_available); + try std.testing.expect(Observer.transaction_closed); + try std.testing.expect(Observer.wrote); + // The rejected hook's audit side-write vanished; only onError's write remains. + try std.testing.expectEqual(@as(i64, 1), try countRows(env, "audit")); + try std.testing.expectEqual(@as(i64, if (phase == .before_create) 0 else 1), try countRows(env, "posts")); + } +} + +test "record dispatcher discards a recovered wildcard failure before the next handler" { + const H = struct { + fn recover(ctx: *Ctx, _: *events.RecordEvent) anyerror!void { + const result: anyerror!void = ctx.fail(409, "Recovered conflict."); + result catch |err| { + try std.testing.expectEqual(error.Handled, err); + }; + } + fn reject(_: *Ctx, _: *events.RecordEvent) anyerror!void { + return error.Handled; + } + }; + const a = std.testing.allocator; + const dispatch = events.Dispatch{ .record = events.buildRecordDispatcher(.{ + .any = .{ .beforeCreate = H.recover }, + .posts = .{ .beforeCreate = H.reject }, + }) }; + var app = app_mod.App{ .allocator = a, .io = std.testing.io, .pool = undefined, .dispatch = &dispatch }; + var conn: db.Db = undefined; + var value = std.json.Value{ .object = .empty }; + var ctx = http.RequestCtx{ .method = .POST, .path = "/", .allocator = RequestArena.forTest(a), .app = &app }; + const rejection = (try emitBeforeRecord(&ctx, &.{}, &conn, "posts", &value, .before_create)).?; + defer a.free(rejection.response.body); + try RejectionHooks.expectResponse(rejection.response, .{ .mode = "unhandled" }); +} + +test "before-hook side-write rolls back with the triggering write on hook error" { + inline for (.{ .before_create, .before_update, .before_delete }) |phase| { + for ([_]RejectionHooks.Case{ RejectionHooks.caseForMode("409"), RejectionHooks.caseForMode("raw"), RejectionHooks.caseForMode("invalid") }) |case| { + var env = try TestEnv.init(); + defer env.deinit(); + try seedRuled(env, "audit", "@public", "@public", "@public"); + var arena = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena.deinit(); + const a = arena.allocator(); + const body = try std.fmt.allocPrint(a, "{{\"title\":\"{s}\"}}", .{case.mode}); + const col_param = [_]http.Param{.{ .key = "col", .value = "posts" }}; + var rid: []const u8 = ""; + if (phase != .before_create) { + var seed = ctxFor(env, RequestArena.from(&arena), .POST, body, &col_param); + const res = try create(&seed); + try std.testing.expectEqual(@as(u16, 201), res.status); + rid = (try std.json.parseFromSlice(std.json.Value, a, res.body, .{})).value.object.get("id").?.string; + } + const dispatch = events.Dispatch{ .record = events.buildRecordDispatcher(.{ + .posts = .{ .beforeCreate = RejectionHooks.reject, .beforeUpdate = RejectionHooks.reject, .beforeDelete = RejectionHooks.reject }, + }) }; + env.app.dispatch = &dispatch; + const row_params = [_]http.Param{ .{ .key = "col", .value = "posts" }, .{ .key = "id", .value = rid } }; + var ctx = ctxFor(env, RequestArena.from(&arena), switch (phase) { + .before_create => .POST, + .before_update => .PATCH, + .before_delete => .DELETE, + else => unreachable, + }, body, if (phase == .before_create) &col_param else &row_params); + // Updates use the rejection mode in their request, while the stored + // title differs so a mistakenly committed update is observable. + if (phase == .before_update) { + const w = env.pool.acquireWriter(); + defer env.pool.releaseWriter(); + try w.exec("UPDATE posts SET title = 'original';"); + } + const response = try switch (phase) { + .before_create => create(&ctx), + .before_update => update(&ctx), + .before_delete => delete(&ctx), + else => unreachable, + }; + const normalized = try @import("../server.zig").normalizeResponse(&ctx, response); + try RejectionHooks.expectResponse(normalized, case); + try std.testing.expectEqual(@as(i64, 0), try countRows(env, "audit")); + try std.testing.expectEqual(@as(i64, if (phase == .before_create) 0 else 1), try countRows(env, "posts")); + if (phase == .before_update) { + var view_ctx = ctxFor(env, RequestArena.from(&arena), .GET, "", &row_params); + const persisted = (try std.json.parseFromSlice(std.json.Value, a, (try view(&view_ctx)).body, .{})).value.object; + try std.testing.expectEqualStrings("original", persisted.get("title").?.string); + } + } + } } // ---- Tenancy: update pre-authorization (reviewer MINOR 1, #156) ------------- diff --git a/src/ctx.zig b/src/ctx.zig index abf43100..6fffb662 100644 --- a/src/ctx.zig +++ b/src/ctx.zig @@ -59,6 +59,8 @@ pub const Ctx = struct { reader: ?events.ReaderData = null, /// Stashed error from ctx.fail/ctx.invalid; consumed by errorResponse(error.Handled). handled: ?error_mod.ApiError = null, + handled_bytes: ?[]u8 = null, + handled_fields: ?[]error_mod.FieldError = null, /// The raw HTTP request context. Non-null for route handlers; null for job/hook contexts /// where there is no HTTP request. Required by `issueSession`. request: ?*http_mod.RequestCtx = null, @@ -143,6 +145,7 @@ pub const Ctx = struct { /// Release any cached reader. Call once (the accessor/handler frame owns this via defer). pub fn deinit(self: *Ctx) void { + self.clearHandled(); if (self.reader) |*r| { r.deinit(); self.reader = null; @@ -670,17 +673,68 @@ pub const Ctx = struct { /// Sentinel error type returned by fail/invalid so callers can propagate. pub const FailError = error{Handled}; - /// Stash a custom status+message error and return error.Handled for propagation. - /// `code` is derived from `status` (`error_codes.forStatus`) since this call carries no - /// code of its own — the same derivation `route_types.zig`'s typed-route path uses. + /// Discard a prior failure, including one a handler caught and recovered from. + pub fn clearHandled(self: *Ctx) void { + if (self.handled_bytes) |bytes| self.arena.a.free(bytes); + if (self.handled_fields) |fields| self.arena.a.free(fields); + self.handled_bytes = null; + self.handled_fields = null; + self.handled = null; + } + + // Own every borrowed string before the caller's stack frame returns. Two + // allocations hold the strings and field descriptors; deinit/clearHandled + // release them even with a general-purpose allocator. + fn copyHandled(self: *Ctx, ae: error_mod.ApiError) !void { + var size = try std.math.add(usize, ae.message.len, ae.code.len); + for (ae.fields) |field| { + size = try std.math.add(usize, size, field.field.len); + size = try std.math.add(usize, size, field.code.len); + size = try std.math.add(usize, size, field.message.len); + } + const bytes = try self.arena.a.alloc(u8, size); + errdefer self.arena.a.free(bytes); + const fields = try self.arena.a.alloc(error_mod.FieldError, ae.fields.len); + errdefer self.arena.a.free(fields); + var remaining = bytes; + var owned = ae; + owned.message = copyString(&remaining, ae.message); + owned.code = copyString(&remaining, ae.code); + for (ae.fields, fields) |source, *dest| dest.* = .{ + .field = copyString(&remaining, source.field), + .code = copyString(&remaining, source.code), + .message = copyString(&remaining, source.message), + }; + owned.fields = fields; + self.clearHandled(); + self.handled_bytes = bytes; + self.handled_fields = fields; + self.handled = owned; + } + + fn copyString(remaining: *[]u8, source: []const u8) []const u8 { + const dest = remaining.*[0..source.len]; + @memcpy(dest, source); + remaining.* = remaining.*[source.len..]; + return dest; + } + + /// Copy a custom error now, so message may be a hook-local stack buffer. + /// Allocation failure preserves the sentinel API and produces a generic 500. pub fn fail(self: *Ctx, status: u16, message: []const u8) FailError { - self.handled = .{ .status = status, .code = error_codes.s(error_codes.forStatus(status)), .message = message }; + self.copyHandled(.{ .status = status, .code = error_codes.s(error_codes.forStatus(status)), .message = message }) catch { + self.clearHandled(); + self.handled = error_mod.ApiError.internal(); + }; return error.Handled; } - /// Stash a validation error (400 with field details) and return error.Handled. + /// Copy all validation descriptors and strings before returning error.Handled. pub fn invalid(self: *Ctx, fields: []const error_mod.FieldError) FailError { - self.handled = error_mod.ApiError.validation(fields); + self.copyHandled(error_mod.ApiError.validation(fields)) catch { + self.clearHandled(); + self.handled = error_mod.ApiError.internal(); + }; return error.Handled; } @@ -706,6 +760,13 @@ pub const Ctx = struct { return ae.toResponse(self.arena.a) catch static_internal_response; } + /// True when a rendered failure degraded to the generic 500 because copying the + /// failure (`fail`/`invalid`) or rendering its envelope ran out of memory. + pub fn usedAllocationFallback(self: *const Ctx, response: http_mod.Response) bool { + return response.body.ptr == static_internal_response.body.ptr or + (self.handled != null and self.handled_bytes == null); + } + // ----------------------------------------------------------------------- // Error reporting (#244). SELF-CONTAINED section so sibling PRs that also // append to ctx.zig rebase cleanly. @@ -3077,3 +3138,48 @@ test "job ctx relation queries remain trusted while client opt-in and row scopes const data = try job.records().dataRead(); try std.testing.expectError(error.RestrictedRelation, data.list("sources", .{ .client_query = true, .sort = "author.name" })); } + +// Mutating all source buffers after fail/invalid proves the payload is owned +// before the hook returns, independently of stack layout or optimizer behavior. +test "handled failures own message and every validation field slice" { + const a = std.testing.allocator; + var ctx = Ctx{ .app = undefined, .arena = RequestArena.forTest(a) }; + defer ctx.deinit(); + var message = "stack message".*; + try std.testing.expectEqual(error.Handled, ctx.fail(503, &message)); + @memset(&message, 'x'); + try std.testing.expectEqual(error.Handled, ctx.fail(503, ctx.handled.?.message)); + const failure = ctx.errorResponse(error.Handled); + defer a.free(failure.body); + try std.testing.expectEqual(@as(u16, 503), failure.status); + try std.testing.expect(std.mem.indexOf(u8, failure.body, "stack message") != null); + var name = "title".*; + var code = "validation_required".*; + var detail = "Choose a title.".*; + var fields = [_]error_mod.FieldError{.{ .field = &name, .code = &code, .message = &detail }}; + try std.testing.expectEqual(error.Handled, ctx.invalid(&fields)); + @memset(&name, 'x'); + @memset(&code, 'x'); + @memset(&detail, 'x'); + fields[0] = .{ .field = "wrong", .code = "wrong", .message = "wrong" }; + try std.testing.expectEqual(error.Handled, ctx.invalid(ctx.handled.?.fields)); + const response = ctx.errorResponse(error.Handled); + defer a.free(response.body); + ctx.deinit(); + try std.testing.expectEqualStrings("{\"status\":400,\"code\":\"validation_failed\",\"message\":\"Failed to validate the request.\",\"data\":{\"title\":{\"code\":\"validation_required\",\"message\":\"Choose a title.\"}}}", response.body); +} + +test "handled payload allocation failures clean up and use an internal error" { + for (0..4) |failure_index| { + var failing = std.testing.FailingAllocator.init(std.testing.allocator, .{ .fail_index = failure_index }); + var ctx = Ctx{ .app = undefined, .arena = RequestArena.forTest(failing.allocator()) }; + defer ctx.deinit(); + if (failure_index >= 2) { + try std.testing.expectEqual(error.Handled, ctx.invalid(&.{.{ .field = "prior", .code = "validation_required", .message = "Prior failure." }})); + try std.testing.expectEqual(error.Handled, ctx.invalid(ctx.handled.?.fields)); + } else try std.testing.expectEqual(error.Handled, ctx.invalid(&.{.{ .field = "title", .code = "validation_required", .message = "Choose a title." }})); + try std.testing.expectEqual(@as(u16, 500), ctx.handled.?.status); + try std.testing.expectEqual(@as(?[]u8, null), ctx.handled_bytes); + try std.testing.expectEqual(@as(?[]error_mod.FieldError, null), ctx.handled_fields); + } +} diff --git a/src/events.zig b/src/events.zig index 049d019e..58a07bba 100644 --- a/src/events.zig +++ b/src/events.zig @@ -1030,6 +1030,7 @@ pub fn buildRecordDispatcher(comptime hooks: anytype) RecordHandler { inline else => |p| { const fname = comptime phaseFieldName(p); if (@hasField(@TypeOf(g), fname)) { + ctx.clearHandled(); try @field(g, fname)(ctx, ev); } },