Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+60 −3
Open

feat(security): adds cors #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7b44fd5
feat(security): enhance content security policy and add HSTS configur…
n3rdc4ptn Nov 5, 2025
e408b0c
fix(build): only enable sourcemaps if not building for production
n3rdc4ptn Nov 5, 2025
379a1ec
fix: removed env output for not leaking secrets
n3rdc4ptn Nov 7, 2025
7e91e6a
feat(cors): add CORS support with configurable allowed origins
n3rdc4ptn Nov 7, 2025
5096ebc
fix(deps): pin @fastify/cors version to 11.1.0
n3rdc4ptn Nov 7, 2025
caf33d7
fix(cors): remove console log for allowed origins in CORS configuration
n3rdc4ptn Nov 7, 2025
302b288
Merge branch 'main' into add-cors
n3rdc4ptn Nov 10, 2025
0a6d83e
fix: add put to cors methods
n3rdc4ptn Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .env.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,7 @@ FEEDBACK_URL_LINK=

# frame-ancestors attribute of CSP. Separate multiple values with a space
FRAME_ANCESTORS=

# Allowed CORS origins (comma-separated). Example: https://app.example.com,https://admin.example.com
# Leave empty to use POST_LOGIN_REDIRECT as the default allowed origin
ALLOWED_CORS_ORIGINS=
21 changes: 21 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"@apollo/client": "3.14.0",
"@fastify/autoload": "6.3.1",
"@fastify/cookie": "11.0.2",
"@fastify/cors": "11.1.0",
"@fastify/env": "5.0.3",
"@fastify/helmet": "13.0.2",
"@fastify/http-proxy": "11.3.0",
Expand Down
34 changes: 32 additions & 2 deletions server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Fastify from 'fastify';
import FastifyVite from '@fastify/vite';
import cors from '@fastify/cors';
import helmet from '@fastify/helmet';
import { fileURLToPath } from 'node:url';
import path from 'node:path';
Expand All @@ -12,8 +13,6 @@ import { injectDynatraceTag } from './server/config/dynatrace.js';

dotenv.config();

console.log(process.env);

const { DYNATRACE_SCRIPT_URL } = process.env;
if (DYNATRACE_SCRIPT_URL) {
injectDynatraceTag(DYNATRACE_SCRIPT_URL);
Expand Down Expand Up @@ -67,6 +66,28 @@ const fastify = Fastify({
logger: true,
});

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())
Comment on lines +75 to +77
Copy link

Copilot AI Nov 10, 2025

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];
Suggested change
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)

Copilot uses AI. Check for mistakes.
: // @ts-ignore
[fastify.config.POST_LOGIN_REDIRECT]; // Fallback to POST_LOGIN_REDIRECT
Comment on lines +74 to +79
Copy link

Copilot AI Nov 10, 2025

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.

Copilot uses AI. Check for mistakes.

if (!origin || allowedOrigins.includes(origin)) {
callback(null, true);
} else {
callback(new Error(`Origin ${origin} not allowed by CORS policy`), false);
Copy link

Copilot AI Nov 10, 2025

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.

Suggested change
callback(new Error(`Origin ${origin} not allowed by CORS policy`), false);
callback(null, false);

Copilot uses AI. Check for mistakes.
}
},
methods: ['GET', 'HEAD', 'POST', 'PUT', 'PATCH', 'DELETE'],
credentials: true, // Required for cookie-based sessions
});

Sentry.setupFastifyErrorHandler(fastify);
await fastify.register(envPlugin);

Expand Down Expand Up @@ -94,6 +115,9 @@ if (DYNATRACE_SCRIPT_URL) {
fastify.register(helmet, {
contentSecurityPolicy: {
directives: {
defaultSrc: ["'self'"],
styleSrc: ["'self'", "'unsafe-inline'"],
imgSrc: ["'self'", 'data:', 'https:'],
Copy link

Copilot AI Nov 10, 2025

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.

Suggested change
imgSrc: ["'self'", 'data:', 'https:'],
// Restrict imgSrc to trusted domains only. Add more domains as needed.
imgSrc: ["'self'", 'data:'],

Copilot uses AI. Check for mistakes.
'connect-src': ["'self'", 'sdk.openui5.org', sentryHost, dynatraceOrigin],
'script-src': isLocalDev
? ["'self'", "'unsafe-inline'", "'unsafe-eval'", sentryHost, dynatraceOrigin]
Expand All @@ -102,6 +126,12 @@ fastify.register(helmet, {
'frame-ancestors': [...fastify.config.FRAME_ANCESTORS.split(',')],
},
},
// Needed for https enforcement
hsts: {
maxAge: 31536000,
includeSubDomains: true,
preload: true,
},
});

fastify.register(proxy, {
Expand Down
1 change: 1 addition & 0 deletions server/config/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const schema = {
FEEDBACK_SLACK_URL: { type: 'string' },
FEEDBACK_URL_LINK: { type: 'string' },
FRAME_ANCESTORS: { type: 'string' },
ALLOWED_CORS_ORIGINS: { type: 'string' },
BFF_SENTRY_DSN: { type: 'string' },
FRONTEND_SENTRY_DSN: { type: 'string' },
FRONTEND_SENTRY_ENVIRONMENT: { type: 'string' },
Expand Down
2 changes: 1 addition & 1 deletion vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export default defineConfig({
},

build: {
sourcemap: true,
sourcemap: process.env.NODE_ENV !== 'production',
Copy link
Contributor

@andreaskienle andreaskienle Nov 10, 2025

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

target: 'esnext', // Support top-level await
},
});
Loading