Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add X-Requested-With header to prevent CSRF #48

Merged
merged 11 commits into from
Feb 5, 2021
Merged

Add X-Requested-With header to prevent CSRF #48

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0e8174b
Add X-Requested-With header check for CSRF
chenilim Feb 3, 2021
2237169
Merge branch 'main' into requested-with-header
chenilim Feb 3, 2021
d257b1f
Add X-Requested-With header check for CSRF
chenilim Feb 3, 2021
6a6c467
Merge branch 'requested-with-header' of github.com:mattermost/focalbo…
chenilim Feb 3, 2021
d655ca3
Add X-Requested-With header check for CSRF
chenilim Feb 3, 2021
6cc4032
Merge branch 'requested-with-header' of github.com:mattermost/focalbo…
chenilim Feb 3, 2021
519cfac
Add X-Requested-With to server test
chenilim Feb 3, 2021
daf8369
Refactor API to use middleware
chenilim Feb 5, 2021
b2a3daf
Merge branch 'main' into requested-with-header
chenilim Feb 5, 2021
c484eb8
Don't require CSRF token for get files
chenilim Feb 5, 2021
1957e1d
cleanup
chenilim Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 52 additions & 17 deletions server/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ import (
"github.com/mattermost/focalboard/server/utils"
)

const (
HEADER_REQUESTED_WITH = "X-Requested-With"
HEADER_REQUESTED_WITH_XML = "XMLHttpRequest"
)

// ----------------------------------------------------------------------------------------------------
// REST APIs

Expand All @@ -35,35 +40,65 @@ func (a *API) app() *app.App {
}

