security(oauth): encode social-login state instead of concatenating it

internal/http_handlers/oauth_callback.go

@@ -73,11 +73,23 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc {
 			ctx.JSON(400, gin.H{"error": "invalid oauth state"})
 			return
 		}
-		// `sessionState` is the oauth provider saved during `/oauth_login/:oauth_provider`.
-		// Ensure the callback route's provider matches what was originally requested.
-		if sessionState != provider {
+		// The flow's parameters are read from the store, NOT parsed out of the
+		// value the provider echoed back. They were validated by /oauth_login
+		// and never left this server, so nothing in transit can alter them.
+		// Fails closed on anything this server did not write — including an
+		// entry from a previous release, whose value was the bare provider name.
+		statePayload, err := unmarshalOAuthState(sessionState)
+		if err != nil {
+			log.Debug().Err(err).Msg("Failed to decode oauth state payload")
+			ctx.JSON(400, gin.H{"error": "invalid oauth state"})
+			return
+		}
+		// Ensure the callback route's provider matches what was originally
+		// requested, so a code obtained at one provider cannot be redeemed at
+		// another.
+		if statePayload.Provider != provider {
 			log.Debug().
-				Str("expected_provider", sessionState).
+				Str("expected_provider", statePayload.Provider).
 				Str("callback_provider", provider).
 				Msg("OAuth provider mismatch for state")
 			ctx.JSON(400, gin.H{"error": "invalid oauth state"})
@@ -100,26 +112,18 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc {
 		}
 		cookie.DeleteOAuthState(ctx, h.Config.AppCookieSecure)
 
-		// contains random token, redirect url, role
-		sessionSplit := strings.Split(state, "___")
-
-		if len(sessionSplit) < 4 {
-			log.Debug().Msg("Invalid state: expected at least 4 segments")
-			ctx.JSON(400, gin.H{"error": "invalid oauth state"})
-			return
-		}
 		// remove state from store
 		_ = h.MemoryStoreProvider.RemoveState(state)
-		stateValue := sessionSplit[0]
-		redirectURL := sessionSplit[1]
+		stateValue := statePayload.State
+		redirectURL := statePayload.RedirectURI
 		hostname := parsers.GetHost(ctx)
 		if !validators.IsValidRedirectURI(redirectURL, h.Config.AllowedOrigins, hostname) {
 			log.Debug().Msg("Invalid redirect URI in OAuth state")
 			ctx.JSON(400, gin.H{"error": "invalid redirect uri"})
 			return
 		}
-		inputRoles := strings.Split(sessionSplit[2], ",")
-		scopeString := sessionSplit[3]
+		inputRoles := strings.Split(statePayload.Roles, ",")
+		scopeString := statePayload.Scope
 		scopes := parseScopes(scopeString)
 		var user *schemas.User
 		// providerEmailVerified is the provider's own assertion that the

internal/http_handlers/oauth_login.go

@@ -83,8 +83,6 @@ func (h *httpProvider) OAuthLoginHandler() gin.HandlerFunc {
 			roles = strings.Join(h.Config.DefaultRoles, ",")
 		}
 
-		oauthStateString := state + "___" + redirectURI + "___" + roles + "___" + strings.Join(scope, " ")
-
 		provider := c.Param("oauth_provider")
 		log := log.With().Str("provider", provider).Logger()
 		cfg, err := h.OAuthProvider.GetOAuthConfig(c, provider)
@@ -95,11 +93,37 @@ func (h *httpProvider) OAuthLoginHandler() gin.HandlerFunc {
 			})
 			return
 		}
+		// The value sent to the provider is an opaque handle, not the flow's
+		// parameters. Nothing the caller supplied travels off this server, so
+		// there is no format for a caller's `state` to collide with on the way
+		// back — see internal/http_handlers/oauth_state.go.
+		oauthStateString, err := newOAuthStateHandle()
+		if err != nil {
+			log.Debug().Err(err).Msg("Error generating oauth state handle")
+			c.JSON(500, gin.H{
+				"error": "internal server error",
+			})
+			return
+		}
+		statePayload, err := marshalOAuthState(oauthStatePayload{
+			Provider:    provider,
+			State:       state,
+			RedirectURI: redirectURI,
+			Roles:       roles,
+			Scope:       strings.Join(scope, " "),
+		})
+		if err != nil {
+			log.Debug().Err(err).Msg("Error encoding oauth state")
+			c.JSON(500, gin.H{
+				"error": "internal server error",
+			})
+			return
+		}
 		// Bind this flow to the browser that started it (RFC 9700 §4.7). Set
 		// before the state is stored so a store failure cannot leave a usable
 		// cookie behind.
 		cookie.SetOAuthState(c, oauthStateString, h.Config.AppCookieSecure)
