Conversation
✅ Deploy Preview for tradetrust-functions ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThe PR introduces origin and referrer validation middleware and M2M (machine-to-machine) CSRF bypass logic. New Changes
Sequence DiagramsequenceDiagram
participant Client
participant Guard as originReferrerGuard
participant CSRFMiddleware as CSRF Validation
participant Route as Route Handler
Client->>Guard: POST request with Origin/Referer
Guard->>Guard: Extract origin candidate
alt Valid API Key (M2M)
Guard->>Guard: Extract API key from header
Guard->>Guard: Validate API key
Guard->>Route: Allow request (bypass CSRF)
else No API Key or Invalid
Guard->>Guard: Validate origin against ALLOWED_ORIGIN_REGEX
alt Trusted Origin
Guard->>CSRFMiddleware: Proceed to CSRF validation
CSRFMiddleware->>Route: CSRF token valid
else Untrusted/Missing Origin
Guard->>Client: 403 Forbidden
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netlify/utils.ts (1)
47-53: CRITICAL: Fix API key validation vulnerability.If
process.env.API_KEYis undefined (not set), requests without an API key will pass validation sinceundefined !== undefinedevaluates tofalse. This creates a security vulnerability where the API becomes unprotected if the environment variable is not configured.Apply this diff to fix the vulnerability:
export const checkApiKey = (req, res, next) => { const apiKey = req.headers["x-api-key"]; - if (apiKey !== process.env.API_KEY) { + if (!process.env.API_KEY || apiKey !== process.env.API_KEY) { return res.status(400).send(ERROR_MESSAGE.API_KEY_INVALID); } next(); };Alternatively, add a startup validation to ensure
API_KEYis always set in production environments.
🧹 Nitpick comments (2)
netlify/utils.ts (1)
88-93: Clarify error message for invalid API key.When an API key is provided but invalid, the error message "Missing Origin/Referer" is misleading. Consider returning a more accurate message that indicates either the Origin/Referer is missing OR the API key is invalid.
Apply this diff to improve the error message:
if (!candidateOrigin) { // Allow M2M (non-browser) requests without Origin/Referer only if API key is valid const apiKey = req.headers["x-api-key"] as string | undefined; if (apiKey && apiKey === process.env.API_KEY) return next(); - return res.status(403).json({ error: "Missing Origin/Referer" }); + return res.status(403).json({ error: "Missing or invalid Origin/Referer or API key" }); }netlify/functions/storage/router.ts (1)
64-72: Consider consolidating M2M bypass logic.The M2M bypass logic (checking for no Origin/Referer + valid API key) is duplicated here and in
originReferrerGuard. SinceoriginReferrerGuardis already applied to these routes (lines 96, 115), the M2M requests will pass through it. You could simplify this middleware by relying on a request property set byoriginReferrerGuardinstead of re-checking headers.Example approach:
+// In originReferrerGuard (utils.ts): + if (!candidateOrigin) { + const apiKey = req.headers["x-api-key"] as string | undefined; + if (apiKey && apiKey === process.env.API_KEY) { + req.isM2MRequest = true; // Mark as M2M + return next(); + } + return res.status(403).json({ error: "Missing Origin/Referer" }); + } // In csrfValidationMiddleware: - const originHeader = req.headers.origin as string | undefined; - const refererHeader = req.headers.referer as string | undefined; - const apiKey = req.headers["x-api-key"] as string | undefined; - - // If no Origin/Referer and valid API key, skip CSRF validation (M2M request) - if (!originHeader && !refererHeader && apiKey === process.env.API_KEY) { + // Skip CSRF validation for M2M requests + if (req.isM2MRequest) { return next(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
netlify/functions/storage/router.ts(4 hunks)netlify/functions/verify/router.ts(1 hunks)netlify/utils.ts(3 hunks)tests/integration/document-astron-verify.test.ts(4 hunks)tests/integration/document-astrontestnet-verify.test.ts(4 hunks)tests/integration/document-verify.test.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
netlify/utils.ts (1)
netlify/constants.ts (2)
ALLOWED_ORIGIN_REGEX(5-6)LOCALHOST_ORIGINS(8-11)
netlify/functions/verify/router.ts (1)
netlify/utils.ts (1)
originReferrerGuard(70-100)
netlify/functions/storage/router.ts (1)
netlify/utils.ts (2)
originReferrerGuard(70-100)checkApiKey(47-53)
🔇 Additional comments (11)
netlify/utils.ts (4)
18-18: LGTM: Express types imported for middleware.The Express type imports are correctly added to support the new middleware signatures.
55-68: LGTM: Origin validation logic is sound.The
isTrustedOriginfunction correctly:
- Handles both full URLs and origin strings
- Validates against the allowed regex pattern
- Permits localhost only in test environments
- Uses safe URL parsing with try-catch
70-100: LGTM: M2M bypass logic correctly implemented.The
originReferrerGuardmiddleware correctly:
- Validates Origin header first, falls back to Referer
- Allows M2M requests with valid API key when Origin/Referer is missing
- Rejects untrusted origins
- Uses proper Express typing
108-116: LGTM: Return type broadened safely.The addition of
unknownto the return type makes the signature more accurate without changing the runtime behavior.netlify/functions/storage/router.ts (3)
2-2: LGTM: originReferrerGuard applied to POST routes.The middleware is correctly applied to the storage POST routes, providing origin/referrer validation with M2M bypass support.
Also applies to: 96-96, 115-115
43-49: LGTM: CSRF cookie properly configured.The cookie configuration is secure:
httpOnly: trueprevents JavaScript accesssecure: trueensures HTTPS-onlysameSite: "None"allows cross-origin (required withsecure)- Path scoped to
/.netlify/functions/storage- 1-hour expiration
34-56: I need to verify whether this repository contains frontend/browser client code that might be affected by this change.Let me check the actual repository structure to determine if this is a backend-only service:
CSRF token endpoint requires API key—verify integration impact with your frontend/browser clients.
All tests show that CSRF tokens are obtained with both
originReferrerGuardandcheckApiKeymiddleware, which is intentional for this API architecture. However, this breaks the traditional CSRF pattern where browsers obtain tokens before authenticating.If your frontend clients or browser-based integrations need to:
- Obtain CSRF tokens without an API key
- Make unauthenticated cross-origin requests
Then consider removing
checkApiKeyfrom the/csrf-tokenendpoint OR update your frontend to acquire API keys before fetching CSRF tokens. Document your intended authentication flow for clarity.netlify/functions/verify/router.ts (1)
2-2: LGTM: originReferrerGuard applied to verify endpoint.The middleware correctly protects the verify endpoint by allowing:
- Browser requests from trusted origins (with or without API key)
- M2M requests with valid API key (no Origin/Referer required)
This is appropriate for a verification endpoint that should be accessible to both browser clients and programmatic API consumers.
Also applies to: 7-7
tests/integration/document-verify.test.ts (1)
34-34: LGTM: Tests updated for API key authentication.All POST requests now include the
x-api-keyheader, correctly simulating M2M requests. This aligns with the neworiginReferrerGuardmiddleware that requires either a trusted Origin/Referer or a valid API key.Also applies to: 44-44, 52-52, 63-63, 74-74, 85-85, 96-96, 107-107, 118-118, 129-129, 140-140
tests/integration/document-astron-verify.test.ts (1)
24-24: LGTM: Tests updated for API key authentication.All POST requests now include the
x-api-keyheader, consistent with the new authentication requirements. The formatting improvements to the request chains also enhance readability.Also applies to: 34-34, 42-42, 53-53, 64-64, 75-75
tests/integration/document-astrontestnet-verify.test.ts (1)
24-24: LGTM: Tests updated for API key authentication.All POST requests now include the
x-api-keyheader, completing the test suite updates to align with the new origin/referrer validation and M2M authentication flow.Also applies to: 34-34, 42-42, 53-53, 64-64, 75-75
2 participants
Summary
Revert the cookies config to hardcoded
Summary by CodeRabbit