From 46fa72ca0fa18ecd592477a77f650122bf5aa3fa Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 22 Apr 2026 10:43:22 -0400 Subject: [PATCH] feat(server): log session IP changes, add optional PAD_IP_CHANGE_ENFORCE=strict (TASK-666) (#191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(server): log session IP changes, add optional PAD_IP_CHANGE_ENFORCE=strict (TASK-666) Sessions stored a client IP at creation but never rechecked it. A stolen cookie could be used from anywhere with no signal to the owner. This change adds mid-lifetime IP-change detection without breaking legitimate mobility (mobile roaming, VPN toggles, carrier NAT) by default. - New audit action ActionSessionIPChanged captures {old_ip, new_ip} in the audit metadata. Visible via the existing /api/v1/admin/audit-log. - handleSessionIPChange wired into both SessionAuth (cookies) and TokenAuth (padsess_ bearer). After UA check passes, compares stored session IP to clientIP(r). On mismatch: - log one audit row - update the stored session IP so we don't spam the log - strict mode: DeleteSession + 401 "session_ip_changed" - default mode: let the request through - Store.UpdateSessionIP lets middleware refresh the recorded IP without tearing down the session. - PAD_IP_CHANGE_ENFORCE=strict env var + ip_change_enforce TOML key + Server.SetIPChangeEnforce setter (case-insensitive, trims whitespace). - Table-driven tests cover log-only, strict rejection with session destruction, and setter parsing edge cases. Parent: PLAN-643 (OSS Security Hardening). * fix(server): dedupe session-IP-change audit via CAS, handle browser vs API paths per Codex review Addresses two P2 comments on PR #191: 1. Race: parallel requests after an IP change could each emit ActionSessionIPChanged before any of them updated the stored IP, producing duplicate audit rows for a single transition. - Replace UpdateSessionIP with UpdateSessionIPIfEquals (compare-and-set on ip_address). Only the request that actually rotates the stored value logs; concurrent siblings lose the CAS and skip logging. - New test TestSessionIPChange_CASDedupesRace fires 20 concurrent requests from the new IP and asserts exactly 1 audit row. 2. Strict-mode 401 on non-API paths: - In current routing the SPA is mounted on the root router outside the auth Group, so SessionAuth only fires for /api/* in practice. The original concern about JSON 401s on browser navigation doesn't surface today, but defense-in-depth keeps the code forward-safe: restructure handleSessionIPChange to return a four-state outcome (Continue / AllowedLogged / Revoked / Terminated) and only write the JSON 401 on /api/* paths. Revoked + non-API falls through unauthenticated so a future SPA-in-group configuration would still render a login screen instead of raw JSON. - Clear the session cookie (MaxAge=-1) in strict rejection so the browser stops sending the now-revoked token on the next request. TestSessionIPChange_StrictClearsCookies verifies the Set-Cookie. Parent: PLAN-643 (OSS Security Hardening), TASK-666. * fix(server): strict mode destroys session atomically, never rotate stored IP when destroying (TASK-666) Addresses Codex P1 on PR #191: previously we rotated the session's stored ip_address via UpdateSessionIPIfEquals BEFORE attempting DeleteSession. If the DELETE failed (transient DB error) the row remained alive — rebound to the attacker's new IP — so follow-up requests saw stored IP == client IP and passed handleSessionIPChange's "match, no-op" branch. That silently defeated strict enforcement. - New Store.DeleteSessionIfExists returns (bool, error) to serve as the CAS primitive for strict mode: only the caller whose DELETE affected a row emits the audit entry, and a DB error fails closed (500 — "Unable to validate session") rather than letting the request through. - handleSessionIPChange splits into two paths: * log-only mode: UpdateSessionIPIfEquals for CAS dedup (unchanged) * strict mode: DeleteSessionIfExists is the CAS; stored IP is NEVER rotated so any failure leaves the session bound to the OLD IP and subsequent requests from the new IP still mismatch + still reject. - TestSessionIPChange_StrictDestroysSessionAtomically regression test verifies a second request from the new IP with the same token still fails after the first strict-mode rejection. Parent: PLAN-643 (OSS Security Hardening). * fix(server): exempt public API paths from strict IP-change termination (TASK-666) Addresses Codex P2 on PR #191: SessionAuth runs for every /api/* path, including public endpoints like /api/v1/auth/login, /api/v1/auth/register, /api/v1/health, /api/v1/s/* (share links), and /api/v1/plan-limits. In strict mode, a stale session cookie on those requests was rejected with a 401 session_ip_changed BEFORE the public handler could run — the user literally couldn't log back in because their own stale cookie blocked the login call. - Extract isPublicAPIPath as a shared helper between RequireAuth and handleSessionIPChange so they can't drift out of sync. - handleSessionIPChange strict-mode flow now: destroy session + clear cookies + audit log (unchanged), then for public API paths return Revoked so the handler still runs. For authenticated-only API paths still return Terminated (401). For non-API paths return Revoked for the SPA fallback. - Updated TokenAuth Revoked handler to match: pass through unauth on public paths, 401 on authenticated-only. - TestSessionIPChange_StrictAllowsPublicAPIPaths regression test: a stale session cookie on /api/v1/auth/login must NOT produce session_ip_changed; /api/v1/plan-limits must still return 200. Parent: PLAN-643 (OSS Security Hardening). * fix(server): short-circuit SessionAuth on token auth + fix IPv6 clientIP parsing (TASK-666) Addresses two more Codex comments on PR #191: P1 — SessionAuth 401'd API-token-authenticated requests: TokenAuth sets currentUser for user-owned tokens AND tokenWorkspaceID for legacy workspace-scoped tokens. SessionAuth short-circuited only on currentUser, so a workspace-scoped-token request that happened to carry a stale session cookie with a mismatched IP would be rejected by the IP-change strict path before RequireAuth could honor the token. Extend the short-circuit to also check tokenWorkspaceID; either signal is enough to say "token auth already succeeded, skip cookie validation". P2 — clientIP mangled IPv6 addresses: clientIP used strings.LastIndex(":") on RemoteAddr. For bare IPv6 addresses like "2001:db8::1" (which TrustedProxyRealIP writes verbatim from X-Forwarded-For, no brackets/port), that strips the final hextet to "2001:db8:" — unusable for comparison in the new IP-change audit path and incorrect for rate-limit keys too. Switch to net.SplitHostPort which handles both "host:port" and "[ipv6]:port", falling back to the raw RemoteAddr when no port is present (the trusted-proxy rewrite case). Tests: - TestClientIP_IPv6NotMangled covers IPv4 w/wo port, bracketed IPv6, bare IPv6 (no port, no brackets), and loopback forms. - TestSessionAuth_ShortCircuitsOnAPITokenAuth exercises the worst case: strict mode + valid API token + stale session cookie + new client IP. Request must succeed (token wins) and NO new session_ip_changed audit row must appear. Parent: PLAN-643 (OSS Security Hardening). * fix(server): canonicalize IPs before session-IP-change comparison (TASK-666) Addresses Codex P2 on PR #191: raw-string comparison of session.IPAddress vs clientIP(r) would fire session_ip_changed spuriously when the same IPv6 address arrived in different valid textual representations (the trusted-proxy path writes X-Forwarded-For verbatim, and different hops normalize differently — "2001:0db8::1" vs "2001:db8::1" etc.). - canonicalIP helper: net.ParseIP + stringify to collapse equivalent IPv6 forms (compressed vs expanded, case, leading zeros) and IPv4-in- IPv6 into a single canonical string. Non-parseable inputs pass through unchanged so debug/malformed values behave predictably. - handleSessionIPChange compares and logs the canonical forms. The CAS still passes session.IPAddress (the raw stored value) to the DB — the compare-and-set is about row identity — but the new IP written in is the canonical form so future comparisons are stable. - TestCanonicalIP covers empty, IPv4, shorthand "::1", expanded 8-group equivalent, mixed-case 2001:DB8::1, fully expanded 2001:0db8:…:0001, and non-IP fallback. Parent: PLAN-643 (OSS Security Hardening). --- .env.example | 8 + cmd/pad/main.go | 1 + internal/config/config.go | 12 +- internal/models/activity.go | 7 + internal/server/middleware_auth.go | 232 +++++++++- internal/server/middleware_ratelimit.go | 24 +- internal/server/middleware_session_ip_test.go | 425 ++++++++++++++++++ internal/server/server.go | 22 +- internal/store/sessions.go | 53 +++ 9 files changed, 763 insertions(+), 21 deletions(-) create mode 100644 internal/server/middleware_session_ip_test.go diff --git a/.env.example b/.env.example index 5009bf7b..43f762c7 100644 --- a/.env.example +++ b/.env.example @@ -49,3 +49,11 @@ PAD_ENCRYPTION_KEY= # "Authorization: Bearer $PAD_METRICS_TOKEN". # openssl rand -hex 32 # PAD_METRICS_TOKEN= + +# OPTIONAL — Session IP-change policy. +# Default (unset) logs an ActionSessionIPChanged audit entry when a session +# presents a new client IP and updates the stored IP. "strict" additionally +# destroys the session and forces re-login. Strict mode breaks legitimate +# mobility (mobile roaming, VPN toggles, carrier NAT); enable only for +# high-sensitivity deployments. +# PAD_IP_CHANGE_ENFORCE=strict diff --git a/cmd/pad/main.go b/cmd/pad/main.go index a6ce0c32..01a50061 100644 --- a/cmd/pad/main.go +++ b/cmd/pad/main.go @@ -275,6 +275,7 @@ func serveCmd() *cobra.Command { srv.SetSecureCookies(cfg.SecureCookies) srv.SetTrustedProxies(cfg.TrustedProxies) srv.SetMetricsToken(cfg.MetricsToken) + srv.SetIPChangeEnforce(cfg.IPChangeEnforce) srv.SetSSELimits(cfg.SSEMaxConnections, cfg.SSEMaxPerWorkspace) // Cloud mode: enable cloud-specific endpoints and behavior diff --git a/internal/config/config.go b/internal/config/config.go index c114b61c..fe97ead3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -48,10 +48,11 @@ type Config struct { EncryptionKeySource string `toml:"-"` // "env", "file", "generated", or "" (unset); populated by EnsureEncryptionKey // Security - CORSOrigins string `toml:"cors_origins"` // Comma-separated allowed origins (e.g. "https://app.pad.dev,https://admin.pad.dev") - SecureCookies bool `toml:"secure_cookies"` // Set Secure flag on cookies (requires TLS) - TrustedProxies string `toml:"trusted_proxies"` // Comma-separated CIDRs whose X-Forwarded-For is trusted. Empty = ignore proxy headers. - MetricsToken string `toml:"metrics_token"` // Shared Bearer token required to scrape /metrics. Empty = loopback-only. + CORSOrigins string `toml:"cors_origins"` // Comma-separated allowed origins (e.g. "https://app.pad.dev,https://admin.pad.dev") + SecureCookies bool `toml:"secure_cookies"` // Set Secure flag on cookies (requires TLS) + TrustedProxies string `toml:"trusted_proxies"` // Comma-separated CIDRs whose X-Forwarded-For is trusted. Empty = ignore proxy headers. + MetricsToken string `toml:"metrics_token"` // Shared Bearer token required to scrape /metrics. Empty = loopback-only. + IPChangeEnforce string `toml:"ip_change_enforce"` // "" (log only) or "strict" (reject session when client IP differs from the one recorded at session creation). // SSE limits SSEMaxConnections int `toml:"sse_max_connections"` // Global max SSE connections (0 = unlimited) @@ -167,6 +168,9 @@ func Load() (*Config, error) { if v := os.Getenv("PAD_METRICS_TOKEN"); v != "" { cfg.MetricsToken = v } + if v := os.Getenv("PAD_IP_CHANGE_ENFORCE"); v != "" { + cfg.IPChangeEnforce = v + } if v := os.Getenv("PAD_SSE_MAX_CONNECTIONS"); v != "" { if max, err := strconv.Atoi(v); err == nil { cfg.SSEMaxConnections = max diff --git a/internal/models/activity.go b/internal/models/activity.go index dd8c3c14..f7e2897d 100644 --- a/internal/models/activity.go +++ b/internal/models/activity.go @@ -32,6 +32,13 @@ const ( ActionUserDisabled = "user_disabled" ActionUserEnabled = "user_enabled" ActionAccountDeleted = "account_deleted" + // ActionSessionIPChanged is logged when a session presents a different + // client IP than the one recorded at creation. We don't strict-check IP + // by default (that breaks legitimate geo shifts — VPN toggle, mobile + // roaming) but surface the change to the audit log for detection. In + // deployments configured with PAD_IP_CHANGE_ENFORCE=strict the middleware + // additionally rejects the request. + ActionSessionIPChanged = "session_ip_changed" ) type Activity struct { diff --git a/internal/server/middleware_auth.go b/internal/server/middleware_auth.go index 9841e21a..2cf29e03 100644 --- a/internal/server/middleware_auth.go +++ b/internal/server/middleware_auth.go @@ -6,12 +6,14 @@ import ( "encoding/hex" "encoding/json" "log/slog" + "net" "net/http" "strings" "time" "github.com/go-chi/chi/v5" "github.com/xarmian/pad/internal/models" + "github.com/xarmian/pad/internal/store" ) type contextKey string @@ -73,6 +75,25 @@ func (s *Server) TokenAuth(next http.Handler) http.Handler { writeError(w, http.StatusUnauthorized, "unauthorized", "Session expired") return } + // Session binding: detect IP changes. Log to audit log in all modes; + // reject only when PAD_IP_CHANGE_ENFORCE=strict. + switch s.handleSessionIPChange(w, r, session, token) { + case sessionIPChangeTerminated: + return + case sessionIPChangeRevoked: + // Session was destroyed. For public API paths (login, + // password reset, health, share links) pass through + // unauthenticated so the caller can recover. For + // authenticated-only paths, the Bearer flow has no SPA + // fallback — treat revocation as a hard 401. + if isPublicAPIPath(r.URL.Path) { + next.ServeHTTP(w, r) + return + } + writeError(w, http.StatusUnauthorized, "session_ip_changed", + "Session client IP changed — please log in again.") + return + } ctx := context.WithValue(r.Context(), ctxCurrentUser, session.User) next.ServeHTTP(w, r.WithContext(ctx)) return @@ -126,8 +147,13 @@ func (s *Server) TokenAuth(next http.Handler) http.Handler { // If a user was already resolved by TokenAuth, this is a no-op. func (s *Server) SessionAuth(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Already authenticated by TokenAuth - if currentUser(r) != nil { + // Already authenticated by TokenAuth — user-bound API tokens set + // currentUser, legacy workspace-scoped tokens set tokenWorkspaceID + // with no user. In either case we must short-circuit: otherwise a + // stale session cookie on the same request could trigger the + // IP-change strict-mode path and 401 the request even though the + // API token itself is valid. + if currentUser(r) != nil || tokenWorkspaceID(r) != "" { next.ServeHTTP(w, r) return } @@ -157,6 +183,18 @@ func (s *Server) SessionAuth(next http.Handler) http.Handler { return } + // Session binding: detect IP changes. Log to audit log in all modes; + // reject only when PAD_IP_CHANGE_ENFORCE=strict. + switch s.handleSessionIPChange(w, r, session, cookie.Value) { + case sessionIPChangeTerminated: + return + case sessionIPChangeRevoked: + // Browser path: let the request through unauthenticated so + // the SPA renders its login flow instead of a JSON error. + next.ServeHTTP(w, r) + return + } + // Re-issue CSRF cookie if the session is valid but the cookie is missing. // This can happen when cookies expire at different times or are selectively cleared. // Skip for auth endpoints — they manage their own CSRF cookies (login sets, logout clears). @@ -171,6 +209,22 @@ func (s *Server) SessionAuth(next http.Handler) http.Handler { }) } +// isPublicAPIPath reports whether the given request path is an API +// endpoint that bypasses authentication entirely. These handlers must +// work even when the caller has no valid session (login, registration, +// password reset, health probes, public plan limits, share-link tokens). +// Shared between RequireAuth and the session-IP-change path so both stay +// in sync — in particular, strict IP-change enforcement must NOT 401 +// these endpoints, otherwise a user with a stale cookie can't even +// recover by logging in again. +func isPublicAPIPath(path string) bool { + return strings.HasPrefix(path, "/api/v1/auth/") || + path == "/api/v1/health" || + strings.HasPrefix(path, "/api/v1/health/") || + strings.HasPrefix(path, "/api/v1/s/") || + path == "/api/v1/plan-limits" +} + // RequireAuth middleware blocks unauthenticated requests when users exist // in the system. When no users exist (fresh install), all requests pass // through to allow the setup flow. @@ -180,8 +234,7 @@ func (s *Server) RequireAuth(next http.Handler) http.Handler { // Auth endpoints, share link resolution, health, and the public // plan-limits endpoint are always exempt from auth. - if strings.HasPrefix(path, "/api/v1/auth/") || path == "/api/v1/health" || strings.HasPrefix(path, "/api/v1/health/") || strings.HasPrefix(path, "/api/v1/s/") || - path == "/api/v1/plan-limits" { + if isPublicAPIPath(path) { next.ServeHTTP(w, r) return } @@ -441,6 +494,177 @@ func sha256hex(s string) string { return hex.EncodeToString(h[:]) } +// canonicalIP returns the semantic (canonical text) form of an IP +// address string. For IPv6 this collapses different valid spellings +// (compressed vs expanded, uppercase vs lowercase, leading zeroes) to +// a single representation so comparing two strings by == reflects +// actual IP equality. For IPv4 it accepts both 4-byte and IPv4-in-IPv6 +// forms. For inputs that don't parse as an IP the original string is +// returned so behavior stays predictable on malformed/debug values. +func canonicalIP(s string) string { + if s == "" { + return s + } + if ip := net.ParseIP(s); ip != nil { + return ip.String() + } + return s +} + +// sessionIPChangeOutcome captures what the caller (TokenAuth / SessionAuth) +// should do after handleSessionIPChange inspected the session. +type sessionIPChangeOutcome int + +const ( + // sessionIPChangeContinue — happy path. IP matches (or no recorded IP). + // Caller proceeds to install the session user in context and call next. + sessionIPChangeContinue sessionIPChangeOutcome = iota + // sessionIPChangeAllowedLogged — IP differs, log-only mode. Session + // remains valid, caller proceeds as usual. + sessionIPChangeAllowedLogged + // sessionIPChangeRevoked — strict mode. The session was destroyed and + // the caller must NOT install the user in context. For browser (non-API) + // paths the request should continue unauthenticated so the SPA can + // render its login screen instead of a raw JSON error. + sessionIPChangeRevoked + // sessionIPChangeTerminated — strict mode + API path. A 401 has already + // been written; caller must return immediately. + sessionIPChangeTerminated +) + +// handleSessionIPChange compares the client IP on this request to the IP +// recorded when the session was created. +// +// Behavior: +// - Session has no recorded IP (legacy pre-migration row): continue. +// - IP matches: continue. +// - Log-only mode (default): atomically rotate the stored IP via +// compare-and-set. The race winner emits exactly one +// ActionSessionIPChanged audit row per transition — concurrent +// requests that lose the CAS skip logging to avoid duplicate rows. +// - Strict mode (PAD_IP_CHANGE_ENFORCE=strict): do NOT rotate the +// stored IP. Instead use DeleteSessionIfExists as the CAS primitive +// — the request that actually deletes the row (a) logs the audit +// entry once, (b) returns 401 for API / revoked for browser paths. +// Rotating the IP first would be unsafe: if the subsequent +// DeleteSession failed (transient DB error) the session would remain +// valid rebound to the new IP, defeating strict enforcement. By +// deleting atomically, any failure leaves the session bound to the +// original IP so a follow-up request from the new IP still mismatches +// and is still rejected. If the delete errors outright, the request +// is rejected with 500 so the client can't proceed. +// +// Log-only mode breaks fewer legitimate clients (mobile roaming, VPN +// toggles, carrier NAT) while still giving operators a visible signal. +func (s *Server) handleSessionIPChange(w http.ResponseWriter, r *http.Request, session *store.SessionInfo, plainToken string) sessionIPChangeOutcome { + // Canonicalize both sides so equivalent IPv6 representations + // (compressed vs expanded, case, leading zeros) don't register as a + // spurious change. Raw trusted-proxy X-Forwarded-For values can + // arrive in any valid form. + storedIP := canonicalIP(session.IPAddress) + newIP := canonicalIP(clientIP(r)) + if storedIP == "" || newIP == "" || storedIP == newIP { + return sessionIPChangeContinue + } + + userID := "" + if session.User != nil { + userID = session.User.ID + } + + if s.ipChangeEnforceStrict { + // Destroy the session atomically. Only the caller whose DELETE + // affected a row logs and issues the "session_ip_changed" response + // body. A failed delete means the session is still valid AND still + // bound to the old IP (we skipped the rotation), so a follow-up + // request from the new IP will hit this path again — safe. + deleted, err := s.store.DeleteSessionIfExists(plainToken) + if err != nil { + // Fail closed: we can't prove the session is gone, so refuse + // to let the request through. A retry will converge once the + // DB recovers. + slog.Error("failed to destroy session on IP-change in strict mode; failing closed", + "session_ip", storedIP, + "client_ip", newIP, + "user_id", userID, + "error", err) + writeError(w, http.StatusInternalServerError, "internal_error", + "Unable to validate session. Please try again.") + return sessionIPChangeTerminated + } + if deleted { + s.logAuditEventForUser(models.ActionSessionIPChanged, r, userID, auditMeta(map[string]string{ + "old_ip": storedIP, + "new_ip": newIP, + })) + slog.Warn("session destroyed: IP changed (strict enforcement)", + "session_ip", storedIP, + "client_ip", newIP, + "user_id", userID) + } + // Clear client-side cookies regardless — even if another request + // already deleted the row, this client is still holding the now- + // invalid token. + clearSessionCookie(w, s.secureCookies) + clearCSRFCookie(w) + // Public API paths (login/register/password-reset, health, share + // links, plan-limits) must STILL work after the session was + // destroyed — a user with a stale cookie needs a way back in, and + // Prometheus health probes shouldn't 401 because some other tab + // left a stale session. Pass the request through unauthenticated. + if isPublicAPIPath(r.URL.Path) { + return sessionIPChangeRevoked + } + if strings.HasPrefix(r.URL.Path, "/api/") { + writeError(w, http.StatusUnauthorized, "session_ip_changed", + "Session client IP changed — please log in again.") + return sessionIPChangeTerminated + } + // Non-API (browser) path: caller must NOT install the destroyed + // session's user into the request context. The SPA will render its + // unauth state and the user will redirect to login. + return sessionIPChangeRevoked + } + + // Log-only mode: CAS-rotate the stored IP so only the winner logs. + // The CAS compares against session.IPAddress (the raw value we read + // from the DB) and writes the canonical newIP so future comparisons + // are consistent. Err on the side of "winner" when the CAS errors + // — we'd rather log an extra row than miss the signal entirely. + // A failed rotate here is non-fatal: worst case the next request + // also gets an audit row. + won, err := s.store.UpdateSessionIPIfEquals(plainToken, session.IPAddress, newIP) + if err != nil { + slog.Warn("failed to rotate session ip after change", "error", err) + won = true + } + if won { + s.logAuditEventForUser(models.ActionSessionIPChanged, r, userID, auditMeta(map[string]string{ + "old_ip": storedIP, + "new_ip": newIP, + })) + slog.Info("session client IP changed (logged, request allowed)", + "session_ip", storedIP, + "client_ip", newIP, + "user_id", userID) + } + return sessionIPChangeAllowedLogged +} + +// clearSessionCookie expires the session cookie on the client so a +// subsequent request doesn't keep presenting a now-revoked token. +func clearSessionCookie(w http.ResponseWriter, secure bool) { + http.SetCookie(w, &http.Cookie{ + Name: sessionCookieName(secure), + Value: "", + Path: "/", + MaxAge: -1, + HttpOnly: true, + Secure: secure, + SameSite: http.SameSiteLaxMode, + }) +} + // tokenScopeAllows checks if the token's scopes permit the given HTTP method // and path. Scopes are stored as a JSON array of strings. // diff --git a/internal/server/middleware_ratelimit.go b/internal/server/middleware_ratelimit.go index 935fa7b3..ac1daaa2 100644 --- a/internal/server/middleware_ratelimit.go +++ b/internal/server/middleware_ratelimit.go @@ -3,6 +3,7 @@ package server import ( "log/slog" "math" + "net" "net/http" "strconv" "strings" @@ -279,16 +280,23 @@ func rateLimitKey(r *http.Request, ip string) string { } // clientIP extracts the client IP from RemoteAddr. This is safe because -// chimiddleware.RealIP runs earlier in the chain and overwrites RemoteAddr -// with the trusted value from X-Real-IP / X-Forwarded-For. We deliberately -// do NOT read proxy headers here to prevent clients from spoofing their IP -// to bypass rate limits. +// TrustedProxyRealIP runs earlier in the chain and — when a trusted +// proxy is configured — overwrites RemoteAddr with the trusted value +// from X-Real-IP / X-Forwarded-For. We deliberately do NOT read proxy +// headers here to prevent clients from spoofing their IP to bypass +// rate limits. +// +// Uses net.SplitHostPort so IPv6 addresses are handled correctly. +// A naive LastIndex(":") strips the final hextet of a bare IPv6 address +// like "2001:db8::1" — TrustedProxyRealIP writes the X-Forwarded-For +// value verbatim (no port, no brackets), so a LastIndex-based parse +// would mangle it. For bare IPs without a port SplitHostPort returns +// an error and we return the address as-is. func clientIP(r *http.Request) string { - host := r.RemoteAddr - if idx := strings.LastIndex(host, ":"); idx != -1 { - return host[:idx] + if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + return host } - return host + return r.RemoteAddr } // writeRateLimitResponse sends a 429 response with Retry-After and X-RateLimit-* headers. diff --git a/internal/server/middleware_session_ip_test.go b/internal/server/middleware_session_ip_test.go new file mode 100644 index 00000000..a902fe3e --- /dev/null +++ b/internal/server/middleware_session_ip_test.go @@ -0,0 +1,425 @@ +package server + +import ( + "bytes" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/xarmian/pad/internal/models" +) + +// doRequestWithCookieFrom is like doRequestWithCookie but lets the caller +// set the request RemoteAddr so tests can simulate a session jumping to a +// different client IP mid-lifetime. +func doRequestWithCookieFrom(srv *Server, method, path string, body interface{}, token, remoteAddr string) *httptest.ResponseRecorder { + var bodyReader io.Reader + if body != nil { + data, _ := json.Marshal(body) + bodyReader = bytes.NewReader(data) + } + req := httptest.NewRequest(method, path, bodyReader) + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + req.RemoteAddr = remoteAddr + req.AddCookie(&http.Cookie{ + Name: "pad_session", + Value: token, + }) + const testCSRF = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + req.AddCookie(&http.Cookie{ + Name: "pad_csrf", + Value: testCSRF, + }) + req.Header.Set("X-CSRF-Token", testCSRF) + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + return rr +} + +// TestSessionIPChange_LogOnlyDefault verifies that without +// PAD_IP_CHANGE_ENFORCE=strict, a session that presents a different client +// IP is allowed through but an ActionSessionIPChanged audit row is written. +func TestSessionIPChange_LogOnlyDefault(t *testing.T) { + srv := testServer(t) + token := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") // bootstrap creates from 127.0.0.1 + + // First request from the original IP — no change, no audit row. + rr := doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "127.0.0.1:2222") + if rr.Code != http.StatusOK { + t.Fatalf("same-IP request: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if got := countIPChangeEvents(t, srv); got != 0 { + t.Fatalf("no audit row expected before IP change, got %d", got) + } + + // Request from a brand-new IP — request still succeeds (log-only). + rr = doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + if rr.Code != http.StatusOK { + t.Fatalf("IP-changed request in log-only mode: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if got := countIPChangeEvents(t, srv); got != 1 { + t.Fatalf("expected 1 ActionSessionIPChanged audit row after IP change, got %d", got) + } + + // Subsequent request from the same new IP must NOT re-log (we updated + // the stored IP in-place on the first hit). + rr = doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + if rr.Code != http.StatusOK { + t.Fatalf("same-new-IP repeat: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if got := countIPChangeEvents(t, srv); got != 1 { + t.Fatalf("no additional audit row expected after IP settles, got %d", got) + } +} + +// TestSessionIPChange_StrictRejects verifies that with strict enforcement +// enabled, a session presenting a different client IP is rejected AND the +// session is destroyed (so a stolen token can't be retried from the same +// new IP). +func TestSessionIPChange_StrictRejects(t *testing.T) { + srv := testServer(t) + srv.SetIPChangeEnforce("strict") + token := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + // Sanity: original IP still works. + rr := doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "127.0.0.1:2222") + if rr.Code != http.StatusOK { + t.Fatalf("same-IP request: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + + // Jump to a new IP — must be rejected with 401 in strict mode. + rr = doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + if rr.Code != http.StatusUnauthorized { + t.Fatalf("IP-changed request in strict mode: expected 401, got %d: %s", rr.Code, rr.Body.String()) + } + // Audit row still gets written. + if got := countIPChangeEvents(t, srv); got != 1 { + t.Fatalf("expected 1 ActionSessionIPChanged audit row in strict mode, got %d", got) + } + + // Retrying with the same token — even from the new IP — must fail + // (session was destroyed). + rr = doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + if rr.Code == http.StatusOK { + t.Fatal("session should have been destroyed after strict rejection") + } +} + +// TestSessionIPChange_StrictDestroysSessionAtomically verifies that in +// strict mode the session is destroyed as part of the IP-mismatch check +// and does NOT survive with a rebound IP. A regression would mean that +// if DeleteSession were ever separated from the rotation, a follow-up +// request from the new IP could pass authentication. +func TestSessionIPChange_StrictDestroysSessionAtomically(t *testing.T) { + srv := testServer(t) + srv.SetIPChangeEnforce("strict") + token := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + // First IP-mismatch request → 401. + rr := doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + if rr.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 on IP mismatch, got %d: %s", rr.Code, rr.Body.String()) + } + + // Second request from the NEW IP must also fail — the session must + // be gone, not rebound to 198.51.100.7. A regression that rotated + // the stored IP before destroying would leak through here. + rr = doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + if rr.Code == http.StatusOK { + t.Fatal("session must be destroyed in strict mode, not rebound to new IP") + } + + // Single audit row for the whole transition (the second request hits + // the ValidateSession-returns-nil branch and never re-logs). + if got := countIPChangeEvents(t, srv); got != 1 { + t.Fatalf("expected exactly 1 audit row for the strict-mode transition, got %d", got) + } +} + +// TestSessionIPChange_StrictClearsCookies verifies that in strict mode +// the session cookie is explicitly cleared on the response (MaxAge=-1) so +// the browser doesn't keep re-sending a now-revoked token on the next +// navigation. Only the API 401 path reaches SessionAuth in the current +// routing (the SPA catch-all is mounted on the root router outside the +// auth group), but cookie hygiene still applies for API clients that +// present cookies (e.g. the web UI's fetch calls). +func TestSessionIPChange_StrictClearsCookies(t *testing.T) { + srv := testServer(t) + srv.SetIPChangeEnforce("strict") + token := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + rr := doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + if rr.Code != http.StatusUnauthorized { + t.Fatalf("expected 401, got %d: %s", rr.Code, rr.Body.String()) + } + + // Session cookie must have been cleared by Set-Cookie: MaxAge=-1. + foundClear := false + for _, c := range rr.Result().Cookies() { + if strings.HasPrefix(c.Name, "pad_session") && c.MaxAge < 0 { + foundClear = true + break + } + } + if !foundClear { + t.Fatalf("expected session cookie to be cleared in strict mode; cookies=%v", rr.Result().Cookies()) + } +} + +// TestSessionIPChange_StrictAllowsPublicAPIPaths verifies that a stale +// session cookie on a PUBLIC API path (login, password reset, health, +// share links, plan-limits) does NOT produce a 401 in strict mode. The +// session still gets destroyed and the cookie cleared so the stale token +// can't be reused, but the request falls through so the user can log in +// again or the health probe succeeds. +func TestSessionIPChange_StrictAllowsPublicAPIPaths(t *testing.T) { + srv := testServer(t) + srv.SetIPChangeEnforce("strict") + token := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + // POST /api/v1/auth/login with the stale session cookie AND a + // different client IP. The handler expects email+password in the body + // and returns 400 "bad_request" on invalid JSON, so we send an empty + // body to keep the assertion simple. The important thing is: we + // don't get 401 session_ip_changed — the stale cookie didn't block + // the user from re-authenticating. + rr := doRequestWithCookieFrom(srv, "POST", "/api/v1/auth/login", map[string]string{ + "email": "admin@example.com", + "password": "wrong-password-on-purpose", + }, token, "198.51.100.7:5555") + if rr.Code == http.StatusUnauthorized { + // Specifically, must NOT be the IP-change error code. + if strings.Contains(rr.Body.String(), "session_ip_changed") { + t.Fatalf("stale cookie on login endpoint blocked user with session_ip_changed: %s", rr.Body.String()) + } + } + // Session must still be destroyed + audit row written. + if got := countIPChangeEvents(t, srv); got != 1 { + t.Fatalf("expected 1 ActionSessionIPChanged audit row, got %d", got) + } + + // A subsequent authenticated API call with the same stale token must + // now fail — session is gone. + rr = doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + if rr.Code == http.StatusOK { + t.Fatal("revoked session must not authenticate after strict IP-change destroy") + } + + // Plan-limits is another public path — same treatment. + srv2 := testServer(t) + srv2.SetIPChangeEnforce("strict") + token2 := bootstrapFirstUser(t, srv2, "admin2@example.com", "Admin2") + rr = doRequestWithCookieFrom(srv2, "GET", "/api/v1/plan-limits", nil, token2, "198.51.100.7:5555") + if rr.Code == http.StatusUnauthorized && strings.Contains(rr.Body.String(), "session_ip_changed") { + t.Fatalf("stale cookie on plan-limits blocked with session_ip_changed: %s", rr.Body.String()) + } +} + +// TestSessionIPChange_CASDedupesRace verifies that the compare-and-set +// update scheme produces at most one audit row per IP-change transition, +// even when many concurrent requests arrive from the new IP before any +// of them has observed the rotated value. +func TestSessionIPChange_CASDedupesRace(t *testing.T) { + srv := testServer(t) + token := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + // Warm the stored IP with an initial request from the bootstrap source. + rr := doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "127.0.0.1:2222") + if rr.Code != http.StatusOK { + t.Fatalf("warmup: expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + + // Fire N concurrent requests from the new IP. Only the request whose + // CAS actually rotated the stored IP should have logged an audit row. + const n = 20 + done := make(chan struct{}, n) + for i := 0; i < n; i++ { + go func() { + doRequestWithCookieFrom(srv, "GET", "/api/v1/auth/me", nil, token, "198.51.100.7:5555") + done <- struct{}{} + }() + } + for i := 0; i < n; i++ { + <-done + } + + if got := countIPChangeEvents(t, srv); got != 1 { + t.Fatalf("expected exactly 1 audit row from %d concurrent requests, got %d", n, got) + } +} + +// TestClientIP_IPv6NotMangled verifies the clientIP helper uses +// net.SplitHostPort correctly so IPv6 addresses aren't truncated at an +// internal colon. After TrustedProxyRealIP writes X-Forwarded-For +// verbatim into RemoteAddr, a bare IPv6 like "2001:db8::1" has no port +// and the old LastIndex(":") approach would return "2001:db8:" — +// mangled and unusable for session-IP audits. +func TestClientIP_IPv6NotMangled(t *testing.T) { + cases := []struct { + remoteAddr string + want string + }{ + {"192.0.2.1:1234", "192.0.2.1"}, + {"192.0.2.1", "192.0.2.1"}, + {"[2001:db8::1]:8080", "2001:db8::1"}, + {"2001:db8::1", "2001:db8::1"}, // bare IPv6 (no port, no brackets) + {"127.0.0.1:1234", "127.0.0.1"}, + {"::1", "::1"}, + } + for _, tc := range cases { + req, _ := http.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = tc.remoteAddr + if got := clientIP(req); got != tc.want { + t.Errorf("clientIP(%q) = %q, want %q", tc.remoteAddr, got, tc.want) + } + } +} + +// TestSessionAuth_ShortCircuitsOnAPITokenAuth verifies that when a +// request carries BOTH a valid API token (Authorization: Bearer pad_...) +// AND a stale session cookie with a different client IP, SessionAuth +// short-circuits and lets the token-authenticated request through +// instead of 401-ing the whole thing in strict mode. Without this, +// valid token calls could be rejected purely because an unrelated +// browser tab dropped a stale cookie into the request. +func TestSessionAuth_ShortCircuitsOnAPITokenAuth(t *testing.T) { + srv := testServer(t) + // Bootstrap + create the token BEFORE enabling strict mode, so the + // setup calls (which come from 127.0.0.1 vs doRequestWithCookie's + // 192.0.2.1) don't trip the IP-change check. + sessionToken := bootstrapFirstUser(t, srv, "admin@example.com", "Admin") + + // api_tokens.workspace_id is NOT NULL on the legacy schema, so create + // a workspace first and scope the token to it. + rr := doRequestWithCookie(srv, "POST", "/api/v1/workspaces", + map[string]string{"name": "IP-Test"}, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create workspace: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var ws struct { + ID string `json:"id"` + Slug string `json:"slug"` + } + parseJSON(t, rr, &ws) + + // Create a user-scoped API token for the workspace. TokenAuth will + // set currentUser from the user_id on the token. + rr = doRequestWithCookie(srv, "POST", "/api/v1/auth/tokens", map[string]interface{}{ + "name": "integration-test-token", + "workspace_id": ws.ID, + }, sessionToken) + if rr.Code != http.StatusCreated { + t.Fatalf("create token: expected 201, got %d: %s", rr.Code, rr.Body.String()) + } + var tokResp map[string]interface{} + parseJSON(t, rr, &tokResp) + apiToken, _ := tokResp["token"].(string) + if apiToken == "" { + t.Fatal("expected token in response") + } + // Snapshot audit count AFTER setup so we can isolate the effect of + // the final request (setup calls from the 192.0.2.1 test remote will + // have emitted their own audit rows in log-only mode before strict + // is enabled). + srv.SetIPChangeEnforce("strict") + before := countIPChangeEvents(t, srv) + + // Request with BOTH the API token AND a stale session cookie, from a + // new client IP. Strict mode would 401 the cookie path — but the + // token-auth short-circuit must kick in first. + req, _ := http.NewRequest(http.MethodGet, "/api/v1/auth/me", nil) + req.RemoteAddr = "203.0.113.9:5555" // different IP from both bootstrap (127.0.0.1) and setup (192.0.2.1) + req.Header.Set("Authorization", "Bearer "+apiToken) + req.AddCookie(&http.Cookie{Name: "pad_session", Value: sessionToken}) + const testCSRF = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + req.AddCookie(&http.Cookie{Name: "pad_csrf", Value: testCSRF}) + req.Header.Set("X-CSRF-Token", testCSRF) + rec := httptest.NewRecorder() + srv.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("token + stale cookie in strict mode: expected 200 (token auth wins), got %d: %s", rec.Code, rec.Body.String()) + } + // The session-IP-change path MUST NOT have fired — no new audit rows + // should appear for this request. + after := countIPChangeEvents(t, srv) + if after != before { + t.Fatalf("expected 0 new ActionSessionIPChanged audit rows when token auth short-circuits; before=%d after=%d", before, after) + } +} + +// TestCanonicalIP verifies equivalent IPv6 representations collapse to a +// single canonical form so the session-IP-change comparison doesn't +// spuriously fire on "0000:0000:0000:0000:0000:0000:0000:0001" vs "::1". +func TestCanonicalIP(t *testing.T) { + cases := []struct { + in, want string + }{ + {"", ""}, + {"127.0.0.1", "127.0.0.1"}, + {"192.0.2.1", "192.0.2.1"}, + {"::1", "::1"}, + {"0000:0000:0000:0000:0000:0000:0000:0001", "::1"}, + {"2001:DB8::1", "2001:db8::1"}, + {"2001:0db8:0000:0000:0000:0000:0000:0001", "2001:db8::1"}, + {"not-an-ip", "not-an-ip"}, // non-parseable stays as-is + } + for _, tc := range cases { + if got := canonicalIP(tc.in); got != tc.want { + t.Errorf("canonicalIP(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} + +// TestSetIPChangeEnforce_CaseInsensitive verifies the setter accepts +// common variants without surprises. +func TestSetIPChangeEnforce_CaseInsensitive(t *testing.T) { + srv := testServer(t) + cases := []struct { + in string + want bool + }{ + {"", false}, + {"strict", true}, + {"STRICT", true}, + {" Strict ", true}, + {"log", false}, + {"yes", false}, + } + for _, tc := range cases { + srv.SetIPChangeEnforce(tc.in) + if srv.ipChangeEnforceStrict != tc.want { + t.Errorf("SetIPChangeEnforce(%q) → ipChangeEnforceStrict=%v, want %v", + tc.in, srv.ipChangeEnforceStrict, tc.want) + } + } +} + +// countIPChangeEvents returns the number of session_ip_changed rows in the +// audit log. +func countIPChangeEvents(t *testing.T, srv *Server) int { + t.Helper() + acts, err := srv.store.ListAuditLog(models.AuditLogParams{ + Action: models.ActionSessionIPChanged, + Limit: 100, + }) + if err != nil { + t.Fatalf("list audit log: %v", err) + } + // Guard against accidental matches on unrelated actions. + n := 0 + for _, a := range acts { + if a.Action == models.ActionSessionIPChanged { + n++ + } + } + return n +} + +// keep strings reference in case future cases need it; harmless no-op. +var _ = strings.EqualFold diff --git a/internal/server/server.go b/internal/server/server.go index b96a1edb..160d3243 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -42,11 +42,12 @@ type Server struct { baseURL string // public base URL for generating links (e.g. invite URLs) corsOrigins string // comma-separated CORS origins (empty = localhost defaults) secureCookies bool // set Secure flag on cookies (for TLS deployments) - metrics *metrics.Metrics // Prometheus metrics (optional) - metricsToken string // shared bearer token for /metrics scrapes ("" = loopback-only) - trustedProxyCIDRs []*net.IPNet // CIDRs allowed to set X-Forwarded-For (nil = proxy headers untrusted) - sseMaxConnections int // global SSE connection limit (0 = unlimited) - sseMaxPerWorkspace int // per-workspace SSE connection limit (0 = unlimited) + metrics *metrics.Metrics // Prometheus metrics (optional) + metricsToken string // shared bearer token for /metrics scrapes ("" = loopback-only) + trustedProxyCIDRs []*net.IPNet // CIDRs allowed to set X-Forwarded-For (nil = proxy headers untrusted) + ipChangeEnforceStrict bool // when true, reject sessions whose client IP differs from the one recorded at session creation + sseMaxConnections int // global SSE connection limit (0 = unlimited) + sseMaxPerWorkspace int // per-workspace SSE connection limit (0 = unlimited) cloudMode bool // true when running as Pad Cloud (PAD_CLOUD=true or PAD_MODE=cloud) cloudSecrets []string // shared secrets for sidecar ↔ pad communication (supports rotation) version string // release version (e.g. "dev", "1.2.3") @@ -237,6 +238,17 @@ func (s *Server) SetTrustedProxies(spec string) { s.trustedProxyCIDRs = ParseTrustedProxyCIDRs(spec) } +// SetIPChangeEnforce controls how the auth middleware reacts when a +// session's client IP changes mid-lifetime: +// - mode == "strict": reject the request (session treated as possibly stolen) +// - anything else (default): log to the audit log, update the stored IP, +// and let the request through. Strict mode breaks legitimate mobility +// (mobile roaming, VPN toggles) so it is opt-in for high-sensitivity +// deployments via the PAD_IP_CHANGE_ENFORCE env var. +func (s *Server) SetIPChangeEnforce(mode string) { + s.ipChangeEnforceStrict = strings.EqualFold(strings.TrimSpace(mode), "strict") +} + // reconfigureEmail reads email settings from the platform_settings table // and updates (or creates) the email sender. Called after admin settings change. func (s *Server) reconfigureEmail() { diff --git a/internal/store/sessions.go b/internal/store/sessions.go index 21d7abf6..3e87513d 100644 --- a/internal/store/sessions.go +++ b/internal/store/sessions.go @@ -94,6 +94,35 @@ func (s *Store) ValidateSession(token string) (*SessionInfo, error) { }, nil } +// UpdateSessionIPIfEquals atomically rotates the stored IP on a session +// from expectedOldIP to newIP. Returns true if a row was updated (i.e. the +// caller is the race winner and should write the audit row); false when +// some other concurrent request already rotated the IP ahead of us. +// +// This compare-and-set behavior is what lets the auth middleware emit +// exactly one ActionSessionIPChanged audit row per transition even when +// multiple requests race after the client's IP flips. +func (s *Store) UpdateSessionIPIfEquals(token, expectedOldIP, newIP string) (bool, error) { + hash := sha256.Sum256([]byte(token)) + tokenHash := hex.EncodeToString(hash[:]) + + res, err := s.db.Exec(s.q(` + UPDATE sessions + SET ip_address = ? + WHERE token_hash = ? AND ip_address = ? + `), newIP, tokenHash, expectedOldIP) + if err != nil { + return false, fmt.Errorf("update session ip: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + // Some drivers don't report RowsAffected reliably. Err on the safe + // side: treat as "not the winner" so we don't log when unsure. + return false, nil + } + return n > 0, nil +} + // DeleteSession destroys a session by its plaintext token. func (s *Store) DeleteSession(token string) error { hash := sha256.Sum256([]byte(token)) @@ -106,6 +135,30 @@ func (s *Store) DeleteSession(token string) error { return nil } +// DeleteSessionIfExists destroys a session by plaintext token and reports +// whether a row was actually deleted. Used by the IP-change strict-mode +// path as a compare-and-set primitive: only the caller that actually +// deletes the row emits the ActionSessionIPChanged audit entry so +// concurrent requests don't write duplicate audit rows, and only the +// caller whose DELETE succeeded on the DB gets a clean "session is now +// gone" guarantee. +func (s *Store) DeleteSessionIfExists(token string) (bool, error) { + hash := sha256.Sum256([]byte(token)) + tokenHash := hex.EncodeToString(hash[:]) + + res, err := s.db.Exec(s.q("DELETE FROM sessions WHERE token_hash = ?"), tokenHash) + if err != nil { + return false, fmt.Errorf("delete session: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + // Drivers that don't report affected rows → treat as "existed" so + // callers don't skip their post-delete work. + return true, nil + } + return n > 0, nil +} + // DeleteUserSessions destroys all sessions for a user (logout everywhere). func (s *Store) DeleteUserSessions(userID string) error { _, err := s.db.Exec(s.q("DELETE FROM sessions WHERE user_id = ?"), userID)