Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe SSO domain check endpoint was updated to remove ChangesSSO Domain Check Response Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
Contributor
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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/sso/check-domain.ts (1)
103-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove internal identifiers from the public-path log payload.
provider_idandorg_idare still written to logs on the anonymous lookup path, which reintroduces the tenant-reconnaissance leak this change is trying to close.🔧 Proposed fix
cloudlog({ requestId, context: 'check_domain - SSO provider found', domain, enforce_sso: enforcementRow?.enforce_sso, - provider_id: legacyRow?.provider_id, - org_id: enforcementRow?.org_id ?? legacyRow?.org_id, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/sso/check-domain.ts` around lines 103 - 110, The cloudlog call in check-domain's anonymous lookup path is emitting internal identifiers (provider_id and org_id), which leaks tenant info; remove provider_id and org_id from the log payload in the cloudlog invocation (the call that currently includes requestId, context: 'check_domain - SSO provider found', domain, enforce_sso, provider_id, org_id) so only non-identifying fields (e.g., requestId, context, domain, enforce_sso) are logged; ensure any references to legacyRow?.provider_id or enforcementRow?.org_id are not logged or included in the cloudlog payload and instead kept only for internal logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@supabase/functions/_backend/private/sso/check-domain.ts`:
- Around line 103-110: The cloudlog call in check-domain's anonymous lookup path
is emitting internal identifiers (provider_id and org_id), which leaks tenant
info; remove provider_id and org_id from the log payload in the cloudlog
invocation (the call that currently includes requestId, context: 'check_domain -
SSO provider found', domain, enforce_sso, provider_id, org_id) so only
non-identifying fields (e.g., requestId, context, domain, enforce_sso) are
logged; ensure any references to legacyRow?.provider_id or
enforcementRow?.org_id are not logged or included in the cloudlog payload and
instead kept only for internal logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 260554ee-240d-463b-8698-ebbcaeaa446a
📒 Files selected for processing (8)
src/composables/useSSORouting.tssrc/pages/login.vuesupabase/functions/_backend/private/sso/check-domain.tssupabase/functions/_backend/private/sso/check-enforcement.tssupabase/migrations/20260502134501_restrict_sso_lookup_rpc_access.sqltests/security-definer-execute-hardening.test.tstests/sso-check-domain-response.unit.test.tstests/sso.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/sso.test.ts (1)
101-110: ⚡ Quick winStrengthen this regression check with an exact public response shape
This currently checks two fields, but a strict shape assertion will catch any future internal-field leakage in one place.
Suggested test adjustment
- const data = await response.json() as { - has_sso: boolean - enforce_sso?: boolean - provider_id?: string - org_id?: string - } - expect(data.has_sso).toBe(true) - expect(data.enforce_sso).toBe(false) - expect(data.provider_id).toBeUndefined() - expect(data.org_id).toBeUndefined() + const data = await response.json() as { + has_sso: boolean + enforce_sso: boolean + } + expect(data).toStrictEqual({ + has_sso: true, + enforce_sso: false, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sso.test.ts` around lines 101 - 110, Replace the multiple loose assertions with a single strict shape assertion on the parsed response object so any unexpected fields leak will fail the test; specifically, after calling response.json() (the variable data), assert the entire object equals the exact public shape (e.g., has_sso: true, enforce_sso: false, provider_id: undefined, org_id: undefined) using a strict equality matcher like toStrictEqual/toEqual on the data variable to catch any extra fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/sso.test.ts`:
- Around line 101-110: Replace the multiple loose assertions with a single
strict shape assertion on the parsed response object so any unexpected fields
leak will fail the test; specifically, after calling response.json() (the
variable data), assert the entire object equals the exact public shape (e.g.,
has_sso: true, enforce_sso: false, provider_id: undefined, org_id: undefined)
using a strict equality matcher like toStrictEqual/toEqual on the data variable
to catch any extra fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a3cb6e0-8e23-494e-9e81-e4a99ab560a3
📒 Files selected for processing (2)
supabase/functions/_backend/private/sso/check-domain.tstests/sso.test.ts
💤 Files with no reviewable changes (1)
- supabase/functions/_backend/private/sso/check-domain.ts
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
org_idandprovider_idfields from the unauthenticated SSO domain check response.service_roleand moved server-side lookup calls to the admin client.Motivation (AI generated)
GHSA-c5jf-5wxg-mgrq reported that unauthenticated callers could enumerate domains and map them to internal organization/provider identifiers. The endpoint only needs to tell the login flow whether SSO exists and whether it is enforced.
Business Impact (AI generated)
This reduces tenant reconnaissance risk for Capgo customers without changing the login UX or plugin/API compatibility surface.
Test Plan (AI generated)
bun lintbun lint:backendbunx eslint tests/sso-check-domain-response.unit.test.ts tests/sso.test.ts tests/security-definer-execute-hardening.test.tssqlfluff lint --dialect postgres supabase/migrations/20260502134501_restrict_sso_lookup_rpc_access.sqlbunx vitest run tests/sso-check-domain-response.unit.test.tsbun typechecktests/sso.test.tswas not run because Docker/OrbStack was not running locally.Generated with AI
Summary by CodeRabbit
Release Notes
Refactor
Tests