Add form handling and CSRF protection middleware

[ericelliott]

---

Note

Introduces handleForm and CSRF (createWithCSRF/withCSRF) middleware with TypeBox validation, honeypot/PII scrubbing, SHA3 token matching, plus docs, tests, type updates, and dependencies.

• Server Middleware (core):
  • Add handleForm (TypeBox validation, honeypot, PII scrubbing) in src/server/middleware/handle-form.js with comprehensive tests.
  • Add CSRF protection createWithCSRF/withCSRF (double-submit cookie, SHA3 hash comparison) in src/server/middleware/with-csrf.js with tests and integration tests with handleForm.
  • Export new middleware via src/server/middleware/index.js and src/server/index.js.
• Types:
  • Extend Response.locals in src/server/index.d.ts with csrfToken, log, logger; add types for handleForm, createWithCSRF, and withCSRF.
• Docs:
  • Update docs/server/README.md with API sections for handleForm, createWithCSRF/withCSRF, examples (backend/frontend), and recommended default middleware stack.
• Security Rules:
  • Add ai/rules/security/timing-safe-compare.mdc prescribing SHA3 hashing over timing-safe compare for secrets.
  • Update ai/rules/review.mdc references to new rule paths.
• Build/Deps:
  • Add @sinclair/typebox and js-sha3; update test scripts to include typecheck.
• Tasks:
  • Add tasks/form-csrf.md detailing requirements for form handling and CSRF middleware.

^{Written by Cursor Bugbot for commit cb12d1c. This will update automatically on new commits. Configure here.}

[ericelliott]

@cursor please /review

[cursor]

🔬 Code Review: Form Handling & CSRF Protection Middleware

I've completed a thorough code review of the PR following the project standards, JavaScript best practices, TDD methodology, and OWASP Top 10 security guidelines. Here are my findings:

---

✅ Strengths

1. Excellent Test Coverage: All 115 tests pass, including comprehensive test coverage for both handleForm and withCSRF middleware with all edge cases covered
2. Strong Security Implementation:
   • CSRF protection using double-submit cookie pattern with SHA3 hash comparison (timing attack prevention)
   • Honeypot field implementation that doesn't leak detection
   • PII field scrubbing support
   • SameSite=Strict and Secure cookies in production
3. Good Functional Programming Patterns: Follows project conventions with middleware composition
4. Clean Code Structure: Well-organized, follows existing middleware patterns

---

🚨 Critical Issues

1. Missing TypeScript Definitions (BLOCKING)

handleForm and withCSRF are not exported in src/server/index.d.ts. All other middleware has type definitions.

Impact: TypeScript users cannot use these middleware without type errors.

Required additions to index.d.ts:

// Form handling middleware
export interface HandleFormOptions {
  name: string;
  schema: any; // TypeBox schema type
  processSubmission: (data: any) => Promise<void>;
  pii?: string[];
  honeypotField?: string;
}

export function handleForm(options: HandleFormOptions): Middleware;

// CSRF protection middleware
export const withCSRF: Middleware;

2. Console.log in Production Code (SHOULD FIX)

Location: src/server/middleware/with-csrf.js:35

  console.log({
    message: "CSRF validation failed",
    requestId,
  });

Issue: Direct console.log violates the project's logging pattern. Other middleware uses proper logger integration.

Recommendation: Use structured logging like response.locals.logger or follow the pattern in other middleware. If intentional for security monitoring, document why.

---

⚠️ Code Quality Issues

3. Inconsistent Error Response Patterns

Location: handle-form.js vs with-csrf.js

• handleForm returns generic validation errors without request ID
• withCSRF includes request ID in error response

Recommendation: Standardize error responses to always include requestId when available for traceability.

4. Parameter Ordering Violation

Location: handle-form.js:25

