From 8ba6f0b97ba4b62b9de25a0b93c6b3b2a9a2f444 Mon Sep 17 00:00:00 2001 From: Jason Ernst Date: Fri, 25 Sep 2026 16:00:28 -0700 Subject: [PATCH] Restart the login when a callback carries no state at all #638 made goblog complete GitHub's OAuth flow itself and reject a callback whose state does not match. That left an upgrade trap: a login page from before 0.12.0 builds the authorize URL itself and asks for no state, so GitHub returns without one and the callback refuses it. The only person who can fix that is an admin, and updating a theme needs a login, which is the thing that no longer works. The absence of a state is the signal. /login/github always sends one, so a callback without one is not our flow. Those discard the code, unexchanged, and start a proper login instead; the visitor ends up signed in as themselves, which is equally why handing this endpoint someone else's code achieves nothing. It cannot cycle. The flow it starts does send a state, so a browser that keeps no cookies comes back *with* one and falls through to the mismatch branch and its error. Only a stateless callback restarts, and only our own flow is stateful, so the two cannot alternate. That is what let this be automatic rather than a page telling the admin to visit /login/github by hand. next rides along when the callback carries one, which a pre-0.12 redirect_uri did, so the restart lands where the visitor was going. GithubLogin puts it through SafeNext, so nothing is trusted here. Verified against a local instance running a pre-0.12 theme: the callback it produces now restarts and mints a real state, with next preserved, while a callback carrying a state from a browser holding no session still gets the error page rather than a second restart. Co-Authored-By: Claude Opus 5 (1M context) --- auth/github_oauth.go | 35 +++++++++++++- auth/github_oauth_internal_test.go | 73 ++++++++++++++++++++++++++++-- 2 files changed, 102 insertions(+), 6 deletions(-) diff --git a/auth/github_oauth.go b/auth/github_oauth.go index 9d979cb..2a4f18e 100644 --- a/auth/github_oauth.go +++ b/auth/github_oauth.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "log" "net/http" + "net/url" "github.com/gin-contrib/sessions" "github.com/gin-gonic/gin" @@ -66,9 +67,39 @@ func (a *Auth) GithubCallback(c *gin.Context) { } got := c.Query("state") + + // No state came back at all. /login/github always sends one, so this is + // not our flow: it is a login page from before goblog took the flow over, + // which built the authorize URL itself and asked for no state. Rejecting + // it would strand the one person who can fix that — an admin cannot + // update a theme without signing in, and signing in is what is broken. + // + // So discard the code, unexchanged, and start a proper flow instead. The + // visitor ends up signed in as themselves, which is also why handing this + // endpoint someone else's code achieves nothing. + // + // This cannot loop: the flow it starts does send a state, so a second + // failure arrives with one and falls through to the check below. A + // browser that keeps no cookies therefore gets an error, not a cycle. + if got == "" { + if err := session.Save(); err != nil { + log.Println("OAuth callback: couldn't clear the login state: " + err.Error()) + } + log.Println("OAuth callback: no state on the callback; restarting the login (a theme older than goblog 0.12.0 does this)") + restart := "/login/github" + // GithubLogin puts this through SafeNext, so a hostile value is + // reduced to "/" there rather than trusted here. + if raw := c.Query("next"); raw != "" { + restart += "?next=" + url.QueryEscape(raw) + } + c.Redirect(http.StatusFound, restart) + return + } + if want == "" || subtle.ConstantTimeCompare([]byte(want), []byte(got)) != 1 { - // Either this browser never started a login, or somebody else's code - // is being handed to it. Send them back to start their own. + // A state came back but it is not the one this browser was issued: + // either somebody else's code is being handed to it, or its cookies + // are not surviving the round trip. reject("state did not match the session; ignoring the code") return } diff --git a/auth/github_oauth_internal_test.go b/auth/github_oauth_internal_test.go index 9a96fe3..e7d064b 100644 --- a/auth/github_oauth_internal_test.go +++ b/auth/github_oauth_internal_test.go @@ -164,16 +164,22 @@ func TestGithubCallback_NoLoginStartedNeverReachesGithub(t *testing.T) { } } -// TestGithubCallback_EmptyStateBothSidesIsNotAMatch: no stored state and no -// state in the callback must not compare equal. -func TestGithubCallback_EmptyStateBothSidesIsNotAMatch(t *testing.T) { +// TestGithubCallback_EmptyStateIsNeverTreatedAsAMatch: two empty strings +// compare equal, so a browser that stored nothing receiving a callback with no +// state must not be read as a match. That case restarts the flow rather than +// being rejected (see the restart tests below), but either way the unvalidated +// code must not be exchanged — which is what this pins. +func TestGithubCallback_EmptyStateIsNeverTreatedAsAMatch(t *testing.T) { r, fake := callbackRouter(t) - get(t, r, "/login?code=attacker-code", nil) + w := get(t, r, "/login?code=attacker-code", nil) if fake.tokenCalls != 0 { t.Errorf("empty state treated as a match (%d calls)", fake.tokenCalls) } + if token := sessionToken(t, r, w.Result().Cookies()); token != "" { + t.Errorf("a session was created from it (token %q)", token) + } } // TestGithubCallback_StateIsSpentAfterOneUse: a state is cleared on use, so @@ -212,3 +218,62 @@ func TestNewOAuthState(t *testing.T) { seen[s] = true } } + +// A login page from before goblog took the OAuth flow over builds the +// authorize URL itself and asks for no state, so GitHub returns without one. +// Rejecting that would strand the admin: updating the theme needs a login, and +// the login is what is broken. Those callbacks restart the flow instead (#637 +// follow-up). + +func TestGithubCallback_NoStateAtAllRestartsTheFlow(t *testing.T) { + r, fake := callbackRouter(t) + + w := get(t, r, "/login?code=code-from-an-old-theme", nil) + + if loc := w.Header().Get("Location"); loc != "/login/github" { + t.Errorf("Location = %q, want a restart at /login/github", loc) + } + // The code is discarded, not exchanged: it was never tied to this browser. + if fake.tokenCalls != 0 { + t.Errorf("the unvalidated code was exchanged (%d calls)", fake.tokenCalls) + } + if token := sessionToken(t, r, w.Result().Cookies()); token != "" { + t.Errorf("a session was created from an unvalidated code (token %q)", token) + } +} + +// TestGithubCallback_RestartKeepsNext: the old flow put next in redirect_uri, +// so it is on the callback URL. Carrying it over means the restart lands where +// the visitor was originally going. +func TestGithubCallback_RestartKeepsNext(t *testing.T) { + r, _ := callbackRouter(t) + + w := get(t, r, "/login?code=c&next=%2Fadmin%2Fsettings", nil) + + if loc := w.Header().Get("Location"); loc != "/login/github?next=%2Fadmin%2Fsettings" { + t.Errorf("Location = %q, want next carried into the restart", loc) + } +} + +// TestGithubCallback_RestartCannotLoop is the property that makes restarting +// safe: /login/github always sends a state, so a browser whose cookies do not +// survive comes back *with* a state and gets an error rather than another +// restart. Only a stateless callback restarts, and only our own flow is +// stateful, so the two cannot alternate. +func TestGithubCallback_RestartCannotLoop(t *testing.T) { + r, fake := callbackRouter(t) + + // A callback carrying a state, but from a browser that stored nothing — + // what the restarted flow looks like when cookies are not kept. + w := get(t, r, "/login?code=c&state=issued-but-never-stored", nil) + + if loc := w.Header().Get("Location"); loc == "/login/github" { + t.Error("a stateful callback restarted the flow, which is how a cycle would form") + } + if loc := w.Header().Get("Location"); loc != "/login?error=github" { + t.Errorf("Location = %q, want the error page", loc) + } + if fake.tokenCalls != 0 { + t.Errorf("exchanged anyway (%d calls)", fake.tokenCalls) + } +}