Replace inline validation with zod middleware for admin and indexer r…#35
Replace inline validation with zod middleware for admin and indexer r…#35anonfedora wants to merge 4 commits into
Conversation
…outes - Create zod schemas for webhook creation, reindex query params, and quarantine reprocess bodies - Apply validateBody and validateQuery middleware to /admin/reindex, /admin/quarantine-events/reprocess, /admin/webhooks, and /indexer/webhooks routes - Remove inline validation checks from createWebhookSubscription, reindexLedgerRange, and reprocessQuarantinedEvents controllers - Add comprehensive validation tests for webhook creation, reindex, and quarantine reprocess endpoints - Ensure error responses match standardized zod error format from errorHandler Closes LabsCrypt#17
ogazboiz
left a comment
There was a problem hiding this comment.
nice clean refactor, the inline checks are fully moved into reusable zod schemas with no dead inline checks left in the handlers, and two of the three schemas have parity or tighter validation (the reindex range query handles string params via z.coerce, and the reprocess-events schema enforces int/positive ids + the limit cap). build passes. two things to fix:
- the callbackUrl schema 500s on an invalid url instead of 400. indexerSchemas.ts:11-17, the .refine() calls new URL(url) in its predicate, and in zod 4 the refine still runs even after .url() already failed, so for a non-url string new URL() throws a raw TypeError (not a ZodError), which the global handler turns into a 500. this fails your own webhookValidation.test.ts:43 and :147. make the protocol check exception-safe:
.refine((url) => { try { const p = new URL(url).protocol; return p === "http:" || p === "https:"; } catch { return false; } }, { message: "callbackUrl must use http or https" }) - prettier fails on src/schemas/indexerSchemas.ts, run
npx prettier --writeon it.
non-blocking: indexerController.ts:691 still does Math.min(limit, 500) but the schema already caps limit at 500, so that's dead now, could simplify to limit ?? 50. fix 1 and 2 and the suite + prettier go green and this is good.
if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0
ogazboiz
left a comment
There was a problem hiding this comment.
the route wiring is correct, schemas are applied to the right endpoints with the right middleware (validateQuery for the reindex range, validateBody for reprocess + webhook on both admin and indexer routes), the inline checks are fully removed with no silent weakening, and indexerSchemas is exported. but both items from the last round are still open:
-
src/schemas/indexerSchemas.ts:11-17, the callbackUrl .refine() still calls new URL(url).protocol without a try/catch, so a non-URL string throws a raw TypeError that the global handler turns into a 500 instead of 400. this fails webhookValidation.test.ts:43 and :147 (expected 400, got 500). make it exception-safe:
.refine((url) => { try { const p = new URL(url).protocol; return p === "http:" || p === "https:"; } catch { return false; } }, { message: "callbackUrl must use http or https" }) -
prettier still fails on src/schemas/indexerSchemas.ts. run: npx prettier --write src/schemas/indexerSchemas.ts
minor (not blocking): indexerController does limit ? Math.min(limit, 500) : 50 but the schema already caps at 500, so the Math.min is redundant, could just be limit ?? 50.
fix those two and it's good to go.
if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0
- Make callbackUrl refine exception-safe to prevent TypeError for non-URL strings - Run prettier on indexerSchemas.ts - Remove redundant Math.min(limit, 500) since schema already caps at 500
Both comments resolved... |
|
Hello, I'm waiting for your review. |
ogazboiz
left a comment
There was a problem hiding this comment.
both items from last round are fixed, merging once ci is green. the callbackUrl .refine() now wraps new URL() in try/catch so an invalid url returns 400 not 500 (webhookValidation.test.ts:43 and :147 are green), prettier passes, and you even dropped the redundant Math.min(limit, 500). the route wiring was already correct. nice.
if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0
ps: CI is red right now on the migrate step, but that is a pre-existing main-branch issue (a commonjs migration file under our ESM package), nothing to do with your change. once that is sorted this lands.
Convert migration from CommonJS to ES module syntax to match project's type: module configuration
|
Changes pushed |
Title: Replace inline validation with zod middleware for admin and indexer routes
Description:
This PR replaces ad-hoc inline validation with standardized zod middleware validation for webhook creation, reindex, and quarantine reprocess endpoints.
Changes:
Created src/schemas/indexerSchemas.ts with zod schemas for:
createWebhookSubscriptionSchema- validates callbackUrl (URL format, http/https protocol), eventTypes (array of valid event types), and optional secretreindexLedgerRangeQuerySchema- validates fromLedger and toLedger as positive integers with range validationreprocessQuarantinedEventsSchema- validates optional ids array (positive integers) and optional limit (positive integer, max 500)Applied validation middleware to routes:
/admin/reindex- added validateQuery(reindexLedgerRangeQuerySchema)/admin/quarantine-events/reprocess- added validateBody(reprocessQuarantinedEventsSchema)/admin/webhooks- added validateBody(createWebhookSubscriptionSchema)/indexer/webhooks- added validateBody(createWebhookSubscriptionSchema)Removed inline validation from controllers:
Added comprehensive tests:
Benefits:
Closes #17