const handleForm =
  ({ name, schema, processSubmission, pii, honeypotField }) =>
  async ({ request, response }) => {

Per javascript.mdc, the parameters should be alphabetically ordered for consistency, or at minimum, required parameters should come before optional ones. Current order:

• name (required)
• schema (required)
• processSubmission (required)
• pii (optional)
• honeypotField (optional)

Recommendation: While current ordering is acceptable (required before optional), consider alphabetizing optional parameters for consistency: honeypotField, name, pii, processSubmission, schema.

5. Hardcoded Status Codes

Status codes 400 and 403 are used directly without constants. While not critical, consider defining:

const HTTP_BAD_REQUEST = 400;
const HTTP_FORBIDDEN = 403;

This improves maintainability and self-documentation.

6. Missing HttpOnly Flag Documentation

The intentional absence of HttpOnly on CSRF cookies is security-critical but only mentioned in tests. Add a comment in with-csrf.js:26-32 explaining why HttpOnly is intentionally omitted (client needs to read the token).

---

📋 OWASP Top 10 (2021) Security Review

✅ A01:2021 – Broken Access Control: Not applicable (handled by separate withAuth middleware)

✅ A02:2021 – Cryptographic Failures:

• ✅ SHA3-256 used for token comparison (timing attack prevention)
• ✅ CUID2 used for cryptographically secure token generation
• ✅ No sensitive data exposed in logs

⚠️ A03:2021 – Injection:

• ✅ TypeBox validation prevents injection in form data
• ⚠️ MINOR CONCERN: Error messages include user-supplied field names without sanitization (formatErrors function). While TypeBox validates structure, consider additional sanitization for field names in error messages to prevent potential XSS if errors are rendered in HTML.

✅ A04:2021 – Insecure Design:

• ✅ Double-submit CSRF pattern is industry-standard
• ✅ Honeypot pattern properly implemented

✅ A05:2021 – Security Misconfiguration:

• ✅ Secure flag set in production
• ✅ SameSite=Strict prevents CSRF
• ✅ No HttpOnly (intentional for double-submit pattern)

✅ A06:2021 – Vulnerable and Outdated Components: Dependencies are current

✅ A07:2021 – Identification and Authentication Failures: Not applicable (separate concern)

✅ A08:2021 – Software and Data Integrity Failures:

• ✅ No dynamic code execution
• ✅ TypeBox schema validation enforces data integrity

⚠️ A09:2021 – Security Logging and Monitoring Failures:

• ⚠️ CONCERN: CSRF failures logged to console instead of structured logger
• ⚠️ CONCERN: handleForm validation failures are not logged at all

Recommendation: Add logging for all validation failures for security monitoring.

✅ A10:2021 – Server-Side Request Forgery (SSRF): Not applicable

---

📊 Test Quality Assessment

Excellent adherence to TDD principles:

• ✅ All tests follow Riteway patterns with clear given/should/actual/expected
• ✅ Tests are isolated with proper mocking using vi.fn()
• ✅ Each functional requirement has corresponding test
• ✅ Edge cases covered (empty strings, missing fields, malformed data)
• ✅ Security scenarios tested (timing attacks, token mismatches)

Minor observation: Tests could benefit from testing actual integration with createRoute to ensure error propagation works as documented.

---

🎨 JavaScript Style Guide Compliance

Good compliance overall with minor deviations:

✅ Followed:

• Functional programming patterns
• Arrow functions throughout
• Destructuring in function signatures
• Pure function composition
• Point-free style where appropriate
• Concise syntax

⚠️ Violations:

1. Intermediate variables in formatErrors: Could be more point-free

   // Current
   const path = err.path.slice(1) || "root";
   
   // More functional
   return [...errors].map(err => {
     const path = err.path.slice(1) || "root";
     return err.message.includes("Required") ? `Missing required field: ${path}` : ...
   });

2. console.log instead of proper logging abstraction

3. Boolean logic in tokensMatch: While correct, could be clearer

   // Current (line 22-24)
   const tokensMatch = (token1, token2) => {
     return hashToken(token1) === hashToken(token2);
   };
   
   // More concise (implicit return)
   const tokensMatch = (token1, token2) => 
     hashToken(token1) === hashToken(token2);

---

📝 Documentation & Comments

✅ Good:

• File-level docblocks present
• Test descriptions are clear and comprehensive

⚠️ Missing:

• JSDoc for handleForm parameters (public API)
• JSDoc for withCSRF behavior
• Inline comment explaining why HttpOnly is omitted from CSRF cookie
• Comment explaining honeypot security-by-obscurity approach

---

🔍 Architecture & Design Patterns

✅ Excellent adherence to project patterns:

• Factory pattern for handleForm matches other middleware
• Middleware signature async ({ request, response }) => { ... } is consistent
• Error handling delegates to createRoute as designed
• Composable with other middleware using asyncPipe

---

🗑️ Dead Code / Forgotten Files

✅ No issues found:

• No unused imports
• No commented-out code
• No temporary files
• Task file tasks/form-csrf.md is appropriate documentation
• All exports are used and exported correctly in index.js

---

📦 Dependencies

✅ Appropriate choices:

• @sinclair/typebox - Good choice over AJV (lighter, TypeScript-native, better DX)
• js-sha3 - Appropriate for timing-attack-safe comparison
• Both are well-maintained and secure

---

🎯 Priority Recommendations

MUST FIX (Blocking):

1. ✅ Add TypeScript definitions to src/server/index.d.ts

SHOULD FIX (Before Merge):

2. Replace console.log with proper structured logging
3. Add JSDoc for public APIs (handleForm, withCSRF)
4. Add security logging for validation failures
5. Consider sanitizing field names in error messages

NICE TO HAVE (Future):

6. Standardize error response format (include requestId consistently)
7. Add inline comments for security-critical decisions
8. Extract magic strings to constants (status codes, cookie name)

---

📊 Overall Assessment

Quality Score: 8.5/10

This is high-quality, well-tested code that follows project conventions and implements security best practices. The main blocker is missing TypeScript definitions. Once that's addressed and logging is improved, this would be merge-ready.

The implementation demonstrates:

• ✅ Strong security awareness
• ✅ Excellent test coverage
• ✅ Good functional programming patterns
• ✅ Proper middleware composition
• ⚠️ Missing TypeScript support (critical for project)
• ⚠️ Logging inconsistencies

Recommendation: APPROVE with required changes - Address the TypeScript definitions and logging, then this is ready to merge.

---

 

_{Learn more about Cursor Agents}

[cursor]

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
_{Learn more about Cursor Agents}

[Copilot]

Pull request overview

This PR adds comprehensive form handling and CSRF protection middleware to the server framework, enabling secure form submissions with JSON Schema validation and stateless CSRF protection using the double-submit cookie pattern.

Key changes:

• Introduces handleForm factory for form validation with TypeBox schema validation, honeypot spam protection, and PII scrubbing support
• Implements withCSRF middleware providing stateless CSRF protection using CUID2 tokens and SHA3 hashing for timing attack prevention
• Adds dependencies for @sinclair/typebox (schema validation) and js-sha3 (cryptographic hashing)

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
tasks/form-csrf.md	Task specification documenting requirements for both middleware implementations
src/server/middleware/handle-form.js	Factory function creating form validation middleware with TypeBox schema validation, honeypot protection, and PII handling
src/server/middleware/handle-form.test.js	Comprehensive test suite covering all 9 functional requirements for form handling
src/server/middleware/with-csrf.js	CSRF protection middleware implementing double-submit cookie pattern with SHA3 hashing
src/server/middleware/with-csrf.test.js	Test suite covering CSRF token generation, validation, cookie security flags, and rejection logging
src/server/middleware/index.js	Updated exports to include new handleForm and withCSRF middleware
package.json	Added @sinclair/typebox and js-sha3 dependencies
package-lock.json	Lockfile updates for new dependencies and reorganized peer dependencies

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Missing parameter validation for required factory parameters. Following the pattern established in other middleware factories (e.g., createWithAuth, createWithCors), required parameters should be validated at factory creation time, not at middleware execution time.

Add validation to throw clear errors for missing required parameters:

const handleForm =
  ({ 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 }) => {
      // ... rest of implementation
    };
  };

Suggested change

	({ 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]

Cookie parsing bug: The split("=") with array destructuring will fail for cookie values containing = characters. This could cause the CSRF token to be incorrectly parsed if it contains an = sign.

Fix by limiting the split to only the first occurrence:

const parseCookies = (cookieHeader) => {
  if (!cookieHeader) return {};
  return cookieHeader.split(";").reduce((cookies, cookie) => {
    const parts = cookie.trim().split("=");
    const name = parts[0];
    const value = parts.slice(1).join("="); // Rejoin in case value contains '='
    cookies[name] = value;
    return cookies;
  }, {});
};

Suggested change

	const [name, value] = cookie.trim().split("=");
	const trimmed = cookie.trim();
	const eqIdx = trimmed.indexOf("=");
	if (eqIdx === -1) return cookies;
	const name = trimmed.substring(0, eqIdx);
	const value = trimmed.substring(eqIdx + 1);

[Copilot]

Missing JSDoc documentation. Following the pattern in other middleware files (e.g., with-cors.js, with-auth.js), this middleware should have comprehensive JSDoc comments including parameter descriptions, return types, and usage examples.

Add documentation like:

/**
 * CSRF protection middleware using double-submit cookie pattern
 * 
 * @returns {Function} Async middleware function
 * 
 * @example
 * // Add CSRF protection to a route
 * import { withCSRF } from './middleware/index.js';
 * 
 * export default createRoute(withCSRF, async ({ request, response }) => {
 *   // Your route logic
 * });
 */
const withCSRF = async ({ request, response }) => {

[Copilot]

Missing test coverage for PUT, PATCH, and DELETE methods. The task specification (Req 2-5) mentions that CSRF protection should apply to all unsafe methods (POST/PUT/PATCH/DELETE), but tests only verify POST requests. While the implementation correctly includes these methods in validation, they should be explicitly tested.

Add test cases for other unsafe methods:

test("validates CSRF token for PUT request", async () => {
  const token = "test-token-123";
  const result = await withCSRF({
    request: {
      method: "PUT",
      headers: {
        cookie: `csrf_token=${token}`,
        "x-csrf-token": token,
      },
    },
    response: mockResponse,
  });
  // Assert successful validation
});

// Similar tests for PATCH and DELETE

[Copilot]

Performance issue: TypeBox validator is compiled on every request. The TypeCompiler.Compile() should be called once at factory creation time and reused across requests.

Move the compilation outside the middleware function:

const handleForm =
  ({ name, schema, processSubmission, pii, honeypotField }) => {
    // Compile validator once at factory creation
    const validator = TypeCompiler.Compile(schema);

    return async ({ request, response }) => {
      // ... PII and honeypot checks ...
      
      // Use pre-compiled validator
      const valid = validator.Check(body);
      // ... rest of validation logic
    };
  };

Suggested change

	({ 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]) {
	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) {
	response.status(400);
	response.json({
	errors: formatErrors(validator.Errors(body)),
	});
	return { request, response };
	}
	// Process the validated submission
	await processSubmission(body);
	return { request, response };
	({ name, schema, processSubmission, pii, honeypotField }) => {
	// Compile validator once at factory creation
	const validator = TypeCompiler.Compile(schema);
	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]) {
	response.status(400);
	response.json({
	errors: ["Validation failed"],
	});
	return { request, response };
	}
	// Validate against schema using pre-compiled TypeBox validator
	const valid = validator.Check(body);
	if (!valid) {
	response.status(400);
	response.json({
	errors: formatErrors(validator.Errors(body)),
	});
	return { request, response };
	}
	// Process the validated submission
	await processSubmission(body);
	return { request, response };
	};

