oauth: bind state to browser session (#708)

* oauth: bind state to browser session

Bind each authorization state to a short-lived HttpOnly cookie so callback URLs cannot create sessions in browsers that did not initiate login.

Use per-state cookie names to preserve concurrent flows and reject callback origins that cannot receive the host-only cookie.

* oauth: preserve explicit proxy redirects

Keep browser-state binding usable when TLS terminates before Neko while still rejecting callback hosts that cannot receive the login cookie.

server/internal/api/oauth.go

@@ -1,17 +1,23 @@
 package api
 
 import (
+	"crypto/sha256"
+	"crypto/subtle"
+	"encoding/hex"
 	"errors"
 	"net/http"
 	"net/url"
 	"path"
 	"strings"
+	"time"
 
 	"github.com/m1k1o/neko/server/internal/member/oauth"
 	"github.com/m1k1o/neko/server/pkg/types"
 	"github.com/m1k1o/neko/server/pkg/utils"
 )
 
+const oauthStateCookiePrefix = "NEKO_OAUTH_STATE_"
+
 type OAuthUIConfig struct {
 	Enabled              bool   `json:"enabled"`
 	Name                 string `json:"name"`
@@ -68,6 +74,9 @@ func (api *ApiManagerCtx) OAuthLogin(w http.ResponseWriter, r *http.Request) err
 	if err != nil {
 		return oauthServiceError(err, true)
 	}
+	if err := api.oauth.setStateCookie(w, location, callbackURL); err != nil {
+		return utils.HttpInternalServerError("unable to initialize OAuth state").WithInternalErr(err)
+	}
 
 	http.Redirect(w, r, location, http.StatusFound)
 	return nil
@@ -78,11 +87,17 @@ func (api *ApiManagerCtx) OAuthCallback(w http.ResponseWriter, r *http.Request)
 		return utils.HttpNotFound()
 	}
 
+	stateToken := r.URL.Query().Get("state")
+	if !api.oauth.stateCookieMatches(r, stateToken) {
+		return utils.HttpBadRequest("OAuth state is invalid or expired")
+	}
+	api.oauth.clearStateCookie(w, stateToken)
+
 	if providerError := r.URL.Query().Get("error"); providerError != "" {
 		return utils.HttpUnauthorized("OAuth authorization was declined").WithInternalMsg(providerError)
 	}
 
-	_, token, err := api.oauth.service.Complete(r.Context(), r.URL.Query().Get("state"), r.URL.Query().Get("code"))
+	_, token, err := api.oauth.service.Complete(r.Context(), stateToken, r.URL.Query().Get("code"))
 	if err != nil {
 		return oauthServiceError(err, false)
 	}
@@ -92,6 +107,73 @@ func (api *ApiManagerCtx) OAuthCallback(w http.ResponseWriter, r *http.Request)
 	return nil
 }
 
