Feature/sso 02 backend #1378
Feature/sso 02 backend #1378
Conversation
Consolidates 12 incremental SSO migrations (20251224022658 through 20260106000000) into a single comprehensive migration. Schema includes: - Tables: org_saml_connections, saml_domain_mappings, sso_audit_logs - Functions: check_org_sso_configured, lookup_sso_provider_*, auto_join_* - Triggers: auto_join_sso_user_trigger, check_sso_domain_on_signup_trigger - RLS policies for all tables - Indexes for performance - Single SSO per org constraint (UNIQUE org_id, entity_id) - auto_join_enabled flag for controlling enrollment This is PR #1 of the SSO feature split (schema foundation only). No backend endpoints, no frontend, no tests included yet. Related: feature/sso-saml-authentication
📝 WalkthroughWalkthroughAdds full SAML SSO support: DB schema and migration, core management service (configure/update/remove/status), HTTP endpoints (configure, update, remove, test, status, sso_check), a local mock SSO callback, and related utilities, validations, and audit logging. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as Admin/User
participant API as SSO Configure<br/>Endpoint
participant Auth as Supabase Auth<br/>(GoTrue)
participant DB as PostgreSQL
participant Cloud as CloudLog
Admin->>API: POST /private/sso/configure<br/>(org_id, metadata, domains)
API->>API: Authenticate & verify super_admin
API->>API: Parse & validate request body
API->>Cloud: Log processing step
API->>DB: Check domain uniqueness
DB-->>API: Domains available / conflict
API->>API: Check rate limit
API->>Auth: Register SAML provider (metadata)
Auth-->>API: provider_id, entity_id
API->>DB: Persist org_saml_connections
API->>DB: Persist saml_domain_mappings
API->>Cloud: Log audit event (provider_added)
API-->>Admin: { status: "ok", result }
sequenceDiagram
actor Admin as Admin/User
participant API as SSO Test<br/>Endpoint
participant Auth as Supabase Auth
participant Meta as Metadata Fetch
participant DB as PostgreSQL
participant Validator as Metadata Validator
Admin->>API: POST /private/sso/test<br/>(org_id, provider_id, applyFixes?)
API->>API: Authenticate & verify super_admin
API->>DB: Fetch SSO config + domain mappings
DB-->>API: Config, domains
API->>Auth: Verify provider exists
Auth-->>API: Provider details
alt metadata XML absent but metadata URL present
API->>Meta: Fetch metadata (URL, SSRF-safe checks)
Meta-->>API: metadata XML
end
API->>Validator: Validate SAML metadata (entityID, certs, SSO endpoints)
Validator-->>API: { valid, errors, warnings }
alt validation passes
alt applyFixes
API->>DB: Update entity_id / enable / mark verified
end
API-->>Admin: 200 { verified: true, checks, warnings }
else validation fails
API-->>Admin: 400 { errors, warnings }
end
sequenceDiagram
actor User as Browser/Client
participant Frontend as Frontend
participant Public as SSO Check<br/>Endpoint
participant DB as PostgreSQL
User->>Frontend: Enter email during signup
Frontend->>Public: POST /sso_check { email }
Public->>Public: Extract domain
Public->>DB: Query saml_domain_mappings + org_saml_connections (verified & enabled)
DB-->>Public: Provider config or null
alt SSO configured
Public-->>Frontend: { available: true, provider_id, entity_id, org_id, org_name }
else no SSO
Public-->>Frontend: { available: false }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
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
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (3.5.0)supabase/migrations/20260107210800_sso_saml_complete.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: 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: 17
🤖 Fix all issues with AI agents
In @supabase/functions/_backend/private/sso_management.ts:
- Around line 558-627: The loop over domains uses repeated pgClient.query calls
(in the for...of over domains and using extractRootDomain) causing an N+1 query
pattern and a TOCTOU race: checks (exactMatch/rootMatch/rootOwnedByOther) happen
before insert and can be lost to concurrent requests. Fix by batching existence
checks for all relevant domains/roots first (use a single query against
saml_domain_mappings with WHERE domain = ANY($1) AND org_id != $2 to get claimed
domains/owners), then perform inserts inside a DB transaction or rely on a
UNIQUE constraint on domain with INSERT ... ON CONFLICT to atomically claim
domains; update the logic around the domains loop, pgClient.query usage and the
error throws (simpleError) to use the batched results or transactional/ON
CONFLICT outcome instead of per-domain pre-checks.
- Around line 362-371: The PUT fetch call that updates the SSO provider can hang
because it has no timeout; wrap this request in an AbortController with a timer
(e.g., setTimeout) and pass controller.signal to fetch, clear the timer on
success, and call controller.abort() on timeout to throw and handle the failure;
update the try/catch surrounding the fetch in the function that uses providerId
to handle AbortError distinctly and ensure the request headers/body logic
remains unchanged (mirror the timeout pattern used in registerWithSupabaseAuth).
- Around line 1391-1416: The /remove route lacks the permission check before
deleting an org's SSO; after parsing body (parseBody) and validating
orgId/providerId, insert the same authorization check used in the /configure
route (i.e., verify the caller has management permission for the given org)
before logging and calling removeSAML; perform this check using the same
helper/pattern used in /configure (apply it to the Context c and body.orgId),
and only then proceed to cloudlog and await removeSAML; keep closeClient(c,
pgClient) in the catch as-is.
- Around line 416-419: The schema metadataUrlSchema currently allows http:// but
validateMetadataURL enforces HTTPS; update metadataUrlSchema to only accept
HTTPS URLs so schema and runtime match by changing its regex to require
^https?:\/\/ to ^https:\/\/ (i.e. only https) and adjust the error message text
if needed to reference HTTPS, ensuring metadataUrlSchema and validateMetadataURL
are consistent.
- Around line 1356-1385: The /update POST handler lacks the super_admin
permission check that /configure uses; before calling updateSAML(c, config)
(after parsing/validating body and before cloudlog/response), invoke the same
permission-checking helper used by the /configure route to assert the caller is
super_admin and return/throw an authorization error if the check fails; keep
using the existing middlewareAPISecret, ensure pgClient is closed on early exit
(reuse closeClient(c, pgClient) as done in the catch), and reuse the same error
shape/message used by /configure for consistency.
- Around line 25-26: Replace direct Hono instantiation with the project's
createHono helper: remove the direct import of Hono and instead import
createHono from utils/hono.ts, then instantiate the app via createHono() where
the Hono constructor is currently used (e.g., the variable that holds the
app/router). Update any type references/imports that relied on Hono to use the
created app instance, and ensure existing route registrations (the code that
calls app.get/post/put/etc.) continue to attach to the createHono() return value
so Sentry and shared middleware are applied.
- Around line 1292-1350: The handler acquires pgClient via getPgClient(c, true)
but never uses or closes it on success; either remove the unused pgClient
acquisition or ensure it's always closed: if you don't need it for hasOrgRight
or configureSAML, delete the getPgClient call and its pgClient variable;
otherwise wrap the logic in a try/finally and call closeClient(c, pgClient) in
the finally block so pgClient is closed regardless of success or error; check
the functions hasOrgRight and configureSAML references to decide which option to
apply.
- Around line 143-154: The POST request in the try block that calls the GoTrue
Admin API to create SSO providers (the fetch to
`${supabaseUrl}/auth/v1/admin/sso/providers`) needs a timeout using
AbortController to avoid hanging; create an AbortController, pass its signal
into fetch, start a timer (e.g., configurable ms) that calls controller.abort()
on expiry, clear the timer after fetch completes, and catch the AbortError to
throw a clear timeout error so upstream handling/logging can differentiate
timeouts from other failures.
In @supabase/functions/_backend/private/sso_status.ts:
- Around line 45-51: The endpoint registered in app.post('/') uses middlewareV2
and checks auth but lacks organization membership/permission validation,
allowing any user to query SSO status for any org; add the same org check used
in sso_configure.ts (call hasOrgRight(c, <orgId>, auth.userId, 'super_admin') or
the appropriate membership right) after extracting the target orgId from the
request (body or params), and if it returns false, return
simpleError('unauthorized', 'Organization access required'); ensure you
reference app.post and hasOrgRight so the check mirrors the existing
sso_configure.ts behavior.
- Around line 1-8: The JSDoc incorrectly describes this endpoint as GET with
query parameters while the code registers it with
app.post('/private/sso/status'); update the JSDoc to "@endpoint POST
/private/sso/status", change the description to indicate it accepts a request
body (not query parameters), and adjust the "Query Parameters" section to
"Request Body" / describe the expected JSON fields so the docs match the
implementation (ensure you update the top comment block above the app.post
handler and any param examples).
In @supabase/functions/_backend/private/sso_test.ts:
- Line 174: Replace the direct Hono instantiation for the exported app with the
shared initializer: import and call createHono(functionName, version, sentryDsn)
instead of using new Hono<MiddlewareKeyVariables>(), and assign its result to
export const app so the module benefits from request ID handling, worker source
headers, logging, and optional Sentry; ensure you pass the function name (e.g.,
"sso_test"), a version value (e.g., process.env.SOURCE_VERSION or a default),
and the Sentry DSN (process.env.SENTRY_DSN) and preserve the
MiddlewareKeyVariables typing.
In @supabase/functions/_backend/private/sso_update.ts:
- Around line 40-46: The JSDoc requires super_admin but sso_update.ts currently
only checks authentication; add the same super-admin permission guard used in
sso_configure.ts: import/ensure availability of hasOrgRight, then after
verifying auth?.userId call and await hasOrgRight(auth.userId, 'super_admin')
(or the project’s equivalent signature) and return a simpleError('forbidden',
'Requires super_admin permissions') if it returns false; mirror the placement
and error handling used in sso_configure.ts so sso_update.ts enforces the
super_admin check consistently.
In @supabase/functions/sso_check/index.ts:
- Around line 67-73: The code currently extracts the domain with
body.email.split('@')[1] into domain which can be an empty string (e.g.,
"user@"); update the validation so after confirming body.email contains '@' you
also verify the domain part is non-empty and well-formed before continuing:
parse the domain using e.g. indexOf/substring or split and check that domain &&
domain.trim().length > 0, normalize with toLowerCase(), and if invalid return a
400 error; reference the variables body.email and domain and the extraction
logic so you replace the unsafe split usage with a guarded extraction and early
error response.
- Around line 124-129: The catch block in the sso_check function currently
returns error.message to callers; change it to return a generic error message
(e.g., { available: false, error: "internal_server_error" } or "Internal server
error") while preserving status 500 and headers (corsHeaders and Content-Type),
and instead log the original error server-side (using the existing logger or
console.error) so internal details aren’t exposed to the public endpoint; update
the Response creation in the catch block (the new Response(...) call)
accordingly.
🧹 Nitpick comments (8)
supabase/functions/sso_check/index.ts (1)
38-41: Consider addingAccess-Control-Allow-Methodsto CORS headers.The CORS preflight response doesn't include
Access-Control-Allow-Methods. While browsers may still work, explicitly specifying allowed methods improves CORS compliance.Proposed fix
const corsHeaders = { 'Access-Control-Allow-Origin': '*', 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', + 'Access-Control-Allow-Methods': 'POST, OPTIONS', }supabase/functions/_backend/private/sso_test.ts (1)
337-343: External fetch calls lack timeout protection.The fetch to the metadata URL (and similarly the Supabase Auth API call at line 149) doesn't have a timeout. This could cause the endpoint to hang if the external server is slow or unresponsive.
Proposed fix using AbortController
try { + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 10000) // 10s timeout + const metadataResponse = await fetch(config.metadata_url, { headers: { Accept: 'application/xml, text/xml', }, + signal: controller.signal, }) + clearTimeout(timeoutId)supabase/functions/mock-sso-callback/index.ts (1)
270-273: RelayState is not sanitized before inclusion in redirect URL.While
sanitizeRedirectUrlexists and is applied to the full redirect URL inrenderSuccessPage, therelayStatevalue is URL-encoded but could still be manipulated. Since this is a local dev mock, the risk is minimal, but consider sanitizingrelayStateor documenting that it's only for local use.supabase/functions/_backend/private/sso_update.ts (1)
81-83: Unnecessary dynamic import - use static import instead.
getSSOStatuscan be imported statically at the top of the file alongsidessoUpdateSchemaandupdateSAMLfrom the same module. Dynamic imports should be reserved for conditional loading or code splitting scenarios.Proposed fix
-import { ssoUpdateSchema, updateSAML } from './sso_management.ts' +import { getSSOStatus, ssoUpdateSchema, updateSAML } from './sso_management.ts' // ... later in the handler ... - // Return updated configuration - const { getSSOStatus } = await import('./sso_management.ts') - const updatedConfig = await getSSOStatus(c, update.orgId) + // Return updated configuration + const updatedConfig = await getSSOStatus(c, update.orgId)supabase/functions/_backend/private/sso_management.ts (4)
234-241: Add timeout to external API call for consistency.While this is best-effort cleanup, a timeout prevents the cleanup from blocking indefinitely if the auth service is unresponsive.
739-753: Consider PII implications of storing email in audit logs.The
sso_audit_logs. Ensure audit log retention policies comply with GDPR/CCPA requirements for PII. Consider whether email is necessary for audit purposes or if user_id alone suffices.
524-538: Incomplete two-part TLD list for root domain extraction.The hardcoded list misses many common two-part TLDs (
.org.uk,.net.au,.co.jp,.com.cn, etc.). This could cause inconsistent domain uniqueness validation across different regional TLDs.Consider using a public suffix list library like
pslfor accurate TLD handling, or at minimum expand the list to cover more common cases.
484-512: Use Deno's built-in DOMParser for XML validation instead of string-based checks.The current string-matching approach blocks basic XXE vectors (
<!DOCTYPE,<!ENTITY,<
…undefined fix(migrations): remove redundant revoke on get_customer_counts for service_role
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* fix: make is_allowed_capgkey support hashed API keys Update is_allowed_capgkey and get_user_id functions to support both plain-text and hashed API keys using find_apikey_by_value(). Add expiration checks to prevent expired keys from passing validation. Add comprehensive tests for hashed key validation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: use find_apikey_by_value RPC in checkKey Refactor checkKey function to use the find_apikey_by_value SQL function instead of duplicating the hashing logic in JavaScript. This ensures consistent key lookup behavior between SQL functions and TypeScript code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: remove isSafeAlphanumeric check from checkKey Remove the isSafeAlphanumeric validation as it's no longer needed for security. The RPC call to find_apikey_by_value uses parameterized queries, which prevents SQL injection regardless of input characters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: remove isSafeAlphanumeric function Remove the isSafeAlphanumeric validation function as it's no longer needed. Both Supabase RPC calls and Drizzle ORM use parameterized queries which prevent SQL injection regardless of input characters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: use find_apikey_by_value in checkKeyPg Refactor checkKeyPg to use the find_apikey_by_value SQL function instead of manually hashing and querying. This ensures consistent key lookup behavior between all code paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * perf: optimize find_apikey_by_value to use single query Replace sequential two-query approach with a single query using OR. This reduces database round-trips and allows PostgreSQL to potentially use index union optimization. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: merge find_apikey_by_value optimization into main migration Consolidate the find_apikey_by_value query optimization (single query with OR instead of two sequential queries) into the original migration file for cleaner PR history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: add index signature to FindApikeyByValueResult type Drizzle's execute method requires the generic type to satisfy Record<string, unknown>, so added intersection with index signature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Haiku 4.5 <[email protected]>
…es across components and stores
- Introduced new keys for allowing and helping with bundle previews in Japanese, Korean, Polish, Brazilian Portuguese, Russian, Turkish, Vietnamese, and Simplified Chinese. - Added error messages for changing preview settings across all supported languages. - Included success messages for changing preview settings in all relevant languages. - Updated device names and other related strings to enhance user experience in the respective languages.
…oints - Updated password compliance validation to use authenticated Supabase client. - Refactored app deletion logic to utilize API key-based access for data queries. - Modified build request handling to enforce RLS with API key authentication. - Enhanced organization member management to ensure proper access control. - Streamlined webhook operations to leverage API key authentication for data access. - Improved statistics retrieval to support authenticated access based on user roles. - Adjusted tests to ensure proper error handling and access control scenarios.
Add comprehensive test coverage for CLI operations using hashed API keys. Tests verify that all core SDK operations (upload, list, channel management) work correctly with hashed API keys stored as SHA-256 hashes in the database. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Haiku 4.5 <[email protected]>
* security: remove passwords from all logs Ensure passwords are never logged to Cloudflare, Supabase, or Discord by: - Removing password field from cloudlog calls in accept_invitation and validate_password_compliance - Sanitizing Discord alerts to completely remove password field and partially redact other sensitive fields 🤖 Generated with Claude Code Co-Authored-By: Claude Haiku 4.5 <[email protected]> * fix: move password redaction after validation to handle null body Address PR feedback - if a client sends JSON null, destructuring before validation throws TypeError (500) instead of returning 400. Move cloudlog calls after safeParse validation to ensure body is valid before destructuring. 🤖 Generated with Claude Code Co-Authored-By: Claude Haiku 4.5 <[email protected]> * chore: remove deno.lock from commit 🤖 Generated with Claude Code Co-Authored-By: Claude Haiku 4.5 <[email protected]> --------- Co-authored-by: Claude Haiku 4.5 <[email protected]>
- Add Webhook type alias to webhooks store and Vue components - Fix apikey.key null check in build/start.ts
The generateEncryptionKeysSDK() function was modifying the project's capacitor.config.ts without cleanup. The SDK asynchronously writes the public key to the config during key generation, which was leaving the test environment dirty. Now we back up and restore the config in a finally block to ensure clean state regardless of test outcome. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Haiku 4.5 <[email protected]>
This adds a dynamic link to the Supabase dashboard for each Stripe customer, allowing easy navigation from Stripe to the customer's org record in Supabase. The link is parameterized with the SUPABASE_URL environment variable and supports both production and local Supabase instances. 🤖 Generated with Claude Code Co-authored-by: Claude Haiku 4.5 <[email protected]>
- Refactored webhook GET, POST, and PUT functions to utilize the capgkey from the context instead of the API key for authenticated client access. - Added new RLS policies to support anon role for webhooks and webhook deliveries, allowing API key-based authentication. - Updated seed data to include dedicated users and API keys for testing, ensuring isolation between tests. - Enhanced tests for CLI hashed API keys and RLS to prevent interference with other tests, using dedicated test data.
* Fix preview URL to use subdomain-based format
Update BundlePreviewFrame.vue to use the subdomain-based preview URL format (*.preview.{domain}) instead of the old path-based format (/files/preview/...). Also add localhost detection and show a warning when preview is not available on local development environments.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Haiku 4.5 <[email protected]>
* Make preview public (no auth token required)
- Remove token from preview URL in frontend
- Use supabaseAdmin instead of user auth in backend
- Preview is public when allow_preview setting is enabled
- Security relies on allow_preview flag and obscure subdomain
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
* Address PR review comments for preview URL
- Add lowercase normalization for DNS compliance
- Preserve environment segments (dev, preprod, staging) in base domain
- Use case-insensitive lookup for app_id in backend
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
---------
Co-authored-by: Claude Haiku 4.5 <[email protected]>
Adds a collapsible prerequisites accordion to the first onboarding step that explains what the CLI command will do and lists the requirements: Node.js 22+/Bun, Capacitor 5+, and locally built app. Translations added to all 15 supported languages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Haiku 4.5 <[email protected]>
* feat: add empty state overlay to dashboard when user has no apps Show dashboard with blur effect and centered overlay prompting user to add their first app, instead of redirecting to the apps page. This provides better UX by letting users see the dashboard structure while encouraging app creation. Added translations for all 15 languages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * fix: don't show empty state overlay when user has subscription or security issues The "no apps" overlay should only appear when the user is in good standing (paying, has security access) but simply has no apps. When subscription fails or security compliance is required, those alerts from FailedCard should be shown without the blur overlay covering them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> --------- Co-authored-by: Claude Haiku 4.5 <[email protected]>
Track device registrations by platform (iOS vs Android) in the last 30 days with separate counts in GlobalStat, LogSnag insights, and admin dashboard. Includes database migration, backend queries from Cloudflare Analytics Engine, and frontend UI components showing the platform breakdown. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Haiku 4.5 <[email protected]>
* feat: Add demo chart data when users have no apps or data Show realistic-looking placeholder data on dashboard charts when users don't have any apps or real data yet. This makes the interface more visually appealing and demonstrates what metrics will look like once they start using the platform. Each chart displays a sample data indicator to clarify that the data is not real. - Added demoChartData.ts service with generators for MAU, bandwidth, storage, updates, deployments, and bundle uploads - Updated all dashboard card components (UsageCard, UpdateStatsCard, DeploymentStatsCard, BundleUploadsCard) to show demo data when no real data exists - Added isDemoData prop to ChartCard with visual indicator overlay - Added i18n translation for demo data indicator message * chore: Add demo-data-indicator translation to all languages Added translations for the demo chart data indicator in: - German, Spanish, French, Hindi, Indonesian, Italian - Japanese, Korean, Polish, Portuguese (BR), Russian - Turkish, Vietnamese, Chinese (Simplified) 🤖 Generated with [Claude Code](https://claude.ai/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
…1390) - Add generateConsistentDemoData() to ensure chart totals match stacked per-app data - Add getDemoDayCount() to match demo data length with chart labels - Update UsageCard, BundleUploadsCard, DeploymentStatsCard to use consistent demo generation Addresses PR review feedback about demo totals not matching stacked chart data. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <[email protected]>
Track device platform (iOS vs Android) in device_usage Analytics Engine dataset instead of relying on device_info which has aggressive caching. This ensures platform breakdown stays fresh and matches the total device count in LogSnag insights. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Haiku 4.5 <[email protected]>
* fix: make is_allowed_capgkey support hashed API keys Update is_allowed_capgkey and get_user_id functions to support both plain-text and hashed API keys using find_apikey_by_value(). Add expiration checks to prevent expired keys from passing validation. Add comprehensive tests for hashed key validation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: use find_apikey_by_value RPC in checkKey Refactor checkKey function to use the find_apikey_by_value SQL function instead of duplicating the hashing logic in JavaScript. This ensures consistent key lookup behavior between SQL functions and TypeScript code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: remove isSafeAlphanumeric check from checkKey Remove the isSafeAlphanumeric validation as it's no longer needed for security. The RPC call to find_apikey_by_value uses parameterized queries, which prevents SQL injection regardless of input characters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: remove isSafeAlphanumeric function Remove the isSafeAlphanumeric validation function as it's no longer needed. Both Supabase RPC calls and Drizzle ORM use parameterized queries which prevent SQL injection regardless of input characters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: use find_apikey_by_value in checkKeyPg Refactor checkKeyPg to use the find_apikey_by_value SQL function instead of manually hashing and querying. This ensures consistent key lookup behavior between all code paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * perf: optimize find_apikey_by_value to use single query Replace sequential two-query approach with a single query using OR. This reduces database round-trips and allows PostgreSQL to potentially use index union optimization. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Haiku 4.5 <[email protected]> * refactor: merge find_apikey_by_value optimization into main migration Consolidate the find_apikey_by_value query optimization (single query with OR instead of two sequential queries) into the original migration file for cleaner PR history. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * fix: add index signature to FindApikeyByValueResult type Drizzle's execute method requires the generic type to satisfy Record<string, unknown>, so added intersection with index signature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> --------- Co-authored-by: Claude Haiku 4.5 <[email protected]>
- Refactored webhook GET, POST, and PUT functions to utilize the capgkey from the context instead of the API key for authenticated client access. - Added new RLS policies to support anon role for webhooks and webhook deliveries, allowing API key-based authentication. - Updated seed data to include dedicated users and API keys for testing, ensuring isolation between tests. - Enhanced tests for CLI hashed API keys and RLS to prevent interference with other tests, using dedicated test data.
Reverting commits that added stub types and re-enabled CI checks. These are infrastructure changes that should be separate from the SSO feature. The SSO backend code is correct - CI failures are pre-existing project issues. Boss can merge with admin override.
- Replace Hono instantiation with createHono helper for consistent Sentry middleware - Add AbortController 30s timeouts to GoTrue Admin API calls (POST/PUT) - Enforce HTTPS-only metadata URLs (remove http:// support) - Optimize domain uniqueness checks with batched query (fix N+1) - Add super_admin permission checks to update/remove endpoints - Add org membership check to status endpoint - Fix JSDoc documentation (GET → POST with body) - Enhance domain validation in sso_check endpoint - Sanitize error messages in sso_check (prevent info leakage) - Add SSO table schemas to postgres_schema.ts - Fix syntax errors in try/catch blocks - Fix null handling in metadata validation Security improvements per issue #12 requirements
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
supabase/functions/_backend/private/validate_password_compliance.ts (2)
14-18: Use Zod v4 standalone format schemas.According to Zod v4 documentation,
z.email()andz.uuid()are now standalone schemas and should be used directly instead of with.check().🔧 Proposed fix
const bodySchema = z.object({ - email: z.string().check(z.email()), + email: z.email(), password: z.string().check(z.minLength(1)), - org_id: z.string().check(z.uuid()), + org_id: z.uuid(), })Based on library documentation for Zod v4.
52-52: UsecreateHonohelper for consistent initialization.The coding guidelines require using
createHonofromutils/hono.tsfor all Hono application initialization to ensure consistent Sentry middleware setup.🔧 Proposed fix
+import { createHono } from '../utils/hono.ts'-export const app = new Hono<MiddlewareKeyVariables>() +export const app = createHono()As per coding guidelines for
supabase/functions/_backend/**/*.{ts,js}.supabase/functions/_backend/files/preview.ts (1)
70-70: UsecreateHonohelper for consistent middleware setup.Direct
Honoinstantiation bypasses the requiredcreateHonohelper, which ensures consistent Sentry middleware and other centralized configurations mentioned in the PR objectives.🔧 Proposed fix
+import { createHono } from '../utils/hono.ts' + -export const app = new Hono<MiddlewareKeyVariables>() +export const app = createHono()Based on coding guidelines and learnings.
supabase/functions/_backend/utils/cloudflare.ts (1)
49-96:trackDeviceUsageCFnow requiresplatform– tighten the typeThe new platform-aware write (
double1= 0/1) is useful and the cache logic is preserved. The function signature change is correctly applied at the sole call site instats.ts:16. However, tighten the type:
The parameter is typed as
platform: stringwhile the codebase already has aDatabase['public']['Enums']['platform_os']enum used consistently elsewhere. Update to:Suggested signature change
export async function trackDeviceUsageCF(c: Context, device_id: string, app_id: string, org_id: string, platform: string) {
- export async function trackDeviceUsageCF(
- c: Context,
- device_id: string,
- app_id: string,
- org_id: string,
- platform: Database['public']['Enums']['platform_os'],
- ) {
</details>This prevents accidental invalid values and aligns with existing enum usage in
plugin_parser.ts,channel_self.ts, anddevice/get.ts.
🤖 Fix all issues with AI agents
In @package.json:
- Line 4: Revert the manual change to the package.json "version" field (the
added "version": "12.90.0") so the repository does not contain a hard-coded
version; remove or restore the prior state of the "version" key in package.json
and rely on the CI/CD release pipeline (semantic-release) to manage version
bumps automatically.
In @supabase/functions/_backend/private/sso_management.ts:
- Around line 396-473: The code contains redundant nested try-catch blocks
around the fetch call (the outer try that wraps the inner try-catch which
already handles AbortError, re-thrown simple errors, and logging). Remove the
outer try { ... } catch { ... } wrapper and leave the inner try-catch that
surrounds the fetch (keeping the AbortController, timeoutId,
clearTimeout(timeoutId), the fetch to
`${supabaseUrl}/auth/v1/admin/sso/providers/${providerId}`, and the existing
error handling that uses cloudlog and simpleError). Ensure no duplicated catch
logic remains and that the AbortError and re-throw behavior for errors with
code/message are preserved for the single catch block.
- Line 1352: Replace the direct Hono instantiation with the shared helper:
import and call createHono and assign its return to app (e.g., export const app
= createHono<MiddlewareKeyVariables>()), ensuring you remove or replace the
direct new Hono(...) usage so the Sentry middleware and other shared setup in
utils/hono.ts are applied consistently.
- Around line 1424-1453: The /update POST handler currently trusts
middlewareAPISecret but lacks a super_admin authorization check; before calling
updateSAML, fetch and validate the caller's permissions (e.g., via the Context
user/session or a permission helper) to ensure they have super_admin rights for
the target orgId in the parsed ssoUpdateSchema payload, and if not return/throw
an authorization error; ensure this check occurs after parseBody/safeParse and
before calling updateSAML, and still properly closes the pgClient via
closeClient on errors.
- Around line 1459-1484: The /remove handler is missing a super_admin permission
check: after parsing the body (parseBody) and before calling removeSAML, use the
pgClient (from getPgClient) to verify the caller (from the
context/middlewareAPISecret identity) has super_admin rights for body.orgId; if
the check fails, call closeClient(c, pgClient) and throw a
forbidden/simpleError; keep the permission check inline between parseBody(...)
and cloudlog(...) so unauthorized requests cannot reach removeSAML.
In @supabase/functions/_backend/private/sso_remove.ts:
- Line 35: The Hono app is initialized without required arguments; replace
export const app = createHono() with the same pattern used in sso_configure.ts
and sso_status.ts so Sentry/request IDs work—e.g., call createHono('sso_remove',
VERSION) (ensuring the VERSION constant or equivalent is imported/available) so
the function name and version are passed into createHono.
In @supabase/functions/_backend/private/sso_status.ts:
- Around line 99-102: The endpoint currently returns only the first connection
(const config = connections[0] || null; return c.json(config)) which conflicts
with the docstring that specifies an object of shape { status: 'ok',
connections: Array<...> }; update the handler to return the documented response
shape by returning an object like { status: 'ok', connections } (preserving the
existing connections array) via c.json, or if you prefer the single-config
response, update the docstring to reflect returning a single connection (config)
— reference the variables connections, config and the c.json response to locate
where to change.
In @supabase/functions/_backend/private/sso_test.ts:
- Around line 214-215: pgClient is created via getPgClient(c, true) and never
closed; wrap the logic that uses pgClient and drizzleClient in a try/finally and
call closeClient(pgClient) in the finally to ensure the connection is cleaned
up. Import closeClient from ../utils/pg.ts at the top, move subsequent use of
pgClient/drizzleClient (e.g., calls that follow getDrizzleClient(pgClient)) into
the try block, and ensure the finally always invokes closeClient(pgClient) even
on errors.
- Around line 337-364: The metadata fetch lacks a timeout and can hang; update
the fetch block that calls fetch(config.metadata_url) (and uses
metadataResponse, metadataXml, validationErrors, cloudlog) to use an
AbortController with a predefined timeout (e.g., 5s), start a timer that calls
controller.abort() after the timeout, pass signal: controller.signal to fetch,
clear the timer on success, and in the catch path detect AbortError (or
error.name === 'AbortError') to push a clear timeout-specific validationErrors
entry and log an appropriate cloudlog message; ensure other errors still push
their messages as before.
- Around line 148-154: The GET call to the GoTrue Admin API (the fetch that sets
response from `${supabaseUrl}/auth/v1/admin/sso/providers/${providerId}`) lacks
an AbortController timeout; wrap this fetch with a new AbortController, pass
controller.signal into the fetch options, start a 30_000ms timer that calls
controller.abort(), and clear the timer after the fetch resolves/rejects to
avoid leaks; ensure error handling treats AbortError appropriately.
In @supabase/functions/_backend/private/validate_password_compliance.ts:
- Around line 66-67: The log is misleading: it logs the validated destructured
object named bodyWithoutPassword but labels it as "raw body" and uses rawBody;
change the log to accurately reflect the parsed/validated object by updating the
context string from 'validate_password_compliance raw body' to something like
'validate_password_compliance parsed body' and replace the property key rawBody
with parsedBody (i.e., cloudlog({ requestId: c.get('requestId'), context:
'validate_password_compliance parsed body', parsedBody: bodyWithoutPassword }))
so the message and property name match the actual data being logged.
In @supabase/functions/_backend/utils/postgres_schema.ts:
- Around line 141-188: The saml_domain_mappings table defines verified with a
default of true which makes new domain mappings trusted by default; change the
verified column definition in saml_domain_mappings so that verified:
boolean('verified').notNull().default(false) (i.e., default to false) and ensure
this schema change is kept in sync with the underlying SQL migration or
migrations that create/update saml_domain_mappings so the Drizzle schema and DB
defaults remain consistent.
In @supabase/functions/_backend/utils/version.ts:
- Line 1: The exported constant version was manually edited; revert that change
so the value comes from the automated script: restore the file to the
autogenerated state (undo the manual edit to the version constant) and do not
modify export const version directly; instead run or rely on update-version.js
(the automation that generates this file) to set version, and remove any manual
edits from the commit so CI can manage synchronization.
In @supabase/migrations/20260107210800_sso_saml_complete.sql:
- Around line 255-280: The function get_sso_provider_id_for_user currently runs
SECURITY DEFINER and is GRANTed broadly, allowing any authenticated caller to
read another user's sso_provider_id and it uses brittle ::uuid casts; fix by
restricting execution surface (either revoke EXECUTE for role authenticated so
only postgres/supabase_auth_admin/internal callers can call it, or add an
auth.uid() check inside get_sso_provider_id_for_user that returns NULL unless
auth.uid() = p_user_id) and make the metadata-to-UUID conversion defensive by
using NULLIF to treat empty strings as NULL and wrapping the cast in a small
BEGIN...EXCEPTION block that catches invalid_text_representation and sets
v_provider_id := NULL (apply same pattern to both raw_app_meta_data and
raw_user_meta_data lookups and ensure any triggers like
trigger_auto_join_on_user_create / update that call this function still behave
safely).
🧹 Nitpick comments (20)
messages/tr.json (1)
898-900: Inconsistent abbreviation of "minimum" across related keys.Line 898 uses the abbreviation "Min. eklenti sürümü" (Min. plugin version), while nearby lines 899-900 use the full words "Minimal güncelleme sürümü" and "Minimum uzunluk". For consistency and professionalism, consider standardizing to either:
- Always use the full word: "Minimum eklenti sürümü"
- Or always abbreviate in space-constrained contexts (though full words are generally preferred in Turkish UI)
Suggested fix for consistency
- "min-plugin-version": "Min. eklenti sürümü", + "min-plugin-version": "Minimum eklenti sürümü",src/components/dashboard/StepsApp.vue (1)
305-346: Enhance accordion accessibility with ARIA attributes.The accordion implementation lacks proper ARIA attributes, which are essential for screen reader users to understand the interactive state of the component.
♿ Proposed accessibility improvements
<div v-if="props.onboarding && step === 0" class="max-w-6xl mx-auto mt-8 sm:px-10"> <div class="overflow-hidden bg-white border border-gray-200 rounded-xl"> <button type="button" class="flex items-center justify-between w-full px-5 py-4 text-left transition-colors hover:bg-gray-50" + :aria-expanded="prerequisitesOpen" + aria-controls="prerequisites-content" @click="togglePrerequisites" > <div class="flex items-center gap-3"> <span class="text-lg font-semibold text-gray-900 font-pj">{{ t('onboarding-prerequisites-title') }}</span> <span class="text-sm text-gray-500">{{ t('onboarding-prerequisites-hint') }}</span> </div> <IconChevronDown class="w-5 h-5 text-gray-500 transition-transform duration-200" :class="{ 'rotate-180': prerequisitesOpen }" + aria-hidden="true" /> </button> <div v-show="prerequisitesOpen" + id="prerequisites-content" + role="region" + :aria-labelledby="'prerequisites-title'" class="px-5 pb-5 border-t border-gray-100" >Note: You'll also need to add an
id="prerequisites-title"to the title span on line 314 for thearia-labelledbyto work correctly.src/pages/settings/organization/Credits.vue (1)
684-684: Responsive form layout implemented correctly.The form container now properly adapts from vertical stacking on mobile to horizontal layout on larger screens, which improves the user experience across devices.
Minor note: unused utility class
The
sm:justify-betweenclass has no effect since the form contains only one child element. If you're not planning to add additional form-level elements, you can remove it:- <form class="flex w-full flex-col p-3 sm:flex-row sm:items-center sm:justify-between" @submit.prevent="handleBuyCredits"> + <form class="flex w-full flex-col p-3 sm:flex-row sm:items-center" @submit.prevent="handleBuyCredits">supabase/functions/_backend/files/preview.ts (1)
156-176: Consider optimizing sequential prefix queries.The current implementation makes up to 4 sequential database queries when searching for files with common prefixes. This could be optimized into a single query using an
INclause orORconditions.♻️ Proposed optimization
- else { - // Try with common prefixes (www/, public/, dist/) - const prefixesToTry = ['www/', 'public/', 'dist/', ''] - for (const prefix of prefixesToTry) { - const tryPath = prefix + filePath - if (tryPath === filePath) - continue // Already tried exact match - - const { data: prefixMatch, error: prefixError } = await supabase - .from('manifest') - .select('s3_path, file_name') - .eq('app_version_id', versionId) - .eq('file_name', tryPath) - .single() - - if (!prefixError && prefixMatch) { - manifestEntry = prefixMatch - cloudlog({ requestId: c.get('requestId'), message: 'found file with prefix', originalPath: filePath, foundPath: tryPath }) - break - } - } - } + else { + // Try with common prefixes in a single query + const prefixesToTry = ['www/', 'public/', 'dist/'] + const pathsToTry = prefixesToTry.map(prefix => prefix + filePath) + + const { data: matches, error: prefixError } = await supabase + .from('manifest') + .select('s3_path, file_name') + .eq('app_version_id', versionId) + .in('file_name', pathsToTry) + .limit(1) + + if (!prefixError && matches && matches.length > 0) { + manifestEntry = matches[0] + cloudlog({ requestId: c.get('requestId'), message: 'found file with prefix', originalPath: filePath, foundPath: manifestEntry.file_name }) + } + }src/pages/admin/dashboard/index.vue (1)
25-49: Align trend data types with nullable backend fields
globalStatsTrendDatamodelsdevices_last_month_ios/devices_last_month_androidas plainnumber, but the correspondingglobal_statscolumns arenumber | null. You already guard chart values with|| 0, so behavior is fine, but for type safety it would be more accurate to declare these asnumber | nullto match the backend and avoid surprises ifnullever leaks into new computations.Also applies to: 166-188
supabase/migrations/20260107210800_sso_saml_complete.sql (2)
20-59: Normalize domains and enforce HTTPS at the schema level for extra safetyTwo small but worthwhile hardening tweaks in the core SSO tables:
saml_domain_mappings.domainis compared againstlower(split_part(p_email, '@', 2)), but the column itself is not normalized. If a domain is stored with mixed case or whitespace, lookups will silently fail. Prefer enforcingLOWER()on write (via a CHECK or trigger) or comparing withLOWER(sdm.domain)in helper functions.org_saml_connections.metadata_urlis documented/used as an HTTPS IdP metadata URL, but the DB doesn’t enforce this. A simple CHECK likemetadata_url IS NULL OR metadata_url ~ '^https://'(or equivalent) would catch accidentalhttp://or malformed values early, in line with the “HTTPS-only metadata URLs” goal of the PR.These are not blockers but would improve robustness and alignment with the intended security posture.
Also applies to: 80-101, 166-168
235-253: Aligncheck_org_sso_configuredwithorg_has_sso_configuredand avoid unnecessary SECURITY DEFINERYou currently have two near-duplicate helpers:
check_org_sso_configured(p_org_id uuid)–SECURITY DEFINER, checksenabled = true.org_has_sso_configured(p_org_id uuid)– invoker, noenabledfilter.Concerns:
SECURITY DEFINERoncheck_org_sso_configuredisn’t needed if callers are already subject to RLS onorg_saml_connections. It also makes behavior less obvious (bypassing policies ifFORCE ROW LEVEL SECURITYis not enabled).- Having two similar functions with slightly different semantics (
enabledfilter vs any row) is easy to misuse.Consider simplifying to a single, invoker-based helper (or at least removing
SECURITY DEFINERhere), and documenting clearly whether “configured” means “row exists” or “row exists AND enabled = true”. This keeps the security model and call sites easier to reason about.Also applies to: 284-296
messages/id.json (1)
684-684: Optional: keep “Pixel” as an untranslated brandYou keep “iPhone” as a brand but translate
device-pixelas “Piksel”. If this label refers to the Google Pixel device frame rather than a generic pixel, consider using “Pixel” for brand consistency across devices.messages/es.json (1)
684-684: Optional: consider “Pixel” as a product name instead of “Píxel”If
device-pixelis meant to represent the Google Pixel device frame (likedevice-iphone), you may want to keep it as “Pixel” (brand) instead of the generic “Píxel” translation, for product naming consistency.messages/ja.json (1)
180-182: Preview/device-related Japanese strings look consistent; only an optional copy tweakAll newly added translations for preview toggles, checksum info, device labels, onboarding prerequisites, dashboard empty state, demo indicator, fullscreen, and QR preview are coherent and match surrounding style. If you want to be extra explicit, you could optionally change the
scan-qr-to-previewvalue to mention “QRコード” (for example,「QRコードをスキャンして、スマートフォンでプレビューしてください」) so users immediately know what they are scanning, but what you have is already acceptable.Also applies to: 316-317, 371-372, 423-426, 681-688, 742-743, 770-771, 797-802, 939-942, 949-950, 1146-1155, 1214-1214
src/pages/app/[package].bundle.[bundle].vue (1)
744-757: Consider adding keyboard accessibility for the tooltip.The tooltip button lacks focus/blur handlers for keyboard users. Consider adding
@focusand@blurto support keyboard navigation, and anaria-describedbyattribute pointing to the tooltip content.Suggested enhancement
<button type="button" class="inline-flex items-center px-2 py-0.5 text-xs font-medium rounded-full cursor-help" :class="{...}" + :aria-expanded="showChecksumTooltip" + aria-haspopup="true" @click="showChecksumTooltip = !showChecksumTooltip" @mouseenter="showChecksumTooltip = true" @mouseleave="showChecksumTooltip = false" + @focus="showChecksumTooltip = true" + @blur="showChecksumTooltip = false" >src/components/dashboard/UpdateStatsCard.vue (1)
83-84: Hardcoded demo data length may not match billing period.The demo data always generates 30 days of data, but when
useBillingPeriodis true, the chart displays a variable number of days based on the billing cycle. This could cause visual inconsistency between demo mode and real data mode.Consider using
getDemoDayCount(imported inDeploymentStatsCard.vue) to match the expected day count:-const demoStats = computed(() => generateDemoUpdateStatsData(30)) +const demoStats = computed(() => { + const days = props.useBillingPeriod ? updateData.value.length || 30 : 30 + return generateDemoUpdateStatsData(days) +})supabase/functions/_backend/private/sso_update.ts (1)
93-94: Unnecessary dynamic import.
getSSOStatusis exported from the same module (sso_management.ts) asssoUpdateSchemaandupdateSAML, which are already statically imported at line 33. The dynamic import adds runtime overhead without benefit.Suggested fix
import { createHono, parseBody, quickError, simpleError, useCors } from '../utils/hono.ts' import { middlewareV2 } from '../utils/hono_middleware.ts' import { cloudlog } from '../utils/logging.ts' import { hasOrgRight } from '../utils/supabase.ts' -import { ssoUpdateSchema, updateSAML } from './sso_management.ts' +import { getSSOStatus, ssoUpdateSchema, updateSAML } from './sso_management.ts'Then at line 93-94:
- // Return updated configuration - const { getSSOStatus } = await import('./sso_management.ts') - const updatedConfig = await getSSOStatus(c, update.orgId) + // Return updated configuration + const updatedConfig = await getSSOStatus(c, update.orgId)src/pages/dashboard.vue (1)
83-85: Consider avoiding rendering when blurred.The
Usagecomponent is still rendered even whenhasNoAppsis true (just blurred). For performance optimization, you could skip rendering entirely since it's not visible anyway.Optional optimization
- <div :class="{ 'blur-sm pointer-events-none select-none': hasNoApps }"> - <Usage v-if="!organizationStore.currentOrganizationFailed && !lacksSecurityAccess" /> - </div> + <Usage v-if="!organizationStore.currentOrganizationFailed && !lacksSecurityAccess && !hasNoApps" />However, if you want to keep the blurred effect visible behind the overlay for visual appeal, the current approach is fine.
src/components/BundlePreviewFrame.vue (1)
74-82: IP address handling may need expansion.The check
hostname !== '127.0.0.1'handles localhost but not other local/development IPs like192.168.x.xor10.x.x.x. These would be treated as domain names.More robust localhost detection
const hostname = window.location.hostname let baseDomain = 'capgo.app' - if (hostname.includes('.') && hostname !== '127.0.0.1') { + const isLocalDev = hostname === 'localhost' + || hostname === '127.0.0.1' + || hostname.startsWith('192.168.') + || hostname.startsWith('10.') + if (hostname.includes('.') && !isLocalDev) {src/components/dashboard/BundleUploadsCard.vue (1)
334-334: Consider a lighter key for re-render triggering.
JSON.stringify(effectiveBundleDataByApp)runs on every render cycle. A hash or length-based key would be more efficient while still triggering re-renders when data changes.Lighter key alternative
<BundleUploadsChart - :key="JSON.stringify(effectiveBundleDataByApp)" + :key="`${isDemoMode}-${Object.keys(effectiveBundleDataByApp).length}-${effectiveTotal}`"Or use a computed property that generates a simple hash.
supabase/functions/_backend/private/sso_test.ts (1)
54-102: String-based XML validation is fragile but acceptable for pre-flight checks.The regex-based parsing (e.g.,
metadataXml.match(/entityID=["']([^"']+)["']/)) can fail on:
- Namespaced attributes (
saml:entityID)- Different quoting styles
- CDATA sections
Since this is a pre-flight validation and actual SAML parsing happens in GoTrue, this approach is acceptable. Consider adding a warning in comments that this is heuristic validation.
src/components/dashboard/UsageCard.vue (1)
63-76: Title-based generator selection is fragile.The
getDataGeneratorfunction relies ontitle.toLowerCase()containing specific substrings. This could break if:
- Titles are translated to non-English
- Title wording changes (e.g., "Monthly Active Users" → "Active Monthly Users")
Consider passing the chart type as a prop instead.
Suggested approach
Add a
chartTypeprop:chartType: { type: String as PropType<'mau' | 'storage' | 'bandwidth'>, default: 'mau', },Then use it directly:
const generatorMap = { mau: generateDemoMauData, storage: generateDemoStorageData, bandwidth: generateDemoBandwidthData, } const generator = generatorMap[props.chartType] ?? generateDemoMauDatasrc/services/demoChartData.ts (1)
150-162: Consider evolution calculation for zero-to-nonzero transitions.The evolution calculation filters out zero days, which means a transition from 0 to a positive value won't be detected as growth. Consider whether this is intentional or if you want to capture zero-to-positive transitions.
Current behavior:
- Data:
[0, 0, 5]→ Returns 0% (only one non-zero day)- Data:
[5, 0, 10]→ Compares 10 vs 5 (skips the zero)💡 Alternative approach to include zero-to-positive transitions
export function calculateDemoEvolution(data: number[]): number { - const nonZeroDays = data.filter(count => count > 0) - if (nonZeroDays.length < 2) + if (data.length < 2) return 0 - const lastDayCount = nonZeroDays[nonZeroDays.length - 1] - const previousDayCount = nonZeroDays[nonZeroDays.length - 2] + const lastDayCount = data[data.length - 1] + const previousDayCount = data[data.length - 2] if (previousDayCount === 0) return lastDayCount > 0 ? 100 : 0 return ((lastDayCount - previousDayCount) / previousDayCount) * 100 }supabase/functions/_backend/private/sso_management.ts (1)
551-579: Consider using a proper XML parser for production.The metadata XML sanitization currently uses basic string checks. The comment on line 565 notes this should use a proper XML parser in production. While the current implementation provides basic protection, a full XML parser would be more robust.
Consider using a library like
fast-xml-parseror similar for proper XML validation and sanitization, especially for production deployments.
supabase/functions/_backend/private/validate_password_compliance.ts
Outdated
Show resolved
Hide resolved
| @@ -1,3 +1,3 @@ | |||
| export const version = '12.89.4' | |||
| export const version = '12.90.0' | |||
There was a problem hiding this comment.
Revert manual version edit—let automation handle this.
This file's own comment (line 3) states it's "automatically generated by the update-version.js script" and should not be edited manually. Manual changes bypass the intended automation and can cause inconsistencies with the release process.
Based on coding guidelines, the CI/CD pipeline manages version synchronization across the codebase.
🤖 Prompt for AI Agents
In @supabase/functions/_backend/utils/version.ts at line 1, The exported
constant version was manually edited; revert that change so the value comes from
the automated script: restore the file to the autogenerated state (undo the
manual edit to the version constant) and do not modify export const version
directly; instead run or rely on update-version.js (the automation that
generates this file) to set version, and remove any manual edits from the commit
so CI can manage synchronization.
- Accept incoming change: use const instead of let for supabaseAdmin - Merge origin/main into feature/sso-02-backend
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
…ensive UUID casting - Add auth.uid() check to prevent users reading other users' SSO provider IDs - Use NULLIF to treat empty strings as NULL before casting - Wrap UUID casts in BEGIN...EXCEPTION blocks to handle invalid formats gracefully - Returns NULL for invalid UUIDs instead of raising exceptions - Triggers still work safely as they run with SECURITY DEFINER bypassing auth check
- Add missing id fields (crypto.randomUUID()) to all SSO table inserts - Replace admin client with anon client in validate_password_compliance - Add super_admin permission checks to SSO management endpoints - Add closeClient in try/finally blocks for proper resource cleanup - Add AbortController timeouts (30s GoTrue, 5s metadata fetch) - Replace return simpleError with throw simpleError for auth failures - Make verifyCaptchaToken throw errors instead of returning - Replace direct Hono instantiation with createHono helper - Change saml_domain_mappings.verified default to false - Fix SSO status endpoint return value structure - Remove non-null assertion for hashed API keys - Revert manual version edits (CI/CD manages versioning)
Replace 'v_domain = ""' with 'NULLIF(trim(v_domain), '') IS NULL' - Follows SQL best practices for NULL comparisons - More portable across database engines - Also handles whitespace-only strings
Replace 'NEW.entity_id IS NULL OR NEW.entity_id = ""' with NULLIF pattern - Consistent with other NULL checks in the migration - Handles whitespace-only strings
- Remove useless try/catch wrapper in /configure handler - Add 5s timeout with AbortController to metadata fetch in sso_test.ts - Fix sso_update.ts to pass functionName and version to createHono - Remove unused pgClient from /configure handler All other issues mentioned in review were already addressed: - Batched domain uniqueness checks (no N+1 queries) - AbortController timeouts on all external API calls (30s for GoTrue, 5s for metadata) - Permission checks (super_admin) on all SSO management endpoints - HTTPS-only enforcement in metadataUrlSchema - Organization membership validation in sso_status - Proper domain validation and error handling in sso_check - createHono helper usage across all SSO endpoints - Resource cleanup with closeClient in try/finally blocks
There was a problem hiding this comment.
Actionable comments posted: 4
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/private/validate_password_compliance.ts (1)
2-2: UsecreateHonoinstead of directHonoinstantiation.Per coding guidelines, all Hono framework application initialization must use
createHonofromutils/hono.ts. Direct instantiation withnew Hono<MiddlewareKeyVariables>()bypasses any centralized configuration or middleware setup provided by the utility.Proposed fix
-import { Hono } from 'hono/tiny' -import { z } from 'zod/mini' -import { parseBody, quickError, simpleError, useCors } from '../utils/hono.ts' +import { z } from 'zod/mini' +import { createHono, parseBody, quickError, simpleError, useCors } from '../utils/hono.ts'-export const app = new Hono<MiddlewareKeyVariables>() +export const app = createHono()Also applies to: 52-52
🤖 Fix all issues with AI agents
In @supabase/migrations/20260107210800_sso_saml_complete.sql:
- Line 39: The migration sets the enabled column as "enabled boolean NOT NULL
DEFAULT false" while the TypeScript schema defines enabled:
boolean('enabled').notNull().default(true); make these consistent by updating
the migration's column default to DEFAULT true (or change the TS schema to
default(false) if the intended behavior is false) so both the SQL migration and
the TypeScript schema use the same default for the enabled column.
🧹 Nitpick comments (5)
supabase/functions/_backend/private/sso_management.ts (1)
1380-1420: Unnecessary database client creation in route handler.The handler creates a
pgClienton line 1382, but theupdateSAMLfunction creates and manages its own database client with proper cleanup in its finally block. This extra client is never used and the error handling attempts to close it, which is redundant.♻️ Simplify by removing unused client
app.post('/update', middlewareAPISecret, async (c: Context<MiddlewareKeyVariables>) => { const requestId = c.get('requestId') - const pgClient = getPgClient(c, true) - try { const body = await parseBody<z.infer<typeof ssoUpdateSchema>>(c) // Validate schema const result = ssoUpdateSchema.safeParse(body) if (!result.success) { throw simpleError('invalid_input', 'Invalid request body', { errors: result.error.issues }) } const config = result.data cloudlog({ requestId, message: '[SSO Update] Request received', orgId: config.orgId, providerId: config.providerId, }) // Verify caller has super_admin permissions for the organization const auth = c.get('auth') if (!auth?.userId) { throw simpleError('unauthorized', 'Authentication required', 401) } const hasPermission = await hasOrgRight(c, config.orgId, auth.userId, 'super_admin') if (!hasPermission) { throw simpleError('insufficient_permissions', 'Only super administrators can update SSO connections', 403) } const response = await updateSAML(c, config) return c.json(response, 200) - } - catch (error: any) { - await closeClient(c, pgClient) - throw error - } })The same pattern applies to the
/remove(lines 1426-1464) and/status(lines 1470-1494) routes.supabase/functions/_backend/private/sso_update.ts (3)
1-27: Docs & auth contract don’t match implementation (JWT-only vs current behavior).
The header comment says JWT-only, but the code doesn’t enforcec.get('auth')?.authType === 'jwt'(andmiddlewareV2(['all'])implies other auth modes). Also the documented “Response” shape doesn’t matchreturn c.json(config).Also applies to: 41-47
34-35: Avoid dynamic import + don’t return{}on missing updated config.
await import('./sso_management.ts')is avoidable here, and returning{}makes failures harder to detect/handle upstream. Prefer a normal import and a hard failure if no connection is returned after an update.Proposed diff
-import { ssoUpdateSchema, updateSAML } from './sso_management.ts' +import { getSSOStatus, ssoUpdateSchema, updateSAML } from './sso_management.ts' @@ - const { getSSOStatus } = await import('./sso_management.ts') - const updatedConfig = await getSSOStatus(c, update.orgId) - - // Return first connection (should only be one per org for now) - const config = updatedConfig[0] || {} - - return c.json(config) + const updatedConfig = await getSSOStatus(c, update.orgId) + const config = updatedConfig[0] + if (!config) { + return quickError(500, 'sso_update_inconsistent_state', 'SSO updated but no configuration found afterwards') + } + return c.json(config)Also applies to: 94-101
41-47: Consider enforcingauthType === 'jwt'explicitly (since you depend onuserId).
Right now API-key auth will always fail due to!auth?.userId, but that’s implicit. Making it explicit improves clarity and prevents future regressions ifmiddlewareV2changes auth payloads. (As per coding guidelines/learnings about checkingc.get('auth')?.authType.)Also applies to: 73-83
supabase/functions/_backend/private/sso_test.ts (1)
40-114: Metadata “validation” is best-effort and can false-pass/false-fail.
String/regex checks (includes,match) are fragile for XML; acceptable as a quick lint, but please set expectations in code/comments (and avoid claiming “parse” failures unless you actually parse XML).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/components/dashboard/StepsApp.vuesupabase/functions/_backend/private/invite_new_user_to_org.tssupabase/functions/_backend/private/sso_configure.tssupabase/functions/_backend/private/sso_management.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/public/build/start.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/sso_check/index.tssupabase/migrations/20260107210800_sso_saml_complete.sql
🚧 Files skipped from review as they are similar to previous changes (5)
- supabase/functions/_backend/private/sso_configure.ts
- supabase/functions/sso_check/index.ts
- src/components/dashboard/StepsApp.vue
- supabase/functions/_backend/private/invite_new_user_to_org.ts
- supabase/functions/_backend/public/build/start.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use single quotes and no semicolons per @antfu/eslint-config
Files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
supabase/functions/_backend/**
📄 CodeRabbit inference engine (CLAUDE.md)
Backend logic should be organized in
supabase/functions/_backend/with subdirectories for plugins, private endpoints, public endpoints, triggers, and utilities
Files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript strict mode with path aliases mapping
~/tosrc/
Files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
supabase/functions/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Supabase Edge Functions use Deno runtime
Files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
supabase/functions/_backend/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
supabase/functions/_backend/**/*.{ts,js}: Backend code must be placed insupabase/functions/_backend/as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
UsecreateHonofromutils/hono.tsfor all Hono framework application initialization and routing
All database operations must usegetPgClient()orgetDrizzleClient()fromutils/pg.tsfor PostgreSQL access during active migration to Cloudflare D1
All Hono endpoint handlers must acceptContext<MiddlewareKeyVariables>and usec.get('requestId'),c.get('apikey'), andc.get('auth')for request context
Use structured logging withcloudlog({ requestId: c.get('requestId'), message: '...' })for all backend logging
UsemiddlewareAPISecretfor internal API endpoints andmiddlewareKeyfor external API keys; validate againstowner_orgin theapikeystable
Checkc.get('auth')?.authTypeto determine authentication type ('apikey' vs 'jwt') in backend endpoints
Use Drizzle ORM query patterns withschemafrompostgress_schema.tsfor all database operations; usealiasV2()for self-joins or multiple table references
Files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
supabase/functions/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend ESLint must pass before commit; run
bun lint:backendfor backend files
Files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
supabase/migrations/**/*.sql
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Database migrations must be created with
supabase migration new <feature_slug>and never modify previously committed migrations
supabase/migrations/**/*.sql: When a feature requires schema changes, create a single migration file with the Supabase CLI (supabase migration new <feature_slug>) and keep editing that file until the feature ships; never edit previously committed migrations.
A migration that introduces a new table may include seed inserts for that table, but treat that seeding as part of the current feature and do not modify previously committed migrations.
Do not create new cron jobs; instead update theprocess_all_cron_tasksfunction in a new migration file to add your job if needed.
Files:
supabase/migrations/20260107210800_sso_saml_complete.sql
🧠 Learnings (18)
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `createHono` from `utils/hono.ts` for all Hono framework application initialization and routing
Applied to files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `middlewareAPISecret` for internal API endpoints and `middlewareKey` for external API keys; validate against `owner_org` in the `apikeys` table
Applied to files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Check `c.get('auth')?.authType` to determine authentication type ('apikey' vs 'jwt') in backend endpoints
Applied to files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/private/sso_update.tssupabase/migrations/20260107210800_sso_saml_complete.sqlsupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : All Hono endpoint handlers must accept `Context<MiddlewareKeyVariables>` and use `c.get('requestId')`, `c.get('apikey')`, and `c.get('auth')` for request context
Applied to files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
📚 Learning: 2025-12-24T14:11:10.256Z
Learnt from: WcaleNieWolny
Repo: Cap-go/capgo PR: 1300
File: supabase/migrations/20251224103713_2fa_enforcement.sql:409-539
Timestamp: 2025-12-24T14:11:10.256Z
Learning: In supabase/migrations for get_orgs_v6 and get_orgs_v7: The inner functions with user_id parameter (get_orgs_v6(uuid) and get_orgs_v7(uuid)) should NOT be granted to anon/authenticated roles as this allows any user to query other users' organizations; only the no-argument wrapper functions should be public as they perform authentication checks.
Applied to files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_update.tssupabase/migrations/20260107210800_sso_saml_complete.sqlsupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
📚 Learning: 2025-12-25T11:22:19.594Z
Learnt from: WcaleNieWolny
Repo: Cap-go/capgo PR: 1300
File: supabase/migrations/20251224103713_2fa_enforcement.sql:85-96
Timestamp: 2025-12-25T11:22:19.594Z
Learning: In the 2FA enforcement implementation for supabase/migrations: When an org has enforcing_2fa=true, all users including super_admins must have 2FA enabled before accessing any org functions (including check_org_members_2fa_enabled); this is intentional behavior to ensure consistent security enforcement without exceptions for admins.
Applied to files:
supabase/functions/_backend/private/sso_status.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_management.ts
📚 Learning: 2026-01-08T00:40:00.524Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T00:40:00.524Z
Learning: Applies to **/{api,server,src/server,backend}/**/*.{ts,tsx,js} : Never use the Supabase admin SDK (with service key) for user-facing APIs. Always use the client SDK with user authentication so RLS policies are enforced. The admin SDK should only be used for internal operations (triggers, CRON jobs, etc.), and when unavoidable, sanitize all user inputs carefully to prevent PostgREST query injection.
Applied to files:
supabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/private/sso_update.tssupabase/functions/_backend/private/sso_remove.tssupabase/functions/_backend/private/sso_management.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : All database operations must use `getPgClient()` or `getDrizzleClient()` from `utils/pg.ts` for PostgreSQL access during active migration to Cloudflare D1
Applied to files:
supabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/utils/postgres_schema.tssupabase/migrations/20260107210800_sso_saml_complete.sqlsupabase/functions/_backend/private/sso_test.tssupabase/functions/_backend/private/sso_management.ts
📚 Learning: 2025-12-05T17:34:25.556Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T17:34:25.556Z
Learning: Applies to supabase/functions/**/*.ts : Supabase Edge Functions use Deno runtime
Applied to files:
supabase/functions/_backend/private/validate_password_compliance.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/utils/postgress_schema.ts : Schema definitions must be placed in `utils/postgress_schema.ts` using Drizzle ORM and never edited in committed migration files
Applied to files:
supabase/functions/_backend/utils/postgres_schema.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use Drizzle ORM query patterns with `schema` from `postgress_schema.ts` for all database operations; use `aliasV2()` for self-joins or multiple table references
Applied to files:
supabase/functions/_backend/utils/postgres_schema.tssupabase/functions/_backend/private/sso_test.ts
📚 Learning: 2026-01-08T00:40:00.524Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T00:40:00.524Z
Learning: Applies to supabase/migrations/**/*.sql : A migration that introduces a new table may include seed inserts for that table, but treat that seeding as part of the current feature and do not modify previously committed migrations.
Applied to files:
supabase/functions/_backend/utils/postgres_schema.tssupabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2026-01-08T00:40:00.524Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T00:40:00.524Z
Learning: Applies to supabase/seed.sql : Updating `supabase/seed.sql` to back new or evolved tests is expected; keep fixtures focused on current behavior while leaving committed migrations unchanged.
Applied to files:
supabase/functions/_backend/utils/postgres_schema.ts
📚 Learning: 2025-12-25T11:22:13.039Z
Learnt from: WcaleNieWolny
Repo: Cap-go/capgo PR: 1300
File: supabase/migrations/20251224103713_2fa_enforcement.sql:85-96
Timestamp: 2025-12-25T11:22:13.039Z
Learning: In SQL migrations under the repository (e.g., supabase/migrations), enforce that when an org has enforcing_2fa=true, all users (including super_admins) must have 2FA enabled before accessing any org functions, including check_org_members_2fa_enabled. Do not grant admin exceptions to 2FA requirements. This ensures consistent security enforcement across all org-related operations; implement this rule within relevant migrations and associated stored procedures/tests.
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Backend code must be placed in `supabase/functions/_backend/` as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use structured logging with `cloudlog({ requestId: c.get('requestId'), message: '...' })` for all backend logging
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2026-01-08T00:40:00.524Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-08T00:40:00.524Z
Learning: Applies to **/{tests,__tests__,test,spec}/**/*.{ts,tsx,js} : Always cover database changes with Postgres-level tests and complement them with end-to-end tests for affected user flows.
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-23T01:19:15.067Z
Learnt from: riderx
Repo: Cap-go/capgo PR: 1297
File: src/components/dashboard/DeploymentBanner.vue:77-79
Timestamp: 2025-12-23T01:19:15.067Z
Learning: In the Cap-go codebase, the 'owner' role exists only at the organization level (used in src/stores/organization.ts), not as an app-level permission. App-level permissions use the user_min_right database enum which includes: read, upload, write, admin, super_admin (but NOT owner). When checking app permissions, never include 'owner' in the role checks.
Applied to files:
supabase/functions/_backend/private/sso_management.ts
🧬 Code graph analysis (4)
supabase/functions/_backend/private/sso_status.ts (12)
supabase/functions/_backend/private/sso_management.ts (2)
app(1315-1315)getSSOStatus(1254-1313)supabase/functions/_backend/private/sso_test.ts (1)
app(180-180)supabase/functions/_backend/private/sso_update.ts (1)
app(37-37)supabase/functions/_backend/private/invite_new_user_to_org.ts (1)
app(32-32)supabase/functions/_backend/private/sso_configure.ts (1)
app(37-37)supabase/functions/_backend/private/validate_password_compliance.ts (1)
app(52-52)supabase/functions/_backend/utils/hono.ts (2)
createHono(137-196)useCors(43-47)supabase/functions/_backend/utils/version.ts (1)
version(1-1)supabase/functions/_backend/utils/hono_middleware.ts (1)
middlewareV2(213-237)scripts/snippet/cloudflare-snippet-filter-appid.js (1)
body(65-65)supabase/functions/_backend/utils/logging.ts (1)
cloudlog(3-15)supabase/functions/_backend/utils/supabase.ts (1)
hasOrgRight(287-304)
supabase/functions/_backend/private/validate_password_compliance.ts (2)
supabase/functions/_backend/utils/logging.ts (1)
cloudlog(3-15)supabase/functions/_backend/utils/supabase.ts (1)
emptySupabase(63-72)
supabase/functions/_backend/private/sso_update.ts (5)
supabase/functions/_backend/private/sso_management.ts (4)
app(1315-1315)ssoUpdateSchema(468-478)updateSAML(1021-1167)getSSOStatus(1254-1313)supabase/functions/_backend/utils/hono.ts (1)
createHono(137-196)supabase/functions/_backend/utils/version.ts (1)
version(1-1)supabase/functions/_backend/utils/hono_middleware.ts (1)
middlewareV2(213-237)supabase/functions/_backend/utils/supabase.ts (1)
hasOrgRight(287-304)
supabase/functions/_backend/private/sso_remove.ts (6)
supabase/functions/_backend/private/sso_management.ts (2)
app(1315-1315)removeSAML(1178-1245)supabase/functions/_backend/utils/hono.ts (2)
createHono(137-196)quickError(224-237)supabase/functions/_backend/utils/version.ts (1)
version(1-1)supabase/functions/_backend/utils/hono_middleware.ts (1)
middlewareV2(213-237)supabase/functions/_backend/utils/logging.ts (1)
cloudlog(3-15)supabase/functions/_backend/utils/supabase.ts (1)
hasOrgRight(287-304)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run tests
🔇 Additional comments (7)
supabase/functions/_backend/private/validate_password_compliance.ts (5)
65-67: LGTM!Good security practice - destructuring to exclude the password from the logged payload, and using structured logging with
requestIdas per coding guidelines.
94-100: Good security improvement using anon client for authentication.Using
emptySupabase(c)(anon client) instead of the admin client forsignInWithPasswordfollows the guideline to never use the admin SDK for user-facing APIs. Minor note: the comment mentions RLS enforcement, but RLS applies to database queries rather than Auth operations. The real benefit here is avoiding admin capability exposure during credential verification. The subsequent use of an authenticated client (line 110) for database queries is the correct pattern.
109-122: LGTM!Correct pattern - using an authenticated client with the user's session token ensures RLS policies are enforced for the membership verification query.
20-50: LGTM!The password validation logic is clean and correctly implements all policy checks with clear, user-friendly error messages.
139-165: LGTM!The compliance record upsert logic is correct, using the authenticated client to enforce RLS and handling errors with structured logging.
supabase/functions/_backend/private/sso_update.ts (1)
41-112: Route mounting and response format verified. The app is correctly mounted atPUT /private/sso/updateviaappGlobal.route('/sso/update', sso_update)insupabase/functions/private/index.ts:61, and the endpoint correctly returns the full config object fromgetSSOStatusrather than a simplified response.supabase/functions/_backend/private/sso_test.ts (1)
121-177: No changes needed—service-role usage is appropriate for this internal endpoint.This is a private/internal endpoint (not user-facing), and per the codebase learnings, using the Supabase admin SDK is correct for internal operations. The response schema from Supabase GoTrue confirms that
saml.entity_idanddomainsfields exist as documented. The code safely accesses these fields using optional chaining (?.), making defensive programming unnecessary but not harmful.
- Add org authorization check requiring super_admin permission - Add SSRF protection for metadata URL fetching (block private networks) - Add 5MB payload size limit for metadata responses - Make state changes opt-in via applyFixes parameter (enable, verified, entity_id updates) - Fix migration: change enabled column default from false to true to match TypeScript schema
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @supabase/functions/_backend/private/sso_test.ts:
- Around line 364-398: The SSRF checks around hostname, url.protocol,
validationErrors and cloudlog are incomplete: extend the
hostname/private-network validation in the same block that currently checks
localhost/172./192./10./[::] to also reject IPv4 link-local 169.254.* (including
the metadata IP 169.254.169.254) and IPv6 link-local addresses in fe80::/10 (and
their bracketed forms) by adding explicit startsWith/equals checks for
'169.254.' and a fe80 prefix check, and log/reject them the same way; also
tighten the protocol check on url.protocol so it rejects non-HTTPS by removing
'http:' and only allowing 'https:' (update the validationErrors messages
accordingly) while keeping cloudlog usage for blocked attempts (refer to
hostname, url.protocol, validationErrors, and cloudlog).
🧹 Nitpick comments (4)
supabase/functions/_backend/private/sso_test.ts (1)
42-45: Remove unnecessaryasynckeyword.The
validateSAMLMetadatafunction is markedasyncbut contains noawaitstatements. This adds unnecessary overhead by wrapping the return value in a Promise.Suggested fix
-async function validateSAMLMetadata( +function validateSAMLMetadata( metadataXml: string, expectedEntityId: string, -): Promise<{ valid: boolean, errors: string[], warnings: string[] }> { +): { valid: boolean, errors: string[], warnings: string[] } {supabase/migrations/20260107210800_sso_saml_complete.sql (3)
310-326: Consider adding authorization toorg_has_sso_configured.This function allows any authenticated user to check if any organization has SSO configured, which could leak information about which organizations use SSO. Consider adding an authorization check similar to
get_sso_provider_id_for_user, or document this as intentional public information.What organization information should be public vs private in SaaS applications?
436-446: Auth check relies on subtle NULL semantics.The authorization check
IF p_user_id != auth.uid() THEN RAISE EXCEPTIONrelies on SQL NULL comparison behavior. When called from triggers during user creation,auth.uid()is NULL, andp_user_id != NULLevaluates to NULL (not TRUE), so the exception is not raised.This is technically correct but fragile. If someone refactors this to
IF p_user_id IS DISTINCT FROM auth.uid(), it would break the trigger flow.Consider making the intent explicit:
Suggested clarification
-- Validate caller identity: p_user_id must match authenticated user + -- Note: In trigger context, auth.uid() is NULL, so this check passes + -- due to SQL NULL comparison semantics (NULL != value = NULL, not TRUE) IF p_user_id != auth.uid() THEN RAISE EXCEPTION 'Unauthorized: user_id mismatch'; END IF;Or make it explicit:
- IF p_user_id != auth.uid() THEN + -- Skip auth check in trigger context (auth.uid() is NULL) + IF auth.uid() IS NOT NULL AND p_user_id != auth.uid() THEN RAISE EXCEPTION 'Unauthorized: user_id mismatch'; END IF;
1017-1022: Inconsistent GRANT fororg_saml_connections.The
anonrole is granted SELECT onorg_saml_connections, but none of the RLS policies allow access foranon(onlyauthenticated). This is harmless since RLS blocks access, but it's inconsistent withsaml_domain_mappingswhich has both the grant and a matching RLS policy foranon.Consider removing
anonfrom this grant for consistency:Suggested fix
--- Grant access to tables -GRANT SELECT ON public.org_saml_connections TO authenticated, anon; +GRANT SELECT ON public.org_saml_connections TO authenticated; GRANT SELECT ON public.saml_domain_mappings TO authenticated, anon;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supabase/functions/_backend/private/sso_test.tssupabase/migrations/20260107210800_sso_saml_complete.sql
🧰 Additional context used
📓 Path-based instructions (9)
supabase/functions/_backend/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
supabase/functions/_backend/**/*.{ts,js}: Backend code must be placed insupabase/functions/_backend/as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
UsecreateHonofromutils/hono.tsfor all Hono framework application initialization and routing
All database operations must usegetPgClient()orgetDrizzleClient()fromutils/pg.tsfor PostgreSQL access during active migration to Cloudflare D1
All Hono endpoint handlers must acceptContext<MiddlewareKeyVariables>and usec.get('requestId'),c.get('apikey'), andc.get('auth')for request context
Use structured logging withcloudlog({ requestId: c.get('requestId'), message: '...' })for all backend logging
UsemiddlewareAPISecretfor internal API endpoints andmiddlewareKeyfor external API keys; validate againstowner_orgin theapikeystable
Checkc.get('auth')?.authTypeto determine authentication type ('apikey' vs 'jwt') in backend endpoints
Use Drizzle ORM query patterns withschemafrompostgress_schema.tsfor all database operations; usealiasV2()for self-joins or multiple table references
Files:
supabase/functions/_backend/private/sso_test.ts
supabase/functions/**/*.{ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Backend ESLint must pass before commit; run
bun lint:backendfor backend files
Files:
supabase/functions/_backend/private/sso_test.ts
**/*.{vue,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintto lint Vue, TypeScript, and JavaScript files; usebun lint:fixto auto-fix issues
Files:
supabase/functions/_backend/private/sso_test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use single quotes and no semicolons per @antfu/eslint-config rules
Files:
supabase/functions/_backend/private/sso_test.ts
supabase/functions/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
supabase/functions/**/*.ts: Never use the Supabase admin SDK with service key for user-facing APIs; always use client SDK with user authentication to enforce RLS policies. Admin SDK should only be used for internal operations (triggers, CRON jobs, etc.)
When admin access is unavoidable for a user-facing endpoint, sanitize all user inputs carefully to prevent PostgREST query injection
Files:
supabase/functions/_backend/private/sso_test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintor lint/format command before validating any backend or frontend task to ensure consistent formatting
Files:
supabase/functions/_backend/private/sso_test.ts
supabase/functions/_backend/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Place core backend logic in supabase/functions/_backend/ with plugins, private, public, triggers, and utils subdirectories
Files:
supabase/functions/_backend/private/sso_test.ts
supabase/migrations/**/*.sql
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Database migrations must be created with
supabase migration new <feature_slug>and never modify previously committed migrations
Files:
supabase/migrations/20260107210800_sso_saml_complete.sql
supabase/migrations/*.sql
📄 CodeRabbit inference engine (AGENTS.md)
supabase/migrations/*.sql: Use Supabase CLI for every migration and operational task; avoid manual changes through dashboard or direct SQL
Create a single migration file per feature usingsupabase migration new <feature_slug>and keep editing that file until the feature ships; never edit previously committed migrations
Never edit previously committed migrations; treat new seeding in migrations as part of the current feature
Do not create new cron jobs; instead update the process_all_cron_tasks function in a new migration file to add your job if needed
Useget_identity_org_appid()for RLS policies when app_id exists on the table instead ofget_identity()directly
Only useget_identity_org_allowed()as a last resort when the table genuinely has no app_id column and there is no way to join to get an app_id
Files:
supabase/migrations/20260107210800_sso_saml_complete.sql
🧠 Learnings (22)
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Check `c.get('auth')?.authType` to determine authentication type ('apikey' vs 'jwt') in backend endpoints
Applied to files:
supabase/functions/_backend/private/sso_test.tssupabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `createHono` from `utils/hono.ts` for all Hono framework application initialization and routing
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use `middlewareAPISecret` for internal API endpoints and `middlewareKey` for external API keys; validate against `owner_org` in the `apikeys` table
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : All Hono endpoint handlers must accept `Context<MiddlewareKeyVariables>` and use `c.get('requestId')`, `c.get('apikey')`, and `c.get('auth')` for request context
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/functions/_backend/**/*.ts : Place core backend logic in supabase/functions/_backend/ with plugins, private, public, triggers, and utils subdirectories
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Backend code must be placed in `supabase/functions/_backend/` as shared code deployed to Cloudflare Workers (API/Plugin/Files workers), Supabase Edge Functions, and other platforms
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use structured logging with `cloudlog({ requestId: c.get('requestId'), message: '...' })` for all backend logging
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : All database operations must use `getPgClient()` or `getDrizzleClient()` from `utils/pg.ts` for PostgreSQL access during active migration to Cloudflare D1
Applied to files:
supabase/functions/_backend/private/sso_test.tssupabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/**/*.{ts,js} : Use Drizzle ORM query patterns with `schema` from `postgress_schema.ts` for all database operations; use `aliasV2()` for self-joins or multiple table references
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-24T14:11:10.256Z
Learnt from: WcaleNieWolny
Repo: Cap-go/capgo PR: 1300
File: supabase/migrations/20251224103713_2fa_enforcement.sql:409-539
Timestamp: 2025-12-24T14:11:10.256Z
Learning: In supabase/migrations for get_orgs_v6 and get_orgs_v7: The inner functions with user_id parameter (get_orgs_v6(uuid) and get_orgs_v7(uuid)) should NOT be granted to anon/authenticated roles as this allows any user to query other users' organizations; only the no-argument wrapper functions should be public as they perform authentication checks.
Applied to files:
supabase/functions/_backend/private/sso_test.tssupabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2025-12-25T11:22:19.594Z
Learnt from: WcaleNieWolny
Repo: Cap-go/capgo PR: 1300
File: supabase/migrations/20251224103713_2fa_enforcement.sql:85-96
Timestamp: 2025-12-25T11:22:19.594Z
Learning: In the 2FA enforcement implementation for supabase/migrations: When an org has enforcing_2fa=true, all users including super_admins must have 2FA enabled before accessing any org functions (including check_org_members_2fa_enabled); this is intentional behavior to ensure consistent security enforcement without exceptions for admins.
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2025-12-23T01:19:15.067Z
Learnt from: riderx
Repo: Cap-go/capgo PR: 1297
File: src/components/dashboard/DeploymentBanner.vue:77-79
Timestamp: 2025-12-23T01:19:15.067Z
Learning: In the Cap-go codebase, the 'owner' role exists only at the organization level (used in src/stores/organization.ts), not as an app-level permission. App-level permissions use the user_min_right database enum which includes: read, upload, write, admin, super_admin (but NOT owner). When checking app permissions, never include 'owner' in the role checks.
Applied to files:
supabase/functions/_backend/private/sso_test.ts
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/seed.sql : Update supabase/seed.sql for new or evolved tests; keep fixtures focused on current behavior while leaving committed migrations unchanged
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2025-12-25T11:22:13.039Z
Learnt from: WcaleNieWolny
Repo: Cap-go/capgo PR: 1300
File: supabase/migrations/20251224103713_2fa_enforcement.sql:85-96
Timestamp: 2025-12-25T11:22:13.039Z
Learning: In SQL migrations under the repository (e.g., supabase/migrations), enforce that when an org has enforcing_2fa=true, all users (including super_admins) must have 2FA enabled before accessing any org functions, including check_org_members_2fa_enabled. Do not grant admin exceptions to 2FA requirements. This ensures consistent security enforcement across all org-related operations; implement this rule within relevant migrations and associated stored procedures/tests.
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/migrations/*.sql : Use `get_identity_org_appid()` for RLS policies when app_id exists on the table instead of `get_identity()` directly
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/migrations/*.sql : Only use `get_identity_org_allowed()` as a last resort when the table genuinely has no app_id column and there is no way to join to get an app_id
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/functions/_backend/utils/postgress_schema.ts : Schema definitions must be placed in `utils/postgress_schema.ts` using Drizzle ORM and never edited in committed migration files
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/functions/**/*.ts : When admin access is unavoidable for a user-facing endpoint, sanitize all user inputs carefully to prevent PostgREST query injection
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/functions/**/*.ts : Never use the Supabase admin SDK with service key for user-facing APIs; always use client SDK with user authentication to enforce RLS policies. Admin SDK should only be used for internal operations (triggers, CRON jobs, etc.)
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/migrations/*.sql : Use Supabase CLI for every migration and operational task; avoid manual changes through dashboard or direct SQL
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2025-12-23T02:53:12.055Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-23T02:53:12.055Z
Learning: Applies to supabase/migrations/**/*.sql : Database migrations must be created with `supabase migration new <feature_slug>` and never modify previously committed migrations
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
📚 Learning: 2026-01-10T04:55:25.264Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.264Z
Learning: Applies to supabase/migrations/*.sql : Never edit previously committed migrations; treat new seeding in migrations as part of the current feature
Applied to files:
supabase/migrations/20260107210800_sso_saml_complete.sql
🔇 Additional comments (10)
supabase/functions/_backend/private/sso_test.ts (4)
22-37: LGTM!Imports correctly follow the coding guidelines: uses
createHonofromutils/hono.ts,getPgClient/getDrizzleClientfromutils/pg.ts, andcloudlogfromutils/logging.ts. The Zod schema correctly uses Zod 4 syntax with.optional().default(false).
118-179: LGTM with minor note.Good implementation with proper timeout handling via
AbortControllerand local development bypass. The service role key usage is appropriate here for internal SSO provider verification.
190-233: LGTM!Proper authentication check, request validation, and authorization with
super_adminpermission requirement. Good use of structured logging withrequestId. As per coding guidelines, correctly usesgetPgClient()andgetDrizzleClient().
596-610: LGTM!Proper error handling with structured logging and correct resource cleanup in the
finallyblock usingcloseClient. This follows the coding guidelines for database connection management.supabase/migrations/20260107210800_sso_saml_complete.sql (6)
20-59: LGTM!Good table structure with appropriate constraints: unique org_id (one SSO per org), unique entity_id, and CHECK constraint for metadata presence. The
enableddefault oftruealigns with the PR objectives.One consideration: the
created_byFK toauth.usershas noON DELETEaction, meaning if the creating user is deleted, the reference will break. ConsiderON DELETE SET NULLif user deletion is possible.
331-412: LGTM!Well-designed lookup functions with proper domain extraction, verification checks, and ordering by priority. Correctly marked as
SECURITY DEFINERwithSET search_path = publicfor security. Theanongrant is appropriate for pre-login SSO detection.
678-763: LGTM!The
enforce_sso_for_domainsfunction has comprehensive logic:
- Checks user metadata for SSO provider ID
- Falls back to auth.identities check for SSO signup
- Only enforces SSO block for non-SSO signups on configured domains
The metadata bypass (lines 711-739) correctly allows users who already have SSO provider metadata to proceed.
900-941: LGTM!RLS policies are well-structured:
- Super admins can manage SSO connections (appropriate for sensitive config)
- Org members can read SSO status (needed for UI)
- Uses
check_min_rightswithget_identity_org_allowedas expected per coding guidelines
1051-1071: LGTM!Good security hygiene: trigger functions have EXECUTE revoked from PUBLIC and only granted to
postgresandsupabase_auth_admin. This prevents direct invocation by users while allowing database triggers to function properly.
80-101: The domain uniqueness constraint is correctly designed for multi-provider support.The UNIQUE constraint on
(domain, sso_connection_id)is intentional and appropriate. The migration file explicitly states this table "supports multi-provider setups" (line 78), thepriorityfield is documented for "Display order when multiple providers exist" (line 106), and the lookup functions search by domain to retrieve available providers. The constraint correctly prevents duplicate mappings of the same domain to the same connection while allowing a single domain to route to multiple SSO providers—which is the documented design requirement.Likely an incorrect or invalid review comment.
| if ( | ||
| hostname === 'localhost' | ||
| || hostname === '127.0.0.1' | ||
| || hostname === '0.0.0.0' | ||
| || hostname.startsWith('192.168.') | ||
| || hostname.startsWith('10.') | ||
| || hostname.startsWith('172.16.') | ||
| || hostname.startsWith('172.17.') | ||
| || hostname.startsWith('172.18.') | ||
| || hostname.startsWith('172.19.') | ||
| || hostname.startsWith('172.20.') | ||
| || hostname.startsWith('172.21.') | ||
| || hostname.startsWith('172.22.') | ||
| || hostname.startsWith('172.23.') | ||
| || hostname.startsWith('172.24.') | ||
| || hostname.startsWith('172.25.') | ||
| || hostname.startsWith('172.26.') | ||
| || hostname.startsWith('172.27.') | ||
| || hostname.startsWith('172.28.') | ||
| || hostname.startsWith('172.29.') | ||
| || hostname.startsWith('172.30.') | ||
| || hostname.startsWith('172.31.') | ||
| || hostname === '[::]' | ||
| || hostname === '[::1]' | ||
| ) { | ||
| validationErrors.push('Metadata URL cannot point to private network addresses') | ||
| cloudlog({ | ||
| requestId, | ||
| message: '[SSO Test] Blocked private network URL', | ||
| url: hostname, | ||
| }) | ||
| } | ||
| else if (url.protocol !== 'https:' && url.protocol !== 'http:') { | ||
| validationErrors.push('Metadata URL must use HTTP or HTTPS protocol') | ||
| } |
There was a problem hiding this comment.
SSRF protection is incomplete.
The private network blocklist is missing some address ranges that could be exploited:
169.254.x.x(IPv4 link-local / APIPA)fe80::/10(IPv6 link-local)- Cloud metadata endpoints like
169.254.169.254(AWS/GCP/Azure metadata service)
Additionally, Line 396 allows HTTP protocol which contradicts the PR objective mentioning "HTTPS-only metadata URLs" and poses a security risk (MITM attacks on metadata).
Suggested fix for comprehensive SSRF protection
|| hostname.startsWith('172.31.')
|| hostname === '[::]'
|| hostname === '[::1]'
+ || hostname.startsWith('169.254.')
+ || hostname.startsWith('fe80:')
+ || hostname === '169.254.169.254' // Cloud metadata endpoint
) {
validationErrors.push('Metadata URL cannot point to private network addresses')
cloudlog({
requestId,
message: '[SSO Test] Blocked private network URL',
url: hostname,
})
}
- else if (url.protocol !== 'https:' && url.protocol !== 'http:') {
- validationErrors.push('Metadata URL must use HTTP or HTTPS protocol')
+ else if (url.protocol !== 'https:') {
+ validationErrors.push('Metadata URL must use HTTPS protocol')
}📝 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.
| if ( | |
| hostname === 'localhost' | |
| || hostname === '127.0.0.1' | |
| || hostname === '0.0.0.0' | |
| || hostname.startsWith('192.168.') | |
| || hostname.startsWith('10.') | |
| || hostname.startsWith('172.16.') | |
| || hostname.startsWith('172.17.') | |
| || hostname.startsWith('172.18.') | |
| || hostname.startsWith('172.19.') | |
| || hostname.startsWith('172.20.') | |
| || hostname.startsWith('172.21.') | |
| || hostname.startsWith('172.22.') | |
| || hostname.startsWith('172.23.') | |
| || hostname.startsWith('172.24.') | |
| || hostname.startsWith('172.25.') | |
| || hostname.startsWith('172.26.') | |
| || hostname.startsWith('172.27.') | |
| || hostname.startsWith('172.28.') | |
| || hostname.startsWith('172.29.') | |
| || hostname.startsWith('172.30.') | |
| || hostname.startsWith('172.31.') | |
| || hostname === '[::]' | |
| || hostname === '[::1]' | |
| ) { | |
| validationErrors.push('Metadata URL cannot point to private network addresses') | |
| cloudlog({ | |
| requestId, | |
| message: '[SSO Test] Blocked private network URL', | |
| url: hostname, | |
| }) | |
| } | |
| else if (url.protocol !== 'https:' && url.protocol !== 'http:') { | |
| validationErrors.push('Metadata URL must use HTTP or HTTPS protocol') | |
| } | |
| if ( | |
| hostname === 'localhost' | |
| || hostname === '127.0.0.1' | |
| || hostname === '0.0.0.0' | |
| || hostname.startsWith('192.168.') | |
| || hostname.startsWith('10.') | |
| || hostname.startsWith('172.16.') | |
| || hostname.startsWith('172.17.') | |
| || hostname.startsWith('172.18.') | |
| || hostname.startsWith('172.19.') | |
| || hostname.startsWith('172.20.') | |
| || hostname.startsWith('172.21.') | |
| || hostname.startsWith('172.22.') | |
| || hostname.startsWith('172.23.') | |
| || hostname.startsWith('172.24.') | |
| || hostname.startsWith('172.25.') | |
| || hostname.startsWith('172.26.') | |
| || hostname.startsWith('172.27.') | |
| || hostname.startsWith('172.28.') | |
| || hostname.startsWith('172.29.') | |
| || hostname.startsWith('172.30.') | |
| || hostname.startsWith('172.31.') | |
| || hostname === '[::]' | |
| || hostname === '[::1]' | |
| || hostname.startsWith('169.254.') | |
| || hostname.startsWith('fe80:') | |
| || hostname === '169.254.169.254' // Cloud metadata endpoint | |
| ) { | |
| validationErrors.push('Metadata URL cannot point to private network addresses') | |
| cloudlog({ | |
| requestId, | |
| message: '[SSO Test] Blocked private network URL', | |
| url: hostname, | |
| }) | |
| } | |
| else if (url.protocol !== 'https:') { | |
| validationErrors.push('Metadata URL must use HTTPS protocol') | |
| } |
🤖 Prompt for AI Agents
In @supabase/functions/_backend/private/sso_test.ts around lines 364 - 398, The
SSRF checks around hostname, url.protocol, validationErrors and cloudlog are
incomplete: extend the hostname/private-network validation in the same block
that currently checks localhost/172./192./10./[::] to also reject IPv4
link-local 169.254.* (including the metadata IP 169.254.169.254) and IPv6
link-local addresses in fe80::/10 (and their bracketed forms) by adding explicit
startsWith/equals checks for '169.254.' and a fe80 prefix check, and log/reject
them the same way; also tighten the protocol check on url.protocol so it rejects
non-HTTPS by removing 'http:' and only allowing 'https:' (update the
validationErrors messages accordingly) while keeping cloudlog usage for blocked
attempts (refer to hostname, url.protocol, validationErrors, and cloudlog).
Backend SSO endpoints.
Summary by CodeRabbit
New Features
Documentation / Tools
Style
✏️ Tip: You can customize this high-level summary in your review settings.