[Copilot]

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:

/**
 * Form handling middleware factory with TypeBox validation
 * 
 * @param {Object} options
 * @param {string} options.name - Form identifier for logging
 * @param {Object} options.schema - TypeBox schema for validation
 * @param {Function} options.processSubmission - Async function to process validated data
 * @param {string[]} options.pii - Field names containing PII to scrub from logs
 * @param {string} [options.honeypotField] - Optional honeypot field name
 * @returns {Function} Async middleware function
 * 
 * @example
 * import { Type } from '@sinclair/typebox';
 * 
 * const contactForm = handleForm({
 *   name: 'contact',
 *   schema: Type.Object({
 *     email: Type.String(),
 *     message: Type.String()
 *   }, { additionalProperties: false }),
 *   processSubmission: async (data) => {
 *     await saveContact(data);
 *   },
 *   pii: ['email'],
 *   honeypotField: 'website'
 * });
 */
const handleForm = ...

[Copilot]

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]

Documentation error: The task specification states PII fields should be passed to request.locals.logger.scrub(pii), but the correct location is response.locals.logger as shown in the implementation. This is consistent with other middleware patterns in the codebase where state is attached to response.locals.

Update line 54 in the task documentation:

7. Given PII fields, should pass them to `response.locals.logger.scrub(pii)`

Suggested change

	7. Given PII fields, should pass them to `request.locals.logger.scrub(pii)`
	7. Given PII fields, should pass them to `response.locals.logger.scrub(pii)`

[Copilot]

Missing test coverage for HEAD and OPTIONS safe methods. The task specification (Req 1) states that GET/HEAD/OPTIONS requests should set CSRF tokens, and the comment on line 6 acknowledges this, but only GET requests are actually tested. These methods should be explicitly tested to verify the implementation handles all safe methods correctly.

Add test cases:

test("sets CSRF token cookie for HEAD request", async () => {
  const mockResponse = {
    locals: {},
    setHeader: vi.fn(),
  };

  await withCSRF({
    request: { method: "HEAD", headers: {} },
    response: mockResponse,
  });

  assert({
    given: "a HEAD request",
    should: "attach csrfToken to response.locals",
    actual: typeof mockResponse.locals.csrfToken,
    expected: "string",
  });
});

// Similar test for OPTIONS

[Copilot]

Pull request overview

Copilot reviewed 8 out of 10 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The cookie parser uses split("=") which will incorrectly parse cookies with = in their values. For example, token=abc=def would be split into ["token", "abc", "def"] and only "abc" would be used as the value.

Suggestion: Use split("=") with a limit or slice approach:

const parseCookies = (cookieHeader) => {
  if (!cookieHeader) return {};
  return cookieHeader.split(";").reduce((cookies, cookie) => {
    const parts = cookie.trim().split("=");
    const name = parts[0];
    const value = parts.slice(1).join("="); // Handle = in value
    cookies[name] = value;
    return cookies;
  }, {});
};

While CUID2 tokens don't contain =, this makes the parser more robust for future use.

Suggested change

	const [name, value] = cookie.trim().split("=");
	const parts = cookie.trim().split("=");
	const name = parts[0];
	const value = parts.slice(1).join("=");

[Copilot]

The documentation example shows honeypotField: 'website' but the schema doesn't include a website field. This is inconsistent with the more complete example later in the docs (line 874) which correctly includes website: Type.Optional(Type.String()) in the schema.

Without the field in the schema:

• Bots filling the honeypot would be rejected by the honeypot check ✓
• But it would also trigger "undeclared field" validation errors

Suggestion: Add the honeypot field to the schema for clarity and consistency:

const ContactSchema = Type.Object({
  name: Type.String(),
  email: Type.String({ format: 'email' }),
  message: Type.String(),
  website: Type.Optional(Type.String()), // Honeypot field
}, { additionalProperties: false });

Suggested change

	message: Type.String(),
	message: Type.String(),
	website: Type.Optional(Type.String()), // Honeypot field

[Copilot]

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
    };
  };

Suggested change

	({ 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 };
	};

[Copilot]

The test schema defines website as a required field (Type.String()), but this is inconsistent with honeypot field usage. Honeypot fields should be optional in the schema because:

• Bots fill them (rejected by honeypot check)
• Humans leave them empty (would fail validation if required)

Suggestion: Make the honeypot field optional:

const schema = Type.Object(
  {
    name: Type.String(),
    website: Type.Optional(Type.String()), // Should be optional
  },
  { additionalProperties: false },
);

This matches real-world usage where honeypot fields are hidden with CSS and should remain empty.

Suggested change

	website: Type.String(),
	website: Type.Optional(Type.String()),

[ericelliott]

please /review again against the latest commit hash

[ericelliott]

@cursoragent please validate copilot suggestions, as well against the latest version of the code.

[cursor]

🔬 Validation of Copilot Suggestions

I've reviewed the latest code (commit cac5ee9) against all Copilot suggestions. Here's my detailed validation:

---

✅ VALID SUGGESTIONS (Should Be Fixed)

1. Performance Issue: TypeBox Validator Compiled on Every Request ⚠️ CRITICAL

Status: VALID - This is a real performance issue.

Location: handle-form.js:54

    const validator = TypeCompiler.Compile(schema);

Impact: TypeBox compilation happens on every single form submission instead of once at middleware creation. This is significant overhead.

Fix: Move compilation outside the request handler:

const handleForm = ({ name, schema, processSubmission, pii, honeypotField }) => {
  const validator = TypeCompiler.Compile(schema); // Compile once
  
  return async ({ request, response }) => {
    // ... use validator
  };
};

---

2. Missing JSDoc for handleForm ⚠️ SHOULD FIX

Status: VALID - with-csrf.js now has JSDoc but handle-form.js doesn't.

