fix: relax csrf to exclude path based apps #11430
Conversation
| @@ -315,3 +319,62 @@ func TestSwagger(t *testing.T) { | |||
| require.Equal(t, "<pre>\n</pre>\n", string(body)) | |||
| }) | |||
| } | |||
|
|
|||
| func TestCSRFExempt(t *testing.T) { | |||
There was a problem hiding this comment.
I am working on a unit test to verify
Is it this one?
There was a problem hiding this comment.
Yup. All of our existing unit tests bypass CSRF since the codersdk uses the HEADER method to auth, not the cookie.
coderd/coderd.go
Outdated
| // CSRF only needs to apply to /api routes. It does not apply to GET requests | ||
| // anyway, which is most other routes. We want to exempt any external auth or | ||
| // application type routes. | ||
| httpmw.CSRF(options.SecureAuthCookie), |
There was a problem hiding this comment.
There are a bunch of ExemptRegexp() calls inside this middleware that may need to be reviewed after this change. It might be nicer to have those passed explicitly as arguments to httpmw.CSRF instead of being hidden inside the middleware.
There was a problem hiding this comment.
The thing is, this will pretty quickly fail on someone when they add a new route if it breaks. The error is a CSRF error, which brings you to here. I prefer to have this all in 1 place, and this should never be configurable by a customer except maybe to disable it entirely.
I think the current exempts are overkill. I intentionally went a bit overkill because I was afraid of breaking anything. Some
The reason there are exempts like this is when I turned this back on, I was trying to error on the side of caution. I think we should have a PR to remove an exempt 1 by 1 when we determine they can never cause an issue.
An example is the /provisionerdaemons ones. They are currently only GET requests, and I do not think the actual daemons are using cookies to authenticate. However I error-ed on the side of caution since these routes should never use CSRF anyway.
I can make follow up PRs to drop these with rational for each one
|
@johnstcn CSRF state is set via go template on page render. So the CSRF middleware has to be present for page loads. I just added this instead. There is only an "exempt" syntax, rather than an "opt in" unfortunately. I think this is the easiest fix. |
coderd/httpmw/csrf_test.go
Outdated
| }{ | ||
| { | ||
| Name: "Root", | ||
| Path: "/", | ||
| Exempt: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
we can add some more test cases here
It is never happy 😄 |
Fixes #11406
I am working on a unit test to verify and make sure this does not break in future. Our current tests use the header auth method, which bypasses CSRF. My manual testing was just a fileserver which only used GET requests.