feat: implement nonce-based CSP middleware for enhanced security#171
Merged
Conversation
- Added nonce-based Content Security Policy (CSP) middleware to replace 'unsafe-inline' directive for script-src. - Generates a unique nonce per request and injects it into the CSP header. - Skips nonce generation for API endpoints, Nuxt assets, and static files. feat: add pgDumpEnv utility to secure environment variable handling - Introduced pgDumpEnv utility to build a sanitized environment for pg_dump. - Only allows a minimal set of system variables and PGPASSWORD to prevent leaking sensitive application secrets. test: add unit tests for pgDumpEnv utility - Created comprehensive tests for pgDumpEnv to ensure application secrets are not leaked. - Validated that only allowed environment variables are forwarded to child processes. fix: enhance rate limiting logic and add tests - Updated rate limiting implementation to warn about in-memory state across replicas. - Added unit tests to verify rate limiting behavior, including request limits and header emissions. test: add security tests for recent fixes - Implemented tests for various security fixes including nonce-based CSP, HKDF key separation, and rate limiting enforcement in CI environments. - Ensured that all security measures are validated and functioning as intended. Co-authored-by: Copilot <copilot@github.com>
📝 WalkthroughWalkthroughThis PR applies security hardening and hygiene: nonce-based per-request CSP, HKDF AES key derivation with legacy fallback, Zod router-param validation across many endpoints, a sanitized env allowlist for pg_dump, removal of CI bypass for production rate-limiting, and runtime warnings about in-memory rate limiter across replicas. ChangesSecurity Hardening & Parameter Validation
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Server as Server (Nitro)
participant Middleware as CSP Middleware
participant Plugin as Nitro CSP Plugin
participant HTML as Rendered HTML
Client->>Server: GET page
Server->>Middleware: invoke event handler
Middleware->>Server: generate nonce, store in event.context.nonce
Server->>Plugin: render:html hook (reads event.context.nonce)
Plugin->>HTML: inject nonce attributes into inline/blocked scripts
Server->>Client: respond with HTML (CSP header includes nonce)
Client->>HTML: executes inline script (allowed by nonce)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
🚅 Deployed to the reqcore-pr-171 environment in applirank
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) | ||
| try { | ||
| const count = 1 | ||
| if (count > 1) { |
| * Fix 5: Horizontal-scaling warning emitted when RAILWAY_REPLICA_COUNT > 1 | ||
| */ | ||
|
|
||
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' |
| // getAuthTag() must be called AFTER cipher.final() | ||
| const ciphertextBody = Buffer.concat([cipher.update('test', 'utf-8'), cipher.final()]) | ||
| const authTag = cipher.getAuthTag() | ||
| const legacyCiphertext = Buffer.concat([iv, authTag, ciphertextBody]) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/public/jobs/[slug]/apply.post.ts (1)
16-21:⚠️ Potential issue | 🟠 MajorE2E test workflow will exceed rate limit on apply endpoint.
The e2e test suite at
./e2e/critical-flows/runs withNODE_ENV=production(per.github/workflows/e2e-tests.ymlline 92), which means rate limiting (5 requests per 15 minutes) is enforced on the apply endpoint. However, the e2e tests make 10 total apply requests sequentially from the same IP:
- candidate-application.spec.ts: 1 request
- resume-upload.spec.ts: 8 requests (3 valid formats as resume + 2 invalid + 3 valid via custom question)
- source-tracking.spec.ts: 1 request
Requests 6–10 will fail with HTTP 429 responses. Either set
NODE_ENV=developmentfor test runs or configure rate limiting to bypass test environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/public/jobs/`[slug]/apply.post.ts around lines 16 - 21, The apply endpoint's rate limiter (applyRateLimit created via createRateLimiter) blocks E2E tests; update the rate limiter initialization to skip or relax limits when running tests by checking the environment (e.g., process.env.NODE_ENV === 'test' or a dedicated flag like process.env.E2E === 'true') and only apply createRateLimiter in non-test environments, or set a much higher maxRequests for test/e2e envs; modify the applyRateLimit definition so it conditionally returns a no-op/mocked middleware for test runs while keeping the existing 15min/5-request behavior for production.
🧹 Nitpick comments (12)
package.json (1)
91-91: Consider bounding theuuidmajor version for reproducibility.Line 91 uses
"uuid": ">=14.0.0", which permits any future major version (e.g.,15.0.0+). While uuid currently has no version beyond14.0.0, bounding the major version follows standard lockdown practices and prevents unexpected breaking changes if a future major is released. Suggested change:- "uuid": ">=14.0.0" + "uuid": ">=14.0.0 <15.0.0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 91, The dependency spec for "uuid" currently allows any future major ( "uuid": ">=14.0.0" ); change it to bound the major version for reproducible installs by restricting to the 14.x range (for example use a semver range like "uuid": ">=14.0.0 <15.0.0" or simply "^14.0.0") so updates are limited to non-breaking releases; update the "uuid" entry in package.json accordingly.server/api/candidates/[id]/documents/index.post.ts (1)
39-39: Replace deprecatedz.string().uuid()with Zod 4'sz.uuid()
z.string().uuid()is deprecated in Zod 4 in favour of the top-levelz.uuid(). The standalonez.uuid()also validates more strictly against RFC 9562/4122 (variant bits must be10);z.guid()is available if the permissive Zod 3 behaviour is needed.Since the codebase generates IDs with
crypto.randomUUID()(UUIDv4, RFC 4122-compliant), switching toz.uuid()is safe and tightens the validation.The same inline schema (
z.object({ id: z.string().uuid() }).parse) is duplicated across 14 route files. Extracting a shared constant avoids the deprecation warning and makes a future change a one-liner.♻️ Suggested fix — shared schema utility + Zod 4 API
Create a shared helper, e.g.
utils/validation/routeParams.ts:+import { z } from 'zod' + +/** Shared validator for route handlers that accept a single UUID `:id` segment. */ +export const routeIdSchema = z.object({ id: z.uuid() })Then in each affected route (replacing the inline expression):
-const { id: candidateId } = await getValidatedRouterParams(event, z.object({ id: z.string().uuid() }).parse) +const { id: candidateId } = await getValidatedRouterParams(event, routeIdSchema.parse)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/candidates/`[id]/documents/index.post.ts at line 39, Replace the deprecated z.string().uuid() inline schema with Zod 4's z.uuid() and centralize the schema: create a shared exported constant (e.g., ROUTE_ID_PARAM_SCHEMA = z.object({ id: z.uuid() })) in a validation utility and update calls that pass the inline parser (the getValidatedRouterParams call currently using z.object({ id: z.string().uuid() }).parse) to use ROUTE_ID_PARAM_SCHEMA.parse; update the same replacement across the ~14 route files that duplicate the inline schema so future changes are one-liners.server/api/documents/[id]/preview.get.ts (1)
26-26: ⚡ Quick winDeprecated
z.string().uuid()— replace withz.uuid()Same pattern as the other endpoint files. Replace
z.string().uuid()withz.uuid().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/documents/`[id]/preview.get.ts at line 26, The call that validates route params uses the deprecated schema z.string().uuid(); update the validator to use z.uuid() instead by changing the schema passed into getValidatedRouterParams (the line that assigns documentId via const { id: documentId } = await getValidatedRouterParams(event, z.string().uuid().parse) — replace z.string().uuid() with z.uuid()) so the route parameter validation uses z.uuid() for the id field.server/api/documents/[id].delete.ts (1)
21-21: ⚡ Quick winDeprecated
z.string().uuid()— replace withz.uuid()Same pattern as flagged in
reject.post.ts. Replacez.string().uuid()with the Zod 4 standalonez.uuid().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/documents/`[id].delete.ts at line 21, The code uses the deprecated z.string().uuid() when validating the route param; replace that call with the Zod 4 standalone validator z.uuid() in the getValidatedRouterParams invocation (so change the schema passed to getValidatedRouterParams to use z.uuid() for the id field while keeping the existing destructuring into documentId and the surrounding call site like getValidatedRouterParams(event, z.object({ id: z.uuid() }).parse).server/api/chatbot/folders/[id].patch.ts (1)
20-20: ⚡ Quick winSame deprecated
z.string().uuid()— replace withz.uuid()Same pattern as
server/api/join-requests/[id]/reject.post.ts. Switch to the sharedrouteIdSchemaonce extracted, or at minimum replace the inline call withz.object({ id: z.uuid() }).parse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/folders/`[id].patch.ts at line 20, The route parameter validation uses the deprecated pattern z.string().uuid() inside the call to getValidatedRouterParams; update the schema passed to getValidatedRouterParams to use z.uuid() (or replace with the shared routeIdSchema if available) so the line becomes getValidatedRouterParams(event, z.object({ id: z.uuid() }).parse) or uses routeIdSchema.parse; update references in the handler where the { id } is destructured to match the new schema.server/api/invite-links/[id].delete.ts (1)
13-13: ⚡ Quick winDeprecated
z.string().uuid()— replace withz.uuid()Same as the pattern flagged in
reject.post.ts. Replacez.string().uuid()withz.uuid()(and ideally pull in the sharedrouteIdSchema).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/invite-links/`[id].delete.ts at line 13, The router params validation uses deprecated z.string().uuid(); update the call that extracts linkId (const { id: linkId } = await getValidatedRouterParams(event, z.object({ id: z.string().uuid() }).parse)) to use z.uuid() instead—ideally replace the inline schema with the shared routeIdSchema (or import routeIdSchema and pass getValidatedRouterParams(event, routeIdSchema.parse)) so validation uses z.uuid() and reuses the common schema; ensure the variable name linkId and the call to getValidatedRouterParams remain unchanged.server/utils/rateLimit.ts (1)
13-20: ⚡ Quick winStartup-warning test exercises a copy of the logic, not the module itself
The test in
tests/unit/security-fixes.test.ts(lines 309–340) reproduces theif (count > 1)branch inline instead of importingrateLimit.ts, so any future change to the threshold condition or message in the actual module would not be caught.Module-level code is awkward to test because Node caches the module after the first import. The usual pattern to make it testable is to extract the check into an exported function:
♻️ Example extraction
-const _replicaCount = Number(process.env.RAILWAY_REPLICA_COUNT ?? 0) -if (_replicaCount > 1) { - console.warn(...) -} +export function warnIfHorizontallyScaled(env: NodeJS.ProcessEnv = process.env): void { + const replicaCount = Number(env.RAILWAY_REPLICA_COUNT ?? 0) + if (replicaCount > 1) { + console.warn( + `[rateLimit] WARNING: RAILWAY_REPLICA_COUNT=${replicaCount}. ` + + 'The in-memory rate limiter is NOT shared across replicas — effective limits are ' + + `${replicaCount}× higher than configured. Move rate limiting to the edge.`, + ) + } +} +warnIfHorizontallyScaled()The test can then call
warnIfHorizontallyScaled({ RAILWAY_REPLICA_COUNT: '3' })directly, verifying the real code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/rateLimit.ts` around lines 13 - 20, Extract the startup check out of the module-level code in rateLimit.ts into an exported function named warnIfHorizontallyScaled(env?: Record<string,string>) that computes replicaCount (currently derived from process.env.RAILWAY_REPLICA_COUNT/_replicaCount) and emits the same console.warn when count > 1; replace the inline if-block with a single call to warnIfHorizontallyScaled() so runtime behavior is unchanged but tests can import and call warnIfHorizontallyScaled({ RAILWAY_REPLICA_COUNT: '3' }) to exercise the branch.server/api/chatbot/agents/[id].patch.ts (1)
28-28: ⚡ Quick winDeprecated
z.string().uuid()— replace withz.uuid()Same pattern as the other endpoint files. Replace
z.string().uuid()withz.uuid().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/agents/`[id].patch.ts at line 28, Replace the deprecated schema usage in the router params: in the call to getValidatedRouterParams where you build the Zod schema (currently using z.object({ id: z.string().uuid() }).parse), switch to z.object({ id: z.uuid() }).parse so the id validation uses z.uuid(); update only the schema expression referenced in the getValidatedRouterParams(...) invocation and keep everything else (event handling and variable destructuring from const { id }) unchanged.server/api/join-requests/[id]/reject.post.ts (1)
13-13: ⚡ Quick win
z.string().uuid()is deprecated in Zod 4 — usez.uuid()instead
z.string().email(); // ❌ deprecated→z.email(); // ✅— the same deprecation applies to all string-format method validators. The method equivalents (z.string().email(), etc.) are still available but have been deprecated and will be removed in the next major version.This inline
z.object({ id: z.string().uuid() }).parseschema is also duplicated verbatim across at least six endpoints. Consider centralising it in a shared util (e.g.server/utils/routeSchemas.ts) to eliminate drift:♻️ Proposed fix (local + shared-schema approach)
// server/utils/routeSchemas.ts (new or existing shared file) +import { z } from 'zod' +export const routeIdSchema = z.object({ id: z.uuid() })// server/api/join-requests/[id]/reject.post.ts -import { z } from 'zod' +import { routeIdSchema } from '../../../utils/routeSchemas' ... - const { id: requestId } = await getValidatedRouterParams(event, z.object({ id: z.string().uuid() }).parse) + const { id: requestId } = await getValidatedRouterParams(event, routeIdSchema.parse)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/join-requests/`[id]/reject.post.ts at line 13, Replace the deprecated z.string().uuid() usage and centralize the duplicated route schema: create a shared schema (e.g., export requestIdSchema = z.object({ id: z.uuid() }) from a new server/utils/routeSchemas.ts) and then update this handler to call getValidatedRouterParams(event, requestIdSchema.parse) instead of the inline z.object({ id: z.string().uuid() }).parse; update other endpoints to import and reuse requestIdSchema to avoid duplication.server/utils/pgDumpEnv.ts (1)
7-7: Consider addingPGSSLMODE(and friends) to the allowlist for remote-DB deploymentsWhen
DATABASE_URLincludes SSL parameters (e.g.?sslmode=require), the caller inbackup.post.tsextractshost/port/user/dbindividually from the URL and passes them as CLI flags — it does not forwardsslmode. BecausePGSSLMODEis also absent from the allowlist here,pg_dumpinherits no SSL hint and defaults toprefer(try SSL, accept plaintext). For self-hosted single-VPS setups (localhost DB) this is fine, but for any cloud-managed Postgres that enforces SSL it could silently back up over an unencrypted connection — or fail when the server rejects the unencrypted attempt.-const PG_DUMP_ENV_ALLOWLIST = ['PATH', 'HOME', 'LANG', 'LC_ALL', 'LC_CTYPE', 'TZ', 'TMPDIR'] as const +const PG_DUMP_ENV_ALLOWLIST = [ + 'PATH', 'HOME', 'LANG', 'LC_ALL', 'LC_CTYPE', 'TZ', 'TMPDIR', + // SSL negotiation (needed for remote/cloud DB with sslmode=require) + 'PGSSLMODE', 'PGSSLROOTCERT', 'PGSSLCERT', 'PGSSLKEY', +] as constIf
PGSSLMODEshould be derived from theDATABASE_URLquery string rather than the parent env, the alternative is to parsedbUrl.searchParams.get('sslmode')in the caller and pass it explicitly as aPGSSLMODEoverride in the returned env.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/pgDumpEnv.ts` at line 7, PG_DUMP_ENV_ALLOWLIST omits PGSSLMODE so pg_dump can lose SSL directives when DATABASE_URL is split into flags; update the allowlist constant PG_DUMP_ENV_ALLOWLIST to include PGSSLMODE (and optionally related vars like PGSSLCERT/PGSSLKEY/PGSSLROOTCERT) so pg_dump inherits SSL settings, or alternatively parse sslmode from the DATABASE_URL in backup.post.ts and explicitly set PGSSLMODE in the env returned to pg_dump; modify PG_DUMP_ENV_ALLOWLIST and/or add code in the caller that reads dbUrl.searchParams.get('sslmode') and injects PGSSLMODE into the process env passed to pg_dump.tests/unit/security-fixes.test.ts (2)
12-13: 💤 Low valueDuplicate import of
randomBytes.Both
randomBytesandcryptoRandomBytesare imported fromnode:crypto, but they're the same function. Line 13's import shadows the one from line 12.♻️ Remove duplicate import
-import { createHash, hkdfSync, createCipheriv, createDecipheriv, randomBytes } from 'node:crypto' -import { randomBytes as cryptoRandomBytes } from 'node:crypto' +import { createHash, hkdfSync, createCipheriv, createDecipheriv, randomBytes } from 'node:crypto'Then update line 39 to use
randomBytesinstead ofcryptoRandomBytes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/security-fixes.test.ts` around lines 12 - 13, The file imports randomBytes twice (as randomBytes and cryptoRandomBytes), causing a duplicate shadowed import; remove the second import alias (cryptoRandomBytes) from the import list and update all usages of cryptoRandomBytes to use randomBytes instead (e.g., replace the call at the current test usage with randomBytes) so only the single randomBytes import from node:crypto is used.
309-386: ⚖️ Poor tradeoffFix 5 tests reproduce logic locally instead of testing actual implementation.
These tests manually reproduce the warning logic from
rateLimit.tsrather than importing and exercising the actual code. If the implementation inrateLimit.tschanges (e.g., different threshold, different message format), these tests will still pass while the real behavior diverges.Consider importing and triggering the actual startup warning logic, or add integration tests that verify the real module's behavior. The current approach documents the expected contract but doesn't validate the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/security-fixes.test.ts` around lines 309 - 386, Tests are reproducing the warning logic locally instead of exercising the real code; update the tests to import the actual rateLimit module and trigger its startup path (rather than duplicating the string/threshold). Specifically, set process.env.RAILWAY_REPLICA_COUNT to the desired value, spy on console.warn, then require/import the module or call its exported startup function (e.g., initRateLimiter or checkReplicaCount) from rateLimit.ts so the real code emits the warning; if no such export exists, add a small exported function (checkReplicaCount or initRateLimiter) in rateLimit.ts that runs the existing startup check so tests can call it directly. Ensure assertions then inspect warnSpy calls for the real message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/api/public/jobs/`[slug]/apply.post.ts:
- Around line 16-21: The apply endpoint's rate limiter (applyRateLimit created
via createRateLimiter) blocks E2E tests; update the rate limiter initialization
to skip or relax limits when running tests by checking the environment (e.g.,
process.env.NODE_ENV === 'test' or a dedicated flag like process.env.E2E ===
'true') and only apply createRateLimiter in non-test environments, or set a much
higher maxRequests for test/e2e envs; modify the applyRateLimit definition so it
conditionally returns a no-op/mocked middleware for test runs while keeping the
existing 15min/5-request behavior for production.
---
Nitpick comments:
In `@package.json`:
- Line 91: The dependency spec for "uuid" currently allows any future major (
"uuid": ">=14.0.0" ); change it to bound the major version for reproducible
installs by restricting to the 14.x range (for example use a semver range like
"uuid": ">=14.0.0 <15.0.0" or simply "^14.0.0") so updates are limited to
non-breaking releases; update the "uuid" entry in package.json accordingly.
In `@server/api/candidates/`[id]/documents/index.post.ts:
- Line 39: Replace the deprecated z.string().uuid() inline schema with Zod 4's
z.uuid() and centralize the schema: create a shared exported constant (e.g.,
ROUTE_ID_PARAM_SCHEMA = z.object({ id: z.uuid() })) in a validation utility and
update calls that pass the inline parser (the getValidatedRouterParams call
currently using z.object({ id: z.string().uuid() }).parse) to use
ROUTE_ID_PARAM_SCHEMA.parse; update the same replacement across the ~14 route
files that duplicate the inline schema so future changes are one-liners.
In `@server/api/chatbot/agents/`[id].patch.ts:
- Line 28: Replace the deprecated schema usage in the router params: in the call
to getValidatedRouterParams where you build the Zod schema (currently using
z.object({ id: z.string().uuid() }).parse), switch to z.object({ id: z.uuid()
}).parse so the id validation uses z.uuid(); update only the schema expression
referenced in the getValidatedRouterParams(...) invocation and keep everything
else (event handling and variable destructuring from const { id }) unchanged.
In `@server/api/chatbot/folders/`[id].patch.ts:
- Line 20: The route parameter validation uses the deprecated pattern
z.string().uuid() inside the call to getValidatedRouterParams; update the schema
passed to getValidatedRouterParams to use z.uuid() (or replace with the shared
routeIdSchema if available) so the line becomes getValidatedRouterParams(event,
z.object({ id: z.uuid() }).parse) or uses routeIdSchema.parse; update references
in the handler where the { id } is destructured to match the new schema.
In `@server/api/documents/`[id].delete.ts:
- Line 21: The code uses the deprecated z.string().uuid() when validating the
route param; replace that call with the Zod 4 standalone validator z.uuid() in
the getValidatedRouterParams invocation (so change the schema passed to
getValidatedRouterParams to use z.uuid() for the id field while keeping the
existing destructuring into documentId and the surrounding call site like
getValidatedRouterParams(event, z.object({ id: z.uuid() }).parse).
In `@server/api/documents/`[id]/preview.get.ts:
- Line 26: The call that validates route params uses the deprecated schema
z.string().uuid(); update the validator to use z.uuid() instead by changing the
schema passed into getValidatedRouterParams (the line that assigns documentId
via const { id: documentId } = await getValidatedRouterParams(event,
z.string().uuid().parse) — replace z.string().uuid() with z.uuid()) so the route
parameter validation uses z.uuid() for the id field.
In `@server/api/invite-links/`[id].delete.ts:
- Line 13: The router params validation uses deprecated z.string().uuid();
update the call that extracts linkId (const { id: linkId } = await
getValidatedRouterParams(event, z.object({ id: z.string().uuid() }).parse)) to
use z.uuid() instead—ideally replace the inline schema with the shared
routeIdSchema (or import routeIdSchema and pass getValidatedRouterParams(event,
routeIdSchema.parse)) so validation uses z.uuid() and reuses the common schema;
ensure the variable name linkId and the call to getValidatedRouterParams remain
unchanged.
In `@server/api/join-requests/`[id]/reject.post.ts:
- Line 13: Replace the deprecated z.string().uuid() usage and centralize the
duplicated route schema: create a shared schema (e.g., export requestIdSchema =
z.object({ id: z.uuid() }) from a new server/utils/routeSchemas.ts) and then
update this handler to call getValidatedRouterParams(event,
requestIdSchema.parse) instead of the inline z.object({ id: z.string().uuid()
}).parse; update other endpoints to import and reuse requestIdSchema to avoid
duplication.
In `@server/utils/pgDumpEnv.ts`:
- Line 7: PG_DUMP_ENV_ALLOWLIST omits PGSSLMODE so pg_dump can lose SSL
directives when DATABASE_URL is split into flags; update the allowlist constant
PG_DUMP_ENV_ALLOWLIST to include PGSSLMODE (and optionally related vars like
PGSSLCERT/PGSSLKEY/PGSSLROOTCERT) so pg_dump inherits SSL settings, or
alternatively parse sslmode from the DATABASE_URL in backup.post.ts and
explicitly set PGSSLMODE in the env returned to pg_dump; modify
PG_DUMP_ENV_ALLOWLIST and/or add code in the caller that reads
dbUrl.searchParams.get('sslmode') and injects PGSSLMODE into the process env
passed to pg_dump.
In `@server/utils/rateLimit.ts`:
- Around line 13-20: Extract the startup check out of the module-level code in
rateLimit.ts into an exported function named warnIfHorizontallyScaled(env?:
Record<string,string>) that computes replicaCount (currently derived from
process.env.RAILWAY_REPLICA_COUNT/_replicaCount) and emits the same console.warn
when count > 1; replace the inline if-block with a single call to
warnIfHorizontallyScaled() so runtime behavior is unchanged but tests can import
and call warnIfHorizontallyScaled({ RAILWAY_REPLICA_COUNT: '3' }) to exercise
the branch.
In `@tests/unit/security-fixes.test.ts`:
- Around line 12-13: The file imports randomBytes twice (as randomBytes and
cryptoRandomBytes), causing a duplicate shadowed import; remove the second
import alias (cryptoRandomBytes) from the import list and update all usages of
cryptoRandomBytes to use randomBytes instead (e.g., replace the call at the
current test usage with randomBytes) so only the single randomBytes import from
node:crypto is used.
- Around line 309-386: Tests are reproducing the warning logic locally instead
of exercising the real code; update the tests to import the actual rateLimit
module and trigger its startup path (rather than duplicating the
string/threshold). Specifically, set process.env.RAILWAY_REPLICA_COUNT to the
desired value, spy on console.warn, then require/import the module or call its
exported startup function (e.g., initRateLimiter or checkReplicaCount) from
rateLimit.ts so the real code emits the warning; if no such export exists, add a
small exported function (checkReplicaCount or initRateLimiter) in rateLimit.ts
that runs the existing startup check so tests can call it directly. Ensure
assertions then inspect warnSpy calls for the real message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72170c91-b413-44f5-bba9-01c70ede3c0a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (32)
ARCHITECTURE.mdSELF-HOSTING.mdapp/app.vuenuxt.config.tspackage.jsonserver/api/candidates/[id]/documents/index.post.tsserver/api/chatbot/agents/[id].delete.tsserver/api/chatbot/agents/[id].patch.tsserver/api/chatbot/conversations/[id].delete.tsserver/api/chatbot/conversations/[id].get.tsserver/api/chatbot/conversations/[id].patch.tsserver/api/chatbot/folders/[id].delete.tsserver/api/chatbot/folders/[id].patch.tsserver/api/documents/[id].delete.tsserver/api/documents/[id]/download.get.tsserver/api/documents/[id]/preview.get.tsserver/api/invite-links/[id].delete.tsserver/api/invite-links/info/[token].get.tsserver/api/join-requests/[id]/approve.post.tsserver/api/join-requests/[id]/reject.post.tsserver/api/public/jobs/[slug]/apply.post.tsserver/api/public/track/[code].get.tsserver/api/updates/backup.post.tsserver/middleware/csp.tsserver/utils/encryption.tsserver/utils/pgDumpEnv.tsserver/utils/rateLimit.tstests/unit/ai-config-schema.test.tstests/unit/auth-security-hardening.test.tstests/unit/pg-dump-env.test.tstests/unit/rate-limit.test.tstests/unit/security-fixes.test.ts
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/layouts/auth.vue`:
- Around line 12-13: The tooltip title for the dark-mode toggle is hardcoded;
update the title binding to use the app's i18n keys instead of raw English by
calling the translation helper with the appropriate keys (e.g., use $t or the
composition API's t) and pass the current state via isDark so the title becomes
something like t(isDark ? 'ui.switchToLight' : 'ui.switchToDark'); apply the
same change in the PublicNavBar component where the title is currently hardcoded
(referencing the same isDark and toggleColorMode bindings) so both components
use localized strings.
In `@server/plugins/csp-nonce.ts`:
- Around line 3-4: The current chained .replace calls on the HTML string only
replace empty nonce attributes and scripts missing a nonce, leaving pre-existing
non-empty nonce values unchanged; update the first .replace (the one targeting
nonce="" on the line with .replace(/<script\b([^>]*?)\snonce=""([^>]*?)>/g, ...)
to instead match any nonce attribute value (e.g., match nonce="...") so it
normalizes existing non-empty nonces to the per-request nonce, while keeping the
second .replace (the one matching scripts without any nonce) to insert a nonce
for scripts that don't have the attribute; locate these two chained .replace
calls in server/plugins/csp-nonce.ts and adjust the first regex to capture any
nonce attribute and replace it with nonce="${nonce}".
🪄 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: 02bea2eb-fd77-4e37-9de5-4b5745db35f0
📒 Files selected for processing (11)
app/app.vueapp/assets/css/main.cssapp/components/PublicNavBar.vueapp/composables/useColorMode.tsapp/layouts/auth.vueapp/pages/index.vueapp/plugins/color-mode.client.tsserver/plugins/csp-nonce.tstests/tsconfig.jsontests/unit/rate-limit.test.tstests/unit/security-fixes.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- app/app.vue
- tests/unit/security-fixes.test.ts
- tests/unit/rate-limit.test.ts
Comment on lines
+12
to
+13
| :title="isDark ? 'Switch to light mode' : 'Switch to dark mode'" | ||
| @click="toggleColorMode" |
There was a problem hiding this comment.
Localize dark-mode toggle tooltip text.
Line 12 uses hardcoded English ("Switch to light mode" / "Switch to dark mode"), which bypasses i18n. The same pattern is also present in app/components/PublicNavBar.vue Line 59.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/layouts/auth.vue` around lines 12 - 13, The tooltip title for the
dark-mode toggle is hardcoded; update the title binding to use the app's i18n
keys instead of raw English by calling the translation helper with the
appropriate keys (e.g., use $t or the composition API's t) and pass the current
state via isDark so the title becomes something like t(isDark ?
'ui.switchToLight' : 'ui.switchToDark'); apply the same change in the
PublicNavBar component where the title is currently hardcoded (referencing the
same isDark and toggleColorMode bindings) so both components use localized
strings.
Comment on lines
+3
to
+4
| .replace(/<script\b([^>]*?)\snonce=""([^>]*?)>/g, `<script$1 nonce="${nonce}"$2>`) | ||
| .replace(/<script\b(?![^>]*\snonce=)/g, `<script nonce="${nonce}"`) |
There was a problem hiding this comment.
Normalize all existing nonce attributes, not just empty ones.
On Line 3 and Line 4, scripts with a pre-existing non-empty nonce are skipped and keep their old value, which can mismatch the per-request CSP nonce and block execution.
Proposed fix
function addNonceToScripts(markup: string, nonce: string) {
return markup
- .replace(/<script\b([^>]*?)\snonce=""([^>]*?)>/g, `<script$1 nonce="${nonce}"$2>`)
+ .replace(/<script\b([^>]*?)\snonce=(['"])[^'"]*\2([^>]*?)>/g, `<script$1 nonce="${nonce}"$3>`)
.replace(/<script\b(?![^>]*\snonce=)/g, `<script nonce="${nonce}"`)
}📝 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
| .replace(/<script\b([^>]*?)\snonce=""([^>]*?)>/g, `<script$1 nonce="${nonce}"$2>`) | |
| .replace(/<script\b(?![^>]*\snonce=)/g, `<script nonce="${nonce}"`) | |
| .replace(/<script\b([^>]*?)\snonce=(['"])[^'"]*\2([^>]*?)>/g, `<script$1 nonce="${nonce}"$3>`) | |
| .replace(/<script\b(?![^>]*\snonce=)/g, `<script nonce="${nonce}"`) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/plugins/csp-nonce.ts` around lines 3 - 4, The current chained .replace
calls on the HTML string only replace empty nonce attributes and scripts missing
a nonce, leaving pre-existing non-empty nonce values unchanged; update the first
.replace (the one targeting nonce="" on the line with
.replace(/<script\b([^>]*?)\snonce=""([^>]*?)>/g, ...) to instead match any
nonce attribute value (e.g., match nonce="...") so it normalizes existing
non-empty nonces to the per-request nonce, while keeping the second .replace
(the one matching scripts without any nonce) to insert a nonce for scripts that
don't have the attribute; locate these two chained .replace calls in
server/plugins/csp-nonce.ts and adjust the first regex to capture any nonce
attribute and replace it with nonce="${nonce}".
…roduction Co-authored-by: Copilot <copilot@github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: add pgDumpEnv utility to secure environment variable handling
test: add unit tests for pgDumpEnv utility
fix: enhance rate limiting logic and add tests
test: add security tests for recent fixes
Summary
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
New Features
Security
Style
Documentation
Tests