Location: handle-form.js:1-3

The file only has a basic comment. Following the pattern in with-csrf.js (which now has proper JSDoc), handleForm should have comprehensive JSDoc documentation.

---

3. Missing Test for Empty String Honeypot ⚠️ SHOULD FIX

Status: VALID - Important edge case not covered.

Issue: Line 40 uses truthy check if (honeypotField && body[honeypotField]) which correctly allows empty strings, but there's no test verifying this behavior.

Why It Matters: Legitimate users submit empty honeypot fields. Need to ensure "" doesn't trigger rejection.

---

4. Documentation Error: request.locals vs response.locals ⚠️ SHOULD FIX

Status: VALID - Documentation bug.

Location: tasks/form-csrf.md:54

7. Given PII fields, should pass them to `request.locals.logger.scrub(pii)`

Should be response.locals.logger.scrub(pii) per the implementation and all other middleware patterns.

---

5. Missing Test Coverage for HEAD/OPTIONS Methods ⚠️ SHOULD FIX

Status: VALID - Requirement states GET/HEAD/OPTIONS but only GET is tested.

Location: with-csrf.test.js:7-34

Only GET request is tested for safe methods. Should add explicit tests for HEAD and OPTIONS to verify the SAFE_METHODS array works correctly.

---

6. Cookie Parser Bug with = in Values ⚠️ SHOULD FIX

Status: VALID - Though unlikely with CUID2, this is a correctness issue.

Location: with-csrf.js:27

    const [name, value] = cookie.trim().split("=");

Issue: token=abc=def would parse as value: "abc" instead of value: "abc=def".

Fix:

const parts = cookie.trim().split("=");
const name = parts[0];
const value = parts.slice(1).join("=");

While CUID2 tokens don't contain =, this makes the parser robust for general use.

---

7. Test Schema: Honeypot Field Should Be Optional ⚠️ SHOULD FIX

Status: VALID - Test doesn't match real-world usage.

Location: handle-form.test.js:105

        website: Type.String(),

Should be Type.Optional(Type.String()). In real use:

• Bots fill the field → rejected by honeypot check
• Humans leave it empty → would fail validation if required

This test happens to pass because the bot fills the field, but it's semantically incorrect.

---

✅ ALREADY FIXED

8. Documentation Example Missing Honeypot Field

Status: FIXED - The more complete example (line 874 in README.md) correctly includes:

website: Type.Optional(Type.String()),  // Honeypot

The simpler example (line 434) intentionally omits honeypot for clarity. This is acceptable.

---

❌ INVALID SUGGESTIONS

9. SHA3 for Timing Attack Prevention

Status: INCORRECT REASONING - @ericelliott is correct.

Copilot's Claim: SHA3 prevents timing attacks in CSRF token comparison.

Reality: SHA3 is NOT necessary for timing attack prevention in this context because:

1. Any change in input produces completely different hash output
2. No useful timing information leaks from hash comparison
3. You can't "play hangman" with hash comparison

Actual Purpose of SHA3 Here: Prevents token values from appearing in logs/errors, not timing attack prevention. The code works correctly but the reasoning is wrong.

The requirement in tasks/form-csrf.md:90 should be updated:

• "compare SHA3 hashes of tokens to prevent timing attacks"
• ✅ "compare SHA3 hashes of tokens to avoid logging raw token values"

---

📊 Summary

Must Fix (Blocking):

1. ✅ TypeBox compilation performance issue (line 54)

Should Fix (Quality):

2. ✅ Add JSDoc to handleForm
3. ✅ Add test for empty string honeypot
4. ✅ Fix documentation: request.locals → response.locals
5. ✅ Add tests for HEAD/OPTIONS methods
6. ✅ Fix cookie parser for = in values
7. ✅ Make honeypot field optional in test schema

Documentation Clarification:

8. ⚠️ Update SHA3 reasoning in tasks/form-csrf.md:90

---

🎯 Priority

