Finish GitHub's OAuth flow server-side, against a state

auth/auth.go

@@ -74,12 +74,17 @@ func (a *Auth) UpdateDb(db *gorm.DB) {
 // use the Github app credentials + the code we received from javascript
 // client side to make the access token (bearer) request
 func (a *Auth) requestAccessToken(parsedCode string) (*AccessTokenResponse, error) {
-	err := godotenv.Load(".env")
-	if err != nil {
-		return nil, errors.New("Error loading .env file: " + err.Error())
+	// .env is the usual home for these, but a missing file is not itself an
+	// error: a deployment may set them in the real environment. What matters
+	// is ending up with both values, which is checked below.
+	if err := godotenv.Load(".env"); err != nil {
+		_ = godotenv.Load("local.env")
 	}
 	clientID := os.Getenv("client_id")
 	clientSecret := os.Getenv("client_secret")
+	if clientID == "" || clientSecret == "" {
+		return nil, errors.New("client_id and client_secret are not configured")
+	}
 
 	data := &AccessTokenResponse{}
 
@@ -89,7 +94,7 @@ func (a *Auth) requestAccessToken(parsedCode string) (*AccessTokenResponse, erro
 		"client_secret": {clientSecret},
 		"code":          {parsedCode},
 	}
-	req, err := http.NewRequest("POST", "https://github.com/login/oauth/access_token", strings.NewReader(formData.Encode()))
+	req, err := http.NewRequest("POST", githubTokenURL, strings.NewReader(formData.Encode()))
 	if err != nil {
 		return nil, err
 	}
@@ -131,7 +136,7 @@ type githubUser struct {
 // unstored BlogUser (ID is zero; the database assigns it on UpsertUser).
 func (a *Auth) RequestUser(accessToken string) (*BlogUser, error) {
 	//get the user info from Github
-	req, err := http.NewRequest("GET", "https://api.github.com/user", strings.NewReader(""))
+	req, err := http.NewRequest("GET", githubUserURL, strings.NewReader(""))
 	if err != nil {
 		return nil, err
 	}
@@ -223,30 +228,24 @@ func (a *Auth) UpsertUser(user *BlogUser) (*BlogUser, error) {
 	return &existing, nil
 }
 
-// LoginPostHandler should be called with the code provided by github. After
-// receiving the code, this will reach out to github to retrieve and auth token
-// which is stored in the db along with the user information from github.
-// this can then be used for authorization when the api user supplies the same
-// auth token later on for API access. Only one auth token per user can be used
-// at once. Logout should remove the auth token from the table.
-func (a *Auth) LoginPostHandler(c *gin.Context) {
-	parsedCode := c.PostForm("code")
-	data, err := a.requestAccessToken(parsedCode)
+// completeGithubLogin exchanges an authorization code for a token, stores the
+// user behind it and puts the token in the session. GithubCallback is its only
+// caller: the code used to arrive by POST from the login page, which meant
+// anyone could hand the server a code for any account (#637).
+func (a *Auth) completeGithubLogin(c *gin.Context, code string) error {
+	data, err := a.requestAccessToken(code)
 	if err != nil {
-		c.JSON(http.StatusUnauthorized, "Error requesting token access: "+err.Error())
-		return
+		return errors.New("error requesting token access: " + err.Error())
 	}
 
 	user, err := a.RequestUser(data.AccessToken)
 	if err != nil {
-		c.JSON(http.StatusInternalServerError, err.Error())
-		return
+		return err
 	}
 
 	stored, err := a.UpsertUser(user)
 	if err != nil {
-		c.JSON(http.StatusInternalServerError, "Error storing user: "+err.Error())
-		return
+		return errors.New("error storing user: " + err.Error())
 	}
 
 	// On a fresh install where the operator has pre-populated .env (e.g. via
@@ -261,14 +260,20 @@ func (a *Auth) LoginPostHandler(c *gin.Context) {
 		log.Println("Error ensuring admin user: " + err.Error())
 	}
 
-	//save the access token in the session
-	session := sessions.Default(c)
-	session.Set("token", data.AccessToken)
-	session.Save()
-
-	c.JSON(http.StatusOK, stored)
+	// Set but do not save: GithubCallback saves once, so the response carries
+	// a single Set-Cookie holding both this token and the cleared OAuth state.
+	sessions.Default(c).Set("token", data.AccessToken)
+	return nil
 }
 
+// GitHub's endpoints, as variables so a test can point them at a stand-in and
+// observe whether the exchange was attempted at all — the difference between
+// a callback that was rejected and one that merely failed to reach GitHub.
+var (
+	githubTokenURL = "https://github.com/login/oauth/access_token"
+	githubUserURL  = "https://api.github.com/user"
+)
+
 // ErrNotConfiguredAdmin is returned by EnsureAdmin when no admin exists yet
 // but the logging-in user is not the identity pinned by admin_login /
 // admin_github_id in .env.

auth/github_oauth.go

@@ -0,0 +1,98 @@
+package auth
+
+import (
+	"crypto/rand"
+	"crypto/subtle"
+	"encoding/base64"
+	"log"
+	"net/http"
+
+	"github.com/gin-contrib/sessions"
+	"github.com/gin-gonic/gin"
+)
+
+// GitHub's OAuth flow used to finish in the browser: the login page read
+// ?code= out of the query and posted it to /api/login, which exchanged it and
+// set the session. Nothing tied the code to the visitor who started the flow,
+// so a code obtained for one account could be replayed into another visitor's
+// browser and leave them signed in as somebody else (#637).
+//
+// The flow is now server-side end to end. /login/github mints a state, keeps
+// it with the intended destination in the session, and sends it to GitHub;
+// GithubCallback will not exchange a code unless the state comes back intact.
+
+const (
+	// OAuthStateKey and OAuthNextKey name the session values that carry a
+	// login across the round trip to GitHub. blog sets them when it starts
+	// the flow; this package reads them when GitHub returns.
+	OAuthStateKey = "oauth_state"
+	OAuthNextKey  = "oauth_next"
+)
+
+// NewOAuthState returns an unguessable value to tie an authorize request to
+// the browser that made it.
+func NewOAuthState() (string, error) {
+	raw := make([]byte, 32)
+	if _, err := rand.Read(raw); err != nil {
+		return "", err
+	}
+	return base64.RawURLEncoding.EncodeToString(raw), nil
+}
+
+// GithubCallback handles GitHub's return to /login.
+//
+// It answers with a redirect either way, so a failure leaves the visitor on
+// the login page rather than on a dead end. ?error=github tells the template
+// something went wrong; a theme that does not render it simply shows the
+// login page again.
+func (a *Auth) GithubCallback(c *gin.Context) {
+	session := sessions.Default(c)
+
+	want, _ := session.Get(OAuthStateKey).(string)
+	next, _ := session.Get(OAuthNextKey).(string)
+
+	// One shot: whether or not it matches, this state is spent. Every path
+	// below saves, so a rejected callback still clears it and the same code
+	// and state cannot be presented twice.
+	session.Delete(OAuthStateKey)
+	session.Delete(OAuthNextKey)
+
+	reject := func(reason string) {
+		log.Println("OAuth callback: " + reason)
+		if err := session.Save(); err != nil {
+			log.Println("OAuth callback: couldn't clear the login state: " + err.Error())
+		}
+		c.Redirect(http.StatusFound, "/login?error=github")
+	}
+
+	got := c.Query("state")
+	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.
+		reject("state did not match the session; ignoring the code")
+		return
+	}
+
+	code := c.Query("code")
+	if code == "" {
+		reject("no code in the callback")
+		return
+	}
+
+	// completeGithubLogin puts the token on the session; the single save
+	// below persists it together with the cleared state.
+	if err := a.completeGithubLogin(c, code); err != nil {
+		reject(err.Error())
+		return
+	}
+	if err := session.Save(); err != nil {
+		log.Println("OAuth callback: couldn't save the session: " + err.Error())
+		c.Redirect(http.StatusFound, "/login?error=github")
+		return
+	}
+
+	if next == "" {
+		next = "/"
+	}
+	c.Redirect(http.StatusFound, next)
+}