diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 5045190072f47..052a6ac0c8d40 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -8,6 +8,7 @@ import ( "errors" "flag" "fmt" + "mime" "net/http" "reflect" "strings" @@ -229,6 +230,38 @@ func WriteIndent(ctx context.Context, rw http.ResponseWriter, status int, respon _ = enc.Encode(response) } +// checkContentType enforces that JSON request bodies are declared as +// application/json when the request is authenticated with a session cookie. +// +// This is a CSRF defense-in-depth measure: browsers send cross-origin POSTs +// without a preflight only for "simple" content types (text/plain, +// application/x-www-form-urlencoded, multipart/form-data). Decoding JSON +// from such bodies would let any endpoint mistakenly exempted from the CSRF +// middleware be forged from a cross-site page. A browser attacker cannot set +// Content-Type: application/json without triggering a CORS preflight, and +// cannot remove the victim's session cookie. +// +// Requests that do not carry a session cookie (agent, CLI, and API-token +// clients) are unaffected, so scripts that omit the header keep working. +func checkContentType(ctx context.Context, rw http.ResponseWriter, r *http.Request) bool { + if _, err := r.Cookie(codersdk.SessionTokenCookie); err != nil { + // No session cookie: not CSRF-relevant. + return true + } + + contentType := r.Header.Get("Content-Type") + mediaType, _, err := mime.ParseMediaType(contentType) + if err == nil && strings.EqualFold(mediaType, "application/json") { + return true + } + + Write(ctx, rw, http.StatusUnsupportedMediaType, codersdk.Response{ + Message: "Unsupported Content-Type.", + Detail: fmt.Sprintf("Cookie-authenticated requests with a JSON body must set the %q header to %q, got %q.", "Content-Type", "application/json", contentType), + }) + return false +} + // Read decodes JSON from the HTTP request into the value provided. It uses // go-validator to validate the incoming request body. ctx is used for tracing // and can be nil. Although tracing this function isn't likely too helpful, it @@ -237,6 +270,10 @@ func Read(ctx context.Context, rw http.ResponseWriter, r *http.Request, value in ctx, span := tracing.StartSpan(ctx) defer span.End() + if !checkContentType(ctx, rw, r) { + return false + } + err := json.NewDecoder(r.Body).Decode(value) if err != nil { if _, ok := errors.AsType[*http.MaxBytesError](err); ok { diff --git a/coderd/httpapi/httpapi_test.go b/coderd/httpapi/httpapi_test.go index dca28196dc56b..9bd12d69091b5 100644 --- a/coderd/httpapi/httpapi_test.go +++ b/coderd/httpapi/httpapi_test.go @@ -141,6 +141,60 @@ func TestRead(t *testing.T) { }) } +// TestReadContentType verifies that Read rejects cookie-authenticated +// requests whose body is not declared as application/json. Browsers can send +// cross-origin POSTs without a CORS preflight only for "simple" content +// types (e.g. text/plain), so decoding JSON from such bodies would enable +// CSRF on any endpoint mistakenly exempted from the CSRF middleware. +// Requests without a session cookie are unaffected. +func TestReadContentType(t *testing.T) { + t.Parallel() + + cases := []struct { + Name string + ContentType string + SessionCookie bool + WantOK bool + }{ + {Name: "CookieJSON", ContentType: "application/json", SessionCookie: true, WantOK: true}, + {Name: "CookieJSONCharset", ContentType: "application/json; charset=utf-8", SessionCookie: true, WantOK: true}, + {Name: "CookieJSONUppercase", ContentType: "APPLICATION/JSON", SessionCookie: true, WantOK: true}, + {Name: "CookieTextPlain", ContentType: "text/plain", SessionCookie: true, WantOK: false}, + {Name: "CookieForm", ContentType: "application/x-www-form-urlencoded", SessionCookie: true, WantOK: false}, + {Name: "CookieMissing", ContentType: "", SessionCookie: true, WantOK: false}, + {Name: "CookieMalformed", ContentType: "application/", SessionCookie: true, WantOK: false}, + {Name: "NoCookieTextPlain", ContentType: "text/plain", SessionCookie: false, WantOK: true}, + {Name: "NoCookieMissing", ContentType: "", SessionCookie: false, WantOK: true}, + } + + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + rw := httptest.NewRecorder() + r := httptest.NewRequest("POST", "/", bytes.NewBufferString(`{"value":"hi"}`)) + if c.ContentType != "" { + r.Header.Set("Content-Type", c.ContentType) + } + if c.SessionCookie { + r.AddCookie(&http.Cookie{Name: codersdk.SessionTokenCookie, Value: "test"}) + } + + var v struct { + Value string `json:"value"` + } + ok := httpapi.Read(ctx, rw, r, &v) + require.Equal(t, c.WantOK, ok) + if !c.WantOK { + require.Equal(t, http.StatusUnsupportedMediaType, rw.Code) + require.Contains(t, rw.Body.String(), "Unsupported Content-Type") + } else { + require.Equal(t, "hi", v.Value) + } + }) + } +} + func TestWebsocketCloseMsg(t *testing.T) { t.Parallel() diff --git a/coderd/httpmw/csrf.go b/coderd/httpmw/csrf.go index 8bd7c4a8b31c5..ce79dbce09b3f 100644 --- a/coderd/httpmw/csrf.go +++ b/coderd/httpmw/csrf.go @@ -3,7 +3,6 @@ package httpmw import ( "fmt" "net/http" - "regexp" "strings" "github.com/justinas/nosurf" @@ -40,26 +39,28 @@ func CSRF(cookieCfg codersdk.HTTPCookieConfig) func(next http.Handler) http.Hand 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. + // + // Exemptions are exact-path matches ONLY. Unanchored regex + // exemptions were removed because nosurf matches them as + // substrings of the request path: a pattern like "derp/*" + // exempted every /api path merely containing "derp", including + // attacker-influenced segments such as usernames, and nosurf + // short-circuits before BOTH the token check and its same-origin + // validation on exempt paths. + // + // The removed exemptions were redundant: + // - Agent, workspace-proxy, and provisioner-daemon requests + // authenticate via headers/PSK and carry no session cookie, + // so the ExemptFunc below already exempts them. + // - /derp and /scim are not under /api, so the ExemptFunc + // prefix check already exempts them. + // - The dashboard sends X-CSRF-TOKEN on every request, so + // browser flows on the previously exempted routes (e.g. + // devcontainer recreate) pass the standard CSRF checks. 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/*")) - // Workspace Proxy routes - mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceproxies/me/*")) - // Derp routes - mw.ExemptRegexp(regexp.MustCompile("derp/*")) - // Scim - mw.ExemptRegexp(regexp.MustCompile("api/v2/scim/*")) - // Provisioner daemon routes - mw.ExemptRegexp(regexp.MustCompile("/organizations/[^/]+/provisionerdaemons/*")) + mw.ExemptPath("/api/v2/users/first") mw.ExemptFunc(func(r *http.Request) bool { // Enforce CSRF on API routes and the OAuth2 authorize diff --git a/coderd/httpmw/csrf_test.go b/coderd/httpmw/csrf_test.go index c1365b39f9f8b..a464f2238a596 100644 --- a/coderd/httpmw/csrf_test.go +++ b/coderd/httpmw/csrf_test.go @@ -71,6 +71,78 @@ func TestCSRFExemptList(t *testing.T) { URL: "https://coder.com/oauth2/register", Exempt: true, }, + // Exact-path exemptions. + { + Name: "CSPReports", + URL: "https://coder.com/api/v2/csp/reports", + Exempt: true, + }, + { + Name: "FirstUser", + URL: "https://coder.com/api/v2/users/first", + Exempt: true, + }, + // Non-/api paths are exempt via the ExemptFunc prefix check. + { + Name: "DERP", + URL: "https://coder.com/derp", + Exempt: true, + }, + { + Name: "SCIM", + URL: "https://coder.com/scim/v2/Users", + Exempt: true, + }, + // Regression tests for the removed unanchored regex exemptions + // (previously exempt as substring matches): cookie-authenticated + // requests on these paths MUST be CSRF-protected. Attacker-chosen + // names (usernames, org names, task names) may legally contain + // substrings like "derp" or a "first" prefix. + { + Name: "UsernameContainingDerp", + URL: "https://coder.com/api/v2/users/derp-attacker/workspaces", + Exempt: false, + }, + { + Name: "UsernameWithFirstPrefix", + URL: "https://coder.com/api/v2/users/firstuser/keys", + Exempt: false, + }, + { + Name: "TaskUserContainingDerp", + URL: "https://coder.com/api/v2/tasks/derp-attacker", + Exempt: false, + }, + { + Name: "OrgNameContainingDerp", + URL: "https://coder.com/api/v2/organizations/derp/members/someone/workspaces", + Exempt: false, + }, + { + Name: "WorkspaceAgentsDevcontainerRecreate", + URL: "https://coder.com/api/v2/workspaceagents/8d3e19b7-4b9e-4a25-a367-927384ee6c2f/containers/devcontainers/dc/recreate", + Exempt: false, + }, + { + Name: "WorkspaceAgentsMe", + URL: "https://coder.com/api/v2/workspaceagents/me/rpc", + Exempt: false, + }, + { + Name: "WorkspaceProxiesMe", + URL: "https://coder.com/api/v2/workspaceproxies/me/register", + Exempt: false, + }, + { + Name: "ProvisionerDaemons", + URL: "https://coder.com/api/v2/organizations/default/provisionerdaemons", + Exempt: false, + }, + { + Name: "SCIMUnderAPI", + URL: "https://coder.com/api/v2/scim/v2/Users", + Exempt: false, + }, } mw := httpmw.CSRF(codersdk.HTTPCookieConfig{}) @@ -88,6 +160,26 @@ func TestCSRFExemptList(t *testing.T) { require.Equal(t, c.Exempt, exempt) }) } + + // Requests without a session cookie are not CSRF-relevant and are exempt + // via the ExemptFunc. This models agents, workspace proxies, and + // provisioner daemons, which authenticate with headers/PSK and carry no + // cookies; the removed regex exemptions for those routes were redundant + // with this behavior. + t.Run("NoSessionCookie", func(t *testing.T) { + t.Parallel() + + for _, u := range []string{ + "https://coder.com/api/v2/workspaceagents/me/rpc", + "https://coder.com/api/v2/workspaceproxies/me/register", + "https://coder.com/api/v2/organizations/default/provisionerdaemons", + "https://coder.com/api/v2/users/first", + } { + r, err := http.NewRequestWithContext(context.Background(), http.MethodPost, u, nil) + require.NoError(t, err) + require.True(t, csrfmw.IsExempt(r), "no-cookie request to %s should be exempt", u) + } + }) } // TestCSRFError verifies the error message returned to a user when CSRF