fix(security): restrict password compliance CORS#1750
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds origin-validation middleware to the password-compliance function: helpers normalize and verify Origin headers against WEBAPP_URL plus configured allowlist, detect native/local origins, and short-circuit forbidden Origins with 403. Middleware is mounted globally; checkOrgReadAccess now accepts a requestId for correlated logging. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Function as PasswordComplianceFunction
participant Env as ConfigService
participant Auth as OrgReadAccess
Client->>Function: POST /private/validate_password_compliance (Origin header)
Function->>Env: getEnv(WEBAPP_URL, allowed_origins)
Function->>Function: normalizeOrigin -> getAllowedOrigins -> isNativeOrLocalOrigin
alt Origin allowed
Function->>Auth: checkOrgReadAccess(supabase, orgId, requestId)
Auth-->>Function: OrgReadAccessResult
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)
📝 Coding Plan
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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/private/validate_password_compliance.ts`:
- Around line 65-75: The origin normalization causes custom schemes to become
the string "null" and break native app validation; update validateOrigin to call
isNativeOrLocalOrigin(originalOrigin) using the raw header value before
performing new URL(origin).origin normalization so that isNativeOrLocalOrigin
(which checks for 'capacitor:' and 'ionic:' protocols) sees the original scheme
and returns true for valid native/webview clients—ensure you pass the unmodified
origin string into isNativeOrLocalOrigin and only use the normalized origin for
subsequent http/https comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eac23ee5-cfd7-44dc-bb1b-5fc02fa807d0
📒 Files selected for processing (1)
supabase/functions/_backend/private/validate_password_compliance.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/_backend/private/validate_password_compliance.ts`:
- Line 2: Update the helper signatures to use the middleware-typed context:
change both getAllowedOrigins(context) and validateOrigin(c) to accept
Context<MiddlewareKeyVariables> instead of raw Context, and import (or
reference) MiddlewareKeyVariables from the hono typings so calls like
c.get('requestId') compile; adjust any local references to the context parameter
name if needed to match the new typed signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d25cbe94-0cbe-4585-9825-05db6ef1c0f6
📒 Files selected for processing (2)
supabase/functions/_backend/private/validate_password_compliance.tstests/password-policy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/password-policy.test.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