693 lines
25 KiB
Go
693 lines
25 KiB
Go
package login
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"io"
|
|
"log/slog"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"net/url"
|
|
"strings"
|
|
"sync"
|
|
"testing"
|
|
"time"
|
|
|
|
"textmachine/platform/internal/auth"
|
|
)
|
|
|
|
func newHandler(t *testing.T, iss *fakeIssuer, st *memStore) *Handler {
|
|
t.Helper()
|
|
h, err := New(Config{
|
|
Provider: "google",
|
|
Issuer: iss.srv.URL,
|
|
ClientID: "test-client",
|
|
ClientSecret: "test-secret",
|
|
RedirectURL: "https://app.example.org/auth/callback",
|
|
AfterLogin: "/library",
|
|
SessionIdleTTL: time.Hour,
|
|
SessionMaxAge: 24 * time.Hour,
|
|
SignupGrantMicroUSD: 5_000_000,
|
|
}, st, auth.Cookies{}, slog.New(slog.NewTextHandler(io.Discard, nil)))
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
h.httpClient = iss.srv.Client()
|
|
return h
|
|
}
|
|
|
|
// begin runs the first leg and returns the redirect plus the browser's login cookie.
|
|
func begin(t *testing.T, h *Handler, returnTo string) (loc string, cookie *http.Cookie) {
|
|
t.Helper()
|
|
target := "/auth/login"
|
|
if returnTo != "" {
|
|
target += "?return_to=" + returnTo
|
|
}
|
|
rec := httptest.NewRecorder()
|
|
h.Routes(passthrough).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, target, nil))
|
|
if rec.Code != http.StatusSeeOther {
|
|
t.Fatalf("login start = %d, want 303 (%s)", rec.Code, rec.Body.String())
|
|
}
|
|
for _, c := range rec.Result().Cookies() {
|
|
if c.Name == auth.LoginCookieName {
|
|
cookie = c
|
|
}
|
|
}
|
|
if cookie == nil {
|
|
t.Fatal("no login cookie: the callback would have nothing to compare the state with")
|
|
}
|
|
return rec.Header().Get("Location"), cookie
|
|
}
|
|
|
|
func passthrough(h http.Handler) http.Handler { return h }
|
|
|
|
// callbackPath builds the authorization response the way a conforming server sends it — with the
|
|
// iss parameter of RFC 9207, which Google does send (its discovery document advertises
|
|
// authorization_response_iss_parameter_supported, checked live 05.08).
|
|
func callbackPath(iss *fakeIssuer, state string) string {
|
|
return "/auth/callback?code=abc&state=" + state + "&iss=" + url.QueryEscape(iss.srv.URL)
|
|
}
|
|
|
|
// The whole flow, end to end, against a provider that really verifies PKCE and really signs the
|
|
// identity token. Mutation caught: dropping S256ChallengeOption, dropping VerifierOption, skipping
|
|
// the nonce comparison, or not creating the session.
|
|
func TestLoginCompletesAndCreatesOurOwnSession(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
|
|
loc, cookie := begin(t, h, "%2Flibrary%2Fbk1")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, callbackPath(iss, state), nil)
|
|
req.AddCookie(cookie)
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
|
|
if rec.Code != http.StatusSeeOther {
|
|
t.Fatalf("callback = %d, want 303 (%s)", rec.Code, rec.Body.String())
|
|
}
|
|
if got := rec.Header().Get("Location"); got != "/library/bk1" {
|
|
t.Fatalf("landed on %q, want the requested page", got)
|
|
}
|
|
if iss.tokenCalls != 1 {
|
|
t.Fatalf("token endpoint called %d times", iss.tokenCalls)
|
|
}
|
|
|
|
var session *http.Cookie
|
|
for _, c := range rec.Result().Cookies() {
|
|
if c.Name == auth.CookieName {
|
|
session = c
|
|
}
|
|
}
|
|
if session == nil || session.Value == "" {
|
|
t.Fatal("no session cookie: the login proved an identity and issued nothing")
|
|
}
|
|
if !session.HttpOnly || !session.Secure || session.SameSite != http.SameSiteLaxMode {
|
|
t.Fatalf("session cookie attributes are weaker than the profile: %+v", session)
|
|
}
|
|
// OUR session, in OUR store, keyed by the digest — never the token.
|
|
st.mu.Lock()
|
|
defer st.mu.Unlock()
|
|
if len(st.sessions) != 1 {
|
|
t.Fatalf("sessions created: %d", len(st.sessions))
|
|
}
|
|
if _, ok := st.sessions[string(auth.Digest(session.Value))]; !ok {
|
|
t.Fatal("the stored session is not keyed by the digest of the issued token")
|
|
}
|
|
if st.identities != 1 {
|
|
t.Fatalf("identity upserts: %d", st.identities)
|
|
}
|
|
if len(st.events) != 1 || st.events[0].Outcome != "success" || st.events[0].UserID == "" {
|
|
t.Fatalf("journal = %+v", st.events)
|
|
}
|
|
// Nothing the provider issued was kept. Asserted against everything the store was actually
|
|
// handed, so the claim can fail.
|
|
if len(st.saw) == 0 {
|
|
t.Fatal("the store recorded nothing: this assertion would pass against any implementation")
|
|
}
|
|
for _, v := range st.saw {
|
|
if strings.Contains(v, "opaque-access-token") || strings.Contains(v, ".") && strings.Count(v, ".") == 2 {
|
|
t.Fatalf("something vendor-issued reached the store: %q", v)
|
|
}
|
|
}
|
|
}
|
|
|
|
// Each of these is a real attack on the callback, and each must end the same way on the wire and
|
|
// differently in the journal.
|
|
func TestCallbackRefusals(t *testing.T) {
|
|
for name, tc := range map[string]struct {
|
|
mutate func(t *testing.T, iss *fakeIssuer, state string, cookie *http.Cookie) (string, *http.Cookie)
|
|
reason string
|
|
}{
|
|
"no cookie": {
|
|
mutate: func(_ *testing.T, _ *fakeIssuer, state string, _ *http.Cookie) (string, *http.Cookie) {
|
|
return state, nil
|
|
},
|
|
reason: "missing_state",
|
|
},
|
|
"cookie does not match the state": {
|
|
mutate: func(_ *testing.T, _ *fakeIssuer, state string, c *http.Cookie) (string, *http.Cookie) {
|
|
c.Value = "someone-elses-state"
|
|
return state, c
|
|
},
|
|
reason: "state_mismatch",
|
|
},
|
|
"state was never issued": {
|
|
mutate: func(_ *testing.T, _ *fakeIssuer, _ string, c *http.Cookie) (string, *http.Cookie) {
|
|
c.Value = "forged"
|
|
return "forged", c
|
|
},
|
|
reason: "unknown_state",
|
|
},
|
|
"token minted for another nonce": {
|
|
mutate: func(_ *testing.T, iss *fakeIssuer, state string, c *http.Cookie) (string, *http.Cookie) {
|
|
iss.nonce = "a-nonce-from-another-request"
|
|
return state, c
|
|
},
|
|
reason: "token_rejected",
|
|
},
|
|
"token minted for another audience": {
|
|
mutate: func(_ *testing.T, iss *fakeIssuer, state string, c *http.Cookie) (string, *http.Cookie) {
|
|
iss.audience = "another-client"
|
|
return state, c
|
|
},
|
|
reason: "token_rejected",
|
|
},
|
|
"expired token": {
|
|
mutate: func(_ *testing.T, iss *fakeIssuer, state string, c *http.Cookie) (string, *http.Cookie) {
|
|
iss.expiresIn = -time.Minute
|
|
return state, c
|
|
},
|
|
reason: "token_rejected",
|
|
},
|
|
} {
|
|
t.Run(name, func(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
loc, cookie := begin(t, h, "")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
|
|
state, cookie = tc.mutate(t, iss, state, cookie)
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, callbackPath(iss, state), nil)
|
|
if cookie != nil {
|
|
req.AddCookie(cookie)
|
|
}
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
|
|
if rec.Code != http.StatusBadRequest {
|
|
t.Fatalf("callback = %d, want 400", rec.Code)
|
|
}
|
|
st.mu.Lock()
|
|
defer st.mu.Unlock()
|
|
if len(st.sessions) != 0 {
|
|
t.Fatal("a refused login created a session")
|
|
}
|
|
if len(st.events) != 1 || st.events[0].Outcome != "denied" {
|
|
t.Fatalf("journal = %+v", st.events)
|
|
}
|
|
if got := st.events[0].Reason; !strings.HasPrefix(got, tc.reason) {
|
|
t.Fatalf("journal reason = %q, want %q", got, tc.reason)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// A state may be spent once. The second callback carrying it — a replay, or the second half of a
|
|
// race — must find nothing.
|
|
func TestStateCannotBeReplayed(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
loc, cookie := begin(t, h, "")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
|
|
call := func() int {
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, callbackPath(iss, state), nil)
|
|
req.AddCookie(cookie)
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
return rec.Code
|
|
}
|
|
if code := call(); code != http.StatusSeeOther {
|
|
t.Fatalf("first callback = %d", code)
|
|
}
|
|
if code := call(); code != http.StatusBadRequest {
|
|
t.Fatalf("replayed callback = %d, want 400", code)
|
|
}
|
|
if iss.tokenCalls != 1 {
|
|
t.Fatalf("a replayed state reached the token endpoint %d times", iss.tokenCalls)
|
|
}
|
|
}
|
|
|
|
// Session fixation: whatever session the browser carried into the login, it does not carry out.
|
|
// Mutation caught: removing the revoke of the presented session.
|
|
func TestLoginRevokesThePresentedSession(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
|
|
planted := auth.NewToken()
|
|
st.sessions[string(auth.Digest(planted))] = "victim"
|
|
|
|
loc, cookie := begin(t, h, "")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, callbackPath(iss, state), nil)
|
|
req.AddCookie(cookie)
|
|
req.AddCookie(&http.Cookie{Name: auth.CookieName, Value: planted})
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
|
|
if rec.Code != http.StatusSeeOther {
|
|
t.Fatalf("callback = %d", rec.Code)
|
|
}
|
|
st.mu.Lock()
|
|
defer st.mu.Unlock()
|
|
if _, alive := st.sessions[string(auth.Digest(planted))]; alive {
|
|
t.Fatal("the session presented at login survived it: that is session fixation")
|
|
}
|
|
}
|
|
|
|
// An open redirect turns our own login link into a phishing tool, and "//evil.example" is a path to
|
|
// a browser. Mutation caught: relaxing safeReturnTo to a HasPrefix("/") check; dropping the second
|
|
// decode (PD-47 — the percent-encoded block below is the only thing that reaches it).
|
|
func TestReturnToNeverLeavesThisSite(t *testing.T) {
|
|
// Every one of these must come back EMPTY. "Starts with a slash" is not the assertion: a
|
|
// browser normalises "\" to "/", so /\evil.example is a host once it reaches the URL parser.
|
|
for _, raw := range []string{
|
|
"//evil.example/",
|
|
"https://evil.example/",
|
|
"http:/evil.example",
|
|
`/\evil.example`,
|
|
`/\/evil.example`,
|
|
"/\tevil",
|
|
"evil.example",
|
|
"",
|
|
// Percent-encoded. The query value arrives here unescaped exactly once, so these are still
|
|
// text to the raw check and only the second decode sees what they say. Judged conservatively
|
|
// rather than by what a conforming browser would do with them.
|
|
"/%5c/evil.example",
|
|
"/%5C/evil.example",
|
|
"/%09evil",
|
|
"/%00evil",
|
|
"/%0d%0aSet-Cookie:%20x=y",
|
|
// Reaches only the protocol-relative check, and only on the decoded form: "/%2f/evil.example"
|
|
// carries no backslash and parses as an ordinary same-site path. A conforming browser would
|
|
// not treat %2f as a separator, so this is the allowlist being deliberately stricter than the
|
|
// parser — and the input that keeps that branch from being deleted unnoticed.
|
|
"/%2f/evil.example",
|
|
"/%2F/evil.example",
|
|
} {
|
|
if got := safeReturnTo(raw); got != "" {
|
|
t.Fatalf("safeReturnTo(%q) = %q, want the default landing page", raw, got)
|
|
}
|
|
}
|
|
// The other direction, so that "reject anything with a percent sign" is not a passing answer:
|
|
// a legitimate encoded query must survive intact.
|
|
for raw, want := range map[string]string{
|
|
"/library/bk1?tab=notes": "/library/bk1?tab=notes",
|
|
"/library/bk1?q=%D0%BA%D0%BD%D0%B8%D0%B3%D0%B0": "/library/bk1?q=%D0%BA%D0%BD%D0%B8%D0%B3%D0%B0",
|
|
} {
|
|
if got := safeReturnTo(raw); got != want {
|
|
t.Fatalf("safeReturnTo(%q) = %q, want %q", raw, got, want)
|
|
}
|
|
}
|
|
}
|
|
|
|
// PD-48. The signup credit is the only place in this flow that spends money, and it goes only to an
|
|
// identity the provider vouched for: a provider that lets anyone self-register would otherwise turn
|
|
// every new subject into free credit. The account is still created — at zero, for an operator to
|
|
// grant by hand. Mutation caught: deleting the EmailVerified condition.
|
|
func TestSignupGrantGoesOnlyToAVerifiedIdentity(t *testing.T) {
|
|
for _, verified := range []bool{true, false} {
|
|
iss := newIssuer(t)
|
|
iss.emailVerified = verified
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
|
|
loc, cookie := begin(t, h, "")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, callbackPath(iss, state), nil)
|
|
req.AddCookie(cookie)
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
if rec.Code != http.StatusSeeOther {
|
|
t.Fatalf("verified=%v: callback = %d, want 303 (%s)", verified, rec.Code, rec.Body.String())
|
|
}
|
|
|
|
st.mu.Lock()
|
|
grants := append([]int64(nil), st.grants...)
|
|
st.mu.Unlock()
|
|
if len(grants) != 1 {
|
|
t.Fatalf("verified=%v: identity upserts = %d, want 1", verified, len(grants))
|
|
}
|
|
want := int64(0)
|
|
if verified {
|
|
want = 5_000_000
|
|
}
|
|
if grants[0] != want {
|
|
t.Fatalf("verified=%v: signup grant = %d micro-USD, want %d", verified, grants[0], want)
|
|
}
|
|
}
|
|
}
|
|
|
|
// PD-49. The state names the provider that issued it, and a state minted for one must not be
|
|
// redeemable at another — the IdP mix-up class. Latent while there is one provider, which is
|
|
// exactly why it needs a test rather than a reader. Mutation caught: deleting the comparison.
|
|
func TestStateFromAnotherProviderIsRefused(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
|
|
loc, cookie := begin(t, h, "")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
|
|
// The row was written by a start leg naming a DIFFERENT provider — the shape a second provider
|
|
// creates the moment one is added.
|
|
st.mu.Lock()
|
|
key := string(auth.Digest(state))
|
|
row := st.states[key]
|
|
row.Provider = "another-idp"
|
|
st.states[key] = row
|
|
st.mu.Unlock()
|
|
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, callbackPath(iss, state), nil)
|
|
req.AddCookie(cookie)
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
|
|
if rec.Code != http.StatusBadRequest {
|
|
t.Fatalf("callback = %d, want 400: a state from another provider was redeemed here", rec.Code)
|
|
}
|
|
st.mu.Lock()
|
|
defer st.mu.Unlock()
|
|
if len(st.sessions) != 0 {
|
|
t.Fatal("a session was issued for a state this provider never minted")
|
|
}
|
|
if len(st.events) != 1 || st.events[0].Reason != "state_from_another_provider" {
|
|
t.Fatalf("journal = %+v, want the refusal named", st.events)
|
|
}
|
|
if iss.tokenCalls != 0 {
|
|
t.Fatal("the code was exchanged before the state's provider was checked")
|
|
}
|
|
}
|
|
|
|
// The one unauthenticated endpoint that WRITES has to be bounded, or the first bot to find it fills
|
|
// the table. Mutation caught: removing the limiter check.
|
|
func TestLoginStartIsRateLimited(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
st := newMemStore()
|
|
h, err := New(Config{
|
|
Provider: "google", Issuer: iss.srv.URL, ClientID: "test-client", ClientSecret: "s",
|
|
RedirectURL: "https://app.example.org/auth/callback",
|
|
StartRate: 1, StartBurst: 3,
|
|
}, st, auth.Cookies{}, slog.New(slog.NewTextHandler(io.Discard, nil)))
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
h.httpClient = iss.srv.Client()
|
|
|
|
var limited bool
|
|
for range 10 {
|
|
rec := httptest.NewRecorder()
|
|
h.Routes(passthrough).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/login", nil))
|
|
if rec.Code == http.StatusTooManyRequests {
|
|
limited = true
|
|
break
|
|
}
|
|
}
|
|
if !limited {
|
|
t.Fatal("the login endpoint accepted ten bursts without a limit")
|
|
}
|
|
}
|
|
|
|
// An identity provider that is down must not take the service with it, and must not read as a bug.
|
|
func TestProviderOutageIsTemporaryNotFatal(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
iss.srv.Close() // discovery has not run yet: the handler never touched the network at boot
|
|
|
|
rec := httptest.NewRecorder()
|
|
h.Routes(passthrough).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/auth/login", nil))
|
|
if rec.Code != http.StatusServiceUnavailable {
|
|
t.Fatalf("login with the provider down = %d, want 503", rec.Code)
|
|
}
|
|
}
|
|
|
|
// memStore is the flow's persistence, in memory. The SQL implementation is tested against a live
|
|
// Postgres in the pgstore package; here the subject is the flow.
|
|
type memStore struct {
|
|
mu sync.Mutex
|
|
states map[string]State
|
|
sessions map[string]string // digest -> user id
|
|
events []LoginEvent
|
|
// saw records every string the flow hands the store. The point is one assertion: nothing the
|
|
// PROVIDER issued may reach persistence. The previous field was never written to, so that
|
|
// assertion could not fail — the package's defining property was unpinned (found by review).
|
|
saw []string
|
|
// grants records the credit passed with each upsert. Kept because the signup grant is the one
|
|
// decision in this flow that spends money, and a store that discarded the amount left the rule
|
|
// unpinned (PD-48).
|
|
grants []int64
|
|
identities int
|
|
}
|
|
|
|
func newMemStore() *memStore {
|
|
return &memStore{states: map[string]State{}, sessions: map[string]string{}}
|
|
}
|
|
|
|
func (m *memStore) PutLoginState(_ context.Context, s State) error {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
m.saw = append(m.saw, s.Provider, s.Issuer, s.Nonce, s.Verifier, s.ReturnTo, s.StartID)
|
|
m.states[string(s.Hash)] = s
|
|
return nil
|
|
}
|
|
|
|
func (m *memStore) TakeLoginState(_ context.Context, hash []byte, now time.Time) (State, error) {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
s, ok := m.states[string(hash)]
|
|
if !ok || !s.ExpiresAt.After(now) {
|
|
return State{}, errors.New("no state")
|
|
}
|
|
delete(m.states, string(hash))
|
|
return s, nil
|
|
}
|
|
|
|
func (m *memStore) UpsertIdentity(_ context.Context, in Identity, _ time.Time, grant int64) (string, error) {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
m.saw = append(m.saw, in.Provider, in.Subject, in.Email)
|
|
m.identities++
|
|
m.grants = append(m.grants, grant)
|
|
return "user-" + in.Provider + "-" + in.Subject, nil
|
|
}
|
|
|
|
func (m *memStore) CreateSession(_ context.Context, digest []byte, userID string, _ time.Time, _, _ time.Duration) error {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
m.sessions[string(digest)] = userID
|
|
return nil
|
|
}
|
|
|
|
func (m *memStore) RevokeSession(_ context.Context, digest []byte, _ time.Time) error {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
delete(m.sessions, string(digest))
|
|
return nil
|
|
}
|
|
|
|
func (m *memStore) RevokeUserSessions(_ context.Context, userID string, _ time.Time) (int64, error) {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
var n int64
|
|
for d, u := range m.sessions {
|
|
if u == userID {
|
|
delete(m.sessions, d)
|
|
n++
|
|
}
|
|
}
|
|
return n, nil
|
|
}
|
|
|
|
func (m *memStore) RecordLogin(_ context.Context, ev LoginEvent) error {
|
|
m.mu.Lock()
|
|
defer m.mu.Unlock()
|
|
m.saw = append(m.saw, ev.UserID, ev.Provider, ev.Outcome, ev.Reason, ev.IPPrefix, ev.Client)
|
|
m.events = append(m.events, ev)
|
|
return nil
|
|
}
|
|
|
|
// PD-57. RFC 9207 §2.4 in both directions: an authorization response carrying an issuer other than
|
|
// the one the request went to must be rejected, and a response with NO issuer must be rejected when
|
|
// the server is known to send one — otherwise stripping the parameter defeats the check.
|
|
//
|
|
// Both refusals happen BEFORE the code is exchanged: RFC 9207 says the client must not proceed with
|
|
// the grant, and a code handed to the wrong token endpoint is already leaked.
|
|
// Mutation caught: deleting the checkIssuer call, or either of its two branches.
|
|
func TestAuthorizationResponseIssuerIsChecked(t *testing.T) {
|
|
for name, tc := range map[string]struct {
|
|
issSupported bool
|
|
iss string // "" means the parameter is absent
|
|
wantCode int
|
|
wantReason string
|
|
}{
|
|
"issuer of another server": {issSupported: true, iss: "https://evil.example", wantCode: http.StatusBadRequest, wantReason: "issuer_mismatch"},
|
|
"parameter stripped": {issSupported: true, iss: "", wantCode: http.StatusBadRequest, wantReason: "issuer_missing"},
|
|
"server that does not send one": {issSupported: false, iss: "", wantCode: http.StatusSeeOther},
|
|
"server that does not send one, but did": {issSupported: false, iss: "https://evil.example", wantCode: http.StatusBadRequest, wantReason: "issuer_mismatch"},
|
|
} {
|
|
t.Run(name, func(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
iss.issSupported = tc.issSupported
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
loc, cookie := begin(t, h, "")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
|
|
target := "/auth/callback?code=abc&state=" + state
|
|
if tc.iss != "" {
|
|
target += "&iss=" + url.QueryEscape(tc.iss)
|
|
}
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, target, nil)
|
|
req.AddCookie(cookie)
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
|
|
if rec.Code != tc.wantCode {
|
|
t.Fatalf("callback = %d, want %d (%s)", rec.Code, tc.wantCode, rec.Body.String())
|
|
}
|
|
st.mu.Lock()
|
|
defer st.mu.Unlock()
|
|
if tc.wantReason == "" {
|
|
return
|
|
}
|
|
if len(st.sessions) != 0 {
|
|
t.Fatal("a session was issued for a response from an unverified issuer")
|
|
}
|
|
if len(st.events) != 1 || st.events[0].Reason != tc.wantReason {
|
|
t.Fatalf("journal = %+v, want reason %q", st.events, tc.wantReason)
|
|
}
|
|
if iss.tokenCalls != 0 {
|
|
t.Fatalf("the code was exchanged %d times before the issuer was checked", iss.tokenCalls)
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// A provider that accepts the connection and never answers must not hold the handler. In production
|
|
// httpClient is nil, so the exchange runs on http.DefaultClient — which has no timeout — and go-oidc
|
|
// builds its key set from context.Background(); the server sets no WriteTimeout either, so nothing
|
|
// else bounds it. The JWKS leg is the worse one: the key set is shared, so one stalled fetch parks
|
|
// every concurrent sign-in behind it. Found by review, reproduced on the production wiring.
|
|
// Mutation caught: removing the context.WithTimeout from identify.
|
|
func TestAStalledProviderDoesNotHoldTheCallback(t *testing.T) {
|
|
for _, stall := range []string{"token", "keys"} {
|
|
t.Run(stall, func(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
release := make(chan struct{})
|
|
t.Cleanup(func() { close(release) })
|
|
iss.stall, iss.stallOn = release, stall
|
|
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
h.exchangeTimeout = 300 * time.Millisecond
|
|
loc, cookie := begin(t, h, "")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
|
|
done := make(chan int, 1)
|
|
go func() {
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, callbackPath(iss, state), nil)
|
|
req.AddCookie(cookie)
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
done <- rec.Code
|
|
}()
|
|
|
|
select {
|
|
case code := <-done:
|
|
if code != http.StatusBadRequest {
|
|
t.Fatalf("callback = %d, want 400", code)
|
|
}
|
|
case <-time.After(5 * time.Second):
|
|
t.Fatal("the callback is still waiting on a provider that never answered: identify applies no deadline")
|
|
}
|
|
st.mu.Lock()
|
|
defer st.mu.Unlock()
|
|
if len(st.sessions) != 0 {
|
|
t.Fatal("a session was issued without an identity")
|
|
}
|
|
})
|
|
}
|
|
}
|
|
|
|
// The bound that matters is the one on the client go-oidc keeps. Provider.Verifier uses the key set
|
|
// built at discovery, and go-oidc stores it with context.WithoutCancel — so our per-request deadline
|
|
// never reaches its refetch. If that refetch is unbounded, one hung JWKS request keeps every later
|
|
// sign-in failing after the endpoint is healthy again, because they all queue on the same inflight
|
|
// fetch. Found by review, measured. Mutation caught: returning http.DefaultClient (or nil) from
|
|
// Handler.client, or making the default client's Timeout zero.
|
|
func TestTheDefaultProviderClientIsBounded(t *testing.T) {
|
|
h, err := New(Config{
|
|
Provider: "google", Issuer: "https://accounts.example.org", ClientID: "c", ClientSecret: "s",
|
|
RedirectURL: "https://app.example.org/auth/callback",
|
|
}, newMemStore(), auth.Cookies{}, slog.New(slog.NewTextHandler(io.Discard, nil)))
|
|
if err != nil {
|
|
t.Fatal(err)
|
|
}
|
|
c := h.httpClient
|
|
if c == nil || c == http.DefaultClient {
|
|
t.Fatal("provider requests would run on http.DefaultClient, which has no timeout")
|
|
}
|
|
if c.Timeout <= 0 || c.Timeout > time.Minute {
|
|
t.Fatalf("default provider client timeout = %v: go-oidc's own key refetch inherits this and nothing else bounds it", c.Timeout)
|
|
}
|
|
}
|
|
|
|
// The behavioural half: a JWKS fetch that hangs must not poison the sign-ins that come after it.
|
|
// Mutation caught: handing go-oidc an unbounded client at discovery.
|
|
func TestAHungKeyFetchDoesNotPoisonLaterSignIns(t *testing.T) {
|
|
iss := newIssuer(t)
|
|
release := make(chan struct{})
|
|
t.Cleanup(func() { close(release) })
|
|
iss.stall, iss.stallOn, iss.stallOnce = release, "keys", true
|
|
|
|
st := newMemStore()
|
|
h := newHandler(t, iss, st)
|
|
// The production client is bounded by providerTimeout; the test's is bounded the same way, just
|
|
// faster. Without a bound on THIS client the second sign-in below never gets its keys.
|
|
h.httpClient = &http.Client{Transport: iss.srv.Client().Transport, Timeout: 300 * time.Millisecond}
|
|
h.exchangeTimeout = 2 * time.Second
|
|
|
|
signIn := func() int {
|
|
loc, cookie := begin(t, h, "")
|
|
state, challenge, nonce := challengeFrom(t, loc)
|
|
iss.expectChallenge, iss.nonce = challenge, nonce
|
|
rec := httptest.NewRecorder()
|
|
req := httptest.NewRequest(http.MethodGet, callbackPath(iss, state), nil)
|
|
req.AddCookie(cookie)
|
|
h.Routes(passthrough).ServeHTTP(rec, req)
|
|
return rec.Code
|
|
}
|
|
|
|
if code := signIn(); code != http.StatusBadRequest {
|
|
t.Fatalf("the sign-in that met the hung key endpoint = %d, want 400", code)
|
|
}
|
|
if code := signIn(); code != http.StatusSeeOther {
|
|
t.Fatalf("the sign-in AFTER the key endpoint recovered = %d, want 303: the hung fetch is still holding every later sign-in", code)
|
|
}
|
|
}
|