fix: reduce OAuth login-CSRF risk (RSK-046)

.ilana/decisions.md

@@ -244,3 +244,4 @@ Process decisions above (DEC-001..DEC-007) belong to the v0.23 FLEET run and sta
 - DEC-237 [v0.47 phase 3c]: timing. Reminder at E-72h, exactly once per period (atomic `UPDATE ... RETURNING` claim on `plan_reminder_period_end`; released if no owner email could be sent). Charges only within 48h before E, only after a "will charge" reminder for that exact period at least 24h old (toggling auto_renew clears the marker, so a late opt-in gets a fresh reminder and may lapse if < 24h remain), max 3 attempts >= 12h apart, all before E. After failures the tenant lapses at E through the unchanged `DowngradeLapsedPlans` (DEC-227 retention pin). Owners are emailed on every success and failure.
 - DEC-238 [v0.47 phase 3c]: at most one in-flight charge per tenant period. Claim under tenant `FOR UPDATE`, committed as `pending` before the network call; a pending attempt (outcome unknown) blocks all further attempts until `verify` settles it (success -> `ApplyPlanPayment`, idempotent by reference; failed/abandoned/reversed/"reference not found" -> failed). A success whose amount/currency differs from the claim is never applied and stays pending (logged `billing_renewal_amount_mismatch` every pass) for manual resolution. Verified by `TestClaimRenewalAttemptConcurrentExactlyOnce` (16 racers; fails when `FOR UPDATE` is removed) and `TestAutoRenewConcurrentPassesChargeOnce` (12 concurrent passes -> 1 Paystack charge, 1 extension).
 - DEC-239 [OAuth/MFA + auto-renewal, CodeRabbit review pass]: fixed 2 findings on PR #26, one a genuine account-takeover vulnerability. (1) **OAuth email-linking account takeover** (CWE-287, major): `ResolveOAuthHuman` linked a provider-verified OAuth identity to ANY existing human with the same normalized email, including one created via ordinary password signup. An attacker could pre-register a victim's real email with a password the attacker controls; when the victim later signs in via Google/GitHub with that same (genuinely theirs) verified email, the OAuth login would silently attach to the attacker's account - the attacker keeps password access to whatever the victim then does under that account. Fixed by adding `database.ErrOAuthAccountRequiresPasswordLogin`: `ResolveOAuthHuman` now only auto-links when the existing account's `password_hash` is still `unusablePasswordHash` (i.e. it was itself only ever reached through OAuth, so every existing "owner" already proved control via provider verification, not a password an attacker could have set unilaterally). A password-holding account is never touched; the caller gets a clear, distinct error instead (`oauth_account_requires_password_login`, 409) telling them to log in with the password first. Deliberately did NOT build an authenticated "link accounts" flow in this pass - refusing silently-unsafe linking is the security-correct minimum; an explicit opt-in linking flow (log in with password, then link a provider) is a reasonable follow-up, not required to close this hole. (2) **Paystack verify-call 4xx misclassification** (major, financial correctness): `Paystack.doTx` mapped ANY 4xx response to `ChargeFailed` ("no money moved"), a rule that is only true for `charge_authorization` (a rejected charge request never creates a charge) - NOT for `VerifyTransaction`, which `reconcilePending` only ever calls for an attempt whose outcome is ALREADY ambiguous, meaning the charge may well have already succeeded. A verify call failing with 401/403 (rotated/misconfigured secret key) or any other non-"unknown reference" 4xx says nothing about whether money moved, but was being classified as `ChargeFailed` - which unblocks `ClaimRenewalAttempt` for a fresh attempt under a NEW reference, double-charging the card. Fixed by splitting verify's 4xx handling: only the specific "Paystack has no record of this reference" response (400/404 with `status:false`) is `ChargeFailed`; every other verify 4xx is `ChargeUnknown` (stays pending, blocks further attempts, gets reconciled again later) - `charge_authorization`'s 4xx handling is unchanged. Both fixes covered by new regression tests: `TestOAuthNewAccountLinkAndState` updated (the old test literally asserted the vulnerable auto-link behavior - now asserts it's refused, and separately that a password-less OAuth-only account still links correctly across a changed provider ID), `TestOAuthRespectsMFA` updated to use an OAuth-created account (a password account can no longer reach the MFA-required path via OAuth at all, by design), and a new `TestAutoRenewVerify401NeverDoubleCharges` (ambiguous charge stays pending through a verify-layer auth failure, no second charge fires). Verified: gofmt/go vet/go build clean, full `go test ./...` and `go test -race ./...` clean, Docker rebuild+boot smoke clean.
