Add unit test to check for secure and samesite cookie flags

coder · Emyrk · Apr 8, 2025 · Mar 11, 2025 · Mar 12, 2025 · Mar 13, 2025
commit a90eff73ef6ff53a4ab15bcce25f87d074d97467
diff --git a/cli/server.go b/cli/server.go
@@ -641,7 +641,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
 				GoogleTokenValidator:        googleTokenValidator,
 				ExternalAuthConfigs:         externalAuthConfigs,
 				RealIPConfig:                realIPConfig,
-				Cookies:                     vals.HTTPCookies,
 				SSHKeygenAlgorithm:          sshKeygenAlgorithm,
 				TracerProvider:              tracerProvider,
 				Telemetry:                   telemetry.NewNoop(),

diff --git a/coderd/apikey.go b/coderd/apikey.go
@@ -382,7 +382,7 @@ func (api *API) createAPIKey(ctx context.Context, params apikey.CreateParams) (*
 		APIKeys: []telemetry.APIKey{telemetry.ConvertAPIKey(newkey)},
 	})
 
-	return api.Cookies.Apply(&http.Cookie{
+	return api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{
 		Name:     codersdk.SessionTokenCookie,
 		Value:    sessionToken,
 		Path:     "/",

diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -155,7 +155,6 @@ type Options struct {
 	GithubOAuth2Config             *GithubOAuth2Config
 	OIDCConfig                     *OIDCConfig
 	PrometheusRegistry             *prometheus.Registry
-	Cookies                        codersdk.HTTPCookieConfig
 	StrictTransportSecurityCfg     httpmw.HSTSConfig
 	SSHKeygenAlgorithm             gitsshkey.Algorithm
 	Telemetry                      telemetry.Reporter
@@ -828,7 +827,7 @@ func New(options *Options) *API {
 				next.ServeHTTP(w, r)
 			})
 		},
-		httpmw.CSRF(options.Cookies),
+		httpmw.CSRF(options.DeploymentValues.HTTPCookies),
 	)
 
 	// This incurs a performance hit from the middleware, but is required to make sure
@@ -868,7 +867,7 @@ func New(options *Options) *API {
 				r.Route(fmt.Sprintf("/%s/callback", externalAuthConfig.ID), func(r chi.Router) {
 					r.Use(
 						apiKeyMiddlewareRedirect,
-						httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.Cookies, nil),
+						httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
 					)
 					r.Get("/", api.externalAuthCallback(externalAuthConfig))
 				})
@@ -1123,14 +1122,14 @@ func New(options *Options) *API {
 					r.Get("/github/device", api.userOAuth2GithubDevice)
 					r.Route("/github", func(r chi.Router) {
 						r.Use(
-							httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.Cookies, nil),
+							httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
 						)
 						r.Get("/callback", api.userOAuth2Github)
 					})
 				})
 				r.Route("/oidc/callback", func(r chi.Router) {
 					r.Use(
-						httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.Cookies, oidcAuthURLParams),
+						httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams),
 					)
 					r.Get("/", api.userOIDC)
 				})

diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go
@@ -1320,7 +1320,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
 // requests will fail.
 func (f *FakeIDP) HTTPClient(rest *http.Client) *http.Client {
 	if f.serve {
-		if rest == nil || rest.Transport == nil {
+		if rest == nil {
 			return &http.Client{}
 		}
 		return rest

diff --git a/coderd/coderdtest/testjar/cookiejar.go b/coderd/coderdtest/testjar/cookiejar.go
@@ -0,0 +1,33 @@
+package testjar
+
+import (
+	"net/http"
+	"net/url"
+	"sync"
+)
+
+func New() *Jar {
+	return &Jar{}
+}
+
+// Jar exists because 'cookiejar.New()' strips many of the http.Cookie fields
+// that are needed to assert. Such as 'Secure' and 'SameSite'.
+type Jar struct {
+	m      sync.Mutex
+	perURL map[string][]*http.Cookie
+}
+
+func (j *Jar) SetCookies(u *url.URL, cookies []*http.Cookie) {
+	j.m.Lock()
+	defer j.m.Unlock()
+	if j.perURL == nil {
+		j.perURL = make(map[string][]*http.Cookie)
+	}
+	j.perURL[u.Host] = append(j.perURL[u.Host], cookies...)
+}
+
+func (j *Jar) Cookies(u *url.URL) []*http.Cookie {
+	j.m.Lock()
+	defer j.m.Unlock()
+	return j.perURL[u.Host]
+}
diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -204,7 +204,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
 		Path:     "/",
 		Value:    token,
 		Expires:  claims.Expiry.Time(),
-		Secure:   api.Cookies.Secure.Value(),
+		Secure:   api.DeploymentValues.HTTPCookies.Secure.Value(),
 		HttpOnly: true,
 		// Must be SameSite to work on the redirected auth flow from the
 		// oauth provider.
@@ -1913,13 +1913,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 				slog.F("user_id", user.ID),
 			)
 		}
-		cookies = append(cookies, &http.Cookie{
+		cookies = append(cookies, api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{
 			Name:     codersdk.SessionTokenCookie,
 			Path:     "/",
 			MaxAge:   -1,
-			Secure:   api.Cookies.Secure.Value(),
 			HttpOnly: true,
-		})
+		}))
 		// This is intentional setting the key to the deleted old key,
 		// as the user needs to be forced to log back in.
 		key = *oldKey

diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto"
 	"crypto/rand"
+	"crypto/tls"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -33,6 +34,7 @@ import (
 	"github.com/coder/coder/v2/coderd/audit"
 	"github.com/coder/coder/v2/coderd/coderdtest"
 	"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
+	"github.com/coder/coder/v2/coderd/coderdtest/testjar"
 	"github.com/coder/coder/v2/coderd/cryptokeys"
 	"github.com/coder/coder/v2/coderd/database"
 	"github.com/coder/coder/v2/coderd/database/dbauthz"
@@ -66,8 +68,16 @@ func TestOIDCOauthLoginWithExisting(t *testing.T) {
 		cfg.SecondaryClaims = coderd.MergedClaimsSourceNone
 	})
 
+	certificates := []tls.Certificate{testutil.GenerateTLSCertificate(t, "localhost")}
 	client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{
-		OIDCConfig: cfg,
+		OIDCConfig:      cfg,
+		TLSCertificates: certificates,
+		DeploymentValues: coderdtest.DeploymentValues(t, func(values *codersdk.DeploymentValues) {
+			values.HTTPCookies = codersdk.HTTPCookieConfig{
+				Secure:   true,
+				SameSite: "none",
+			}
+		}),
 	})
 
 	const username = "alice"
@@ -78,15 +88,35 @@ func TestOIDCOauthLoginWithExisting(t *testing.T) {
 		"sub":                uuid.NewString(),
 	}
 
-	helper := oidctest.NewLoginHelper(client, fake)
 	// Signup alice
-	userClient, _ := helper.Login(t, claims)
+	freshClient := func() *codersdk.Client {
+		cli := codersdk.New(client.URL)
+		cli.HTTPClient.Transport = &http.Transport{
+			TLSClientConfig: &tls.Config{
+				InsecureSkipVerify: true,
+			},
+		}
+		cli.HTTPClient.Jar = testjar.New()
+		return cli
+	}
+
+	unauthenticated := freshClient()
+	userClient, _ := fake.Login(t, unauthenticated, claims)
+
+	cookies := unauthenticated.HTTPClient.Jar.Cookies(client.URL)
+	require.True(t, len(cookies) > 0)
+	for _, c := range cookies {
+		require.Truef(t, c.Secure, "cookie %q", c.Name)
+		require.Equalf(t, http.SameSiteNoneMode, c.SameSite, "cookie %q", c.Name)
+	}
 
 	// Expire the link. This will force the client to refresh the token.
+	helper := oidctest.NewLoginHelper(userClient, fake)
 	helper.ExpireOauthToken(t, api.Database, userClient)
 
 	// Instead of refreshing, just log in again.
-	helper.Login(t, claims)
+	unauthenticated = freshClient()
+	fake.Login(t, unauthenticated, claims)
 }
 
 func TestUserLogin(t *testing.T) {