Conversation
WalkthroughAdds a security module and configuration, integrates rate limiting and HTTP security headers, updates CORS to an allowlist, enforces body size limits, optionally trusts proxies, and documents new environment variables. Updates app bootstrap to apply Helmet, body-parser, CORS, and a global ThrottlerGuard. Adds dependencies and documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant N as Nest App (Express)
participant Conf as ConfigService (security)
participant H as Helmet (headers)
participant CO as CORS (allowlist)
participant BP as Body Parser (limits)
participant TG as ThrottlerGuard (global)
participant Ctrl as Controller
Note over N,Conf: Bootstrap
N->>Conf: Load security config (origins, limits, CSP, trustProxy, bodyLimit)
Conf-->>N: Config values
N->>H: Register Helmet with CSP defaults
N->>CO: Register CORS with allowlist + credentials
N->>BP: Register body size limits
N->>N: Enable trust proxy (optional)
N->>TG: Register global throttling guard
Note over C,N: Request handling
C->>N: HTTP request
N->>H: Apply security headers
N->>CO: Validate Origin against allowlist
alt Origin not allowed
CO-->>C: 403 Forbidden
else Allowed
N->>BP: Enforce body size limits
alt Exceeds limit
BP-->>C: 413 Payload Too Large
else Within limit
N->>TG: Check rate limit
alt Limit exceeded
TG-->>C: 429 Too Many Requests
else OK
N->>Ctrl: Handle route
Ctrl-->>N: Response
N-->>C: Response with security headers
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.env.example(1 hunks)README.md(1 hunks)package.json(2 hunks)src/app.module.ts(2 hunks)src/main.ts(1 hunks)src/security/security.config.ts(1 hunks)src/security/security.module.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.ts
📄 CodeRabbit inference engine (.cursorrules)
src/**/*.ts: Do not access environment variables via process.env directly; import and use the config object from src/config/env
Provide explicit return types for all functions
Do not use the any type; use unknown or specific types instead
Create interfaces for complex object shapes
Type all function parameters explicitly
Do not use console.log; use proper logging (e.g., NestJS Logger)
Remove unused imports
Do not leave commented-out code in commits
Wrap risky operations in try-catch and handle errors appropriately
Add JSDoc comments for complex logic
Files:
src/app.module.tssrc/security/security.config.tssrc/main.tssrc/security/security.module.ts
🧬 Code graph analysis (1)
src/security/security.module.ts (1)
src/app.module.ts (1)
Module(54-120)
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 40-40: [UnorderedKey] The RATE_LIMIT_DURATION key should go before the RATE_LIMIT_POINTS key
(UnorderedKey)
[warning] 41-41: [UnorderedKey] The BODY_LIMIT key should go before the RATE_LIMIT_DURATION key
(UnorderedKey)
[warning] 43-43: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 43-43: [UnorderedKey] The CSP_DEFAULT_SRC key should go before the RATE_LIMIT_DURATION key
(UnorderedKey)
🔇 Additional comments (4)
README.md (1)
551-582: Excellent security documentation.The Security section is comprehensive and well-structured. It clearly documents policies, environment variables, and acceptance criteria.
Minor suggestion: Consider adding a note that CSP directive values must use quoted tokens (e.g.,
'self'notself) as required by Helmet v8's stricter validation. The example at line 571 is correct, but clarifying this requirement would help prevent configuration errors.Based on learnings.
src/app.module.ts (2)
24-25: LGTM!Security module and throttler guard imports are correct and properly integrated.
113-118: LGTM!Global
ThrottlerGuardconfiguration is correctly implemented for application-wide rate limiting.package.json (1)
50-50: LGTM — Node.js v18+ requirement is documented in README.md (line 112).
| RATE_LIMIT_DURATION=60 | ||
| BODY_LIMIT=1mb | ||
| TRUST_PROXY=true | ||
| CSP_DEFAULT_SRC='self' |
There was a problem hiding this comment.
Remove quotes from CSP_DEFAULT_SRC environment variable value.
The quotes around 'self' will become part of the environment variable value, resulting in "'self'" (including the quotes) being read by the application. When this value is passed to Helmet's CSP configuration, it will be double-quoted and invalid.
Apply this diff to fix the environment variable:
-CSP_DEFAULT_SRC='self'
+CSP_DEFAULT_SRC=selfThe security configuration code (src/security/security.config.ts) should add the required quotes when constructing the CSP directive for Helmet, as CSP requires quoted keywords like 'self'.
Verify that security.config.ts properly quotes the value when passing it to Helmet:
#!/bin/bash
# Description: Verify CSP configuration handling in security.config.ts
# Check if security.config.ts exists and how it handles CSP_DEFAULT_SRC
echo "=== Checking security.config.ts for CSP handling ==="
rg -n -A3 -B3 "CSP_DEFAULT_SRC|cspDefaultSrc" src/security/security.config.ts || echo "security.config.ts not found or no CSP handling"🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 43-43: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 43-43: [UnorderedKey] The CSP_DEFAULT_SRC key should go before the RATE_LIMIT_DURATION key
(UnorderedKey)
🤖 Prompt for AI Agents
.env.example around line 43: the CSP_DEFAULT_SRC value includes literal quotes
('self') which will be read into the environment and break Helmet's CSP
handling; remove the surrounding quotes so the env var is CSP_DEFAULT_SRC=self
and then update src/security/security.config.ts to ensure it adds the required
single quotes around CSP keywords when building the Helmet CSP directives (e.g.,
wrap plain self in quotes when constructing the directive string or array) so
the runtime supplies correctly quoted values to Helmet.
| EscrowModule, | ||
| SupabaseModule, | ||
| EscrowModule, | ||
| EscrowModule, | ||
| StoresModule, | ||
| EscrowsModule, | ||
| SecurityModule, |
There was a problem hiding this comment.
Remove duplicate EscrowModule import.
EscrowModule appears twice in the imports array (lines 106 and 108). This duplication will cause NestJS module initialization to fail or behave unexpectedly.
Apply this diff to remove the duplicate:
OffersModule,
EscrowModule,
SupabaseModule,
- EscrowModule,
StoresModule,
EscrowsModule,
SecurityModule,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EscrowModule, | |
| SupabaseModule, | |
| EscrowModule, | |
| EscrowModule, | |
| StoresModule, | |
| EscrowsModule, | |
| SecurityModule, | |
| OffersModule, | |
| EscrowModule, | |
| SupabaseModule, | |
| StoresModule, | |
| EscrowsModule, | |
| SecurityModule, |
🤖 Prompt for AI Agents
In src/app.module.ts around lines 106 to 111, the imports array contains a
duplicate EscrowModule entry (lines 106 and 108); remove the second occurrence
so each module is listed only once, updating the imports array accordingly to
prevent NestJS initialization issues.
| const allowedOrigins = configService.get<string[]>('security.allowedOrigins', ['http://localhost:3000']); | ||
| app.enableCors({ | ||
| origin: (origin, callback) => { | ||
| if (!origin || allowedOrigins.includes(origin)) { | ||
| callback(null, true); | ||
| } else { | ||
| callback(new Error('Not allowed by CORS')); | ||
| } | ||
| }, | ||
| credentials: true, | ||
| }); |
There was a problem hiding this comment.
Trim allowlist entries before CORS check.
When ALLOWED_ORIGINS comes from env vars like "https://foo.com, https://bar.com", the leading space remains and the real origin will be rejected even though it’s listed. Please normalize and filter the values before comparing so legitimate requests aren’t blocked. (stackoverflow.com)
- const allowedOrigins = configService.get<string[]>('security.allowedOrigins', ['http://localhost:3000']);
+ const rawAllowedOrigins = configService.get<string[]>('security.allowedOrigins', ['http://localhost:3000']);
+ const allowedOrigins = rawAllowedOrigins
+ .map((origin) => origin.trim())
+ .filter((origin) => origin.length > 0);🤖 Prompt for AI Agents
In src/main.ts around lines 25 to 35, the CORS allowlist may contain entries
with leading/trailing spaces (e.g., "https://foo.com, https://bar.com") causing
legitimate origins to be rejected; normalize the configured allowedOrigins by
mapping each entry through trim() and filtering out empty strings (e.g., const
normalized = allowedOrigins.map(s => s.trim()).filter(Boolean)) and use that
normalized list in the origin check (or compare origin.trim() against the
normalized list) so whitespace does not prevent matches.
| contentSecurityPolicy: { | ||
| useDefaults: true, | ||
| directives: { | ||
| defaultSrc: [configService.get<string>('security.cspDefaultSrc', "'self'")], | ||
| }, | ||
| }, | ||
| crossOriginEmbedderPolicy: true, | ||
| crossOriginResourcePolicy: { policy: 'same-origin' }, | ||
| frameguard: { action: 'deny' }, | ||
| hsts: process.env.NODE_ENV === 'production' ? undefined : false, | ||
| noSniff: true, | ||
| xssFilter: true, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use config-derived NODE_ENV instead of process.env.
Directly touching process.env in src/**/*.ts violates our coding guidelines and makes testing harder. Please read the environment from ConfigService (e.g., const nodeEnv = configService.get<string>('NODE_ENV', 'development');) and reuse that for the Helmet HSTS toggle. As per coding guidelines
const app = await NestFactory.create(AppModule);
- const configService = app.get(ConfigService);
+ const configService = app.get(ConfigService);
+ const nodeEnv = configService.get<string>('NODE_ENV', 'development');
@@
- hsts: process.env.NODE_ENV === 'production' ? undefined : false,
+ hsts: nodeEnv === 'production' ? undefined : false,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contentSecurityPolicy: { | |
| useDefaults: true, | |
| directives: { | |
| defaultSrc: [configService.get<string>('security.cspDefaultSrc', "'self'")], | |
| }, | |
| }, | |
| crossOriginEmbedderPolicy: true, | |
| crossOriginResourcePolicy: { policy: 'same-origin' }, | |
| frameguard: { action: 'deny' }, | |
| hsts: process.env.NODE_ENV === 'production' ? undefined : false, | |
| noSniff: true, | |
| xssFilter: true, | |
| }) | |
| const app = await NestFactory.create(AppModule); | |
| const configService = app.get(ConfigService); | |
| const nodeEnv = configService.get<string>('NODE_ENV', 'development'); | |
| app.use(helmet({ | |
| contentSecurityPolicy: { | |
| useDefaults: true, | |
| directives: { | |
| defaultSrc: [configService.get<string>('security.cspDefaultSrc', "'self'")], | |
| }, | |
| }, | |
| crossOriginEmbedderPolicy: true, | |
| crossOriginResourcePolicy: { policy: 'same-origin' }, | |
| frameguard: { action: 'deny' }, | |
| hsts: nodeEnv === 'production' ? undefined : false, | |
| noSniff: true, | |
| xssFilter: true, | |
| })); |
🤖 Prompt for AI Agents
In src/main.ts around lines 57 to 69, the HSTS toggle currently reads
process.env.NODE_ENV directly; change it to use ConfigService so tests and code
follow guidelines — retrieve NODE_ENV from configService (e.g., const nodeEnv =
configService.get<string>('NODE_ENV', 'development')) before the helmet setup
and use nodeEnv === 'production' ? undefined : false for the hsts option,
ensuring no direct access to process.env in this file.
| // Global rate limiting guard | ||
| app.useGlobalGuards(new ThrottlerGuard()); | ||
|
|
There was a problem hiding this comment.
Instantiate ThrottlerGuard via DI.
new ThrottlerGuard() skips Nest’s injector, so required dependencies like the storage service and reflector come through as undefined, breaking rate limiting at runtime. Fetch the guard from the container instead. (docs.nestjs.com)
- app.useGlobalGuards(new ThrottlerGuard());
+ app.useGlobalGuards(app.get(ThrottlerGuard));🤖 Prompt for AI Agents
In src/main.ts around lines 72-74, do not instantiate the guard with new
ThrottlerGuard() because that bypasses Nest's injector and leaves dependencies
undefined; instead resolve the guard from the application container and pass
that instance to app.useGlobalGuards, and ensure the ThrottlerModule (or a
provider for ThrottlerGuard) is imported/provided so DI can create the guard
with its required dependencies.
| import { registerAs } from '@nestjs/config'; | ||
|
|
||
| export default registerAs('security', () => ({ | ||
| allowedOrigins: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000'], |
There was a problem hiding this comment.
Normalize origin list when splitting env var.
Comma-delimited env values often carry spaces or blank entries, which later fail the strict origin comparison. Trim and drop empty strings before exposing the allowlist so CORS behaves predictably. (stackoverflow.com)
-export default registerAs('security', () => ({
- allowedOrigins: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000'],
+export default registerAs('security', () => ({
+ allowedOrigins: process.env.ALLOWED_ORIGINS
+ ? process.env.ALLOWED_ORIGINS
+ .split(',')
+ .map((origin) => origin.trim())
+ .filter((origin) => origin.length > 0)
+ : ['http://localhost:3000'],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| allowedOrigins: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000'], | |
| export default registerAs('security', () => ({ | |
| allowedOrigins: process.env.ALLOWED_ORIGINS | |
| ? process.env.ALLOWED_ORIGINS | |
| .split(',') | |
| .map((origin) => origin.trim()) | |
| .filter((origin) => origin.length > 0) | |
| : ['http://localhost:3000'], | |
| // …other security settings | |
| })); |
🤖 Prompt for AI Agents
In src/security/security.config.ts around line 4, the ALLOWED_ORIGINS parse uses
a raw split that can leave spaces or empty strings; change the expression to
split on ',' only when the env var exists, then map each entry to trim() and
filter out empty strings (e.g. .split(',').map(...).filter(...)), otherwise fall
back to the default ['http://localhost:3000']; ensure the final value is an
array of non-empty, trimmed origins so CORS origin comparisons work reliably.
| rateLimitPoints: parseInt(process.env.RATE_LIMIT_POINTS || '100', 10), | ||
| rateLimitDuration: parseInt(process.env.RATE_LIMIT_DURATION || '60', 10), |
There was a problem hiding this comment.
Guard against NaN when parsing rate-limit settings.
If RATE_LIMIT_POINTS or RATE_LIMIT_DURATION contain non-numeric input, parseInt currently returns NaN, which then propagates into the throttler config and breaks enforcement. Please detect NaN and fall back to the documented defaults.
- rateLimitPoints: parseInt(process.env.RATE_LIMIT_POINTS || '100', 10),
- rateLimitDuration: parseInt(process.env.RATE_LIMIT_DURATION || '60', 10),
+ rateLimitPoints: (() => {
+ const parsed = Number.parseInt(process.env.RATE_LIMIT_POINTS ?? '', 10);
+ return Number.isNaN(parsed) ? 100 : parsed;
+ })(),
+ rateLimitDuration: (() => {
+ const parsed = Number.parseInt(process.env.RATE_LIMIT_DURATION ?? '', 10);
+ return Number.isNaN(parsed) ? 60 : parsed;
+ })(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rateLimitPoints: parseInt(process.env.RATE_LIMIT_POINTS || '100', 10), | |
| rateLimitDuration: parseInt(process.env.RATE_LIMIT_DURATION || '60', 10), | |
| rateLimitPoints: (() => { | |
| const parsed = Number.parseInt(process.env.RATE_LIMIT_POINTS ?? '', 10); | |
| return Number.isNaN(parsed) ? 100 : parsed; | |
| })(), | |
| rateLimitDuration: (() => { | |
| const parsed = Number.parseInt(process.env.RATE_LIMIT_DURATION ?? '', 10); | |
| return Number.isNaN(parsed) ? 60 : parsed; | |
| })(), |
🤖 Prompt for AI Agents
In src/security/security.config.ts around lines 5 to 6, the rateLimitPoints and
rateLimitDuration values are set via parseInt which can return NaN for
non-numeric env input; update the logic to parse the env values, check
Number.isNaN on the parsed result, and if NaN fall back to the documented
defaults (100 for points, 60 for duration), ensuring the parseInt call still
uses radix 10; this prevents NaN from propagating into the throttler config.
| ConfigModule.forRoot({ | ||
| isGlobal: true, | ||
| load: [securityConfig], | ||
| }), |
There was a problem hiding this comment.
Remove duplicate ConfigModule registration.
ConfigModule is already registered globally in src/app.module.ts (line 56). Re-registering it here with different configuration will override the global instance and may cause conflicts or unexpected behavior.
Apply this diff to remove the duplicate registration:
@Module({
imports: [
- ConfigModule.forRoot({
- isGlobal: true,
- load: [securityConfig],
- }),
ThrottlerModule.forRootAsync({
- useFactory: (config) => ({
+ imports: [ConfigModule],
+ inject: [ConfigService],
+ useFactory: (config: ConfigService) => ({
ttl: config.get('security.rateLimitDuration'),
limit: config.get('security.rateLimitPoints'),
}),
- inject: [ConfigModule],
}),
],
})Ensure security.config.ts is loaded in the global ConfigModule in src/app.module.ts instead:
#!/bin/bash
# Description: Verify security config is loaded in app.module.ts ConfigModule
echo "=== Checking app.module.ts ConfigModule registration ==="
rg -n -A5 "ConfigModule\.forRoot" src/app.module.ts
echo -e "\n=== Checking if securityConfig is imported in app.module.ts ==="
rg -n "securityConfig" src/app.module.ts || echo "securityConfig not found in app.module.ts - needs to be added to ConfigModule.forRoot load array"🤖 Prompt for AI Agents
In src/security/security.module.ts around lines 8 to 11, remove the duplicate
ConfigModule.forRoot({...}) registration because ConfigModule is already
registered globally in src/app.module.ts; instead, delete this local
registration block and ensure that the securityConfig export from
security.config.ts is added to the load array of the single global
ConfigModule.forRoot call in src/app.module.ts so the security configuration is
loaded centrally and no conflicting overrides occur.
1 participant
Solution: Enhanced Security Module #149 🔒
Implemented
Outcome
Oversized payloads return 413.
Application runs securely in both development and production.
✅ CI pipeline passes with security enabled.
Summary by CodeRabbit