feat: Implement (but not enforce) CSRF for FE requests #3786
Conversation
| // Exempt all requests that do not require CSRF protection. | ||
| // All GET requests are exempt by default. | ||
| mw.ExemptPath("/api/v2/csp/reports") | ||
|
|
||
| // Top level agent routes. | ||
| mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/[^/]*$")) | ||
| // Agent authenticated routes | ||
| mw.ExemptRegexp(regexp.MustCompile("api/v2/workspaceagents/me/*")) |
There was a problem hiding this comment.
Pretty sure this is all we need to exempt. If provisionerd needs to make api calls, we should omit that as well.
|
I'll see if I can pull this down and fix the FE errors. |
|
This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity. |
|
Chatted with @deansheather offline who said he can help with this |
Might need to await some page load or detect in puppeteer
| })) | ||
|
|
||
| // Exempt all requests that do not require CSRF protection. | ||
| // All GET requests are exempt by default. |
There was a problem hiding this comment.
Is this not an issue? Can't we still send data with a GET?
There was a problem hiding this comment.
CSRF really only needs to protect endpoints that have side effects. Our router checks the method on all API calls. See https://security.stackexchange.com/questions/115794/should-i-use-csrf-protection-for-get-requests.
If we design our api correctly, there is no need to enforce CSRF on GET requests.
| OAuth2StateKey = "oauth_state" | ||
| OAuth2RedirectKey = "oauth_redirect" | ||
| SessionTokenKey = "coder_session_token" | ||
| // SessionCustomHeader is the custom header to use for authentication. |
There was a problem hiding this comment.
The cli needs to bypass CSRF (it'd be dumb to use on the cli), so we use a custom header rather than a cookie. The old code required using a -H 'cooke:...' which is kinda odd.
In future we might want to use an Authorization Bearer token to be more standard?
| export const hardCodedCSRFCookie = (): string => { | ||
| // This is a hard coded CSRF token/cookie pair for local development. | ||
| // In prod, the GoLang webserver generates a random cookie with a new token for | ||
| // each document request. For local development, we don't use the Go webserver for static files, | ||
| // so this is the 'hack' to make local development work with remote apis. | ||
| // The CSRF cookie for this token is "JXm9hOUdZctWt0ZZGAy9xiS/gxMKYOThdxjjMnMUyn4=" | ||
| const csrfToken = | ||
| "KNKvagCBEHZK7ihe2t7fj6VeJ0UyTDco1yVUJE8N06oNqxLu5Zx1vRxZbgfC0mJJgeGkVjgs08mgPbcWPBkZ1A==" | ||
| axios.defaults.headers.common["X-CSRF-TOKEN"] = csrfToken | ||
| return csrfToken | ||
| } |
There was a problem hiding this comment.
For local development, we don't use the Go webserver for static files
@Emyrk Dumb question...why don't we use the Go webserver for static files? What do we instead? (Just trying to understand the whole picture).
There was a problem hiding this comment.
We do, but for the FE team to have a hot reload, we have to use webpack server instead to host the files. So the webpack server has to do what the golang server is doing.
Instead of implementing CSRF completely, we just hardcode it since it's only used for developement.
| } | ||
| } else { | ||
| // Do not write error logs if we are in a FE unit test. | ||
| if (process.env.JEST_WORKER_ID === undefined) { |
There was a problem hiding this comment.
Bummer we have to do this because of Jest but I don't know a better way lol thanks for adding a comment!
What this does
Implements CSRF in the frontend. All non get requests using cookie auth require CSRF header as well.
Allows using
Coder-Session-Tokenheader to bypass CSRF for cli auth.fixes #3751
Future Work
Enforce CSRF. To be backwards compatible, we are delaying enforcing csrf. In 1/2 releases we can enforce it and force users to update to this version of the cli.