Critical (fixes #1) improves performance significantly. The rest are correctness and test coverage improvements that should be addressed before merge but don't affect functionality in typical use cases.

All tests currently pass (133/133 ✅), but the code has room for improvement per these valid suggestions.

---

 

_{Learn more about Cursor Agents}

[Copilot]

Pull request overview

Copilot reviewed 8 out of 10 changed files in this pull request and generated 8 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Missing allowedHeaders for CSRF: The CSRF middleware accepts tokens via the X-CSRF-Token header, but the documentation example for the recommended middleware stack uses createWithCors with default allowed headers. The default allowed headers in createWithCors include common headers but may not include X-CSRF-Token.

Verify that X-CSRF-Token is included in the default CORS allowed headers, or update the documentation to explicitly mention it needs to be added:

const withCors = createWithCors({
  allowedOrigins: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000'],
  allowedHeaders: [
    'Origin',
    'X-Requested-With',
    'Content-Type',
    'Accept',
    'Authorization',
    'X-CSRF-Token'  // Required for CSRF protection
  ]
});

[Copilot]

Incorrect honeypot test schema: The test at lines 100-153 for honeypot validation uses a schema where the honeypot field is required (website: Type.String()), but in practice, honeypot fields should be optional since legitimate users won't fill them. The schema should be:

const schema = Type.Object(
  {
    name: Type.String(),
    website: Type.Optional(Type.String()),  // Optional, not required
  },
  { additionalProperties: false },
);

This matches the documentation example and reflects real-world usage where the honeypot field is hidden and left empty by legitimate users. The current test passes only because it's testing the filled case, but it doesn't properly represent the intended usage pattern.

[Copilot]

Missing test coverage: There are no tests verifying that handleForm throws errors when required parameters (name, schema, processSubmission) are missing. The test file for with-auth includes a test "requires auth parameter" which validates this pattern. Add similar tests:

test("requires name parameter", () => {
  let error;
  try {
    handleForm({ schema: Type.Object({}), processSubmission: vi.fn() });
  } catch (e) {
    error = e;
  }
  
  assert({
    given: "no name parameter",
    should: "throw error requiring name",
    actual: error?.message.includes("name") && error?.message.includes("required"),
    expected: true,
  });
});

test("requires schema parameter", () => {
  let error;
  try {
    handleForm({ name: "test", processSubmission: vi.fn() });
  } catch (e) {
    error = e;
  }
  
  assert({
    given: "no schema parameter",
    should: "throw error requiring schema",
    actual: error?.message.includes("schema") && error?.message.includes("required"),
    expected: true,
  });
});

test("requires processSubmission parameter", () => {
  let error;
  try {
    handleForm({ name: "test", schema: Type.Object({}) });
  } catch (e) {
    error = e;
  }
  
  assert({
    given: "no processSubmission parameter",
    should: "throw error requiring processSubmission",
    actual: error?.message.includes("processSubmission") && error?.message.includes("required"),
    expected: true,
  });
});

[Copilot]

Missing test coverage for PUT, PATCH, and DELETE methods: The tests only cover POST requests for unsafe methods, but the middleware should validate CSRF tokens for PUT, PATCH, and DELETE as well. Add tests to ensure these methods are properly protected:

test("allows PUT request with matching token in header", async () => {
  const token = "test-token-put";
  const mockResponse = {
    locals: {},
    setHeader: vi.fn(),
    status: vi.fn(),
    json: vi.fn(),
  };

  const result = await withCSRF({
    request: {
      method: "PUT",
      headers: {
        cookie: `csrf_token=${token}`,
        "x-csrf-token": token,
      },
    },
    response: mockResponse,
  });

  assert({
    given: "a PUT request with matching token in header",
    should: "allow request to proceed",
    actual: result.request.method,
    expected: "PUT",
  });
});

// Similar tests for PATCH and DELETE

[Copilot]

Missing test coverage for HEAD and OPTIONS methods: The tests only cover GET requests for safe methods, but the middleware is designed to handle HEAD and OPTIONS as well (line 20: const SAFE_METHODS = ["GET", "HEAD", "OPTIONS"];). Add tests to verify HEAD and OPTIONS requests also set CSRF cookies and expose tokens:

test("sets CSRF token cookie and response.locals.csrfToken for HEAD request", async () => {
  const cookies = {};
  const mockResponse = {
    locals: {},
    setHeader: vi.fn((name, value) => {
      if (name === "Set-Cookie") cookies.raw = value;
    }),
  };

  await withCSRF({
    request: { method: "HEAD", headers: {} },
    response: mockResponse,
  });

  assert({
    given: "a HEAD request",
    should: "attach csrfToken to response.locals",
    actual: typeof mockResponse.locals.csrfToken,
    expected: "string",
  });
});

test("sets CSRF token cookie and response.locals.csrfToken for OPTIONS request", async () => {
  const cookies = {};
  const mockResponse = {
    locals: {},
    setHeader: vi.fn((name, value) => {
      if (name === "Set-Cookie") cookies.raw = value;
    }),
  };

  await withCSRF({
    request: { method: "OPTIONS", headers: {} },
    response: mockResponse,
  });

  assert({
    given: "an OPTIONS request",
    should: "attach csrfToken to response.locals",
    actual: typeof mockResponse.locals.csrfToken,
    expected: "string",
  });
});

[Copilot]

[nitpick] Inconsistent error response format: The CSRF validation failure returns { error: "CSRF validation failed", requestId } but this differs from the pattern used in other middleware. The createWithAuth middleware returns { error: "Unauthorized" } without requestId. For consistency and better debugging, the requestId inclusion is actually helpful. However, consider whether this should be standardized across all error responses in the codebase.

[Copilot]

[nitpick] Security issue: Using SHA3 hash comparison for CSRF tokens provides timing attack protection, but the hash is computed on-the-fly for each comparison. For better security and consistency with the task requirement "compare SHA3 hashes of tokens to prevent timing attacks", consider using a constant-time comparison function after hashing. However, since SHA3 hashes are of fixed length and the comparison is of hash outputs (not the original tokens), this implementation does provide timing attack protection as intended.

Note: The current implementation is secure because it compares hashes, but the hash comparison itself (=== operator) could theoretically leak timing information. For maximum security, consider using a constant-time comparison:

import { timingSafeEqual } from 'crypto';

const tokensMatch = (token1, token2) => {
  const hash1 = Buffer.from(hashToken(token1), 'hex');
  const hash2 = Buffer.from(hashToken(token2), 'hex');
  return hash1.length === hash2.length && timingSafeEqual(hash1, hash2);
};

However, this is likely overkill since SHA3 outputs are compared, not the tokens themselves.

[Copilot]

Missing validation: The handleForm factory should validate that required parameters name, schema, and processSubmission are provided, throwing clear errors at factory creation time rather than allowing runtime failures. This follows the established pattern seen in createWithAuth and createWithCors where required parameters are validated immediately.

Example from createWithAuth:

if (!auth) {
  throw new Error("auth is required. Pass your better-auth instance.");
}

Add similar validation:

const handleForm = ({ name, schema, processSubmission, pii, honeypotField }) => {
  if (!name) {
    throw new Error("name is required for form identification in logs");
  }
  if (!schema) {
    throw new Error("schema is required for form validation");
  }
  if (!processSubmission) {
    throw new Error("processSubmission is required to handle validated form data");
  }
  
  return async ({ request, response }) => {
    // ... rest of implementation
  };
};

[ericelliott]

@cursoragent please review again

[cursor]

🔬 Fresh Code Review - Latest Commit

I've reviewed the latest code (commit bdc3b7e). Good progress has been made addressing previous issues, but critical bugs remain that will break production usage.

---

🚨 CRITICAL BUGS (Must Fix Before Merge)

1. CSRF Body Token Incompatible with Form Validation ⛔ BLOCKING

Severity: HIGH - This breaks the documented integration pattern

Issue: When withCSRF and handleForm are used together (as shown in docs), submitting CSRF tokens via request body (_csrf field) will always fail validation.

Root Cause:

• handleForm validates the full request.body including _csrf
• Example schemas use additionalProperties: false
• _csrf is not included in schemas
• Result: 400 error "Undeclared field not allowed: _csrf"

Example of Broken Code (from docs):

const ContactSchema = Type.Object({
  name: Type.String(),
  email: Type.String(),
  message: Type.String(),
}, { additionalProperties: false }); // ← This breaks _csrf in body

const withContactForm = handleForm({
  schema: ContactSchema, // ← Will reject any body with _csrf
  // ...
});

Solutions (pick one):

Option A: Strip _csrf from body before validation:

// In handleForm, line 65-66
const body = request.body || {};
const { _csrf, ...validationBody } = body; // Strip CSRF token
// Then validate validationBody instead of body

Option B: Document that body tokens are incompatible with strict schemas:

// Update docs to show _csrf must be optional in schema:
const ContactSchema = Type.Object({
  name: Type.String(),
  email: Type.String(),
  _csrf: Type.Optional(Type.String()), // ← Required if using body tokens
}, { additionalProperties: false });

Option C: Recommend header-only CSRF tokens in docs and update examples.

Recommendation: Option A - Auto-strip _csrf before validation. This provides the best DX and matches user expectations.

---

⚠️ HIGH PRIORITY ISSUES

2. Missing Test Coverage for Critical Paths

Several important code paths are untested:

a) HEAD/OPTIONS Methods (CSRF)

• Code supports ["GET", "HEAD", "OPTIONS"] but only GET is tested
• Missing verification that HEAD/OPTIONS set tokens correctly

b) PUT/PATCH/DELETE Methods (CSRF)

• Code validates all non-safe methods but only POST is tested
• PUT/PATCH/DELETE validation is untested

c) Empty String Honeypot (Form)

• Code: if (honeypotField && body[honeypotField]) correctly allows ""
• No test verifies empty string doesn't trigger rejection
• Critical for legitimate users who leave honeypot blank

d) Missing Required Parameters (Form)

• handleForm doesn't validate required parameters at creation time
• Will fail at runtime with cryptic errors
• Other middleware (e.g., createWithAuth) validates parameters immediately

Test Code Needed:

