fix: relax csrf to exclude path based apps (#11430)

Emyrk · web-flow · commit fb29af664be5 · 2024-01-08T22:33:57.000Z
* fix: relax csrf to exclude path based apps
* add unit test to verify path based apps are not CSRF blocked
diff --git 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, "<pre>\n</pre>\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
@@ -3,6 +3,7 @@ package httpmw
 import (
 	"net/http"
 	"regexp"
+	"strings"
 
 	"github.com/justinas/nosurf"
 	"golang.org/x/xerrors"
@@ -12,17 +13,25 @@ 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)
 		mw.SetBaseCookie(http.Cookie{Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: secureCookie})
 		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
@@ -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
@@ -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),
 	)
 
