fix(security): serialize SCIM replacement with account merges - #39
Closed
seonghobae wants to merge 22 commits into
Closed
fix(security): serialize SCIM replacement with account merges#39seonghobae wants to merge 22 commits into
seonghobae wants to merge 22 commits into
Conversation
…lib finding The central SAST Semgrep gate fails on the base branch because p/default's python.lang.security.audit.dynamic-urllib-use-detected flags app/healthcheck.py: urlopen() receives a non-literal url, which urllib would happily open as a file:// path. This Medium finding blocks every open keyverse PR, since each PR scans a tree that still contains this file. Harden the probe by rejecting any URL whose scheme is not http/https before opening it, so a stray value can never coerce urlopen into a file:// read or another protocol handler. The residual audit finding on the (still non-literal) urlopen call is suppressed narrowly with an inline `# nosemgrep: dynamic-urllib-use-detected`, justified by the scheme allow-list and the fact that the container self-probe URL is not attacker-controlled. Add a regression test for the rejected-scheme path. Verified locally: semgrep marks the finding suppressed (gate passes), ruff is clean, and the healthcheck tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
…uplicate
A merged-away duplicate is tombstoned (disabled + a `merged_into_user_id`
pointer to the survivor) so it can never authenticate again, per
docs/merge-unification-flow.md and the CLAUDE.md merge invariant. But the SCIM
shim's `PUT /scim/v2/Users/{id}` (`replace_user`) only did a 404 existence
check and then translated the resource (whose `active` defaults to true) into a
replace — with no tombstone guard.
SCIM PUT is the *only* reactivation vector: `create` guards uniqueness and
`patch`/`delete` only ever disable. So a routine upstream HR/IGA full-sync PUT
that still lists the decommissioned person silently re-enabled the tombstoned
account (restoring its untouched passkey/WebAuthn login), and against a live
Keycloak the PUT would also overwrite the user representation, wiping the
`merged_into_user_id` pointer that resolves stale references to the survivor.
SCIM endpoints carry no app-level authz (trust terminates at the WAF edge), so
the trigger is unprivileged.
Fix: in `replace_user`, refuse with SCIM 409 when the target carries the
tombstone attribute, keeping a merged duplicate immutable via SCIM. Adds
`get_user_attribute` to the `AdminApi` protocol, the HTTP client, and the mock
so the guard reads the pointer uniformly. Regression test asserts the PUT is
refused and the duplicate stays disabled with its survivor pointer intact
(verified red→green: without the guard the test fails as the account is
re-enabled). Full suite 54 passed; interrogate 100%.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
…fied email
`UnificationService.merge_accounts` nested the unverified-email refusal inside
`if not decision.matched:`. Because `decide_match(..., explicit_link=True)`
returns `matched=True, reason=EXPLICIT_LINK`, an operator "explicit link" merge
skipped the guard entirely and merged + tombstoned two accounts whose only
shared tie was an UNVERIFIED email — the account-takeover vector the hard rule
exists to block (an attacker registers a duplicate holding the victim's
unverified email, then one explicit_link=True merge folds it into the victim).
This violates three contract sources:
- `app/models.py` MergeRequest.explicit_link docstring: "Even so, the service
refuses if the only tie is an UNVERIFIED email."
- `docs/merge-unification-flow.md`: "reject if only tie is unverified email ->
422 UnverifiedEmailMerge" is an unconditional step after decide_match.
- `CLAUDE.md`: "Never link or merge accounts on an unverified email."
Fix: hoist the unverified-email guard out of the not-matched branch and run it
for every decision reason except a genuine tie (EXACT_IDP_SUBJECT / VERIFIED_
EMAIL), so explicit-link and no-match are both covered. Legitimate merges are
preserved: an explicit link with different/absent emails still merges, and
verified-email / exact-(idp,subject) matches are exempt.
TDD: added test_refuse_explicit_merge_on_shared_unverified_email — confirmed it
fails on the pre-fix code ("DID NOT RAISE UnverifiedEmailMergeError", duplicate
gets tombstoned) and passes after. No existing test changes.
Verified (CI parity, py3.12, services/account_unification): ruff clean,
interrogate 100% (>=80 gate), pytest all pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
…n unverified email" This reverts commit e95e87b.
…e handlers The healthcheck validated only the *initial* URL scheme, then used the default `urllib` opener -- which follows redirects and carries an `FTPHandler`. A `http:// -> ftp://` (or `file://`) redirect from the probed endpoint would have been followed by another protocol handler (the code comment even wrongly claimed it could not be). CodeRabbit flagged it (CWE-918 SSRF, Major). Fix: route the probe through a purpose-built opener that (1) carries only HTTP/HTTPS handlers -- no ftp/file/data handler exists to open such a target -- and (2) uses `_HttpOnlyRedirectHandler`, which drops any redirect whose `Location` scheme is not in the http/https allow-list. Both are belt-and- suspenders; either alone fails the ftp redirect closed. `main` now opens via the patchable `_open_health_url` seam. Added `test_healthcheck_opener_drops_non_http_redirect_target` (ftp target dropped, same-scheme redirect kept, no ftp/file/data handler on the opener); the three existing tests re-point to the new seam. Verified (CI parity, services/account_unification): pytest all pass, ruff clean, interrogate 99.4% (>=80 gate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
Comment |
Reject explicit-link merges when the only shared signal is an unverified email, including case-normalized variants, while preserving exact-subject and mutually verified-email matches.
seonghobae
enabled auto-merge (squash)
August 3, 2026 02:37
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Author
|
Superseded by #42. The consolidated PR carries the shared SCIM/merge lock boundary, timeout handling, concurrency regressions, healthcheck scheme restrictions, the current Semgrep fix, and the larger Keycloak 26/product-hardening integration so validation occurs on one exact head. |
auto-merge was automatically disabled
August 3, 2026 04:02
Pull request was closed
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
3 participants
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
Closes the remaining TOCTOU race between SCIM full-user replacement and account merge tombstoning.
UserOperationLockscontract used by both paths;merged_into_user_idand deactivation, inside that shared critical section;The SQLite backend deliberately serializes all user mutations to avoid multi-key deadlocks. The Helm default remains one account-unification replica; clustered deployments must provide an implementation of the same lock contract backed by a shared service such as ordered PostgreSQL advisory locks.
Verification required
uv sync --locked --extra devuv run ruff check app tests toolsuv run interrogate .at 100%uv run pytest -qSupersedes #32. Closes #38.