Restart the login when a callback carries no state at all

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
 	}

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