diff --git a/auth/auth.go b/auth/auth.go index 79582804..2847d097 100644 --- a/auth/auth.go +++ b/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. diff --git a/auth/github_oauth.go b/auth/github_oauth.go new file mode 100644 index 00000000..9d979cb4 --- /dev/null +++ b/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) +} diff --git a/auth/github_oauth_internal_test.go b/auth/github_oauth_internal_test.go new file mode 100644 index 00000000..9a96fe32 --- /dev/null +++ b/auth/github_oauth_internal_test.go @@ -0,0 +1,214 @@ +package auth + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-contrib/sessions" + "github.com/gin-contrib/sessions/cookie" + "github.com/gin-gonic/gin" + "gorm.io/driver/sqlite" + "gorm.io/gorm" +) + +// GithubCallback must not exchange a code unless the state comes back exactly +// as it was issued. Without that, a code obtained for one account can be +// replayed into another visitor's browser and leave them signed in as +// somebody else (#637). +// +// These are internal tests so they can point githubTokenURL at a stand-in. +// That matters: asserting only that no session was created would pass even +// with the state check removed, because an exchange that never reaches GitHub +// fails anyway. What distinguishes a rejected callback from a failed one is +// whether the token endpoint was called at all. + +type fakeGithub struct { + tokenCalls int + userCalls int + server *httptest.Server +} + +func newFakeGithub(t *testing.T) *fakeGithub { + t.Helper() + f := &fakeGithub{} + mux := http.NewServeMux() + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + f.tokenCalls++ + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]string{"access_token": "tok-from-github", "token_type": "bearer"}) + }) + mux.HandleFunc("/user", func(w http.ResponseWriter, r *http.Request) { + f.userCalls++ + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{"id": 42, "login": "octocat", "name": "Octo Cat", "email": "octo@example.invalid"}) + }) + f.server = httptest.NewServer(mux) + t.Cleanup(f.server.Close) + + oldToken, oldUser := githubTokenURL, githubUserURL + githubTokenURL, githubUserURL = f.server.URL+"/token", f.server.URL+"/user" + t.Cleanup(func() { githubTokenURL, githubUserURL = oldToken, oldUser }) + return f +} + +func callbackRouter(t *testing.T) (*gin.Engine, *fakeGithub) { + t.Helper() + gin.SetMode(gin.TestMode) + t.Setenv("client_id", "id") + t.Setenv("client_secret", "secret") + + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + if err != nil { + t.Fatalf("open db: %v", err) + } + if err := db.AutoMigrate(&BlogUser{}, &AdminUser{}); err != nil { + t.Fatalf("migrate: %v", err) + } + a := New(db, "test") + fake := newFakeGithub(t) + + r := gin.New() + r.Use(sessions.Sessions("goblog", cookie.NewStore([]byte("test-session-key")))) + r.GET("/seed", func(c *gin.Context) { // what /login/github does + s := sessions.Default(c) + s.Set(OAuthStateKey, c.Query("state")) + s.Set(OAuthNextKey, "/admin/settings") + if err := s.Save(); err != nil { + t.Fatalf("seeding the session: %v", err) + } + c.Status(http.StatusOK) + }) + r.GET("/login", a.GithubCallback) + r.GET("/whoami", func(c *gin.Context) { + token, _ := sessions.Default(c).Get("token").(string) + c.String(http.StatusOK, token) + }) + return r, fake +} + +func seed(t *testing.T, r *gin.Engine, state string) []*http.Cookie { + t.Helper() + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/seed?state="+state, nil)) + return w.Result().Cookies() +} + +func get(t *testing.T, r *gin.Engine, target string, cookies []*http.Cookie) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodGet, target, nil) + for _, c := range cookies { + req.AddCookie(c) + } + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w +} + +func sessionToken(t *testing.T, r *gin.Engine, cookies []*http.Cookie) string { + t.Helper() + return get(t, r, "/whoami", cookies).Body.String() +} + +// TestGithubCallback_HappyPath pins what a real login does, so the rejection +// tests below mean something: on a matching state the code IS exchanged and +// the visitor lands on the next they asked for. +func TestGithubCallback_HappyPath(t *testing.T) { + r, fake := callbackRouter(t) + cookies := seed(t, r, "matching-state") + + w := get(t, r, "/login?code=good-code&state=matching-state", cookies) + + if fake.tokenCalls != 1 { + t.Errorf("token endpoint called %d times, want 1", fake.tokenCalls) + } + if loc := w.Header().Get("Location"); loc != "/admin/settings" { + t.Errorf("Location = %q, want the stored next", loc) + } + if token := sessionToken(t, r, w.Result().Cookies()); token != "tok-from-github" { + t.Errorf("session token = %q, want the one GitHub returned", token) + } +} + +func TestGithubCallback_MismatchedStateNeverReachesGithub(t *testing.T) { + r, fake := callbackRouter(t) + cookies := seed(t, r, "the-real-state") + + w := get(t, r, "/login?code=attacker-code&state=not-the-real-state", cookies) + + if fake.tokenCalls != 0 { + t.Errorf("the code was exchanged despite a mismatched state (%d calls)", fake.tokenCalls) + } + if loc := w.Header().Get("Location"); loc != "/login?error=github" { + t.Errorf("Location = %q, want /login?error=github", loc) + } + if token := sessionToken(t, r, w.Result().Cookies()); token != "" { + t.Errorf("a mismatched state logged someone in (token %q)", token) + } +} + +// TestGithubCallback_NoLoginStartedNeverReachesGithub is the replay this +// exists to stop: a browser that never visited /login/github has no stored +// state, so a code handed to it must be ignored. +func TestGithubCallback_NoLoginStartedNeverReachesGithub(t *testing.T) { + r, fake := callbackRouter(t) + + w := get(t, r, "/login?code=attacker-code&state=anything", nil) + + if fake.tokenCalls != 0 { + t.Errorf("a code was exchanged without a login having been started (%d calls)", fake.tokenCalls) + } + if token := sessionToken(t, r, w.Result().Cookies()); token != "" { + t.Errorf("session created anyway (token %q)", token) + } +} + +// TestGithubCallback_EmptyStateBothSidesIsNotAMatch: no stored state and no +// state in the callback must not compare equal. +func TestGithubCallback_EmptyStateBothSidesIsNotAMatch(t *testing.T) { + r, fake := callbackRouter(t) + + get(t, r, "/login?code=attacker-code", nil) + + if fake.tokenCalls != 0 { + t.Errorf("empty state treated as a match (%d calls)", fake.tokenCalls) + } +} + +// TestGithubCallback_StateIsSpentAfterOneUse: a state is cleared on use, so +// the same code and state cannot be presented twice. +func TestGithubCallback_StateIsSpentAfterOneUse(t *testing.T) { + r, fake := callbackRouter(t) + cookies := seed(t, r, "one-shot") + + first := get(t, r, "/login?code=good-code&state=one-shot", cookies) + if fake.tokenCalls != 1 { + t.Fatalf("first use: token calls = %d, want 1", fake.tokenCalls) + } + + second := get(t, r, "/login?code=good-code&state=one-shot", first.Result().Cookies()) + if fake.tokenCalls != 1 { + t.Errorf("the state was usable a second time (token calls = %d)", fake.tokenCalls) + } + if loc := second.Header().Get("Location"); loc != "/login?error=github" { + t.Errorf("Location = %q, want the replay rejected", loc) + } +} + +func TestNewOAuthState(t *testing.T) { + seen := map[string]bool{} + for i := 0; i < 32; i++ { + s, err := NewOAuthState() + if err != nil { + t.Fatalf("NewOAuthState: %v", err) + } + if len(s) < 40 { + t.Fatalf("state %q is shorter than 32 random bytes would give", s) + } + if seen[s] { + t.Fatalf("state %q was issued twice", s) + } + seen[s] = true + } +} diff --git a/blog/blog.go b/blog/blog.go index c3d53cf0..75b8a81d 100644 --- a/blog/blog.go +++ b/blog/blog.go @@ -1474,26 +1474,25 @@ func RequestOrigin(c *gin.Context) string { // GithubAuthorizeURL builds the URL that starts GitHub's OAuth flow. // -// redirect_uri keeps the ?next= the visitor arrived with, because that is how -// next survives the round trip: GitHub sends them back to this exact URL with -// &code= appended, and Login reads next from that query. Stripping the query -// would quietly land everyone on / after signing in. +// redirect_uri carries no query of its own. GitHub matches it against the +// callback registered for the app, so a constant value is the one most likely +// to match; where the visitor was heading rides in the session instead, which +// also keeps it out of a URL that gets logged and passed to a third party. // -// Both the redirect and the next inside it are escaped. Unescaped, a next -// containing & would end the redirect_uri value early and the rest would -// reach GitHub as further authorize parameters (#631). -func GithubAuthorizeURL(origin, clientID, next string) string { - redirect := origin + "/login" - if next != "" && next != "/" { - redirect += "?next=" + url.QueryEscape(next) - } +// state ties the authorize request to the browser that made it: GithubCallback +// will not exchange a code unless it comes back unchanged (#637). +func GithubAuthorizeURL(origin, clientID, state string) string { return "https://github.com/login/oauth/authorize?client_id=" + url.QueryEscape(clientID) + - "&redirect_uri=" + url.QueryEscape(redirect) + "&redirect_uri=" + url.QueryEscape(origin+"/login") + + "&state=" + url.QueryEscape(state) } -// GithubLogin serves /login/github: it sends the visitor to GitHub rather than +// GithubLogin serves /login/github: it starts the OAuth flow rather than // having each theme assemble the authorize URL in an inline script, where the // escaping was wrong and had to be fixed in every theme separately (#631). +// +// The state it mints, and where the visitor was heading, are kept in the +// session for GithubCallback to check when GitHub returns (#637). func (b *Blog) GithubLogin(c *gin.Context) { if err := godotenv.Load(".env"); err != nil { _ = godotenv.Load("local.env") @@ -1504,8 +1503,26 @@ func (b *Blog) GithubLogin(c *gin.Context) { c.Redirect(http.StatusFound, "/login") return } - next := SafeNext(c.Query("next")) - c.Redirect(http.StatusFound, GithubAuthorizeURL(RequestOrigin(c), clientID, next)) + + state, err := auth.NewOAuthState() + if err != nil { + log.Println("Couldn't generate an OAuth state: " + err.Error()) + c.Redirect(http.StatusFound, "/login?error=github") + return + } + + session := sessions.Default(c) + session.Set(auth.OAuthStateKey, state) + session.Set(auth.OAuthNextKey, SafeNext(c.Query("next"))) + if err := session.Save(); err != nil { + // Without a stored state the callback would reject the login, so + // there is no point sending them to GitHub. + log.Println("Couldn't save the OAuth state: " + err.Error()) + c.Redirect(http.StatusFound, "/login?error=github") + return + } + + c.Redirect(http.StatusFound, GithubAuthorizeURL(RequestOrigin(c), clientID, state)) } func (b *Blog) Logout(c *gin.Context) { diff --git a/blog/github_login_test.go b/blog/github_login_test.go index f1a7d7bf..1650ddb5 100644 --- a/blog/github_login_test.go +++ b/blog/github_login_test.go @@ -7,65 +7,128 @@ import ( "strings" "testing" + "github.com/gin-contrib/sessions" + "github.com/gin-contrib/sessions/cookie" "github.com/gin-gonic/gin" + "goblog/auth" "goblog/blog" ) // The login page used to build GitHub's authorize URL in an inline script, -// concatenating window.location straight into redirect_uri. #631 moved it into -// Go so the escaping is done once and can be tested. +// concatenating window.location into redirect_uri (#631). It is built in Go +// now, and carries a state that ties the request to this browser (#637). -// redirectURI pulls redirect_uri back out of an authorize URL, decoded. -func redirectURI(t *testing.T, authorize string) string { - t.Helper() - u, err := url.Parse(authorize) +func TestGithubAuthorizeURL_HasAConstantRedirectAndAState(t *testing.T) { + got := blog.GithubAuthorizeURL("https://example.com", "abc123", "st4te") + + u, err := url.Parse(got) if err != nil { t.Fatalf("authorize URL does not parse: %v", err) } - return u.Query().Get("redirect_uri") -} - -func TestGithubAuthorizeURL_KeepsNextThroughTheRoundTrip(t *testing.T) { - got := blog.GithubAuthorizeURL("https://example.com", "abc123", "/admin/settings") - if want := "https://example.com/login?next=%2Fadmin%2Fsettings"; redirectURI(t, got) != want { - t.Errorf("redirect_uri = %q, want %q", redirectURI(t, got), want) + q := u.Query() + if len(q) != 3 { + t.Errorf("authorize URL has %d parameters, want client_id, redirect_uri and state: %v", len(q), q) + } + // GitHub matches redirect_uri against the callback registered for the + // app, so it must not vary with where the visitor was heading. + if want := "https://example.com/login"; q.Get("redirect_uri") != want { + t.Errorf("redirect_uri = %q, want the constant %q", q.Get("redirect_uri"), want) } - // GitHub returns the visitor to redirect_uri with &code= appended, and - // Login reads next from that query. Dropping it would send everyone to /. - if !strings.Contains(redirectURI(t, got), "next=") { - t.Error("redirect_uri must keep ?next=, it is how next survives the round trip") + if q.Get("state") != "st4te" { + t.Errorf("state = %q, want it passed through", q.Get("state")) } } -// TestGithubAuthorizeURL_EscapesAmpersandInNext is the bug in #631: unescaped, -// everything after the & in next reaches GitHub as further authorize -// parameters and redirect_uri is truncated. -func TestGithubAuthorizeURL_EscapesAmpersandInNext(t *testing.T) { - got := blog.GithubAuthorizeURL("https://example.com", "abc123", "/search?q=a&b=c") +// TestGithubAuthorizeURL_EscapesItsParameters is the bug in #631: unescaped, a +// value containing & would end its parameter early and the rest would reach +// GitHub as further authorize parameters. +func TestGithubAuthorizeURL_EscapesItsParameters(t *testing.T) { + got := blog.GithubAuthorizeURL("https://example.com", "id&injected=1", "st&ate") u, err := url.Parse(got) if err != nil { t.Fatalf("authorize URL does not parse: %v", err) } q := u.Query() - if len(q) != 2 { - t.Errorf("authorize URL has %d parameters, want exactly client_id and redirect_uri: %v", len(q), q) + if len(q) != 3 { + t.Errorf("a value smuggled in an extra parameter: %v", q) + } + if q.Get("client_id") != "id&injected=1" || q.Get("state") != "st&ate" { + t.Errorf("values did not survive escaping: client_id=%q state=%q", q.Get("client_id"), q.Get("state")) + } +} + +// newSessionRouter wires the cookie session store the real server uses, so +// these tests exercise the same Set/Save path as production. +func newSessionRouter(t *testing.T) *gin.Engine { + t.Helper() + gin.SetMode(gin.TestMode) + r := gin.New() + r.Use(sessions.Sessions("goblog", cookie.NewStore([]byte("test-session-key")))) + return r +} + +// startLogin runs GET /login/github and returns the redirect target and the +// cookies it set. +func startLogin(t *testing.T, target string) (string, []*http.Cookie) { + t.Helper() + t.Setenv("client_id", "abc123") + b := blog.New(nil, nil, "test") + r := newSessionRouter(t) + r.GET("/login/github", b.GithubLogin) + + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, target, nil)) + if w.Code != http.StatusFound { + t.Fatalf("code = %d, want 302", w.Code) + } + return w.Header().Get("Location"), w.Result().Cookies() +} + +func TestGithubLogin_SendsAStateAndKeepsItInTheSession(t *testing.T) { + loc, cookies := startLogin(t, "/login/github?next=/admin/settings") + + u, _ := url.Parse(loc) + if !strings.HasPrefix(loc, "https://github.com/login/oauth/authorize") { + t.Fatalf("did not redirect to GitHub: %s", loc) } - if want := "https://example.com/login?next=%2Fsearch%3Fq%3Da%26b%3Dc"; q.Get("redirect_uri") != want { - t.Errorf("redirect_uri = %q, want %q", q.Get("redirect_uri"), want) + if u.Query().Get("state") == "" { + t.Error("authorize URL carries no state, so the callback cannot tie the code to this browser") } - // The raw string must not carry a bare & or ? from next. - raw := got[strings.Index(got, "redirect_uri="):] - if strings.Contains(raw, "&b=c") || strings.Contains(raw, "?q=") { - t.Errorf("next is not escaped inside redirect_uri: %s", raw) + // next is no longer in redirect_uri; it rides in the session. + if strings.Contains(u.Query().Get("redirect_uri"), "next") { + t.Errorf("redirect_uri should be constant, got %q", u.Query().Get("redirect_uri")) + } + if len(cookies) == 0 { + t.Error("no session cookie set, so the state was never stored") } } -func TestGithubAuthorizeURL_OmitsEmptyNext(t *testing.T) { - for _, next := range []string{"", "/"} { - got := redirectURI(t, blog.GithubAuthorizeURL("https://example.com", "abc", next)) - if got != "https://example.com/login" { - t.Errorf("next %q: redirect_uri = %q, want no query string", next, got) +// TestGithubLogin_StateDiffersEveryTime: a predictable state would defeat the +// point of having one. +func TestGithubLogin_StateDiffersEveryTime(t *testing.T) { + seen := map[string]bool{} + for i := 0; i < 8; i++ { + loc, _ := startLogin(t, "/login/github") + u, _ := url.Parse(loc) + state := u.Query().Get("state") + if len(state) < 20 { + t.Fatalf("state %q is too short to be unguessable", state) + } + if seen[state] { + t.Fatalf("state %q was issued twice", state) + } + seen[state] = true + } +} + +// TestGithubLogin_RejectsOffsiteNext: next becomes the post-login redirect, so +// an unchecked value here is an open redirect. +func TestGithubLogin_RejectsOffsiteNext(t *testing.T) { + for _, next := range []string{"https://evil.example", "//evil.example", "/\\evil.example"} { + loc, _ := startLogin(t, "/login/github?next="+url.QueryEscape(next)) + if strings.Contains(loc, "evil.example") { + t.Errorf("next %q reached the authorize URL: %s", next, loc) } } } @@ -96,25 +159,10 @@ func TestRequestOrigin(t *testing.T) { } } -// TestGithubLogin_RejectsOffsiteNext: next reaches redirect_uri, so an open -// redirect here would be handed to GitHub. SafeNext already guards Login; it -// has to guard this entry point too. -func TestGithubLogin_RejectsOffsiteNext(t *testing.T) { - t.Setenv("client_id", "abc123") - b := blog.New(nil, nil, "test") - - for _, next := range []string{"https://evil.example", "//evil.example", "/\\evil.example"} { - router := gin.New() - router.GET("/login/github", b.GithubLogin) - w := httptest.NewRecorder() - router.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/login/github?next="+url.QueryEscape(next), nil)) - - if w.Code != http.StatusFound { - t.Fatalf("next %q: code = %d, want 302", next, w.Code) - } - got := redirectURI(t, w.Header().Get("Location")) - if strings.Contains(got, "evil.example") { - t.Errorf("next %q leaked into redirect_uri: %s", next, got) - } +// TestOAuthSessionKeys guards the contract between the two packages: blog +// writes these, auth reads them. +func TestOAuthSessionKeys(t *testing.T) { + if auth.OAuthStateKey == "" || auth.OAuthNextKey == "" || auth.OAuthStateKey == auth.OAuthNextKey { + t.Errorf("session keys are not distinct and non-empty: %q %q", auth.OAuthStateKey, auth.OAuthNextKey) } } diff --git a/csrf_test.go b/csrf_test.go index 9f5275b5..5b060bc9 100644 --- a/csrf_test.go +++ b/csrf_test.go @@ -3,6 +3,7 @@ package main import ( "net/http" "net/http/httptest" + "os" "strings" "testing" @@ -34,7 +35,6 @@ func TestRequireJSON(t *testing.T) { r.POST("/api/v1/upload", ok) r.POST("/api/v1/directory/repos/:id/approve", ok) r.GET("/api/v1/posts", ok) - r.POST("/api/login", ok) r.POST("/wizard_db", ok) cases := []struct { @@ -56,7 +56,6 @@ func TestRequireJSON(t *testing.T) { {"multipart upload allowed", "POST", "/api/v1/upload", "multipart/form-data; boundary=x", "--x--", 200}, {"multipart elsewhere rejected", "POST", "/api/v1/posts", "multipart/form-data; boundary=x", "--x--", 415}, {"GET untouched", "GET", "/api/v1/posts", "", "", 200}, - {"login form untouched", "POST", "/api/login", "application/x-www-form-urlencoded", "a=b", 200}, {"wizard untouched", "POST", "/wizard_db", "application/x-www-form-urlencoded", "a=b", 200}, } for _, tc := range cases { @@ -71,3 +70,29 @@ func TestRequireJSON(t *testing.T) { } } } + +// TestNoRawOAuthCodeEndpoint: /api/login took an OAuth code as a form post and +// set the session from it, and requireJSON deliberately let it through, so a +// cross-site form could plant a session on a visitor. SameSite=Lax does not +// help — it governs sending a cookie cross-site, not setting one. GitHub +// returns to /login now and the server does the exchange itself, against a +// state it minted (#637), so nothing accepts a bare code any more. +func TestNoRawOAuthCodeEndpoint(t *testing.T) { + source, err := os.ReadFile("goblog.go") + if err != nil { + t.Fatalf("read goblog.go: %v", err) + } + // The closing quote matters: "/api/login/email" does not contain + // `"/api/login"`, so the OTP routes below are not caught by this. + if strings.Contains(string(source), `"/api/login"`) { + t.Error(`/api/login is routed again: it accepts an OAuth code from anywhere, which is the login CSRF #637 closed`) + } + // Asserted positively so the check above is demonstrably about the OAuth + // route alone, and so removing it does not quietly take the OTP endpoints + // with it: those are a different flow and still wanted. + for _, keep := range []string{`"/api/login/email"`, `"/api/login/email/verify"`} { + if !strings.Contains(string(source), keep) { + t.Errorf("%s is no longer routed; email login needs it", keep) + } + } +} diff --git a/goblog.go b/goblog.go index d578963b..c6c7c572 100644 --- a/goblog.go +++ b/goblog.go @@ -200,6 +200,11 @@ func (g *goblog) loginHandler(c *gin.Context) { g.addRoutes() g._blog.Home(c) } + } else if c.Query("code") != "" { + // GitHub's redirect_uri is /login, so its return lands here. The + // exchange happens server-side against the state minted by + // /login/github; the code never reaches the page (#637). + g._auth.GithubCallback(c) } else { g._blog.Login(c) } @@ -491,7 +496,10 @@ func (g *goblog) addRoutesInner() { log.Println("Adding main blog routes") //all of this is the json api g.router.MaxMultipartMemory = 50 << 20 - g.router.POST("/api/login", g._auth.LoginPostHandler) + // No /api/login: it took a raw OAuth code as a form post and set the + // session from it, so a cross-site form could plant a session on a + // visitor. GitHub returns to /login and the server does the exchange + // itself, against a state it minted (#637). g.router.POST("/api/login/email", g._auth.SendLoginCodeHandler) g.router.POST("/api/login/email/verify", g._auth.VerifyLoginCodeHandler) g.router.POST("/api/v1/posts", g._admin.CreatePost) diff --git a/themes/default/templates/login.html b/themes/default/templates/login.html index c6bd7833..d7a24a05 100644 --- a/themes/default/templates/login.html +++ b/themes/default/templates/login.html @@ -5,20 +5,8 @@ // script has run by the time DOMContentLoaded fires, which also makes the // old $(function () { ... }) wrapper redundant. document.addEventListener("DOMContentLoaded", function () { - var host = location.protocol + '//' + window.location.host - var urlParams = new URLSearchParams(window.location.search); - var code = urlParams.get('code'); // Where to go after logging in; validated server-side to a same-site path. var next = {{ .next }}; - - if (code) { - //todo: get the post to be tied to the uri of the page (so it works on localhost, dev and prod) - var post = $.post(host + "/api/login", {"code": code}); - post.done(function(data) { - window.location.href = next; - }); - } - var emailForm = $("#email-form"), codeForm = $("#code-form"), status = $("#email-login-status"), change = $("#email-change"); if (!emailForm.length) { return; } @@ -28,7 +16,7 @@ emailForm.on("submit", function (e) { e.preventDefault(); status.text(""); - $.post(host + "/api/login/email", {email: $("#email-input").val()}) + $.post("/api/login/email", {email: $("#email-input").val()}) .done(function () { emailForm.hide(); codeForm.css("display", "flex"); change.show(); status.text("Check your inbox for a 6-digit code."); @@ -39,7 +27,7 @@ codeForm.on("submit", function (e) { e.preventDefault(); status.text(""); - $.post(host + "/api/login/email/verify", {email: $("#email-input").val(), code: $("#code-input").val()}) + $.post("/api/login/email/verify", {email: $("#email-input").val(), code: $("#code-input").val()}) .done(function () { window.location.href = next; }) .fail(function (xhr) { showError(xhr, "Could not verify the code."); }); });