diff --git a/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-REVIEW.md b/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-REVIEW.md new file mode 100644 index 00000000..1a3e0486 --- /dev/null +++ b/.planning/phases/19.5-social-login-in-sm-user-plugin/19.5-REVIEW.md @@ -0,0 +1,257 @@ +--- +phase: 19.5-social-login-in-sm-user-plugin +reviewed: 2026-10-21T12:00:00Z +depth: standard +files_reviewed: 49 +files_reviewed_list: + - summercms.go/modules/cooler/cooler.go + - summercms.go/modules/cooler/memory.go + - summercms.go/modules/cooler/rediscache.go + - summercms.go/modules/cooler/pull_smoke_test.go + - summercms.go/modules/cooler/pull_test.go + - summercms.go/modules/cooler/docker_test.go + - summercms.go/scripts/check-phase19.5.sh + - fonoteka.go/plugins/golem15/user/classes/oauth_providers.go + - fonoteka.go/plugins/golem15/user/console/backfill_handles.go + - fonoteka.go/plugins/golem15/user/controllers/api_controller.go + - fonoteka.go/plugins/golem15/user/controllers/oauth_identities.go + - fonoteka.go/plugins/golem15/user/controllers/registration.go + - fonoteka.go/plugins/golem15/user/README.md + - sm-sociallogin-plugin/plugin.go + - sm-sociallogin-plugin/service.go + - sm-sociallogin-plugin/routes.go + - sm-sociallogin-plugin/config.go + - sm-sociallogin-plugin/admin_settings.go + - sm-sociallogin-plugin/admin_permissions.go + - sm-sociallogin-plugin/classes/flow.go + - sm-sociallogin-plugin/classes/registration.go + - sm-sociallogin-plugin/classes/callback.go + - sm-sociallogin-plugin/classes/statecookie.go + - sm-sociallogin-plugin/classes/codes.go + - sm-sociallogin-plugin/classes/providers_goth.go + - sm-sociallogin-plugin/classes/provider.go + - sm-sociallogin-plugin/classes/credentials.go + - sm-sociallogin-plugin/classes/config.go + - sm-sociallogin-plugin/classes/messages.go + - sm-sociallogin-plugin/controllers/callback.go + - sm-sociallogin-plugin/controllers/redirect.go + - sm-sociallogin-plugin/controllers/helpers.go + - sm-sociallogin-plugin/controllers/complete.go + - sm-sociallogin-plugin/controllers/link_ticket.go + - sm-sociallogin-plugin/controllers/register_complete.go + - sm-sociallogin-plugin/models/settings.go + - sm-sociallogin-plugin/updates/202610200001_create_golem15_sociallogin_settings.go + - sm-bm-app/frontend/src/lib/oauth.ts + - sm-bm-app/frontend/src/views/OAuthCallbackView.vue + - sm-bm-app/frontend/src/views/OAuthRegisterView.vue + - sm-bm-app/frontend/src/views/LoginView.vue + - sm-bm-app/frontend/src/views/ProfileView.vue + - sm-bm-app/frontend/src/stores/auth.ts + - sm-bm-app/frontend/src/pwa/sw.js + - sm-bm-app/sociallogin_test.go + - sm-bm-app/config/golem15.sociallogin.yaml + - sm-demo-app/sociallogin_test.go + - sm-demo-app/deploy/env.example + - sm-demo-app/go.work +findings: + critical: 2 + warning: 5 + info: 9 + total: 16 +status: issues_found +--- + +# Phase 19.5: Code Review Report + +**Reviewed:** 2026-10-21 +**Depth:** standard +**Files Reviewed:** 49 (listed in the frontmatter; paths prefixed with the repo dir) +**Status:** issues_found + +## Summary + +Scope: `cooler.Cache.Pull` (5e0a04e, 671d388), the gate script, the sm-user-plugin diff 1fc0ce7..c7635c5, the whole of sm-sociallogin-plugin, the bm frontend and wiring (e76f8bc, 117ef8f) and the demo wiring (9eb8492, d5a98d9, 61a00e9). + +The low-level pieces are solid. AES-GCM sealing with HKDF purpose keys, the cookie name as AAD, constant-time state compare, `GETDEL` and otter `Compute` for Pull, the `FOR UPDATE` unlink and the callback-target rebuild all match what the security review says. I found no defect in them. + +The account-linking model is still not safe, and the review lists it as closed: + +- **CR-01 (T-19.5-38 mitigation broken under default config).** `ownerVerifiedEmail` treats `has_self_set_password` as proof that the owner controls the address. With golem15.user's default `activate_mode: auto`, `POST /register` creates an activated account with a self-set password for any email, and nobody proves the address. An attacker who registers the victim's email first gets the victim's later verified Google or GitHub sign-in linked onto the attacker's account. This is the same pre-account takeover, now via password registration. +- **CR-02 (the "reset proves the address" path leaves the attacker's identity linked).** The fix's recovery path, a password reset, does not remove a pre-planted Microsoft or Facebook identity. bm runs Microsoft with tenant `common`, so the attacker keeps permanent access to an account the victim "recovered". + +The warnings cover: + +- the one-time handoff code is not bound to the browser that started the flow (login CSRF; in `token_cookie` mode a plain GET is enough); +- registration consent, including marketing, comes from a GET that any site can trigger; +- the reusable pending code and the email sit in URLs and browser history; +- `localhost` and `127.0.0.1` are always allowed as callback hosts in production; +- the gate's additive-API check has blind spots. + +Core-plugin check (sm-user-plugin): no exported declaration was removed or changed in signature. The only behaviour change is the relaxed unlink rule and the wider default allow-list (IN-01). It is intentional (D-13), documented in the README, and not raised as critical. + +## Narrative Findings (AI reviewer) + +## Critical Issues + +### CR-01: A verified sign-in still auto-links onto an attacker-created account, because a self-set password does not prove the address under `activate_mode: auto` (T-19.5-38 not actually closed) + +**File:** `sm-sociallogin-plugin/classes/flow.go:97-101`, `sm-sociallogin-plugin/classes/flow.go:202-207`, `sm-sociallogin-plugin/classes/flow.go:262-264`; root cause `fonoteka.go/plugins/golem15/user/controllers/api_controller.go:658` with `fonoteka.go/plugins/golem15/user/config/config.yaml:10` (`activate_mode: auto`, the default; neither sm-bm-app nor sm-demo-app overrides it) + +**Issue:** `ownerVerifiedEmail(u)` returns `u.HasSelfSetPassword`. Its doc comment and the security review assume a self-set password means "registered with one, or set it through the mailed reset code", i.e. the owner proved the address. That is false for `POST /_user/api/v1/register` with the default `activate_mode: auto`. `RegisterUser(..., RegisterOptions{Activate: true})` stores `has_self_set_password = true` and an activated account, and no mail is ever sent to the address. + +Failure scenario (default config, any app with Google or GitHub enabled): +1. The attacker calls `POST /register` with `email=victim@gmail.com` and a password of their choice. The account is activated immediately (auto mode) with `has_self_set_password = true`. +2. Later the victim clicks "Sign in with Google". Google asserts `email_verified = true` for `victim@gmail.com`. +3. `HandleCallback` finds the email match. `profile.EmailVerified && ownerVerifiedEmail(match)` is true, so `loginWithIdentity` links the victim's Google identity onto the attacker's account and signs the victim in. +4. The victim now uses an account the attacker can enter at any time with the password. This is the classic-federated-merge pre-account takeover that T-19.5-38 was meant to close. `CompletePending` (lines 202-207) has the same hole. + +`TestPreAccountTakeover` only covers the password-less (social-created) variant, so it stays green. + +**Fix:** Auto-link only on proof of address ownership, not on a password flag. Options, from simplest: +```go +// Simplest and safest: never auto-link on an email match. Always go pending +// (unverified_match / 422 email_exists_link_account); the owner signs in and +// links from the profile with a link ticket, as bm already tells its users. +if match != nil { + return pendingOutcome(provider, profile, returnTo, ReasonUnverifiedMatch, st.BetaKey), nil +} +``` +Alternatively, keep auto-link but gate it on a real proof signal: an additive `email_verified_at` column in golem15.user, set by `VerifyActivationCode`, `VerifyResetCode` and admin activation, never by auto-mode registration. Then use `ownerVerifiedEmail(u) = u.EmailVerifiedAt != nil`. At minimum, refuse auto-link when `RegistrationSettings` reports `mode == "auto"`. Update README rows 49-51 and the security review's T-19.5-38 entry. + +### CR-02: An account pre-created from an unverified provider email keeps the attacker's identity after the victim "proves the address" with a password reset + +**File:** `sm-sociallogin-plugin/classes/registration.go:141` (`Activate: true` for every social account, also when `p.EmailVerified` is false); `fonoteka.go/plugins/golem15/user/classes/codes.go:94-114` (`VerifyResetCode` sets `has_self_set_password` and leaves `golem15_user_oauth_identities` alone); the documented recovery path in `sm-sociallogin-plugin/README.md:50` and `sm-sociallogin-plugin/classes/flow.go:254-261` + +**Issue:** The T-19.5-38 fix stops the *auto-link* onto a password-less account. It still creates an **activated** account from an email the provider did not verify (Microsoft, Facebook, or Google without the flag). Its recovery advice ("the owner proves the address with the mailed password reset, after which verified providers link again") hands the victim an account that still carries the attacker's identity. + +Failure scenario on sm-bm-app (`config/golem15.sociallogin.yaml`, tenant `common` by default; DEPLOY.md lists `common` as a valid choice): +1. The attacker controls an Entra tenant and sets `mail = student@uwr.edu.pl` on a user. They sign in at `/oauth/microsoft`, get pending (no match), accept the consent, and `CompletePending` creates an activated password-less account for `student@uwr.edu.pl` with the attacker's Microsoft identity linked. +2. The real student tries to register: the email is taken. They use "Nie pamiętam hasła", receive the reset code (the address really is theirs) and set a password. `VerifyResetCode` sets `has_self_set_password = true` and nothing else. +3. The student uses the account (quizzes, presentations, profile). The attacker signs in with their Microsoft identity whenever they like (`identityUser` → `loginWithIdentity`), because the identity was never removed. bm's profile does not list linked identities, so the student cannot see it. + +**Fix:** Close both ends. +- Do not create an activated account from an unverified email. Use `RegisterOptions{Activate: p.EmailVerified, ...}`. When it is false, send golem15.user's activation mail (as `activate_mode: user` does) and answer the frontend with "check your mail". `checkCanSignIn` already refuses inactive users. +- When a password-less account's owner proves the address (activation or reset code), revoke what was attached before the proof: delete the user's `oauth_identities` rows and set `tokens_valid_after = now`. sm-user-plugin is a core plugin, so do it additively: fire a new event (e.g. `user.EmailProven`) from `VerifyResetCode` and `VerifyActivationCode` when `HasSelfSetPassword` flips from false, and have golem15.sociallogin drop the identities in its listener. Add a test: pre-created Microsoft account → reset → the Microsoft identity no longer signs in. + +## Warnings + +### WR-01: The one-time completion code is not bound to the browser that started the flow (login CSRF; a plain GET in `token_cookie` mode) + +**File:** `sm-sociallogin-plugin/controllers/callback.go:113-124`, `sm-sociallogin-plugin/controllers/complete.go:17-35`, `sm-sociallogin-plugin/controllers/helpers.go:148-168`; `sm-bm-app/frontend/src/views/OAuthCallbackView.vue:36-52` + +**Issue:** The state cookie protects `/oauth/{provider}/callback`, but the second leg does not. `?code=` on the frontend callback, redeemed by `GET /_user/api/v1/oauth-complete`, is a bearer value that any browser can redeem. + +An attacker runs the flow with their own provider account, stops at `https://app/auth/oauth/callback?code=X` and sends that link to the victim (or iframes or redirects to it). The bm view redeems it and calls `setSession(token, user, true)`: the victim is now persistently signed into the attacker's account. If the victim was already signed in, their session is silently replaced. Anything they upload or enter (quiz answers, presentations) lands in the attacker's account. + +With `golem15.sociallogin.token_cookie: true` it is worse. A top-level GET to `/_user/api/v1/oauth-complete?code=X` sets the session cookie directly, with no SPA involved. + +**Fix:** Bind the code to the initiating browser while keeping the PHP response shape. The frontend generates a random verifier, keeps it in `sessionStorage` and sends `code_challenge = base64url(SHA-256(verifier))` with the start request. Seal the challenge in `State` and copy it into `CompletionPayload` and `PendingPayload`. `oauth-complete` and `oauth-register-complete` then require a matching `code_verifier` and answer 410 otherwise. Clients that send no challenge can keep the old behaviour behind a config flag during migration. For same-site deployments, a `__Host-` binding cookie set at `/oauth/{provider}` and checked at `oauth-complete` also works. + +### WR-02: Registration consent (terms, privacy and **marketing**) is read from a cross-site-triggerable GET and recorded as given + +**File:** `sm-sociallogin-plugin/controllers/redirect.go:69-72`, `sm-sociallogin-plugin/controllers/redirect.go:143-155`; `sm-sociallogin-plugin/classes/flow.go:103-119`; `sm-sociallogin-plugin/classes/registration.go:127-129`, `:165-177` + +**Issue:** `action=register&terms_accepted=1&privacy_accepted=1&marketing_consent=1` on `GET /oauth/{provider}` is sealed into the state cookie as the user's consent. On callback the account is created with `marketing_consent = true`, and the register event records the consent with the victim's IP and user agent. + +Any page can start that navigation. GitHub, and Microsoft or Google with an existing session, often return without showing anything new, so a victim who never saw the terms ends up with an account and a recorded opt-in to marketing. That is a forged GDPR consent record, and the "repudiation" protection in T-19.5-21 records the forgery as genuine. + +**Fix:** Do not take consent from the start request. Always route a first sign-in through the pending screen and collect consent with the POST `oauth-register-complete`, which already exists. If the PHP contract must keep `action=register`, at least ignore `marketing_consent` on the GET and require a same-origin or CSRF-bound nonce (e.g. the WR-01 challenge) before honouring the GET's consent. + +### WR-03: The pending registration code (reusable for 10 minutes) and the user's email are kept in URLs and browser history + +**File:** `sm-sociallogin-plugin/controllers/callback.go:102-110`; `sm-bm-app/frontend/src/views/OAuthCallbackView.vue:27-31`; `sm-bm-app/frontend/src/views/OAuthRegisterView.vue:17-18` + +**Issue:** The callback view takes the completion code out of the URL, but for the pending case it does `router.replace({ name: 'oauth-register', query: { pending, provider, reason, email } })`. The pending code and the email stay in the address bar and history. + +The pending code is not consumed by reads (`GetPending`). It stays valid for `PendingTTL` (10 min) until someone completes it, and completing it returns a session token for the new account. On a shared or lab computer, through history sync or screen sharing, or in a separate frontend host's access log (the Nuxt apps that will use this plugin), a third party can finish the registration first and take the session. The email in the query is also PII in logs. T-19.5-26 covers only the completion code. + +**Fix:** In the frontend, move `pending`, `provider`, `reason` and `email` into `history.state` (`router.replace({ name: 'oauth-register', state: {...} })`) or `sessionStorage`, and keep the URL bare. On the server, keep the PHP redirect shape but consider shortening `PendingTTL` and binding the pending code to the browser as in WR-01. + +### WR-04: `localhost` and `127.0.0.1` are always allowed `frontend_callback` hosts, also in production + +**File:** `sm-sociallogin-plugin/classes/callback.go:20-36` (lines 22-23) + +**Issue:** `NewHostSet` always adds `localhost` and `127.0.0.1`. A crafted link `https://app/oauth/github?frontend_callback=http://localhost:4444/auth/oauth/callback` sends the victim's one-time login code to whatever listens on that port on the victim's machine: another local user's process on a shared machine, a dev server or a malicious local app. Providers that auto-approve (GitHub, or Microsoft with an existing session) need no interaction beyond the click. The config comment presents this as intended, but nothing ties it to development. + +**Fix:** Add the loopback hosts only outside production (e.g. when `app.debug` is true or `SUMMER_ENV` is `local`/`testing`), or only through an explicit `allowed_hosts` entry: +```go +if cfg.AllowLoopback { // golem15.sociallogin.allow_loopback, default false; true in dev config + s.Add("localhost"); s.Add("127.0.0.1") +} +``` + +### WR-05: The gate's "additive only" check for the core user plugin misses several kinds of breaking change + +**File:** `summercms.go/scripts/check-phase19.5.sh:328-330` (`removed_exports`), `:427-437` + +**Issue:** `removed_exports` only catches a removed line that starts with `func `, `func (recv) ` or `type `. T-19.5-04 relies on it as a guarantee, but it is green for: +- a removed or renamed exported struct field, such as a field of `RegisterOptions` or `OAuthIdentitiesOptions`; +- an exported `const` or `var`, including members of `const (...)` and `var (...)` blocks; +- a method **added** to an exported interface, which breaks every implementer; +- a changed config key in `config/*.yaml`, a route path or middleware in `routes.go`, or a JSON payload key. + +The check is limited to `*.go` lines starting with `func`/`type`. So the claim "no route, payload, table, migration or config key changed" is checked only for migrations. + +**Fix:** Compare the exported API of the two commits instead of grepping diff lines. For example, run `go doc -all ./... | sort` (or `apidiff` from golang.org/x/exp, which would need a dependency note) at `$USER_BASE` (via `git worktree`) and at HEAD, and refuse any removed or changed line. Also add a `git diff --name-only "$USER_BASE" -- config/ routes.go` check that requires review. + +## Info + +### IN-01: Core plugin behaviour change in `OAuthIdentitiesDestroy` (intentional, D-13) + +**File:** `fonoteka.go/plugins/golem15/user/controllers/oauth_identities.go:31-35`, `:116-121`, `:126-134` +**Issue:** A user with a self-set password and a single identity used to get 409 and now gets 204. When no `Providers` option is set, the allow-list is now the provider source's `KnownOAuthProviders()`, or the default list, which now includes `microsoft`. The change is documented in the README and was decided in D-13. I note it only so that consumers that relied on the old 409 (fonoteka, the PHP-ported frontends) are checked. There is also a small race: an identity deleted between the `First` lookup (line 82) and the guarded DELETE yields 409 `last_method_blocked` instead of 404. +**Fix:** None required for the rule. For the race, return 404 when `deleted == 0` and the row no longer exists (re-check inside the transaction after the lock). + +### IN-02: The registration throttle fails open + +**File:** `fonoteka.go/plugins/golem15/user/controllers/api_controller.go:744-750` (used by `sm-sociallogin-plugin/classes/registration.go:76`) +**Issue:** `RegistrationsFromIP` ignores the query error and returns 0, and `checkRegistrationAllowed` skips the throttle when `ClientIP` is empty. On a database hiccup both creation paths stop throttling. This matches `/register`, but the exported helper now carries the behaviour into a second plugin. +**Fix:** Have the exported helper return `(int64, error)` (a new function, to stay additive) and fail closed in golem15.sociallogin. + +### IN-03: The `fb_no_email` pending registration can never complete + +**File:** `sm-sociallogin-plugin/classes/flow.go:90-91`, `:197-229`; `sm-sociallogin-plugin/controllers/register_complete.go:37-189` +**Issue:** A provider without an email goes pending. `oauth-register-complete` accepts no email field and the payload's email is empty, so `RegisterUser` always answers 422 `errors.email` (pinned by `register_complete_test.go:187`). bm's consent form shows no email field error, so the user is stuck on an unsolvable form. +**Fix:** Either accept an `email` field when `payload.Email == ""` (treated as unverified, sent through activation), or have the frontend show "your provider shared no email, register with a password" for `reason=fb_no_email`. + +### IN-04: The Microsoft tenant is validated loosely in the admin form and not at all from config or env + +**File:** `sm-sociallogin-plugin/models/settings.go:79-90`; `sm-sociallogin-plugin/classes/credentials.go:38` +**Issue:** `^[A-Za-z0-9.-]+$` accepts `.` and `..`, and the tenant is put into the authority path (`login.microsoftonline.com//oauth2/...`). A tenant from config or `MICROSOFT_TENANT` is not checked at all. Only administrators control it, so the impact is a misdirected authority URL, not an attack. +**Fix:** Require `common|organizations|consumers||`, and apply the same check in `ResolveCredentials` (refuse the provider as not configured on a bad value). + +### IN-05: The Redis `Pull` needs Redis 6.2 or newer, with no boot-time check + +**File:** `summercms.go/modules/cooler/rediscache.go:55-69` +**Issue:** On an older server `GETDEL` fails with "unknown command" at request time. Every oauth-complete, register-complete and link ticket then answers 500. The requirement is only documented. +**Fix:** Check the version once (`INFO server` → `redis_version`) when the Redis cache is built, and log or refuse a version below 6.2. Alternatively, fall back to a `MULTI GET DEL EXEC` transaction. + +### IN-06: An added method on an exported interface shipped as a patch release + +**File:** `summercms.go/modules/cooler/cooler.go:77-81` +**Issue:** Adding `Pull` to `cooler.Cache` breaks any out-of-tree `Cache` implementation (decorators, test fakes) and shipped as v0.3.0 → v0.3.1. No implementation exists in the sibling repositories (checked), so nothing breaks today. +**Fix:** Bump the minor version for interface additions, or mention it in the changelog post for the tag. + +### IN-07: The state cookie is not `Secure` on sm-bm-app + +**File:** `sm-sociallogin-plugin/classes/statecookie.go:79-88`; `sm-bm-app/config/golem15.sociallogin.yaml` +**Issue:** `StateCookieSecure` falls back to `auth.cookie.secure`, else an https `app.url`. bm has no `app.url`, so the sealed state cookie goes out over plain http too. The value is encrypted and short-lived, so the impact is low. +**Fix:** Set `auth.cookie.secure: true` (or `app.url`) in bm's production config, or derive Secure from `r.TLS` / a trusted `X-Forwarded-Proto` when nothing is configured. + +### IN-08: The app-level social login tests depend on the host environment + +**File:** `sm-bm-app/sociallogin_test.go:26-29`, `:99-102`; `sm-demo-app/sociallogin_test.go` (`socialServer`) +**Issue:** `copyLegacyEnv` reads `GOOGLE_CLIENT_ID`, `FACEBOOK_*`, `GITHUB_*`, `MICROSOFT_*`, `BETATESTER_KEY` and `FRONTEND_URL` from the process environment. A developer or CI host with any of them set changes the provider list ("only microsoft") or the redirect assertions, which makes the tests flaky on such hosts. +**Fix:** `t.Setenv(name, "")` for every `legacyEnv` variable the test does not set on purpose (export the list from the plugin, or copy it into a test helper). + +### IN-09: The gate's vitest detector refuses on words in test titles or logs + +**File:** `summercms.go/scripts/check-phase19.5.sh:221` +**Issue:** `\bfailed\b|\bskipped\b|\btodo\b|\bFAIL\b` is matched against the whole output. A test titled "shows an error when the login failed", or a component logging "FAIL" to stderr, fails the gate although every test passed. This fails closed, so it gives a false red, never a false green. Separately, `mktemp` logs in `go_json` and `run_bm` are not removed when the script is interrupted (no `trap`). +**Fix:** Run vitest with `--reporter=json --outputFile=...` and check `numFailedTests`, `numPendingTests` and `numTodoTests` and the per-file status. Add `trap 'rm -f "$log" "$err"' RETURN` in `go_json`. + +--- + +_Reviewed: 2026-10-21_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_