+- DEC-240 [RSK-046/RSK-048 risk reduction pass, no live credentials available]: before starting v0.48, spent a pass trying to lower the two open OAuth/billing risks without requiring new setup. Verified Google's and GitHub's OAuth contracts against their CURRENT real documentation (not just "as understood"): confirmed the OIDC userinfo endpoint (`https://openidconnect.googleapis.com/v1/userinfo`) returns `email_verified` as a genuine boolean (the LEGACY `oauth2/v3/userinfo` endpoint is known to sometimes return it as a string - this codebase correctly uses the OIDC endpoint, not the legacy one), confirmed GitHub's token endpoint requires `Accept: application/json` to avoid a form-encoded response (already correctly set in `doJSON` for every request) and confirmed GitHub reports a bad/expired code via an `error` JSON field. Paystack's docs are bot-blocked from automated fetching, so `internal/billing`'s charge_authorization/verify contracts remain unverified against current live documentation - RSK-048 stands as-is; only a live Paystack test-mode credential can actually close it. For RSK-046 (OAuth login-CSRF, no client-side state binding since no frontend consumes this API yet): added a real, zero-frontend-dependency defense-in-depth layer rather than leaving it purely as an accepted risk. `handleOAuthCallback` now rejects a request whose `Referer` header is PRESENT and names neither the expected OAuth provider's own domain (`accounts.google.com` / `github.com`) nor nothing at all. This works because a genuine callback arrival is always a browser redirect FROM the provider's consent page (Referer names the provider), while the practical CSRF delivery vector - a crafted callback URL placed in an email or on an attacker's page - arrives with either no Referer (many clients strip it) or the WRONG one (the attacker's own page), never the provider's. An absent Referer is still allowed through to the normal state-validation path (some browsers/extensions legitimately omit it, and rejecting on absence would break real users for a header that was never guaranteed) - this is explicitly NOT a complete fix (a determined attacker's page could still be made to omit or spoof a referrer via `rel=noreferrer`/meta tag, though spoofing the exact string `accounts.google.com`/`github.com` from an arbitrary page is not possible), but it meaningfully raises the bar for the common real-world delivery vector (email/chat/webpage links) at zero cost to any legitimate flow. RSK-046 stays open in risks.md - this is risk reduction, not closure; the complete fix still needs client-side state binding once a real frontend exists. Verified: gofmt/go vet/go build clean, full `go test ./...` and `go test -race ./...` clean (new `TestOAuthCallbackRejectsWrongReferer` covers wrong/correct/absent referer), Docker rebuild+boot smoke clean.

.ilana/ledger.md

@@ -341,3 +341,6 @@ Opt-in, owner-controlled auto-renewal via MailX-initiated Paystack charge_author
 
 ## 2026-09-25 | OAuth/MFA + auto-renewal: CodeRabbit review pass (PR #26) | GATE PASS
 CodeRabbit reviewed the combined OAuth/MFA + auto-renewal PR and caught 2 real findings, one of them a genuine account-takeover vulnerability that my own earlier manual security review missed: ResolveOAuthHuman auto-linked a provider-verified OAuth login to ANY existing account with the same email, including a password-based one - meaning an attacker could pre-register a victim's real email with an attacker-controlled password, then have the victim's own later, genuinely-theirs OAuth login silently attach to the attacker's account. Fixed by only auto-linking when the existing account has never had a real password (still carries the OAuth-only placeholder hash); a password-holding account now gets a clear "log in with your password first" error instead of being silently linked. Also fixed a real financial-correctness bug in the auto-renewal reconciliation path: a verify-call failure (e.g. 401 from a rotated secret key) was being misclassified the same way as "Paystack has no record of this charge," which would have unblocked a second charge attempt under a fresh reference and double-charged a card whose original charge may have actually succeeded - now only the specific "unknown reference" response counts as proof no charge happened; any other verify failure keeps the attempt pending for later reconciliation. Both fixes came with an existing test that had to change (one literally asserted the vulnerable auto-link behavior as correct) plus new regression coverage. This is a useful reminder even after a careful manual review: account-linking logic needs to be reasoned about from the attacker's setup-in-advance perspective (what if they got there first?), not just the happy-path perspective the code was reviewed from. Verified: gofmt/go vet/go build clean, full go test ./... and go test -race ./... clean, Docker rebuild+boot smoke clean. Ilana updated: decisions.md (DEC-239), state.json (DEC counter).
+
+## 2026-09-26 | RSK-046/RSK-048 risk reduction before v0.48 | GATE PASS
+User asked to lower the two open risks flagged after v0.47 phase 3 before moving to the next milestone. Without live Paystack/Google/GitHub credentials, focused on what was actually achievable: verified Google's and GitHub's OAuth API contracts against their current real documentation rather than relying on prior knowledge - confirmed the OIDC userinfo endpoint's email_verified field is genuinely boolean (the codebase correctly avoids the legacy endpoint known for returning it as a string) and confirmed GitHub's Accept: application/json requirement was already correctly handled. Paystack's docs are bot-blocked from automated fetching, so RSK-048's live-verification gap remains open - genuinely needs a live test-mode credential to close, not something checkable from here. For RSK-046 (OAuth login-CSRF), added a real backend-only defense-in-depth layer instead of leaving it purely as an accepted risk: the callback route now rejects a request whose Referer is present and names neither the OAuth provider's own domain nor nothing at all, since a genuine callback always arrives as a redirect FROM the provider (Referer = the provider) while the practical attack (a crafted link in an email or on an attacker's page) arrives with either no Referer or the wrong one. This is explicitly risk reduction, not closure - documented as such in risks.md - since a determined attacker can still omit or spoof a referrer from their own page; the complete fix needs client-side state binding once a real frontend exists. Verified: gofmt/go vet/go build clean, full go test ./... and go test -race ./... clean (new TestOAuthCallbackRejectsWrongReferer), Docker rebuild+boot smoke clean. Ilana updated: decisions.md (DEC-240), risks.md (RSK-046/048 updated in place), state.json (DEC counter).

.ilana/risks.md

@@ -54,6 +54,6 @@
 - RSK-043 [low, v0.47 phase 1 PR review]: `internal/broadcast/expander.go`'s GDPR-erasure disk cleanup (see DEC-210's `ErrRecipientErased` path) retries 3 times on failure, but a disk error that survives all 3 (a genuine, non-transient FS problem, not just a momentary blip) still leaves the raw message file orphaned on disk with no database row left to ever find or retry it - the `broadcast_recipients` row that would normally drive a retry is already gone (that's what triggered the cleanup in the first place). Logged at Error level and counted via `BroadcastExpansionBatch("erasure_cleanup","error")` so an operator can alert on it, but not auto-healing. A fully durable fix needs a reconciliation sweep (e.g. periodically diff on-disk message directories against `messages` rows and delete orphans), which is a separate, larger change not undertaken here. Revisit if this metric/log ever fires in practice.
 - RSK-044 [CLOSED in v0.47 phase 3c, was high]: paid plans did not auto-renew and no reminder existed. Now: reminder before every period end for all paid orgs, opt-in auto-renewal via MailX-initiated charge_authorization (DEC-235..238). Residuals tracked in RSK-048.
 - RSK-045 [medium, v0.47 phase 2]: plan-enforcement gaps. (1) Broadcast expansion creates messages without passing admitSend, so broadcasts do not count toward the daily send cap. (2) The daily cap and the domain cap are checks, not reservations; concurrent requests can overshoot slightly. (3) The system tenant (`MAILX_SYSTEM_TENANT_ID`) is subject to its own plan once billing is enabled; operators must set it to pro (`UPDATE tenants SET plan='pro' ...`; there is no CLI for plan changes yet, and plan-lapse would downgrade it if a period end is set) or password-reset/invite mail stops at 500/day. (4) Paystack USD charging requires the Paystack account to be approved for USD; not verifiable from the repo.
-- RSK-046 [medium, v0.47 phase 3b]: OAuth `state` is single-use and server-stored but not bound to the initiating browser (no cookie), because start/callback are JSON API endpoints. A login-CSRF (attacker gets a victim to complete the attacker's own callback URL, logging the victim into the attacker's account) is not prevented. Mitigation: bind state to an HttpOnly cookie or have the dashboard verify it initiated the flow.
+- RSK-046 [medium, reduced not closed, v0.47 phase 3b/DEC-240]: OAuth `state` is single-use and server-stored but not bound to the initiating browser (no cookie), because start/callback are JSON API endpoints. A login-CSRF (attacker gets a victim to complete the attacker's own callback URL, logging the victim into the attacker's account) is not fully prevented. DEC-240 added a Referer check on `/callback` (reject if present and not the provider's own domain, allow if absent) - meaningfully raises the bar for the practical delivery vector (a crafted link in an email/chat/webpage) at zero cost to legitimate flows, but is bypassable by an attacker page that spoofs or omits Referer via `rel=noreferrer`. Full mitigation still needs client-side state binding (HttpOnly cookie or the dashboard verifying it initiated the flow) once a real frontend exists.
 - RSK-047 [medium, v0.47 phase 3b]: MFA brute-force limits are per-IP (5 burst, 1/12s) and per-challenge (5 wrong codes). An attacker who already has the password and many IPs can open new challenges (each login is also authIP-limited) and keep guessing; there is no per-account lockout or alert. Losing `MAILX_MFA_MASTER_KEY` makes TOTP unverifiable (backup codes still work).
-- RSK-048 [medium, v0.47 phase 3c]: auto-renewal residuals. (1) "Transaction reference not found" from verify is treated as no charge; if Paystack were eventually consistent past the 1h reconcile delay this could permit a retry (not observed; unverified against live Paystack). (2) Charge-endpoint request/response shapes were implemented from Paystack's documented API and tested only against a fake server; needs a live test-mode run before launch. (3) An unknown-outcome attempt still pending at period end does not delay the lapse; a later verified success re-activates with a fresh 30-day period. (4) Amount-mismatch successes need manual operator resolution. (5) Renewal requires the system mailer; without it nothing is reminded or charged (safe but silent beyond a startup WARN). (6) No endpoint to delete a saved card (disable auto_renew stops charging but keeps the encrypted authorization).
+- RSK-048 [medium, v0.47 phase 3c; DEC-240 fixed one sub-item, rest unchanged]: auto-renewal residuals. (1) "Transaction reference not found" from verify is treated as no charge; if Paystack were eventually consistent past the 1h reconcile delay this could permit a retry (not observed; unverified against live Paystack). (2) Charge-endpoint request/response shapes were implemented from Paystack's documented API and tested only against a fake server; needs a live test-mode run before launch - Paystack's docs are bot-blocked from automated fetching (tried during DEC-240's pass), so this could not be re-verified without live credentials; Google's and GitHub's OAuth contracts WERE re-verified against current docs and confirmed correct. (3) An unknown-outcome attempt still pending at period end does not delay the lapse; a later verified success re-activates with a fresh 30-day period. (4) Amount-mismatch successes need manual operator resolution. (5) Renewal requires the system mailer; without it nothing is reminded or charged (safe but silent beyond a startup WARN). (6) No endpoint to delete a saved card (disable auto_renew stops charging but keeps the encrypted authorization).

.ilana/state.json

@@ -18,7 +18,7 @@
     "DEF": 27,
     "CR": 24,
     "RSK": 48,
-    "DEC": 239,
+    "DEC": 240,
     "MET": 72,
     "ETH": 1
   },

internal/api/humanauth_mfa_handler.go

@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"errors"
 	"net/http"
+	"net/url"
 	"time"
 
 	"github.com/Ferousco-dev/mailx/internal/humanauth"
@@ -60,13 +61,45 @@ func (h *humanAuthHandler) handleOAuthStart(w http.ResponseWriter, r *http.Reque
 	writeJSON(w, http.StatusOK, map[string]string{"authorization_url": u})
 }
 
+// oauthCallbackRefererHosts is the browser's Referer host on a GENUINE
+// callback arrival: the provider's own consent page redirects the browser
+// here, so Referer (when the browser sends one at all) names the provider,
+// never our own frontend. Defense-in-depth against login CSRF (RSK-046):
+// the full fix needs client-side state binding that doesn't exist until a
+// real frontend consumes this API, but a forged callback link (the
+// practical delivery vector - a crafted URL in an email or on an
+// attacker's page) arrives with either no Referer or the WRONG one, never
+// this. A present-and-wrong Referer is rejected; an absent one is allowed,
+// since browsers/extensions legitimately omit it and this is best-effort
+// hardening, not the complete mitigation.
+var oauthCallbackRefererHosts = map[string][]string{
+	"google": {"accounts.google.com"},
+	"github": {"github.com"},
+}
+
 func (h *humanAuthHandler) handleOAuthCallback(w http.ResponseWriter, r *http.Request) {
+	provider := r.PathValue("provider")
+	if ref := r.Referer(); ref != "" {
+		if u, err := url.Parse(ref); err == nil && u.Host != "" {
+			ok := false
+			for _, host := range oauthCallbackRefererHosts[provider] {
+				if u.Host == host {
+					ok = true
+					break
+				}
+			}
+			if !ok {
+				writeError(w, r, newError(ErrAuthentication, "invalid_oauth_state", "sign-in state is missing, expired, or already used; start again"))
+				return
+			}
+		}
+	}
 	q := r.URL.Query()
 	code := q.Get("code")
 	if q.Get("error") != "" {
 		code = "" // consent denied: still consume the state, then report a provider error
 	}
-	session, err := h.svc.CompleteOAuth(r.Context(), r.PathValue("provider"), code, q.Get("state"))
+	session, err := h.svc.CompleteOAuth(r.Context(), provider, code, q.Get("state"))
 	if writeMFARequired(w, err) {
 		return
 	}