From ffc668d48e663f6952afb7230de3ee01391fe4f1 Mon Sep 17 00:00:00 2001
From: Kristoffer Dalby <kristoffer@dalby.cc>
Date: Sat, 17 Oct 2026 10:20:01 +0000
Subject: [PATCH] oidc: refuse logins whose subject the provider identifier
 would alter

---
 hscontrol/oidc.go             |  7 +++++++
 hscontrol/oidc_test.go        | 21 +++++++++++++++++++
 hscontrol/types/users.go      |  7 +++++++
 hscontrol/types/users_test.go | 39 +++++++++++++++++++++++++++++++++++
 4 files changed, 74 insertions(+)

diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go
index a1d0e3ee9..e79b9e4d7 100644
--- a/hscontrol/oidc.go
+++ b/hscontrol/oidc.go
@@ -370,6 +370,13 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler(
 		return
 	}
 
+	if !claims.IdentifierKeepsSubject() {
+		log.Warn().Str("subject", claims.Sub).Msg("OIDC subject would be altered by provider identifier cleaning")
+		httpUserError(writer, NewHTTPError(http.StatusForbidden, "OIDC subject cannot be used as a provider identifier", nil))
+
+		return
+	}
+
 	user, err := a.createOrUpdateUserFromClaim(&claims)
 	if err != nil {
 		httpUserError(writer, NewHTTPError(
diff --git a/hscontrol/oidc_test.go b/hscontrol/oidc_test.go
index 0b423f7d8..94372f9b9 100644
--- a/hscontrol/oidc_test.go
+++ b/hscontrol/oidc_test.go
@@ -1449,6 +1449,27 @@ func TestMergeEmailClaims(t *testing.T) {
 	}
 }
 
+func TestOIDCRefusesAlteredSubject(t *testing.T) {
+	b := newOIDCBrowser(t)
+	b.idp.QueueUser(&mockoidc.MockUser{Subject: "alice", PreferredUsername: "alice"})
+	_, registerURL := b.pendingNode(t)
+	status, _, _ := b.get(t, registerURL)
+	require.Equal(t, http.StatusOK, status)
+
+	before, err := b.app.state.GetUserByOIDCIdentifier(b.idp.Issuer() + "/alice")
+	require.NoError(t, err)
+
+	b.idp.QueueUser(&mockoidc.MockUser{Subject: "alice/", PreferredUsername: "renamed"})
+	_, registerURL = b.pendingNode(t)
+	status, _, _ = b.get(t, registerURL)
+	assert.Equal(t, http.StatusForbidden, status)
+
+	users, err := b.app.state.ListAllUsers()
+	require.NoError(t, err)
+	require.Len(t, users, 1)
+	assert.Equal(t, *before, users[0], "a refused subject must not update an existing row")
+}
+
 func TestOIDCRegistrationRejectsOversizedMergedHostinfo(t *testing.T) {
 	t.Parallel()
 
diff --git a/hscontrol/types/users.go b/hscontrol/types/users.go
index f44428c71..b8dbbc4be 100644
--- a/hscontrol/types/users.go
+++ b/hscontrol/types/users.go
@@ -354,6 +354,13 @@ func (c *OIDCClaims) Identifier() string {
 	return CleanIdentifier(result)
 }
 
+// IdentifierKeepsSubject reports whether the stored key preserves the subject.
+// Refusing altered subjects avoids changing existing provider identifiers.
+func (c *OIDCClaims) IdentifierKeepsSubject() bool {
+	return c.Identifier() == CleanIdentifier(c.Iss)+"/"+
+		(&url.URL{Path: c.Sub, RawPath: c.Sub}).EscapedPath()
+}
+
 // CleanIdentifier cleans a potentially malformed identifier by removing double slashes
 // while preserving protocol specifications like http://. This function will:
 // - Trim all whitespace from the beginning and end of the identifier
diff --git a/hscontrol/types/users_test.go b/hscontrol/types/users_test.go
index 1218979c6..80fb19237 100644
--- a/hscontrol/types/users_test.go
+++ b/hscontrol/types/users_test.go
@@ -552,3 +552,42 @@ func TestOIDCClaimsJSONToUser(t *testing.T) {
 		})
 	}
 }
+
+func TestOIDCClaimsIdentifierKeepsSubject(t *testing.T) {
+	kept := []string{
+		"c8872e9e-9a4f-4b8f-b111-c6191595d450",
+		"1234567890", "0123456789abcdef", "ab_C-dEF0123",
+		"ab/cd+ef==", "auth0|12345", "google-oauth2|12345",
+		"samlp|connection|alice@example.com", "f:realm:alice",
+		"alice@example.com", "\u7528\u6237", "alice smith", ".", "..",
+	}
+	refused := []string{
+		"", "/alice", "alice/", "alice//smith", " alice", "alice ",
+		"alice/ smith", "alice /smith", "alice#", "alice?x", "alice%23",
+		"alice%3Fx", "alice%2F", "alice%252F", "alice%20", "alice%2520",
+		"alice%zz", "a\x01b", "a\nb", "a\tb",
+	}
+
+	for _, issuer := range []string{
+		"https://sso.example.com",
+		"https://sso.example.com/realm/",
+		"https://login.microsoftonline.com/tenant/v2.0",
+	} {
+		t.Run(issuer, func(t *testing.T) {
+			identifiers := make(map[string]string)
+
+			for _, subject := range kept {
+				claims := OIDCClaims{Iss: issuer, Sub: subject}
+				assert.True(t, claims.IdentifierKeepsSubject(), "subject %q", subject)
+				identifier := claims.Identifier()
+				assert.NotContains(t, identifiers, identifier, "subject %q", subject)
+				identifiers[identifier] = subject
+			}
+
+			for _, subject := range refused {
+				claims := OIDCClaims{Iss: issuer, Sub: subject}
+				assert.False(t, claims.IdentifierKeepsSubject(), "subject %q", subject)
+			}
+		})
+	}
+}