Skip to content

Commit be29b4a

Browse files
EmyrkEmyrk
committed
Add unit test to check for secure and samesite cookie flags
1 parent 493bddb commit be29b4a

File tree

7 files changed

+76
-16
lines changed

7 files changed

+76
-16
lines changed

‎cli/server.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -641,7 +641,6 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
641641
GoogleTokenValidator: googleTokenValidator,
642642
ExternalAuthConfigs: externalAuthConfigs,
643643
RealIPConfig: realIPConfig,
644-
Cookies: vals.HTTPCookies,
645644
SSHKeygenAlgorithm: sshKeygenAlgorithm,
646645
TracerProvider: tracerProvider,
647646
Telemetry: telemetry.NewNoop(),

‎coderd/apikey.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -382,7 +382,7 @@ func (api *API) createAPIKey(ctx context.Context, params apikey.CreateParams) (*
382382
APIKeys: []telemetry.APIKey{telemetry.ConvertAPIKey(newkey)},
383383
})
384384

385-
return api.Cookies.Apply(&http.Cookie{
385+
return api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{
386386
Name: codersdk.SessionTokenCookie,
387387
Value: sessionToken,
388388
Path: "/",

‎coderd/coderd.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,6 @@ type Options struct {
155155
GithubOAuth2Config *GithubOAuth2Config
156156
OIDCConfig *OIDCConfig
157157
PrometheusRegistry *prometheus.Registry
158-
Cookies codersdk.HTTPCookieConfig
159158
StrictTransportSecurityCfg httpmw.HSTSConfig
160159
SSHKeygenAlgorithm gitsshkey.Algorithm
161160
Telemetry telemetry.Reporter
@@ -828,7 +827,7 @@ func New(options *Options) *API {
828827
next.ServeHTTP(w, r)
829828
})
830829
},
831-
httpmw.CSRF(options.Cookies),
830+
httpmw.CSRF(options.DeploymentValues.HTTPCookies),
832831
)
833832

834833
// This incurs a performance hit from the middleware, but is required to make sure
@@ -868,7 +867,7 @@ func New(options *Options) *API {
868867
r.Route(fmt.Sprintf("/%s/callback", externalAuthConfig.ID), func(r chi.Router) {
869868
r.Use(
870869
apiKeyMiddlewareRedirect,
871-
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.Cookies, nil),
870+
httpmw.ExtractOAuth2(externalAuthConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
872871
)
873872
r.Get("/", api.externalAuthCallback(externalAuthConfig))
874873
})
@@ -1123,14 +1122,14 @@ func New(options *Options) *API {
11231122
r.Get("/github/device", api.userOAuth2GithubDevice)
11241123
r.Route("/github", func(r chi.Router) {
11251124
r.Use(
1126-
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.Cookies, nil),
1125+
httpmw.ExtractOAuth2(options.GithubOAuth2Config, options.HTTPClient, options.DeploymentValues.HTTPCookies, nil),
11271126
)
11281127
r.Get("/callback", api.userOAuth2Github)
11291128
})
11301129
})
11311130
r.Route("/oidc/callback", func(r chi.Router) {
11321131
r.Use(
1133-
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.Cookies, oidcAuthURLParams),
1132+
httpmw.ExtractOAuth2(options.OIDCConfig, options.HTTPClient, options.DeploymentValues.HTTPCookies, oidcAuthURLParams),
11341133
)
11351134
r.Get("/", api.userOIDC)
11361135
})

‎coderd/coderdtest/oidctest/idp.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1320,7 +1320,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler {
13201320
// requests will fail.
13211321
func (f *FakeIDP) HTTPClient(rest *http.Client) *http.Client {
13221322
if f.serve {
1323-
if rest == nil || rest.Transport == nil {
1323+
if rest == nil {
13241324
return &http.Client{}
13251325
}
13261326
return rest

‎coderd/coderdtest/testjar/cookiejar.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package testjar
2+
3+
import (
4+
"net/http"
5+
"net/url"
6+
"sync"
7+
)
8+
9+
func New() *Jar {
10+
return &Jar{}
11+
}
12+
13+
// Jar exists because 'cookiejar.New()' strips many of the http.Cookie fields
14+
// that are needed to assert. Such as 'Secure' and 'SameSite'.
15+
type Jar struct {
16+
m sync.Mutex
17+
perURL map[string][]*http.Cookie
18+
}
19+
20+
func (j *Jar) SetCookies(u *url.URL, cookies []*http.Cookie) {
21+
j.m.Lock()
22+
defer j.m.Unlock()
23+
if j.perURL == nil {
24+
j.perURL = make(map[string][]*http.Cookie)
25+
}
26+
j.perURL[u.Host] = append(j.perURL[u.Host], cookies...)
27+
}
28+
29+
func (j *Jar) Cookies(u *url.URL) []*http.Cookie {
30+
j.m.Lock()
31+
defer j.m.Unlock()
32+
return j.perURL[u.Host]
33+
}

‎coderd/userauth.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) {
204204
Path: "/",
205205
Value: token,
206206
Expires: claims.Expiry.Time(),
207-
Secure: api.Cookies.Secure.Value(),
207+
Secure: api.DeploymentValues.HTTPCookies.Secure.Value(),
208208
HttpOnly: true,
209209
// Must be SameSite to work on the redirected auth flow from the
210210
// oauth provider.
@@ -1913,13 +1913,12 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
19131913
slog.F("user_id", user.ID),
19141914
)
19151915
}
1916-
cookies = append(cookies, &http.Cookie{
1916+
cookies = append(cookies, api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{
19171917
Name: codersdk.SessionTokenCookie,
19181918
Path: "/",
19191919
MaxAge: -1,
1920-
Secure: api.Cookies.Secure.Value(),
19211920
HttpOnly: true,
1922-
})
1921+
}))
19231922
// This is intentional setting the key to the deleted old key,
19241923
// as the user needs to be forced to log back in.
19251924
key = *oldKey

‎coderd/userauth_test.go

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"crypto"
66
"crypto/rand"
7+
"crypto/tls"
78
"encoding/json"
89
"fmt"
910
"io"
@@ -33,6 +34,7 @@ import (
3334
"github.com/coder/coder/v2/coderd/audit"
3435
"github.com/coder/coder/v2/coderd/coderdtest"
3536
"github.com/coder/coder/v2/coderd/coderdtest/oidctest"
37+
"github.com/coder/coder/v2/coderd/coderdtest/testjar"
3638
"github.com/coder/coder/v2/coderd/cryptokeys"
3739
"github.com/coder/coder/v2/coderd/database"
3840
"github.com/coder/coder/v2/coderd/database/dbauthz"
@@ -66,8 +68,16 @@ func TestOIDCOauthLoginWithExisting(t *testing.T) {
6668
cfg.SecondaryClaims = coderd.MergedClaimsSourceNone
6769
})
6870

71+
certificates := []tls.Certificate{testutil.GenerateTLSCertificate(t, "localhost")}
6972
client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{
70-
OIDCConfig: cfg,
73+
OIDCConfig: cfg,
74+
TLSCertificates: certificates,
75+
DeploymentValues: coderdtest.DeploymentValues(t, func(values *codersdk.DeploymentValues) {
76+
values.HTTPCookies = codersdk.HTTPCookieConfig{
77+
Secure: true,
78+
SameSite: "none",
79+
}
80+
}),
7181
})
7282

7383
const username = "alice"
@@ -78,15 +88,35 @@ func TestOIDCOauthLoginWithExisting(t *testing.T) {
7888
"sub": uuid.NewString(),
7989
}
8090

81-
helper := oidctest.NewLoginHelper(client, fake)
8291
// Signup alice
83-
userClient, _ := helper.Login(t, claims)
92+
freshClient := func() *codersdk.Client {
93+
cli := codersdk.New(client.URL)
94+
cli.HTTPClient.Transport = &http.Transport{
95+
TLSClientConfig: &tls.Config{
96+
InsecureSkipVerify: true,
97+
},
98+
}
99+
cli.HTTPClient.Jar = testjar.New()
100+
return cli
101+
}
102+
103+
unauthenticated := freshClient()
104+
userClient, _ := fake.Login(t, unauthenticated, claims)
105+
106+
cookies := unauthenticated.HTTPClient.Jar.Cookies(client.URL)
107+
require.True(t, len(cookies) > 0)
108+
for _, c := range cookies {
109+
require.Truef(t, c.Secure, "cookie %q", c.Name)
110+
require.Equalf(t, http.SameSiteNoneMode, c.SameSite, "cookie %q", c.Name)
111+
}
84112

85113
// Expire the link. This will force the client to refresh the token.
114+
helper := oidctest.NewLoginHelper(userClient, fake)
86115
helper.ExpireOauthToken(t, api.Database, userClient)
87116

88117
// Instead of refreshing, just log in again.
89-
helper.Login(t, claims)
118+
unauthenticated = freshClient()
119+
fake.Login(t, unauthenticated, claims)
90120
}
91121

92122
func TestUserLogin(t *testing.T) {

0 commit comments

Comments
 (0)