fix: webapp url#1707
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a new exported getBaseUrl(c) helper that validates and normalizes the WEBAPP_URL, and replaces direct getEnv(c, 'WEBAPP_URL') usage across several backend modules to centralize base URL handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/utils/stripe.ts (1)
462-475:⚠️ Potential issue | 🟠 MajorPreserve non-Stripe fallback by deferring
getBaseUrl(c)lookup.Line 462 now executes before the
isStripeConfigured(c)guard. IfWEBAPP_URLis missing/invalid,createCustomerthrows before reaching the fake-customer fallback path.🛠️ Proposed fix
export async function createCustomer(c: Context, email: string, userId: string, orgId: string, name: string) { cloudlog({ requestId: c.get('requestId'), message: 'createCustomer', email, userId, orgId, name }) - const baseConsoleUrl = (getBaseUrl(c) || '').replace(/\/+$/, '') + const stripeConfigured = isStripeConfigured(c) const metadata: Record<string, string> = { user_id: userId, org_id: orgId, } - if (baseConsoleUrl) { - metadata.log_as = `${baseConsoleUrl}/log-as/${userId}` - } - if (!isStripeConfigured(c)) { + if (!stripeConfigured) { cloudlog({ requestId: c.get('requestId'), message: 'createCustomer no stripe key', email, userId, name }) // create a fake customer id like stripe one and random id const randomId = Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15) return { id: `cus_${randomId}`, email, name, metadata } } + const baseConsoleUrl = getBaseUrl(c).replace(/\/+$/, '') + if (baseConsoleUrl) + metadata.log_as = `${baseConsoleUrl}/log-as/${userId}` + const customer = await getStripe(c).customers.create({ email, name, metadata, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/stripe.ts` around lines 462 - 475, The code computes baseConsoleUrl by calling getBaseUrl(c) before checking isStripeConfigured(c), which can throw when WEBAPP_URL is missing and prevents the non-Stripe fallback in createCustomer from running; fix by deferring the getBaseUrl(c) lookup until after the isStripeConfigured(c) guard (or only compute baseConsoleUrl when isStripeConfigured(c) is true), so that metadata and the fake customer return path remain reachable when Stripe is not configured; update references around baseConsoleUrl, getBaseUrl, isStripeConfigured, and createCustomer accordingly.
🤖 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/utils/utils.ts`:
- Around line 178-180: The HTTPS upgrade currently checks if
baseUrlParsed.protocol === 'https:' then sets it to 'https:', which never
upgrades http URLs; in getBaseUrl update the condition on baseUrlParsed to check
that the hostname is not 'localhost' and not '127.0.0.1' AND that
baseUrlParsed.protocol !== 'https:' (or startsWith 'http:'), then set
baseUrlParsed.protocol = 'https:' so non-TLS WEBAPP_URL values are upgraded for
non-local hosts while leaving localhost/127.0.0.1 unchanged.
---
Outside diff comments:
In `@supabase/functions/_backend/utils/stripe.ts`:
- Around line 462-475: The code computes baseConsoleUrl by calling getBaseUrl(c)
before checking isStripeConfigured(c), which can throw when WEBAPP_URL is
missing and prevents the non-Stripe fallback in createCustomer from running; fix
by deferring the getBaseUrl(c) lookup until after the isStripeConfigured(c)
guard (or only compute baseConsoleUrl when isStripeConfigured(c) is true), so
that metadata and the fake customer return path remain reachable when Stripe is
not configured; update references around baseConsoleUrl, getBaseUrl,
isStripeConfigured, and createCustomer accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
supabase/functions/_backend/private/credits.tssupabase/functions/_backend/private/invite_new_user_to_org.tssupabase/functions/_backend/private/stripe_checkout.tssupabase/functions/_backend/utils/stripe.tssupabase/functions/_backend/utils/utils.ts
Contributor
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a critical production issue where Stripe billing portal links fail to open because WEBAPP_URL is configured with http:// instead of https://, preventing customers from purchasing plans. The solution introduces a getBaseUrl() helper function that should enforce HTTPS for production URLs while allowing HTTP for localhost development.
Changes:
- Added
getBaseUrl()function to automatically upgrade HTTP to HTTPS for non-localhost URLs - Replaced all
getEnv(c, 'WEBAPP_URL')calls withgetBaseUrl(c)across Stripe checkout, customer creation, and invitation flows
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/functions/_backend/utils/utils.ts | Introduces getBaseUrl() helper with protocol upgrade logic and error handling |
| supabase/functions/_backend/utils/stripe.ts | Replaces getEnv(c, 'WEBAPP_URL') with getBaseUrl(c) in redirect URL validation and customer metadata |
| supabase/functions/_backend/private/stripe_checkout.ts | Updates checkout endpoint to use getBaseUrl(c) for success/cancel URL defaults |
| supabase/functions/_backend/private/invite_new_user_to_org.ts | Updates invitation link generation to use getBaseUrl(c) |
| supabase/functions/_backend/private/credits.ts | Updates credit checkout URLs to use getBaseUrl(c) |
Comment on lines
+166
to
+191
| export function getBaseUrl(c: Context): string { | ||
| try { | ||
| const baseUrl = getEnv(c, 'WEBAPP_URL') | ||
| if (!baseUrl) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'missing_web_app_url', | ||
| baseUrl, | ||
| }) | ||
| throw simpleError('missing_web_app_url', 'Web app URL is required') | ||
| } | ||
| const baseUrlParsed = new URL(baseUrl) | ||
| if (baseUrlParsed.hostname !== 'localhost' && baseUrlParsed.hostname !== '127.0.0.1' && baseUrlParsed.protocol === 'https:') { | ||
| baseUrlParsed.protocol = 'https:' | ||
| } | ||
| return baseUrlParsed.toString() | ||
| } | ||
| catch (e) { | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'invalid_web_app_url', | ||
| error: e, | ||
| }) | ||
| throw simpleError('invalid_web_app_url', 'Invalid web app URL') | ||
| } | ||
| } |
There was a problem hiding this comment.
The new getBaseUrl() function lacks test coverage, particularly for the http-to-https upgrade logic. Consider adding unit tests that verify: (1) http URLs are upgraded to https for non-localhost domains, (2) localhost/127.0.0.1 URLs remain http, (3) already-https URLs remain unchanged, and (4) proper error handling for invalid/missing URLs. The existing stripe-redirects.unit.test.ts only mocks WEBAPP_URL with https, which won't catch the logic bug in line 178.
| throw simpleError('missing_web_app_url', 'Web app URL is required') | ||
| } | ||
| const baseUrlParsed = new URL(baseUrl) | ||
| if (baseUrlParsed.hostname !== 'localhost' && baseUrlParsed.hostname !== '127.0.0.1' && baseUrlParsed.protocol === 'https:') { |
There was a problem hiding this comment.
Critical logic error: The condition baseUrlParsed.protocol === 'https:' will never be true when the WEBAPP_URL is configured with 'http://' (the exact problem you're trying to fix). The condition should check if the protocol is NOT https (baseUrlParsed.protocol !== 'https:') to upgrade http to https for non-localhost URLs. Currently, this code only sets protocol to https when it's already https, which does nothing.
Suggested change
| if (baseUrlParsed.hostname !== 'localhost' && baseUrlParsed.hostname !== '127.0.0.1' && baseUrlParsed.protocol === 'https:') { | |
| if (baseUrlParsed.hostname !== 'localhost' && baseUrlParsed.hostname !== '127.0.0.1' && baseUrlParsed.protocol !== 'https:') { |
| cloudlogErr({ | ||
| requestId: c.get('requestId'), | ||
| message: 'missing_web_app_url', | ||
| baseUrl, |
There was a problem hiding this comment.
The baseUrl parameter in this error log will always be undefined or falsy (since it's inside the !baseUrl condition), making the log unhelpful. Consider removing this field from the log or moving it outside the if block to log the actual value (even if empty).
Suggested change
| baseUrl, |
Comment on lines
429
to
430
| if (!baseWebAppUrl) | ||
| throw simpleError('invalid_redirect_url', 'WEBAPP_URL is not configured', { field }) |
There was a problem hiding this comment.
The check if (!baseWebAppUrl) is redundant because getBaseUrl(c) always throws an error if WEBAPP_URL is missing or invalid (it never returns a falsy value). This condition can never be true in normal execution flow.
Suggested change
| if (!baseWebAppUrl) | |
| throw simpleError('invalid_redirect_url', 'WEBAPP_URL is not configured', { field }) |
| export async function createCustomer(c: Context, email: string, userId: string, orgId: string, name: string) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'createCustomer', email, userId, orgId, name }) | ||
| const baseConsoleUrl = (getEnv(c, 'WEBAPP_URL') || '').replace(/\/+$/, '') | ||
| const baseConsoleUrl = (getBaseUrl(c) || '').replace(/\/+$/, '') |
There was a problem hiding this comment.
The COALESCE pattern (getBaseUrl(c) || '') is unnecessary since getBaseUrl(c) always returns a non-empty string or throws an error. You can safely use getBaseUrl(c) directly here.
Suggested change
| const baseConsoleUrl = (getBaseUrl(c) || '').replace(/\/+$/, '') | |
| const baseConsoleUrl = getBaseUrl(c).replace(/\/+$/, '') |
| return | ||
|
|
||
| const baseConsoleUrl = (getEnv(c, 'WEBAPP_URL') || '').replace(/\/+$/, '') | ||
| const baseConsoleUrl = (getBaseUrl(c) || '').replace(/\/+$/, '') |
There was a problem hiding this comment.
The COALESCE pattern (getBaseUrl(c) || '') is unnecessary since getBaseUrl(c) always returns a non-empty string or throws an error. You can safely use getBaseUrl(c) directly here.
|
WcaleNieWolny
added a commit
that referenced
this pull request
Feb 26, 2026
This reverts commit ff20d1a.
riderx
added a commit
that referenced
this pull request
Mar 3, 2026
* fix(security): restrict apikey oracle rpc access * fix: webapp url * fix: fix * chore(release): 12.116.9 * fix: envs * Revert "Merge pull request #1707 from Cap-go/fix_webapp_url" This reverts commit ff20d1a. * fix: typo * chore(release): 12.116.10 * fix(security): restrict apikey oracle rpc access * fix: return 503 instead of 400 for service_unavailable build errors Builder availability errors (not configured, call failed, error response, missing upload URL) are transient server-side failures, not client errors. Returning 503 allows the CLI retry logic to automatically retry these requests instead of treating them as terminal 400 errors. * chore(release): 12.116.11 * fix: update PWD script * fix: env vars * fix: modal responsive * feat: forward buildOptions + buildCredentials to builder (pass-through) * fix: correct vue/html-indent in DemoOnboardingModal * fix: use snake_case (build_options, build_credentials) in public API, map to camelCase for builder * fix(security): sanitize SQL interpolation in Cloudflare Analytics Engine queries (#1702) * chore(release): 12.116.12 * Add unit tests for builder payload shape contract Extract buildBuilderPayload() from the inline fetch body so the snake_case → camelCase mapping and exact key set can be tested. 6 vitest cases verify: camelCase output, no legacy credentials field, correct metadata keys, and pass-through of contents. * Reject deprecated `credentials` field with clear upgrade error Old CLI clients sending the flat `credentials` field would have it silently dropped, causing confusing builder failures. Now the proxy explicitly rejects non-empty `credentials` with a migration message pointing to `build_credentials`. * fix(security): clean up role_bindings on member removal (#1722) * chore(release): 12.116.13 * fix(security): use parameterized query in getStoreAppByIdCF to prevent SQL injection The appId parameter was directly interpolated into the D1 SQL query string, creating a SQL injection vulnerability. Switched to bound parameter via .bind(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): prevent privilege escalation in role_bindings endpoint Add priority_rank check so callers cannot assign or update roles with higher privileges than their own. Without this, any user with org.update_user_roles could escalate to org_super_admin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): enforce is_assignable in role_bindings INSERT RLS policy Direct PostgREST inserts could bypass the endpoint's is_assignable check and assign non-assignable roles (e.g. platform_super_admin). The RLS INSERT policy now requires the target role to have is_assignable = true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): cascade all role bindings on member removal delete_org_member_role previously only deleted the org-level binding, leaving orphaned app/channel bindings. A removed member could retain app-level access. Now deletes all bindings for the user in the org. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add trigger to prevent deleting last super_admin binding Direct PostgREST DELETEs on role_bindings could bypass the last super_admin guards in delete_org_member_role. A BEFORE DELETE trigger now rejects deletion of the last org_super_admin binding in any org. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): support hashed API keys in rbac_check_permission_direct The RBAC path in rbac_check_permission_direct looked up API keys with WHERE key = p_apikey, which silently failed for hashed keys. Switched to find_apikey_by_value() which handles both plain-text and hashed keys. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: reword comment to pass typos CI check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove unused desc import from role_bindings.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(security): add FOR UPDATE lock to prevent write-skew on last super_admin delete Two concurrent DELETE transactions could both pass the count check and both delete their rows, leaving zero super_admins. A SELECT ... FOR UPDATE on the super_admin binding set now serializes concurrent deletes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: prevent API key privilege escalation and fix organization member deletion test - Add validation to prevent limited API keys from creating unlimited keys - Fix organization-api test to work with sync_org_user_to_role_binding trigger - Change test user_right from 'invite_read' to 'read' (trigger-compatible) - Verify trigger-created role_bindings instead of manually inserting them * fix: allow CASCADE deletions in prevent_last_super_admin_binding_delete and fix RBAC test compatibility - Add org existence check in trigger to allow CASCADE deletions when org is being deleted - Add service_role bypass for administrative operations and tests - Update tests to work with RBAC security constraints: - 34_test_rbac_rls.sql: Remove DELETE operation that violated super_admin protection - 35_test_is_admin_rbac.sql: Use service_role for test setup INSERT - All SQL database tests now pass (860 tests) - Backend tests remain passing (68 tests) * fix(security): make getCallerMaxPriorityRank auth-type-aware and remove API key data leak * chore(release): 12.116.14 * fix(security): correct API key RBAC principal mapping and remove service_role bypass * fix(security): correct RBAC migration comments and add privilege check on delete - Update migration comments to accurately reflect that service_role is NOT exempt from the last super_admin protection trigger - Replace FOR UPDATE scan with pg_advisory_xact_lock to avoid cross-transaction deadlocks - Add privilege-rank check in delete handler to prevent deleting higher-ranked role bindings - Aligns with established advisory lock patterns in codebase Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> * fix: add self-2fa-required message for 2FA enforcement in multiple languages * chore(release): 12.116.15 * fix(frontend): validate 2fa before enabling org enforcement (#1729) * chore(release): 12.116.16 * fix(deps): update vue monorepo to v3.5.29 (#1731) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(release): 12.116.17 * chore: remove unused cloudflare function getStoreAppByIdCF * chore(release): 12.116.18 * chore: stop editing immutable base migration * fix(frontend): disable auto demo onboarding modal (#1733) * chore(release): 12.116.19 * fix(auth): block sensitive account actions for unverified users (#1690) * fix(auth): block account deletion for unverified users * fix(auth): refresh session fields for email verification gate * fix(auth): make delete_user insert idempotent * fix(auth): explain blocked delete/settings when email unverified * fix(auth): block delete action when email is unverified * fix(auth): localize resend email block and make delete_user idempotent * Restrict invite_user_to_org RPC to authenticated callers (#1710) * fix(db): restrict invite_user_to_org public rpc * fix(db): use caller identity in invite 2FA check * fix(security): restrict webhook select to admin users (#1705) * Secure record_build_time RPC for authorized callers (#1711) * fix(db): secure record_build_time rpc writes * fix(db): preserve service-role record_build_time path * fix(security): restrict apikey oracle rpc access * chore: stop editing immutable base migration * fix(security): restrict apikey oracle rpc access * chore(release): 12.116.20 * fix(security): restrict apikey oracle rpc access * chore: stop editing immutable base migration * fix(security): restrict apikey oracle rpc access * chore: stop editing immutable base migration * fix(security): restrict apikey oracle rpc access --------- Co-authored-by: WcaleNieWolny <isupermichael007@gmail.com> Co-authored-by: WcaleNieWolny <50914789+WcaleNieWolny@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: LOLO <131777939+artylobos@users.noreply.github.com> Co-authored-by: Jordan Lorho <jordan.lorho@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
6 tasks
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
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.
WEBAPP_URL=http://console.capgo.app
Martin is on vacation and because of the recent PRs and the changes to the billing portal link, when generating the link to stripe portal, the link must start with "httpS://console.capgo.app"
Because WEBAPP_URL uses http without TLS, clients cannot open their stripe billing page
One client complained in support - he cannot buy a plan - this is causing capgo to loose money - until martin fixes WEBAPP_URL this is my best solution
Summary by CodeRabbit
Refactor
Bug Fixes