fix(security): restrict password compliance CORS#1750
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds origin-validation middleware to the password compliance function: new helpers normalize and verify request Origins against WEBAPP_URL and a configurable allowlist, and the middleware is mounted globally to reject forbidden Origins before existing password validation logic runs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Function as PasswordComplianceFunction
participant Env as Config
Client->>Function: POST /private/validate_password_compliance (includes Origin)
Function->>Env: read WEBAPP_URL and allowlist via getEnv
Function->>Function: normalizeOrigin -> validateOrigin against allowed list
alt Origin allowed
Function->>Function: proceed to password validation handler
Function-->>Client: 200/400 (password validation response)
else Origin forbidden
Function-->>Client: 403 (forbidden_origin)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 760b259012
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const webappUrl = normalizeOrigin(getEnv(c, 'WEBAPP_URL')) | ||
| const allowed = new Set<string>() | ||
| if (webappUrl) | ||
| allowed.add(webappUrl) |
There was a problem hiding this comment.
This origin allowlist only admits WEBAPP_URL, so any browser request from the Capacitor app context (which uses a non-web origin such as capacitor://localhost/http://localhost) will be rejected with forbidden_origin; that breaks legitimate password-compliance checks from the shared account settings flow (src/pages/settings/account/ChangePassword.vue) for mobile users even though the request is first-party. Please include the native app origins (or a configurable allowlist) so this security hardening does not lock out mobile clients.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/_backend/private/validate_password_compliance.ts (1)
45-58: Add regression coverage for the new origin gate.This middleware is now the security boundary for the endpoint, but the PR only lists manual checks. Please add tests for at least: allowed
Origin, disallowedOrigin, and requests with noOriginheader, withWEBAPP_URLexplicitly mocked in the test setup.Also applies to: 119-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/validate_password_compliance.ts` around lines 45 - 58, Add regression tests covering the new origin gate in validateOrigin: write unit/integration tests that exercise validateOrigin (or the middleware chain invoking validateOrigin) for three scenarios—an allowed Origin header, a disallowed Origin header, and a request with no Origin header—while explicitly mocking WEBAPP_URL in the test setup so getAllowedOrigins returns predictable values; assert that allowed Origin calls next(), disallowed Origin returns the quickError 403 path (and optionally that cloudlog is called with the expected context), and no Origin header also calls next(). Use the existing helpers normalizeOrigin, getAllowedOrigins, quickError, and cloudlog symbols to locate the middleware and hook the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/private/validate_password_compliance.ts`:
- Around line 45-58: Add regression tests covering the new origin gate in
validateOrigin: write unit/integration tests that exercise validateOrigin (or
the middleware chain invoking validateOrigin) for three scenarios—an allowed
Origin header, a disallowed Origin header, and a request with no Origin
header—while explicitly mocking WEBAPP_URL in the test setup so
getAllowedOrigins returns predictable values; assert that allowed Origin calls
next(), disallowed Origin returns the quickError 403 path (and optionally that
cloudlog is called with the expected context), and no Origin header also calls
next(). Use the existing helpers normalizeOrigin, getAllowedOrigins, quickError,
and cloudlog symbols to locate the middleware and hook the assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24bf33d6-fa75-435c-911a-dc13668f0627
📒 Files selected for processing (1)
supabase/functions/_backend/private/validate_password_compliance.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
supabase/functions/_backend/private/validate_password_compliance.ts (2)
82-95: Type annotation could be more specific forContext.The middleware uses
c.get('requestId')which expectsMiddlewareKeyVariables. While this works at runtime, usingContext<MiddlewareKeyVariables>would provide better type safety.♻️ Optional: Use typed Context
-async function validateOrigin(c: Context, next: () => Promise<void>) { +async function validateOrigin(c: Context<MiddlewareKeyVariables>, next: () => Promise<void>) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/validate_password_compliance.ts` around lines 82 - 95, The Context parameter in validateOrigin is currently untyped and should be made specific: change the function signature to use Context<MiddlewareKeyVariables> so calls like c.get('requestId') are type-safe; update the import or type reference for MiddlewareKeyVariables if needed and ensure validateOrigin(c: Context<MiddlewareKeyVariables>, next: () => Promise<void>) compiles with existing usage of c.get('requestId') and other middleware helpers.
62-77: Consider adding IPv6 loopback support.The function correctly allows localhost and 127.0.0.1 for development/native scenarios, but omits the IPv6 loopback address
::1. If IPv6 local development is expected, this could cause unexpected rejections.♻️ Optional: Add IPv6 loopback support
function isNativeOrLocalOrigin(origin: string): boolean { try { const parsed = new URL(origin) if (parsed.protocol === 'capacitor:') return parsed.hostname === 'localhost' if (!(['http:', 'https:'].includes(parsed.protocol))) return false - return parsed.hostname === 'localhost' || parsed.hostname === '127.0.0.1' + return parsed.hostname === 'localhost' || parsed.hostname === '127.0.0.1' || parsed.hostname === '[::1]' } catch { return false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/validate_password_compliance.ts` around lines 62 - 77, The isNativeOrLocalOrigin function currently allows 'localhost' and '127.0.0.1' but omits IPv6 loopback; update the hostname checks in isNativeOrLocalOrigin to also accept the IPv6 loopback ('::1') (note that URL.hostname yields '::1' for bracketed IPv6 literals), and optionally include the full expanded IPv6 loopback if you want broader coverage; make the change in the parsed.hostname comparisons inside isNativeOrLocalOrigin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/private/validate_password_compliance.ts`:
- Around line 82-95: The Context parameter in validateOrigin is currently
untyped and should be made specific: change the function signature to use
Context<MiddlewareKeyVariables> so calls like c.get('requestId') are type-safe;
update the import or type reference for MiddlewareKeyVariables if needed and
ensure validateOrigin(c: Context<MiddlewareKeyVariables>, next: () =>
Promise<void>) compiles with existing usage of c.get('requestId') and other
middleware helpers.
- Around line 62-77: The isNativeOrLocalOrigin function currently allows
'localhost' and '127.0.0.1' but omits IPv6 loopback; update the hostname checks
in isNativeOrLocalOrigin to also accept the IPv6 loopback ('::1') (note that
URL.hostname yields '::1' for bracketed IPv6 literals), and optionally include
the full expanded IPv6 loopback if you want broader coverage; make the change in
the parsed.hostname comparisons inside isNativeOrLocalOrigin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90fe79f5-1cd6-47a0-a5f1-48955fee4235
📒 Files selected for processing (1)
supabase/functions/_backend/private/validate_password_compliance.ts
|
1 participant
Summary (AI generated)
/private/validate_password_complianceorigin validation so browser-origin requests are restricted to configured allowed origins instead of accepting all origins.capacitor://localhostand loopback origins (http://localhost,https://localhost,http://127.0.0.1,https://127.0.0.1) when validating theOriginheader.PASSWORD_COMPLIANCE_ALLOWED_ORIGINSsupport for operationally configurable allowlisting.Motivation (AI generated)
WEBAPP_URLalone blocked valid mobile flows where password checks originate from a Capacitor webview.Business Impact (AI generated)
Screenshots (AI generated)
Test Plan (AI generated)
bun lint:backendOrigin: https://<WEBAPP_URL>returns success from a valid account-password payload (not run in this environment)Origin: capacitor://localhostreturns success from a valid account-password payload (not run in this environment)Origin: http://malicious.examplereturnsforbidden_origin(not run in this environment)Checklist (AI generated)
origin/main.Summary by CodeRabbit