From c66cbb8eca0e7fbd00722e990e4e09cfe85bcb54 Mon Sep 17 00:00:00 2001 From: Chris Palmer Date: Mon, 1 Apr 2024 18:20:58 -0700 Subject: [PATCH] Protect save and update from CSRF (#117) Also, fix a lint. Fixes #116 Signed-off-by: Chris Palmer --- golink.go | 68 ++++++++++++++++++++++++++++++++++++++++++++---- golink_test.go | 24 +++++++++++++++-- tmpl/delete.html | 1 + tmpl/detail.html | 1 + tmpl/help.html | 2 +- tmpl/home.html | 1 + 6 files changed, 89 insertions(+), 8 deletions(-) diff --git a/golink.go b/golink.go index f18d600..298eb86 100644 --- a/golink.go +++ b/golink.go @@ -39,7 +39,20 @@ import ( "tailscale.com/util/dnsname" ) -const defaultHostname = "go" +const ( + defaultHostname = "go" + + // Used as a placeholder short name for generating the XSRF defense token, + // when creating new links. + newShortName = ".new" + + // If the caller sends this header set to a non-empty value, we will allow + // them to make the call even without an XSRF token. JavaScript in browser + // cannot set this header, per the [Fetch Spec]. + // + // [Fetch Spec]: https://fetch.spec.whatwg.org + secHeaderName = "Sec-Golink" +) var ( verbose = flag.Bool("verbose", false, "be verbose") @@ -254,11 +267,19 @@ type visitData struct { NumClicks int } -// homeData is the data used by the homeTmpl template. +// homeData is the data used by homeTmpl. type homeData struct { Short string Long string Clicks []visitData + XSRF string +} + +// deleteData is the data used by deleteTmpl. +type deleteData struct { + Short string + Long string + XSRF string } var xsrfKey string @@ -416,10 +437,16 @@ func serveHome(w http.ResponseWriter, r *http.Request, short string) { } } + cu, err := currentUser(r) + if err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } homeTmpl.Execute(w, homeData{ Short: short, Long: long, Clicks: clicks, + XSRF: xsrftoken.Generate(xsrfKey, cu.login, newShortName), }) } @@ -739,6 +766,11 @@ func serveDelete(w http.ResponseWriter, r *http.Request) { return } + // Deletion by CLI has never worked because it has always required the XSRF + // token. (Refer to commit c7ac33d04c33743606f6224009a5c73aa0b8dec0.) If we + // want to enable deletion via CLI and to honor allowUnknownUsers for + // deletion, we could change the below to a call to isRequestAuthorized. For + // now, always require the XSRF token, thus maintaining the status quo. if !xsrftoken.Valid(r.PostFormValue("xsrf"), xsrfKey, cu.login, short) { http.Error(w, "invalid XSRF token", http.StatusBadRequest) return @@ -750,7 +782,11 @@ func serveDelete(w http.ResponseWriter, r *http.Request) { } deleteLinkStats(link) - deleteTmpl.Execute(w, link) + deleteTmpl.Execute(w, deleteData{ + Short: link.Short, + Long: link.Long, + XSRF: xsrftoken.Generate(xsrfKey, cu.login, newShortName), + }) } // serveSave handles requests to save or update a Link. Both short name and @@ -787,6 +823,11 @@ func serveSave(w http.ResponseWriter, r *http.Request) { return } + if !isRequestAuthorized(r, cu, short) { + http.Error(w, "invalid XSRF token", http.StatusBadRequest) + return + } + // allow transferring ownership to valid users. If empty, set owner to current user. owner := r.FormValue("owner") if owner != "" { @@ -886,8 +927,7 @@ func restoreLastSnapshot() error { _, err := db.Load(link.Short) if err == nil { continue // exists - } - if err != nil && !errors.Is(err, fs.ErrNotExist) { + } else if !errors.Is(err, fs.ErrNotExist) { return err } if err := db.Save(link); err != nil { @@ -923,3 +963,21 @@ func resolveLink(link *url.URL) (*url.URL, error) { } return dst, err } + +func isRequestAuthorized(r *http.Request, u user, short string) bool { + if *allowUnknownUsers { + return true + } + if r.Header.Get(secHeaderName) != "" { + return true + } + + // If the request is to create a new link, test the XSRF token against + // newShortName instead of short. + tokenShortName := short + _, err := db.Load(short) + if r.URL.Path == "/" && r.Method == http.MethodPost && errors.Is(err, fs.ErrNotExist) { + tokenShortName = newShortName + } + return xsrftoken.Valid(r.PostFormValue("xsrf"), xsrfKey, u.login, tokenShortName) +} diff --git a/golink_test.go b/golink_test.go index 2cd29f8..8ec39fb 100644 --- a/golink_test.go +++ b/golink_test.go @@ -129,12 +129,19 @@ func TestServeSave(t *testing.T) { if err != nil { t.Fatal(err) } - db.Save(&Link{Short: "link-owned-by-tagged-devices", Long: "/before", Owner: "tagged-devices"}) + fooXSRF := func(short string) string { + return xsrftoken.Generate(xsrfKey, "foo@example.com", short) + } + barXSRF := func(short string) string { + return xsrftoken.Generate(xsrfKey, "bar@example.com", short) + } + tests := []struct { name string short string + xsrf string long string allowUnknownUsers bool currentUser func(*http.Request) (user, error) @@ -155,12 +162,14 @@ func TestServeSave(t *testing.T) { { name: "save simple link", short: "who", + xsrf: fooXSRF(newShortName), long: "http://who/", wantStatus: http.StatusOK, }, { name: "disallow editing another's link", short: "who", + xsrf: barXSRF("who"), long: "http://who/", currentUser: func(*http.Request) (user, error) { return user{login: "bar@example.com"}, nil }, wantStatus: http.StatusForbidden, @@ -168,6 +177,7 @@ func TestServeSave(t *testing.T) { { name: "allow editing link owned by tagged-devices", short: "link-owned-by-tagged-devices", + xsrf: barXSRF("link-owned-by-tagged-devices"), long: "/after", currentUser: func(*http.Request) (user, error) { return user{login: "bar@example.com"}, nil }, wantStatus: http.StatusOK, @@ -175,6 +185,7 @@ func TestServeSave(t *testing.T) { { name: "admins can edit any link", short: "who", + xsrf: barXSRF("who"), long: "http://who/", currentUser: func(*http.Request) (user, error) { return user{login: "bar@example.com", isAdmin: true}, nil }, wantStatus: http.StatusOK, @@ -182,6 +193,7 @@ func TestServeSave(t *testing.T) { { name: "disallow unknown users", short: "who2", + xsrf: fooXSRF("who2"), long: "http://who/", currentUser: func(*http.Request) (user, error) { return user{}, errors.New("") }, wantStatus: http.StatusInternalServerError, @@ -194,6 +206,13 @@ func TestServeSave(t *testing.T) { currentUser: func(*http.Request) (user, error) { return user{}, nil }, wantStatus: http.StatusOK, }, + { + name: "invalid xsrf", + short: "goat", + xsrf: fooXSRF("sheep"), + long: "https://goat.example.com/goat.php?goat=true", + wantStatus: http.StatusBadRequest, + }, } for _, tt := range tests { @@ -213,6 +232,7 @@ func TestServeSave(t *testing.T) { r := httptest.NewRequest("POST", "/", strings.NewReader(url.Values{ "short": {tt.short}, "long": {tt.long}, + "xsrf": {tt.xsrf}, }.Encode())) r.Header.Set("Content-Type", "application/x-www-form-urlencoded") w := httptest.NewRecorder() @@ -252,7 +272,7 @@ func TestServeDelete(t *testing.T) { wantStatus: http.StatusBadRequest, }, { - name: "non-existant link", + name: "nonexistent link", short: "does-not-exist", wantStatus: http.StatusNotFound, }, diff --git a/tmpl/delete.html b/tmpl/delete.html index 2857e45..a4d5bf5 100644 --- a/tmpl/delete.html +++ b/tmpl/delete.html @@ -4,6 +4,7 @@

Link go/{{.Short}} Deleted

Deleted this by mistake? You can recreate the same link below.

+
diff --git a/tmpl/detail.html b/tmpl/detail.html index fba48ce..0c5dbf6 100644 --- a/tmpl/detail.html +++ b/tmpl/detail.html @@ -3,6 +3,7 @@

Link Details

{{ if .Editable }} +
diff --git a/tmpl/help.html b/tmpl/help.html index b64eb36..3d10406 100644 --- a/tmpl/help.html +++ b/tmpl/help.html @@ -122,7 +122,7 @@

Application Programming Interface (API)

Create a new link by sending a POST request with a short and long value: -

{{`$ curl -L -d short=cs -d long=https://cs.github.com/ go
+
{{`$ curl -L -H Sec-Golink:1 -d short=cs -d long=https://cs.github.com/ go
 {"Short":"cs","Long":"https://cs.github.com/","Created":"2022-06-03T22:15:29.993978392Z","LastEdit":"2022-06-03T22:15:29.993978392Z","Owner":"amelie@example.com"}`}}
 

 
diff --git a/tmpl/home.html b/tmpl/home.html
index e85229e..64e2267 100644
--- a/tmpl/home.html
+++ b/tmpl/home.html
@@ -5,6 +5,7 @@ 
Create a new link

     
Did you mean {{.}} ? Create a go link for it now:

     {{ end }}
     
+