fix(scim): harden Phase 7 SCIM after security/pitfall/plan review#56
Merged
fix(scim): harden Phase 7 SCIM after security/pitfall/plan review#56
Conversation
Post-implementation review of Phase 7 SCIM found 6 actionable issues.
This commit addresses all of them:
S1: SCIM group store methods now accept orgID and use withOrgTx instead
of withBypassTx, adding AND org_id to all SQL queries for defense-
in-depth tenant isolation. All callers and tests updated.
S2: Extract shared scimScheme() helper that validates X-Forwarded-Proto
(only "http"/"https" accepted). Fixes inconsistency between
scimUserLocation and scimBaseURL.
S5: Replace N+1 per-member GetIdentityByProviderAndUser in list users
with batch ListIdentitiesByProviderAndUsers using ANY($2::uuid[]).
P2: SCIM audit log actor_id now nil (SQL NULL) instead of uuid.Nil
(all-zeros UUID), matching design doc and enabling IS NULL queries.
P3: matchesSCIMGroupFilter now logs slog.Warn for unsupported filter
attributes instead of silently returning false.
P4: Entry-level slog.InfoContext with org_id/scim_config_id added to
all 11 SCIM handlers for consistent observability.
All SCIM tests passing (auth: 3, store: 21, api: 55+). Lint clean.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
GetSCIMGroup now queries with AND org_id, so groups from other orgs return nil. The nil check already handles this — the manual group.OrgID != orgID comparisons were dead code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Same pattern as the SCIM group store refactor: withBypassTx → withOrgTx, AND org_id added to SQL query. Removes redundant manual group.OrgID check in patchSCIMGroupMappingHandler (now dead code — query returns nil for cross-org groups). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The MSRC adapter's /cvrf/v3.0/csaf/{id} endpoint never existed on
Microsoft's API. Plan covers switching to the real CSAF 2.0 static
file distribution at msrc.microsoft.com/csaf/advisories/, capturing
golden fixtures, writing EPSS/MSRC golden tests, adding MSRC to
SeedCorpus, and completing Phase 10 verification.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
RemoveSCIMManagedGroupMember was missing orgID entirely — SQL had no AND org_id clause and used withBypassTx, bypassing RLS. Added orgID parameter and AND org_id = $3 to the DELETE query. AddGroupMemberSCIMManaged already had orgID in the INSERT but used withBypassTx. Switched to withOrgTx for dual-layer isolation. Both methods now follow the same withOrgTx + AND org_id pattern as all other org-scoped store methods. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Post-implementation review of Phase 7 SCIM provisioning surfaced 6 actionable findings across security review, pitfall check, and plan compliance check. This PR addresses all of them, plus follow-up issues found during dead code review.
Original findings
withBypassTxtowithOrgTxwithAND org_idin all SQL queries — defense-in-depth tenant isolationX-Forwarded-Protovalidated (only http/https) via sharedscimScheme()helper; fixed inconsistency between user and group URL buildersListIdentitiesByProviderAndUsersusingANY($2::uuid[])actor_idnownil(SQL NULL) instead ofuuid.Nil, matching design docslog.Warninstead of silently filteringslog.InfoContextwithorg_id/scim_config_idadded to all 11 SCIM handlersFollow-up findings from dead code review
GetGroupIfActiverefactored with samewithOrgTx+AND org_idpatternRemoveSCIMManagedGroupMemberwas missingorgIDentirely — SQL had noAND org_idand usedwithBypassTx(bypassed RLS). Fixed.AddGroupMemberSCIMManagedswitched fromwithBypassTxtowithOrgTxgroup.OrgID != orgIDchecks removed (dead code after SQL changes)Review context
Full review ran:
/pitfall-check(clean),/security-review(1H→Info, 2M, 3L),/plan-checkagainst design doc (4 gaps). Pitfall check found 0 issues. The "High" (SHA-256 vs argon2id) was downgraded — 256-bit entropy makes SHA-256 correct for bearer tokens.Test plan
internal/authSCIM tests: 3 PASSinternal/storeSCIM tests: 21 PASS (testcontainers)internal/apiSCIM tests: 55+ PASS (e2e with testcontainers)golangci-lint run: 0 issueswithBypassTxon org-scoped tables🤖 Generated with Claude Code