diff --git a/internal/api/api.go b/internal/api/api.go index 7e04e4c8..d27e2e47 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -87,6 +87,9 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Put("/me/timezone", h.handlePutTimezone) authed.Get("/me/api-token", h.handleGetMyAPIToken) authed.Post("/me/api-token", h.handleRegenerateMyAPIToken) + authed.Get("/me/sessions", h.handleListMySessions) + authed.Delete("/me/sessions/{id}", h.handleRevokeMySession) + authed.Post("/me/sessions/logout-others", h.handleRevokeMyOtherSessions) authed.Get("/artists", h.handleListArtists) authed.Get("/artists/{id}", h.handleGetArtist) diff --git a/internal/api/auth.go b/internal/api/auth.go index 00fd5164..46e18c2e 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -96,6 +96,11 @@ func (h *handlers) handleLogin(w http.ResponseWriter, r *http.Request) { UserID: user.ID, TokenHash: auth.HashSessionToken(token), UserAgent: r.UserAgent(), + // Origin address, frozen at issue time. Compared against last_ip in + // the active-sessions surface: a session that was born somewhere the + // user recognises but is being used from somewhere they don't is the + // case this whole surface exists to surface. + Ip: auth.ClientIP(r), }); err != nil { h.logger.Error("api: insert session failed", "err", err) writeErr(w, apierror.InternalMsg("insert failed", err)) diff --git a/internal/api/auth_register.go b/internal/api/auth_register.go index 849573c1..5bf1803e 100644 --- a/internal/api/auth_register.go +++ b/internal/api/auth_register.go @@ -175,6 +175,7 @@ func (h *handlers) handleRegister(w http.ResponseWriter, r *http.Request) { UserID: user.ID, TokenHash: auth.HashSessionToken(sessionToken), UserAgent: r.UserAgent(), + Ip: auth.ClientIP(r), }); err != nil { h.logger.Error("register: insert session failed", "err", err) writeErr(w, apierror.Internal(err)) diff --git a/internal/api/me_sessions.go b/internal/api/me_sessions.go new file mode 100644 index 00000000..e2f2f508 --- /dev/null +++ b/internal/api/me_sessions.go @@ -0,0 +1,136 @@ +package api + +import ( + "errors" + "net/http" + "time" + + "github.com/go-chi/chi/v5" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/audit" + "git.fabledsword.com/bvandeusen/minstrel/internal/auth" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// errNoCurrentSession means the request authenticated but the middleware +// didn't record which session did it — which should be impossible on a route +// behind RequireUser. It matters because "log out everywhere else" is defined +// by exclusion: without knowing which session is ours, the safe-looking +// action would sign the caller out too. +var errNoCurrentSession = errors.New("no session id in request context") + +// sessionResp is one row of the active-sessions list. +// +// token_hash is absent, and that is the point of storing only a hash: it +// never leaves the database, so this surface can list sessions without +// handing out anything that could be replayed. +type sessionResp struct { + ID string `json:"id"` + UserAgent string `json:"user_agent"` + // CreatedIP is frozen at issue time; LastIP moves with the session. The + // pair is what makes a stolen token legible — same device string, but an + // address the user doesn't recognise. + CreatedIP string `json:"created_ip"` + LastIP string `json:"last_ip"` + CreatedAt time.Time `json:"created_at"` + LastSeenAt time.Time `json:"last_seen_at"` + // Current marks the session making this request so the UI can label it + // and not offer a "log out" that signs the user out of the page they're + // standing on. + Current bool `json:"current"` +} + +type revokedResp struct { + Revoked int `json:"revoked"` +} + +// handleListMySessions implements GET /api/me/sessions. +func (h *handlers) handleListMySessions(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + // Absent id is tolerated here (unlike logout-others): the list still + // renders, it just won't flag a current row. + currentID, _ := auth.SessionIDFromContext(r.Context()) + + rows, err := dbq.New(h.pool).ListSessionsForUser(r.Context(), user.ID) + if err != nil { + h.logger.Error("list sessions: query failed", "err", err) + writeErr(w, apierror.Internal(err)) + return + } + out := make([]sessionResp, 0, len(rows)) + for _, s := range rows { + out = append(out, sessionResp{ + ID: uuidToString(s.ID), + UserAgent: s.UserAgent, + CreatedIP: s.CreatedIp, + LastIP: s.LastIp, + CreatedAt: s.CreatedAt.Time, + LastSeenAt: s.LastSeenAt.Time, + Current: s.ID == currentID, + }) + } + writeJSON(w, http.StatusOK, out) +} + +// handleRevokeMySession implements DELETE /api/me/sessions/{id}. +func (h *handlers) handleRevokeMySession(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + id, ok := parseUUID(chi.URLParam(r, "id")) + if !ok { + // Malformed and belongs-to-someone-else collapse to one answer on + // purpose: a distinguishable response would let a caller probe + // whether another user's session id exists. + writeErr(w, apierror.NotFound("session")) + return + } + n, err := dbq.New(h.pool).DeleteSessionForUser(r.Context(), dbq.DeleteSessionForUserParams{ + ID: id, + UserID: user.ID, + }) + if err != nil { + h.logger.Error("revoke session: delete failed", "err", err) + writeErr(w, apierror.Internal(err)) + return + } + if n == 0 { + writeErr(w, apierror.NotFound("session")) + return + } + audit.WriteOrLog(r.Context(), h.pool, h.logger, user.ID, user.ID, audit.ActionSessionRevoke, nil) + w.WriteHeader(http.StatusNoContent) +} + +// handleRevokeMyOtherSessions implements POST /api/me/sessions/logout-others. +func (h *handlers) handleRevokeMyOtherSessions(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + currentID, ok := auth.SessionIDFromContext(r.Context()) + if !ok { + // Refuse rather than guess: deleting "all but unknown" is deleting + // all, which would log the caller out of the page they invoked this + // from and look exactly like the attack they were defending against. + h.logger.Error("revoke other sessions: no session id in context") + writeErr(w, apierror.Internal(errNoCurrentSession)) + return + } + n, err := dbq.New(h.pool).DeleteOtherSessionsForUser(r.Context(), dbq.DeleteOtherSessionsForUserParams{ + UserID: user.ID, + ID: currentID, + }) + if err != nil { + h.logger.Error("revoke other sessions: delete failed", "err", err) + writeErr(w, apierror.Internal(err)) + return + } + audit.WriteOrLog(r.Context(), h.pool, h.logger, user.ID, user.ID, audit.ActionSessionRevokeOthers, nil) + writeJSON(w, http.StatusOK, revokedResp{Revoked: int(n)}) +} diff --git a/internal/api/me_sessions_test.go b/internal/api/me_sessions_test.go new file mode 100644 index 00000000..8f9c3d6d --- /dev/null +++ b/internal/api/me_sessions_test.go @@ -0,0 +1,226 @@ +package api + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/auth" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// seedSession inserts a session for userID and returns its id. +func seedSession(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, ip string) pgtype.UUID { + t.Helper() + token, err := auth.MintSessionToken() + if err != nil { + t.Fatalf("mint: %v", err) + } + sess, err := dbq.New(pool).InsertSession(context.Background(), dbq.InsertSessionParams{ + UserID: userID, + TokenHash: auth.HashSessionToken(token), + UserAgent: "test-agent", + Ip: ip, + }) + if err != nil { + t.Fatalf("insert session: %v", err) + } + return sess.ID +} + +// withSession attaches the user and current-session id the handlers expect +// from RequireUser. +func withSession(r *http.Request, user dbq.User, sessionID pgtype.UUID) *http.Request { + ctx := context.WithValue(r.Context(), userCtxKeyForTest(), user) + ctx = context.WithValue(ctx, auth.SessionIDCtxKeyForTest(), sessionID) + return r.WithContext(ctx) +} + +// withURLParam wires a chi route param, which handlers read via chi.URLParam. +func withURLParam(r *http.Request, key, value string) *http.Request { + rctx := chi.NewRouteContext() + rctx.URLParams.Add(key, value) + return r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, rctx)) +} + +// The rule #47 assertion. A delete keyed only on session id would let any +// household member revoke any other member's session by id — this pins that +// the user scope is actually in the WHERE clause and not just intended. +func TestRevokeMySession_CannotRevokeAnotherUsersSession(t *testing.T) { + h, pool := testHandlers(t) + alice := seedUser(t, pool, "alice", "hunter2", false) + bob := seedUser(t, pool, "bob", "hunter2", false) + + bobSession := seedSession(t, pool, bob.ID, "203.0.113.9") + aliceSession := seedSession(t, pool, alice.ID, "203.0.113.1") + + target := uuidToString(bobSession) + req := httptest.NewRequest(http.MethodDelete, "/api/me/sessions/"+target, nil) + req = withURLParam(req, "id", target) + req = withSession(req, alice, aliceSession) + w := httptest.NewRecorder() + h.handleRevokeMySession(w, req) + + if w.Code != http.StatusNotFound { + t.Errorf("status = %d, want 404 (not another user's to revoke)", w.Code) + } + + // The 404 must mean "didn't happen", not merely "wasn't reported". + var stillThere bool + if err := pool.QueryRow(context.Background(), + `SELECT EXISTS (SELECT 1 FROM sessions WHERE id = $1)`, bobSession, + ).Scan(&stillThere); err != nil { + t.Fatalf("exists check: %v", err) + } + if !stillThere { + t.Error("bob's session was deleted by alice's request") + } +} + +func TestRevokeMySession_DeletesOwnSession(t *testing.T) { + h, pool := testHandlers(t) + alice := seedUser(t, pool, "alice", "hunter2", false) + current := seedSession(t, pool, alice.ID, "203.0.113.1") + other := seedSession(t, pool, alice.ID, "198.51.100.7") + + target := uuidToString(other) + req := httptest.NewRequest(http.MethodDelete, "/api/me/sessions/"+target, nil) + req = withURLParam(req, "id", target) + req = withSession(req, alice, current) + w := httptest.NewRecorder() + h.handleRevokeMySession(w, req) + + if w.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204", w.Code) + } + var gone bool + if err := pool.QueryRow(context.Background(), + `SELECT NOT EXISTS (SELECT 1 FROM sessions WHERE id = $1)`, other, + ).Scan(&gone); err != nil { + t.Fatalf("exists check: %v", err) + } + if !gone { + t.Error("session survived its own owner's revoke") + } +} + +// "Log out everywhere else" must spare the caller — otherwise the button +// signs you out of the page you pressed it on, which is indistinguishable +// from the compromise it's meant to remedy. +func TestRevokeMyOtherSessions_SparesCurrentAndOtherUsers(t *testing.T) { + h, pool := testHandlers(t) + alice := seedUser(t, pool, "alice", "hunter2", false) + bob := seedUser(t, pool, "bob", "hunter2", false) + + current := seedSession(t, pool, alice.ID, "203.0.113.1") + seedSession(t, pool, alice.ID, "198.51.100.7") + seedSession(t, pool, alice.ID, "198.51.100.8") + bobSession := seedSession(t, pool, bob.ID, "203.0.113.9") + + req := httptest.NewRequest(http.MethodPost, "/api/me/sessions/logout-others", nil) + req = withSession(req, alice, current) + w := httptest.NewRecorder() + h.handleRevokeMyOtherSessions(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", w.Code) + } + var body revokedResp + if err := json.NewDecoder(w.Body).Decode(&body); err != nil { + t.Fatalf("decode: %v", err) + } + if body.Revoked != 2 { + t.Errorf("revoked = %d, want 2 (alice's other two, not bob's)", body.Revoked) + } + + var aliceRemaining, bobRemaining int + if err := pool.QueryRow(context.Background(), + `SELECT count(*) FROM sessions WHERE user_id = $1`, alice.ID, + ).Scan(&aliceRemaining); err != nil { + t.Fatalf("count alice: %v", err) + } + if aliceRemaining != 1 { + t.Errorf("alice sessions = %d, want 1 (the current one)", aliceRemaining) + } + if err := pool.QueryRow(context.Background(), + `SELECT count(*) FROM sessions WHERE id = $1`, bobSession, + ).Scan(&bobRemaining); err != nil { + t.Fatalf("count bob: %v", err) + } + if bobRemaining != 1 { + t.Error("bob's session was caught in alice's logout-others") + } +} + +// Without a current-session id the exclusion has nothing to exclude, so the +// handler must refuse rather than delete everything. +func TestRevokeMyOtherSessions_RefusesWithoutCurrentSession(t *testing.T) { + h, pool := testHandlers(t) + alice := seedUser(t, pool, "alice", "hunter2", false) + seedSession(t, pool, alice.ID, "203.0.113.1") + + req := httptest.NewRequest(http.MethodPost, "/api/me/sessions/logout-others", nil) + req = req.WithContext(context.WithValue(req.Context(), userCtxKeyForTest(), alice)) + w := httptest.NewRecorder() + h.handleRevokeMyOtherSessions(w, req) + + if w.Code != http.StatusInternalServerError { + t.Errorf("status = %d, want 500", w.Code) + } + var remaining int + if err := pool.QueryRow(context.Background(), + `SELECT count(*) FROM sessions WHERE user_id = $1`, alice.ID, + ).Scan(&remaining); err != nil { + t.Fatalf("count: %v", err) + } + if remaining != 1 { + t.Errorf("sessions = %d, want 1 — refusing must not delete", remaining) + } +} + +func TestListMySessions_FlagsCurrentAndScopesToUser(t *testing.T) { + h, pool := testHandlers(t) + alice := seedUser(t, pool, "alice", "hunter2", false) + bob := seedUser(t, pool, "bob", "hunter2", false) + + current := seedSession(t, pool, alice.ID, "203.0.113.1") + seedSession(t, pool, alice.ID, "198.51.100.7") + seedSession(t, pool, bob.ID, "203.0.113.9") + + req := httptest.NewRequest(http.MethodGet, "/api/me/sessions", nil) + req = withSession(req, alice, current) + w := httptest.NewRecorder() + h.handleListMySessions(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", w.Code) + } + var got []sessionResp + if err := json.NewDecoder(w.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + if len(got) != 2 { + t.Fatalf("sessions = %d, want 2 (bob's must not appear)", len(got)) + } + currentCount := 0 + for _, s := range got { + if s.Current { + currentCount++ + if s.ID != uuidToString(current) { + t.Errorf("current flagged on %s, want %s", s.ID, uuidToString(current)) + } + } + if s.CreatedIP == "" { + t.Error("created_ip empty — the whole point of the surface") + } + } + if currentCount != 1 { + t.Errorf("current-flagged rows = %d, want exactly 1", currentCount) + } +} diff --git a/internal/audit/audit.go b/internal/audit/audit.go index e38b5ee4..eb2e8a5b 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -48,6 +48,13 @@ const ( ActionTokenRegenerate Action = "token_regenerate" ActionForgotPasswordInit Action = "forgot_password_initiated" ActionPasswordResetByEmail Action = "password_reset_via_email" + + // Active-sessions surface (#370). Worth auditing rather than silent: + // revoking sessions is what a user does when they think an account is + // compromised, so the audit trail is most useful precisely when it's + // exercised. + ActionSessionRevoke Action = "session_revoke" + ActionSessionRevokeOthers Action = "session_revoke_others" ) // Write inserts one audit_log row. metadata is marshaled as JSON; diff --git a/internal/auth/clientip.go b/internal/auth/clientip.go new file mode 100644 index 00000000..ba228fe0 --- /dev/null +++ b/internal/auth/clientip.go @@ -0,0 +1,92 @@ +package auth + +import ( + "net" + "net/http" + "strings" +) + +// ClientIP returns the caller's address for the active-sessions surface (#370). +// +// Both obvious implementations are wrong, and they're wrong in ways that +// matter specifically because this feeds a compromise-detection UI: +// +// - r.RemoteAddr alone. Minstrel is normally behind a reverse proxy, so +// every session would show the proxy's address — noise shaped like data, +// hiding the exact thing the operator is looking for. +// - Trusting X-Forwarded-For. Any client can set that header, so an +// attacker could choose what appears in their victim's session list. +// A security surface an attacker can write to is worse than none. +// +// So the header is trusted only when the request actually arrived from a +// proxy. If RemoteAddr is public, the caller reached us directly and its XFF +// is attacker-controlled, so it's ignored outright. If RemoteAddr is +// private/loopback, XFF is walked from the RIGHT — entries are appended as a +// request passes through infrastructure, so the rightmost end is the one our +// own proxies wrote — and the first address that isn't itself a proxy range +// wins. A client forging XFF can only prepend to the untrusted left end, +// which that walk never reaches. +// +// Known limitation, failing closed on purpose: if the proxy sits on a PUBLIC +// address (a separate host, or a CDN in front), RemoteAddr isn't in a proxy +// range, so we report the proxy rather than the end user. That's a true fact +// about where the request came from, which beats trusting a forgeable header. +// +// Returns "" when nothing usable can be determined. Callers store that as-is +// and the UI renders "unknown" rather than inventing a value. +func ClientIP(r *http.Request) string { + remote := hostOf(r.RemoteAddr) + ip := net.ParseIP(remote) + if ip == nil || !isProxyRange(ip) { + return remote + } + if forwarded := forwardedClient(r.Header.Get("X-Forwarded-For")); forwarded != "" { + return forwarded + } + // Some proxies set only X-Real-IP. The trust condition is already + // satisfied — we know this request came from a proxy range. + if real := net.ParseIP(strings.TrimSpace(r.Header.Get("X-Real-IP"))); real != nil { + return real.String() + } + return remote +} + +// hostOf strips the port from a RemoteAddr, tolerating values that have none. +func hostOf(remoteAddr string) string { + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + return strings.TrimSpace(remoteAddr) + } + return host +} + +// forwardedClient walks an X-Forwarded-For value right-to-left and returns +// the first address outside our proxy ranges — see ClientIP for why the +// direction matters. Returns "" if the header is absent, malformed, or +// contains nothing but proxy addresses. +func forwardedClient(header string) string { + parts := strings.Split(header, ",") + for i := len(parts) - 1; i >= 0; i-- { + ip := net.ParseIP(strings.TrimSpace(parts[i])) + if ip == nil || isProxyRange(ip) { + continue + } + return ip.String() + } + return "" +} + +// isProxyRange reports whether ip is an address a reverse proxy would +// plausibly occupy in a self-hosted deployment: loopback, RFC1918 / ULA +// (both covered by IsPrivate), link-local, or unspecified. +// +// Deliberately not configurable. These ranges cover proxy-on-same-host and +// proxy-on-the-same-docker-network, which is essentially every self-hosted +// install, and it works with no setup at all (rule #26). An exotic topology +// can motivate a setting when one actually turns up. +func isProxyRange(ip net.IP) bool { + return ip.IsLoopback() || + ip.IsPrivate() || + ip.IsLinkLocalUnicast() || + ip.IsUnspecified() +} diff --git a/internal/auth/clientip_test.go b/internal/auth/clientip_test.go new file mode 100644 index 00000000..8b49633e --- /dev/null +++ b/internal/auth/clientip_test.go @@ -0,0 +1,121 @@ +package auth + +import ( + "net/http" + "testing" +) + +// The spoofing cases below are the reason this function exists rather than a +// one-line r.RemoteAddr read, so they're asserted explicitly rather than +// folded into the happy-path table. +func TestClientIP(t *testing.T) { + tests := []struct { + name string + remoteAddr string + forwarded string + realIP string + want string + }{ + { + name: "direct connection, no proxy headers", + remoteAddr: "203.0.113.5:51234", + want: "203.0.113.5", + }, + { + // The attack this guards against: a client connecting straight to + // us claims to be someone else. RemoteAddr is public, so it did + // NOT come through our proxy, so its XFF is worthless. + name: "direct connection ignores forged X-Forwarded-For", + remoteAddr: "203.0.113.5:51234", + forwarded: "198.51.100.99", + want: "203.0.113.5", + }, + { + name: "direct connection ignores forged X-Real-IP", + remoteAddr: "203.0.113.5:51234", + realIP: "198.51.100.99", + want: "203.0.113.5", + }, + { + name: "behind proxy, single forwarded client", + remoteAddr: "172.18.0.1:40000", + forwarded: "203.0.113.5", + want: "203.0.113.5", + }, + { + // A client that prepends a lie to XFF only pollutes the LEFT end; + // the proxy appends the address it actually saw on the right. The + // right-to-left walk reaches the truth first. + name: "behind proxy, forged prefix is skipped for the appended truth", + remoteAddr: "10.0.0.2:40000", + forwarded: "198.51.100.99, 203.0.113.5", + want: "203.0.113.5", + }, + { + name: "behind proxy chain, internal hops skipped", + remoteAddr: "10.0.0.2:40000", + forwarded: "203.0.113.5, 10.0.0.7, 172.18.0.3", + want: "203.0.113.5", + }, + { + name: "behind proxy, X-Real-IP used when no forwarded header", + remoteAddr: "127.0.0.1:40000", + realIP: "203.0.113.5", + want: "203.0.113.5", + }, + { + // LAN client through a LAN proxy: everything is private, so there + // is no public address to find. Reporting the peer is honest. + name: "behind proxy, all-private chain falls back to remote", + remoteAddr: "172.18.0.1:40000", + forwarded: "192.168.1.50, 172.18.0.3", + want: "172.18.0.1", + }, + { + name: "behind proxy, malformed forwarded entries ignored", + remoteAddr: "172.18.0.1:40000", + forwarded: "not-an-ip, 203.0.113.5, also-garbage", + want: "203.0.113.5", + }, + { + name: "remote addr without a port is tolerated", + remoteAddr: "203.0.113.5", + want: "203.0.113.5", + }, + { + name: "ipv6 remote addr", + remoteAddr: "[2001:db8::1]:51234", + want: "2001:db8::1", + }, + { + name: "ipv6 forwarded client behind proxy", + remoteAddr: "[fd00::1]:40000", + forwarded: "2001:db8::5", + want: "2001:db8::5", + }, + { + name: "empty remote addr yields empty", + remoteAddr: "", + want: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + r, err := http.NewRequest(http.MethodGet, "/api/me/sessions", nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + r.RemoteAddr = tc.remoteAddr + if tc.forwarded != "" { + r.Header.Set("X-Forwarded-For", tc.forwarded) + } + if tc.realIP != "" { + r.Header.Set("X-Real-IP", tc.realIP) + } + if got := ClientIP(r); got != tc.want { + t.Errorf("ClientIP() = %q, want %q", got, tc.want) + } + }) + } +} diff --git a/internal/auth/middleware.go b/internal/auth/middleware.go index 1438c418..139bf1f7 100644 --- a/internal/auth/middleware.go +++ b/internal/auth/middleware.go @@ -3,12 +3,17 @@ package auth import ( "context" + "github.com/jackc/pgx/v5/pgtype" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) type ctxKey int -const userCtxKey ctxKey = 1 +const ( + userCtxKey ctxKey = 1 + sessionIDCtxKey ctxKey = 2 +) // UserFromContext returns the authenticated user placed in context by // RequireUser. Returns false when RequireUser has not run (e.g. in tests that @@ -17,3 +22,13 @@ func UserFromContext(ctx context.Context) (dbq.User, bool) { u, ok := ctx.Value(userCtxKey).(dbq.User) return u, ok } + +// SessionIDFromContext returns the id of the session that authenticated this +// request. The active-sessions surface needs it for the two things it can't +// do from the user alone: mark which row is "this device", and exclude that +// row from "log out everywhere else" so the action doesn't sign the caller +// out of the page they invoked it from. +func SessionIDFromContext(ctx context.Context) (pgtype.UUID, bool) { + id, ok := ctx.Value(sessionIDCtxKey).(pgtype.UUID) + return id, ok +} diff --git a/internal/auth/session.go b/internal/auth/session.go index e949223e..9ebed6e4 100644 --- a/internal/auth/session.go +++ b/internal/auth/session.go @@ -98,10 +98,17 @@ func RequireUser(pool *pgxpool.Pool) func(http.Handler) http.Handler { } // Best-effort last-seen update. A failure here shouldn't fail the // request; the session is still valid and this is observability. - if err := q.TouchSessionLastSeen(r.Context(), sess.ID); err != nil { + // last_ip rides the same UPDATE — a session whose address has + // moved since it was issued is the signal the active-sessions + // surface exists to show, and it costs nothing extra here. + if err := q.TouchSessionLastSeen(r.Context(), dbq.TouchSessionLastSeenParams{ + ID: sess.ID, + LastIp: ClientIP(r), + }); err != nil { slog.Warn("api: touch session last_seen failed", "err", err) } ctx := context.WithValue(r.Context(), userCtxKey, user) + ctx = context.WithValue(ctx, sessionIDCtxKey, sess.ID) next.ServeHTTP(w, r.WithContext(ctx)) }) } @@ -112,6 +119,12 @@ func RequireUser(pool *pgxpool.Pool) func(http.Handler) http.Handler { // middleware. Do not use this outside _test.go files. func UserCtxKeyForTest() any { return userCtxKey } +// SessionIDCtxKeyForTest is the sibling of UserCtxKeyForTest for the session +// id, so handler tests can exercise the current-session logic (which row is +// "this device", which one logout-others must spare) without standing up the +// middleware. Do not use this outside _test.go files. +func SessionIDCtxKeyForTest() any { return sessionIDCtxKey } + // OptionalUser is RequireUser's permissive sibling: it resolves the caller // from the session cookie or bearer header and attaches the user to context // when present + valid, but does NOT 401 on absence. The downstream handler @@ -153,6 +166,7 @@ func OptionalUser(pool *pgxpool.Pool, logger *slog.Logger) func(http.Handler) ht return } ctx := context.WithValue(r.Context(), userCtxKey, user) + ctx = context.WithValue(ctx, sessionIDCtxKey, sess.ID) next.ServeHTTP(w, r.WithContext(ctx)) }) } diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index bfbb4ef8..56fc74af 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -514,6 +514,8 @@ type Session struct { UserAgent string CreatedAt pgtype.Timestamptz LastSeenAt pgtype.Timestamptz + CreatedIp string + LastIp string } type SkipEvent struct { diff --git a/internal/db/dbq/sessions.sql.go b/internal/db/dbq/sessions.sql.go index bc34671b..95c05d84 100644 --- a/internal/db/dbq/sessions.sql.go +++ b/internal/db/dbq/sessions.sql.go @@ -11,6 +11,25 @@ import ( "github.com/jackc/pgx/v5/pgtype" ) +const deleteOtherSessionsForUser = `-- name: DeleteOtherSessionsForUser :execrows +DELETE FROM sessions WHERE user_id = $1 AND id <> $2 +` + +type DeleteOtherSessionsForUserParams struct { + UserID pgtype.UUID + ID pgtype.UUID +} + +// "Log out everywhere else." Excludes the caller's own session so the action +// doesn't log them out of the page they just used to invoke it. +func (q *Queries) DeleteOtherSessionsForUser(ctx context.Context, arg DeleteOtherSessionsForUserParams) (int64, error) { + result, err := q.db.Exec(ctx, deleteOtherSessionsForUser, arg.UserID, arg.ID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const deleteSession = `-- name: DeleteSession :exec DELETE FROM sessions WHERE id = $1 ` @@ -29,8 +48,29 @@ func (q *Queries) DeleteSessionByTokenHash(ctx context.Context, tokenHash []byte return err } +const deleteSessionForUser = `-- name: DeleteSessionForUser :execrows +DELETE FROM sessions WHERE id = $1 AND user_id = $2 +` + +type DeleteSessionForUserParams struct { + ID pgtype.UUID + UserID pgtype.UUID +} + +// Scoped by user_id, not just id (rule #47). Keyed on the id alone, any +// household member could revoke another member's session by guessing a uuid. +// execrows lets the handler answer 404 rather than a false 204 when the row +// isn't theirs. +func (q *Queries) DeleteSessionForUser(ctx context.Context, arg DeleteSessionForUserParams) (int64, error) { + result, err := q.db.Exec(ctx, deleteSessionForUser, arg.ID, arg.UserID) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const getSessionByTokenHash = `-- name: GetSessionByTokenHash :one -SELECT id, user_id, token_hash, user_agent, created_at, last_seen_at FROM sessions WHERE token_hash = $1 +SELECT id, user_id, token_hash, user_agent, created_at, last_seen_at, created_ip, last_ip FROM sessions WHERE token_hash = $1 ` func (q *Queries) GetSessionByTokenHash(ctx context.Context, tokenHash []byte) (Session, error) { @@ -43,24 +83,35 @@ func (q *Queries) GetSessionByTokenHash(ctx context.Context, tokenHash []byte) ( &i.UserAgent, &i.CreatedAt, &i.LastSeenAt, + &i.CreatedIp, + &i.LastIp, ) return i, err } const insertSession = `-- name: InsertSession :one -INSERT INTO sessions (user_id, token_hash, user_agent) -VALUES ($1, $2, $3) -RETURNING id, user_id, token_hash, user_agent, created_at, last_seen_at +INSERT INTO sessions (user_id, token_hash, user_agent, created_ip, last_ip) +VALUES ($1, $2, $3, $4, $4) +RETURNING id, user_id, token_hash, user_agent, created_at, last_seen_at, created_ip, last_ip ` type InsertSessionParams struct { UserID pgtype.UUID TokenHash []byte UserAgent string + Ip string } +// created_ip and last_ip start equal: at issue time the origin IS the current +// location. They diverge as the session is used from elsewhere, which is what +// makes a stolen token visible in the active-sessions surface. func (q *Queries) InsertSession(ctx context.Context, arg InsertSessionParams) (Session, error) { - row := q.db.QueryRow(ctx, insertSession, arg.UserID, arg.TokenHash, arg.UserAgent) + row := q.db.QueryRow(ctx, insertSession, + arg.UserID, + arg.TokenHash, + arg.UserAgent, + arg.Ip, + ) var i Session err := row.Scan( &i.ID, @@ -69,15 +120,57 @@ func (q *Queries) InsertSession(ctx context.Context, arg InsertSessionParams) (S &i.UserAgent, &i.CreatedAt, &i.LastSeenAt, + &i.CreatedIp, + &i.LastIp, ) return i, err } -const touchSessionLastSeen = `-- name: TouchSessionLastSeen :exec -UPDATE sessions SET last_seen_at = now() WHERE id = $1 +const listSessionsForUser = `-- name: ListSessionsForUser :many +SELECT id, user_id, token_hash, user_agent, created_at, last_seen_at, created_ip, last_ip FROM sessions WHERE user_id = $1 ORDER BY last_seen_at DESC ` -func (q *Queries) TouchSessionLastSeen(ctx context.Context, id pgtype.UUID) error { - _, err := q.db.Exec(ctx, touchSessionLastSeen, id) +// Most-recently-active first: the row a user is most likely to act on is the +// one that moved last, and an unfamiliar entry at the top is the alarm. +func (q *Queries) ListSessionsForUser(ctx context.Context, userID pgtype.UUID) ([]Session, error) { + rows, err := q.db.Query(ctx, listSessionsForUser, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Session + for rows.Next() { + var i Session + if err := rows.Scan( + &i.ID, + &i.UserID, + &i.TokenHash, + &i.UserAgent, + &i.CreatedAt, + &i.LastSeenAt, + &i.CreatedIp, + &i.LastIp, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const touchSessionLastSeen = `-- name: TouchSessionLastSeen :exec +UPDATE sessions SET last_seen_at = now(), last_ip = $2 WHERE id = $1 +` + +type TouchSessionLastSeenParams struct { + ID pgtype.UUID + LastIp string +} + +func (q *Queries) TouchSessionLastSeen(ctx context.Context, arg TouchSessionLastSeenParams) error { + _, err := q.db.Exec(ctx, touchSessionLastSeen, arg.ID, arg.LastIp) return err } diff --git a/internal/db/migrations/0052_session_ips.down.sql b/internal/db/migrations/0052_session_ips.down.sql new file mode 100644 index 00000000..e1e631d6 --- /dev/null +++ b/internal/db/migrations/0052_session_ips.down.sql @@ -0,0 +1,3 @@ +ALTER TABLE sessions + DROP COLUMN created_ip, + DROP COLUMN last_ip; diff --git a/internal/db/migrations/0052_session_ips.up.sql b/internal/db/migrations/0052_session_ips.up.sql new file mode 100644 index 00000000..1da0bf48 --- /dev/null +++ b/internal/db/migrations/0052_session_ips.up.sql @@ -0,0 +1,19 @@ +-- Session provenance for the active-sessions surface (#370). +-- +-- TWO addresses, not one, and the pair is the point: a session created at +-- home and now being used from somewhere else is the shape of a stolen +-- token. A single "current IP" column can't express that, and a single +-- "origin IP" column goes stale the moment the token moves. +-- +-- text rather than inet, matching user_agent directly above: these are +-- stored to be displayed, never queried by subnet, and inet round-trips +-- through pgx/sqlc as a netip.Prefix that renders as "1.2.3.4/32" and would +-- need unwrapping at every display site. +-- +-- DEFAULT '' rather than NULL so existing rows — and any future insert that +-- genuinely can't determine an address — stay renderable without a null +-- check at every call site. The UI reads empty as "unknown" rather than +-- inventing a value. +ALTER TABLE sessions + ADD COLUMN created_ip text NOT NULL DEFAULT '', + ADD COLUMN last_ip text NOT NULL DEFAULT ''; diff --git a/internal/db/queries/sessions.sql b/internal/db/queries/sessions.sql index 438b066f..9c809053 100644 --- a/internal/db/queries/sessions.sql +++ b/internal/db/queries/sessions.sql @@ -1,16 +1,36 @@ -- name: InsertSession :one -INSERT INTO sessions (user_id, token_hash, user_agent) -VALUES ($1, $2, $3) +-- created_ip and last_ip start equal: at issue time the origin IS the current +-- location. They diverge as the session is used from elsewhere, which is what +-- makes a stolen token visible in the active-sessions surface. +INSERT INTO sessions (user_id, token_hash, user_agent, created_ip, last_ip) +VALUES ($1, $2, $3, sqlc.arg(ip), sqlc.arg(ip)) RETURNING *; -- name: GetSessionByTokenHash :one SELECT * FROM sessions WHERE token_hash = $1; -- name: TouchSessionLastSeen :exec -UPDATE sessions SET last_seen_at = now() WHERE id = $1; +UPDATE sessions SET last_seen_at = now(), last_ip = $2 WHERE id = $1; + +-- name: ListSessionsForUser :many +-- Most-recently-active first: the row a user is most likely to act on is the +-- one that moved last, and an unfamiliar entry at the top is the alarm. +SELECT * FROM sessions WHERE user_id = $1 ORDER BY last_seen_at DESC; -- name: DeleteSession :exec DELETE FROM sessions WHERE id = $1; -- name: DeleteSessionByTokenHash :exec DELETE FROM sessions WHERE token_hash = $1; + +-- name: DeleteSessionForUser :execrows +-- Scoped by user_id, not just id (rule #47). Keyed on the id alone, any +-- household member could revoke another member's session by guessing a uuid. +-- execrows lets the handler answer 404 rather than a false 204 when the row +-- isn't theirs. +DELETE FROM sessions WHERE id = $1 AND user_id = $2; + +-- name: DeleteOtherSessionsForUser :execrows +-- "Log out everywhere else." Excludes the caller's own session so the action +-- doesn't log them out of the page they just used to invoke it. +DELETE FROM sessions WHERE user_id = $1 AND id <> $2;