fix(security): SCIM tombstone guard (prevent merged-account resurrection) + healthcheck scheme allow-list - #32
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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughHealthcheck 요청을 HTTP(S) 전용 opener와 리디렉션 검증으로 제한하고, Keycloak 사용자 속성 조회를 추가해 tombstone 계정의 SCIM PUT 재활성화를 409로 차단합니다. Changes계정 병합 안전성
Healthcheck URL 검증
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SCIMClient
participant replace_user
participant HttpAdminApi
SCIMClient->>replace_user: PUT 사용자 교체 요청
replace_user->>HttpAdminApi: tombstone 속성 조회
HttpAdminApi-->>replace_user: 속성값 반환
replace_user-->>SCIMClient: tombstone이면 409 반환
sequenceDiagram
participant HealthcheckMain
participant RestrictedOpener
participant HttpOnlyRedirectHandler
HealthcheckMain->>RestrictedOpener: HTTP(S) health URL 요청
RestrictedOpener->>HttpOnlyRedirectHandler: 리디렉션 대상 전달
HttpOnlyRedirectHandler-->>RestrictedOpener: HTTP(S) 허용 또는 비HTTP(S) 거부
RestrictedOpener-->>HealthcheckMain: 응답 또는 오류 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head2fd34a91e211d21b8a6d210d49b248bd2527daa8. -
Head SHA:
2fd34a91e211d21b8a6d210d49b248bd2527daa8 -
Workflow run: 30471068072
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: healthcheck.py"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: healthcheck.py"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_healthcheck.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_healthcheck.py"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: healthcheck.py"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: healthcheck.py"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_healthcheck.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_healthcheck.py"]
R2 --> V2["targeted test run"]
|
|
Blocker note (not fixable in this PR): every required mechanical check on this head is green — Semgrep now passes (the base finding this PR fixes), plus CodeQL, The offline sandbox materialized an empty base-python-requirements set for this repo — it does not discover the dependencies declared under Generated by Claude Code |
…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
seonghobae
changed the title
Status: this PR's code is sound; its only blocker is a central tooling gap now fixedThe stale Local verification of the current head Root cause of the coverage-evidence failure (not this PR): Fix (central): ContextualWisdomLab/.github#668 adds a So once #668 merges to Generated by Claude Code |
…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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@services/account_unification/app/healthcheck.py`:
- Around line 19-29: Restrict redirected URLs as well as the initial URL to
_ALLOWED_SCHEMES: update the healthcheck urlopen flow and its redirect handling
so any Location with a non-HTTP(S) scheme is rejected before another protocol
handler, especially FTPHandler, can connect. Add a regression test covering an
HTTP-to-FTP redirect and verify no FTP connection is attempted.
In `@services/account_unification/app/scim.py`:
- Around line 202-209: Serialize the tombstone check and replacement in the SCIM
update flow around get_user_attribute and replace_user using the existing shared
distributed lock keyed by user_id, or an equivalent Keycloak conditional update.
Ensure merge operations use the same synchronization mechanism so a passed check
cannot race with account merging; do not address this with a second read alone.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93e8eca0-ed8f-4866-8d9e-b6b7b2965bac
📒 Files selected for processing (8)
services/account_unification/app/healthcheck.pyservices/account_unification/app/keycloak_client.pyservices/account_unification/app/scim.pyservices/account_unification/app/service.pyservices/account_unification/tests/mock_keycloak.pyservices/account_unification/tests/test_healthcheck.pyservices/account_unification/tests/test_merge.pyservices/account_unification/tests/test_scim.py
| if provisioner.get_user_attribute(user_id, TOMBSTONE_ATTRIBUTE_KEY): | ||
| raise _scim_error( | ||
| 409, | ||
| f"user '{user_id}' has been merged into another account " | ||
| "and cannot be modified", | ||
| ) | ||
| account = _to_user_account(resource, user_id=user_id) | ||
| provisioner.replace_user(user_id, account) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
tombstone 검사와 PUT을 원자적으로 묶어 주세요.
Line 202의 조회와 Line 209의 교체는 별도 Keycloak 요청입니다. 그 사이에 병합이 merged_into_user_id를 기록하고 계정을 비활성화하면, 이미 검사를 통과한 PUT이 tombstone 포인터를 덮어쓰고 계정을 재활성화할 수 있습니다. 두 경로를 사용자 ID 기준의 공유 분산 락 또는 Keycloak 측 조건부 갱신으로 직렬화하세요. 단순 재조회만으로는 경쟁 구간이 남습니다.
🤖 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 `@services/account_unification/app/scim.py` around lines 202 - 209, Serialize
the tombstone check and replacement in the SCIM update flow around
get_user_attribute and replace_user using the existing shared distributed lock
keyed by user_id, or an equivalent Keycloak conditional update. Ensure merge
operations use the same synchronization mechanism so a passed check cannot race
with account merging; do not address this with a second read alone.
There was a problem hiding this comment.
Valid observation, but deferring the serialization to a follow-up rather than fixing it in this PR — here's the reasoning.
The residual TOCTOU is real: get_user_attribute (the tombstone check) and replace_user are two separate Admin-API round-trips, so a merge that tombstones the target between them can be clobbered by the already-passed PUT. But the two remedies you name are both architecturally significant and out of scope for this atomic guard:
- Distributed lock keyed by user_id — there is no lock backend in this service today (it is a stateless FastAPI shim over the Keycloak Admin API; no Redis/DB-lock primitive exists). Introducing one and threading the same lock through the merge path in
service.pyso the two operations actually serialize is a cross-cutting design change, not a local edit. - Keycloak-side conditional update — the Admin API's
PUT /users/{id}exposes no ETag/If-Match (or optimistic-version) conditional-update mechanism, so there is no drop-in conditional PUT to key off.
This PR is still a strict net improvement over the pre-change state, where SCIM PUT had no tombstone check at all and could reactivate a merged-away account unconditionally on any full-sync. The guard narrows that from "always reactivatable" to "reactivatable only inside a narrow concurrent-merge race." A correct serialization (shared lock introduced once, applied to both the SCIM-replace and merge paths) deserves its own focused PR/issue; I'll leave this thread open to track it rather than close it with a partial fix. The first finding (redirect-scheme restriction) is fixed in ee085a3.
Generated by Claude Code
There was a problem hiding this comment.
@seonghobae, agreed. The current guard is a meaningful security improvement, while fully closing the remaining race requires a shared cross-path synchronization design rather than a local second read.
I’ll leave this finding unresolved for follow-up. Would you like me to create a GitHub issue to track introducing serialization across the SCIM replacement and account-merge paths?
🐇💭
✏️ Learnings added
Learnt from: seonghobae
Repo: ContextualWisdomLab/keyverse PR: 32
File: services/account_unification/app/scim.py:202-209
Timestamp: 2026-07-31T01:48:00.448Z
Learning: In `services/account_unification`, SCIM tombstone protection in `services/account_unification/app/scim.py` currently performs `AdminApi.get_user_attribute` followed by `AdminApi.replace_user` as separate Keycloak Admin API calls. The service has no distributed-lock backend, and Keycloak Admin API user replacement has no ETag/If-Match conditional update support; fully preventing a concurrent merge from racing this flow requires a shared synchronization design covering both SCIM replacement and the merge path.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
There was a problem hiding this comment.
@coderabbitai yes, please create a GitHub issue to track introducing shared serialization (a user-id-keyed lock, or an equivalent atomic mechanism) across the SCIM replace_user path and the account-merge path, so the tombstone check and the write cannot be raced by a concurrent merge. Reference this thread and note the two constraints established here: the service has no distributed-lock backend today, and the Keycloak Admin API PUT /users/{id} exposes no ETag/If-Match conditional update — so the fix is a cross-path design change, not a local edit.
Generated by Claude Code
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="ContextualWisdomLab/keyverse"
title="SCIM 사용자 교체와 계정 병합 간 tombstone 경쟁 조건 직렬화"
body=$(cat <<'EOF'
## 요약
SCIM `PUT /scim/v2/Users/{id}`의 tombstone 검사와 계정 병합의 tombstone 기록/비활성화가 동시에 실행되면, 검사 통과 후 수행된 SCIM 교체가 병합 포인터를 덮어쓰고 계정을 재활성화할 수 있습니다. 사용자 ID 기준의 공유 직렬화(분산 락 또는 동등한 원자적 메커니즘)를 도입하여 이 경쟁 조건을 제거합니다.
## 배경 및 근거
현재 SCIM 교체 경로는 tombstone 속성 조회와 Keycloak 사용자 교체를 별도 Admin API 요청으로 수행합니다. 두 요청 사이에 병합이 발생할 수 있으므로, 현재 보호는 비동시 요청에 대해서는 효과적이지만 동시 병합에 대한 TOCTOU 구간은 남아 있습니다.
## 제약 사항
- 이 서비스는 현재 분산 락 백엔드를 제공하지 않는 stateless FastAPI shim입니다.
- Keycloak Admin API의 `PUT /users/{id}`는 ETag/`If-Match` 또는 optimistic-version 기반 조건부 갱신을 제공하지 않습니다.
- 따라서 단순 재조회나 `scim.py`의 국소 수정으로는 해결할 수 없으며, SCIM 교체와 병합 경로 모두를 포괄하는 설계 변경이 필요합니다.
## 영향 범위
- `services/account_unification/app/scim.py`: SCIM `replace_user`의 tombstone 검사 및 사용자 교체 흐름
- `services/account_unification/app/service.py`: 계정 병합 시 tombstone 기록 및 사용자 비활성화 흐름
- 필요 시 잠금/원자성 제공을 위한 서비스 인프라 및 구성
## 요구 사항
1. 사용자 ID를 키로 하는 공유 동기화 메커니즘 또는 동등한 원자적 처리 방식을 설계하고 도입합니다.
2. SCIM `replace_user`가 tombstone 검사부터 `replace_user` 쓰기까지 해당 메커니즘으로 보호되도록 합니다.
3. 계정 병합 경로가 동일한 메커니즘을 사용하여 대상 사용자의 tombstone 기록과 비활성화를 직렬화하도록 합니다.
4. 검사 통과 후 동시 병합이 발생해도 SCIM 쓰기가 tombstone 포인터를 덮어쓰거나 병합된 계정을 재활성화할 수 없음을 검증하는 동시성 회귀 테스트를 추가합니다.
5. 장애, 타임아웃, 락 획득 실패 시의 동작과 운영상 요구되는 인프라를 문서화합니다.
## 관련 링크
- PR: https://github.com/ContextualWisdomLab/keyverse/pull/32
- 검토 스레드: https://github.com/ContextualWisdomLab/keyverse/pull/32#discussion_r3687464262
요청자: `@seonghobae`
EOF
)
gh issue create \
--repo "$repo" \
--title "$title" \
--body "$body" \
--assignee "seonghobae"Length of output: 2773
@seonghobae, created and assigned the follow-up issue: #38.
It documents the shared user-ID-keyed serialization/atomicity requirement, both established constraints, the affected SCIM and merge paths, acceptance criteria, and links back to this PR and review thread.
🧬 Code Graph Analysis Results
-
services/account_unification/app/keycloak_client.py:35-113—AdminApiprotocol defines the Keycloak operations used byreplace_user, includingget_user,replace_user,set_user_attribute, andget_user_attribute. -
services/account_unification/app/keycloak_client.py:330-335—deactivate_user(user_id)disables the Keycloak account through aPUTrequest to/admin/realms/{realm}/users/{user_id}with{"enabled": False}. -
services/account_unification/app/keycloak_client.py:346-356—get_user_attribute(user_id, key)fetches the user representation and returns the first string value for the requested attribute, orNoneif unset. -
services/account_unification/app/models.py:57-70—UserAccountis the domain representation passed toAdminApi.replace_user; it includes the Keycloak user ID, account state, identity fields, and SCIMexternal_id.
🧠 Learnings used
Learnt from: seonghobae
Repo: ContextualWisdomLab/keyverse PR: 32
File: services/account_unification/app/scim.py:202-209
Timestamp: 2026-07-31T01:48:00.448Z
Learning: In the account unification service, do not treat the SCIM tombstone-protection sequence of AdminApi.get_user_attribute followed by AdminApi.replace_user as atomic: Keycloak Admin API user replacement lacks ETag/If-Match conditional updates, and the service has no distributed-lock backend. Preventing races with concurrent merges requires a shared synchronization mechanism that covers both the SCIM replacement flow and the merge path; otherwise, document the limitation rather than claiming full concurrency protection.
You are interacting with an AI system.
…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
2 participants
Two base-branch security hardenings for the
cwl-idpadmin service, both verified against the in-memory Keycloak fake.1. SCIM PUT resurrected a tombstoned merged duplicate — HIGH (commit
5e9c4eb)A merged-away duplicate is tombstoned (disabled + a
merged_into_user_idpointer to the survivor) so it "can never authenticate again" — the invariant indocs/merge-unification-flow.mdandCLAUDE.md. ButPUT /scim/v2/Users/{id}(app/scim.py::replace_user) did only a 404 existence check, then translated the SCIM resource (whoseactivedefaults to true) into a replace — with no tombstone guard.create_userguards uniqueness (409);patch_user/delete_useronly ever disable. So SCIM PUT is the one path that can re-enable an account.merged_into_user_idpointer 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.replace_usernow refuses with SCIM 409 when the target carries the tombstone attribute, keeping a merged duplicate immutable via SCIM. Addsget_user_attributeto theAdminApiprotocol, the HTTP client, and the mock so the guard reads the pointer uniformly (real Keycloak stores attributes as string lists; the mock stores plain strings — both handled).test_scim_replace_refuses_to_resurrect_a_tombstoned_duplicateasserts the PUT is refused and the duplicate stays disabled with its survivor pointer intact. Verified red→green: with the guard disabled the test fails (the account is re-enabled).service.py); fix: Keycloak 26 import compatibility + DB-backed runtime federation registry #18 touchesscim.pyonly to add path-segment validation. No open PR guardsreplace_user.2. Healthcheck URL-scheme allow-list + Semgrep dynamic-urllib finding (commit
2fd34a9)Restricts the healthcheck probe to an explicit URL-scheme allow-list and clears the base-branch Semgrep
dynamic-urllib-use-detectedfinding.Verification
From
services/account_unification/(mirrors CIaccount-unification-tests):(fastapi/httpx available; ruff shows only the pre-existing
Depends-default B008 pattern, unchanged from base and not flagged by the CI-locked ruff 0.15.21 + default rule set.)🤖 Generated with Claude Code
https://claude.ai/code/session_01HdCssGnNMhKHNu3TXFstWH
Summary by CodeRabbit
버그 수정
테스트