-		if err := h.MemoryStoreProvider.SetState(oauthStateString, provider); err != nil {
+		if err := h.MemoryStoreProvider.SetState(oauthStateString, statePayload); err != nil {
 			log.Debug().Err(err).Msg("Error setting state")
 			c.JSON(500, gin.H{
 				"error": "internal server error",

internal/http_handlers/oauth_state.go

@@ -0,0 +1,98 @@
+package http_handlers
+
+import (
+	"encoding/json"
+	"errors"
+
+	"github.com/authorizerdev/authorizer/internal/crypto"
+)
+
+// The social-login `state` used to carry four values through the provider round
+// trip, joined with "___" and split apart on the way back:
+//
+//	state + "___" + redirectURI + "___" + roles + "___" + scope
+//
+// The FIRST of them is supplied by the caller, so a caller whose state contained
+// the delimiter shifted every later field left. Sending
+// `state=A___https://evil.example___admin___openid` made the callback read its
+// redirect URI from the caller's segment and its ROLES from the next one — and
+// the callback checks roles only against ProtectedRoles, not the allowed-roles
+// list that /oauth_login enforces on the way in. The RFC 9700 browser binding
+// does not help: an attacker crafting this is attacking their own signup, so the
+// state is bound to their own browser.
+//
+// The benign face of the same bug was far more common. A caller's state is
+// typically base64url, whose alphabet includes "_", so a token that merely ENDED
+// in one produced "____", split a character early, and yielded a redirect URI
+// with a leading underscore — a hard "invalid redirect uri" on roughly 1 in 64
+// social logins, for every provider.
+//
+// The fix is not a better delimiter. NOTHING the caller controls travels to the
+// provider any more: the state is an opaque random handle, and the four values
+// live server-side in the state store, which already held an entry per login.
+// There is no format for a caller to collide with because there is nothing to
+// parse on the way back.
+//
+// Encoding the fields instead (base64 per field, joined by a character outside
+// the alphabet) would also have closed the injection, but it inflates the state
+// by ~28% — and X/Twitter documents a 100-character limit on `state`, which a
+// realistic redirect URI already approaches. A fixed-size handle is shorter than
+// what this server sent before, so no provider limit gets closer.
+//
+// It is also less to leak: the redirect URI, roles and scope no longer pass
+// through a third party at all.
+
+// oauthStateHandleBytes is the entropy behind the handle. 32 bytes is the same
+// budget as the PKCE verifier and yields a 43-character base64url value — well
+// inside every provider limit found, including X/Twitter's 100.
+const oauthStateHandleBytes = 32
+
+// errMalformedOAuthState means the store held something this server did not
+// write. Callers must refuse rather than guess: a lenient parse is exactly how
+// caller input became roles in the first place.
+var errMalformedOAuthState = errors.New("malformed oauth state")
+
+// oauthStatePayload is what the handle resolves to. It never leaves this server.
+type oauthStatePayload struct {
+	// Provider is the provider the flow was started for. The callback compares
+	// it against its own route parameter, so a code obtained at one provider
+	// cannot be redeemed at another.
+	Provider string `json:"provider"`
+	// State is the CALLER's opaque value, returned to them unchanged at the end
+	// of the flow. It is data here, never structure.
+	State string `json:"state"`
+	// RedirectURI, Roles and Scope were all validated by /oauth_login before
+	// this record was written, and nothing between here and the callback can
+	// alter them — they never travel to the provider.
+	RedirectURI string `json:"redirect_uri"`
+	Roles       string `json:"roles"`
+	Scope       string `json:"scope"`
+}
+
+// newOAuthStateHandle returns the opaque value sent to the provider as `state`.
+func newOAuthStateHandle() (string, error) {
+	return crypto.NewRandomString(oauthStateHandleBytes)
+}
+
+// marshalOAuthState serialises the payload for the state store.
+func marshalOAuthState(p oauthStatePayload) (string, error) {
+	b, err := json.Marshal(p)
+	if err != nil {
+		return "", err
+	}
+	return string(b), nil
+}
+
+// unmarshalOAuthState reads a payload back, failing closed on anything this
+// server did not write — including an entry left by a previous release, whose
+// value was the bare provider name rather than JSON.
+func unmarshalOAuthState(raw string) (oauthStatePayload, error) {
+	var p oauthStatePayload
+	if err := json.Unmarshal([]byte(raw), &p); err != nil {
+		return oauthStatePayload{}, errMalformedOAuthState
+	}
+	if p.Provider == "" {
+		return oauthStatePayload{}, errMalformedOAuthState
+	}
+	return p, nil
+}

internal/http_handlers/oauth_state_test.go

@@ -0,0 +1,126 @@
+package http_handlers
+
+import (
+	"encoding/base64"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestOAuthStateHandleCarriesNothingFromTheCaller is the regression guard for a
+// field-shifting bug in the old "___"-joined state.
+//
+// The caller supplied the FIRST field, so any delimiter it contained moved every
+// later field left — handing the callback its redirect URI and its ROLES from
+// caller input. The callback validates roles only against ProtectedRoles, so an
+// injected role that is merely non-protected was accepted.
+//
+// The property now is stronger than "the delimiter is escaped": the caller's
+// value never travels to the provider at all. The handle is opaque and the
+// parameters are read from the store, so each state below — every one of which
+// broke the old format — cannot influence anything.
+func TestOAuthStateHandleCarriesNothingFromTheCaller(t *testing.T) {
+	const (
+		provider    = "google"
+		redirectURI = "http://authorizer:8080/app"
+		roles       = "user"
+		scope       = "openid profile email"
+	)
+
+	for _, callerState := range []string{
+		"UlFosaMoufBG7UKydMfQJDscQHkYwZA71kGGQgFmN",
+		// ~1 in 64 base64url tokens ends in "_". Merged with the old "___" it
+		// produced "____", split a character early, and left the redirect URI
+		// as "_http://..." — a hard failure on every provider.
+		"UlFosaMoufBG7UKydMfQJDscQHkYwZA71kGGQgFmN_",
+		"abc___def",
+		// The malicious shape: forging every field that follows.
+		"A___https://evil.example/steal___admin___openid",
+		"______",
+		"",
+		"état.测试.🔐",
+		"a.b.c.d.e",
+	} {
+		t.Run(callerState, func(t *testing.T) {
+			handle, err := newOAuthStateHandle()
+			require.NoError(t, err)
+
+			// The handle is what the provider sees. It must contain nothing of
+			// the caller's, whatever they sent. (An empty state has no prefix to
+			// look for — there is nothing it could have leaked.)
+			if len(callerState) >= 8 {
+				assert.NotContains(t, handle, callerState[:8],
+					"the caller's value must not appear in the value sent to the provider")
+			}
+
+			raw, err := marshalOAuthState(oauthStatePayload{
+				Provider: provider, State: callerState,
+				RedirectURI: redirectURI, Roles: roles, Scope: scope,
+			})
+			require.NoError(t, err)
+
+			got, err := unmarshalOAuthState(raw)
+			require.NoError(t, err)
+
+			assert.Equal(t, callerState, got.State, "the caller's state must round-trip unchanged")
+			assert.Equal(t, redirectURI, got.RedirectURI,
+				"the redirect URI must come from the server, never from the caller")
+			assert.Equal(t, roles, got.Roles,
+				"roles must come from the server, never from the caller")
+			assert.Equal(t, scope, got.Scope)
+			assert.Equal(t, provider, got.Provider)
+		})
+	}
+}
+
+// TestOAuthStateHandleIsOpaqueAndShort pins the two properties that made a
+// handle preferable to encoding the fields inline.
+//
+// X/Twitter documents a 100-character limit on `state`, and a realistic
+// redirect URI already pushed the old concatenated form past it. Encoding each
+// field instead would have closed the injection but inflated the value by ~28%,
+// making that worse. A fixed-size handle is shorter than what this server sent
+// before, whatever the caller's state and redirect URI happen to be.
+func TestOAuthStateHandleIsOpaqueAndShort(t *testing.T) {
+	seen := map[string]bool{}
+	for range 200 {
+		h, err := newOAuthStateHandle()
+		require.NoError(t, err)
+
+		assert.LessOrEqual(t, len(h), 64, "must stay well inside the tightest provider limit (X/Twitter: 100)")
+		assert.False(t, seen[h], "handles must not repeat")
+		seen[h] = true
+
+		// URL-safe and unreserved (RFC 3986 §2.3), so no provider has to
+		// percent-encode it and hand back something subtly different.
+		for _, r := range h {
+			assert.True(t,
+				(r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') ||
+					(r >= '0' && r <= '9') || r == '-' || r == '_',
+				"unexpected character %q in handle", r)
+		}
+		_, err = base64.RawURLEncoding.DecodeString(h)
+		assert.NoError(t, err, "handle must be raw base64url")
+	}
+}
+
+// TestOAuthStateRejectsForeignEntries pins that the callback fails closed on a
+// store entry it did not write — including one left by the previous release,
+// whose value was the bare provider name rather than JSON. Guessing at such an
+// entry is precisely how caller input became privileges.
+func TestOAuthStateRejectsForeignEntries(t *testing.T) {
+	for _, tc := range []struct{ name, raw string }{
+		{"empty", ""},
+		{"previous release's bare provider name", "google"},
+		{"the old ___ format", "tok___http://x/app___user___openid"},
+		{"json without a provider", `{"state":"x","redirect_uri":"http://x/app"}`},
+		{"not json", "!!!not-json!!!"},
+		{"json array", `["google","x"]`},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := unmarshalOAuthState(tc.raw)
+			assert.ErrorIs(t, err, errMalformedOAuthState)
+		})
+	}
+}