From 3d4ee87b67771df49e679e122adad6abf6933d62 Mon Sep 17 00:00:00 2001 From: Jason Ernst Date: Fri, 25 Sep 2026 13:19:52 -0700 Subject: [PATCH 1/2] Finish GitHub's OAuth flow server-side, against a state (#637) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The flow used to end 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. /login/github now mints an unguessable state, keeps it and the intended destination in the session, and sends the state to GitHub. GitHub's redirect_uri is already /login, so its return lands in loginHandler, which hands it to GithubCallback rather than rendering the page. The code is exchanged only if the state comes back unchanged, and the state is spent either way, so the same code and state cannot be presented twice. /api/login is gone. It was the hole rather than the page: it took a bare code as a form post and set the session from it, and requireJSON deliberately exempted it. SameSite=Lax does not help — it governs sending a cookie cross-site, not setting one — so a cross-site form could plant a session without the login page being involved at all. Nothing accepts a raw code now. #637 proposed a new /login/github/callback route, which would have changed redirect_uri and required the callback registered in each site's GitHub OAuth app to be updated. Handling the return at /login, where redirect_uri already points, gives the same property with no operational step. Along the way: - redirect_uri is constant now. GitHub matches it against the registered callback, so a value that does not vary is likelier to match; next rides in the session instead, which also keeps it out of a URL handed to a third party and logged. - requestAccessToken no longer fails outright when .env is missing. A deployment may set client_id and client_secret in the environment; what matters is having both, which it now checks. - The themes drop the ?code= block, which can no longer fire, and post the email-login endpoints relatively rather than rebuilding an origin. The callback tests are internal so they can point the token endpoint at a stand-in. That matters: asserting only that no session was created passes even with the state check removed, because an exchange that never reaches GitHub fails anyway. What separates a rejected callback from a failed one is whether the token endpoint was called at all, and with the check bypassed these fail with "a mismatched state logged someone in". Verified over HTTP against a local instance: a wrong state, and a code handed to a browser that never started a login, are both refused before any exchange; a matching state reaches the exchange; replaying it is refused; and POST /api/login is 404. Co-Authored-By: Claude Opus 5 (1M context) --- auth/auth.go | 57 ++++---- auth/github_oauth.go | 98 +++++++++++++ auth/github_oauth_internal_test.go | 214 ++++++++++++++++++++++++++++ blog/blog.go | 49 ++++--- blog/github_login_test.go | 158 +++++++++++++------- csrf_test.go | 19 ++- goblog.go | 10 +- themes/default/templates/login.html | 16 +-- 8 files changed, 507 insertions(+), 114 deletions(-) create mode 100644 auth/github_oauth.go create mode 100644 auth/github_oauth_internal_test.go 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..c92aba11 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,19 @@ 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) + } + 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`) + } +} 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."); }); }); From 25a712dbee101093bc1c96124c5d7409842b4875 Mon Sep 17 00:00:00 2001 From: Jason Ernst Date: Fri, 25 Sep 2026 13:24:48 -0700 Subject: [PATCH 2/2] Make the OAuth route guard's distinction explicit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review read the check as matching /api/login/email. It does not: the search string carries the closing quote, so "/api/login/email" is not a match — grep -c '"/api/login"' over goblog.go finds none, and the test passes. Kept the broad check, which catches the route however it is registered, and said why in a comment. The OTP routes are now asserted positively as well, so the distinction is demonstrated rather than argued, and so removing the OAuth route cannot quietly take email login with it. Co-Authored-By: Claude Opus 5 (1M context) --- csrf_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/csrf_test.go b/csrf_test.go index c92aba11..5b060bc9 100644 --- a/csrf_test.go +++ b/csrf_test.go @@ -82,7 +82,17 @@ func TestNoRawOAuthCodeEndpoint(t *testing.T) { 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) + } + } }