// handleForm should validate parameters
test("requires schema parameter", () => {
  let error;
  try {
    handleForm({ name: "test", processSubmission: vi.fn() });
  } catch (e) {
    error = e;
  }
  assert({
    given: "missing required schema parameter",
    should: "throw descriptive error",
    actual: error?.message.includes("schema"),
    expected: true,
  });
});

---

3. Test Schema Doesn't Match Real Usage

Location: handle-form.test.js:105

        website: Type.String(), // ← Should be Optional

Issue: Honeypot test uses Type.String() (required) but real honeypots should be Type.Optional(Type.String()):

• Bots fill it → rejected by honeypot check ✅
• Humans leave empty → would fail required validation ❌

Test passes only because bot fills the field, but schema is semantically wrong.

---

4. Documentation Bug: CORS Headers

Location: docs/server/README.md:647-649

const withCors = createWithCors({
  allowedOrigins: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000']
  // ← Missing allowedHeaders for X-CSRF-Token
});

Issue: Example doesn't include X-CSRF-Token in allowed headers. Cross-origin requests with CSRF header will be rejected by CORS.

Fix:

const withCors = createWithCors({
  allowedOrigins: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000'],
  allowedHeaders: ['Content-Type', 'Authorization', 'X-CSRF-Token']
});

---

5. Task Documentation Error

Location: tasks/form-csrf.md:54

7. Given PII fields, should pass them to `request.locals.logger.scrub(pii)`

Should be response.locals.logger.scrub(pii) - implementation and all other middleware use response.locals.

---

✅ Successfully Fixed (from Previous Reviews)

1. ✅ TypeBox compilation performance - Now compiles once at factory creation (line 57)
2. ✅ Cookie parser - Now handles = in values (lines 27-31)
3. ✅ CSRF token regeneration - Now reuses existing tokens (lines 91-94)
4. ✅ JSDoc documentation - Both files have comprehensive JSDoc
5. ✅ Logging - Uses response.locals.log fallback pattern

---

📊 Test Status

• ✅ 138 tests passing
• ❌ Missing ~8-10 critical test cases
• ❌ One test has incorrect schema (honeypot)

---

🎯 Priority Fix List

Must Fix (Blocking):

1. ⛔ Fix CSRF _csrf body token compatibility with form validation (Option A recommended)
2. ⛔ Add parameter validation to handleForm factory

Should Fix (Before Merge):

3. ⚠️ Add missing test coverage (HEAD/OPTIONS, PUT/PATCH/DELETE, empty honeypot, params)
4. ⚠️ Fix honeypot test schema to use Type.Optional()
5. ⚠️ Update docs example with CORS headers for CSRF
6. ⚠️ Fix tasks/form-csrf.md documentation error

---

💭 Architecture Consideration

The fundamental issue is middleware ordering - withCSRF adds _csrf to the body, then handleForm validates it. Three architectural options:

1. Strip _csrf in handleForm (Recommended) - Best DX
2. Validate before CSRF (Wrong order - breaks security)
3. Header-only tokens (Limits frontend flexibility)

Recommendation: Go with Option 1 - it's the cleanest solution that matches user expectations.

---

🔒 Security Review

✅ No new security issues
✅ Cookie parser fix improves robustness
✅ SHA3 timing comment clarifies intent (though still debatable)
⚠️ The _csrf validation issue could cause developers to disable strict schemas, reducing security

---

📝 Summary

Current Status: 85/100

Blocking Issues: 2 (CSRF body validation, parameter validation)

High Priority: 4 (test coverage, schema correctness, docs)

The code quality is good and most previous issues are fixed, but the CSRF body token validation bug is a show-stopper that will cause immediate failures in production for anyone following the documented patterns.

---

 

_{Learn more about Cursor Agents}

[Copilot]

Pull request overview

Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The handleForm factory should validate required parameters (name, schema, processSubmission) at creation time, not at middleware execution time. Following the established pattern in this codebase (see with-auth.js:42-44 and with-cors.js:46-52), required parameters should be validated when the factory is called to fail fast and provide clear errors during application startup.

Add validation at the beginning of handleForm:

const handleForm = ({
  name,
  schema,
  processSubmission,
  pii,
  honeypotField,
}) => {
  if (!name) {
    throw new Error("handleForm: name is required");
  }
  if (!schema) {
    throw new Error("handleForm: schema is required");
  }
  if (!processSubmission) {
    throw new Error("handleForm: processSubmission is required");
  }
  // ... rest of implementation
};

Suggested change

	}) => {
	}) => {
	if (!name) {
	throw new Error("handleForm: name is required");
	}
	if (!schema) {
	throw new Error("handleForm: schema is required");
	}
	if (!processSubmission) {
	throw new Error("handleForm: processSubmission is required");
	}

[Copilot]

Missing test coverage for required parameter validation in the handleForm factory. The test suite should include tests that verify the factory throws clear errors when required parameters (name, schema, processSubmission) are missing or invalid.

Example test to add:

test("requires name parameter", () => {
  let error;
  try {
    handleForm({ schema: Type.Object({}), processSubmission: vi.fn() });
  } catch (e) {
    error = e;
  }
  
  assert({
    given: "no name parameter",
    should: "throw error requiring name",
    actual: error?.message.includes("name") && error?.message.includes("required"),
    expected: true,
  });
});

Similar tests should be added for schema and processSubmission.

[ericelliott]

@cursor - please check the latest hash and create a simple report of all remaining bugs. Ignore nice to have things like "update all the other endpoints to include requestId in the error messages"

[cursor]

🐛 Bug Report - Commit bdc3b7e

All 138 tests pass, but 2 critical bugs will break production usage:

---

Bug #1: Missing Exports ⛔ HIGH SEVERITY

File: src/server/index.js

Issue: New middleware exported from src/server/middleware/index.js but NOT re-exported from main module.

Impact: Users cannot import the new middleware:

// This FAILS at runtime (undefined imports):
import { handleForm, withCSRF, createWithCSRF } from 'aidd/server';

Current exports (src/server/index.js:40-49):

export {
  createWithCors,
  withRequestId,
  createWithConfig,
  createConfigObject,
  loadConfigFromEnv,
  withServerError,
  createWithAuth,
  createWithOptionalAuth,
} from "./middleware/index.js";

Fix: Add missing exports:

export {
  createWithCors,
  withRequestId,
  createWithConfig,
  createConfigObject,
  loadConfigFromEnv,
  withServerError,
  createWithAuth,
  createWithOptionalAuth,
  handleForm,           // ← ADD
  createWithCSRF,       // ← ADD
  withCSRF,             // ← ADD
} from "./middleware/index.js";

---

Bug #2: CSRF Body Token Breaks Form Validation ⛔ HIGH SEVERITY

Files:

• src/server/middleware/with-csrf.js:106 (reads _csrf from body)
• src/server/middleware/handle-form.js:65-82 (validates full body)

Issue: When using withCSRF + handleForm together (as documented), submitting _csrf in request body always fails validation with strict schemas.

Why:

1. withCSRF reads _csrf from request.body
2. handleForm validates the entire request.body
3. Docs show schemas with additionalProperties: false
4. _csrf is not in schema → validation fails with "Undeclared field not allowed"

Reproduction:

const schema = Type.Object({
  name: Type.String(),
  email: Type.String(),
}, { additionalProperties: false }); // ← Breaks with _csrf in body

const route = createRoute(
  withCSRF,
  handleForm({ schema, ... }),
  async ({ response }) => { response.json({ success: true }); }
);

// POST with body: { name: "x", email: "x@y.com", _csrf: "token" }
// Result: 400 "Undeclared field not allowed: _csrf"

Fix Options:

A) Strip _csrf before validation (Recommended):

