Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,11 @@ PUSHER_APP_ID=your_app_id
PUSHER_KEY=your_key
PUSHER_SECRET=your_secret
PUSHER_CLUSTER=your_cluster

# --- Security Configuration ---
ALLOWED_ORIGINS=http://localhost:3000,https://yourapp.com
RATE_LIMIT_POINTS=100
RATE_LIMIT_DURATION=60
BODY_LIMIT=1mb
TRUST_PROXY=true
CSP_DEFAULT_SRC='self'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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=self

The 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.

33 changes: 33 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -547,3 +547,36 @@ For technical support or questions:
- [TypeORM Documentation](https://typeorm.io/)
- [Stellar SDK](https://stellar.github.io/js-stellar-sdk/)
- [OpenAPI Specification](https://swagger.io/specification/)

## 🔒 Security

StarShop Backend enforces robust security policies by default. These can be customized via environment variables:

- **HTTP Security Headers**: All responses include standard headers (HSTS, X-Frame-Options, XSS Protection, NoSniff, CSP) via Helmet.
- **CORS Policy**: Origins are restricted using an allowlist from `ALLOWED_ORIGINS`.
- **Rate Limiting**: Requests are throttled globally using `@nestjs/throttler`. Configure limits with `RATE_LIMIT_POINTS` and `RATE_LIMIT_DURATION`.
- **Validation**: All payloads are validated and sanitized globally. Unknown fields are stripped and invalid payloads are rejected.
- **Body Size Limits**: Request payload size is limited via `BODY_LIMIT` (default: 1mb). Oversized requests return HTTP 413.
- **Trust Proxy**: Enable with `TRUST_PROXY=true` if running behind a load balancer or reverse proxy.
- **Content Security Policy (CSP)**: Defaults to `'self'`, configurable via `CSP_DEFAULT_SRC`.

### Example Security Environment Variables

```env
ALLOWED_ORIGINS=http://localhost:3000,https://yourapp.com
RATE_LIMIT_POINTS=100
RATE_LIMIT_DURATION=60
BODY_LIMIT=1mb
TRUST_PROXY=true
CSP_DEFAULT_SRC='self'
```

### Acceptance Criteria
- All responses include Helmet headers (HSTS in production)
- CORS allowlist enforced from env; requests from disallowed origins are rejected
- Global validation strips unknown fields and rejects invalid payloads
- Rate limiting returns 429 when limits are exceeded; limits configurable by env
- Body size limits enforced; oversized payloads return 413
- CI passes and app boots with `npm run start:dev` and in production mode with security enabled

See `src/security/` for implementation details and further customization.
32 changes: 31 additions & 1 deletion package-lock.json

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

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"@nestjs/platform-express": "^11.1.0",
"@nestjs/schedule": "^6.0.0",
"@nestjs/swagger": "^11.2.0",
"@nestjs/throttler": "^6.4.0",
"@nestjs/typeorm": "^11.0.0",
"@supabase/supabase-js": "^2.46.1",
"@trustless-work/escrow": "^2.0.2",
Expand All @@ -62,6 +63,7 @@
"express-async-handler": "^1.2.0",
"express-rate-limit": "^7.5.0",
"express-validator": "^7.2.0",
"helmet": "^8.1.0",
"jsonwebtoken": "^9.0.2",
"multer": "^1.4.5-lts.2",
"multer-s3": "^3.0.1",
Expand Down
11 changes: 10 additions & 1 deletion src/app.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import { SupabaseModule } from './modules/supabase/supabase.module';
import { EscrowModule } from './modules/escrow/escrow.module';
import { AppCacheModule } from './cache/cache.module';
import { StoresModule } from './modules/stores/stores.module';
import { SecurityModule } from './security/security.module';
import { ThrottlerGuard } from '@nestjs/throttler';

// Entities
import { User } from './modules/users/entities/user.entity';
Expand Down Expand Up @@ -103,9 +105,16 @@ import { EscrowsModule } from './modules/escrows/escrows.module';
OffersModule,
EscrowModule,
SupabaseModule,
EscrowModule,
EscrowModule,
StoresModule,
EscrowsModule,
SecurityModule,
Comment on lines 106 to +111

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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.

],
providers: [
{
provide: 'APP_GUARD',
useClass: ThrottlerGuard,
},
],
})
export class AppModule { }
52 changes: 50 additions & 2 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,72 @@ import { DocumentBuilder, SwaggerModule } from '@nestjs/swagger';
import { BuyerRequestSchedulerService } from './modules/buyer-requests/services/buyer-request-scheduler.service';
import { ResponseInterceptor } from './common/interceptors/response.interceptor';
import { HttpExceptionFilter } from './common/filters/http-exception.filter';
import { ConfigService } from '@nestjs/config';
import helmet from 'helmet';
import * as bodyParser from 'body-parser';
import { ThrottlerGuard } from '@nestjs/throttler';

async function bootstrap(): Promise<void> {
const app = await NestFactory.create(AppModule);
const configService = app.get(ConfigService);

// Middleware to track request start time for latency
app.use((req, res, next) => {
req._startTime = Date.now();
next();
});

// Enable CORS
app.enableCors();
// Enable CORS with allowlist from config
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,
});
Comment on lines +25 to +35

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


// Global validation pipe
app.useGlobalPipes(
new ValidationPipe({ whitelist: true, forbidNonWhitelisted: true, transform: true })
);

// Set body size limits
app.use(bodyParser.json({ limit: configService.get<string>('security.bodyLimit', '1mb') }));
app.use(bodyParser.urlencoded({ extended: true, limit: configService.get<string>('security.bodyLimit', '1mb') }));

// Trust proxy if enabled
if (configService.get<boolean>('security.trustProxy', false)) {
const expressApp = app.getHttpAdapter().getInstance();
if (expressApp && typeof expressApp.set === 'function') {
expressApp.set('trust proxy', 1);
}
}

// Helmet security headers
app.use(
helmet({
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,
})
Comment on lines +57 to +69

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

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

Comment on lines +72 to +74

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

// Global response interceptor
app.useGlobalInterceptors(new ResponseInterceptor());

Expand Down
10 changes: 10 additions & 0 deletions src/security/security.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { registerAs } from '@nestjs/config';

export default registerAs('security', () => ({
allowedOrigins: process.env.ALLOWED_ORIGINS?.split(',') || ['http://localhost:3000'],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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),
Comment on lines +5 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

trustProxy: process.env.TRUST_PROXY === 'true',
bodyLimit: process.env.BODY_LIMIT || '1mb',
cspDefaultSrc: process.env.CSP_DEFAULT_SRC || "'self'",
}));
21 changes: 21 additions & 0 deletions src/security/security.module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { Module } from '@nestjs/common';
import { ConfigModule } from '@nestjs/config';
import securityConfig from './security.config';
import { ThrottlerModule } from '@nestjs/throttler';

@Module({
imports: [
ConfigModule.forRoot({
isGlobal: true,
load: [securityConfig],
}),
Comment on lines +8 to +11

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

ThrottlerModule.forRootAsync({
useFactory: (config) => ({
ttl: config.get('security.rateLimitDuration'),
limit: config.get('security.rateLimitPoints'),
}),
inject: [ConfigModule],
}),
],
})
export class SecurityModule {}