Skip to content

Conversation

resure
Copy link
Contributor

@resure resure commented Aug 14, 2025

Mostly copied from DataLens. Implementation is similar to CSRF middleware in core.

Main caveat: to work with this CSRF middleware, auth methods should set userId property to the request original context.

Few more differences with Core CSRF implementation:

  • This new middleware no longer sets cookie
  • We're only trying to read token from one header (x-csrf-token with option to override that name) without fallbacks

Token it still being written to the res.locals. I think we should continue to use locals, but with two rules:

  • We'll use res.locals (because it's standard way to express), but as little as possible and we'll extend locals typings for each of that properties (default express typings for locals is just record<string,any>)
  • We'll use locals only for things that are relevant only for HTTP request lifecycle and should not be written to the ctx (and passed down to application logic). For example, userId is ubiquitous thing and probably be useful for the context, but thing like csrfToken is only relevant on the routing and controller level and should not be passed down

@resure resure requested a review from DakEnviy as a code owner August 14, 2025 21:38
@resure
Copy link
Contributor Author

resure commented Aug 20, 2025

@melikhov-dev you might want to take a look too

melikhov-dev
melikhov-dev previously approved these changes Aug 20, 2025
@resure resure requested review from DakEnviy and Copilot October 10, 2025 15:49
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds CSRF (Cross-Site Request Forgery) protection middleware to ExpressKit, providing security against malicious cross-origin requests. The implementation is inspired by DataLens and similar to the core CSRF middleware with some key differences.

Key changes:

  • CSRF middleware generates and validates tokens for state-changing HTTP methods
  • Token generation requires user ID to be set in the request's original context by auth methods
  • Comprehensive test coverage for various CSRF scenarios and configurations

Reviewed Changes

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

Show a summary per file
File Description
src/types.ts Adds CSRF configuration options to AppConfig and removes deprecated Express module declarations
src/csrf.ts Implements the core CSRF middleware with token generation and validation logic
src/router.ts Integrates CSRF middleware into the route setup and moves context cleanup
src/constants.ts Adds CSRF token context key constant
src/base-middleware.ts Removes premature context cleanup to allow CSRF middleware access
src/tests/csrf.test.ts Comprehensive test suite covering all CSRF functionality
src/tests/lang-detect.test.ts Updates test handler naming and comments out assertions
package.json Updates nodekit dependency version
README.md Adds documentation for CSRF protection configuration and usage

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

res.set(headerName, csrfToken);

const isCsrfDisabled = Boolean(req.routeInfo?.disableCsrf);
const nonApplicableAuthMethod = Boolean(res.locals.oauth); // TODO we should move to something like res.locals.skipCsrf instead of this
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded check for res.locals.oauth creates tight coupling with OAuth implementation. Consider implementing the suggested res.locals.skipCsrf approach for better maintainability.

Suggested change
const nonApplicableAuthMethod = Boolean(res.locals.oauth); // TODO we should move to something like res.locals.skipCsrf instead of this
const nonApplicableAuthMethod = Boolean(res.locals.skipCsrf);

Copilot uses AI. Check for mistakes.

@resure resure merged commit 855e1a1 into main Oct 13, 2025
5 checks passed
@resure resure deleted the csrf branch October 13, 2025 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants