Skip to content

Harden backend: CSRF, rate limit, validation, observability#289

Merged
hackerwins merged 15 commits into
mainfrom
backend-hardening-quickwins
May 24, 2026
Merged

Harden backend: CSRF, rate limit, validation, observability#289
hackerwins merged 15 commits into
mainfrom
backend-hardening-quickwins

Conversation

@hackerwins

@hackerwins hackerwins commented May 24, 2026

Copy link
Copy Markdown
Collaborator

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.

  • CSRF defense — Auth cookies switch from SameSite=None to Lax across all environments. Cookie set/clear options unified. Three regression specs guard the policy.
  • Rate limiting@nestjs/throttler registered as APP_GUARD. Single default bucket (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 (default 0) gates Express trust proxy to prevent X-Forwarded-For spoofing 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.
  • Request validation — Global ValidationPipe({ whitelist, forbidNonWhitelisted, transform }) in main.ts. DTO classes with class-validator decorators across workspace, document, api-key, auth (cliExchange), and datasource controllers. The workspace invite role enum ('member' | 'owner') closes a privilege-smuggling vector. New applyGlobalBootstrap(app) helper wired into all e2e specs so integration coverage matches production.
  • Observabilitynestjs-pino as the global logger with auth-header redaction and dev pretty-print. GET /health (liveness) and GET /health/ready (Prisma SELECT 1) routes added with @SkipThrottle(); readiness logs full errors via Pino, exposes only a coarse status to anonymous callers.

docs/design/backend.md updated 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/throttler named-bucket stacking, undecorated DTOs + global pipe, e2e specs silently bypassing main.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:fast green at every commit
  • Backend unit suites — 18 suites / 175 tests including the new auth.controller.spec SameSite cases, throttler.spec (mirrored config + regression guard), workspace.dto.spec, datasource.dto.spec, health.controller.spec
  • pnpm verify:self green (pre-push: builds, chunks, entropy)
  • CI integration lane (verify:integration w/ Postgres + Yorkie) — runs in CI; e2e specs now apply the same ValidationPipe as production
  • Manual smoke: GitHub login → frontend session works; /health returns 200; /health/ready 503s when Postgres is down; invalid role in invite payload returns 400; 11th request to /auth/refresh returns 429

Summary by CodeRabbit

  • New Features

    • Added health check endpoints for monitoring system status and database connectivity.
    • Implemented request rate limiting on image and authentication endpoints.
  • Improvements

    • Enhanced request validation for API inputs across multiple endpoints.
    • Updated backend security documentation regarding cookie handling and rate limiting policies.
  • Documentation

    • Added production hardening lessons and quick-wins guidance for backend operations.

Review Change Stack

hackerwins and others added 9 commits May 24, 2026 20:42
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]>
@coderabbitai

coderabbitai Bot commented May 24, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@hackerwins, we couldn't start this review because you've used your available PR reviews for now.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5585ada6-6e48-45c8-9e84-0469f5a1dc8d

📥 Commits

Reviewing files that changed from the base of the PR and between 7ed1f97 and 85b5978.

📒 Files selected for processing (2)
  • docs/design/backend.md
  • packages/backend/src/app.module.ts
📝 Walkthrough

Walkthrough

This PR implements four backend hardening features: updating auth cookies to use SameSite=lax consistently, adding request rate limiting via @nestjs/throttler, enabling global input validation using class-validator DTOs across controllers, and adding observability with structured nestjs-pino logging and health check endpoints. All changes are documented in design and task files.

Changes

Backend Hardening Quick Wins

Layer / File(s) Summary
Cookie SameSite Security Fix
packages/backend/src/auth/auth.controller.ts, packages/backend/src/auth/auth.controller.spec.ts
Auth cookies now use sameSite: 'lax' in all environments, removing the prior production sameSite: none behavior. Tests verify the policy is enforced across NODE_ENV values and never set to none.
Global Rate Limiting Infrastructure
packages/backend/package.json, packages/backend/src/app.module.ts, packages/backend/src/throttler.spec.ts
@nestjs/throttler is wired as a global guard with a default bucket and skipIf configuration to disable throttling during test execution. Integration tests verify bucket enforcement, per-route overrides, and test-mode bypass behavior.
Rate Limiting Decorators Applied to Controllers
packages/backend/src/auth/auth.controller.ts, packages/backend/src/api/v1/images.controller.ts, packages/backend/src/image/image.controller.ts
Auth endpoints receive stricter 10 req/min limits; image endpoints receive higher limits; health endpoints are exempted via @SkipThrottle().
Input Validation DTO Definitions
packages/backend/src/api-key/api-key.dto.ts, packages/backend/src/auth/auth.dto.ts, packages/backend/src/datasource/datasource.dto.ts, packages/backend/src/document/document.dto.ts, packages/backend/src/workspace/workspace.dto.ts
New DTO classes with class-validator decorators enforce type constraints, length bounds, allowed values, UUID validation, and custom error messages across API key, auth, datasource, document, and workspace entities.
Controller Updates to Use Typed DTOs
packages/backend/src/api-key/api-key.controller.ts, packages/backend/src/auth/auth.controller.ts, packages/backend/src/datasource/datasource.controller.ts, packages/backend/src/document/document.controller.ts
Controllers now accept typed DTO request bodies instead of inline type annotations, removing manual validation logic. Document type defaulting changes from allowlist to nullish coalescing.
DTO Validation Test Suites
packages/backend/src/datasource/datasource.dto.spec.ts, packages/backend/src/workspace/workspace.dto.spec.ts
Jest specs verify validation rules: accepted payloads, length/range constraints, type enforcement, allowed value lists, pattern matching, and rejection of non-whitelisted properties.
Global ValidationPipe Configuration
packages/backend/src/main.ts, packages/backend/test/helpers/integration-helpers.ts, packages/backend/test/*
Main.ts enables global ValidationPipe with stricter whitelist behavior and disables implicit conversion. Integration helpers provide applyGlobalBootstrap to ensure e2e tests exercise the same validation contract as production.
Structured Logging with nestjs-pino
packages/backend/package.json, packages/backend/src/main.ts, packages/backend/src/app.module.ts
nestjs-pino and pino-http are added as dependencies. Main.ts wires buffered logs and injects the Logger. App.module configures LoggerModule with redaction for sensitive headers/cookies, environment-based log levels, and conditional pretty-printing in non-production.
Health Check Endpoints
packages/backend/src/health/health.controller.ts, packages/backend/src/health/health.controller.spec.ts, packages/backend/src/health/health.module.ts
New HealthController provides GET /health (liveness, no DB call) and GET /health/ready (readiness with Prisma raw query). Comprehensive tests cover success and failure paths including ServiceUnavailableException on DB failure.
Express Trust Proxy and Body Limit Configuration
packages/backend/src/main.ts
Main.ts conditionally sets Express "trust proxy" based on BACKEND_TRUST_PROXY environment variable for correct IP forwarding behind proxies, and configures JSON body size limit.
Design Documentation and Task Records
docs/design/backend.md, docs/tasks/README.md, docs/tasks/archive/2026/05/20260524-backend-hardening-quickwins-*.md, docs/tasks/archive/README.md
Backend design doc is updated with new SameSite=lax policy, expanded observability section, and rate limiting guidance. Task docs capture the hardening plan and lessons learned. Archive counts are incremented.
Environment Variable Documentation
packages/backend/README.md
Backend README adds documentation for optional environment variables: LOG_LEVEL, BACKEND_TRUST_PROXY, and BACKEND_JSON_BODY_LIMIT.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

  • wafflebase/wafflebase#34: Updates auth cookie behavior and CLI OAuth flow endpoints that overlap with this PR's cookie SameSite changes and auth controller updates.
  • wafflebase/wafflebase#72: Introduces document type support via DocumentController and document.dto.ts, overlapping with this PR's document DTO validation layer.
  • wafflebase/wafflebase#28: Implements API key REST endpoints using ApiKeyController, directly overlapping with this PR's API key DTO validation work.

🐇 A hardened backend hops with glee,
Cookies lax, throttles set to decree,
Validation guards each request's way,
Health checks bounce through the day,
Security's strong from the ground to the tree!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Harden backend: CSRF, rate limit, validation, observability' accurately reflects the four main changes in the PR: cookie SameSite hardening (CSRF), rate limiting via @nestjs/throttler, request validation with DTOs, and structured logging with health endpoints.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch backend-hardening-quickwins

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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]>
@github-actions

github-actions Bot commented May 24, 2026

Copy link
Copy Markdown
Contributor

Verification: verify:self

Result: ✅ PASS in 225.8s

Lane Status Duration
sheets:build ✅ pass 14.5s
docs:build ✅ pass 13.0s
slides:build ✅ pass 14.4s
verify:fast ✅ pass 135.3s
frontend:build ✅ pass 19.0s
verify:frontend:chunks ✅ pass 0.3s
backend:build ✅ pass 5.1s
cli:build ✅ pass 2.2s
verify:entropy ✅ pass 21.9s

Verification: verify:integration

Result: ✅ PASS

hackerwins and others added 3 commits May 24, 2026 21:13
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]>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/backend/test/helpers/integration-helpers.ts (1)

70-84: ⚡ Quick win

Deduplicate ValidationPipe config shared with production bootstrap.

Line 71 already calls out drift risk; today the same options are manually duplicated in main.ts and 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 value

Consider 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:

  • $queryRaw returning an empty array
  • $queryRaw returning null or undefined
  • $queryRaw returning malformed data

These 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

📥 Commits

Reviewing files that changed from the base of the PR and between 98eae34 and 7ed1f97.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (32)
  • docs/design/backend.md
  • docs/tasks/README.md
  • docs/tasks/archive/2026/05/20260524-backend-hardening-quickwins-lessons.md
  • docs/tasks/archive/2026/05/20260524-backend-hardening-quickwins-todo.md
  • docs/tasks/archive/README.md
  • packages/backend/README.md
  • packages/backend/package.json
  • packages/backend/src/api-key/api-key.controller.ts
  • packages/backend/src/api-key/api-key.dto.ts
  • packages/backend/src/api/v1/images.controller.ts
  • packages/backend/src/app.module.ts
  • packages/backend/src/auth/auth.controller.spec.ts
  • packages/backend/src/auth/auth.controller.ts
  • packages/backend/src/auth/auth.dto.ts
  • packages/backend/src/datasource/datasource.controller.ts
  • packages/backend/src/datasource/datasource.dto.spec.ts
  • packages/backend/src/datasource/datasource.dto.ts
  • packages/backend/src/document/document.controller.ts
  • packages/backend/src/document/document.dto.ts
  • packages/backend/src/health/health.controller.spec.ts
  • packages/backend/src/health/health.controller.ts
  • packages/backend/src/health/health.module.ts
  • packages/backend/src/image/image.controller.ts
  • packages/backend/src/main.ts
  • packages/backend/src/throttler.spec.ts
  • packages/backend/src/workspace/workspace.dto.spec.ts
  • packages/backend/src/workspace/workspace.dto.ts
  • packages/backend/test/api-key-http.e2e-spec.ts
  • packages/backend/test/authenticated-http.e2e-spec.ts
  • packages/backend/test/docs-cli-roundtrip.e2e-spec.ts
  • packages/backend/test/helpers/integration-helpers.ts
  • packages/backend/test/slides-pptx-import.e2e-spec.ts

Comment on lines +241 to +243
afterEach(() => {
process.env.NODE_ENV = originalNodeEnv;
});

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 | 🟡 Minor | ⚡ Quick win

🧩 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(() =&gt; {
-      process.env.NODE_ENV = originalNodeEnv;
-    });
+    afterEach(() =&gt; {
+      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.

hackerwins and others added 2 commits May 24, 2026 21:27
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]>
@hackerwins
hackerwins merged commit a4c3341 into main May 24, 2026
1 check passed
@hackerwins
hackerwins deleted the backend-hardening-quickwins branch May 24, 2026 12:36
@hackerwins hackerwins mentioned this pull request May 24, 2026
2 tasks
hackerwins added a commit that referenced this pull request May 24, 2026
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]>
hackerwins added a commit that referenced this pull request May 24, 2026
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant