Harden backend: CSRF, rate limit, validation, observability#289
Conversation
Cookies were set with SameSite=None in production, letting any cross-origin site ride the user's session. Lax blocks third-party requests while still sending cookies on the OAuth callback top-level redirect and same-eTLD+1 XHR. Adds three regression tests (production, non-production, never None) so the policy can't silently revert. Assumes frontend and backend share an eTLD+1; cross-eTLD deployment would need a paired CSRF token instead. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Login, refresh, CLI exchange, and every JSON endpoint were unprotected against brute force and scraping. Registers ThrottlerModule with two buckets — default (60/min/IP) and auth (10/min/IP) — wired as an APP_GUARD. Auth routes opt into the stricter bucket via @Throttle. Sets Express trust proxy so req.ip resolves to the real client behind one upstream proxy. Skips the limiter under NODE_ENV=test so existing unit and e2e suites still blast through. New throttler.spec verifies both the 429 path and the test-env bypass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Controllers accepted raw inline-typed payloads, so the workspace invite endpoint would have stored any string a caller wrote into `role` (e.g. `superadmin`). Wires a global ValidationPipe with whitelist + forbidNonWhitelisted + transform, then promotes the highest-risk bodies to decorated DTOs: workspace create/update/invite, document create/update (workspace-scoped and legacy), and api-key create. The role enum closes the privilege-smuggling path; slug/name length caps stop oversized inputs; expiration regex matches the parser the service already enforces. Adds workspace.dto.spec covering accept and reject cases for each DTO. v1 controllers are intentionally deferred — their endpoints have more shape to bring under DTOs and warrant a separate pass. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Production incidents were invisible: no structured logger, no liveness/readiness signal for the orchestrator. Wires nestjs-pino as the global logger with NODE_ENV-aware transport (pretty in dev, raw JSON in prod), redacts auth headers and set-cookie, and gates autoLogging off under NODE_ENV=test so Jest output stays clean. Adds a HealthModule with `/health` (always-200 liveness) and `/health/ready` (Prisma SELECT 1, 503 on failure). Both routes opt out of throttling so an orchestrator's poll budget can't lock us out. backend.md gains observability + rate-limit + SameSite sections covering the policies introduced across this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Records the Review section with per-commit notes and the explicit follow-up list (v1 controller DTOs, share-link/datasource DTOs, Sentry, per-API-key throttling) so the next iteration has a starting point. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Two critical bugs the original commits introduced, both confirmed by
isolated reproductions:
1. The named `auth: 10/min` throttler stacked across every route, so
any authenticated user would hit 429 after ten requests in a
minute. Collapsed to a single `default` bucket; auth routes
override per-route via `@Throttle({ default: ... })`. The
throttler spec now mirrors the production config and adds a
regression guard that fails red if two named buckets are
reintroduced.
2. DataSourceController DTO classes were declared without
class-validator decorators, so the global ValidationPipe would
return 400 on every datasource write (`unknownValue` + whitelist
errors). The four datasource DTOs now carry full decorators;
`datasource.dto.spec` covers accept/reject cases and pins the
"no undecorated DTO" invariant.
Plus four follow-ups the reviewer surfaced:
- E2E specs were silently bypassing the global pipe. Added an
`applyGlobalBootstrap(app)` helper in `integration-helpers` and
wired it into all four e2e spec files so the integration lane now
catches drift from main.ts.
- `/health/ready` no longer echoes the raw Prisma error to anonymous
callers — Pino logs the detail, the response only says
`database: 'unreachable'`.
- `trust proxy` is now gated on `BACKEND_TRUST_PROXY` (default 0).
Enabling it without an upstream proxy that strips client-supplied
`X-Forwarded-For` would let a client spoof IPs and bypass per-IP
limits.
- `cliExchange` body promoted to `CliExchangeDto`. Stale Security
paragraphs in `backend.md` (claiming SameSite=None in prod and "no
rate limiting") rewritten to match the new behavior. Env table and
package README pick up `LOG_LEVEL`, `BACKEND_TRUST_PROXY`, and
`BACKEND_JSON_BODY_LIMIT`.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Captures three sharp gotchas the reviewer surfaced so future-me doesn't hit them again: @nestjs/throttler named buckets stack across every route (the @Throttle decorator overrides, not opt-ins); undecorated DTO classes 400 every request under the global pipe; e2e specs that build their own Nest app silently bypass main.ts's bootstrap layer. Plus two meta-lessons — write the reviewer's repro before pushing back, and verify:fast does not exercise the bootstrap pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Picks up this branch's todo + lessons entries that were resolved in favour of origin/main during the rebase. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
verify:entropy resolves file references mentioned in design docs and fails when it can't find them. The new observability/rate-limit sections referenced `app.module.ts`, `main.ts`, and `src/health/health.controller.ts` without their package prefix, which broke the check. Switched to the existing convention of `packages/backend/src/...` used elsewhere in the same doc. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 40 minutes and 7 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements four backend hardening features: updating auth cookies to use ChangesBackend Hardening Quick Wins
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Move todo + lessons to docs/tasks/archive/2026/05/ now that PR #289 is open. Both indices regenerated. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Verification: verify:selfResult: ✅ PASS in 225.8s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Manual smoke against pnpm dev showed the previous 60 req/min default cutting off image traffic mid-burst: a single docs/slides document open uploads tens of images, then re-fetches them, easily blowing past the cap and leaving image-shaped holes in the rendered page. Logs captured ~80 POST /images followed by a wave of 429s. Lifts global default to 120 req/min and gives both image controllers (ImageController and ApiV1ImagesController) a 600 req/min controller- level @Throttle override. Image traffic is authenticated and bounded by the file size limit, so the higher ceiling is safe. Doc table updated; the regression spec already mirrors prod topology and stays valid (number values are intentionally smaller for fast tests). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The default pino-http auto-logging dumps every request header (sec-ch-ua-*, accept-encoding, if-none-match, etc.) and the full response headers on a single line per request. A heavy docs open (~80 image fetches) emits ~160KB of logs — most of it Chrome client-hint noise that bills against the log aggregator without helping on-call. Adds three knobs: - autoLogging.ignore skips /health and /health/ready so orchestrator probes don't dominate the stream. - customLogLevel pages on 5xx, warns on 4xx, drops 304 to debug. - Custom req/res serializers strip the access log down to method, url, remoteAddress, userAgent, statusCode (plus responseTime that pino-http already adds). Full headers stay reachable at debug. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Even the slim serializers still emit one info line per request. At
production traffic the access log is noise: read-only GETs and image
fetches dominate the stream, and on-call ends up scrolling past
thousands of routine 200s to find anything actionable.
Reframes the policy: the access log is not the audit trail. Default
auto-logging drops to debug — silent under LOG_LEVEL=info. Only
DELETE stays at info so destructive operations still get a baseline
record without service-layer changes. 4xx (warn) and 5xx (error)
remain visible. Operators can flip LOG_LEVEL=debug temporarily for
incident response.
Important business events (document.create, login success,
datasource.test, etc.) should be emitted explicitly from service
code via Logger.log({ event, ... }) — that's a separate follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/backend/test/helpers/integration-helpers.ts (1)
70-84: ⚡ Quick winDeduplicate ValidationPipe config shared with production bootstrap.
Line 71 already calls out drift risk; today the same options are manually duplicated in
main.tsand here. Extract one shared options object/factory and consume it in both places.♻️ Proposed refactor
diff --git a/packages/backend/test/helpers/integration-helpers.ts b/packages/backend/test/helpers/integration-helpers.ts @@ import { INestApplication, ValidationPipe } from '`@nestjs/common`'; +import { globalValidationPipeOptions } from 'src/bootstrap/global-validation-pipe-options'; @@ export function applyGlobalBootstrap(app: INestApplication) { - app.useGlobalPipes( - new ValidationPipe({ - whitelist: true, - forbidNonWhitelisted: true, - transform: true, - transformOptions: { enableImplicitConversion: false }, - }), - ); + app.useGlobalPipes(new ValidationPipe(globalValidationPipeOptions)); }// packages/backend/src/bootstrap/global-validation-pipe-options.ts import { ValidationPipeOptions } from '`@nestjs/common`'; export const globalValidationPipeOptions: ValidationPipeOptions = { whitelist: true, forbidNonWhitelisted: true, transform: true, transformOptions: { enableImplicitConversion: false }, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/test/helpers/integration-helpers.ts` around lines 70 - 84, The ValidationPipe options used inside applyGlobalBootstrap are duplicated with production bootstrap; extract the shared options into a single exported symbol (e.g., export const globalValidationPipeOptions or a factory getGlobalValidationPipeOptions) and update applyGlobalBootstrap to import that symbol and pass it to new ValidationPipe(...); likewise update main.ts to import and reuse the same exported options (or construct the pipe via a shared factory) so both production bootstrap and the test helper use the identical ValidationPipe configuration.packages/backend/src/health/health.controller.spec.ts (1)
11-42: 💤 Low valueConsider adding edge case tests for unexpected database responses.
The current test coverage is solid for the main paths (liveness, readiness success, and DB failure). For more comprehensive coverage, consider adding tests for edge cases like:
$queryRawreturning an empty array$queryRawreturningnullorundefined$queryRawreturning malformed dataThese would further harden the readiness check, though the current coverage is good for the primary scenarios.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/backend/src/health/health.controller.spec.ts` around lines 11 - 42, Add unit tests in health.controller.spec.ts that exercise HealthController.readiness for unexpected DB responses: create Prisma mocks via createPrismaMock that return an empty array, null/undefined, and malformed rows (e.g., missing expected ok field), then assert whether readiness resolves to the expected shape or rejects with ServiceUnavailableException as appropriate; also assert prisma.$queryRaw was called in those readiness tests and keep the existing liveness test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/src/auth/auth.controller.spec.ts`:
- Around line 241-243: The afterEach teardown currently assigns
process.env.NODE_ENV = originalNodeEnv which turns undefined into the string
"undefined"; change the restoration logic in the afterEach block (where
originalNodeEnv is captured) to check if originalNodeEnv === undefined and if so
delete process.env.NODE_ENV, otherwise set process.env.NODE_ENV =
originalNodeEnv so the environment key is restored safely.
---
Nitpick comments:
In `@packages/backend/src/health/health.controller.spec.ts`:
- Around line 11-42: Add unit tests in health.controller.spec.ts that exercise
HealthController.readiness for unexpected DB responses: create Prisma mocks via
createPrismaMock that return an empty array, null/undefined, and malformed rows
(e.g., missing expected ok field), then assert whether readiness resolves to the
expected shape or rejects with ServiceUnavailableException as appropriate; also
assert prisma.$queryRaw was called in those readiness tests and keep the
existing liveness test unchanged.
In `@packages/backend/test/helpers/integration-helpers.ts`:
- Around line 70-84: The ValidationPipe options used inside applyGlobalBootstrap
are duplicated with production bootstrap; extract the shared options into a
single exported symbol (e.g., export const globalValidationPipeOptions or a
factory getGlobalValidationPipeOptions) and update applyGlobalBootstrap to
import that symbol and pass it to new ValidationPipe(...); likewise update
main.ts to import and reuse the same exported options (or construct the pipe via
a shared factory) so both production bootstrap and the test helper use the
identical ValidationPipe configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d84c47e7-4e40-4a16-aed4-7631dcc1bedb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
docs/design/backend.mddocs/tasks/README.mddocs/tasks/archive/2026/05/20260524-backend-hardening-quickwins-lessons.mddocs/tasks/archive/2026/05/20260524-backend-hardening-quickwins-todo.mddocs/tasks/archive/README.mdpackages/backend/README.mdpackages/backend/package.jsonpackages/backend/src/api-key/api-key.controller.tspackages/backend/src/api-key/api-key.dto.tspackages/backend/src/api/v1/images.controller.tspackages/backend/src/app.module.tspackages/backend/src/auth/auth.controller.spec.tspackages/backend/src/auth/auth.controller.tspackages/backend/src/auth/auth.dto.tspackages/backend/src/datasource/datasource.controller.tspackages/backend/src/datasource/datasource.dto.spec.tspackages/backend/src/datasource/datasource.dto.tspackages/backend/src/document/document.controller.tspackages/backend/src/document/document.dto.tspackages/backend/src/health/health.controller.spec.tspackages/backend/src/health/health.controller.tspackages/backend/src/health/health.module.tspackages/backend/src/image/image.controller.tspackages/backend/src/main.tspackages/backend/src/throttler.spec.tspackages/backend/src/workspace/workspace.dto.spec.tspackages/backend/src/workspace/workspace.dto.tspackages/backend/test/api-key-http.e2e-spec.tspackages/backend/test/authenticated-http.e2e-spec.tspackages/backend/test/docs-cli-roundtrip.e2e-spec.tspackages/backend/test/helpers/integration-helpers.tspackages/backend/test/slides-pptx-import.e2e-spec.ts
| afterEach(() => { | ||
| process.env.NODE_ENV = originalNodeEnv; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, what happens if you execute process.env.NODE_ENV = undefined? Does it unset the variable or store the literal string "undefined"?
💡 Result:
In Node.js, process.env only stores strings. When you do:
process.env.NODE_ENV = undefined;Node coerces the value to a string, so process.env.NODE_ENV becomes the literal string "undefined", not “unset”. This is documented behavior: “Assigning a property on process.env will implicitly convert the value to a string.” [1][2]
To actually remove/unset it, use:
delete process.env.NODE_ENV;Sources: Node.js process docs [1][2]
[1] https://nodejs.org/api/process.html
[2] https://nodejs.org/download/release/v20.18.2/docs/api/process.html
Restore NODE_ENV safely to avoid cross-test contamination.
If originalNodeEnv is undefined, process.env coerces the assignment to the literal string "undefined"; delete the key in that case.
Suggested fix
- afterEach(() => {
- process.env.NODE_ENV = originalNodeEnv;
- });
+ afterEach(() => {
+ if (originalNodeEnv === undefined) {
+ delete process.env.NODE_ENV;
+ } else {
+ process.env.NODE_ENV = originalNodeEnv;
+ }
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/src/auth/auth.controller.spec.ts` around lines 241 - 243,
The afterEach teardown currently assigns process.env.NODE_ENV = originalNodeEnv
which turns undefined into the string "undefined"; change the restoration logic
in the afterEach block (where originalNodeEnv is captured) to check if
originalNodeEnv === undefined and if so delete process.env.NODE_ENV, otherwise
set process.env.NODE_ENV = originalNodeEnv so the environment key is restored
safely.
Previous commit dropped everything but DELETE to debug, but content imports (PUT /api/v1/.../content — the DOCX/PPTX bulk write endpoint the CLI hits) are audit-worthy bulk operations and should not vanish into debug. Adds a second info case for that URL+method combination. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Previous "only DELETE + PUT /content at info" missed POST /documents (new doc create) and a host of other audit-worthy mutations. The suffix-allowlist approach was whack-a-mole. Inverts the rule: every mutation (POST/PUT/PATCH/DELETE) gets info so the audit trail covers doc create, import, share-link, invite, api-key, datasource, etc. without per-endpoint instrumentation. Two high-volume paths drop to debug regardless of method — `/images/*` (upload/fetch bursts on doc open) and `/cells/*` (cell-level CRUD via CLI). Reads stay at debug. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Patch release covering 25 commits since v0.4.1. The headline theme is infrastructure & quality: backend hardening (CSRF, rate limiting, request validation, structured logging), a new @wafflebase/tokens shared design-tokens package, a frontend test migration to Vitest, a repaired and CI-wired frontend Yorkie integration lane, and a dependency sweep clearing 22 Dependabot alerts. Product work: Slides rulers + guides, group/ungroup, connector hit-test and drag fixes, ghost-preview drag-move, a mobile editor shell, and PPTX shape-level blipFill import; Sheets gained power(^) and unary postfix percentage operators plus an IFERROR/IFNA fix. No Prisma migration. #289 adds three optional backend env vars (BACKEND_TRUST_PROXY, BACKEND_JSON_BODY_LIMIT, LOG_LEVEL) with safe defaults, so the devops image bump stays routine. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
Patch release covering 25 commits since v0.4.1. The headline theme is infrastructure & quality: backend hardening (CSRF, rate limiting, request validation, structured logging), a new @wafflebase/tokens shared design-tokens package, a frontend test migration to Vitest, a repaired and CI-wired frontend Yorkie integration lane, and a dependency sweep clearing 22 Dependabot alerts. Product work: Slides rulers + guides, group/ungroup, connector hit-test and drag fixes, ghost-preview drag-move, a mobile editor shell, and PPTX shape-level blipFill import; Sheets gained power(^) and unary postfix percentage operators plus an IFERROR/IFNA fix. No Prisma migration. #289 adds three optional backend env vars (BACKEND_TRUST_PROXY, BACKEND_JSON_BODY_LIMIT, LOG_LEVEL) with safe defaults, so the devops image bump stays routine. Also move @wafflebase/tokens from dependencies to devDependencies in @wafflebase/docs. tokens is private (never published to npm) and is bundled into the docs dist by the Vite library build, so declaring it as a runtime dependency made the published @wafflebase/[email protected] manifest reference a package that does not exist on the registry, breaking the npm publish. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Summary
Backend hardening pass off the CTO review (
docs/tasks/active/20260524-backend-hardening-quickwins-todo.md). Four high-ROI security/ops fixes plus the issues a self-review surfaced before push.SameSite=NonetoLaxacross all environments. Cookie set/clear options unified. Three regression specs guard the policy.@nestjs/throttlerregistered asAPP_GUARD. Singledefaultbucket (60 req / 60s / IP) with per-route@Throttle({ default: { limit: 10, ttl: 60_000 } })on/auth/github/callback,/auth/refresh,/auth/cli/exchange.BACKEND_TRUST_PROXY(default0) gates Expresstrust proxyto preventX-Forwarded-Forspoofing when no real proxy is in front.skipIf: NODE_ENV==='test'keeps the limiter off Jest. Regression spec mirrors the production config and pins the "two named buckets stack across every route" gotcha.ValidationPipe({ whitelist, forbidNonWhitelisted, transform })inmain.ts. DTO classes withclass-validatordecorators across workspace, document, api-key, auth (cliExchange), and datasource controllers. The workspace invite role enum ('member' | 'owner') closes a privilege-smuggling vector. NewapplyGlobalBootstrap(app)helper wired into all e2e specs so integration coverage matches production.nestjs-pinoas the global logger with auth-header redaction and dev pretty-print.GET /health(liveness) andGET /health/ready(PrismaSELECT 1) routes added with@SkipThrottle(); readiness logs full errors via Pino, exposes only a coarse status to anonymous callers.docs/design/backend.mdupdated with new SameSite, observability, rate-limit, and env-var sections. Stale "no rate limiting" / "SameSite=None in production" paragraphs rewritten. Lessons file captures the three production traps (@nestjs/throttlernamed-bucket stacking, undecorated DTOs + global pipe, e2e specs silently bypassingmain.ts).Out of scope: Sentry/external error tracker, v1 controller DTOs, share-link/datasource controller DTO sweep, per-API-key throttling, Jest→Vitest migration, CSRF token (only needed if SameSite=None ever returns).
Test plan
pnpm verify:fastgreen at every commitauth.controller.specSameSite cases,throttler.spec(mirrored config + regression guard),workspace.dto.spec,datasource.dto.spec,health.controller.specpnpm verify:selfgreen (pre-push: builds, chunks, entropy)verify:integrationw/ Postgres + Yorkie) — runs in CI; e2e specs now apply the sameValidationPipeas production/healthreturns 200;/health/ready503s when Postgres is down; invalidrolein invite payload returns 400; 11th request to/auth/refreshreturns 429Summary by CodeRabbit
New Features
Improvements
Documentation