feat: Implement (but not enforce) CSRF for FE requests #3786
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| package httpmw | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "regexp" | ||
|
|
||
| "github.com/justinas/nosurf" | ||
| "golang.org/x/xerrors" | ||
|
|
||
| "github.com/coder/coder/codersdk" | ||
| ) | ||
|
|
||
| // CSRF is a middleware that verifies that a CSRF token is present in the request | ||
| // for non-GET requests. | ||
| func CSRF(secureCookie bool) func(next http.Handler) http.Handler { | ||
| return func(next http.Handler) http.Handler { | ||
| mw := nosurf.New(next) | ||
| mw.SetBaseCookie(http.Cookie{Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: secureCookie}) | ||
| mw.SetFailureHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| http.Error(w, "Something is wrong with your CSRF token. Please refresh the page. If this error persists, try clearing your cookies.", http.StatusBadRequest) | ||
| })) | ||
|
|
||
| // 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/*")) | ||
|
Comment on lines
+23
to
+30
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. |
||
| // Derp routes | ||
| mw.ExemptRegexp(regexp.MustCompile("derp/*")) | ||
|
|
||
| mw.ExemptFunc(func(r *http.Request) bool { | ||
| // Enable CSRF in November 2022 by deleting this "return true" line. | ||
| // CSRF is not enforced to ensure backwards compatibility with older | ||
| // cli versions. | ||
| //nolint:revive | ||
| return true | ||
|
|
||
| // CSRF only affects requests that automatically attach credentials via a cookie. | ||
| // If no cookie is present, then there is no risk of CSRF. | ||
| //nolint:govet | ||
| sessCookie, err := r.Cookie(codersdk.SessionTokenKey) | ||
| if xerrors.Is(err, http.ErrNoCookie) { | ||
| return true | ||
| } | ||
|
|
||
| if token := r.Header.Get(codersdk.SessionCustomHeader); token == sessCookie.Value { | ||
| // If the cookie and header match, we can assume this is the same as just using the | ||
| // custom header auth. Custom header auth can bypass CSRF, as CSRF attacks | ||
| // cannot add custom headers. | ||
| return true | ||
| } | ||
|
|
||
| if token := r.URL.Query().Get(codersdk.SessionTokenKey); token == sessCookie.Value { | ||
| // If the auth is set in a url param and matches the cookie, it | ||
| // is the same as just using the url param. | ||
| return true | ||
| } | ||
|
|
||
| // If the X-CSRF-TOKEN header is set, we can exempt the func if it's valid. | ||
| // This is the CSRF check. | ||
| sent := r.Header.Get("X-CSRF-TOKEN") | ||
| if sent != "" { | ||
| return nosurf.VerifyToken(nosurf.Token(r), sent) | ||
| } | ||
| return false | ||
| }) | ||
| return mw | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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. 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.