fix(account-unification): enforce unverified-email hard rule on explicit_link merges - #34
Conversation
…cit_link merges
The merge-safety hard rule ("never link or merge accounts on an unverified
email"; docs/merge-unification-flow.md, CLAUDE.md, and the
MergeRequest.explicit_link contract "Even so, the service refuses if the only
tie is an UNVERIFIED email") was only enforced inside the `not decision.matched`
branch of UnificationService.merge_accounts. When explicit_link=True,
decide_match classifies the pair as EXPLICIT_LINK (matched=True), so the guard
was skipped: two accounts coinciding only on an UNVERIFIED — i.e.
attacker-registerable — email were merged (survivor absorbed the duplicate,
duplicate tombstoned/disabled). That is an account-takeover vector the hard rule
exists to close.
Fix: compute the shared-unverified-email condition and refuse with
UnverifiedEmailMergeError whenever it holds and the match is not backed by a
strong tie (exact idp subject or a mutually verified email), regardless of
explicit_link. The legitimate explicit_link override (no shared email, or a
strong tie) still merges; the merge-flow pseudocode ordering (reject-unverified
before reject-no-match) is preserved.
Regression tests: explicit_link cannot override a shared unverified email
(exact and case-variant); existing explicit-link-without-shared-signal merge
still passes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
|
Warning Review limit reached
Next review available in: 28 minutes 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 (1)
📝 WalkthroughWalkthrough계정 병합은 명시적 연결이 있어도 비검증 이메일만 일치하면 거부하도록 변경되었으며, 해당 동작을 테스트한다. 헬스체크의 고정 루프백 URL 사용 근거와 Semgrep 억제가 문서화되었다. Changes계정 병합 가드 강화
헬스체크 감사 경고 처리
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
aa5a9ea912f6c7cefd7e2457fa560a85c2370239. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- SAST Semgrep/Semgrep (multi-language SAST): FAILURE (https://github.com/ContextualWisdomLab/keyverse/actions/runs/30506532527/job/90757404078)
- Semgrep (multi-language SAST) check run: failure (https://github.com/ContextualWisdomLab/keyverse/actions/runs/30506532527/job/90757404078)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: service.py"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: service.py"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_merge.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_merge.py"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed. Findings1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
Failed checks:
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: service.py"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: service.py"]
R1 --> V1["required checks"]
Evidence --> S2["Test: test_merge.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_merge.py"]
R2 --> V2["targeted test run"]
|
…n healthcheck The central Semgrep SAST gate (sast-semgrep.yml, --severity WARNING/ERROR --error) blocked PR #34 on a single WARNING finding, rule python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected at services/account_unification/app/healthcheck.py:18. Semgrep does not honor the existing bandit-style `# noqa: S310`. This is a pre-existing finding (healthcheck.py), not in the PR #34 merge-guard diff (service.py). It is a verified-safe false positive: the urlopen target is the hardcoded loopback constant DEFAULT_URL; the module entrypoint calls main() with no argument, and the `url` parameter exists only for test injection of trusted URLs, so no untrusted/file:// value can reach urlopen. Because the finding is a genuine false positive for an .audit. advisory rule (no code fix removes it without breaking test injection, since any non-literal argument trips the pattern), it is scope-suppressed with a precise inline `# nosemgrep: <exact-rule-id>` plus a justification comment above the line; the bandit `# noqa: S310` is preserved. Suppression verified offline with a rule reproducing the finding (rc 1 -> 0). ruff, interrogate (100%), and pytest all pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REEc4WtvMHbGD23XK6xbLK
2 participants
Summary
Closes an account-takeover vector in
UnificationService.merge_accounts: the documented hard rule "never link or merge accounts on an unverified email" was only enforced inside theif not decision.matched:branch. When an operator setsexplicit_link=True,decide_matchclassifies the pair asEXPLICIT_LINK(matched=True), so the guard was skipped — two accounts coinciding only on an unverified (attacker-registerable) email were merged (survivor absorbs the duplicate's identities/roles/groups; duplicate tombstoned).Three authoritative sources require the refusal, all violated by the old code path:
app/models.pyMergeRequest.explicit_linkdocstring: "Even so, the service refuses if the only tie is an UNVERIFIED email."docs/merge-unification-flow.md: hard rule "If the accounts share only an unverified email, the merge is refused with 422" (unconditional).CLAUDE.md: "Never link or merge accounts on an unverified email."Fix
Hoist the check so it runs regardless of
decision.matched: refuse withUnverifiedEmailMergeErrorwhen the accounts share a non-empty, non-mutually-verified email and the match is not backed by a strong tie (EXACT_IDP_SUBJECTorVERIFIED_EMAIL). The legitimateexplicit_linkoverride (no shared email, or a strong tie) still merges; the reject-unverified-before-reject-no-match ordering from the merge-flow pseudocode is preserved.Behavior matrix (unchanged except the fixed case):
Verification (via
uv)test_explicit_link_cannot_override_shared_unverified_email+..._case_variant_unverified_emailfail before (DID NOT RAISE UnverifiedEmailMergeError) and pass after; the case-variant test also asserts nothing mutated ("dup" not in api.deactivated).uv run pytest→ 54 passed (was 52; +2 regression tests, 0 regressions).uv run ruff check app tests tools→ clean;uv run interrogate .→ 100%.The fuzz seam (
fuzz/fuzz_matching.py) targetsdecide_matchinvariants (correct/unchanged); this bug is in the service enforcement layer, so pytest is the right coverage.Generated by Claude Code
Summary by CodeRabbit
버그 수정
안정성