From ff4e0fa7adad8cc273dfdef6cc3e0235d1a6556b Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Wed, 21 Oct 2026 15:10:32 +0200 Subject: [PATCH] docs(phase-19.5): add security threat verification --- .../19.5-SECURITY-REVIEW.md | 2 +- .../19.5-SECURITY.md | 111 ++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 .planning/phases/19.5-social-login-in-sm-user-plugin/19.5-SECURITY.md diff --git a/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-SECURITY-REVIEW.md b/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-SECURITY-REVIEW.md index c58d7643..f8147b2b 100644 --- a/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-SECURITY-REVIEW.md +++ b/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-SECURITY-REVIEW.md @@ -94,7 +94,7 @@ The rest of the pins are this plan's unit tests (Tasks 2 to 4); the register nam | Finding | Severity | Description | Status | |---------|----------|-------------|--------| -| 19.5-REVIEW CR-02 | high (review) | An account pre-created from an unverified provider email (sociallogin creates it activated, S `classes/registration.go`) keeps the attacker's identity after the real owner proves the address with a reset-code password reset: the reset stamps `email_verified_at` and sets a password but leaves `golem15_user_oauth_identities` alone, so the attacker keeps signing in through the pre-planted Microsoft or Facebook identity. | open, no user decision; not covered by D-26..D-28 | +| 19.5-REVIEW CR-02 | high (review) | An account pre-created from an unverified provider email (sociallogin creates it activated, S `classes/registration.go`) keeps the attacker's identity after the real owner proves the address with a reset-code password reset: the reset stamps `email_verified_at` and sets a password but leaves `golem15_user_oauth_identities` alone, so the attacker keeps signing in through the pre-planted Microsoft or Facebook identity. | accepted by the user 2026-10-21 (wording fixed in sm-sociallogin-plugin 79cebb9; 19.5-SECURITY.md AR-19.5-01) | ## Log calls diff --git a/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-SECURITY.md b/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-SECURITY.md new file mode 100644 index 00000000..78bf7fa6 --- /dev/null +++ b/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-SECURITY.md @@ -0,0 +1,111 @@ +--- +phase: "19.5" +slug: "social-login-in-sm-user-plugin" +status: verified +# threats_open = count of OPEN threats at or above workflow.security_block_on severity (the blocking gate) +threats_open: 0 +asvs_level: 1 +created: "2026-10-21" +--- + +# Phase 19.5 — Security + +> Per-phase security contract: threat register, accepted risks, and audit trail. The per-threat evidence (implementation lines, the test that fails without each mitigation, mutation checks, log sweep) is in `19.5-SECURITY-REVIEW.md`; this file is the register and sign-off. + +--- + +## Trust Boundaries + +| Boundary | Description | Data Crossing | +|----------|-------------|---------------| +| Browser → `/oauth/{provider}`, `/oauth/{provider}/callback` | Untrusted query (state, code, callback, return_to, action, link_ticket) and the sealed state cookie | OAuth state, provider authorization code | +| Server → OAuth providers (Google, GitHub, Microsoft, Facebook) | goth wrappers over HTTPS; provider profile and email-verified claims are untrusted input | Provider tokens, profile, email | +| SPA → `/_user/api/v1/oauth-*` | One-time completion, pending-registration and link-ticket codes | JWT, user payload, consent | +| sociallogin → golem15.user | Account matching, creation, linking under row locks; `users.email_verified_at` proof | User records, identities (encrypted tokens) | +| Admin → settings | Client ids and encrypted, write-only secrets | Client secrets | + +--- + +## Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation | Status | +|-----------|----------|-----------|----------|-------------|------------|--------| +| T-19.5-01 | Spoofing | cooler.Cache.Pull (memory.go, rediscache.go) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestPullConcurrent` (64 pullers, 20 rounds, memory and miniredis), `TestPullExpired`, `TestPullPrefixIsolation`, `TestPullDocker` (redis:7-alpine) (F) | closed (mutation-checked) | +| T-19.5-02 | Denial of service | OAuthIdentitiesDestroy | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestOAuthIdentitiesDestroyRace` (two transactions in both orders, the handler waiting for the lock, concurrent handlers), `TestOAuthIdentitiesDestroyConcurrent | closed (mutation-checked) | +| T-19.5-03 | Elevation of privilege | OAuthIdentitiesDestroy rule relaxation (D-13) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestOAuthIdentitiesDestroyRule` (table incl. another user's identity 404, EN and PL 409 bodies) (U) | closed | +| T-19.5-04 | Tampering | sm-user-plugin core contract | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: gate `--userplugin`: no exported declaration removed and no file under `updates/` changed since 4c5d3b8; `--fonoteka` green (F) | closed | +| T-19.5-05 | Spoofing | controllers.FinishLogin restoring a soft-deleted account | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestFinishLogin/trashed_user_restored_with_one_mail` (U); `TestSoftDeleted/unverified_match_pending_and_no_second_row` (an unverified match never restores) (S) | closed | +| T-19.5-06 | Information disclosure | logs of the new code | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: section "Log calls" | closed: accepted: a single fixed-shape log line with no token, secret or code; nothing to pin beyond the review | +| T-19.5-07 | Spoofing | Callback (login CSRF, injected code) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestStateCookie/callback` (missing cookie, empty and unequal state, replay after clear, another provider's callback, cleared on success, provider error, failed | closed | +| T-19.5-08 | Tampering | SealState and OpenState | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestStateCookie/refused` (16 cases), `age_window`, `seal_limits`, `key_domain_separated` (new) (S) | closed | +| T-19.5-09 | Tampering | NormalizeFrontendCallback, NormalizeReturnTo | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestNormalizeFrontendCallback` (44 subtests), `TestNormalizeReturnTo` (22 subtests) (S); `TestLinkSmoke/bad_callback_falls_back` (S) | closed | +| T-19.5-10 | Elevation of privilege | HandleCallback email matching (account takeover through an unverified or attacker-set email) | critical | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestEmailVerified`, `TestNOAuth` (Microsoft mail of an existing user in four tenants: pending, no identity, completion 422), `TestMatching`, `TestPHPSocialAuth | closed (the creation paths: T-19.5-38 and T-19.5-39, closed) | +| T-19.5-11 | Elevation of privilege | RegisterComplete (pending registration hijack) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestRegisterComplete/email_exists_link_account_keeps_code`, `TestPHPPendingRegistrationTakeover` (incl. a payload sealed without the flag) (S) | closed | +| T-19.5-12 | Spoofing | PullCompletion, PullPending, PullLinkTicket (code replay or race) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestCodes` (incl. moved values, 32 concurrent pulls), `TestOAuthComplete`, `TestRegisterComplete/concurrent_submit_one_winner` (S) | closed: fixed in commit 21f3fe5 (with its test) | +| T-19.5-13 | Elevation of privilege | beta gate (D-12) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestBetaGate` (S) | closed | +| T-19.5-14 | Spoofing | action=link and LinkTicket (linking a provider to the wrong account) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestLink` (bearer, both cookies, blacklisted token, `tokens_valid_after`, trashed at start and at callback, token in query ignored), `TestLinkTicket`, `TestPHP | closed | +| T-19.5-15 | Information disclosure | URLs and logs | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestLinkTicket/no_jwt_in_locations`, `TestSettings/no_secret_in_logs` (a provider whose errors carry the secret and a token), `TestGothProviders/expired_code_w | closed | +| T-19.5-16 | Information disclosure | client secrets and provider tokens at rest | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestSettings/admin_json_hides_secrets` (stored, env and empty states; a saved secret is ciphertext), `TestIdentityTokens/encrypted_tokens`, `TestCodes/completi | closed: fixed in commit c99f5f9 (the write path; confidentiality held) | +| T-19.5-17 | Denial of service | provider HTTP calls and /oauth routes | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestGothProviders/timeout` (all four wrappers against a stalled transport, the configured timeout), `TestRouteMiddleware` (route table, 21st request 429) (S) | closed | +| T-19.5-18 | Tampering | route shadowing with sm-oauthserver-plugin | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestRegistry/reserved_names`, `TestRoutes/unknown_and_reserved_404` (S); `TestSocialLoginCoexistsWithOAuthServer` (D) | closed | +| T-19.5-19 | Elevation of privilege | login of inactive or banned users | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestInactiveAndBanned` (identity, verified match, link, pending completion), `TestSoftDeleted` (S) | closed | +| T-19.5-20 | Denial of service | account creation flood | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestRegistrationGates` (S) | closed | +| T-19.5-21 | Repudiation | consent records | low | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestIdentityTokens/register_event_payload`, `TestRegisterComplete/form_encoded` (S) | closed | +| T-19.5-22 | Spoofing | stale consent | low | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestConsent/expired_after_600s` (forged 601 s and 590 s cookies) (S) | closed | +| T-19.5-23 | Tampering | remote operations (Task 6) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: release control | closed: accepted: a process control, recorded in 19.5-02-SUMMARY | +| T-19.5-24 | Tampering | jz.bm static SPA fallback | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestStaticReservedPaths` (B, gate `--bm`); `pwa.test.ts` (B) | closed | +| T-19.5-25 | Information disclosure | committed configuration and env templates | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: grep in this review: no non-empty `CLIENT_SECRET=` or `client_secret:` in D or B config and templates | closed: accepted: verified by grep; no runtime behaviour to test | +| T-19.5-26 | Information disclosure | OAuthCallbackView (code in history and logs) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestOAuthComplete/once_with_shape` (second use 410) (S); `oauth.test.ts` (B); `src/views/OAuthCallbackView.test.ts` (B, 5fd0cc3: the real view mounted with vue | closed (the view half by `OAuthCallbackView.test.ts`, plan 19.5-05) | +| T-19.5-27 | Spoofing | Microsoft sign-in on bm (nOAuth, tenant misconfiguration) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestNOAuth` (four tenants) (S); `TestSocialLoginMounted` (B) | closed (the creation paths: T-19.5-38 and T-19.5-39, closed) | +| T-19.5-28 | Information disclosure | link ticket in the bm profile flow | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestLinkTicket` (S); `oauth.test.ts` (B) | closed | +| T-19.5-29 | Tampering | pushes of shared repositories | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: release control | closed: accepted: a process control, recorded in 19.5-03-SUMMARY | +| T-19.5-30 | Denial of service | fonoteka regression from the D-13 relaxation | low | mitigate | see 19.5-SECURITY-REVIEW.md; tests: gate `--fonoteka` (F); `TestOAuthIdentitiesDestroyRule` (U) | closed (the unrelated `TestPhase09SecurityRoutes` failure is outside this phase, deferred-items.md) | +| T-19.5-31 | Repudiation | scripts/check-phase19.5.sh (false green) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `scripts/check-phase19.5.sh --self-test` (F) | closed | +| T-19.5-32 | Tampering | race tests (D-13, D-08) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: the mutation checks above (F, U) | closed | +| T-19.5-33 | Elevation of privilege | security review completeness | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: the plan's acceptance count | closed | +| T-19.5-34 | Information disclosure | test fixtures and logs | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestGothProviders/fake_transport_refuses_unknown_hosts`, `TestSettings/no_secret_in_logs` (S) | closed | +| T-19.5-35 | Spoofing | PHP map honesty | low | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestPHPMethodMap` (mutation-checked) (S) | closed | +| T-19.5-36 | Elevation of privilege | post-release fixes (Tasks 5-7) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: Task 7's acceptance criteria | closed: release control: Task 6 reply "release"; v0.1.1 and the pointer bumps checked by Task 7's acceptance criteria (19.5-04-SUMMARY) | +| T-19.5-37 | Tampering | remote operations in Task 7 | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: Task 7's acceptance criteria | closed: release control: recorded with Task 7's acceptance criteria | +| T-19.5-39 | Elevation of privilege | sociallogin `ownerVerifiedEmail` (password-registration pre-account takeover, 19.5-REVIEW CR-01) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestOwnerVerifiedEmail/register_auto_then_google_pending`, `register_action_pending`, `legacy_account_pending`, `mail_activated_by_code_links`, `reset_code_pas | closed (RED: the self-set-password rule fails `register_auto_then_google_pending`) | +| T-19.5-40 | Spoofing | sm-user-plugin `Update` and `FormBeforeUpdate` (proof carried over an email change) | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestEmailVerifiedAt/update_email_change_clears`, `update_same_email_keeps`, `TestAdminEmailVerifiedAt/admin_email_change_clears`, `admin_save_keeps_proof` (U); | closed (RED: the controllers before a99cb46 fail exactly the two `*_email_change_clears` cases) | +| T-19.5-41 | Tampering | /register and /update request bodies (mass assignment of the proof) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestEmailVerifiedAt/register_ignores_submitted_proof`, `update_ignores_submitted_proof` (U) | closed | +| T-19.5-42 | Elevation of privilege | admin create and Activate action, `/register` auto and admin modes, social sign-in (non-mailbox paths treated as proof) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestEmailVerifiedAt/register_auto_no_proof`, `register_admin_mode_no_proof`, `change_password_no_proof`, `TestAdminEmailVerifiedAt/admin_create_no_proof`, `adm | closed | +| T-19.5-43 | Information disclosure | user API payloads and admin JSON (contract drift in a core plugin) | low | mitigate | see 19.5-SECURITY-REVIEW.md; tests: the payload-key assertion in `TestEmailVerifiedAt` (no `email_verified_at` in any answer), `TestAdminEmailVerifiedAt/admin_json_hides_proof` (U); gate `--userpl | closed | +| T-19.5-44 | Tampering | migration `202610210001_add_users_email_verified_at` (data rewrite or a broken upgrade) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `TestEmailVerifiedAtMigration` (`column_shape`, `no_backfill`, `existing_column_kept`, `rollback_keeps_column`, `history_once`), `TestCreateOrganisationsMigrati | closed | +| T-19.5-45 | Repudiation | scripts/check-phase19.5.sh (false green for the new tests) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: `--self-test` (`missing-view-file` exits 5, the migration-check cases) (F) | closed | +| T-19.5-46 | Tampering | release and pointer bumps (Task 7) | medium | mitigate | see 19.5-SECURITY-REVIEW.md; tests: Task 7's acceptance criteria | closed: no remote operation ran (user answered hold); control carried by the release todo (g15office sqnwn1zi) | +| T-19.5-SC | Tampering | Go module installs | high | mitigate | see 19.5-SECURITY-REVIEW.md; tests: gate `--sociallogin` (deps and importer checks) (F) | closed | + +*Status: open · closed · open — below high threshold (non-blocking)* +*Severity: critical > high > medium > low — only open threats at or above workflow.security_block_on count toward threats_open* +*Disposition: mitigate (implementation required) · accept (documented risk) · transfer (third-party)* + +--- + +## Accepted Risks Log + +| Risk ID | Threat Ref | Rationale | Accepted By | Date | +|---------|------------|-----------|-------------|------| +| AR-19.5-01 | 19.5-REVIEW CR-02 | An account sociallogin created from an unverified provider email (Facebook, Microsoft) keeps that identity after the real owner proves the address with a reset-code password reset. Behaviour accepted; the sociallogin README (79cebb9) states that proving the address removes no identity already linked and that the owner removes it from the identities list. | user | 2026-10-21 | +| AR-19.5-02 | T-19.5-39 (release) | The email-proof fix is committed but not released (user answered hold at 19.5-05 Task 6); deployed sm-demo-app and sm-bm-app keep the password-registration takeover path until `.planning/todos/pending/email-proof-release.md` (g15office sqnwn1zi) ships with the 19.8 release. | user | 2026-10-21 | +| AR-19.5-03 | low residuals | Redirect URI from Host/X-Forwarded-Proto without `app.url`; provider factory errors logged as written; `parental_authority=false` ignored at completion (PHP parity). See the review's "Accepted residuals (low)". | review (low, below block threshold) | 2026-10-21 | + +--- + +## Security Audit Trail + +| Audit Date | Threats Total | Closed | Open | Run By | +|------------|---------------|--------|------|--------| +| 2026-10-21 | 46 | 46 | 0 | 19.5-04/05 security review (msd-executor) + /msd-secure-phase (orchestrator, ASVS L1 register check) | + +--- + +## Sign-Off + +- [x] All threats have a disposition (mitigate / accept / transfer) +- [x] Accepted risks documented in Accepted Risks Log +- [x] `threats_open: 0` confirmed +- [x] `status: verified` set in frontmatter + +**Approval:** verified 2026-10-21