mirror of https://github.com/coder/coder.git
fix: relax csrf to exclude path based apps (#11430)
* fix: relax csrf to exclude path based apps * add unit test to verify path based apps are not CSRF blocked
This commit is contained in:
parent
9f5a59d5c5
commit
fb29af664b
|
@ -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")
|
||||
})
|
||||
}
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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)
|
||||
})
|
||||
}
|
||||
}
|
|
@ -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),
|
||||
)
|
||||
|
||||
|
|
Loading…
Reference in New Issue