Silent self-update, active sessions with real client IPs, genre/year browsing, handoff fix #119
@@ -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)
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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)})
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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))
|
||||
})
|
||||
}
|
||||
|
||||
@@ -514,6 +514,8 @@ type Session struct {
|
||||
UserAgent string
|
||||
CreatedAt pgtype.Timestamptz
|
||||
LastSeenAt pgtype.Timestamptz
|
||||
CreatedIp string
|
||||
LastIp string
|
||||
}
|
||||
|
||||
type SkipEvent struct {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -0,0 +1,3 @@
|
||||
ALTER TABLE sessions
|
||||
DROP COLUMN created_ip,
|
||||
DROP COLUMN last_ip;
|
||||
@@ -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 '';
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user