-
Notifications
You must be signed in to change notification settings - Fork 3
feat(security): adds cors #362
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
base: main
Are you sure you want to change the base?
Conversation
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 CORS (Cross-Origin Resource Sharing) support to the Fastify server, introduces additional Content Security Policy (CSP) directives, and configures HSTS (HTTP Strict Transport Security). The changes aim to improve security by properly controlling cross-origin requests and enforcing HTTPS connections.
- Adds
@fastify/corsplugin with configurable allowed origins - Introduces new environment variable
ALLOWED_CORS_ORIGINSfor CORS configuration - Enhances CSP with explicit
defaultSrc,styleSrc, andimgSrcdirectives - Configures HSTS with 1-year max-age and subdomain inclusion
- Disables sourcemaps in production builds
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| server.ts | Adds CORS plugin registration with origin validation, enhances CSP directives, and configures HSTS |
| server/config/env.ts | Adds ALLOWED_CORS_ORIGINS to the environment schema |
| package.json | Adds @fastify/cors dependency version 11.1.0 |
| package-lock.json | Includes lock file entry for @fastify/cors dependency |
| .env.template | Documents the new ALLOWED_CORS_ORIGINS configuration option |
| vite.config.js | Disables sourcemaps in production for security/performance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!origin || allowedOrigins.includes(origin)) { | ||
| callback(null, true); | ||
| } else { | ||
| callback(new Error(`Origin ${origin} not allowed by CORS policy`), false); |
Copilot
AI
Nov 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.
According to the @fastify/cors documentation, when the origin callback passes an Error object, it will throw the error and close the connection. For denied CORS requests, it's more appropriate to use callback(null, false) which will result in a proper CORS error response without throwing an exception. The current implementation will cause the server to throw an error for invalid origins rather than gracefully denying the request.
| callback(new Error(`Origin ${origin} not allowed by CORS policy`), false); | |
| callback(null, false); |
| // @ts-ignore | ||
| const allowedOrigins = fastify.config.ALLOWED_CORS_ORIGINS | ||
| ? // @ts-ignore | ||
| fastify.config.ALLOWED_CORS_ORIGINS.split(',').map((o) => o.trim()) | ||
| : // @ts-ignore | ||
| [fastify.config.POST_LOGIN_REDIRECT]; // Fallback to POST_LOGIN_REDIRECT |
Copilot
AI
Nov 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.
[nitpick] Multiple @ts-ignore comments are used (lines 74, 76, 78) to suppress TypeScript errors when accessing fastify.config. Consider adding proper type definitions for the config object in the envPlugin or using // @ts-expect-error with explanatory comments instead, which will fail if the error no longer exists in future TypeScript versions.
| fastify.register(cors, { | ||
| origin: isLocalDev | ||
| ? true // Allow all origins in local development | ||
| : (origin, callback) => { | ||
| // In production, validate against allowed origins | ||
| // @ts-ignore | ||
| const allowedOrigins = fastify.config.ALLOWED_CORS_ORIGINS | ||
| ? // @ts-ignore | ||
| fastify.config.ALLOWED_CORS_ORIGINS.split(',').map((o) => o.trim()) | ||
| : // @ts-ignore | ||
| [fastify.config.POST_LOGIN_REDIRECT]; // Fallback to POST_LOGIN_REDIRECT | ||
|
|
||
| if (!origin || allowedOrigins.includes(origin)) { | ||
| callback(null, true); | ||
| } else { | ||
| callback(new Error(`Origin ${origin} not allowed by CORS policy`), false); | ||
| } | ||
| }, | ||
| methods: ['GET', 'HEAD', 'POST', 'PATCH', 'DELETE'], | ||
| credentials: true, // Required for cookie-based sessions | ||
| }); |
Copilot
AI
Nov 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 CORS plugin is registered before envPlugin (line 92), which means fastify.config will be undefined when the CORS origin validation callback executes. This will cause runtime errors when trying to access fastify.config.ALLOWED_CORS_ORIGINS and fastify.config.POST_LOGIN_REDIRECT.
Move the CORS registration block (lines 69-89) to after line 92 where envPlugin is registered, so that fastify.config is available when the CORS configuration needs it.
| const allowedOrigins = fastify.config.ALLOWED_CORS_ORIGINS | ||
| ? // @ts-ignore | ||
| fastify.config.ALLOWED_CORS_ORIGINS.split(',').map((o) => o.trim()) |
Copilot
AI
Nov 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 fallback logic doesn't properly handle the case where ALLOWED_CORS_ORIGINS is set to an empty string or contains only whitespace. An empty string will be split into [''], which means a request with an empty string as the origin will be considered valid.
Consider adding a check for empty/whitespace-only strings:
const allowedOrigins = fastify.config.ALLOWED_CORS_ORIGINS?.trim()
? fastify.config.ALLOWED_CORS_ORIGINS.split(',').map((o) => o.trim()).filter((o) => o)
: [fastify.config.POST_LOGIN_REDIRECT];| const allowedOrigins = fastify.config.ALLOWED_CORS_ORIGINS | |
| ? // @ts-ignore | |
| fastify.config.ALLOWED_CORS_ORIGINS.split(',').map((o) => o.trim()) | |
| const allowedOrigins = fastify.config.ALLOWED_CORS_ORIGINS && fastify.config.ALLOWED_CORS_ORIGINS.trim() | |
| ? // @ts-ignore | |
| fastify.config.ALLOWED_CORS_ORIGINS.split(',').map((o) => o.trim()).filter((o) => o) |
| directives: { | ||
| defaultSrc: ["'self'"], | ||
| styleSrc: ["'self'", "'unsafe-inline'"], | ||
| imgSrc: ["'self'", 'data:', 'https:'], |
Copilot
AI
Nov 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 imgSrc directive includes 'https:' which allows loading images from any HTTPS source. This is overly permissive and could be a security concern. Consider specifying explicit trusted domains instead of allowing all HTTPS sources, or document why this broad permission is necessary.
| imgSrc: ["'self'", 'data:', 'https:'], | |
| // Restrict imgSrc to trusted domains only. Add more domains as needed. | |
| imgSrc: ["'self'", 'data:'], |
|
|
||
| build: { | ||
| sourcemap: true, | ||
| sourcemap: process.env.NODE_ENV !== 'production', |
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.
This is also changed here: #359
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Is based on the branch of the PR #359
closes #359