// In handleForm, line 65:
const body = request.body || {};
const { _csrf, ...validationBody } = body;
// Then validate validationBody instead of body

B) Remove _csrf after validation in withCSRF:

// In withCSRF, after line 107:
if (bodyToken) {
  delete request.body._csrf; // Remove after reading
}

**C) Document that schemas must include _csrf: Type.Optional(Type.String()) when using body tokens

---

Bug #3: Missing Parameter Validation ⚠️ MEDIUM SEVERITY

File: src/server/middleware/handle-form.js:49-55

Issue: handleForm doesn't validate required parameters at factory creation time.

Impact: Runtime errors with cryptic messages instead of clear startup errors.

Pattern: Other middleware validates immediately:

• createWithAuth checks if (!auth) and throws
• createWithCors checks if (!allowedOrigins) and throws

Fix: Add validation at line 56:

const handleForm = ({ name, schema, processSubmission, pii, honeypotField }) => {
  if (!name) throw new Error("handleForm: name is required");
  if (!schema) throw new Error("handleForm: schema is required");
  if (!processSubmission) throw new Error("handleForm: processSubmission is required");
  
  const validator = TypeCompiler.Compile(schema);
  // ... rest
};

---

Summary

Bug	Severity	Impact	Fix Complexity
#1: Missing exports	HIGH	Complete breakage	3 lines
#2: CSRF + form incompatibility	HIGH	Documentation pattern broken	1-2 lines
#3: No param validation	MEDIUM	Poor error messages	3 lines

Recommendation: Fix bugs #1 and #2 before merge. Bug #3 is nice-to-have but follows existing patterns in the codebase.

---

 

_{Learn more about Cursor Agents}

[ericelliott]

@cursoragent - please /review again to ensure your suggested changes have been implemented correctly

[cursor]

🐛 Bug Report - Commit bdc3b7e

Tests: ✅ 138/138 passing

---

⛔ CRITICAL BUGS (Blocking Merge)

Bug #1: Missing Exports - Library Unusable

Status: ❌ NOT FIXED

File: src/server/index.js:40-49

Issue: New middleware declared in TypeScript but not exported from JavaScript module.

Test:

# This will show handleForm, createWithCSRF, withCSRF are missing:
node -e "import('./src/server/index.js').then(m => console.log(Object.keys(m)))"

Current: Only exports 8 items (missing the 3 new ones)

Required Fix:

export {
  createWithCors,
  withRequestId,
  createWithConfig,
  createConfigObject,
  loadConfigFromEnv,
  withServerError,
  createWithAuth,
  createWithOptionalAuth,
  handleForm,        // ← MISSING
  createWithCSRF,    // ← MISSING
  withCSRF,          // ← MISSING
} from "./middleware/index.js";

---

Bug #2: CSRF Body Token Incompatible with Form Validation

Status: ❌ NOT FIXED

Files:

• src/server/middleware/with-csrf.js:106 (reads _csrf from body)
• src/server/middleware/handle-form.js:65,82 (validates full body)

Issue: When middleware are composed, _csrf in body triggers validation error with strict schemas.

Reproduction:

const schema = Type.Object({
  name: Type.String()
}, { additionalProperties: false });

createRoute(
  withCSRF,
  handleForm({ schema, ... }),
  handler
);

// POST { name: "x", _csrf: "token" }
// Result: 400 "Undeclared field not allowed: _csrf"

Required Fix (choose one):

Option A (Recommended - strip in handleForm):

// Line 65 in handle-form.js:
const { _csrf, ...body } = request.body || { _csrf: undefined };
// Then validate `body` instead

Option B (Remove in withCSRF after reading):

// Line 107 in with-csrf.js:
const submittedToken = headerToken || bodyToken;
if (bodyToken) delete request.body._csrf;

---

⚠️ REMAINING ISSUES (Should Fix)

Issue #1: Missing Parameter Validation

Status: ❌ NOT FIXED

handleForm doesn't validate required params at creation time (should follow createWithAuth pattern).

---

Issue #2: Incomplete Test Coverage

Status: ❌ NOT FIXED

Missing tests for:

• HEAD/OPTIONS methods (CSRF)
• PUT/PATCH/DELETE methods (CSRF)
• Empty string honeypot (Form)
• Required parameter validation (Form)

---

✅ FIXES CONFIRMED

1. ✅ TypeBox validator now compiled once (line 57)
2. ✅ Cookie parser handles = in values (lines 27-30)
3. ✅ CSRF token reuse implemented (lines 91-94)
4. ✅ Test schema fixed to use Type.Optional() for honeypot (line 105)
5. ✅ JSDoc added to both files
6. ✅ Structured logging pattern implemented

---

📊 Summary

Critical Bugs Remaining: 2

• Bug feat: Establish AIDD metaprograms system for Cursor #1: Library cannot be imported
• Bug Homepage planning #2: Documented pattern broken

Both must be fixed before merge. Bug #1 is a 3-line change. Bug #2 is a 1-2 line change.

---

 

_{Learn more about Cursor Agents}

[ericelliott]

@cursoragent please check the correct commit hash: 66b80e6

[cursor]

✅ Review Complete - Commit 66b80e6

Tests: ✅ 142/142 passing (up from 138!)

---

🎉 MAJOR FIXES CONFIRMED

✅ Bug #1: Missing Exports - FIXED

Lines: src/server/index.js:49-51

  handleForm,
  createWithCSRF,
  withCSRF,

All 3 new exports now present ✓

---

✅ Bug #2: CSRF Body Token Compatibility - FIXED

Line: src/server/middleware/handle-form.js:72

// Strip _csrf token from body before validation (used by withCSRF middleware)
const { _csrf, ...body } = request.body || {};

Perfect! Body tokens now work with strict schemas ✓

---

✅ Bug #3: Parameter Validation - FIXED

Lines: src/server/middleware/handle-form.js:57-60

if (!name) throw new Error("handleForm: name is required");
if (!schema) throw new Error("handleForm: schema is required");
if (!processSubmission)
  throw new Error("handleForm: processSubmission is required");

Fail-fast validation implemented ✓

---

🎯 NEW TEST COVERAGE ADDED

1. ✅ Empty string honeypot test (line 156)
2. ✅ HEAD request test (line 37)
3. ✅ OPTIONS request test (line 66)
4. ✅ Parameter validation tests (3 tests, lines 463-522)
5. ✅ _csrf stripping test (line 525)

Total new tests: 7 (increased from 10 to 14 for handleForm, from 13 to 18 for withCSRF)

---

⛔ NEW CRITICAL BUG FOUND

