diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 22d23001..54b798bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,22 @@ on: name: ci jobs: + lint: + name: Lint + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + - name: Install Go + uses: actions/setup-go@v5 + with: + go-version: '1.23.x' + - name: Run golangci-lint + uses: golangci/golangci-lint-action@v6 + with: + version: latest + args: --timeout=5m + test: strategy: matrix: @@ -24,7 +40,7 @@ jobs: - name: Checkout code uses: actions/checkout@v4 - name: Restore cache - uses: actions/cache@v3 + uses: actions/cache@v4 with: path: ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 00000000..915f8749 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,52 @@ +version: "2" + +run: + timeout: 5m + modules-download-mode: readonly + # Skip examples directory (demo code, not production) + exclude-dirs: + - ^examples$ + +linters: + default: standard + + settings: + errcheck: + exclude-functions: + - fmt.Fprint + - fmt.Fprintf + - fmt.Fprintln + - fmt.Printf + - fmt.Println + - (net/http.ResponseWriter).Write + - (*net/http.Request).ParseForm + - (*github.com/gorilla/sessions.Session).Save + - (*bytes.Buffer).ReadFrom + - io.Copy + - (io.Closer).Close + - os.Setenv + - (*encoding/json.Encoder).Encode + + staticcheck: + # Disable some noisy checks for legacy code + checks: + - "all" + - "-ST1000" # Package comments + - "-ST1003" # Naming conventions (clientId vs clientID) + - "-ST1005" # Error string capitalization + - "-S1011" # Loop simplification suggestions + +issues: + exclude-rules: + # Ignore unchecked errors in all tests + - path: _test\.go + linters: + - errcheck + + # Ignore ineffassign in providers (legacy code) + - path: providers/ + linters: + - ineffassign + + max-issues-per-linter: 50 + max-same-issues: 10 diff --git a/go.mod b/go.mod index 2e460bcc..9f546780 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/golang-jwt/jwt/v5 v5.2.2 github.com/gorilla/mux v1.6.2 github.com/gorilla/pat v0.0.0-20180118222023-199c85a7f6d1 - github.com/gorilla/sessions v1.1.1 + github.com/gorilla/sessions v1.4.0 github.com/jarcoal/httpmock v0.0.0-20180424175123-9c70cfe4a1da github.com/lestrrat-go/jwx v1.2.29 github.com/markbates/going v1.0.0 @@ -22,7 +22,7 @@ require ( github.com/decred/dcrd/dcrec/secp256k1/v4 v4.2.0 // indirect github.com/goccy/go-json v0.10.2 // indirect github.com/gorilla/context v1.1.1 // indirect - github.com/gorilla/securecookie v1.1.1 // indirect + github.com/gorilla/securecookie v1.1.2 // indirect github.com/kr/pretty v0.3.1 // indirect github.com/lestrrat-go/backoff/v2 v2.0.8 // indirect github.com/lestrrat-go/blackmagic v1.0.2 // indirect diff --git a/go.sum b/go.sum index 7bde5339..2b6fe819 100644 --- a/go.sum +++ b/go.sum @@ -13,16 +13,18 @@ github.com/goccy/go-json v0.10.2 h1:CrxCmQqYDkv1z7lO7Wbh2HN93uovUHgrECaO5ZrCXAU= github.com/goccy/go-json v0.10.2/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I= github.com/golang-jwt/jwt/v5 v5.2.2 h1:Rl4B7itRWVtYIHFrSNd7vhTiz9UpLdi6gZhZ3wEeDy8= github.com/golang-jwt/jwt/v5 v5.2.2/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk= +github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0= +github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/gorilla/context v1.1.1 h1:AWwleXJkX/nhcU9bZSnZoi3h/qGYqQAGhq6zZe/aQW8= github.com/gorilla/context v1.1.1/go.mod h1:kBGZzfjB9CEq2AlWe17Uuf7NDRt0dE0s8S51q0aT7Yg= github.com/gorilla/mux v1.6.2 h1:Pgr17XVTNXAk3q/r4CpKzC5xBM/qW1uVLV+IhRZpIIk= github.com/gorilla/mux v1.6.2/go.mod h1:1lud6UwP+6orDFRuTfBEV8e9/aOM/c4fVVCaMa2zaAs= github.com/gorilla/pat v0.0.0-20180118222023-199c85a7f6d1 h1:LqbZZ9sNMWVjeXS4NN5oVvhMjDyLhmA1LG86oSo+IqY= github.com/gorilla/pat v0.0.0-20180118222023-199c85a7f6d1/go.mod h1:YeAe0gNeiNT5hoiZRI4yiOky6jVdNvfO2N6Kav/HmxY= -github.com/gorilla/securecookie v1.1.1 h1:miw7JPhV+b/lAHSXz4qd/nN9jRiAFV5FwjeKyCS8BvQ= -github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+HVt/4epWDjd4= -github.com/gorilla/sessions v1.1.1 h1:YMDmfaK68mUixINzY/XjscuJ47uXFWSSHzFbBQM0PrE= -github.com/gorilla/sessions v1.1.1/go.mod h1:8KCfur6+4Mqcc6S0FEfKuN15Vl5MgXW92AE8ovaJD0w= +github.com/gorilla/securecookie v1.1.2 h1:YCIWL56dvtr73r6715mJs5ZvhtnY73hBvEF8kXD8ePA= +github.com/gorilla/securecookie v1.1.2/go.mod h1:NfCASbcHqRSY+3a8tlWJwsQap2VX5pwzwo4h3eOamfo= +github.com/gorilla/sessions v1.4.0 h1:kpIYOp/oi6MG/p5PgxApU8srsSw9tuFbt46Lt7auzqQ= +github.com/gorilla/sessions v1.4.0/go.mod h1:FLWm50oby91+hl7p/wRxDth9bWSuk0qVL2emc7lT5ik= github.com/jarcoal/httpmock v0.0.0-20180424175123-9c70cfe4a1da h1:FjHUJJ7oBW4G/9j1KzlHaXL09LyMVM9rupS39lncbXk= github.com/jarcoal/httpmock v0.0.0-20180424175123-9c70cfe4a1da/go.mod h1:ks+b9deReOc7jgqp+e7LuFiCBH6Rm5hL32cLcEAArb4= github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI= diff --git a/gothic/gothic.go b/gothic/gothic.go index b895a12d..e97a265a 100644 --- a/gothic/gothic.go +++ b/gothic/gothic.go @@ -175,7 +175,7 @@ var CompleteUserAuth = func(res http.ResponseWriter, req *http.Request) (goth.Us if err != nil { return goth.User{}, err } - defer Logout(res, req) + defer Logout(res, req) //nolint:errcheck // error cannot be handled in defer sess, err := provider.UnmarshalSession(value) if err != nil { return goth.User{}, err @@ -204,11 +204,11 @@ var CompleteUserAuth = func(res http.ResponseWriter, req *http.Request) (goth.Us return goth.User{}, err } - err = StoreInSession(providerName, sess.Marshal(), req, res) - - if err != nil { - return goth.User{}, err - } + // Note: We intentionally do NOT call StoreInSession here. + // The session is only used during the OAuth flow and is cleared by + // the deferred Logout call. Storing it would create a duplicate + // Set-Cookie header that gets immediately invalidated. + // See: https://github.com/markbates/goth/issues/626 gu, err := provider.FetchUser(sess) return gu, err diff --git a/gothic/gothic_test.go b/gothic/gothic_test.go index 22c8448a..f9f17118 100644 --- a/gothic/gothic_test.go +++ b/gothic/gothic_test.go @@ -290,3 +290,122 @@ func ungzipString(value string) string { return string(s) } + +// Test_StoreInSession_ReturnsErrorOnSaveFailure verifies that StoreInSession +// properly propagates errors from session.Save(). +// See: https://github.com/markbates/goth/issues/549 +func Test_StoreInSession_ReturnsErrorOnSaveFailure(t *testing.T) { + a := assert.New(t) + + // Use a store that always fails on save + originalStore := Store + Store = &failingStore{err: fmt.Errorf("session save failed: hash key is not set")} + defer func() { Store = originalStore }() + + res := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/auth?provider=faux", nil) + a.NoError(err) + + // StoreInSession should return the error from the failing store + err = StoreInSession("faux", "test-value", req, res) + if a.Error(err, "Expected error from failing store") { + a.Contains(err.Error(), "session save failed", "Error should be propagated from store") + } +} + +// Test_GetAuthURL_PropagatesSessionErrors verifies that GetAuthURL returns +// errors from session operations, such as when securecookie fails due to +// missing hash key. +// See: https://github.com/markbates/goth/issues/549 +func Test_GetAuthURL_PropagatesSessionErrors(t *testing.T) { + a := assert.New(t) + + // Use a store that fails on save (simulating securecookie with no hash key) + originalStore := Store + Store = &failingStore{err: fmt.Errorf("securecookie: hash key is not set")} + defer func() { Store = originalStore }() + + res := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/auth?provider=faux", nil) + a.NoError(err) + + // GetAuthURL should propagate the session error + _, err = GetAuthURL(res, req) + if a.Error(err, "Expected error when session save fails") { + a.Contains(err.Error(), "hash key is not set", "Original error should be propagated") + } +} + +// failingStore is a test store that always fails on Save +type failingStore struct { + err error +} + +func (f *failingStore) Get(r *http.Request, name string) (*sessions.Session, error) { + return sessions.NewSession(f, name), nil +} + +func (f *failingStore) New(r *http.Request, name string) (*sessions.Session, error) { + return sessions.NewSession(f, name), nil +} + +func (f *failingStore) Save(r *http.Request, w http.ResponseWriter, s *sessions.Session) error { + return f.err +} + +// Test_CompleteUserAuth_SingleSetCookie verifies that CompleteUserAuth only +// produces a single Set-Cookie header (the logout cookie), not two headers +// where one is valid and one is expired. +// See: https://github.com/markbates/goth/issues/626 +func Test_CompleteUserAuth_SingleSetCookie(t *testing.T) { + a := assert.New(t) + + // Use a real cookie store to test Set-Cookie header behavior + cookieStore := sessions.NewCookieStore([]byte("test-secret-key-32-bytes-long!!")) + cookieStore.Options.MaxAge = 86400 * 30 + originalStore := Store + Store = cookieStore + defer func() { Store = originalStore }() + + // Create request with session containing auth data + req, err := http.NewRequest("GET", "/auth/callback?provider=faux", nil) + a.NoError(err) + + // Set up session with provider data (simulating after BeginAuth) + sess := faux.Session{Name: "Homer Simpson", Email: "homer@example.com"} + session, _ := Store.New(req, SessionName) + session.Values["faux"] = gzipString(sess.Marshal()) + + // Save session and get the cookie to include in request + setupRes := httptest.NewRecorder() + err = session.Save(req, setupRes) + a.NoError(err) + + // Extract the session cookie from setup response + cookies := setupRes.Result().Cookies() + a.NotEmpty(cookies, "Expected session cookie to be set") + + // Create new request with the session cookie + req, err = http.NewRequest("GET", "/auth/callback?provider=faux", nil) + a.NoError(err) + for _, c := range cookies { + req.AddCookie(c) + } + + // Now call CompleteUserAuth + res := httptest.NewRecorder() + user, err := CompleteUserAuth(res, req) + a.NoError(err) + a.Equal("Homer Simpson", user.Name) + + // Count Set-Cookie headers - should be exactly 1 (the logout cookie) + setCookieHeaders := res.Result().Header["Set-Cookie"] + a.Len(setCookieHeaders, 1, "Expected exactly 1 Set-Cookie header, got %d: %v", len(setCookieHeaders), setCookieHeaders) + + // The single cookie should be the logout cookie (MaxAge=-1 or expires in past) + if len(setCookieHeaders) == 1 { + cookie := setCookieHeaders[0] + // Logout sets MaxAge=-1 which results in immediate expiry + a.Contains(cookie, "Max-Age=0", "Expected logout cookie with Max-Age=0") + } +}