fix(auth): refuse to create a local password on a directory-backed user (SA-7)

internal/handler/admin.go

@@ -262,10 +262,29 @@ func (h *Handler) handleSetPassword(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	log.Printf("[admin] Password set guid=%s force_change=%v ip=%s", guid, user.ForcePasswordChange, getClientIP(r))
+	// A master admin MAY set a local password on a directory-backed user — the
+	// master key is the top of this system's trust model, and break-glass is a
+	// legitimate need. But it creates the same shadow credential SA-7 blocks on the
+	// self-service path (it survives AD disablement, because nothing here mirrors
+	// userAccountControl), so record it: this flag is how an operator later tells a
+	// deliberate break-glass apart from an account takeover.
+	directoryBacked := h.isDirectoryBacked(user)
+	// Render the flag as a fresh label rather than logging the field value. It is a
+	// plain bool, not credential material, but a `*Password*` field access flowing
+	// into a sink is what CodeQL's clear-text-logging heuristic keys on — and a
+	// yes/no label is more readable in a log anyway.
+	mustChange := "no"
+	if user.ForcePasswordChange {
+		mustChange = "yes"
+	}
+	// Log user.GUID, not the raw `guid` path parameter: GetUser above succeeded, so
+	// user.GUID is the store's canonical value for the same record, and no
+	// request-supplied text reaches the log or audit sink.
+	log.Printf("[admin] Password set guid=%s force_change=%s directory_backed=%v ip=%s", user.GUID, mustChange, directoryBacked, getClientIP(r))
 	h.audit("password_set", "admin", getClientIP(r), map[string]interface{}{
-		"target_guid":  guid,
-		"force_change": user.ForcePasswordChange,
+		"target_guid":      user.GUID,
+		"force_change":     user.ForcePasswordChange,
+		"directory_backed": directoryBacked,
 	})
 
 	jsonResp(w, map[string]interface{}{

internal/handler/auth.go

@@ -433,6 +433,65 @@ func (h *Handler) issueTokenPair(user *store.User, roles []string, perms []strin
 	return accessToken, refreshToken, int(h.cfg.AccessTTL.Seconds()), nil
 }
 
+// isDirectoryBacked reports whether the user's AUTHORITATIVE credential lives in
+// the directory (AD, via LDAP or Kerberos) rather than in SimpleAuth.
+//
+// This is the server-side truth behind the `auth_source` hint /api/auth/userinfo
+// returns. It exists because a local password hash on a directory identity is a
+// permanent SHADOW credential: authenticateUser resolves the "local" mapping
+// FIRST ("local users always take priority") and nothing in SimpleAuth mirrors AD
+// account state — there is no userAccountControl read anywhere — so such a hash
+// keeps working after the AD account is disabled or the employee is terminated,
+// while the victim's AD password still works (a failed local check falls through
+// to the LDAP step) so nothing looks wrong (SA-7).
+//
+// No single signal is complete, so three are combined:
+//
+//   - OwnerAppID != "" → an app-LOCAL user (M5). Their password genuinely lives
+//     here and they must keep self-service. Checked first because it is exact.
+//   - SAMAccountName != "" → written ONLY by syncUserFromLDAP and the LDAP/Kerberos
+//     JIT paths; no admin or app API exposes the field, so it is a reliable
+//     positive. Not sufficient alone: POST /api/admin/ldap/import-users creates the
+//     "ldap" mapping but leaves SAMAccountName empty until the first login.
+//   - any identity mapping whose provider is neither "local" nor "applocal:<app_id>"
+//     — "ldap", "kerberos", or any federated provider an admin attached. Note this
+//     is deliberately NOT userinfo's plain `!= "local"` test, which would wrongly
+//     call an app-local customer a directory user.
+//
+// A user who is BOTH (an admin-created local account later linked to AD) counts as
+// directory-backed here. That only bites on the CREATE path — callers consult this
+// when the local hash is EMPTY; an existing local hash stays rotatable with proof
+// of possession.
+//
+// NOTE: internal/migrate/bundle.go:classifyUser answers a DIFFERENT question — it
+// must produce a portable KEY for a user, not a yes/no verdict — and classifies on
+// hash-presence and SAMAccountName rather than this predicate. Do not unify the
+// two: internal/migrate importing from internal/handler would invert the
+// dependency direction.
+func (h *Handler) isDirectoryBacked(user *store.User) bool {
+	if user.OwnerAppID != "" {
+		return false
+	}
+	if user.SAMAccountName != "" {
+		return true
+	}
+	mappings, err := h.store.GetMappingsForUser(user.GUID)
+	if err != nil {
+		// Fail CLOSED. A store error means we cannot PROVE the account is local. The
+		// cost of a false positive is one refused password change (an admin can still
+		// set it); the cost of a false negative is a permanent shadow credential on
+		// an AD identity.
+		log.Printf("[password] mappings lookup failed guid=%s err=%v — treating as directory-backed", user.GUID, err)
+		return true
+	}
+	for _, m := range mappings {
+		if m.Provider != "local" && !strings.HasPrefix(m.Provider, "applocal:") {
+			return true
+		}
+	}
+	return false
+}
+
 // resolvePreferredUsername finds the username for a user from identity mappings.
 // Priority: local mapping > ldap mapping > email > display name.
 func (h *Handler) resolvePreferredUsername(user *store.User) string {
@@ -813,8 +872,54 @@ func (h *Handler) handleResetPassword(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Verify current password if user has one set (skip if force_password_change is set)
-	if user.PasswordHash != "" && !user.ForcePasswordChange {
+	// This endpoint ROTATES an existing SimpleAuth-local password. It must never
+	// CREATE one. The old guard (`PasswordHash != "" && !ForcePasswordChange`)
+	// skipped the entire proof-of-possession block whenever the hash was empty —
+	// which is exactly the state of every directory user, whose credential lives
+	// in AD — so a bearer token minted for ANY audience could POST
+	// {"new_password":...} with no current_password and plant a permanent local
+	// bcrypt hash on an AD-backed record (SA-7). See isDirectoryBacked for why
+	// that hash then outlives the AD account.
+	//
+	// Both refusals are 403, never "supply current_password": a directory user has
+	// no local password to prove, so demanding one would be an unsatisfiable 400
+	// loop rather than an honest "wrong place — change it in AD".
+	if user.PasswordHash == "" {
+		ip := getClientIP(r)
+		if h.isDirectoryBacked(user) {
+			// Log the GUID, not the resolved username: the GUID is server-generated
+			// and unspoofable, whereas a directory-supplied username in a security
+			// log is both injectable and less useful for correlation.
+			log.Printf("[password] REFUSED directory-backed guid=%s ip=%s (attempt to plant a local password)",
+				user.GUID, ip)
+			h.audit("password_change_denied", user.GUID, ip, map[string]interface{}{
+				"reason": "directory_backed",
+			})
+			jsonError(w, "password is managed by the directory — change it in Active Directory", http.StatusForbidden)
+			return
+		}
+		// Not directory-backed, but no local credential exists either: an
+		// admin-created account with no password. There is no first-time-set flow
+		// to protect here — authenticateUser requires a non-empty hash in every
+		// local branch, so this user can never log in and can never hold a token of
+		// their own. Setting the initial password is an administrative act
+		// (PUT /api/admin/users/{guid}/password).
+		log.Printf("[password] REFUSED no local password guid=%s ip=%s", user.GUID, ip)
+		h.audit("password_change_denied", user.GUID, ip, map[string]interface{}{
+			"reason": "no_local_password",
+		})
+		jsonError(w, "no local password is set for this account — ask an administrator to set one", http.StatusForbidden)
+		return
+	}
+
+	// Proof of possession. ForcePasswordChange is the admin temp-password flow: the
+	// admin already wrote a REAL hash via PUT /api/admin/users/{guid}/password
+	// (admin.go writes user.PasswordHash immediately before setting the flag), the
+	// login response carried force_password_change:true, and the user now changes
+	// it without re-typing the temp password. That flow always runs with a
+	// NON-EMPTY hash, which is precisely why the empty-hash gate above can sit
+	// ahead of it.
+	if !user.ForcePasswordChange {
 		if req.CurrentPassword == "" {
 			jsonError(w, "current_password required", http.StatusBadRequest)
 			return

internal/handler/clientip_test.go

@@ -0,0 +1,81 @@
+package handler
+
+import (
+	"net"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+// withTrustedProxy makes 192.0.2.0/24 a trusted proxy range for one test.
+// trustedCIDRs is package state set at handler init; without this the forwarded
+// headers are never consulted and every assertion below would pass vacuously.
+func withTrustedProxy(t *testing.T) {
+	t.Helper()
+	_, cidr, err := net.ParseCIDR("192.0.2.0/24")
+	if err != nil {
+		t.Fatalf("parse cidr: %v", err)
+	}
+	prev := trustedCIDRs
+	trustedCIDRs = []*net.IPNet{cidr}
+	t.Cleanup(func() { trustedCIDRs = prev })
+}
+
+// TestGetClientIPRejectsNonIPForwardedValues pins the log-forgery fix.
+//
+// getClientIP's result is written into log lines and audit records across the
+// whole codebase. It used to return the X-Forwarded-For / X-Real-IP value
+// verbatim, so a client behind the trusted proxy could put arbitrary text —
+// including newlines — into an "IP" field and forge entries that look like
+// genuine security events. A value that is not an IP is meaningless in that
+// field anyway, so it must fall back to the real remote address.
+func TestGetClientIPRejectsNonIPForwardedValues(t *testing.T) {
+	withTrustedProxy(t)
+	cases := []struct {
+		name   string
+		header string
+		value  string
+		want   string
+	}{
+		{"forged log line via XFF", "X-Forwarded-For",
+			"1.2.3.4\n[admin] Password set guid=victim force_change=false ip=1.2.3.4", "192.0.2.1"},
+		{"forged log line via X-Real-IP", "X-Real-IP",
+			"9.9.9.9\r\n[auth] Local auth success user=\"root\"", "192.0.2.1"},
+		{"plain junk", "X-Forwarded-For", "not-an-ip", "192.0.2.1"},
+		{"empty after trim", "X-Forwarded-For", "   ", "192.0.2.1"},
+		// A real IP must still be honoured — the fix must not break proxy support.
+		{"legitimate IPv4", "X-Forwarded-For", "203.0.113.7", "203.0.113.7"},
+		{"legitimate IPv4 with padding", "X-Forwarded-For", "  203.0.113.7 , 10.0.0.1", "203.0.113.7"},
+		{"legitimate IPv6", "X-Real-IP", "2001:db8::1", "2001:db8::1"},
+		// Canonicalised by net.IP.String(), so the same client correlates across
+		// entries — and, crucially, the returned string is built by the stdlib
+		// rather than sliced out of the header.
+		{"IPv6 non-canonical spelling", "X-Real-IP", "2001:0db8:0000::0001", "2001:db8::1"},
+	}
+	for _, tc := range cases {
+		req := httptest.NewRequest("GET", "/", nil)
+		req.RemoteAddr = "192.0.2.1:1234" // httptest's default, inside the trusted range
+		req.Header.Set(tc.header, tc.value)
+
+		got := getClientIP(req)
+		if got != tc.want {
+			t.Errorf("%s: getClientIP = %q, want %q", tc.name, got, tc.want)
+		}
+		if strings.ContainsAny(got, "\r\n") {
+			t.Errorf("%s: client IP carried a newline into a log field: %q", tc.name, got)
+		}
+	}
+}
+
+// TestGetClientIPIgnoresUntrustedProxy guards the pre-existing trust gate: the
+// forwarded headers are only consulted for a connection from a trusted proxy.
+func TestGetClientIPIgnoresUntrustedProxy(t *testing.T) {
+	withTrustedProxy(t)
+	req := httptest.NewRequest("GET", "/", nil)
+	req.RemoteAddr = "203.0.113.99:5555" // not a trusted proxy
+	req.Header.Set("X-Forwarded-For", "198.51.100.1")
+
+	if got := getClientIP(req); got != "203.0.113.99" {
+		t.Fatalf("forwarded header honoured from an untrusted peer: got %q", got)
+	}
+}

internal/handler/hosted_login.go

@@ -327,8 +327,16 @@ func (h *Handler) handleLogout(w http.ResponseWriter, r *http.Request) {
 	// is signed out of every other app that shares this SimpleAuth.
 	h.deleteCurrentSession(w, r)
 
-	// Redirect to login with manual=1 to prevent auto-SSO on this page load only
+	// Redirect to login with manual=1 to prevent auto-SSO on this page load only.
+	// Carry client_id so the login page that follows resolves the SAME app the
+	// user just logged out of: GET /login rejects an unknown client_id with 400
+	// and validates redirect_uri against that app's OWN allowlist, so dropping it
+	// dead-ends the documented logout round-trip on a 400 for any app with its own
+	// redirect_uris. Same defect class as H13 / the client_id round-trip fixes.
 	u := h.url("/login") + "?manual=1"
+	if clientID := r.URL.Query().Get("client_id"); clientID != "" {
+		u += "&client_id=" + url.QueryEscape(clientID)
+	}
 	if redirectURI != "" {
 		u += "&redirect_uri=" + url.QueryEscape(redirectURI)
 	}

internal/handler/middleware.go

@@ -136,20 +136,34 @@ func (rl *rateLimiter) cleanup() {
 }
 
 // trustedCIDRs is set during handler initialization from config.TrustedProxyCIDRs.
-// If empty, forwarded headers are trusted from any source (backwards compatible).
+// If empty, forwarded headers are trusted from NO source (isTrustedProxy returns
+// false), which is what prevents X-Forwarded-For spoofing by default.
 var trustedCIDRs []*net.IPNet
 
 func getClientIP(r *http.Request) string {
 	remoteIP := extractIP(r.RemoteAddr)
 
 	// Only trust forwarded headers if the direct connection is from a trusted proxy
 	if isTrustedProxy(remoteIP, trustedCIDRs) {
+		// The forwarded value must actually PARSE AS AN IP. It is header content, so
+		// without this an attacker behind the trusted proxy can put arbitrary text —
+		// including newlines — into every log line and audit record that carries the
+		// client IP, forging entries that look like genuine events. An IP field
+		// holding a non-IP is meaningless anyway, so falling back to the real remote
+		// address is both safer and more truthful.
+		//
+		// Return net.IP.String(), not the header substring: the canonical form is a
+		// fresh string built by the stdlib from a parsed address, so no header text
+		// reaches a log or audit sink at all. It also normalises IPv6 spellings, so
+		// the same client correlates across entries.
 		if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
 			parts := strings.SplitN(xff, ",", 2)
-			return strings.TrimSpace(parts[0])
+			if ip := net.ParseIP(strings.TrimSpace(parts[0])); ip != nil {
+				return ip.String()
+			}
 		}
-		if xri := r.Header.Get("X-Real-IP"); xri != "" {
-			return xri
+		if ip := net.ParseIP(strings.TrimSpace(r.Header.Get("X-Real-IP"))); ip != nil {
+			return ip.String()
 		}
 	}