Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitfb29af6

Browse files
authored
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
1 parent9f5a59d commitfb29af6

File tree

4 files changed

+151
-2
lines changed

4 files changed

+151
-2
lines changed

‎coderd/coderd_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package coderd_test
33
import (
44
"context"
55
"flag"
6+
"fmt"
67
"io"
78
"net/http"
89
"net/netip"
@@ -21,6 +22,9 @@ import (
2122

2223
"cdr.dev/slog"
2324
"cdr.dev/slog/sloggers/slogtest"
25+
"github.com/coder/coder/v2/coderd/database"
26+
"github.com/coder/coder/v2/coderd/database/dbfake"
27+
"github.com/coder/coder/v2/provisionersdk/proto"
2428

2529
"github.com/coder/coder/v2/agent/agenttest"
2630
"github.com/coder/coder/v2/buildinfo"
@@ -315,3 +319,63 @@ func TestSwagger(t *testing.T) {
315319
require.Equal(t,"<pre>\n</pre>\n",string(body))
316320
})
317321
}
322+
323+
funcTestCSRFExempt(t*testing.T) {
324+
t.Parallel()
325+
326+
// This test build a workspace with an agent and an app. The app is not
327+
// a real http server, so it will fail to serve requests. We just want
328+
// to make sure the failure is not a CSRF failure, as path based
329+
// apps should be exempt.
330+
t.Run("PathBasedApp",func(t*testing.T) {
331+
t.Parallel()
332+
333+
client,_,api:=coderdtest.NewWithAPI(t,nil)
334+
first:=coderdtest.CreateFirstUser(t,client)
335+
owner,err:=client.User(context.Background(),"me")
336+
require.NoError(t,err)
337+
338+
ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitMedium)
339+
defercancel()
340+
341+
// Create a workspace.
342+
constagentSlug="james"
343+
constappSlug="web"
344+
wrk:=dbfake.WorkspaceBuild(t,api.Database, database.Workspace{
345+
OwnerID:owner.ID,
346+
OrganizationID:first.OrganizationID,
347+
}).
348+
WithAgent(func(agents []*proto.Agent) []*proto.Agent {
349+
agents[0].Name=agentSlug
350+
agents[0].Apps= []*proto.App{{
351+
Slug:appSlug,
352+
DisplayName:appSlug,
353+
Subdomain:false,
354+
Url:"/",
355+
}}
356+
357+
returnagents
358+
}).
359+
Do()
360+
361+
u:=client.URL.JoinPath(fmt.Sprintf("/@%s/%s.%s/apps/%s",owner.Username,wrk.Workspace.Name,agentSlug,appSlug)).String()
362+
req,err:=http.NewRequestWithContext(ctx,http.MethodPost,u,nil)
363+
req.AddCookie(&http.Cookie{
364+
Name:codersdk.SessionTokenCookie,
365+
Value:client.SessionToken(),
366+
Path:"/",
367+
Domain:client.URL.String(),
368+
})
369+
require.NoError(t,err)
370+
371+
resp,err:=client.HTTPClient.Do(req)
372+
require.NoError(t,err)
373+
data,_:=io.ReadAll(resp.Body)
374+
_=resp.Body.Close()
375+
376+
// A StatusBadGateway means Coderd tried to proxy to the agent and failed because the agent
377+
// was not there. This means CSRF did not block the app request, which is what we want.
378+
require.Equal(t,http.StatusBadGateway,resp.StatusCode,"status code 500 is CSRF failure")
379+
require.NotContains(t,string(data),"CSRF")
380+
})
381+
}

