diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index 9823b2b62a..8d7c129746 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,63 @@ func TestSwagger(t *testing.T) { require.Equal(t, "
\n
\n", string(body)) }) } + +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() + + 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 := 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) + _ = 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. + require.Equal(t, http.StatusBadGateway, resp.StatusCode, "status code 500 is CSRF failure") + require.NotContains(t, string(data), "CSRF") + }) +} diff --git a/coderd/httpmw/csrf.go b/coderd/httpmw/csrf.go index 7888365741..529cac3a72 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,10 +22,16 @@ 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") + // 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/*")) @@ -36,6 +45,11 @@ 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 + } + // 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 0000000000..12c6afe825 --- /dev/null +++ b/coderd/httpmw/csrf_test.go @@ -0,0 +1,71 @@ +package httpmw_test + +import ( + "context" + "net/http" + "testing" + + "github.com/justinas/nosurf" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/codersdk" +) + +func TestCSRFExemptList(t *testing.T) { + t.Parallel() + + cases := []struct { + Name string + URL string + Exempt bool + }{ + { + Name: "Root", + 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) + 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) { + t.Parallel() + + r, err := http.NewRequestWithContext(context.Background(), 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) + }) + } +} diff --git a/enterprise/wsproxy/wsproxy.go b/enterprise/wsproxy/wsproxy.go index cb1671e133..9ad8f16764 100644 --- a/enterprise/wsproxy/wsproxy.go +++ b/enterprise/wsproxy/wsproxy.go @@ -329,8 +329,8 @@ 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. + // CSRF is required here because we need to set the CSRF cookies on + // responses. httpmw.CSRF(s.Options.SecureAuthCookie), )