fix: resolve all audit report issues - #35
Merged
Merged
Conversation
🔴 C-1: Implement CSRF protection (csrf-csrf, double-submit cookie pattern) 🔴 C-2: Add missing ENCRYPTION_KEY/LINK_ACCESS_SECRET/FINGERPRINT_SECRET to .env.example 🔴 C-3: Mount generalLimiter on /api before route handlers 🟠 H-1: Gate bulk ops, CSV import, and scheduled links behind isFeatureEnabled 🟠 H-2: Validate URL safety on each chain hop in resolveChain 🟡 M-1: Delete orphaned apps/api/.env.local 🟡 M-2: Remove @types/sharp (sharp bundles its own types) 🟡 M-3: Rename BLOCKLIST_TLDS to BLOCKLIST_DOMAINS 🟡 M-4: Already gitignored (apps/*/dist/) 🔵 L-1: Fix docs/PROJECT.md reference (blockedDomains -> blocklistDomains) 🔵 L-2: Move pino-http after stripe webhook route 🔵 L-3: Already resolved (getRateLimiter doesn't exist in current code) 🔵 L-4: Add .length(64) validation for secret env vars
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe API adds CSRF protection and token issuance, tightens secret validation, gates bulk and scheduled operations, validates redirect targets, and renames the domain blocklist export. API and dashboard audit reports document verification results and findings. ChangesAPI security and feature controls
Audit documentation
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthRoutes
participant csrfTokenHandler
participant csrfProtection
participant ExpressAPI
Client->>AuthRoutes: GET /api/auth/csrf-token
AuthRoutes->>csrfTokenHandler: Generate token
csrfTokenHandler-->>Client: Return token and set CSRF cookie
Client->>csrfProtection: Send protected request with x-csrf-token
csrfProtection->>ExpressAPI: Forward validated request
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@apps/api/.env.example`:
- Around line 38-40: Update the CSRF_SECRET entry in the environment example to
use the same 64-character hexadecimal placeholder format as the other secrets,
while preserving the existing CSRF configuration key.
In `@apps/api/src/app.ts`:
- Line 42: Update the Express middleware registration in the app initialization
to call express.json() without configuration arguments. Remove the custom 1mb
limit while preserving the middleware placement and behavior otherwise.
In `@apps/api/src/middleware/csrf.ts`:
- Around line 21-24: Update getCsrfTokenFromRequest to read the CSRF token
exclusively from the x-csrf-token request header. Remove the fallback to
req.cookies[CSRF_COOKIE_NAME], while preserving the existing undefined-to-null
behavior when the header is absent.
- Line 5: Update the CSRF cookie name definition around CSRF_COOKIE_NAME to use
the __Host- prefixed name only in production and a regular cookie name in
non-production environments, matching the secure setting in cookieOptions so
local browsers accept the cookie.
In `@apps/api/src/services/link.service.ts`:
- Around line 174-177: Update the error-handling path in the link URL processing
function so malformed targets that cause URL parsing to throw are passed through
validateUrlSafety before being returned. Preserve the existing AppError
handling, and ensure non-AppError parsing failures do not return targetUrl
without validation.
In `@apps/api/src/services/url.services.ts`:
- Around line 104-106: Update the AppError message in the ScheduledLinks feature
check to remove the “on your plan” wording, while preserving the existing status
code and error code.
In `@apps/api/src/utils/env.ts`:
- Around line 10-13: Update the LINK_ACCESS_SECRET, ENCRYPTION_KEY,
FINGERPRINT_SECRET, and CSRF_SECRET validators in the environment schema to use
regex validation that accepts only hexadecimal characters and requires exactly
64 characters. Preserve the existing validation error messages and reject
spaces, symbols, and other non-hex input.
🪄 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: 64fc9e8a-a656-44ca-85ac-951fb98e9fd0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
apps/api/.env.exampleapps/api/package.jsonapps/api/src/app.tsapps/api/src/constants/blocklistDomains.tsapps/api/src/controllers/link.controller.tsapps/api/src/controllers/url.controllers.tsapps/api/src/middleware/csrf.tsapps/api/src/routes/auth.routes.tsapps/api/src/services/link.service.tsapps/api/src/services/url.services.tsapps/api/src/services/urlSafety.tsapps/api/src/utils/__tests__/env.test.tsapps/api/src/utils/env.tsapps/api/vitest.config.tsdocs/API.mddocs/PROJECT.md
| import type { Request, Response, NextFunction } from 'express' | ||
| import { env } from '../utils/env' | ||
|
|
||
| const CSRF_COOKIE_NAME = '__Host-linkify.x-csrf-token' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid the __Host- prefix in non-production environments to prevent cookie rejection.
The __Host- cookie prefix enforces strict browser rules, one of which requires the Secure attribute to be present. Because your cookieOptions sets secure: env.NODE_ENV === 'production', the Secure attribute will be missing in local development. As a result, the browser will actively reject the cookie, completely breaking CSRF functionality locally.
To fix this, dynamically use a regular cookie name in non-production environments.
🐛 Proposed fix
-const CSRF_COOKIE_NAME = '__Host-linkify.x-csrf-token'
+const CSRF_COOKIE_NAME = env.NODE_ENV === 'production' ? '__Host-linkify.x-csrf-token' : 'linkify.x-csrf-token'📝 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
| const CSRF_COOKIE_NAME = '__Host-linkify.x-csrf-token' | |
| const CSRF_COOKIE_NAME = env.NODE_ENV === 'production' ? '__Host-linkify.x-csrf-token' : 'linkify.x-csrf-token' |
🤖 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 `@apps/api/src/middleware/csrf.ts` at line 5, Update the CSRF cookie name
definition around CSRF_COOKIE_NAME to use the __Host- prefixed name only in
production and a regular cookie name in non-production environments, matching
the secure setting in cookieOptions so local browsers accept the cookie.
… add audit reports
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.
🔴 C-1: Implement CSRF protection (csrf-csrf, double-submit cookie pattern)
🔴 C-2: Add missing ENCRYPTION_KEY/LINK_ACCESS_SECRET/FINGERPRINT_SECRET to .env.example
🔴 C-3: Mount generalLimiter on /api before route handlers
🟠 H-1: Gate bulk ops, CSV import, and scheduled links behind isFeatureEnabled
🟠 H-2: Validate URL safety on each chain hop in resolveChain
🟡 M-1: Delete orphaned apps/api/.env.local
🟡 M-2: Remove @types/sharp (sharp bundles its own types)
🟡 M-3: Rename BLOCKLIST_TLDS to BLOCKLIST_DOMAINS
🟡 M-4: Already gitignored (apps/*/dist/)
🔵 L-1: Fix docs/PROJECT.md reference (blockedDomains -> blocklistDomains)
🔵 L-2: Move pino-http after stripe webhook route
🔵 L-3: Already resolved (getRateLimiter doesn't exist in current code)
🔵 L-4: Add .length(64) validation for secret env vars
Summary by CodeRabbit