+func (handler *oauthHandler) setStateCookie(w http.ResponseWriter, location, callbackURL string) error {
+	authorizationURL, err := url.Parse(location)
+	if err != nil {
+		return err
+	}
+
+	stateToken := authorizationURL.Query().Get("state")
+	redirectURL, err := url.Parse(authorizationURL.Query().Get("redirect_uri"))
+	if stateToken == "" || err != nil || !httpURL(redirectURL) {
+		return errors.New("OAuth authorization URL is missing a valid state or redirect_uri")
+	}
+
+	requestCallbackURL, err := url.Parse(callbackURL)
+	if err != nil || !sameCookieHost(redirectURL, requestCallbackURL) {
+		return errors.New("OAuth redirect_uri host does not match the login request host")
+	}
+
+	http.SetCookie(w, &http.Cookie{
+		Name:     oauthStateCookieName(stateToken),
+		Value:    stateToken,
+		Path:     "/",
+		MaxAge:   int(oauth.StateLifetime / time.Second),
+		Expires:  time.Now().Add(oauth.StateLifetime),
+		Secure:   redirectURL.Scheme == "https",
+		HttpOnly: true,
+		SameSite: http.SameSiteLaxMode,
+	})
+	return nil
+}
+
+func (handler *oauthHandler) stateCookieMatches(r *http.Request, stateToken string) bool {
+	if stateToken == "" {
+		return false
+	}
+
+	cookie, err := r.Cookie(oauthStateCookieName(stateToken))
+	if err != nil {
+		return false
+	}
+
+	return subtle.ConstantTimeCompare([]byte(cookie.Value), []byte(stateToken)) == 1
+}
+
+func (handler *oauthHandler) clearStateCookie(w http.ResponseWriter, stateToken string) {
+	http.SetCookie(w, &http.Cookie{
+		Name:     oauthStateCookieName(stateToken),
+		Path:     "/",
+		MaxAge:   -1,
+		Expires:  time.Unix(1, 0),
+		HttpOnly: true,
+		SameSite: http.SameSiteLaxMode,
+	})
+}
+
+func oauthStateCookieName(stateToken string) string {
+	digest := sha256.Sum256([]byte(stateToken))
+	return oauthStateCookiePrefix + hex.EncodeToString(digest[:16])
+}
+
+func httpURL(value *url.URL) bool {
+	return value != nil && value.Host != "" && (value.Scheme == "http" || value.Scheme == "https")
+}
+
+func sameCookieHost(left, right *url.URL) bool {
+	return httpURL(left) && httpURL(right) && strings.EqualFold(left.Hostname(), right.Hostname())
+}
+
 func oauthServiceError(err error, start bool) error {
 	if errors.Is(err, types.ErrSessionAlreadyConnected) {
 		return utils.HttpUnprocessableEntity("session already connected")
@@ -147,7 +229,7 @@ func (handler *oauthHandler) callbackURL(r *http.Request) (string, error) {
 	callback := (&url.URL{
 		Scheme: scheme,
 		Host:   host,
-		Path:   path.Join(handler.pathPrefix, "/api/oauth/callback"),
+		Path:   handler.callbackPath(),
 	}).String()
 	if parsed, err := url.Parse(callback); err != nil || parsed.Host == "" {
 		return "", errors.New("request has no valid public URL")
@@ -156,6 +238,10 @@ func (handler *oauthHandler) callbackURL(r *http.Request) (string, error) {
 	return callback, nil
 }
 
+func (handler *oauthHandler) callbackPath() string {
+	return path.Join(handler.pathPrefix, "/api/oauth/callback")
+}
+
 func (handler *oauthHandler) successRedirect() string {
 	redirect := handler.service.SuccessRedirect()
 	if strings.HasPrefix(redirect, "/") && !strings.HasPrefix(redirect, "//") {

server/internal/api/oauth_test.go

@@ -4,6 +4,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"net/http"
+	"net/http/cookiejar"
 	"net/http/httptest"
 	"net/url"
 	"testing"
@@ -82,7 +83,7 @@ func TestOAuthLoginSynchronizesProfile(t *testing.T) {
 	api := New(sessionManager, members, nil, nil, memberConfig, &config.Server{PathPrefix: "/"})
 
 	loginRecorder := httptest.NewRecorder()
-	api.OAuthLogin(loginRecorder, httptest.NewRequest(http.MethodGet, "/api/oauth/login", nil))
+	api.OAuthLogin(loginRecorder, httptest.NewRequest(http.MethodGet, "https://neko.example.test/api/oauth/login", nil))
 	if loginRecorder.Code != http.StatusFound {
 		t.Fatalf("login status = %d", loginRecorder.Code)
 	}
@@ -94,10 +95,18 @@ func TestOAuthLoginSynchronizesProfile(t *testing.T) {
 	if state == "" || authorizeURL.Query().Get("code_challenge") == "" {
 		t.Fatalf("OAuth redirect is missing state or PKCE challenge: %s", authorizeURL.String())
 	}
+	stateCookie := requireOAuthStateCookie(t, loginRecorder, state)
+	if stateCookie.Value != state {
+		t.Fatal("OAuth state cookie does not match authorization state")
+	}
 
 	callbackRecorder := httptest.NewRecorder()
 	callbackURL := "/api/oauth/callback?state=" + url.QueryEscape(state) + "&code=authorization-code"
-	api.OAuthCallback(callbackRecorder, httptest.NewRequest(http.MethodGet, callbackURL, nil))
+	callbackRequest := httptest.NewRequest(http.MethodGet, callbackURL, nil)
+	callbackRequest.AddCookie(stateCookie)
+	if err := api.OAuthCallback(callbackRecorder, callbackRequest); err != nil {
+		t.Fatal(err)
+	}
 	if callbackRecorder.Code != http.StatusSeeOther {
 		t.Fatalf("callback status = %d", callbackRecorder.Code)
 	}
@@ -108,11 +117,16 @@ func TestOAuthLoginSynchronizesProfile(t *testing.T) {
 		t.Fatal("token request is missing PKCE verifier")
 	}
 	cookies := callbackRecorder.Result().Cookies()
-	if len(cookies) != 1 {
+	sessionCookie := findCookie(cookies, "NEKO_SESSION")
+	if sessionCookie == nil {
 		t.Fatal("OAuth callback did not set a session cookie")
 	}
-	if cookies[0].Path != "/" {
+	if sessionCookie.Path != "/" {
-		t.Fatalf("OAuth session cookie path = %q, want root path", cookies[0].Path)
+		t.Fatalf("OAuth session cookie path = %q, want root path", sessionCookie.Path)
+	}
+	clearedStateCookie := findCookie(cookies, oauthStateCookieName(state))
+	if clearedStateCookie == nil || clearedStateCookie.MaxAge >= 0 {
+		t.Fatal("OAuth callback did not clear the state cookie")
 	}
 
 	userSession, ok := sessionManager.Get("oauth:user-123")
@@ -128,6 +142,235 @@ func TestOAuthLoginSynchronizesProfile(t *testing.T) {
 	}
 }
 
+func requireOAuthStateCookie(t *testing.T, recorder *httptest.ResponseRecorder, stateToken string) *http.Cookie {
+	t.Helper()
+
+	cookie := findCookie(recorder.Result().Cookies(), oauthStateCookieName(stateToken))
+	if cookie == nil {
+		t.Fatal("OAuth login did not set a state cookie")
+	}
+	if cookie.Path != "/" {
+		t.Fatalf("OAuth state cookie path = %q", cookie.Path)
+	}
+	if cookie.MaxAge != int(oauth.StateLifetime/time.Second) {
+		t.Fatalf("OAuth state cookie max age = %d", cookie.MaxAge)
+	}
+	if !cookie.Secure || !cookie.HttpOnly || cookie.SameSite != http.SameSiteLaxMode {
+		t.Fatalf("OAuth state cookie flags = Secure:%t HttpOnly:%t SameSite:%d", cookie.Secure, cookie.HttpOnly, cookie.SameSite)
+	}
+	return cookie
+}
+
+func findCookie(cookies []*http.Cookie, name string) *http.Cookie {
+	for _, cookie := range cookies {
+		if cookie.Name == name {
+			return cookie
+		}
+	}
+	return nil
+}
+
+func TestOAuthCallbackRequiresStateCookie(t *testing.T) {
+	tokenRequests := 0
+	provider := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/token" {
+			tokenRequests++
+		}
+		http.Error(w, "invalid request", http.StatusBadRequest)
+	}))
+	defer provider.Close()
+
+	memberConfig := &config.Member{
+		Provider: "oauth",
+		OAuth: oauth.Config{
+			Enabled:          true,
+			ClientID:         "client-id",
+			ClientSecret:     "client-secret",
+			AuthorizationURL: provider.URL + "/authorize",
+			TokenURL:         provider.URL + "/token",
+			UserInfoURL:      provider.URL + "/userinfo",
+			RedirectURL:      "https://neko.example.test/api/oauth/callback",
+			Scopes:           []string{"openid"},
+			SubjectField:     "sub",
+			UsernameField:    "name",
+			UserProfile:      types.MemberProfile{CanLogin: true},
+		},
+	}
+	sessionManager := session.New(&config.Session{
+		Cookie: config.SessionCookie{Enabled: true, Name: "NEKO_SESSION", Expiration: time.Hour},
+	})
+	members := member.New(sessionManager, memberConfig)
+	api := New(sessionManager, members, nil, nil, memberConfig, &config.Server{PathPrefix: "/"})
+
+	loginRecorder := httptest.NewRecorder()
+	if err := api.OAuthLogin(loginRecorder, httptest.NewRequest(http.MethodGet, "https://neko.example.test/api/oauth/login", nil)); err != nil {
+		t.Fatal(err)
+	}
+	authorizationURL, err := url.Parse(loginRecorder.Header().Get("Location"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	state := authorizationURL.Query().Get("state")
+	if state == "" {
+		t.Fatal("OAuth redirect is missing state")
+	}
+
+	callbackURL := "/api/oauth/callback?state=" + url.QueryEscape(state) + "&code=authorization-code"
+	callbackErr := api.OAuthCallback(httptest.NewRecorder(), httptest.NewRequest(http.MethodGet, callbackURL, nil))
+	if callbackErr == nil {
+		t.Fatal("callback without the initiating browser cookie succeeded")
+	}
+	if tokenRequests != 0 {
+		t.Fatalf("callback without the initiating browser cookie reached token endpoint %d times", tokenRequests)
+	}
+
+	wrongCookieRequest := httptest.NewRequest(http.MethodGet, callbackURL, nil)
+	wrongCookieRequest.AddCookie(&http.Cookie{Name: oauthStateCookieName(state), Value: "wrong-state"})
+	if err := api.OAuthCallback(httptest.NewRecorder(), wrongCookieRequest); err == nil {
+		t.Fatal("callback with a mismatched state cookie succeeded")
+	}
+	if tokenRequests != 0 {
+		t.Fatalf("callback with a mismatched state cookie reached token endpoint %d times", tokenRequests)
+	}
+
+	firstCookie := requireOAuthStateCookie(t, loginRecorder, state)
+	secondLoginRecorder := httptest.NewRecorder()
+	if err := api.OAuthLogin(secondLoginRecorder, httptest.NewRequest(http.MethodGet, "https://neko.example.test/api/oauth/login", nil)); err != nil {
+		t.Fatal(err)
+	}
+	secondAuthorizationURL, err := url.Parse(secondLoginRecorder.Header().Get("Location"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	secondState := secondAuthorizationURL.Query().Get("state")
+	secondCookie := requireOAuthStateCookie(t, secondLoginRecorder, secondState)
+	if firstCookie.Name == secondCookie.Name {
+		t.Fatal("concurrent OAuth logins use the same state cookie name")
+	}
+
+	concurrentRequest := httptest.NewRequest(http.MethodGet, "/", nil)
+	concurrentRequest.AddCookie(firstCookie)
+	concurrentRequest.AddCookie(secondCookie)
+	if !api.oauth.stateCookieMatches(concurrentRequest, state) || !api.oauth.stateCookieMatches(concurrentRequest, secondState) {
+		t.Fatal("concurrent OAuth state cookies are not independently valid")
+	}
+
+	declinedRecorder := httptest.NewRecorder()
+	declinedURL := "/api/oauth/callback?state=" + url.QueryEscape(secondState) + "&error=access_denied"
+	declinedRequest := httptest.NewRequest(http.MethodGet, declinedURL, nil)
+	declinedRequest.AddCookie(secondCookie)
+	if err := api.OAuthCallback(declinedRecorder, declinedRequest); err == nil {
+		t.Fatal("declined OAuth callback succeeded")
+	}
+	clearedCookie := findCookie(declinedRecorder.Result().Cookies(), secondCookie.Name)
+	if clearedCookie == nil || clearedCookie.MaxAge >= 0 {
+		t.Fatal("declined OAuth callback did not clear its state cookie")
+	}
+}
+
+func TestOAuthLoginUsesConfiguredRedirectBehindProxy(t *testing.T) {
+	memberConfig := &config.Member{
+		Provider: "oauth",
+		OAuth: oauth.Config{
+			Enabled:          true,
+			ClientID:         "client-id",
+			ClientSecret:     "client-secret",
+			AuthorizationURL: "https://auth.example.test/authorize",
+			TokenURL:         "https://auth.example.test/token",
+			UserInfoURL:      "https://auth.example.test/userinfo",
+			RedirectURL:      "https://neko.example.test/api/oauth/callback",
+			Scopes:           []string{"openid"},
+			SubjectField:     "sub",
+			UsernameField:    "name",
+			UserProfile:      types.MemberProfile{CanLogin: true},
+		},
+	}
+	sessionManager := session.New(&config.Session{
+		Cookie: config.SessionCookie{Enabled: true, Name: "NEKO_SESSION", Expiration: time.Hour},
+	})
+	members := member.New(sessionManager, memberConfig)
+	api := New(sessionManager, members, nil, nil, memberConfig, &config.Server{PathPrefix: "/"})
+
+	recorder := httptest.NewRecorder()
+	request := httptest.NewRequest(http.MethodGet, "http://neko.example.test/api/oauth/login", nil)
+	if err := api.OAuthLogin(recorder, request); err != nil {
+		t.Fatal(err)
+	}
+	if recorder.Code != http.StatusFound {
+		t.Fatalf("login status = %d", recorder.Code)
+	}
+
+	authorizationURL, err := url.Parse(recorder.Header().Get("Location"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := authorizationURL.Query().Get("redirect_uri"); got != memberConfig.OAuth.RedirectURL {
+		t.Fatalf("OAuth redirect_uri = %q", got)
+	}
+	state := authorizationURL.Query().Get("state")
+	cookie := requireOAuthStateCookie(t, recorder, state)
+	if !cookie.Secure {
+		t.Fatal("HTTPS OAuth callback requires a Secure state cookie")
+	}
+}
+
+func TestOAuthStateCookieValidatesRedirectHost(t *testing.T) {
+	handler := newOAuthHandler(nil, "/", false, true)
+	location := func(redirectURL string) string {
+		values := url.Values{"state": {"state-token"}, "redirect_uri": {redirectURL}}
+		return "https://auth.example.test/authorize?" + values.Encode()
+	}
+
+	mismatchRecorder := httptest.NewRecorder()
+	if err := handler.setStateCookie(
+		mismatchRecorder,
+		location("https://other.example.test/api/oauth/callback"),
+		"https://neko.example.test/api/oauth/callback",
+	); err == nil {
+		t.Fatal("OAuth redirect host mismatch was accepted")
+	}
+	if len(mismatchRecorder.Result().Cookies()) != 0 {
+		t.Fatal("OAuth redirect host mismatch set a state cookie")
+	}
+
+	publicPathRecorder := httptest.NewRecorder()
+	publicCallback := "https://neko.example.test/public/oauth/callback"
+	if err := handler.setStateCookie(
+		publicPathRecorder,
+		location(publicCallback),
+		"https://neko.example.test:443/api/oauth/callback",
+	); err != nil {
+		t.Fatal(err)
+	}
+	cookie := requireOAuthStateCookie(t, publicPathRecorder, "state-token")
+	if cookie.Path != "/" {
+		t.Fatalf("OAuth state cookie path = %q", cookie.Path)
+	}
+
+	jar, err := cookiejar.New(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	loginURL, _ := url.Parse("https://neko.example.test/api/oauth/login")
+	callbackURL, _ := url.Parse(publicCallback)
+	jar.SetCookies(loginURL, publicPathRecorder.Result().Cookies())
+	if findCookie(jar.Cookies(callbackURL), cookie.Name) == nil {
+		t.Fatal("browser cookie jar did not send the state cookie to the configured callback path")
+	}
+
+	httpRecorder := httptest.NewRecorder()
+	if err := handler.setStateCookie(
+		httpRecorder,
+		location("http://neko.example.test/api/oauth/callback"),
+		"http://neko.example.test/api/oauth/callback",
+	); err != nil {
+		t.Fatal(err)
+	}
+	if cookie := findCookie(httpRecorder.Result().Cookies(), oauthStateCookieName("state-token")); cookie == nil || cookie.Secure {
+		t.Fatal("HTTP OAuth state cookie must not be Secure")
+	}
+}
+
 func testIDToken(t *testing.T, claims map[string]string) string {
 	t.Helper()
 	payload, err := json.Marshal(claims)
@@ -212,8 +455,13 @@ func TestOAuthIssuerDiscoveryTakesPrecedence(t *testing.T) {
 	}
 
 	callbackRecorder := httptest.NewRecorder()
-	callbackURL := "/api/oauth/callback?state=" + url.QueryEscape(authorizationURL.Query().Get("state")) + "&code=authorization-code"
+	state := authorizationURL.Query().Get("state")
-	api.OAuthCallback(callbackRecorder, httptest.NewRequest(http.MethodGet, callbackURL, nil))
+	callbackURL := "/api/oauth/callback?state=" + url.QueryEscape(state) + "&code=authorization-code"
+	callbackRequest := httptest.NewRequest(http.MethodGet, callbackURL, nil)
+	callbackRequest.AddCookie(requireOAuthStateCookie(t, loginRecorder, state))
+	if err := api.OAuthCallback(callbackRecorder, callbackRequest); err != nil {
+		t.Fatal(err)
+	}
 	if callbackRecorder.Code != http.StatusSeeOther {
 		t.Fatalf("callback status = %d", callbackRecorder.Code)
 	}

server/internal/member/oauth/service.go

@@ -18,7 +18,7 @@ import (
 	"github.com/m1k1o/neko/server/pkg/types"
 )
 
-const stateLifetime = 10 * time.Minute
+const StateLifetime = 10 * time.Minute
 
 type state struct {
 	verifier, redirectURL string
@@ -364,7 +364,7 @@ func (service *Service) storeState(value, verifier, redirectURL string) {
 		}
 	}
 
-	service.states[value] = state{verifier, redirectURL, now.Add(stateLifetime)}
+	service.states[value] = state{verifier, redirectURL, now.Add(StateLifetime)}
 }
 
 func (service *Service) popState(value string) (state, bool) {