Repository navigation
fix(security): stop serving compiled backend at HTTP root, enforce validation and throttling - #124
dogancanbakir wants to merge 2 commits into
Conversation
…lidation and throttling - Remove app.use(express.static(path.join(__dirname))) from backend/src/main.ts, which mounted the entire compiled dist/ (including .js.map source maps, since tsconfig sets sourceMap: true) as static files at the server root, exposing the full backend source to any unauthenticated client. Drop the now-unused express and path imports. Closes #123. - Register a global ValidationPipe (whitelist: true, transform: true) in main.ts so every controller validates request bodies against its DTO decorators, and remove the now-redundant per-controller @UsePipes from the meetings, tasks, subtasks and tags controllers (they used the exact same options). forbidNonWhitelisted is intentionally left off, matching the issue's recommendation: whitelist already strips unknown properties, while forbidNonWhitelisted would turn previously-accepted extra fields into 400s for existing clients. Closes #38. - Enforce the ThrottlerModule config by registering GlobalThrottlerGuard as APP_GUARD in app.module.ts. The guard skips routes that already apply their own ThrottlerGuard subclass via @UseGuards (ApiKeyThrottlerGuard on the clubs, meetings, notifications and tasks controllers, which must run after the auth guards to key limits on the resolved API key), so those routes are not counted twice against the same throttler storage. Non-HTTP contexts (WebSockets) are skipped. Closes #39.
'ps aux | grep node' always exits 0 because grep matches its own process in the ps output, so the healthcheck could never report an unhealthy container. Use pgrep (available in Alpine's busybox), which matches only the node process and exits non-zero when it is gone.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Configure global validation backend/src/main.ts, backend/src/modules/meetings/presentation/controllers/meetings.controller.ts, backend/src/modules/tasks/presentation/controllers/*controller.ts |
The application configures a global validation pipe with whitelisting and transformation. The meetings, tasks, subtasks, and tags controllers no longer configure their own validation pipes. |
Global throttling
| Layer / File(s) | Summary |
|---|---|
Implement and register the global guard backend/src/shared/guards/global-throttler.guard.ts, backend/src/app.module.ts |
The new guard bypasses non-HTTP requests and routes with controller- or handler-level throttler guards. Other HTTP requests pass to the parent throttler guard. AppModule registers the guard globally. |
Compiled-file static serving
| Layer / File(s) | Summary |
|---|---|
Remove compiled-directory static serving backend/src/main.ts |
The application no longer serves its compiled directory as static files. |
Docker health check
| Layer / File(s) | Summary |
|---|---|
Update process check backend/Dockerfile |
The health check uses pgrep node and retains exit status 1 when the check fails. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: High
Sequence Diagram(s)
sequenceDiagram
participant HTTPRequest
participant GlobalThrottlerGuard
participant ThrottlerGuard
HTTPRequest->>GlobalThrottlerGuard: canActivate(context)
alt Non-HTTP request or local ThrottlerGuard metadata
GlobalThrottlerGuard-->>HTTPRequest: allow request
else HTTP request without local ThrottlerGuard metadata
GlobalThrottlerGuard->>ThrottlerGuard: canActivate(context)
ThrottlerGuard-->>GlobalThrottlerGuard: throttling result
GlobalThrottlerGuard-->>HTTPRequest: return result
end
Suggested reviewers: arjinalbay
Merge Risk: 
9cb3d
-
This PR stops serving compiled backend files, applies validation globally, adds global throttling, and makes the Docker healthcheck able to fail. No new merge-blocking regression was found. Meetings and tasks routes still have no rate limit for requests that fail authentication. That was already true before this change, so it is a possible follow-up rather than a blocker.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title names the two main changes: stopping the compiled backend from being served at the HTTP root, and enforcing validation and throttling. The changeset matches both. The title also includes the… Linked Issues check The PR removes the compiled-backend static mount from backend/src/main.tsand registersValidationPipeglobally withwhitelist: trueandtransform: true[#123,#38]. It removes the redundant c…Out of Scope Changes check The reviewed changes stay within the linked security fixes. The Docker healthcheck supports #123. Controller import and decorator removal supports global validation. The new guard and its registration…Full details: Title check
-
Explanation
The title names the two main changes: stopping the compiled backend from being served at the HTTP root, and enforcing validation and throttling. The changeset matches both. The title also includes the 'fix(security)' prefix. Its wording is long, but it is clear and specific. The changes match the title: the static-file mount is removed from main.ts, a global ValidationPipe is added, per-controller pipes are removed, and a GlobalThrottlerGuard is registered in app.module.ts. The Docker healthcheck change is not named in the title, but that is a minor detail.
Full details: Linked Issues check
-
Explanation
The PR removes the compiled-backend static mount from
backend/src/main.tsand registersValidationPipeglobally withwhitelist: trueandtransform: true[#123,#38]. It removes the redundant controller-level pipes [#38]. It registersGlobalThrottlerGuardasAPP_GUARD, skips non-HTTP contexts, and avoids duplicate throttling for routes with aThrottlerGuardsubclass [#39]. The Docker healthcheck change topgrep nodeaddresses the related healthcheck defect in#123. The reported build passes, and the reported Jest load failures reproduce on the base branch.
Full details: Out of Scope Changes check
-
Explanation
The reviewed changes stay within the linked security fixes. The Docker healthcheck supports
#123. Controller import and decorator removal supports global validation. The new guard and its registration support#39. The reported tests and build verification support these changes. No unrelated change is established.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 5 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the server at dawn,
The old process search is gone.
Pipes now greet each request in line,
A guard checks limits at the sign.
No compiled files greet the breeze,
The rabbit nibbles carrots with ease.
9cb3d8e to
2372c71
Compare
Fixes three verified backend security issues, plus a broken Docker healthcheck, in two commits.
1. Stop serving the compiled backend at the HTTP root (#123)
What: Removed
app.use(express.static(path.join(__dirname)))frombackend/src/main.ts(and the now-unusedexpress/pathimports).Why: At runtime
__dirnameis the compiled output directory, so the entire compiled backend was mounted as static files at the server root — any unauthenticated client could fetchGET /main.js,GET /app.module.js, etc. Becausetsconfig.jsonsetssourceMap: true, the matching.js.mapfiles effectively exposed the original TypeScript source as well. Nothing in the repo relied on this static mount (the frontend is a separate Next.js app).2. Register a global ValidationPipe (#38)
What: Added
app.useGlobalPipes(new ValidationPipe({ whitelist: true, transform: true }))inbackend/src/main.ts, and removed the now-redundant per-controller@UsePipes(new ValidationPipe({ transform: true, whitelist: true }))from themeetings,tasks,subtasksandtagscontrollers (they used the exact same options, so behavior there is unchanged).Why: Previously only those 4 controllers validated input; the other controllers (
auth,users,api-keys,communities,clubs,activity-feed,notifications,notification-preferences,moderation,gamification) accepted unvalidated bodies and their DTO decorators were ignored. All@Body()parameters across controllers are typed DTOs, so the global pipe now validates everywhere.Note on
forbidNonWhitelisted: deliberately left off, following the recommendation in #38.whitelist: truealready strips unknown properties (closing the mass-assignment hole), whereasforbidNonWhitelisted: truewould turn previously-accepted extra fields into hard 400s for existing clients that were never tested against validation. It can be enabled later as a follow-up once clients are known-clean.3. Enforce the configured rate limiting (#39)
What: Added
backend/src/shared/guards/global-throttler.guard.tsand registered it asAPP_GUARDinbackend/src/app.module.ts, so the existingThrottlerModule.forRootconfig (short/medium/long limits) is actually enforced on every HTTP route.Why: The throttler was configured but no global guard existed, so most endpoints — including auth — had no rate limiting.
Avoiding double-throttling: the
clubs,meetings,notificationsandtaskscontrollers already applyApiKeyThrottlerGuardvia@UseGuards. That guard must stay controller-level because it runs after the auth guards and keys limits on the resolved API key (req.user.apiKeyId); a global guard runs before auth and would only ever see the IP.GlobalThrottlerGuardtherefore skips any route whose class/handler already applies aThrottlerGuardsubclass, so those routes keep their existing behavior and requests are not counted twice against the same throttler storage. Non-HTTP contexts (the notifications WebSocket gateway) are also skipped, sinceThrottlerGuardonly understands HTTP requests.4. Fix the Docker healthcheck (separate commit)
What:
backend/Dockerfilehealthcheck changed fromps aux | grep node || exit 1topgrep node || exit 1.Why:
grep nodealways matches its own process in thepsoutput, so the healthcheck could never fail — a dead app still reported healthy.pgrep(available in Alpine busybox) matches only the node process and exits non-zero when it is gone. (The repo has no dedicated health endpoint, so a process check remains the simplest honest option.)Closes #123, Closes #38, Closes #39
Testing
npm ci && npm run buildinbackend/: passes.npm test: 42/42 tests pass. Two suites (community.entity.spec.ts,community-member.entity.spec.ts) fail to load withCannot find module 'src/shared/domain/base-entity'— a pre-existing jestmoduleNameMappergap that reproduces identically onmain(verified by stashing these changes and re-running).npm run test:e2e) were not run: they require live Postgres/Redis, which are not available in this environment.@Body()parameters are decorated DTO classes, so the global pipe does not break DTO-less endpoints (GET /apitakes no body).Summary by CodeRabbit