- Notifications
You must be signed in to change notification settings - Fork928
feat: Implement (but not enforce) CSRF for FE requests#3786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pretty sure this is all we need to exempt. If provisionerd needs to make api calls, we should omit that as well.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FE: requesting changes for theconsole.error message whentoken isn't found. At the bare minimum, we should provide a helpful error message. I don't think we should link to the laravel docs.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this not an issue? Can't we still send data with a GET?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
CSRF really only needs to protect endpoints that have side effects. Our router checks the method on all API calls. Seehttps://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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
great work as always steven
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is a custom header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
great docs on this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
✅ FE working fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice work!
| } | ||
| } 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Bummer we have to do this because of Jest but I don't know a better way lol thanks for adding a comment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is unfortunate :/
Uh oh!
There was an error while loading.Please reload this page.
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 tothis version of the cli.