func (a *API) RegisterRoutes(r *mux.Router) {
r.HandleFunc("/api/v1/blocks", a.sessionRequired(a.handleGetBlocks)).Methods("GET")
r.HandleFunc("/api/v1/blocks", a.sessionRequired(a.handlePostBlocks)).Methods("POST")
r.HandleFunc("/api/v1/blocks/{blockID}", a.sessionRequired(a.handleDeleteBlock)).Methods("DELETE")
r.HandleFunc("/api/v1/blocks/{blockID}/subtree", a.attachSession(a.handleGetSubTree, false)).Methods("GET")
apiv1 := r.PathPrefix("/api/v1").Subrouter()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice touch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I smoke tested this and it looks ok, but please LMK if you see anything off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, everything is looking great now

apiv1.Use(a.requireCSRFToken)

apiv1.HandleFunc("/blocks", a.sessionRequired(a.handleGetBlocks)).Methods("GET")
apiv1.HandleFunc("/blocks", a.sessionRequired(a.handlePostBlocks)).Methods("POST")
apiv1.HandleFunc("/blocks/{blockID}", a.sessionRequired(a.handleDeleteBlock)).Methods("DELETE")
apiv1.HandleFunc("/blocks/{blockID}/subtree", a.attachSession(a.handleGetSubTree, false)).Methods("GET")

apiv1.HandleFunc("/users/me", a.sessionRequired(a.handleGetMe)).Methods("GET")
apiv1.HandleFunc("/users/{userID}", a.sessionRequired(a.handleGetUser)).Methods("GET")
apiv1.HandleFunc("/users/{userID}/changepassword", a.sessionRequired(a.handleChangePassword)).Methods("POST")

r.HandleFunc("/api/v1/users/me", a.sessionRequired(a.handleGetMe)).Methods("GET")
r.HandleFunc("/api/v1/users/{userID}", a.sessionRequired(a.handleGetUser)).Methods("GET")
r.HandleFunc("/api/v1/users/{userID}/changepassword", a.sessionRequired(a.handleChangePassword)).Methods("POST")
apiv1.HandleFunc("/#", a.handleLogin).Methods("POST")
apiv1.HandleFunc("/register", a.handleRegister).Methods("POST")

r.HandleFunc("/api/v1/#", a.handleLogin).Methods("POST")
r.HandleFunc("/api/v1/register", a.handleRegister).Methods("POST")
apiv1.HandleFunc("/blocks/export", a.sessionRequired(a.handleExport)).Methods("GET")
apiv1.HandleFunc("/blocks/import", a.sessionRequired(a.handleImport)).Methods("POST")

r.HandleFunc("/api/v1/files", a.sessionRequired(a.handleUploadFile)).Methods("POST")
r.HandleFunc("/files/{filename}", a.sessionRequired(a.handleServeFile)).Methods("GET")
apiv1.HandleFunc("/sharing/{rootID}", a.sessionRequired(a.handlePostSharing)).Methods("POST")
apiv1.HandleFunc("/sharing/{rootID}", a.sessionRequired(a.handleGetSharing)).Methods("GET")

r.HandleFunc("/api/v1/blocks/export", a.sessionRequired(a.handleExport)).Methods("GET")
r.HandleFunc("/api/v1/blocks/import", a.sessionRequired(a.handleImport)).Methods("POST")
apiv1.HandleFunc("/workspace", a.sessionRequired(a.handleGetWorkspace)).Methods("GET")
apiv1.HandleFunc("/workspace/regenerate_#_token", a.sessionRequired(a.handlePostWorkspaceRegenerate#Token)).Methods("POST")

r.HandleFunc("/api/v1/sharing/{rootID}", a.sessionRequired(a.handlePostSharing)).Methods("POST")
r.HandleFunc("/api/v1/sharing/{rootID}", a.sessionRequired(a.handleGetSharing)).Methods("GET")
// Files API

r.HandleFunc("/api/v1/workspace", a.sessionRequired(a.handleGetWorkspace)).Methods("GET")
r.HandleFunc("/api/v1/workspace/regenerate_#_token", a.sessionRequired(a.handlePostWorkspaceRegenerate#Token)).Methods("POST")
files := r.PathPrefix("/files/").Subrouter()
files.Use(a.requireCSRFToken)

files.HandleFunc("/", a.sessionRequired(a.handleUploadFile)).Methods("POST")
files.HandleFunc("/{filename}", a.sessionRequired(a.handleServeFile)).Methods("GET")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure about the need of the CSRFToken check for the static files, actually I think it should be only for the API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't have much control over the image loading and files downloading in the browser to add that header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I had refactored contentBlock to get the images with fetch, passing the session token and CSRF header. However, I removed the CSRF check for GET /files for now, as that (in theory) shouldn't be a CSRF vulnerability as no state is changed. We can think about this one more, and change later if needed.

}

func (a *API) RegisterAdminRoutes(r *mux.Router) {
r.HandleFunc("/api/v1/admin/users/{username}/password", a.adminRequired(a.handleAdminSetPassword)).Methods("POST")
}

func (a *API) requireCSRFToken(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if !a.checkCSRFToken(r) {
log.Println("checkCSRFToken FAILED")
errorResponse(w, http.StatusBadRequest, nil, nil)
return
}

next.ServeHTTP(w, r)
})
}

func (a *API) checkCSRFToken(r *http.Request) bool {
token := r.Header.Get(HEADER_REQUESTED_WITH)

if token == HEADER_REQUESTED_WITH_XML {
return true
}

return false
}

func (a *API) handleGetBlocks(w http.ResponseWriter, r *http.Request) {
query := r.URL.Query()
parentID := query.Get("parent_id")
Expand Down
5 changes: 4 additions & 1 deletion server/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ type Client struct {

func NewClient(url string) *Client {
url = strings.TrimRight(url, "/")
return &Client{url, url + API_URL_SUFFIX, &http.Client{}, map[string]string{}}
headers := map[string]string{
"X-Requested-With": "XMLHttpRequest",
}
return &Client{url, url + API_URL_SUFFIX, &http.Client{}, headers}
}

func (c *Client) DoApiGet(url string, etag string) (*http.Response, error) {
Expand Down
1 change: 1 addition & 0 deletions webapp/src/octoClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ class OctoClient {
Accept: 'application/json',
'Content-Type': 'application/json',
Authorization: this.token ? 'Bearer ' + this.token : '',
'X-Requested-With': 'XMLHttpRequest',
}
}

Expand Down