diff --git a/internal/oauth/device.go b/internal/oauth/device.go index 8e195c662..697de057c 100644 --- a/internal/oauth/device.go +++ b/internal/oauth/device.go @@ -49,9 +49,9 @@ func RequestDeviceCode(ctx context.Context, client *http.Client, cfg Config, now if err := validateTokenEndpoint(endpoint); err != nil { return DeviceAuth{}, err } - if client == nil { - client = http.DefaultClient - } + // Refuse redirects so a device-authorization redirect can't replay the + // client_id/client_secret POST body to an unvalidated origin. + client = withoutRedirects(client) if now == nil { now = time.Now } @@ -157,9 +157,9 @@ func pollDeviceOnce(ctx context.Context, client *http.Client, cfg Config, device if err := validateTokenEndpoint(cfg.TokenEndpoint); err != nil { return Token{}, err } - if client == nil { - client = http.DefaultClient - } + // Refuse redirects so a poll redirect can't replay the device_code/secret + // POST body to an unvalidated origin. + client = withoutRedirects(client) form := url.Values{} form.Set("grant_type", deviceGrantType) form.Set("device_code", deviceCode) diff --git a/internal/oauth/device_test.go b/internal/oauth/device_test.go index d5cc3deee..9ccb17440 100644 --- a/internal/oauth/device_test.go +++ b/internal/oauth/device_test.go @@ -2,6 +2,7 @@ package oauth import ( "context" + "errors" "net/http" "net/http/httptest" "strings" @@ -28,6 +29,30 @@ func TestRequestDeviceCodeSendsClientSecret(t *testing.T) { } } +func TestRequestDeviceCodeRefusesRedirect(t *testing.T) { + endpoint, hits := redirectTrap(t, http.StatusTemporaryRedirect) // 307 + _, err := RequestDeviceCode(context.Background(), http.DefaultClient, + Config{ClientID: "c", ClientSecret: "shh", DeviceAuthorizationEndpoint: endpoint}, nil) + if !errors.Is(err, ErrUnsafeRedirect) { + t.Fatalf("RequestDeviceCode err = %v, want ErrUnsafeRedirect", err) + } + if n := hits(); n != 0 { + t.Fatalf("attacker received %d request(s) — client_secret replayed", n) + } +} + +func TestPollDeviceOnceRefusesRedirect(t *testing.T) { + endpoint, hits := redirectTrap(t, http.StatusPermanentRedirect) // 308 + _, err := pollDeviceOnce(context.Background(), http.DefaultClient, + Config{ClientID: "c", ClientSecret: "shh", TokenEndpoint: endpoint}, "device-code", nil) + if !errors.Is(err, ErrUnsafeRedirect) { + t.Fatalf("pollDeviceOnce err = %v, want ErrUnsafeRedirect", err) + } + if n := hits(); n != 0 { + t.Fatalf("attacker received %d request(s) — device_code/secret replayed", n) + } +} + func TestRequestDeviceCode(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(`{"device_code":"dc","user_code":"WXYZ-1234","verification_uri":"https://x/dev","verification_uri_complete":"https://x/dev?c=WXYZ","expires_in":600,"interval":3}`)) diff --git a/internal/oauth/flow.go b/internal/oauth/flow.go index 0c8a343cb..9ff94cccd 100644 --- a/internal/oauth/flow.go +++ b/internal/oauth/flow.go @@ -116,6 +116,23 @@ func validateTokenEndpoint(endpoint string) error { return ValidateEndpointURL(endpoint) } +// withoutRedirects returns a shallow copy of client whose redirect policy +// refuses to follow ANY redirect. A credential-bearing OAuth POST (token +// exchange/refresh, device authorization, device-token poll) must never let a +// 307/308 replay its form body — codes, PKCE verifiers, refresh tokens, client +// secrets — to a target that was never endpoint-validated. The caller's client +// is left unmodified. +func withoutRedirects(client *http.Client) *http.Client { + if client == nil { + client = http.DefaultClient + } + copied := *client + copied.CheckRedirect = func(*http.Request, []*http.Request) error { + return ErrUnsafeRedirect + } + return &copied +} + func isLoopbackHost(host string) bool { if host == "localhost" { return true @@ -176,9 +193,9 @@ func PostToken(ctx context.Context, client *http.Client, tokenEndpoint string, f if err := validateTokenEndpoint(tokenEndpoint); err != nil { return Token{}, err } - if client == nil { - client = http.DefaultClient - } + // Refuse redirects so a 307/308 from the token endpoint can't replay this + // credential-bearing POST body to another (unvalidated) origin. + client = withoutRedirects(client) if now == nil { now = time.Now } diff --git a/internal/oauth/flow_test.go b/internal/oauth/flow_test.go index 3b98d8981..09ef01251 100644 --- a/internal/oauth/flow_test.go +++ b/internal/oauth/flow_test.go @@ -5,10 +5,12 @@ import ( "crypto/sha256" "encoding/base64" "errors" + "io" "net/http" "net/http/httptest" "net/url" "strings" + "sync/atomic" "testing" "time" ) @@ -126,6 +128,64 @@ func TestBuildAuthorizationURLRejectsInsecureEndpoint(t *testing.T) { } } +// redirectTrap starts an "attacker" server that counts any hit, and a credential +// endpoint that redirects to it with the given status. It returns the endpoint +// URL and a func reporting how many times the attacker was contacted — a +// non-zero count means the credential-bearing POST body was replayed (#729). +func redirectTrap(t *testing.T, status int) (endpoint string, attackerHits func() int32) { + t.Helper() + var hits int32 + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&hits, 1) + _, _ = io.WriteString(w, `{"access_token":"leaked","refresh_token":"leaked"}`) + })) + t.Cleanup(attacker.Close) + endp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, attacker.URL, status) + })) + t.Cleanup(endp.Close) + return endp.URL, func() int32 { return atomic.LoadInt32(&hits) } +} + +func TestPostTokenRefusesRedirect(t *testing.T) { + endpoint, hits := redirectTrap(t, http.StatusTemporaryRedirect) // 307 + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("code", "the-code") + form.Set("client_secret", "shh") + _, err := PostToken(context.Background(), http.DefaultClient, endpoint, form, Token{}, nil) + if !errors.Is(err, ErrUnsafeRedirect) { + t.Fatalf("PostToken err = %v, want ErrUnsafeRedirect", err) + } + if n := hits(); n != 0 { + t.Fatalf("attacker received %d request(s) — credential body was replayed", n) + } +} + +func TestExchangeCodeRefusesRedirect(t *testing.T) { + endpoint, hits := redirectTrap(t, http.StatusPermanentRedirect) // 308 + _, err := ExchangeCode(context.Background(), http.DefaultClient, + Config{ClientID: "c", TokenEndpoint: endpoint}, "the-code", "verifier", "http://127.0.0.1/cb", nil) + if !errors.Is(err, ErrUnsafeRedirect) { + t.Fatalf("ExchangeCode err = %v, want ErrUnsafeRedirect", err) + } + if n := hits(); n != 0 { + t.Fatalf("attacker received %d request(s) — code/verifier replayed", n) + } +} + +func TestRefreshRefusesRedirect(t *testing.T) { + endpoint, hits := redirectTrap(t, http.StatusTemporaryRedirect) // 307 + _, err := Refresh(context.Background(), http.DefaultClient, + Config{ClientID: "c", TokenEndpoint: endpoint}, Token{RefreshToken: "rt"}, nil) + if !errors.Is(err, ErrUnsafeRedirect) { + t.Fatalf("Refresh err = %v, want ErrUnsafeRedirect", err) + } + if n := hits(); n != 0 { + t.Fatalf("attacker received %d request(s) — refresh_token replayed", n) + } +} + func TestValidateTokenEndpoint(t *testing.T) { cases := map[string]bool{ "https://auth.example.com/token": true, diff --git a/internal/oauth/oauth.go b/internal/oauth/oauth.go index df0bb33bc..4938e7bf3 100644 --- a/internal/oauth/oauth.go +++ b/internal/oauth/oauth.go @@ -90,6 +90,11 @@ var ( // ErrInsecureTokenEndpoint is returned when a credential would be sent over a // non-https, non-loopback endpoint. ErrInsecureTokenEndpoint = errors.New("oauth: refusing to send credential to a non-https token endpoint") + // ErrUnsafeRedirect is returned when a credential-bearing OAuth POST (token + // exchange/refresh, device authorization, or device-token poll) is redirected. + // Following it would replay the form body — codes, PKCE verifiers, refresh + // tokens, client secrets — to a target that never passed endpoint validation. + ErrUnsafeRedirect = errors.New("oauth: refusing to follow a redirect from a credential endpoint") // ErrNoRefreshToken is returned when a refresh is attempted without one. ErrNoRefreshToken = errors.New("oauth: no refresh token available") // ErrAuthorizationPending is the RFC 8628 "keep polling" signal.