-
Notifications
You must be signed in to change notification settings - Fork 3
feat(csrf): add csrf middleware #79
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
Conversation
@melikhov-dev you might want to take a look too |
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.
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 |
Copilot
AI
Oct 10, 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 hardcoded check for res.locals.oauth
creates tight coupling with OAuth implementation. Consider implementing the suggested res.locals.skipCsrf
approach for better maintainability.
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.
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:
Token it still being written to the
res.locals
. I think we should continue to use locals, but with two rules: