Skip to content

Commit 52c7575

Browse files
EmyrkEmyrk
committed
feat: Implement CSRF in cli client and FE api
1 parent 5362f46 commit 52c7575

15 files changed

+119
-96
lines changed

‎coderd/coderd.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ func New(options *Options) *API {
130130
httpmw.Recover(api.Logger),
131131
httpmw.Logger(api.Logger),
132132
httpmw.Prometheus(options.PrometheusRegistry),
133+
httpmw.CSRF(options.SecureAuthCookie),
133134
)
134135

135136
apps := func(r chi.Router) {

‎coderd/httpmw/apikey.go

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,12 +82,29 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs, redirectToLogin bool
8282
}
8383

8484
var cookieValue string
85+
// Find the session token from:
86+
// 1: The cookie
87+
// 2: The old cookie
88+
// 3. The coder_session_token query parameter
89+
// 4. The custom auth header
8590
cookie, err := r.Cookie(codersdk.SessionTokenKey)
8691
if err != nil {
87-
cookieValue = r.URL.Query().Get(codersdk.SessionTokenKey)
92+
// TODO: @emyrk in October 2022, remove this oldCookie check.
93+
// This is just to support the old cli for 1 release. Then everyone
94+
// must update.
95+
oldCookie, err := r.Cookie("session_token")
96+
if err != nil {
97+
cookieValue = r.URL.Query().Get(codersdk.SessionTokenKey)
98+
if cookieValue == "" {
99+
cookieValue = r.Header.Get(codersdk.SessionCustomHeader)
100+
}
101+
} else {
102+
cookieValue = oldCookie.Value
103+
}
88104
} else {
89105
cookieValue = cookie.Value
90106
}
107+
91108
if cookieValue == "" {
92109
write(http.StatusUnauthorized, codersdk.Response{
93110
Message: signedOutErrorMessage,

‎coderd/httpmw/apikey_test.go

Lines changed: 12 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,7 @@ func TestAPIKey(t *testing.T) {
7474
r = httptest.NewRequest("GET", "/", nil)
7575
rw = httptest.NewRecorder()
7676
)
77-
r.AddCookie(&http.Cookie{
78-
Name: codersdk.SessionTokenKey,
79-
Value: "test-wow-hello",
80-
})
77+
r.Header.Set(codersdk.SessionCustomHeader, "test-wow-hello")
8178

8279
httpmw.ExtractAPIKey(db, nil, false)(successHandler).ServeHTTP(rw, r)
8380
res := rw.Result()
@@ -92,10 +89,7 @@ func TestAPIKey(t *testing.T) {
9289
r = httptest.NewRequest("GET", "/", nil)
9390
rw = httptest.NewRecorder()
9491
)
95-
r.AddCookie(&http.Cookie{
96-
Name: codersdk.SessionTokenKey,
97-
Value: "test-wow",
98-
})
92+
r.Header.Set(codersdk.SessionCustomHeader, "test-wow")
9993

10094
httpmw.ExtractAPIKey(db, nil, false)(successHandler).ServeHTTP(rw, r)
10195
res := rw.Result()
@@ -110,10 +104,7 @@ func TestAPIKey(t *testing.T) {
110104
r = httptest.NewRequest("GET", "/", nil)
111105
rw = httptest.NewRecorder()
112106
)
113-
r.AddCookie(&http.Cookie{
114-
Name: codersdk.SessionTokenKey,
115-
Value: "testtestid-wow",
116-
})
107+
r.Header.Set(codersdk.SessionCustomHeader, "testtestid-wow")
117108

118109
httpmw.ExtractAPIKey(db, nil, false)(successHandler).ServeHTTP(rw, r)
119110
res := rw.Result()
@@ -129,10 +120,7 @@ func TestAPIKey(t *testing.T) {
129120
r = httptest.NewRequest("GET", "/", nil)
130121
rw = httptest.NewRecorder()
131122
)
132-
r.AddCookie(&http.Cookie{
133-
Name: codersdk.SessionTokenKey,
134-
Value: fmt.Sprintf("%s-%s", id, secret),
135-
})
123+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
136124

137125
httpmw.ExtractAPIKey(db, nil, false)(successHandler).ServeHTTP(rw, r)
138126
res := rw.Result()
@@ -149,10 +137,7 @@ func TestAPIKey(t *testing.T) {
149137
rw = httptest.NewRecorder()
150138
user = createUser(r.Context(), t, db)
151139
)
152-
r.AddCookie(&http.Cookie{
153-
Name: codersdk.SessionTokenKey,
154-
Value: fmt.Sprintf("%s-%s", id, secret),
155-
})
140+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
156141

157142
// Use a different secret so they don't match!
158143
hashed := sha256.Sum256([]byte("differentsecret"))
@@ -178,10 +163,7 @@ func TestAPIKey(t *testing.T) {
178163
rw = httptest.NewRecorder()
179164
user = createUser(r.Context(), t, db)
180165
)
181-
r.AddCookie(&http.Cookie{
182-
Name: codersdk.SessionTokenKey,
183-
Value: fmt.Sprintf("%s-%s", id, secret),
184-
})
166+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
185167

186168
_, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
187169
ID: id,
@@ -206,10 +188,7 @@ func TestAPIKey(t *testing.T) {
206188
rw = httptest.NewRecorder()
207189
user = createUser(r.Context(), t, db)
208190
)
209-
r.AddCookie(&http.Cookie{
210-
Name: codersdk.SessionTokenKey,
211-
Value: fmt.Sprintf("%s-%s", id, secret),
212-
})
191+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
213192

214193
sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
215194
ID: id,
@@ -280,10 +259,7 @@ func TestAPIKey(t *testing.T) {
280259
rw = httptest.NewRecorder()
281260
user = createUser(r.Context(), t, db)
282261
)
283-
r.AddCookie(&http.Cookie{
284-
Name: codersdk.SessionTokenKey,
285-
Value: fmt.Sprintf("%s-%s", id, secret),
286-
})
262+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
287263

288264
sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
289265
ID: id,
@@ -316,10 +292,7 @@ func TestAPIKey(t *testing.T) {
316292
rw = httptest.NewRecorder()
317293
user = createUser(r.Context(), t, db)
318294
)
319-
r.AddCookie(&http.Cookie{
320-
Name: codersdk.SessionTokenKey,
321-
Value: fmt.Sprintf("%s-%s", id, secret),
322-
})
295+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
323296

324297
sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
325298
ID: id,
@@ -352,10 +325,7 @@ func TestAPIKey(t *testing.T) {
352325
rw = httptest.NewRecorder()
353326
user = createUser(r.Context(), t, db)
354327
)
355-
r.AddCookie(&http.Cookie{
356-
Name: codersdk.SessionTokenKey,
357-
Value: fmt.Sprintf("%s-%s", id, secret),
358-
})
328+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
359329

360330
sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
361331
ID: id,
@@ -395,10 +365,7 @@ func TestAPIKey(t *testing.T) {
395365
rw = httptest.NewRecorder()
396366
user = createUser(r.Context(), t, db)
397367
)
398-
r.AddCookie(&http.Cookie{
399-
Name: codersdk.SessionTokenKey,
400-
Value: fmt.Sprintf("%s-%s", id, secret),
401-
})
368+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
402369

403370
sentAPIKey, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
404371
ID: id,
@@ -449,10 +416,7 @@ func TestAPIKey(t *testing.T) {
449416
user = createUser(r.Context(), t, db)
450417
)
451418
r.RemoteAddr = "1.1.1.1:3555"
452-
r.AddCookie(&http.Cookie{
453-
Name: codersdk.SessionTokenKey,
454-
Value: fmt.Sprintf("%s-%s", id, secret),
455-
})
419+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
456420

457421
_, err := db.InsertAPIKey(r.Context(), database.InsertAPIKeyParams{
458422
ID: id,

‎coderd/httpmw/authorize_test.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,7 @@ func TestExtractUserRoles(t *testing.T) {
9393
})
9494

9595
req := httptest.NewRequest("GET", "/", nil)
96-
req.AddCookie(&http.Cookie{
97-
Name: codersdk.SessionTokenKey,
98-
Value: token,
99-
})
96+
req.Header.Set(codersdk.SessionCustomHeader, token)
10097

10198
rtr.ServeHTTP(rw, req)
10299
resp := rw.Result()

‎coderd/httpmw/csrf.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
package httpmw
2+
3+
import (
4+
"net/http"
5+
"regexp"
6+
7+
"github.com/justinas/nosurf"
8+
"golang.org/x/xerrors"
9+
10+
"github.com/coder/coder/codersdk"
11+
)
12+
13+
// CSRF is a middleware that verifies that a CSRF token is present in the request
14+
// for non-GET requests.
15+
func CSRF(secureCookie bool) func(next http.Handler) http.Handler {
16+
return func(next http.Handler) http.Handler {
17+
mw := nosurf.New(next)
18+
mw.SetBaseCookie(http.Cookie{Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: secureCookie})
19+
mw.SetFailureHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
20+
http.Error(w, "Something is wrong with your CSRF token. Please refresh the page. If this error persists, try clearing your cookies.", http.StatusBadRequest)
21+
}))
22+
23+
// Exempt all requests that do not require CSRF protection.
24+
// All GET requests are exempt by default.
25+
mw.ExemptPath("/api/v2/csp/reports")
26+
27+
// Top level agent routes.
28+
mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/[^/]*$"))
29+
// Agent authenticated routes
30+
mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/me/*"))
31+
32+
mw.ExemptFunc(func(r *http.Request) bool {
33+
// CSRF only affects requests that automatically attach credentials via a cookie.
34+
// If no cookie is present, then there is no risk of CSRF.
35+
sessCookie, err := r.Cookie(codersdk.SessionTokenKey)
36+
if xerrors.Is(err, http.ErrNoCookie) {
37+
return true
38+
}
39+
40+
if token := r.Header.Get(codersdk.SessionCustomHeader); token == sessCookie.Value {
41+
// If the cookie and header match, we can assume this is the same as just using the
42+
// custom header auth. Custom header auth can bypass CSRF, as CSRF attacks
43+
// cannot add custom headers.
44+
return true
45+
}
46+
47+
if token := r.URL.Query().Get(codersdk.SessionTokenKey); token == sessCookie.Value {
48+
// If the auth is set in a url param and matches the cookie, it
49+
// is the same as just using the url param.
50+
return true
51+
}
52+
53+
// If the X-CSRF-TOKEN header is set, we can exempt the func if it's valid.
54+
// This is the CSRF check.
55+
sent := r.Header.Get("X-CSRF-TOKEN")
56+
if sent != "" {
57+
return nosurf.VerifyToken(nosurf.Token(r), sent)
58+
}
59+
return false
60+
})
61+
return mw
62+
}
63+
}

‎coderd/httpmw/organizationparam_test.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ func TestOrganizationParam(t *testing.T) {
2929
r = httptest.NewRequest("GET", "/", nil)
3030
hashed = sha256.Sum256([]byte(secret))
3131
)
32-
r.AddCookie(&http.Cookie{
33-
Name: codersdk.SessionTokenKey,
34-
Value: fmt.Sprintf("%s-%s", id, secret),
35-
})
32+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
3633

3734
userID := uuid.New()
3835
username, err := cryptorand.String(8)

‎coderd/httpmw/templateparam_test.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ func TestTemplateParam(t *testing.T) {
2929
hashed = sha256.Sum256([]byte(secret))
3030
)
3131
r := httptest.NewRequest("GET", "/", nil)
32-
r.AddCookie(&http.Cookie{
33-
Name: codersdk.SessionTokenKey,
34-
Value: fmt.Sprintf("%s-%s", id, secret),
35-
})
32+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
3633

3734
userID := uuid.New()
3835
username, err := cryptorand.String(8)

‎coderd/httpmw/templateversionparam_test.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ func TestTemplateVersionParam(t *testing.T) {
2929
hashed = sha256.Sum256([]byte(secret))
3030
)
3131
r := httptest.NewRequest("GET", "/", nil)
32-
r.AddCookie(&http.Cookie{
33-
Name: codersdk.SessionTokenKey,
34-
Value: fmt.Sprintf("%s-%s", id, secret),
35-
})
32+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
3633

3734
userID := uuid.New()
3835
username, err := cryptorand.String(8)

‎coderd/httpmw/userparam_test.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ func TestUserParam(t *testing.T) {
2929
r = httptest.NewRequest("GET", "/", nil)
3030
rw = httptest.NewRecorder()
3131
)
32-
r.AddCookie(&http.Cookie{
33-
Name: codersdk.SessionTokenKey,
34-
Value: fmt.Sprintf("%s-%s", id, secret),
35-
})
32+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
3633

3734
user, err := db.InsertUser(r.Context(), database.InsertUserParams{
3835
ID: uuid.New(),

‎coderd/httpmw/workspaceagent_test.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,7 @@ func TestWorkspaceAgent(t *testing.T) {
2222
setup := func(db database.Store) (*http.Request, uuid.UUID) {
2323
token := uuid.New()
2424
r := httptest.NewRequest("GET", "/", nil)
25-
r.AddCookie(&http.Cookie{
26-
Name: codersdk.SessionTokenKey,
27-
Value: token.String(),
28-
})
25+
r.Header.Set(codersdk.SessionCustomHeader, token.String())
2926
return r, token
3027
}
3128

‎coderd/httpmw/workspaceagentparam_test.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ func TestWorkspaceAgentParam(t *testing.T) {
2929
hashed = sha256.Sum256([]byte(secret))
3030
)
3131
r := httptest.NewRequest("GET", "/", nil)
32-
r.AddCookie(&http.Cookie{
33-
Name: codersdk.SessionTokenKey,
34-
Value: fmt.Sprintf("%s-%s", id, secret),
35-
})
32+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
3633

3734
userID := uuid.New()
3835
username, err := cryptorand.String(8)

‎coderd/httpmw/workspacebuildparam_test.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ func TestWorkspaceBuildParam(t *testing.T) {
2929
hashed = sha256.Sum256([]byte(secret))
3030
)
3131
r := httptest.NewRequest("GET", "/", nil)
32-
r.AddCookie(&http.Cookie{
33-
Name: codersdk.SessionTokenKey,
34-
Value: fmt.Sprintf("%s-%s", id, secret),
35-
})
32+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
3633

3734
userID := uuid.New()
3835
username, err := cryptorand.String(8)

‎coderd/httpmw/workspaceparam_test.go

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,7 @@ func TestWorkspaceParam(t *testing.T) {
3232
hashed = sha256.Sum256([]byte(secret))
3333
)
3434
r := httptest.NewRequest("GET", "/", nil)
35-
r.AddCookie(&http.Cookie{
36-
Name: codersdk.SessionTokenKey,
37-
Value: fmt.Sprintf("%s-%s", id, secret),
38-
})
35+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
3936

4037
userID := uuid.New()
4138
username, err := cryptorand.String(8)
@@ -340,10 +337,7 @@ func setupWorkspaceWithAgents(t testing.TB, cfg setupConfig) (database.Store, *h
340337
hashed = sha256.Sum256([]byte(secret))
341338
)
342339
r := httptest.NewRequest("GET", "/", nil)
343-
r.AddCookie(&http.Cookie{
344-
Name: codersdk.SessionTokenKey,
345-
Value: fmt.Sprintf("%s-%s", id, secret),
346-
})
340+
r.Header.Set(codersdk.SessionCustomHeader, fmt.Sprintf("%s-%s", id, secret))
347341

348342
userID := uuid.New()
349343
username, err := cryptorand.String(8)

‎codersdk/client.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,11 @@ import (
2020
// Be sure to strip additional cookies in httpapi.StripCoder Cookies!
2121
const (
2222
// SessionTokenKey represents the name of the cookie or query parameter the API key is stored in.
23-
SessionTokenKey = "session_token"
24-
OAuth2StateKey = "oauth_state"
25-
OAuth2RedirectKey = "oauth_redirect"
23+
SessionTokenKey = "coder_session_token"
24+
// SessionCustomHeader is the custom header to use for authentication.
25+
SessionCustomHeader = "Coder-Session-Token"
26+
OAuth2StateKey = "oauth_state"
27+
OAuth2RedirectKey = "oauth_redirect"
2628
)
2729

2830
// New creates a Coder client for the provided URL.
@@ -70,10 +72,7 @@ func (c *Client) Request(ctx context.Context, method, path string, body interfac
7072
if err != nil {
7173
return nil, xerrors.Errorf("create request: %w", err)
7274
}
73-
req.AddCookie(&http.Cookie{
74-
Name: SessionTokenKey,
75-
Value: c.SessionToken,
76-
})
75+
req.Header.Set(SessionCustomHeader, c.SessionToken)
7776
if body != nil {
7877
req.Header.Set("Content-Type", "application/json")
7978
}

0 commit comments

Comments
 (0)