From 8f5873afca2e478dc4f840c13631d8339050c4e5 Mon Sep 17 00:00:00 2001
From: mrhid6 <mrhid6@gmail.com>
Date: Mon, 3 Aug 2026 10:41:30 +0100
Subject: [PATCH] refactor: carry provider id in the OIDC state token

---
 server/internal/auth/oidc.go    |  5 +--
 server/internal/auth/session.go | 12 -------
 server/internal/auth/state.go   | 56 +++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 14 deletions(-)
 create mode 100644 server/internal/auth/state.go

diff --git a/server/internal/auth/oidc.go b/server/internal/auth/oidc.go
index 9872810..fd64f8c 100644
--- a/server/internal/auth/oidc.go
+++ b/server/internal/auth/oidc.go
@@ -84,7 +84,7 @@ func HandleOIDCStart(c *gin.Context) {
 		c.JSON(http.StatusInternalServerError, gin.H{"error": "state gen failed"})
 		return
 	}
-	if err := SaveStateInstance(ctx, state, inst.InstanceID); err != nil {
+	if err := saveState(ctx, state, oidcState{InstanceID: inst.InstanceID, ProviderID: ""}); err != nil {
 		c.JSON(http.StatusInternalServerError, gin.H{"error": "state save failed"})
 		return
 	}
@@ -93,11 +93,12 @@ func HandleOIDCStart(c *gin.Context) {
 
 func HandleOIDCCallback(c *gin.Context) {
 	ctx := c.Request.Context()
-	instanceID, ok := ConsumeStateInstance(ctx, c.Query("state"))
+	st, ok := consumeState(ctx, c.Query("state"))
 	if !ok {
 		c.JSON(http.StatusBadRequest, gin.H{"error": "invalid state"})
 		return
 	}
+	instanceID := st.InstanceID
 
 	// The start handler checks this too, but an ungated callback is the half
 	// that matters: a start that refuses is a dead end, while a callback that
diff --git a/server/internal/auth/session.go b/server/internal/auth/session.go
index c7146ec..1959731 100644
--- a/server/internal/auth/session.go
+++ b/server/internal/auth/session.go
@@ -89,15 +89,3 @@ func GetSession(ctx context.Context, id string) (*Session, error) {
 func DeleteSession(ctx context.Context, id string) error {
 	return rdb.Del(ctx, sessionPrefix+id).Err()
 }
-
-func SaveStateInstance(ctx context.Context, state, instanceID string) error {
-	return rdb.Set(ctx, statePrefix+state, instanceID, 10*time.Minute).Err()
-}
-
-func ConsumeStateInstance(ctx context.Context, state string) (string, bool) {
-	instanceID, err := rdb.GetDel(ctx, statePrefix+state).Result()
-	if err != nil || instanceID == "" {
-		return "", false
-	}
-	return instanceID, true
-}
diff --git a/server/internal/auth/state.go b/server/internal/auth/state.go
new file mode 100644
index 0000000..1173928
--- /dev/null
+++ b/server/internal/auth/state.go
@@ -0,0 +1,56 @@
+package auth
+
+import (
+	"context"
+	"encoding/json"
+	"time"
+)
+
+// oidcState is what a login flow parks in Redis between the start redirect and
+// the callback. It carries the provider as well as the instance: the callback's
+// :providerId path segment is attacker-controlled, and this is the half that
+// was issued by the start handler.
+type oidcState struct {
+	InstanceID string `json:"instance_id"`
+	ProviderID string `json:"provider_id"`
+}
+
+func encodeState(s oidcState) (string, error) {
+	b, err := json.Marshal(s)
+	if err != nil {
+		return "", err
+	}
+	return string(b), nil
+}
+
+func decodeState(raw string) (oidcState, bool) {
+	var s oidcState
+	if raw == "" {
+		return oidcState{}, false
+	}
+	if err := json.Unmarshal([]byte(raw), &s); err != nil {
+		return oidcState{}, false
+	}
+	if s.InstanceID == "" || s.ProviderID == "" {
+		return oidcState{}, false
+	}
+	return s, true
+}
+
+func saveState(ctx context.Context, state string, s oidcState) error {
+	raw, err := encodeState(s)
+	if err != nil {
+		return err
+	}
+	return rdb.Set(ctx, statePrefix+state, raw, 10*time.Minute).Err()
+}
+
+// consumeState is GetDel: a state is single-use, so a replayed callback finds
+// nothing and is refused.
+func consumeState(ctx context.Context, state string) (oidcState, bool) {
+	raw, err := rdb.GetDel(ctx, statePrefix+state).Result()
+	if err != nil {
+		return oidcState{}, false
+	}
+	return decodeState(raw)
+}