‎coderd/httpmw/csrf.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package httpmw
33
import (
44
"net/http"
55
"regexp"
6+
"strings"
67

78
"github.com/justinas/nosurf"
89
"golang.org/x/xerrors"
@@ -12,17 +13,25 @@ import (
1213

1314
// CSRF is a middleware that verifies that a CSRF token is present in the request
1415
// for non-GET requests.
16+
// If enforce is false, then CSRF enforcement is disabled. We still want
17+
// to include the CSRF middleware because it will set the CSRF cookie.
1518
funcCSRF(secureCookiebool)func(next http.Handler) http.Handler {
1619
returnfunc(next http.Handler) http.Handler {
1720
mw:=nosurf.New(next)
1821
mw.SetBaseCookie(http.Cookie{Path:"/",HttpOnly:true,SameSite:http.SameSiteLaxMode,Secure:secureCookie})
1922
mw.SetFailureHandler(http.HandlerFunc(func(w http.ResponseWriter,r*http.Request) {
2023
http.Error(w,"Something is wrong with your CSRF token. Please refresh the page. If this error persists, try clearing your cookies.",http.StatusBadRequest)
2124
}))
25+
26+
mw.ExemptRegexp(regexp.MustCompile("/api/v2/users/first"))
27+
2228
// Exempt all requests that do not require CSRF protection.
2329
// All GET requests are exempt by default.
2430
mw.ExemptPath("/api/v2/csp/reports")
2531

32+
// This should not be required?
33+
mw.ExemptRegexp(regexp.MustCompile("/api/v2/users/first"))
34+
2635
// Agent authenticated routes
2736
mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/me/*"))
2837
mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/*"))
@@ -36,6 +45,11 @@ func CSRF(secureCookie bool) func(next http.Handler) http.Handler {
3645
mw.ExemptRegexp(regexp.MustCompile("/organizations/[^/]+/provisionerdaemons/*"))
3746

3847
mw.ExemptFunc(func(r*http.Request)bool {
48+
// Only enforce CSRF on API routes.
49+
if!strings.HasPrefix(r.URL.Path,"/api") {
50+
returntrue
51+
}
52+
3953
// CSRF only affects requests that automatically attach credentials via a cookie.
4054
// If no cookie is present, then there is no risk of CSRF.
4155
//nolint:govet

‎coderd/httpmw/csrf_test.go‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
package httpmw_test
2+
3+
import (
4+
"context"
5+
"net/http"
6+
"testing"
7+
8+
"github.com/justinas/nosurf"
9+
"github.com/stretchr/testify/require"
10+
11+
"github.com/coder/coder/v2/coderd/httpmw"
12+
"github.com/coder/coder/v2/codersdk"
13+
)
14+
15+
funcTestCSRFExemptList(t*testing.T) {
16+
t.Parallel()
17+
18+
cases:= []struct {
19+
Namestring
20+
URLstring
21+
Exemptbool
22+
}{
23+
{
24+
Name:"Root",
25+
URL:"https://example.com",
26+
Exempt:true,
27+
},
28+
{
29+
Name:"WorkspacePage",
30+
URL:"https://coder.com/workspaces",
31+
Exempt:true,
32+
},
33+
{
34+
Name:"SubApp",
35+
URL:"https://app--dev--coder--user--apps.coder.com/",
36+
Exempt:true,
37+
},
38+
{
39+
Name:"PathApp",
40+
URL:"https://coder.com/@USER/test.instance/apps/app",
41+
Exempt:true,
42+
},
43+
{
44+
Name:"API",
45+
URL:"https://coder.com/api/v2",
46+
Exempt:false,
47+
},
48+
{
49+
Name:"APIMe",
50+
URL:"https://coder.com/api/v2/me",
51+
Exempt:false,
52+
},
53+
}
54+
55+
mw:=httpmw.CSRF(false)
56+
csrfmw:=mw(http.HandlerFunc(func(w http.ResponseWriter,r*http.Request) {})).(*nosurf.CSRFHandler)
57+
58+
for_,c:=rangecases {
59+
c:=c
60+
t.Run(c.Name,func(t*testing.T) {
61+
t.Parallel()
62+
63+
r,err:=http.NewRequestWithContext(context.Background(),http.MethodPost,c.URL,nil)
64+
require.NoError(t,err)
65+
66+
r.AddCookie(&http.Cookie{Name:codersdk.SessionTokenCookie,Value:"test"})
67+
exempt:=csrfmw.IsExempt(r)
68+
require.Equal(t,c.Exempt,exempt)
69+
})
70+
}
71+
}

‎enterprise/wsproxy/wsproxy.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -329,8 +329,8 @@ func New(ctx context.Context, opts *Options) (*Server, error) {
329329
next.ServeHTTP(w,r)
330330
})
331331
},
332-
//TODO: @emyrk we might notneedthis? But goodtohave if it does
333-
//not break anything.
332+
//CSRF is required here because weneed toset the CSRF cookies on
333+
//responses.
334334
httpmw.CSRF(s.Options.SecureAuthCookie),
335335
)
336336

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp