-
Notifications
You must be signed in to change notification settings - Fork 7
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
PS-703 : Added CORS and helmet to middle ware for security #16
base: main
Are you sure you want to change the base?
Changes from 2 commits
09af75b
fb4859c
9ee37bc
ea31670
d5819fd
ca36ef7
3a4e18b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,11 +1,31 @@ | ||||||||||||||||||||||||||||||||
import { NestFactory } from '@nestjs/core'; | ||||||||||||||||||||||||||||||||
import helmet from 'helmet'; | ||||||||||||||||||||||||||||||||
import { AppModule } from './app.module'; | ||||||||||||||||||||||||||||||||
import { DocumentBuilder, SwaggerModule } from '@nestjs/swagger'; | ||||||||||||||||||||||||||||||||
import * as dotenv from 'dotenv'; | ||||||||||||||||||||||||||||||||
import { ConfigService } from '@nestjs/config'; | ||||||||||||||||||||||||||||||||
import { InternalServerErrorException } from '@nestjs/common'; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
async function bootstrap() { | ||||||||||||||||||||||||||||||||
dotenv.config(); // Load environment variables from .env file | ||||||||||||||||||||||||||||||||
const app = await NestFactory.create(AppModule); | ||||||||||||||||||||||||||||||||
const app = await NestFactory.create(AppModule, { cors: true }); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const configService = app.get(ConfigService); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const corsOriginList = configService | ||||||||||||||||||||||||||||||||
.get<string>('CORS_ORIGIN_LIST') | ||||||||||||||||||||||||||||||||
?.split(','); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (corsOriginList[0] !== '*' && !validateCorsOriginList(corsOriginList)) { | ||||||||||||||||||||||||||||||||
throw new Error('Invalid CORS_ORIGIN_LIST'); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
Comment on lines
+23
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential issue with empty If Add a check to handle cases where Apply this diff: + if (!corsOriginList || corsOriginList.length === 0) {
+ throw new Error('CORS_ORIGIN_LIST is not defined or empty');
+ }
if (corsOriginList[0] !== '*' && !validateCorsOriginList(corsOriginList)) {
throw new Error('Invalid CORS_ORIGIN_LIST');
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const corsOptions = { | ||||||||||||||||||||||||||||||||
origin: corsOriginList, // Array of allowed origins | ||||||||||||||||||||||||||||||||
methods: 'GET,HEAD,PUT,PATCH,POST,DELETE', // Specify allowed methods | ||||||||||||||||||||||||||||||||
credentials: true, // Allow credentials (cookies, authorization headers) | ||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
const config = new DocumentBuilder() | ||||||||||||||||||||||||||||||||
.setTitle('Middleware APIs') | ||||||||||||||||||||||||||||||||
.setDescription('The Middlware service') | ||||||||||||||||||||||||||||||||
|
@@ -17,8 +37,35 @@ async function bootstrap() { | |||||||||||||||||||||||||||||||
.build(); | ||||||||||||||||||||||||||||||||
const document = SwaggerModule.createDocument(app, config); | ||||||||||||||||||||||||||||||||
SwaggerModule.setup('api/swagger-docs', app, document); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
app.use((req, res, next) => { | ||||||||||||||||||||||||||||||||
const origin = req.headers.origin; | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
if (corsOriginList.includes(origin) || corsOriginList[0] === '*') { | ||||||||||||||||||||||||||||||||
res.setHeader('Access-Control-Allow-Origin', origin); | ||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||
throw new InternalServerErrorException('Invalid CORS_ORIGIN'); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improper error handling for invalid CORS origins. Throwing an Instead of throwing an exception, you can simply not set the Apply this diff: if (corsOriginList.includes(origin) || corsOriginList[0] === '*') {
res.setHeader('Access-Control-Allow-Origin', origin);
} else {
- throw new InternalServerErrorException('Invalid CORS_ORIGIN');
+ res.status(403).send('Forbidden');
+ return;
} However, removing the custom middleware as suggested earlier would eliminate this issue. 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
res.setHeader('Access-Control-Allow-Credentials', 'true'); | ||||||||||||||||||||||||||||||||
res.header( | ||||||||||||||||||||||||||||||||
'Access-Control-Allow-Methods', | ||||||||||||||||||||||||||||||||
'GET,HEAD,PUT,PATCH,POST,DELETE', | ||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||
next(); | ||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redundant and conflicting CORS middleware. You have manually implemented a middleware to handle CORS, but you also enable CORS using Consider removing the custom middleware and rely on Apply this diff to remove the custom middleware: - app.use((req, res, next) => {
- const origin = req.headers.origin;
-
- if (corsOriginList.includes(origin) || corsOriginList[0] === '*') {
- res.setHeader('Access-Control-Allow-Origin', origin);
- } else {
- throw new InternalServerErrorException('Invalid CORS_ORIGIN');
- }
- res.setHeader('Access-Control-Allow-Credentials', 'true');
- res.header(
- 'Access-Control-Allow-Methods',
- 'GET,HEAD,PUT,PATCH,POST,DELETE',
- );
- next();
- }); If you need custom logic, you can adjust the 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
app.enableCors(corsOptions); | ||||||||||||||||||||||||||||||||
app.use(helmet()); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
await app.listen(4000, () => { | ||||||||||||||||||||||||||||||||
console.log(`Server middleware on port - 4000`); | ||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
function validateCorsOriginList(corsOriginList: string[]): boolean { | ||||||||||||||||||||||||||||||||
return corsOriginList.every((origin) => { | ||||||||||||||||||||||||||||||||
return origin.includes('http://') || origin.includes('https://'); | ||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve CORS origin validation. The current validation only checks if the origins include Consider using the Apply this diff: function validateCorsOriginList(corsOriginList: string[]): boolean {
- return corsOriginList.every((origin) => {
- return origin.includes('http://') || origin.includes('https://');
- });
+ return corsOriginList.every((origin) => {
+ try {
+ new URL(origin);
+ return true;
+ } catch (error) {
+ return false;
+ }
+ });
} This approach ensures that each origin is a valid URL. 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
bootstrap(); |
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.
Redundant CORS configuration detected.
You have enabled CORS by setting
{ cors: true }
inNestFactory.create
, and later you enable CORS again usingapp.enableCors(corsOptions);
. This redundancy might cause unexpected behavior.Consider removing the
{ cors: true }
option when creating the app, since you are configuring CORS with custom options later.Apply this diff:
📝 Committable suggestion