refactor(backend): make request validation schema-driven - #478
Open
solaawojobi00-bit wants to merge 2 commits into
Open
Conversation
Validation was spread across addressValidator.js and inline checks in every route handler, so the same rule was written several different ways and there was no single place to see what a route accepts. Every route now declares its request as a zod schema in src/schemas/. One middleware (src/middleware/validate.js) enforces those schemas before the handler runs; handlers read coerced values from req.valid and do no checking of their own. addressValidator.js is gone, folded into schemas/common.js. Validation failures share one response shape. `error` and `code` keep the values the API already returned, so clients see no break, and a new `details` array lists every problem found rather than only the first. The OpenAPI 3.1 document is generated from the same schemas via zod's toJSONSchema, served at GET /api/openapi.json and emitted by `npm run openapi`. It cannot drift from what the server enforces. Rules that were inconsistent between endpoints are now uniform, which changes some behaviour: - `:id` params are strict positive integers everywhere. Previously /services/:id used parseInt, so "7abc" was read as 7, while /services/:id/deactivate rejected it. - Non-numeric query params are rejected instead of silently defaulting. ?lat=abc was a 400 only by accident before; ?lat=0 became 40.7128 because the old code used `parseFloat(x) || default`. - POST /agents/:address/payment validates its body before the idempotency replay lookup rather than after. - amountUsdc must parse as a number; it used to reach BigInt(NaN) and surface as a 500 instead of a 400. parseActivityPagination is removed: the activity and payment-history schemas own those rules now, and keeping it would restore the duplication this change exists to remove. Its cases moved to src/schemas/common.test.js. src/schemas/routes.test.js walks the Express routers and fails if a route is registered without a declared schema, if a declared schema has no route, or if a route accepting input is missing the validation middleware. Closes Stellar-Ecosystem#346 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gy7MYRindtXZLNU4fE4kpQ
|
@solaawojobi00-bit Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
📝 WalkthroughWalkthroughThe backend adds Zod-based request schemas, centralized validation middleware, standardized 400 responses, route integrations, schema coverage tests, and OpenAPI generation exposed through the API and a CLI command. ChangesRequest validation and OpenAPI
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Express
participant openapiRouter
participant buildOpenApiDocument
participant schemas
Express->>openapiRouter: dispatch GET /api/openapi.json
openapiRouter->>buildOpenApiDocument: build document at startup
buildOpenApiDocument->>schemas: read route descriptors and Zod schemas
schemas-->>buildOpenApiDocument: route metadata and schemas
buildOpenApiDocument-->>openapiRouter: OpenAPI document
openapiRouter-->>Client: JSON specification
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
Please review and Merge |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/src/schemas/services.js (1)
10-14: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
demoRunIdhas no length limit.It's optional but otherwise unconstrained, and is echoed into the activity feed per its own description. Consider adding a
.max(...)to bound it, consistent with other length-constrained fields in this schema set (e.g.requiredString).♻️ Suggested bound
const demoRunId = z .string() + .max(100, { error: "`demoRunId` is too long" }) .optional() .describe("Opaque ID echoed into the activity feed");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/schemas/services.js` around lines 10 - 14, Update the demoRunId schema definition to enforce a maximum string length, using the same established bound as other constrained fields such as requiredString. Preserve its optional behavior and activity-feed description.backend/src/schemas/registry.js (2)
120-124: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
endpointonly checks the prefix, not URL validity.
startsWith("https://")doesn't guarantee a parseable URL —"https://"alone, or a string with embedded whitespace/control characters, would pass. Consider layeringz.url()(or anew URL()refinement) on top of the prefix check so malformed endpoints are rejected here instead of failing later when the service is invoked.♻️ Suggested tightening
endpoint: z .string({ error: "`endpoint` must start with https://" }) .trim() .startsWith("https://", { error: "`endpoint` must start with https://" }) + .refine((v) => { try { new URL(v); return true; } catch { return false; } }, { + error: "`endpoint` must be a valid URL", + }) .describe("Public HTTPS URL the service is served from"),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/schemas/registry.js` around lines 120 - 124, Update the endpoint schema to validate both the required https:// prefix and full URL parseability, using z.url() or an equivalent URL refinement after trimming. Ensure bare or malformed URLs, including embedded whitespace/control characters, are rejected while preserving the existing public HTTPS URL description and error behavior.
50-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
pagehas no upper bound, unlike other paginated fields.
limit/offsetelsewhere in this PR (e.g.getActivityinservices.js) are clamped viamax.pagehere is unbounded, so an arbitrarily large value flows straight intolistServices({ page, pageSize: PAGE_SIZE }).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/schemas/registry.js` around lines 50 - 52, Update the page query parameter definition in the registry schema to enforce an upper bound using the same max-clamping pattern as other paginated fields. Preserve the existing zero-based default and description while ensuring large page values cannot flow unbounded into listServices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/schemas/common.js`:
- Around line 123-131: Update stroopsField to validate string values as
well-formed positive integer amounts before they reach BigInt conversion,
rejecting non-numeric strings and negative values while preserving the existing
positive-number validation and error message. Mirror the validation approach
used by positiveIntegerField rather than only checking string length.
---
Nitpick comments:
In `@backend/src/schemas/registry.js`:
- Around line 120-124: Update the endpoint schema to validate both the required
https:// prefix and full URL parseability, using z.url() or an equivalent URL
refinement after trimming. Ensure bare or malformed URLs, including embedded
whitespace/control characters, are rejected while preserving the existing public
HTTPS URL description and error behavior.
- Around line 50-52: Update the page query parameter definition in the registry
schema to enforce an upper bound using the same max-clamping pattern as other
paginated fields. Preserve the existing zero-based default and description while
ensuring large page values cannot flow unbounded into listServices.
In `@backend/src/schemas/services.js`:
- Around line 10-14: Update the demoRunId schema definition to enforce a maximum
string length, using the same established bound as other constrained fields such
as requiredString. Preserve its optional behavior and activity-feed description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 32477e57-6525-4ab8-937e-48e95e74b4f5
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
README.mdbackend/package.jsonbackend/scripts/generate-openapi.jsbackend/src/index.jsbackend/src/lib/activityFeed.jsbackend/src/lib/openapi.jsbackend/src/lib/openapi.test.jsbackend/src/middleware/addressValidator.jsbackend/src/middleware/validate.jsbackend/src/middleware/validate.test.jsbackend/src/routes/agents.jsbackend/src/routes/agents.test.jsbackend/src/routes/demo.jsbackend/src/routes/openapi.jsbackend/src/routes/registry.jsbackend/src/routes/services.jsbackend/src/routes/services.test.jsbackend/src/schemas/agents.jsbackend/src/schemas/common.jsbackend/src/schemas/common.test.jsbackend/src/schemas/demo.jsbackend/src/schemas/index.jsbackend/src/schemas/registry.jsbackend/src/schemas/routes.test.jsbackend/src/schemas/services.jsbackend/test/activity.test.jsbackend/test/agents.test.js
💤 Files with no reviewable changes (3)
- backend/src/middleware/addressValidator.js
- backend/src/routes/services.test.js
- backend/src/routes/agents.test.js
Comment on lines
+123
to
+131
| export function stroopsField(field) { | ||
| const message = `\`${field}\` is required (string or number)`; | ||
| return z | ||
| .union([z.number(), z.string()], { error: message }) | ||
| .refine((value) => (typeof value === "number" ? value > 0 : value.length > 0), { | ||
| error: message, | ||
| }) | ||
| .describe("Amount in stroops (1 USDC = 10,000,000 stroops)"); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
stroopsField doesn't actually validate the string case.
.refine((value) => (typeof value === "number" ? value > 0 : value.length > 0), ...) only checks that a string is non-empty — not that it's a well-formed, non-negative integer. "abc" or "-100" both pass this schema. maxPerTxStroops/maxPerDayStroops flow from here straight into contract.js's BigInt() conversion (per the comment on line 121), so:
- a non-numeric string throws a
SyntaxErroratBigInt(), producing an unhandled-style 500 instead of the clean 400 the rest of this file is designed to guarantee (see theusdcAmountFielddocstring on the exact same failure mode, lines 96-99). - a negative numeric string like
"-100"parses fine and would set a negative spending limit on-chain.
🛡️ Proposed fix (mirrors `positiveIntegerField`)
export function stroopsField(field) {
const message = `\`${field}\` is required (string or number)`;
return z
.union([z.number(), z.string()], { error: message })
- .refine((value) => (typeof value === "number" ? value > 0 : value.length > 0), {
- error: message,
- })
+ .transform((value, ctx) => {
+ const wellFormed =
+ typeof value === "number" ? Number.isSafeInteger(value) && value > 0
+ : POSITIVE_INT_REGEX.test(value);
+ if (!wellFormed) {
+ ctx.addIssue({ code: "custom", message });
+ return z.NEVER;
+ }
+ return value; // keep original number/string — contract.js owns the BigInt conversion
+ })
.describe("Amount in stroops (1 USDC = 10,000,000 stroops)");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function stroopsField(field) { | |
| const message = `\`${field}\` is required (string or number)`; | |
| return z | |
| .union([z.number(), z.string()], { error: message }) | |
| .refine((value) => (typeof value === "number" ? value > 0 : value.length > 0), { | |
| error: message, | |
| }) | |
| .describe("Amount in stroops (1 USDC = 10,000,000 stroops)"); | |
| } | |
| export function stroopsField(field) { | |
| const message = `\`${field}\` is required (string or number)`; | |
| return z | |
| .union([z.number(), z.string()], { error: message }) | |
| .transform((value, ctx) => { | |
| const wellFormed = | |
| typeof value === "number" ? Number.isSafeInteger(value) && value > 0 | |
| : POSITIVE_INT_REGEX.test(value); | |
| if (!wellFormed) { | |
| ctx.addIssue({ code: "custom", message }); | |
| return z.NEVER; | |
| } | |
| return value; // keep original number/string — contract.js owns the BigInt conversion | |
| }) | |
| .describe("Amount in stroops (1 USDC = 10,000,000 stroops)"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/schemas/common.js` around lines 123 - 131, Update stroopsField to
validate string values as well-formed positive integer amounts before they reach
BigInt conversion, rejecting non-numeric strings and negative values while
preserving the existing positive-number validation and error message. Mirror the
validation approach used by positiveIntegerField rather than only checking
string length.
Collaborator
|
This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
Resolved conflicts across 6 files where upstream's hand-rolled request parsing collided with this branch's schema-driven validation (Stellar-Ecosystem#478): - backend/package.json: kept both branches' additions (openapi/bench scripts, zod/pino-pretty deps) - backend/package-lock.json: regenerated via npm install - backend/src/routes/agents.js: kept schema-validated req.valid.* over upstream's manual parseInt/parseFloat parsing - backend/src/routes/registry.js: kept schema validation for GET /services, restored the annotateTtlWarning helper (missing on this branch, needed after adopting upstream's offset/limit contract.js pagination interface), dropped now-redundant hand-rolled helpers - backend/src/schemas/registry.js: switched listServices to offset/limit to match the current contract.js interface; raised name/description max length to 64/256 to match a business-rule change made independently upstream - backend/src/routes/services.js: kept schema validation for GET /search, restored upstream's query sanitization (control-char stripping, 256-char cap) and the usdcToStroops import upstream's merged code still needs Also fixes two tests left asserting pre-refactor behavior, surfaced by the merge: - backend/src/lib/openapi.test.js: expected name maxLength 50, now 64 - backend/test/demo.test.js: asserted the old single-string error message; now checks code and the per-field details array the validate() middleware actually returns
Author
|
Hi @ritik4ever, the merge conflicts are resolved and pushed (f02e082) — this branch now shows as cleanly mergeable against main. Ready for review whenever you get a chance. Thanks! |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
5-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the readiness claim with
Project Status and Roadmap.Line 5 calls Lodestar a “production-grade open source project”. Line 13 states that it is an early-stage demo and is not production-grade. Replace the opening claim with “demo-ready”, or update the maturity section if production readiness is intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 5, Update the opening README description around the Lodestar project summary to replace the conflicting “production-grade” readiness claim with “demo-ready,” while preserving the rest of the feature description and keeping it consistent with the Project Status and Roadmap maturity statement.backend/src/routes/demo.js (1)
84-89: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep the disconnect listener active for the full cancellable operation.
Register the listener before
getService()awaits, and remove it infinally. Also pass the abort signal intowaitForActivityTxHash()and cancel its sleep loop, because the route currently exitsfetchWithTx()at header receipt and then continues withresponse.json()and polling after a client close.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/routes/demo.js` around lines 84 - 89, Update the route’s request-disconnect handling around getService(), fetchWithTx(), response.json(), and waitForActivityTxHash(): register the close listener before any await, pass the same abort signal to waitForActivityTxHash() and make its sleep loop cancellable, then remove the listener in a finally block so cancellation remains active through the entire operation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/src/routes/demo.js`:
- Around line 84-89: Update the route’s request-disconnect handling around
getService(), fetchWithTx(), response.json(), and waitForActivityTxHash():
register the close listener before any await, pass the same abort signal to
waitForActivityTxHash() and make its sleep loop cancellable, then remove the
listener in a finally block so cancellation remains active through the entire
operation.
In `@README.md`:
- Line 5: Update the opening README description around the Lodestar project
summary to replace the conflicting “production-grade” readiness claim with
“demo-ready,” while preserving the rest of the feature description and keeping
it consistent with the Project Status and Roadmap maturity statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9718a1a2-979e-462a-a323-b73f20692d02
⛔ Files ignored due to path filters (1)
backend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
README.mdbackend/package.jsonbackend/src/index.jsbackend/src/lib/openapi.test.jsbackend/src/routes/agents.jsbackend/src/routes/demo.jsbackend/src/routes/registry.jsbackend/src/routes/services.jsbackend/src/routes/services.test.jsbackend/src/schemas/registry.jsbackend/test/activity.test.jsbackend/test/demo.test.js
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/src/index.js
- backend/package.json
- backend/test/activity.test.js
- backend/src/routes/services.test.js
- backend/src/lib/openapi.test.js
- backend/src/schemas/registry.js
- backend/src/routes/services.js
- backend/src/routes/agents.js
Author
|
Also flagging: while resolving conflicts, I found two pre-existing issues in main unrelated to this PR: rate-limit-redis and pino-http are imported in src/middleware/rateLimiter.js and requestContext.js but never declared in package.json, so both test files fail to load. Left both untouched since they predate this PR. Happy to open a separate issue if useful. |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Validation was spread across addressValidator.js and inline checks in every route handler, so the same rule was written several different ways and there was no single place to see what a route accepts.
Every route now declares its request as a zod schema in src/schemas/. One middleware (src/middleware/validate.js) enforces those schemas before the handler runs; handlers read coerced values from req.valid and do no checking of their own. addressValidator.js is gone, folded into schemas/common.js.
Validation failures share one response shape.
errorandcodekeep the values the API already returned, so clients see no break, and a newdetailsarray lists every problem found rather than only the first.The OpenAPI 3.1 document is generated from the same schemas via zod's toJSONSchema, served at GET /api/openapi.json and emitted by
npm run openapi. It cannot drift from what the server enforces.Rules that were inconsistent between endpoints are now uniform, which changes some behaviour:
:idparams are strict positive integers everywhere. Previously /services/:id used parseInt, so "7abc" was read as 7, while /services/:id/deactivate rejected it.parseFloat(x) || default.parseActivityPagination is removed: the activity and payment-history schemas own those rules now, and keeping it would restore the duplication this change exists to remove. Its cases moved to src/schemas/common.test.js.
src/schemas/routes.test.js walks the Express routers and fails if a route is registered without a declared schema, if a declared schema has no route, or if a route accepting input is missing the validation middleware.
Closes #346
Claude-Session: https://claude.ai/code/session_01Gy7MYRindtXZLNU4fE4kpQ
Summary by CodeRabbit
New Features
/api/openapi.json.Documentation
Tests