-
Notifications
You must be signed in to change notification settings - Fork 33
Add form handling and CSRF protection middleware #52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
b79f78d
c2feb3a
fe29a78
2edb0fc
ba5de09
0e4d62d
0e5bbc3
cac5ee9
81e7800
c486a5d
21ea9c2
66b80e6
53f73f5
076399a
33080d5
cb12d1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,76 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Form handling middleware factory with TypeBox validation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { TypeCompiler } from "@sinclair/typebox/compiler"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const log = (response, data) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const logger = response.locals?.log || console.log; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger(data); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const formatErrors = (errors) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return [...errors].map((err) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const path = err.path.slice(1) || "root"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (err.message.includes("Required")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return `Missing required field: ${path}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (err.message.includes("Unexpected property")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return `Undeclared field not allowed: ${path}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (err.message.includes("Expected")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return `Field '${path}' ${err.message.toLowerCase()}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return err.message; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleForm = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ({ name, schema, processSubmission, pii, honeypotField }) => | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async ({ request, response }) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Register PII fields with logger scrubber | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (pii?.length && response.locals?.logger?.scrub) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response.locals.logger.scrub(pii); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ({ name, schema, processSubmission, pii, honeypotField }) => | |
| async ({ request, response }) => { | |
| // Register PII fields with logger scrubber | |
| if (pii?.length && response.locals?.logger?.scrub) { | |
| response.locals.logger.scrub(pii); | |
| } | |
| ({ name, schema, processSubmission, pii, honeypotField }) => { | |
| if (!name) { | |
| throw new Error("name is required for handleForm configuration"); | |
| } | |
| if (!schema) { | |
| throw new Error("schema is required for handleForm configuration"); | |
| } | |
| if (!processSubmission) { | |
| throw new Error("processSubmission is required for handleForm configuration"); | |
| } | |
| return async ({ request, response }) => { | |
| // Register PII fields with logger scrubber | |
| if (pii?.length && response.locals?.logger?.scrub) { | |
| response.locals.logger.scrub(pii); | |
| } |
Copilot
AI
Dec 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for honeypot field with empty string. The implementation correctly uses a truthy check (line 35) that would allow empty strings, but there's no test verifying that an empty honeypot field doesn't trigger rejection. This is important to verify that legitimate users who leave the field blank aren't incorrectly flagged.
Add a test case:
test("allows submission when honeypot field is empty string", async () => {
const processSubmission = vi.fn().mockResolvedValue({});
const schema = Type.Object(
{
name: Type.String(),
website: Type.String(),
},
{ additionalProperties: false },
);
const middleware = handleForm({
name: "signup",
schema,
processSubmission,
pii: [],
honeypotField: "website",
});
await middleware({
request: {
body: { name: "Human", website: "" },
},
response: mockResponse,
});
assert({
given: "honeypot field with empty string",
should: "allow submission (call processSubmission)",
actual: processSubmission.mock.calls.length,
expected: 1,
});
});
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The honeypot check uses truthy evaluation which treats empty strings as valid (empty). However, the check body[honeypotField] will trigger for 0, false, and other falsy values that are not empty strings.
Since the functional requirement states "must be empty" and the test at line 182 checks for empty string specifically, the condition should be:
if (honeypotField && body[honeypotField] !== undefined && body[honeypotField] !== '') {This ensures that:
undefined(field not submitted) is allowed""(empty string) is allowed- Any other value triggers rejection
| if (honeypotField && body[honeypotField]) { | |
| if (honeypotField && body[honeypotField] !== undefined && body[honeypotField] !== '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: CSRF bypass allows form processing after rejection
When withCSRF rejects a request with 403, the asyncPipe composition pattern continues executing subsequent middleware. The handleForm middleware has no check for prior rejection and will still call processSubmission(body) if the body passes schema validation. This means CSRF-protected endpoints can still have their side effects (database writes, emails, etc.) executed by attackers, completely bypassing the CSRF protection. The handleForm middleware needs to check if the response has already been sent or if a prior middleware set an error status before calling processSubmission.
Additional Locations (1)
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TypeBox schema is compiled on every request (line 54), which is inefficient. TypeBox schemas should be compiled once during factory creation and reused across requests for better performance.
Suggestion: Move the compilation outside the middleware function:
const handleForm =
({ name, schema, processSubmission, pii, honeypotField }) => {
const validator = TypeCompiler.Compile(schema); // Compile once
return async ({ request, response }) => {
// ... rest of the code using validator
};
};| ({ name, schema, processSubmission, pii, honeypotField }) => | |
| async ({ request, response }) => { | |
| // Register PII fields with logger scrubber | |
| if (pii?.length && response.locals?.logger?.scrub) { | |
| response.locals.logger.scrub(pii); | |
| } | |
| const body = request.body || {}; | |
| // Check honeypot field if configured | |
| if (honeypotField && body[honeypotField]) { | |
| log(response, { | |
| message: "Form honeypot triggered", | |
| form: name, | |
| requestId: response.locals?.requestId, | |
| }); | |
| response.status(400); | |
| response.json({ | |
| errors: ["Validation failed"], | |
| }); | |
| return { request, response }; | |
| } | |
| // Validate against schema using TypeBox compiler | |
| const validator = TypeCompiler.Compile(schema); | |
| const valid = validator.Check(body); | |
| if (!valid) { | |
| const errors = formatErrors(validator.Errors(body)); | |
| log(response, { | |
| message: "Form validation failed", | |
| form: name, | |
| requestId: response.locals?.requestId, | |
| errorCount: errors.length, | |
| }); | |
| response.status(400); | |
| response.json({ errors }); | |
| return { request, response }; | |
| } | |
| // Process the validated submission | |
| await processSubmission(body); | |
| return { request, response }; | |
| ({ name, schema, processSubmission, pii, honeypotField }) => { | |
| const validator = TypeCompiler.Compile(schema); // Compile once per middleware instance | |
| return async ({ request, response }) => { | |
| // Register PII fields with logger scrubber | |
| if (pii?.length && response.locals?.logger?.scrub) { | |
| response.locals.logger.scrub(pii); | |
| } | |
| const body = request.body || {}; | |
| // Check honeypot field if configured | |
| if (honeypotField && body[honeypotField]) { | |
| log(response, { | |
| message: "Form honeypot triggered", | |
| form: name, | |
| requestId: response.locals?.requestId, | |
| }); | |
| response.status(400); | |
| response.json({ | |
| errors: ["Validation failed"], | |
| }); | |
| return { request, response }; | |
| } | |
| // Validate against schema using TypeBox compiler | |
| const valid = validator.Check(body); | |
| if (!valid) { | |
| const errors = formatErrors(validator.Errors(body)); | |
| log(response, { | |
| message: "Form validation failed", | |
| form: name, | |
| requestId: response.locals?.requestId, | |
| errorCount: errors.length, | |
| }); | |
| response.status(400); | |
| response.json({ errors }); | |
| return { request, response }; | |
| } | |
| // Process the validated submission | |
| await processSubmission(body); | |
| return { request, response }; | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing JSDoc documentation. Following the pattern in other middleware factories (e.g.,
createWithCors,createWithAuth), this factory should have comprehensive JSDoc comments including parameter descriptions, return types, and usage examples.Add documentation like: