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) + } +}