From 658d7df3a66d5783646c5078e98ced09e878bbda Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 4 Jan 2024 17:50:22 -0600 Subject: [PATCH 1/9] fix: relax csrf to exclude path based apps --- coderd/coderd.go | 5 ++++- enterprise/wsproxy/wsproxy.go | 7 ++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 7de4e3207135f..da5d65b673bf5 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -579,7 +579,6 @@ func New(options *Options) *API { next.ServeHTTP(w, r) }) }, - httpmw.CSRF(options.SecureAuthCookie), ) r.Get("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("OK")) }) @@ -627,6 +626,10 @@ func New(options *Options) *API { // limit must be configurable by the admin. apiRateLimiter, httpmw.ReportCLITelemetry(api.Logger, options.Telemetry), + // CSRF only needs to apply to /api routes. It does not apply to GET requests + // anyway, which is most other routes. We want to exempt any external auth or + // application type routes. + httpmw.CSRF(options.SecureAuthCookie), ) r.Get("/", apiRoot) // All CSP errors will be logged diff --git a/enterprise/wsproxy/wsproxy.go b/enterprise/wsproxy/wsproxy.go index cb1671e133f03..242c3f14ed3db 100644 --- a/enterprise/wsproxy/wsproxy.go +++ b/enterprise/wsproxy/wsproxy.go @@ -329,9 +329,10 @@ func New(ctx context.Context, opts *Options) (*Server, error) { next.ServeHTTP(w, r) }) }, - // TODO: @emyrk we might not need this? But good to have if it does - // not break anything. - httpmw.CSRF(s.Options.SecureAuthCookie), + // CSRF middleware is intentionally excluded here. All coder requests + // which require CSRF protection are forwarded to the primary Coderd + // via a proxy function. Since the primary enforces this, the proxy does + // not. ) // Attach workspace apps routes. From fe4e2bafd9468c7ebabaeb8ebaa3dffb0f86eda6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 4 Jan 2024 18:19:45 -0600 Subject: [PATCH 2/9] add unit test to verify path based apps are not CSRF blocked --- coderd/coderd_test.go | 57 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 9823b2b62a123..2423d82098dc9 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -3,6 +3,7 @@ package coderd_test import ( "context" "flag" + "fmt" "io" "net/http" "net/netip" @@ -21,6 +22,9 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbfake" + "github.com/coder/coder/v2/provisionersdk/proto" "github.com/coder/coder/v2/agent/agenttest" "github.com/coder/coder/v2/buildinfo" @@ -315,3 +319,56 @@ func TestSwagger(t *testing.T) { require.Equal(t, "
\n
\n", string(body)) }) } + +func TestCSRFExempt(t *testing.T) { + t.Run("PathBasedApp", func(t *testing.T) { + t.Parallel() + + client, _, api := coderdtest.NewWithAPI(t, nil) + first := coderdtest.CreateFirstUser(t, client) + owner, err := client.User(context.Background(), "me") + require.NoError(t, err) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) + defer cancel() + + // Create a workspace. + const agentSlug = "james" + const appSlug = "web" + wrk := dbfake.WorkspaceBuild(t, api.Database, database.Workspace{ + OwnerID: owner.ID, + OrganizationID: first.OrganizationID, + }). + WithAgent(func(agents []*proto.Agent) []*proto.Agent { + agents[0].Name = agentSlug + agents[0].Apps = []*proto.App{{ + Slug: appSlug, + DisplayName: appSlug, + Subdomain: false, + Url: "/", + }} + + return agents + }). + Do() + + u := fmt.Sprintf(client.URL.JoinPath(fmt.Sprintf("/@%s/%s.%s/apps/%s", owner.Username, wrk.Workspace.Name, agentSlug, appSlug)).String()) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, u, nil) + req.AddCookie(&http.Cookie{ + Name: codersdk.SessionTokenCookie, + Value: client.SessionToken(), + Path: "/", + Domain: client.URL.String(), + }) + require.NoError(t, err) + + resp, err := client.HTTPClient.Do(req) + require.NoError(t, err) + data, _ := io.ReadAll(resp.Body) + + // A StatusBadGateway means Coderd tried to proxy to the agent and failed because the agent + // was not there. This means CSRF did not block the app request, which is what we want. + require.Equal(t, http.StatusBadGateway, resp.StatusCode, "status code 500 is CSRF failure") + require.NotContains(t, string(data), "CSRF") + }) +} From f2e615d8713e946c7c572c3324539d4e0e5bfcaa Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 4 Jan 2024 18:21:04 -0600 Subject: [PATCH 3/9] testing needs to be parallel --- coderd/coderd_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 2423d82098dc9..9e607592e97d7 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -321,6 +321,12 @@ func TestSwagger(t *testing.T) { } func TestCSRFExempt(t *testing.T) { + t.Parallel() + + // This test build a workspace with an agent and an app. The app is not + // a real http server, so it will fail to serve requests. We just want + // to make sure the failure is not a CSRF failure, as path based + // apps should be exempt. t.Run("PathBasedApp", func(t *testing.T) { t.Parallel() From a4bbc8036dff4071891a321022c33df8b97b96f7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 5 Jan 2024 09:55:18 -0600 Subject: [PATCH 4/9] Linting --- coderd/coderd_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 9e607592e97d7..8d7c12974650f 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -358,7 +358,7 @@ func TestCSRFExempt(t *testing.T) { }). Do() - u := fmt.Sprintf(client.URL.JoinPath(fmt.Sprintf("/@%s/%s.%s/apps/%s", owner.Username, wrk.Workspace.Name, agentSlug, appSlug)).String()) + u := client.URL.JoinPath(fmt.Sprintf("/@%s/%s.%s/apps/%s", owner.Username, wrk.Workspace.Name, agentSlug, appSlug)).String() req, err := http.NewRequestWithContext(ctx, http.MethodPost, u, nil) req.AddCookie(&http.Cookie{ Name: codersdk.SessionTokenCookie, @@ -371,6 +371,7 @@ func TestCSRFExempt(t *testing.T) { resp, err := client.HTTPClient.Do(req) require.NoError(t, err) data, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() // A StatusBadGateway means Coderd tried to proxy to the agent and failed because the agent // was not there. This means CSRF did not block the app request, which is what we want. From c6c4141eed87ccfb5145ac0d3a0e1211d1e582ac Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 5 Jan 2024 16:02:33 -0600 Subject: [PATCH 5/9] Try exempting first user --- coderd/httpmw/csrf.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/coderd/httpmw/csrf.go b/coderd/httpmw/csrf.go index 7888365741873..bdef62dd978b2 100644 --- a/coderd/httpmw/csrf.go +++ b/coderd/httpmw/csrf.go @@ -23,6 +23,9 @@ func CSRF(secureCookie bool) func(next http.Handler) http.Handler { // All GET requests are exempt by default. mw.ExemptPath("/api/v2/csp/reports") + // This should not be required? + mw.ExemptRegexp(regexp.MustCompile("/api/v2/users/first")) + // Agent authenticated routes mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/me/*")) mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/*")) From 043c79d2c233aff5a43c8f1cd4865d17381f82e0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 8 Jan 2024 10:20:09 -0600 Subject: [PATCH 6/9] we need to add csrf mw to set cookie --- coderd/coderd.go | 5 +---- coderd/httpmw/csrf.go | 10 +++++++++ coderd/httpmw/csrf_test.go | 39 +++++++++++++++++++++++++++++++++++ enterprise/wsproxy/wsproxy.go | 7 +++---- 4 files changed, 53 insertions(+), 8 deletions(-) create mode 100644 coderd/httpmw/csrf_test.go diff --git a/coderd/coderd.go b/coderd/coderd.go index 8e742ac5dd8cc..3f16c89cb0713 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -585,6 +585,7 @@ func New(options *Options) *API { next.ServeHTTP(w, r) }) }, + httpmw.CSRF(options.SecureAuthCookie), ) r.Get("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("OK")) }) @@ -632,10 +633,6 @@ func New(options *Options) *API { // limit must be configurable by the admin. apiRateLimiter, httpmw.ReportCLITelemetry(api.Logger, options.Telemetry), - // CSRF only needs to apply to /api routes. It does not apply to GET requests - // anyway, which is most other routes. We want to exempt any external auth or - // application type routes. - httpmw.CSRF(options.SecureAuthCookie), ) r.Get("/", apiRoot) // All CSP errors will be logged diff --git a/coderd/httpmw/csrf.go b/coderd/httpmw/csrf.go index bdef62dd978b2..d52d99c88fe88 100644 --- a/coderd/httpmw/csrf.go +++ b/coderd/httpmw/csrf.go @@ -3,6 +3,7 @@ package httpmw import ( "net/http" "regexp" + "strings" "github.com/justinas/nosurf" "golang.org/x/xerrors" @@ -12,6 +13,8 @@ import ( // CSRF is a middleware that verifies that a CSRF token is present in the request // for non-GET requests. +// If enforce is false, then CSRF enforcement is disabled. We still want +// to include the CSRF middleware because it will set the CSRF cookie. func CSRF(secureCookie bool) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { mw := nosurf.New(next) @@ -19,6 +22,9 @@ func CSRF(secureCookie bool) func(next http.Handler) http.Handler { mw.SetFailureHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.Error(w, "Something is wrong with your CSRF token. Please refresh the page. If this error persists, try clearing your cookies.", http.StatusBadRequest) })) + + mw.ExemptRegexp(regexp.MustCompile("/api/v2/users/first")) + // Exempt all requests that do not require CSRF protection. // All GET requests are exempt by default. mw.ExemptPath("/api/v2/csp/reports") @@ -39,6 +45,10 @@ func CSRF(secureCookie bool) func(next http.Handler) http.Handler { mw.ExemptRegexp(regexp.MustCompile("/organizations/[^/]+/provisionerdaemons/*")) mw.ExemptFunc(func(r *http.Request) bool { + if !strings.HasPrefix(r.URL.Path, "/api") { + return true + } + // CSRF only affects requests that automatically attach credentials via a cookie. // If no cookie is present, then there is no risk of CSRF. //nolint:govet diff --git a/coderd/httpmw/csrf_test.go b/coderd/httpmw/csrf_test.go new file mode 100644 index 0000000000000..f9838593b38cb --- /dev/null +++ b/coderd/httpmw/csrf_test.go @@ -0,0 +1,39 @@ +package httpmw_test + +import ( + "net/http" + "net/url" + "testing" + + "github.com/justinas/nosurf" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/httpmw" +) + +func TestCSRFExempt(t *testing.T) { + t.Parallel() + + cases := []struct { + Name string + Path string + Exempt bool + }{ + { + Name: "Root", + Path: "/", + Exempt: true, + }, + } + + mw := httpmw.CSRF(false) + csrfmw := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})).(*nosurf.CSRFHandler) + + for _, c := range cases { + c := c + t.Run(c.Name, func(t *testing.T) { + exempt := csrfmw.IsExempt(&http.Request{URL: &url.URL{Path: c.Path}}) + require.Equal(t, c.Exempt, exempt) + }) + } +} diff --git a/enterprise/wsproxy/wsproxy.go b/enterprise/wsproxy/wsproxy.go index 242c3f14ed3db..9ad8f16764c28 100644 --- a/enterprise/wsproxy/wsproxy.go +++ b/enterprise/wsproxy/wsproxy.go @@ -329,10 +329,9 @@ func New(ctx context.Context, opts *Options) (*Server, error) { next.ServeHTTP(w, r) }) }, - // CSRF middleware is intentionally excluded here. All coder requests - // which require CSRF protection are forwarded to the primary Coderd - // via a proxy function. Since the primary enforces this, the proxy does - // not. + // CSRF is required here because we need to set the CSRF cookies on + // responses. + httpmw.CSRF(s.Options.SecureAuthCookie), ) // Attach workspace apps routes. From 388e56e5856fe1a7c7557b5dea5e5bc0fca8b8e7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 8 Jan 2024 10:20:44 -0600 Subject: [PATCH 7/9] add comment --- coderd/httpmw/csrf.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/httpmw/csrf.go b/coderd/httpmw/csrf.go index d52d99c88fe88..529cac3a727d7 100644 --- a/coderd/httpmw/csrf.go +++ b/coderd/httpmw/csrf.go @@ -45,6 +45,7 @@ func CSRF(secureCookie bool) func(next http.Handler) http.Handler { mw.ExemptRegexp(regexp.MustCompile("/organizations/[^/]+/provisionerdaemons/*")) mw.ExemptFunc(func(r *http.Request) bool { + // Only enforce CSRF on API routes. if !strings.HasPrefix(r.URL.Path, "/api") { return true } From fee870b289c79b51747b68f35974a25d6bb0fc0b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 8 Jan 2024 13:12:16 -0600 Subject: [PATCH 8/9] add unit tests --- coderd/httpmw/csrf_test.go | 41 +++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/coderd/httpmw/csrf_test.go b/coderd/httpmw/csrf_test.go index f9838593b38cb..fc220b757de66 100644 --- a/coderd/httpmw/csrf_test.go +++ b/coderd/httpmw/csrf_test.go @@ -2,28 +2,53 @@ package httpmw_test import ( "net/http" - "net/url" "testing" "github.com/justinas/nosurf" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/codersdk" ) -func TestCSRFExempt(t *testing.T) { +func TestCSRFExemptList(t *testing.T) { t.Parallel() cases := []struct { Name string - Path string + URL string Exempt bool }{ { Name: "Root", - Path: "/", + URL: "https://example.com", Exempt: true, }, + { + Name: "WorkspacePage", + URL: "https://coder.com/workspaces", + Exempt: true, + }, + { + Name: "SubApp", + URL: "https://app--dev--coder--user--apps.coder.com/", + Exempt: true, + }, + { + Name: "PathApp", + URL: "https://coder.com/@USER/test.instance/apps/app", + Exempt: true, + }, + { + Name: "API", + URL: "https://coder.com/api/v2", + Exempt: false, + }, + { + Name: "APIMe", + URL: "https://coder.com/api/v2/me", + Exempt: false, + }, } mw := httpmw.CSRF(false) @@ -32,7 +57,13 @@ func TestCSRFExempt(t *testing.T) { for _, c := range cases { c := c t.Run(c.Name, func(t *testing.T) { - exempt := csrfmw.IsExempt(&http.Request{URL: &url.URL{Path: c.Path}}) + t.Parallel() + + r, err := http.NewRequest(http.MethodPost, c.URL, nil) + require.NoError(t, err) + + r.AddCookie(&http.Cookie{Name: codersdk.SessionTokenCookie, Value: "test"}) + exempt := csrfmw.IsExempt(r) require.Equal(t, c.Exempt, exempt) }) } From 6966f87bb235dc921c1e8383a21238d0041110cb Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 8 Jan 2024 16:25:14 -0600 Subject: [PATCH 9/9] Linting --- coderd/httpmw/csrf_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/csrf_test.go b/coderd/httpmw/csrf_test.go index fc220b757de66..12c6afe825f75 100644 --- a/coderd/httpmw/csrf_test.go +++ b/coderd/httpmw/csrf_test.go @@ -1,6 +1,7 @@ package httpmw_test import ( + "context" "net/http" "testing" @@ -59,7 +60,7 @@ func TestCSRFExemptList(t *testing.T) { t.Run(c.Name, func(t *testing.T) { t.Parallel() - r, err := http.NewRequest(http.MethodPost, c.URL, nil) + r, err := http.NewRequestWithContext(context.Background(), http.MethodPost, c.URL, nil) require.NoError(t, err) r.AddCookie(&http.Cookie{Name: codersdk.SessionTokenCookie, Value: "test"})