diff --git a/server/internal/api/oauth.go b/server/internal/api/oauth.go index 16449df0..7cbf3b34 100644 --- a/server/internal/api/oauth.go +++ b/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, "//") { diff --git a/server/internal/api/oauth_test.go b/server/internal/api/oauth_test.go index 2524fddc..a8bd6020 100644 --- a/server/internal/api/oauth_test.go +++ b/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 != "/" { - t.Fatalf("OAuth session cookie path = %q, want root path", cookies[0].Path) + if sessionCookie.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" - api.OAuthCallback(callbackRecorder, httptest.NewRequest(http.MethodGet, callbackURL, nil)) + state := authorizationURL.Query().Get("state") + 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) } diff --git a/server/internal/member/oauth/service.go b/server/internal/member/oauth/service.go index f6d2b4bc..8910125f 100644 --- a/server/internal/member/oauth/service.go +++ b/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) {