diff --git a/cmd/syncthing/gui_csrf.go b/cmd/syncthing/gui_csrf.go index ccb90b61f..74e425717 100644 --- a/cmd/syncthing/gui_csrf.go +++ b/cmd/syncthing/gui_csrf.go @@ -17,9 +17,16 @@ import ( "github.com/syncthing/syncthing/lib/sync" ) +// csrfTokens is a list of valid tokens. It is sorted so that the most +// recently used token is first in the list. New tokens are added to the front +// of the list (as it is the most recently used at that time). The list is +// pruned to a maximum of maxCsrfTokens, throwing away the least recently used +// tokens. var csrfTokens []string var csrfMut = sync.NewMutex() +const maxCsrfTokens = 25 + // Check for CSRF token on /rest/ URLs. If a correct one is not given, reject // the request with 403. For / and /index.html, set a new CSRF cookie if none // is currently set. @@ -36,6 +43,7 @@ func csrfMiddleware(unique, prefix, apiKey string, next http.Handler) http.Handl if !strings.HasPrefix(r.URL.Path, prefix) { cookie, err := r.Cookie("CSRF-Token-" + unique) if err != nil || !validCsrfToken(cookie.Value) { + httpl.Debugln("new CSRF cookie in response to request for", r.URL) cookie = &http.Cookie{ Name: "CSRF-Token-" + unique, Value: newCsrfToken(), @@ -47,7 +55,13 @@ func csrfMiddleware(unique, prefix, apiKey string, next http.Handler) http.Handl } if r.Method == "GET" { - // Allow GET requests unconditionally + // Allow GET requests unconditionally, but if we got the CSRF + // token cookie do the verification anyway so we keep the + // csrfTokens list sorted by recent usage. We don't care about the + // outcome of the validity check. + if cookie, err := r.Cookie("CSRF-Token-" + unique); err == nil { + validCsrfToken(cookie.Value) + } next.ServeHTTP(w, r) return } @@ -66,8 +80,15 @@ func csrfMiddleware(unique, prefix, apiKey string, next http.Handler) http.Handl func validCsrfToken(token string) bool { csrfMut.Lock() defer csrfMut.Unlock() - for _, t := range csrfTokens { + for i, t := range csrfTokens { if t == token { + if i > 0 { + // Move this token to the head of the list. Copy the tokens at + // the front one step to the right and then replace the token + // at the head. + copy(csrfTokens[1:], csrfTokens[:i+1]) + csrfTokens[0] = token + } return true } } @@ -78,9 +99,9 @@ func newCsrfToken() string { token := randomString(32) csrfMut.Lock() - csrfTokens = append(csrfTokens, token) - if len(csrfTokens) > 10 { - csrfTokens = csrfTokens[len(csrfTokens)-10:] + csrfTokens = append([]string{token}, csrfTokens...) + if len(csrfTokens) > maxCsrfTokens { + csrfTokens = csrfTokens[:maxCsrfTokens] } defer csrfMut.Unlock() diff --git a/cmd/syncthing/gui_test.go b/cmd/syncthing/gui_test.go new file mode 100644 index 000000000..f286fc2e4 --- /dev/null +++ b/cmd/syncthing/gui_test.go @@ -0,0 +1,42 @@ +// Copyright (C) 2016 The Syncthing Authors. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +package main + +import "testing" + +func TestCSRFToken(t *testing.T) { + t1 := newCsrfToken() + t2 := newCsrfToken() + + t3 := newCsrfToken() + if !validCsrfToken(t3) { + t.Fatal("t3 should be valid") + } + + for i := 0; i < 250; i++ { + if i%5 == 0 { + // t1 and t2 should remain valid by virtue of us checking them now + // and then. + if !validCsrfToken(t1) { + t.Fatal("t1 should be valid at iteration", i) + } + if !validCsrfToken(t2) { + t.Fatal("t2 should be valid at iteration", i) + } + } + + // The newly generated token is always valid + t4 := newCsrfToken() + if !validCsrfToken(t4) { + t.Fatal("t4 should be valid at iteration", i) + } + } + + if validCsrfToken(t3) { + t.Fatal("t3 should have expired by now") + } +}