From 7d6b215db8703fcde0e56cc6d1a35b5ce75562e0 Mon Sep 17 00:00:00 2001 From: karitham Date: Mon, 24 Aug 2026 23:10:48 +0200 Subject: [PATCH 1/2] oauth: fix csrf & pkce verifier leak --- cmd/main.go | 28 ++- oauth/oauth2.go | 256 +++++++++++++++--------- oauth/oauth2_test.go | 394 +++++++++++++++++++++++++++++++++++++ oauth/service.go | 2 +- service/spotify/spotify.go | 18 +- 5 files changed, 582 insertions(+), 116 deletions(-) create mode 100644 oauth/oauth2_test.go diff --git a/cmd/main.go b/cmd/main.go index 002ddf1..118306f 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -1,6 +1,7 @@ package main import ( + "context" "encoding/json" "fmt" "log" @@ -8,18 +9,20 @@ import ( "strings" "time" - "github.com/teal-fm/piper/service/applemusic" - "github.com/teal-fm/piper/service/lastfm" - "github.com/teal-fm/piper/service/playingnow" - "github.com/spf13/viper" + "golang.org/x/oauth2" + spotifyOauth "golang.org/x/oauth2/spotify" + "github.com/teal-fm/piper/config" "github.com/teal-fm/piper/db" "github.com/teal-fm/piper/oauth" "github.com/teal-fm/piper/oauth/atproto" "github.com/teal-fm/piper/pages" apikeyService "github.com/teal-fm/piper/service/apikey" + "github.com/teal-fm/piper/service/applemusic" + "github.com/teal-fm/piper/service/lastfm" "github.com/teal-fm/piper/service/musicbrainz" + "github.com/teal-fm/piper/service/playingnow" "github.com/teal-fm/piper/service/spotify" "github.com/teal-fm/piper/session" ) @@ -180,12 +183,19 @@ func main() { // Register Spotify OAuth service only if Spotify is enabled and configured if spotifyService != nil { spotifyOAuth := oauth.NewOAuth2Service( - viper.GetString("spotify.client_id"), - viper.GetString("spotify.client_secret"), - viper.GetString("callback.spotify"), - viper.GetStringSlice("spotify.scopes"), - "spotify", + oauth2.Config{ + ClientID: viper.GetString("spotify.client_id"), + ClientSecret: viper.GetString("spotify.client_secret"), + RedirectURL: viper.GetString("callback.spotify"), + Scopes: viper.GetStringSlice("spotify.scopes"), + Endpoint: spotifyOauth.Endpoint, + }, spotifyService, + log.Default(), + func(ctx context.Context) int64 { + id, _ := session.GetUserID(ctx) + return id + }, ) oauthManager.RegisterService("spotify", spotifyOAuth) log.Println("Spotify OAuth service registered") diff --git a/oauth/oauth2.go b/oauth/oauth2.go index 8eb214d..35bd663 100644 --- a/oauth/oauth2.go +++ b/oauth/oauth2.go @@ -9,156 +9,222 @@ import ( "fmt" "log" "net/http" - "strings" + "sync" + "time" - "github.com/teal-fm/piper/session" "golang.org/x/oauth2" - "golang.org/x/oauth2/spotify" ) +type userIDGetter func(context.Context) int64 + +// Service implements the PKCE authorization-code flow with single-use CSRF +// states. It is safe for concurrent use. type Service struct { config oauth2.Config - state string - codeVerifier string - codeChallenge string tokenReceiver TokenReceiver + store *memoryStateStore + logger *log.Logger + getUserID userIDGetter } -func GenerateRandomState() string { - b := make([]byte, 16) - //This probably should panic - rand.Read(b) - return base64.URLEncoding.EncodeToString(b) +type stateEntry struct { + verifier string + expiresAt time.Time } -func NewOAuth2Service(clientID, clientSecret, redirectURI string, scopes []string, provider string, tokenReceiver TokenReceiver) *Service { - var endpoint oauth2.Endpoint +type memoryStateStore struct { + mu sync.Mutex + entries map[string]stateEntry +} - switch strings.ToLower(provider) { - case "spotify": - endpoint = spotify.Endpoint - default: - // placeholder - log.Printf("Warning: OAuth2 provider '%s' not explicitly configured. Using placeholder endpoints.", provider) - endpoint = oauth2.Endpoint{ - AuthURL: "https://example.com/auth", - TokenURL: "https://example.com/token", +func newMemoryStateStore() *memoryStateStore { + return &memoryStateStore{ + entries: make(map[string]stateEntry), + } +} + +func (s *memoryStateStore) Set(state, verifier string, ttl time.Duration) { + s.mu.Lock() + defer s.mu.Unlock() + + now := time.Now() + for k, v := range s.entries { + if now.After(v.expiresAt) { + delete(s.entries, k) } } - codeVerifier := GenerateCodeVerifier() - codeChallenge := GenerateCodeChallenge(codeVerifier) + s.entries[state] = stateEntry{verifier: verifier, expiresAt: now.Add(ttl)} +} + +func (s *memoryStateStore) GetAndDelete(state string) (string, bool) { + s.mu.Lock() + defer s.mu.Unlock() + + e, ok := s.entries[state] + if !ok { + return "", false + } + + delete(s.entries, state) + if time.Now().After(e.expiresAt) { + return "", false + } - return &Service{ - config: oauth2.Config{ - ClientID: clientID, - ClientSecret: clientSecret, - RedirectURL: redirectURI, - Scopes: scopes, - Endpoint: endpoint, - }, - state: GenerateRandomState(), - codeVerifier: codeVerifier, - codeChallenge: codeChallenge, - tokenReceiver: tokenReceiver, + return e.verifier, true +} + +const randAlphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567" + +func randText(n int) string { + b := make([]byte, n) + if _, err := rand.Read(b); err != nil { + panic(fmt.Sprintf("rand.Read: %v", err)) } + + for i := range b { + b[i] = randAlphabet[int(b[i])%len(randAlphabet)] + } + + return string(b) } -// GenerateCodeVerifier generate a random code verifier, for PKCE -func GenerateCodeVerifier() string { - b := make([]byte, 64) - //This probably should panic - rand.Read(b) - return base64.RawURLEncoding.EncodeToString(b) +func NewOAuth2Service(cfg oauth2.Config, tokenReceiver TokenReceiver, logger *log.Logger, getUserID userIDGetter) *Service { + return &Service{ + config: cfg, + tokenReceiver: tokenReceiver, + store: newMemoryStateStore(), + logger: logger, + getUserID: getUserID, + } } -// GenerateCodeChallenge generate a code challenge for verification later -func GenerateCodeChallenge(verifier string) string { +func generateCodeChallenge(verifier string) string { h := sha256.New() h.Write([]byte(verifier)) + return base64.RawURLEncoding.EncodeToString(h.Sum(nil)) } +// HandleLogin redirects to the provider's authorization endpoint with a +// single-use state and S256 PKCE challenge. func (o *Service) HandleLogin(w http.ResponseWriter, r *http.Request) { + state := randText(26) + verifier := randText(64) + challenge := generateCodeChallenge(verifier) + + o.store.Set(state, verifier, 10*time.Minute) + opts := []oauth2.AuthCodeOption{ - oauth2.SetAuthURLParam("code_challenge", o.codeChallenge), + oauth2.SetAuthURLParam("code_challenge", challenge), oauth2.SetAuthURLParam("code_challenge_method", "S256"), } - authURL := o.config.AuthCodeURL(o.state, opts...) + + authURL := o.config.AuthCodeURL(state, opts...) http.Redirect(w, r, authURL, http.StatusSeeOther) } func (o *Service) HandleLogout(w http.ResponseWriter, r *http.Request) { - //TODO not implemented yet. not sure what the api call is for this package + // TODO not implemented yet. not sure what the api call is for this package http.Redirect(w, r, "/", http.StatusSeeOther) } +type completeLoginParams struct { + State string + Code string + ProviderError string + ProviderDesc string + UserID int64 +} + +// HandleCallback handles the provider redirect. It requires query parameters +// state and code (or error/error_description on provider denial). +// +// On success it returns the authenticated user ID. On failure it writes a +// generic error (never reflecting provider strings) with 400 for client +// errors and 500 for server errors and returns a sentinel for errors.Is. func (o *Service) HandleCallback(w http.ResponseWriter, r *http.Request) (int64, error) { - state := r.URL.Query().Get("state") - if state != o.state { - log.Printf("OAuth2 Callback Error: State mismatch. Expected '%s', got '%s'", o.state, state) - http.Error(w, "State mismatch", http.StatusBadRequest) - return 0, errors.New("state mismatch") + params := completeLoginParams{ + State: r.URL.Query().Get("state"), + Code: r.URL.Query().Get("code"), + ProviderError: r.URL.Query().Get("error"), + ProviderDesc: r.URL.Query().Get("error_description"), + UserID: o.getUserID(r.Context()), } - code := r.URL.Query().Get("code") - if code == "" { - errMsg := r.URL.Query().Get("error") - errDesc := r.URL.Query().Get("error_description") - log.Printf("OAuth2 Callback Error: No code provided. Error: '%s', Description: '%s'", errMsg, errDesc) - http.Error(w, fmt.Sprintf("Authorization failed: %s (%s)", errMsg, errDesc), http.StatusBadRequest) - return 0, errors.New("no code provided") + userID, err := o.completeLogin(r.Context(), params) + if err != nil { + status := httpStatusForOAuthError(err) + http.Error(w, err.Error(), status) + return 0, err } - if o.tokenReceiver == nil { - log.Printf("OAuth2 Callback Error: TokenReceiver is not configured for this service.") - http.Error(w, "Internal server configuration error", http.StatusInternalServerError) - return 0, errors.New("token receiver not configured") - } + return userID, nil +} - opts := []oauth2.AuthCodeOption{ - oauth2.SetAuthURLParam("code_verifier", o.codeVerifier), +var ( + errStateMismatch = errors.New("state mismatch") + errNoCode = errors.New("no code provided") + errNoReceiver = errors.New("token receiver not configured") + errExchangeFailed = errors.New("failed to exchange code for token") +) + +func httpStatusForOAuthError(err error) int { + if errors.Is(err, errNoReceiver) || errors.Is(err, errExchangeFailed) { + return http.StatusInternalServerError } - log.Println(code) + return http.StatusBadRequest +} - token, err := o.config.Exchange(context.Background(), code, opts...) - if err != nil { - log.Printf("OAuth2 Callback Error: Failed to exchange code for token: %v", err) - http.Error(w, fmt.Sprintf("Error exchanging code for token: %v", err), http.StatusInternalServerError) - return 0, errors.New("failed to exchange code for token") +// State is consumed before any other check so a missing code still +// invalidates the entry and provider errors are only logged for a valid +// state, preventing CSRF bypass via forged error. +func (o *Service) completeLogin(ctx context.Context, p completeLoginParams) (int64, error) { + verifier, ok := o.store.GetAndDelete(p.State) + if !ok { + o.logger.Printf("OAuth2 Callback Error: State mismatch or expired. Got '%s'", p.State) + return 0, errStateMismatch } - userId, hasSession := session.GetUserID(r.Context()) - // store token and get uid - userID, err := o.tokenReceiver.SetAccessToken(token.AccessToken, token.RefreshToken, userId, hasSession) - if err != nil { - log.Printf("OAuth2 Callback Info: TokenReceiver did not return a valid user ID for token: %s...", token.AccessToken[:customMin(10, len(token.AccessToken))]) + if p.Code == "" { + if p.ProviderError != "" || p.ProviderDesc != "" { + o.logger.Printf("OAuth2 Callback Error: provider returned error for state '%s': error=%q desc=%q", p.State, p.ProviderError, p.ProviderDesc) + } else { + o.logger.Printf("OAuth2 Callback Error: No code provided for state '%s'", p.State) + } + return 0, errNoCode } - log.Printf("OAuth2 Callback Success: Exchanged code for token, UserID: %d", userID) - return userID, nil + if o.tokenReceiver == nil { + o.logger.Printf("OAuth2 Callback Error: TokenReceiver is not configured") + return 0, errNoReceiver + } + + return o.exchangeAndStore(ctx, p.Code, verifier, p.UserID) } -func (o *Service) GetToken(code string) (*oauth2.Token, error) { +func (o *Service) exchangeAndStore(ctx context.Context, code, verifier string, uid int64) (int64, error) { opts := []oauth2.AuthCodeOption{ - oauth2.SetAuthURLParam("code_verifier", o.codeVerifier), + oauth2.SetAuthURLParam("code_verifier", verifier), } - return o.config.Exchange(context.Background(), code, opts...) -} -func (o *Service) GetClient(token *oauth2.Token) *http.Client { - return o.config.Client(context.Background(), token) -} - -func (o *Service) RefreshToken(token *oauth2.Token) (*oauth2.Token, error) { - source := o.config.TokenSource(context.Background(), token) - return oauth2.ReuseTokenSource(token, source).Token() -} + token, err := o.config.Exchange(ctx, code, opts...) + if err != nil { + o.logger.Printf("OAuth2 Callback Error: Failed to exchange code for token: %v", err) + return 0, errExchangeFailed + } -func customMin(a, b int) int { - if a < b { - return a + userID, err := o.tokenReceiver.SetAccessToken(token.AccessToken, token.RefreshToken, uid) + if err != nil { + o.logger.Printf( + "OAuth2 Callback Info: TokenReceiver failed for token %q...: %v", + token.AccessToken[:min(len(token.AccessToken), 10)], + err, + ) } - return b + + o.logger.Printf("OAuth2 Callback Success: Exchanged code for token, UserID: %d", userID) + + return userID, nil } diff --git a/oauth/oauth2_test.go b/oauth/oauth2_test.go new file mode 100644 index 0000000..76e17b2 --- /dev/null +++ b/oauth/oauth2_test.go @@ -0,0 +1,394 @@ +package oauth + +import ( + "context" + "io" + "log" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "golang.org/x/oauth2" +) + +type mockReceiver struct { + returnID int64 + returnErr error + called bool + lastUID int64 +} + +func (m *mockReceiver) SetAccessToken(token, refresh string, uid int64) (int64, error) { + m.called = true + m.lastUID = uid + + if m.returnErr != nil { + return 0, m.returnErr + } + + if m.returnID != 0 { + return m.returnID, nil + } + + return 42, nil +} + +func noUser(context.Context) int64 { return 0 } + +func stateFromLogin(svc *Service) string { + req := httptest.NewRequest(http.MethodGet, "/login", nil) + rr := httptest.NewRecorder() + svc.HandleLogin(rr, req) + loc := rr.Result().Header.Get("Location") + u, _ := url.Parse(loc) + return u.Query().Get("state") +} + +func forgedState(svc *Service) string { + _ = stateFromLogin(svc) + return "forged-state" +} + +func consumedState(svc *Service) string { + s := stateFromLogin(svc) + _, _ = svc.completeLogin(context.Background(), completeLoginParams{State: s, Code: "c"}) + return s +} + +func queryMissingCode(svc *Service) string { + s := stateFromLogin(svc) + return "state=" + url.QueryEscape(s) + "&error=access_denied" +} + +func queryValidCode(svc *Service) string { + s := stateFromLogin(svc) + return "state=" + url.QueryEscape(s) + "&code=c" +} + +func queryMismatch(_ *Service) string { + return "state=not-issued&code=c" +} + +func okTokenHandler(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"access_token":"at","token_type":"bearer","refresh_token":"rt"}`)) +} + +func errorTokenHandler(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(500) + _, _ = w.Write([]byte(`{"error":"server_error"}`)) +} + +func wantErr(substr string) func(t *testing.T, gotID int64, err error, r *mockReceiver) { + return func(t *testing.T, _ int64, err error, _ *mockReceiver) { + t.Helper() + if err == nil || !strings.Contains(err.Error(), substr) { + t.Fatalf("err = %v, want containing %q", err, substr) + } + } +} + +func wantID(want int64) func(t *testing.T, gotID int64, err error, r *mockReceiver) { + return func(t *testing.T, gotID int64, err error, _ *mockReceiver) { + t.Helper() + if err != nil { + t.Fatalf("unexpected err %v", err) + } + if gotID != want { + t.Fatalf("id = %d, want %d", gotID, want) + } + } +} + +func wantIDWithSession(wantID, wantUID int64) func(t *testing.T, gotID int64, err error, r *mockReceiver) { + return func(t *testing.T, gotID int64, err error, r *mockReceiver) { + t.Helper() + if err != nil { + t.Fatalf("unexpected err %v", err) + } + if gotID != wantID { + t.Fatalf("id = %d, want %d", gotID, wantID) + } + if r.lastUID != wantUID { + t.Fatalf("receiver = %+v, want uid %d", r, wantUID) + } + } +} + +func TestGenerateCodeChallenge(t *testing.T) { + tests := []struct { + name string + verifier string + want string + }{ + { + name: "rfc7636", + verifier: "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk", + want: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + }, + { + name: "empty", + verifier: "", + want: "47DEQpj8HBSa-_TImW-5JCeuQeRkm5NMpJWZG3hSuFU", + }, + { + name: "hello", + verifier: "hello", + want: "LPJNul-wow4m6DsqxbninhsWHlwfp0JecwQzYpOLmCQ", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := generateCodeChallenge(tt.verifier); got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} + +func TestHandleLogin_Redirect(t *testing.T) { + svc := NewOAuth2Service( + oauth2.Config{ + ClientID: "id", + Endpoint: oauth2.Endpoint{ + AuthURL: "http://example.com/auth", + TokenURL: "http://example.com/token", + }, + }, + nil, + log.New(io.Discard, "", 0), + noUser, + ) + + req := httptest.NewRequest(http.MethodGet, "http://example.com/login", nil) + rr := httptest.NewRecorder() + svc.HandleLogin(rr, req) + + resp := rr.Result() + if resp.StatusCode != http.StatusSeeOther { + t.Fatalf("status = %d, want %d", resp.StatusCode, http.StatusSeeOther) + } + + loc := resp.Header.Get("Location") + u, err := url.Parse(loc) + if err != nil { + t.Fatalf("invalid Location: %v", err) + } + q := u.Query() + + if q.Get("state") == "" { + t.Fatalf("missing state") + } + if q.Get("code_challenge") == "" { + t.Fatalf("missing code_challenge") + } + if q.Get("code_challenge_method") != "S256" { + t.Fatalf("method = %q, want S256", q.Get("code_challenge_method")) + } +} + +func TestHandleLogin_ChallengeBindsToStoredVerifier(t *testing.T) { + var capturedVerifier string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = r.ParseForm() + capturedVerifier = r.Form.Get("code_verifier") + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"access_token":"tok","token_type":"bearer"}`)) + })) + defer ts.Close() + + cfg := oauth2.Config{ + ClientID: "id", + ClientSecret: "s", + RedirectURL: "http://localhost/cb", + Endpoint: oauth2.Endpoint{ + AuthURL: "http://example.com/auth", + TokenURL: ts.URL, + }, + } + svc := NewOAuth2Service(cfg, &mockReceiver{returnID: 1}, log.New(io.Discard, "", 0), noUser) + + req := httptest.NewRequest(http.MethodGet, "/login", nil) + rr := httptest.NewRecorder() + svc.HandleLogin(rr, req) + loc := rr.Result().Header.Get("Location") + u, _ := url.Parse(loc) + state := u.Query().Get("state") + challenge := u.Query().Get("code_challenge") + + reqCB := httptest.NewRequest(http.MethodGet, "/callback?state="+url.QueryEscape(state)+"&code=c", nil) + rrCB := httptest.NewRecorder() + if _, err := svc.HandleCallback(rrCB, reqCB); err != nil { + t.Fatalf("HandleCallback: %v", err) + } + + if capturedVerifier == "" { + t.Fatalf("no code_verifier sent to token endpoint") + } + if got := generateCodeChallenge(capturedVerifier); got != challenge { + t.Fatalf("challenge %q != S256(verifier %q) %q", challenge, capturedVerifier, got) + } +} + +func TestCompleteLogin(t *testing.T) { + tests := []struct { + name string + receiver *mockReceiver + tokenFunc http.HandlerFunc + setup func(svc *Service) string + code string + uid int64 + check func(t *testing.T, gotID int64, err error, receiver *mockReceiver) + }{ + { + name: "state mismatch", + receiver: &mockReceiver{}, + tokenFunc: okTokenHandler, + setup: forgedState, + code: "c", + check: wantErr("state mismatch"), + }, + { + name: "no code", + receiver: &mockReceiver{}, + tokenFunc: okTokenHandler, + setup: stateFromLogin, + code: "", + check: wantErr("no code provided"), + }, + { + name: "no receiver", + receiver: nil, + tokenFunc: okTokenHandler, + setup: stateFromLogin, + code: "c", + check: wantErr("token receiver not configured"), + }, + { + name: "exchange failure", + receiver: &mockReceiver{}, + tokenFunc: errorTokenHandler, + setup: stateFromLogin, + code: "c", + check: wantErr("failed to exchange"), + }, + { + name: "success", + receiver: &mockReceiver{returnID: 777}, + tokenFunc: okTokenHandler, + setup: stateFromLogin, + code: "c", + check: wantID(777), + }, + { + name: "success with session", + receiver: &mockReceiver{returnID: 99}, + tokenFunc: okTokenHandler, + setup: stateFromLogin, + code: "c", + uid: 555, + check: wantIDWithSession(99, 555), + }, + { + name: "replay single-use", + receiver: &mockReceiver{returnID: 1}, + tokenFunc: okTokenHandler, + setup: consumedState, + code: "c", + check: wantErr("state mismatch"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ts := httptest.NewServer(tt.tokenFunc) + defer ts.Close() + + var recv TokenReceiver + if tt.receiver != nil { + recv = tt.receiver + } + svc := NewOAuth2Service( + oauth2.Config{ + ClientID: "id", + ClientSecret: "s", + RedirectURL: "http://localhost/cb", + Endpoint: oauth2.Endpoint{ + AuthURL: "http://example.com/auth", + TokenURL: ts.URL, + }, + }, + recv, + log.New(io.Discard, "", 0), + noUser, + ) + state := tt.setup(svc) + + gotID, err := svc.completeLogin(t.Context(), completeLoginParams{State: state, Code: tt.code, UserID: tt.uid}) + tt.check(t, gotID, err, tt.receiver) + }) + } +} + +func TestHandleCallback_HTTPMapping(t *testing.T) { + tests := []struct { + name string + setup func(svc *Service) string + receiver TokenReceiver + wantStatus int + }{ + { + name: "missing code maps to 400", + setup: queryMissingCode, + receiver: &mockReceiver{}, + wantStatus: http.StatusBadRequest, + }, + { + name: "nil receiver maps to 500", + setup: queryValidCode, + receiver: nil, + wantStatus: http.StatusInternalServerError, + }, + { + name: "state mismatch maps to 400", + setup: queryMismatch, + receiver: &mockReceiver{}, + wantStatus: http.StatusBadRequest, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"access_token":"at","token_type":"bearer"}`)) + })) + defer ts.Close() + + svc := NewOAuth2Service( + oauth2.Config{ + ClientID: "id", + Endpoint: oauth2.Endpoint{ + AuthURL: "http://a", + TokenURL: ts.URL, + }, + }, + tt.receiver, + log.New(io.Discard, "", 0), + noUser, + ) + + query := tt.setup(svc) + req := httptest.NewRequest(http.MethodGet, "/callback?"+query, nil) + rr := httptest.NewRecorder() + _, _ = svc.HandleCallback(rr, req) + + resp := rr.Result() + if resp.StatusCode != tt.wantStatus { + t.Fatalf("status = %d, want %d", resp.StatusCode, tt.wantStatus) + } + }) + } +} diff --git a/oauth/service.go b/oauth/service.go index fff06aa..2dc930a 100644 --- a/oauth/service.go +++ b/oauth/service.go @@ -18,5 +18,5 @@ type AuthService interface { type TokenReceiver interface { // SetAccessToken stores the access token in the db // if there is a session, will associate the token with the session - SetAccessToken(token string, refreshToken string, currentId int64, hasSession bool) (int64, error) + SetAccessToken(token string, refreshToken string, userID int64) (int64, error) } diff --git a/service/spotify/spotify.go b/service/spotify/spotify.go index 8d25445..472a4f7 100644 --- a/service/spotify/spotify.go +++ b/service/spotify/spotify.go @@ -1,6 +1,7 @@ package spotify import ( + "context" "crypto/sha256" "encoding/base64" "encoding/json" @@ -15,12 +16,7 @@ import ( "sync" "time" - "context" // Added for context.Context - - // Added for atproto.RepoCreateRecord_Input - // Added for lexutil.LexiconTypeDecoder - // Added for xrpc.Client - "github.com/spf13/viper" // Added for teal.FeedPlay + "github.com/spf13/viper" "github.com/teal-fm/piper/db" "github.com/teal-fm/piper/models" atprotoauth "github.com/teal-fm/piper/oauth/atproto" @@ -106,8 +102,8 @@ func (s *Service) SubmitTrackToPDS(did string, mostRecentAtProtoSessionID string return atprotoservice.SubmitPlayToPDS(ctx, did, mostRecentAtProtoSessionID, track, s.atprotoAuthService) } -func (s *Service) SetAccessToken(token string, refreshToken string, userId int64, hasSession bool) (int64, error) { - userID, err := s.identifyAndStoreUser(token, refreshToken, userId, hasSession) +func (s *Service) SetAccessToken(token string, refreshToken string, userId int64) (int64, error) { + userID, err := s.identifyAndStoreUser(token, refreshToken, userId) if err != nil { s.logger.Printf("Error identifying and storing user: %v", err) return 0, err @@ -115,14 +111,14 @@ func (s *Service) SetAccessToken(token string, refreshToken string, userId int64 return userID, nil } -func (s *Service) identifyAndStoreUser(token string, refreshToken string, userId int64, hasSession bool) (int64, error) { +func (s *Service) identifyAndStoreUser(token string, refreshToken string, userId int64) (int64, error) { userProfile, err := s.fetchSpotifyProfile(token) if err != nil { s.logger.Printf("Error fetching Spotify profile: %v", err) return 0, err } - s.logger.Printf("uid: %d hasSession: %t", userId, hasSession) + s.logger.Printf("uid: %d", userId) user, err := s.DB.GetUserBySpotifyID(userProfile.ID) if err != nil { @@ -135,7 +131,7 @@ func (s *Service) identifyAndStoreUser(token string, refreshToken string, userId // We don't intend users to log in via spotify! if user == nil { - if !hasSession { + if userId == 0 { s.logger.Printf("User does not seem to exist") return 0, fmt.Errorf("user does not seem to exist") } From 0c4a12d8a6cc9955af56bdf5a64f25eeb66a9748 Mon Sep 17 00:00:00 2001 From: karitham Date: Tue, 25 Aug 2026 01:21:27 +0200 Subject: [PATCH 2/2] oauth/oauth2: require auth to validate oauth2 pkce code exchange --- cmd/routes.go | 6 +- oauth/oauth2.go | 39 ++++++++---- oauth/oauth2_test.go | 137 +++++++++++++++++++++++++++++++++++++++---- session/session.go | 4 +- 4 files changed, 158 insertions(+), 28 deletions(-) diff --git a/cmd/routes.go b/cmd/routes.go index eda0246..2a58bad 100644 --- a/cmd/routes.go +++ b/cmd/routes.go @@ -17,8 +17,6 @@ func (app *application) routes() http.Handler { mux.HandleFunc("/", session.WithPossibleAuth(home(app.database, app.pages), app.sessionManager)) // OAuth Routes - mux.HandleFunc("/login/spotify", app.oauthManager.HandleLogin("spotify")) - mux.HandleFunc("/callback/spotify", session.WithPossibleAuth(app.oauthManager.HandleCallback("spotify"), app.sessionManager)) // Use possible auth mux.HandleFunc("/login/atproto", app.oauthManager.HandleLogin("atproto")) mux.HandleFunc("/callback/atproto", session.WithPossibleAuth(app.oauthManager.HandleCallback("atproto"), app.sessionManager)) // Use possible auth @@ -26,6 +24,10 @@ func (app *application) routes() http.Handler { mux.HandleFunc("/current-track", session.WithAuth(app.spotifyService.HandleCurrentTrack, app.sessionManager)) mux.HandleFunc("/history", session.WithAuth(app.spotifyService.HandleTrackHistory, app.sessionManager)) mux.HandleFunc("/api-keys", session.WithAuth(app.apiKeyService.HandleAPIKeyManagement(app.database, app.pages), app.sessionManager)) + + mux.HandleFunc("/login/spotify", session.WithAuth(app.oauthManager.HandleLogin("spotify"), app.sessionManager)) + mux.HandleFunc("/callback/spotify", session.WithAuth(app.oauthManager.HandleCallback("spotify"), app.sessionManager)) + mux.HandleFunc("/link-lastfm", session.WithAuth(handleLinkLastfmForm(app.database, app.pages), app.sessionManager)) // GET form mux.HandleFunc("/link-lastfm/submit", session.WithAuth(handleLinkLastfmSubmit(app.database), app.sessionManager)) // POST submit - Changed route slightly mux.HandleFunc("/link-applemusic", session.WithAuth(handleAppleMusicLink(app.pages, app.appleMusicService), app.sessionManager)) diff --git a/oauth/oauth2.go b/oauth/oauth2.go index 35bd663..ae3b8a1 100644 --- a/oauth/oauth2.go +++ b/oauth/oauth2.go @@ -30,6 +30,7 @@ type Service struct { type stateEntry struct { verifier string expiresAt time.Time + userID int64 } type memoryStateStore struct { @@ -43,7 +44,7 @@ func newMemoryStateStore() *memoryStateStore { } } -func (s *memoryStateStore) Set(state, verifier string, ttl time.Duration) { +func (s *memoryStateStore) Set(state, verifier string, userID int64, ttl time.Duration) { s.mu.Lock() defer s.mu.Unlock() @@ -54,24 +55,24 @@ func (s *memoryStateStore) Set(state, verifier string, ttl time.Duration) { } } - s.entries[state] = stateEntry{verifier: verifier, expiresAt: now.Add(ttl)} + s.entries[state] = stateEntry{verifier: verifier, expiresAt: now.Add(ttl), userID: userID} } -func (s *memoryStateStore) GetAndDelete(state string) (string, bool) { +func (s *memoryStateStore) GetAndDelete(state string) (string, int64, bool) { s.mu.Lock() defer s.mu.Unlock() e, ok := s.entries[state] if !ok { - return "", false + return "", 0, false } delete(s.entries, state) if time.Now().After(e.expiresAt) { - return "", false + return "", 0, false } - return e.verifier, true + return e.verifier, e.userID, true } const randAlphabet = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567" @@ -106,14 +107,23 @@ func generateCodeChallenge(verifier string) string { return base64.RawURLEncoding.EncodeToString(h.Sum(nil)) } -// HandleLogin redirects to the provider's authorization endpoint with a -// single-use state and S256 PKCE challenge. +// HandleLogin starts the authorization code flow with PKCE. +// It generates a single-use state and a S256 code challenge, stores the +// verifier with the initiating user ID and a 10-minute expiry, and redirects +// to the provider authorization URL. The request must carry an authenticated +// Piper session; otherwise HandleLogin responds with 401. func (o *Service) HandleLogin(w http.ResponseWriter, r *http.Request) { + initiatorID := o.getUserID(r.Context()) + if initiatorID == 0 { + http.Error(w, "authentication required", http.StatusUnauthorized) + return + } + state := randText(26) verifier := randText(64) challenge := generateCodeChallenge(verifier) - o.store.Set(state, verifier, 10*time.Minute) + o.store.Set(state, verifier, initiatorID, 10*time.Minute) opts := []oauth2.AuthCodeOption{ oauth2.SetAuthURLParam("code_challenge", challenge), @@ -167,10 +177,11 @@ var ( errNoCode = errors.New("no code provided") errNoReceiver = errors.New("token receiver not configured") errExchangeFailed = errors.New("failed to exchange code for token") + errStoreFailed = errors.New("failed to store access token") ) func httpStatusForOAuthError(err error) int { - if errors.Is(err, errNoReceiver) || errors.Is(err, errExchangeFailed) { + if errors.Is(err, errNoReceiver) || errors.Is(err, errExchangeFailed) || errors.Is(err, errStoreFailed) { return http.StatusInternalServerError } @@ -181,12 +192,17 @@ func httpStatusForOAuthError(err error) int { // invalidates the entry and provider errors are only logged for a valid // state, preventing CSRF bypass via forged error. func (o *Service) completeLogin(ctx context.Context, p completeLoginParams) (int64, error) { - verifier, ok := o.store.GetAndDelete(p.State) + verifier, storedUserID, ok := o.store.GetAndDelete(p.State) if !ok { o.logger.Printf("OAuth2 Callback Error: State mismatch or expired. Got '%s'", p.State) return 0, errStateMismatch } + if storedUserID != p.UserID { + o.logger.Printf("OAuth2 Callback Error: State user mismatch: expected %d, got %d", storedUserID, p.UserID) + return 0, errStateMismatch + } + if p.Code == "" { if p.ProviderError != "" || p.ProviderDesc != "" { o.logger.Printf("OAuth2 Callback Error: provider returned error for state '%s': error=%q desc=%q", p.State, p.ProviderError, p.ProviderDesc) @@ -222,6 +238,7 @@ func (o *Service) exchangeAndStore(ctx context.Context, code, verifier string, u token.AccessToken[:min(len(token.AccessToken), 10)], err, ) + return 0, errStoreFailed } o.logger.Printf("OAuth2 Callback Success: Exchanged code for token, UserID: %d", userID) diff --git a/oauth/oauth2_test.go b/oauth/oauth2_test.go index 76e17b2..ddc1bc0 100644 --- a/oauth/oauth2_test.go +++ b/oauth/oauth2_test.go @@ -2,6 +2,7 @@ package oauth import ( "context" + "errors" "io" "log" "net/http" @@ -11,6 +12,8 @@ import ( "testing" "golang.org/x/oauth2" + + "github.com/teal-fm/piper/session" ) type mockReceiver struct { @@ -35,10 +38,22 @@ func (m *mockReceiver) SetAccessToken(token, refresh string, uid int64) (int64, return 42, nil } -func noUser(context.Context) int64 { return 0 } +func sessionUser(ctx context.Context) int64 { + id, _ := session.GetUserID(ctx) + return id +} + +func withUserCtx(uid int64) context.Context { + return session.WithUserID(context.Background(), uid) +} func stateFromLogin(svc *Service) string { - req := httptest.NewRequest(http.MethodGet, "/login", nil) + return stateFromLoginAs(svc, 1) +} + +func stateFromLoginAs(svc *Service, uid int64) string { + ctx := withUserCtx(uid) + req := httptest.NewRequestWithContext(ctx, http.MethodGet, "/login", nil) rr := httptest.NewRecorder() svc.HandleLogin(rr, req) loc := rr.Result().Header.Get("Location") @@ -53,7 +68,7 @@ func forgedState(svc *Service) string { func consumedState(svc *Service) string { s := stateFromLogin(svc) - _, _ = svc.completeLogin(context.Background(), completeLoginParams{State: s, Code: "c"}) + _, _ = svc.completeLogin(withUserCtx(1), completeLoginParams{State: s, Code: "c", UserID: 1}) return s } @@ -117,6 +132,19 @@ func wantIDWithSession(wantID, wantUID int64) func(t *testing.T, gotID int64, er } } +func wantStoreFailed(t *testing.T, gotID int64, err error, _ *mockReceiver) { + t.Helper() + if err == nil || !strings.Contains(err.Error(), "failed to store") { + t.Fatalf("err = %v, want containing %q", err, "failed to store") + } + if !errors.Is(err, errStoreFailed) { + t.Fatalf("err = %v, want errStoreFailed", err) + } + if got := httpStatusForOAuthError(err); got != http.StatusInternalServerError { + t.Fatalf("http status = %d, want %d", got, http.StatusInternalServerError) + } +} + func TestGenerateCodeChallenge(t *testing.T) { tests := []struct { name string @@ -159,10 +187,10 @@ func TestHandleLogin_Redirect(t *testing.T) { }, nil, log.New(io.Discard, "", 0), - noUser, + sessionUser, ) - req := httptest.NewRequest(http.MethodGet, "http://example.com/login", nil) + req := httptest.NewRequestWithContext(withUserCtx(1), http.MethodGet, "http://example.com/login", nil) rr := httptest.NewRecorder() svc.HandleLogin(rr, req) @@ -208,9 +236,9 @@ func TestHandleLogin_ChallengeBindsToStoredVerifier(t *testing.T) { TokenURL: ts.URL, }, } - svc := NewOAuth2Service(cfg, &mockReceiver{returnID: 1}, log.New(io.Discard, "", 0), noUser) + svc := NewOAuth2Service(cfg, &mockReceiver{returnID: 1}, log.New(io.Discard, "", 0), sessionUser) - req := httptest.NewRequest(http.MethodGet, "/login", nil) + req := httptest.NewRequestWithContext(withUserCtx(1), http.MethodGet, "/login", nil) rr := httptest.NewRecorder() svc.HandleLogin(rr, req) loc := rr.Result().Header.Get("Location") @@ -218,7 +246,7 @@ func TestHandleLogin_ChallengeBindsToStoredVerifier(t *testing.T) { state := u.Query().Get("state") challenge := u.Query().Get("code_challenge") - reqCB := httptest.NewRequest(http.MethodGet, "/callback?state="+url.QueryEscape(state)+"&code=c", nil) + reqCB := httptest.NewRequestWithContext(withUserCtx(1), http.MethodGet, "/callback?state="+url.QueryEscape(state)+"&code=c", nil) rrCB := httptest.NewRecorder() if _, err := svc.HandleCallback(rrCB, reqCB); err != nil { t.Fatalf("HandleCallback: %v", err) @@ -240,6 +268,7 @@ func TestCompleteLogin(t *testing.T) { setup func(svc *Service) string code string uid int64 + initUID int64 check func(t *testing.T, gotID int64, err error, receiver *mockReceiver) }{ { @@ -248,6 +277,8 @@ func TestCompleteLogin(t *testing.T) { tokenFunc: okTokenHandler, setup: forgedState, code: "c", + uid: 0, + initUID: 0, check: wantErr("state mismatch"), }, { @@ -256,6 +287,8 @@ func TestCompleteLogin(t *testing.T) { tokenFunc: okTokenHandler, setup: stateFromLogin, code: "", + uid: 1, + initUID: 1, check: wantErr("no code provided"), }, { @@ -264,6 +297,8 @@ func TestCompleteLogin(t *testing.T) { tokenFunc: okTokenHandler, setup: stateFromLogin, code: "c", + uid: 1, + initUID: 1, check: wantErr("token receiver not configured"), }, { @@ -272,6 +307,8 @@ func TestCompleteLogin(t *testing.T) { tokenFunc: errorTokenHandler, setup: stateFromLogin, code: "c", + uid: 1, + initUID: 1, check: wantErr("failed to exchange"), }, { @@ -280,15 +317,18 @@ func TestCompleteLogin(t *testing.T) { tokenFunc: okTokenHandler, setup: stateFromLogin, code: "c", + uid: 1, + initUID: 1, check: wantID(777), }, { name: "success with session", receiver: &mockReceiver{returnID: 99}, tokenFunc: okTokenHandler, - setup: stateFromLogin, + setup: func(svc *Service) string { return stateFromLoginAs(svc, 555) }, code: "c", uid: 555, + initUID: 555, check: wantIDWithSession(99, 555), }, { @@ -297,8 +337,30 @@ func TestCompleteLogin(t *testing.T) { tokenFunc: okTokenHandler, setup: consumedState, code: "c", + uid: 0, + initUID: 0, check: wantErr("state mismatch"), }, + { + name: "cross-user state transfer blocked", + receiver: &mockReceiver{returnID: 99}, + tokenFunc: okTokenHandler, + setup: func(svc *Service) string { return stateFromLoginAs(svc, 111) }, + code: "c", + uid: 222, + initUID: 111, + check: wantErr("state mismatch"), + }, + { + name: "store failure maps to 500", + receiver: &mockReceiver{returnErr: errors.New("db down")}, + tokenFunc: okTokenHandler, + setup: stateFromLogin, + code: "c", + uid: 1, + initUID: 1, + check: wantStoreFailed, + }, } for _, tt := range tests { @@ -322,11 +384,11 @@ func TestCompleteLogin(t *testing.T) { }, recv, log.New(io.Discard, "", 0), - noUser, + sessionUser, ) state := tt.setup(svc) - gotID, err := svc.completeLogin(t.Context(), completeLoginParams{State: state, Code: tt.code, UserID: tt.uid}) + gotID, err := svc.completeLogin(withUserCtx(tt.uid), completeLoginParams{State: state, Code: tt.code, UserID: tt.uid}) tt.check(t, gotID, err, tt.receiver) }) } @@ -338,24 +400,42 @@ func TestHandleCallback_HTTPMapping(t *testing.T) { setup func(svc *Service) string receiver TokenReceiver wantStatus int + uid int64 }{ { name: "missing code maps to 400", setup: queryMissingCode, receiver: &mockReceiver{}, wantStatus: http.StatusBadRequest, + uid: 1, }, { name: "nil receiver maps to 500", setup: queryValidCode, receiver: nil, wantStatus: http.StatusInternalServerError, + uid: 1, }, { name: "state mismatch maps to 400", setup: queryMismatch, receiver: &mockReceiver{}, wantStatus: http.StatusBadRequest, + uid: 1, + }, + { + name: "store failure maps to 500", + setup: queryValidCode, + receiver: &mockReceiver{returnErr: errors.New("db down")}, + wantStatus: http.StatusInternalServerError, + uid: 1, + }, + { + name: "user mismatch maps to 400", + setup: func(svc *Service) string { return stateFromLoginAs(svc, 111) }, + receiver: &mockReceiver{}, + wantStatus: http.StatusBadRequest, + uid: 222, }, } @@ -377,11 +457,11 @@ func TestHandleCallback_HTTPMapping(t *testing.T) { }, tt.receiver, log.New(io.Discard, "", 0), - noUser, + sessionUser, ) query := tt.setup(svc) - req := httptest.NewRequest(http.MethodGet, "/callback?"+query, nil) + req := httptest.NewRequestWithContext(withUserCtx(tt.uid), http.MethodGet, "/callback?"+query, nil) rr := httptest.NewRecorder() _, _ = svc.HandleCallback(rr, req) @@ -392,3 +472,34 @@ func TestHandleCallback_HTTPMapping(t *testing.T) { }) } } + +func TestHandleLogin_RequiresAuth(t *testing.T) { + svc := NewOAuth2Service( + oauth2.Config{ + ClientID: "id", + Endpoint: oauth2.Endpoint{ + AuthURL: "http://example.com/auth", + TokenURL: "http://example.com/token", + }, + }, + nil, + log.New(io.Discard, "", 0), + sessionUser, + ) + + // Unauthenticated should be 401 + req := httptest.NewRequest(http.MethodGet, "/login", nil) + rr := httptest.NewRecorder() + svc.HandleLogin(rr, req) + if rr.Code != http.StatusUnauthorized { + t.Fatalf("unauth status = %d, want %d", rr.Code, http.StatusUnauthorized) + } + + // Authenticated should redirect + req2 := httptest.NewRequestWithContext(withUserCtx(123), http.MethodGet, "/login", nil) + rr2 := httptest.NewRecorder() + svc.HandleLogin(rr2, req2) + if rr2.Code != http.StatusSeeOther { + t.Fatalf("auth status = %d, want %d", rr2.Code, http.StatusSeeOther) + } +} diff --git a/session/session.go b/session/session.go index 4defa9f..8b619bd 100644 --- a/session/session.go +++ b/session/session.go @@ -242,13 +242,13 @@ func WithAuth(handler http.HandlerFunc, sm *Manager) http.HandlerFunc { // if not found, check cookies for session value cookie, err := r.Cookie("session") if err != nil { - http.Redirect(w, r, "/login/spotify", http.StatusSeeOther) + http.Redirect(w, r, "/", http.StatusSeeOther) return } session, exists := sm.GetSession(cookie.Value) if !exists { - http.Redirect(w, r, "/login/spotify", http.StatusSeeOther) + http.Redirect(w, r, "/", http.StatusSeeOther) return }