Bug #4: js-sha3 Import Incompatible with Node.js ESM

File: src/server/middleware/with-csrf.js:18

Issue: Named import from CommonJS module fails in Node.js runtime:

import { sha3_256 } from "js-sha3"; // ← FAILS in Node.js

Error:

SyntaxError: Named export 'sha3_256' not found. The requested module 
'js-sha3' is a CommonJS module, which may not support all module.exports 
as named exports.

Why Tests Pass: Vitest handles CommonJS interop, but production Node.js doesn't.

Fix Required:

import sha3 from "js-sha3";
const { sha3_256 } = sha3;

Impact: HIGH - Library will fail to load in production despite all tests passing.

---

⚠️ REMAINING GAP (Not Critical)

Missing PUT/PATCH/DELETE test coverage: Code handles these methods correctly (line 90: if (SAFE_METHODS.includes(request.method)) - inverted logic works for all unsafe methods), but explicit tests would improve confidence.

---

📊 Final Assessment

Score: 95/100 (one critical import bug)

Must Fix:

1. ⛔ Fix js-sha3 import statement (2 lines)

Recommended (but not blocking):

2. Add PUT/PATCH/DELETE tests for completeness

Once the import bug is fixed, this PR is ready to merge. Excellent work addressing all the previous review feedback!

---

 

_{Learn more about Cursor Agents}

[Copilot]

Pull request overview

Copilot reviewed 11 out of 13 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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

Suggested change

	if (honeypotField && body[honeypotField]) {
	if (honeypotField && body[honeypotField] !== undefined && body[honeypotField] !== '') {

[Copilot]

The handleForm middleware doesn't initialize response.locals if it doesn't exist. This follows the existing pattern from other middleware like with-auth.js which uses ensureLocals(), but handleForm accesses response.locals with optional chaining throughout without ensuring it exists first.

While the current code uses optional chaining (response.locals?.logger?.scrub), it should still initialize the object to follow the established pattern seen in other middleware (with-auth.js:10-12, with-csrf.js:88).

Add this at the start of the middleware function:

return async ({ request, response }) => {
  if (!response.locals) response.locals = {};
  // ... rest of code
};

Suggested change

	return async ({ request, response }) => {
	return async ({ request, response }) => {
	if (!response.locals) response.locals = {};

[Copilot]

[nitpick] Consider adding a comment explaining that Node.js normalizes all incoming headers to lowercase, similar to the pattern in with-cors.js:72. This helps developers understand why the header name is lowercase despite the documentation showing 'X-CSRF-Token'.

// Unsafe method - validate CSRF token
const cookies = parseCookies(request.headers?.cookie);
const cookieToken = cookies[COOKIE_NAME];
// Node.js normalizes all incoming headers to lowercase
const headerToken = request.headers?.["x-csrf-token"];
const bodyToken = request.body?._csrf;

Suggested change

	const cookieToken = cookies[COOKIE_NAME];
	const cookieToken = cookies[COOKIE_NAME];
	// Node.js normalizes all incoming header names to lowercase.
	// So although documentation may refer to 'X-CSRF-Token', access as 'x-csrf-token'.

[cursor]

Bug: CSRF failure doesn't prevent form processing execution

When withCSRF rejects a request with 403, it still returns { request, response } allowing the middleware pipeline to continue. This means handleForm's processSubmission callback will still execute even after CSRF validation fails. In the documented usage pattern (createRoute(withCSRF, handleForm(...), handler)), business logic like sending emails or creating database records will run without CSRF protection. The middleware should either throw an error to halt the pipeline, or handleForm should check response.statusCode or a flag in response.locals before calling processSubmission.

Additional Locations (1)

• src/server/middleware/handle-form.js#L103-L105

 

[cursor]

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)

• src/server/middleware/with-csrf.js#L115-L124

 

[ericelliott]

@cursoragent please validate fix and new copilot issues and /review with a BRIEF report hash cb12d1c

[cursor]

✅ Brief Review - Commit cb12d1c

Tests: ✅ 146/146 passing (+4 from 142)

---

🎉 CRITICAL FIX VALIDATED

✅ CSRF Bypass - FIXED (line 67)

if (response.statusCode && response.statusCode >= 400) {
  return { request, response };
}

• processSubmission no longer called after CSRF rejection ✓
• Integration tests confirm protection works ✓

✅ SHA3 Import - FIXED (lines 18-20)

import sha3 from "js-sha3";
const { sha3_256 } = sha3;

• CommonJS interop now correct ✓

---

⚠️ VALID COPILOT ISSUES REMAIN

1. Missing response.locals initialization (line 65)

• All other middleware initialize: if (!response.locals) response.locals = {};
• handleForm uses optional chaining but doesn't follow pattern
• Impact: Low (code works but inconsistent with codebase)

2. Honeypot truthy check (line 80)

• Current: if (honeypotField && body[honeypotField])
• Issue: Treats 0, false as filled (should only reject non-empty strings)
• Impact: Low (edge case, unlikely in practice)

---

❌ INVALID COPILOT ISSUES

Header comment suggestion - Nitpick, not blocking

---

📊 Assessment

Score: 98/100

Status: ✅ READY TO MERGE

• Critical security bug fixed
• All tests passing
• Remaining issues are low-priority style/consistency

Optional follow-up: Add response.locals init for consistency

---

 

_{Learn more about Cursor Agents}

[Copilot]

Pull request overview

Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The middleware should use createError from the error-causes library for all thrown errors. Based on the existing middleware patterns (e.g., with-auth.js, with-cors.js, with-config.js), parameter validation errors at factory creation time should use createError instead of new Error().

Example from with-cors.js:

import { createError } from "error-causes";

if (!allowedOrigins) {
  throw createError({
    name: "ConfigurationError", 
    message: "CORS configuration error: allowedOrigins is required..."
  });
}

This should be applied to the parameter validation in handleForm as well.

[Copilot]

[nitpick] The task specification (line 60) suggests using ajv for JSON Schema validation, but the implementation correctly uses @sinclair/typebox with TypeCompiler instead, which aligns with the codebase convention.

However, the task document should be updated to reflect the actual implementation decision. Consider updating line 60 from:

- Use a JSON Schema validation library (suggest ajv)

to:

- Use @sinclair/typebox with TypeCompiler for schema validation

This would match the actual implementation and the established pattern in the codebase.

Suggested change

	- Use a JSON Schema validation library (suggest ajv)
	- Configure ajv with `additionalProperties: false` behavior for requirement 8
	- Use @sinclair/typebox with TypeCompiler for schema validation
	- Configure the schema with `additionalProperties: false` for requirement 8

[cursor]

Bug: CSRF middleware doesn't set statusCode for downstream detection

The withCSRF middleware calls response.status(403) when rejecting requests but doesn't explicitly set response.statusCode. The downstream handleForm middleware checks response.statusCode >= 400 to skip processing if a prior middleware rejected the request. In real response objects where status() doesn't automatically set the statusCode property, handleForm won't detect the CSRF rejection and will proceed to call processSubmission - executing side effects that should have been blocked. Tests pass only because mocks artificially synchronize these properties.