feat: SCIM/SSO/CORS identity-boundary hardening (WS5)#412
Merged
Conversation
…(WS5)
Server-only identity-boundary hardening across SCIM, OIDC SSO, and CORS.
SCIM
- Group member-add requires provider-ownership of every target user at all
four add-sinks (reconcile / patch-add / patch-replace / create) — closes the
cross-provider IDOR; unowned members are skipped (idempotent).
- AutoLinkByEmail refuses to bind an IdP-asserted email to a pre-existing local
PASSWORD account unless the provider's new trust_email_assertions flag is set
(account-takeover guard). New column via migration 012 + Go projector + sqlc;
passwordless/already-SSO accounts still link.
- SCIM follows the provider login switch (requires enabled = TRUE, not just
scim_enabled).
- Auth has no existence/timing oracle: unknown-slug / no-token / wrong-token all
return one identical 401 ("invalid credentials") with a constant-time bcrypt
compare against auth.DummyHash.
OIDC/SSO
- Bound every outbound OIDC call (discovery, exchange, lazy JWKS) with
connect/TLS/response/overall timeouts via oidc.ClientContext (no
http.DefaultClient).
- Fix a real linker bug: a soft-deleted linked user's stale-link cleanup fell
through to a bogus "lookup identity link: <nil>" error instead of re-linking/
re-creating (the post-block err==nil guard returned). Now gated on err != nil.
CORS
- Allow-all mode reflects the Origin but never sends Allow-Credentials (closes
the credentialed-wildcard hole); named origins keep credentials. The control
server refuses to boot with CORS_ALLOW_ALL when TLS is enabled or the listen
addr is non-localhost (validateConfig extracted as a pure, testable fn).
Tests: cross-provider IDOR (member-add ×4 sinks + user-resource verbs),
AutoLinkByEmail trust gate (takeover/trust/passwordless/absent), SCIM auth
rejection matrix + disabled-provider + timing-indistinguishability, OIDC
bounded-client + discovery-timeout, verifier aud/exp/iss/byte-tamper rejections,
linker auto-link/existing-link/soft-deleted branches, PKCE S256 known-answer +
state entropy, SSOCallback slug-mismatch, CORS credentialed-wildcard + boot
guard. ADR 0008 + control README. Migration 012.
Closes #411.
|
Warning Review limit reached
More reviews will be available in 49 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements WS5 SCIM/SSO/CORS identity-boundary hardening across six domains: boot-time CORS safety validation, identity-provider schema extension for email-linking trust gates, SCIM authentication timing-indistinguishability, provider-scoped group member authorization, OIDC timeout bounding, and CORS credential separation. All changes are fail-closed, with extensive test coverage and accompanying ADR documentation. ChangesSCIM/SSO/CORS Identity-Boundary Hardening (WS5)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/scim/auth.go (1)
24-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnify no-token/malformed-token failures to the same
invalid credentialsresponse.Line 24-40 still returns distinct 401 bodies (
missing authorization header,authorization header must use Bearer scheme,bearer token is empty). That conflicts with the WS5 objective for a single indistinguishable rejection across unknown-slug/no-token/wrong-token paths.Suggested fix
- // Extract bearer token from Authorization header - authHeader := r.Header.Get("Authorization") - if authHeader == "" { - h.logger.Warn("SCIM auth failed: missing authorization header", "method", r.Method, "path", r.URL.Path) - writeError(w, http.StatusUnauthorized, "missing authorization header") - return - } - - if !strings.HasPrefix(authHeader, "Bearer ") { - h.logger.Warn("SCIM auth failed: not a Bearer token", "method", r.Method, "path", r.URL.Path) - writeError(w, http.StatusUnauthorized, "authorization header must use Bearer scheme") - return - } - - token := strings.TrimPrefix(authHeader, "Bearer ") - if token == "" { - h.logger.Warn("SCIM auth failed: empty token", "method", r.Method, "path", r.URL.Path) - writeError(w, http.StatusUnauthorized, "bearer token is empty") - return - } + // Extract bearer token from Authorization header. + // WS5: no-token/malformed-token paths are indistinguishable from other auth failures. + authHeader := r.Header.Get("Authorization") + token := "" + if strings.HasPrefix(authHeader, "Bearer ") { + token = strings.TrimPrefix(authHeader, "Bearer ") + } + if token == "" { + h.logger.Warn("SCIM auth failed: missing or malformed bearer token", "method", r.Method, "path", r.URL.Path) + _ = bcrypt.CompareHashAndPassword([]byte(auth.DummyHash), []byte("invalid")) + writeError(w, http.StatusUnauthorized, "invalid credentials") + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scim/auth.go` around lines 24 - 40, The three auth failure branches (missing authHeader, non‑Bearer prefix, empty token) currently return distinct 401 bodies; change them to return the same generic response to meet WS5: replace the specific writeError messages in those branches with a single message like "invalid credentials" and use the same HTTP 401 status for each, and also avoid leaking different failure reasons in logs by updating the logger.Warn calls in those branches to a single generic message (e.g., "SCIM auth failed: invalid credentials") while retaining the method/path fields; the changes apply to the code that checks authHeader, the strings.HasPrefix("Bearer ") branch, and the token == "" branch and should still call the existing writeError function and return immediately as before.
🧹 Nitpick comments (1)
internal/scim/groups_authz_test.go (1)
100-124: ⚡ Quick winAssert success status in cross-provider member-add negative paths.
These subtests currently validate membership outcome only. Capture and assert the HTTP status as well, so regressions don’t pass due to unrelated request failures.
Proposed test hardening
t.Run("patch_add_cross_provider_rejected", func(t *testing.T) { gid := createSCIMGroup(t, env, slugB, tokenB, "g-padd-"+testutil.NewID()[:6], nil) - scimReq(t, env, slugB, tokenB, http.MethodPatch, "/Groups/"+gid, patchAdd(userA)) + w := scimReq(t, env, slugB, tokenB, http.MethodPatch, "/Groups/"+gid, patchAdd(userA)) + require.Equal(t, http.StatusOK, w.Code, "%s", w.Body.String()) assert.NotContains(t, groupMemberIDs(t, env, gid), userA, "providerB must NOT add providerA's user via PATCH add (IDOR)") }) t.Run("patch_replace_cross_provider_rejected", func(t *testing.T) { gid := createSCIMGroup(t, env, slugB, tokenB, "g-prep-"+testutil.NewID()[:6], nil) - scimReq(t, env, slugB, tokenB, http.MethodPatch, "/Groups/"+gid, patchReplaceMembers(userA)) + w := scimReq(t, env, slugB, tokenB, http.MethodPatch, "/Groups/"+gid, patchReplaceMembers(userA)) + require.Equal(t, http.StatusOK, w.Code, "%s", w.Body.String()) assert.NotContains(t, groupMemberIDs(t, env, gid), userA, "providerB must NOT add providerA's user via PATCH replace members (IDOR)") }) t.Run("put_reconcile_cross_provider_rejected", func(t *testing.T) { gid := createSCIMGroup(t, env, slugB, tokenB, "g-put-"+testutil.NewID()[:6], nil) put := map[string]any{ "schemas": []string{"urn:ietf:params:scim:schemas:core:2.0:Group"}, "displayName": "g-put-" + testutil.NewID()[:6], "members": []map[string]any{{"value": userA}}, } - scimReq(t, env, slugB, tokenB, http.MethodPut, "/Groups/"+gid, put) + w := scimReq(t, env, slugB, tokenB, http.MethodPut, "/Groups/"+gid, put) + require.Equal(t, http.StatusOK, w.Code, "%s", w.Body.String()) assert.NotContains(t, groupMemberIDs(t, env, gid), userA, "providerB must NOT add providerA's user via PUT reconcile (IDOR)") })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/scim/groups_authz_test.go` around lines 100 - 124, In the three subtests patch_add_cross_provider_rejected, patch_replace_cross_provider_rejected, and put_reconcile_cross_provider_rejected, capture the HTTP status returned by scimReq and assert that it indicates a successful request (e.g. 2xx) before asserting on groupMemberIDs; specifically, change the scimReq calls in those tests to return the response/status (from scimReq) and add an assertion like require.True(t, status >= 200 && status < 300, "expected success status for PATCH/PUT request") so failures in the request itself do not make the negative membership assertions pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/idp/oidc_verify_test.go`:
- Around line 335-344: The tamper test mutates the final base64url signature
character which can be semantically no-op; instead mutate a non-terminal
signature character: in the block that builds tampered from idToken (variables
parts, sig, tampered), choose an index like idx := len(sig)-2 (ensure len(sig)
>= 2) and flip sig[idx] (e.g., toggle between 'A' and 'B' or XOR the byte)
rather than mutating sig[len(sig)-1], then set parts[2] = string(sig) and join
to tampered; this makes the mutation deterministic and avoids harmless changes
to the final base64 padding bits.
---
Outside diff comments:
In `@internal/scim/auth.go`:
- Around line 24-40: The three auth failure branches (missing authHeader,
non‑Bearer prefix, empty token) currently return distinct 401 bodies; change
them to return the same generic response to meet WS5: replace the specific
writeError messages in those branches with a single message like "invalid
credentials" and use the same HTTP 401 status for each, and also avoid leaking
different failure reasons in logs by updating the logger.Warn calls in those
branches to a single generic message (e.g., "SCIM auth failed: invalid
credentials") while retaining the method/path fields; the changes apply to the
code that checks authHeader, the strings.HasPrefix("Bearer ") branch, and the
token == "" branch and should still call the existing writeError function and
return immediately as before.
---
Nitpick comments:
In `@internal/scim/groups_authz_test.go`:
- Around line 100-124: In the three subtests patch_add_cross_provider_rejected,
patch_replace_cross_provider_rejected, and
put_reconcile_cross_provider_rejected, capture the HTTP status returned by
scimReq and assert that it indicates a successful request (e.g. 2xx) before
asserting on groupMemberIDs; specifically, change the scimReq calls in those
tests to return the response/status (from scimReq) and add an assertion like
require.True(t, status >= 200 && status < 300, "expected success status for
PATCH/PUT request") so failures in the request itself do not make the negative
membership assertions pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48351ad1-5f3f-4e2d-8325-89dd5f09398f
⛔ Files ignored due to path filters (3)
internal/store/generated/identity_providers.sql.gois excluded by!**/generated/**internal/store/generated/models.gois excluded by!**/generated/**internal/store/generated/scim.sql.gois excluded by!**/generated/**
📒 Files selected for processing (29)
cmd/control/README.mdcmd/control/config_guard_test.gocmd/control/flags.godocs/adr/0008-scim-sso-identity-boundary.mdinternal/api/sso_handler_failure_paths_test.gointernal/idp/linker.gointernal/idp/linker_test.gointernal/idp/oidc.gointernal/idp/oidc_timeout_test.gointernal/idp/oidc_verify_test.gointernal/idp/state_test.gointernal/middleware/cors.gointernal/middleware/cors_test.gointernal/projectors/identity_provider.gointernal/projectors/identity_provider_listener.gointernal/scim/auth.gointernal/scim/auth_matrix_test.gointernal/scim/groups_authz_test.gointernal/scim/groups_create.gointernal/scim/groups_helpers.gointernal/scim/groups_mutate.gointernal/scim/handler_test.gointernal/scim/users_create.gointernal/scim/users_link_test.gointernal/store/identity_provider.gointernal/store/migrations/012_idp_trust_email_assertions.sqlinternal/store/postgres/identity_provider.gointernal/store/queries/identity_providers.sqlinternal/store/queries/scim.sql
Flipping the final base64url char of an RS256 signature can decode to the same bytes (it carries only a couple of meaningful bits), a no-op that could let verification pass. Mutate a mid-segment char so the decoded signature always changes.
This was referenced Jun 13, 2026
PaulDotterer
added a commit
that referenced
this pull request
Jun 13, 2026
…sponse (#416) PR #412 added the trust_email_assertions flag + SCIM AutoLinkByEmail account-takeover gate, but the flag had no Create/Update RPC field — operators could only set it via a raw event. With the sdk proto field (manchtools/power-manage-sdk#98), map req.Msg.TrustEmailAssertions into the IdentityProviderCreated/Updated events and return it on the IdentityProvider response, so it is settable + visible via the API/UI. Default false (secure). Bumps sdk to the proto field. Closes the WS5 trust_email follow-up.
PaulDotterer
added a commit
that referenced
this pull request
Jun 26, 2026
…ation (#465) v2026.06 shipped migrations 001–008 as a clean baseline (itself a consolidation of v2026.05's set). Six incremental migrations then landed on main for the 2026.07 cycle — 009 role-permission split (#333), 010 role-grant scope columns (#334), 011 events append-only trigger (#404), 012 IdP trust_email_assertions (#412), 013 LUKS token hashing (#420), 014 reconciler-owned system-role permissions (#440). Per the project's per-release migration convention, collapse them into a single ordered 009_v2026_07.sql so the 2026.06→2026.07 delta is one migration. Faithful concatenation: every statement preserved verbatim, Up in original apply order, Down reversed, each StatementBegin/End block intact. No schema change — `sqlc generate` produces an identical generated/ tree, and the seed-parity guard (system-role permissions stay reconciler-owned) still passes.
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.
WS5 — SCIM / SSO / CORS identity-boundary hardening (server-only)
Hardens the identity boundary where SCIM and OIDC SSO take input from partially-trusted IdPs. Closes the WS5 SCIM + OIDC + CORS findings (crypto AAD #8 ships separately). See ADR 0008.
SCIM
trust_email_assertionsflag is set (the operator's explicit opt-in, e.g. migrating password users to SSO). Passwordless/already-SSO accounts still link. New column via migration 012 + Go projector + sqlc regen.enabled = TRUE, not justscim_enabled.401 invalid credentialswith a constant-time bcrypt compare (auth.DummyHash).OIDC / SSO
*http.Clientwith connect/TLS/response/overall timeouts viaoidc.ClientContext(neverhttp.DefaultClient).lookup identity link: <nil>error instead of re-linking/re-creating (the post-blockerr==nilguard wrongly returned). Now gated onerr != nil.CORS
Access-Control-Allow-Credentials(closes the credentialed-wildcard CSRF/token-theft hole); named origins keep credentials. The control server refuses to boot withCORS_ALLOW_ALLwhen TLS is enabled or the listen addr is non-localhost (validateConfigextracted as a pure, testable fn).Tests
Cross-provider IDOR (4 member-add sinks + 4 user verbs), AutoLinkByEmail trust gate (takeover/trust/passwordless/absent), SCIM auth rejection matrix + disabled-provider + timing-indistinguishability, OIDC bounded-client + discovery-timeout, verifier aud/exp/iss/byte-tamper rejections, linker auto-link/existing-link/soft-deleted branches, PKCE S256 known-answer + state entropy, SSOCallback slug-mismatch, CORS credentialed-wildcard + boot guard.
Migration 012, ADR 0008, control README. Closes #411.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation