From b37020d14a647992be6af1a996d7b56f13012d23 Mon Sep 17 00:00:00 2001 From: Patrick O'Doherty Date: Thu, 1 May 2025 10:51:53 -0700 Subject: [PATCH] golink: revert Sec-Fetch-Site xsrftoken replacement Unfortunately Sec-Fetch-Site headers are not sent over plaintext HTTP making this unsuitable for non-TLS deployments. Reverts c89d35095a4884eb84f7652b3376cf789592715f Reverts 7646755c2e049abfeb65015115964c0bd3b2c887 Reverts a1ce1eb2b611877a42ae612e031c45b660543c51 Updates #160 Updates #156 Updates #130 Signed-off-by: Patrick O'Doherty --- flake.nix | 2 +- go.mod | 2 +- golink.go | 97 ++++++++++++++++++++------------- golink_test.go | 139 ++++++++++++++++------------------------------- tmpl/delete.html | 1 + tmpl/detail.html | 3 +- tmpl/help.html | 3 - tmpl/home.html | 1 + 8 files changed, 111 insertions(+), 137 deletions(-) diff --git a/flake.nix b/flake.nix index a345d4a..8ee0d4f 100644 --- a/flake.nix +++ b/flake.nix @@ -39,7 +39,7 @@ "-X tailscale.com/version.longStamp=${tsVersion}" "-X tailscale.com/version.shortStamp=${tsVersion}" ]; - vendorHash = "sha256-PUobfmZyn5YtiFTpxTtjnPhqijR5tsONk4HNlIs1oNk="; # SHA based on vendoring go.mod + vendorHash = "sha256-RqKsIgwkCbIMQdCKtSHqW9eiZuNcZLCYeXFhPbgxjEU="; # SHA based on vendoring go.mod }; }); diff --git a/go.mod b/go.mod index f35987e..74b2be0 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.24.0 require ( github.com/google/go-cmp v0.6.0 + golang.org/x/net v0.38.0 modernc.org/sqlite v1.19.4 tailscale.com v1.82.5 ) @@ -76,7 +77,6 @@ require ( golang.org/x/crypto v0.36.0 // indirect golang.org/x/exp v0.0.0-20250210185358-939b2ce775ac // indirect golang.org/x/mod v0.23.0 // indirect - golang.org/x/net v0.38.0 // indirect golang.org/x/sync v0.12.0 // indirect golang.org/x/sys v0.31.0 // indirect golang.org/x/term v0.30.0 // indirect diff --git a/golink.go b/golink.go index 74bcc10..0b98f66 100644 --- a/golink.go +++ b/golink.go @@ -10,6 +10,7 @@ import ( "context" "crypto/rand" "embed" + "encoding/base64" "encoding/json" "errors" "flag" @@ -29,6 +30,7 @@ import ( texttemplate "text/template" "time" + "golang.org/x/net/xsrftoken" "tailscale.com/client/tailscale" "tailscale.com/hostinfo" "tailscale.com/ipn" @@ -39,8 +41,17 @@ import ( const ( defaultHostname = "go" - secFetchSite = "Sec-Fetch-Site" - secGolink = "Sec-Golink" + + // 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 ( @@ -200,8 +211,6 @@ out: fqdn := strings.TrimSuffix(status.Self.DNSName, ".") httpHandler := serveHandler() - httpHandler = EnforceSecFetchSiteOrSecGolink(httpHandler) - if enableTLS { httpsHandler := HSTS(httpHandler) httpHandler = redirectHandler(fqdn) @@ -266,6 +275,7 @@ type homeData struct { Short string Long string Clicks []visitData + XSRF string ReadOnly bool } @@ -273,8 +283,11 @@ type homeData struct { type deleteData struct { Short string Long string + XSRF string } +var xsrfKey string + func init() { homeTmpl = newTemplate("base.html", "home.html") detailTmpl = newTemplate("base.html", "detail.html") @@ -286,6 +299,7 @@ func init() { b := make([]byte, 24) rand.Read(b) + xsrfKey = base64.StdEncoding.EncodeToString(b) } var tmplFuncs = template.FuncMap{ @@ -402,34 +416,6 @@ func HSTS(h http.Handler) http.Handler { }) } -// EnforceSecFetchSiteOrSecGolink is a Cross-Site Request Forgery protection -// middleware that validates the Sec-Fetch-Site header for non-idempotent -// requests. It requires clients to send Sec-Fetch-Site set to "same-origin". -// -// It alternatively allows for clients to send the header "Sec-Golink" set to -// any value to maintain compatibility with clients developed against earlier -// versions of golink that relied on xsrf token based CSRF protection. -func EnforceSecFetchSiteOrSecGolink(h http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch r.Method { - case "GET", "HEAD", "OPTIONS": // allow idempotent methods - h.ServeHTTP(w, r) - return - } - - // Check for Sec-Fetch-Site header set to "same-origin" - // or Sec-Golink header set to any value for backwards compatibility. - sameOrigin := r.Header.Get(secFetchSite) == "same-origin" - secGolink := r.Header.Get(secGolink) != "" - if sameOrigin || secGolink { - h.ServeHTTP(w, r) - return - } - - http.Error(w, "invalid non `Sec-Fetch-Site: same-origin` request", http.StatusBadRequest) - }) -} - // serverHandler returns the main http.Handler for serving all requests. func serveHandler() http.Handler { mux := http.NewServeMux() @@ -490,10 +476,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), ReadOnly: *readonly, }) } @@ -605,6 +597,7 @@ type detailData struct { // Editable indicates whether the current user can edit the link. Editable bool Link *Link + XSRF string } func serveDetail(w http.ResponseWriter, r *http.Request) { @@ -648,6 +641,7 @@ func serveDetail(w http.ResponseWriter, r *http.Request) { data := detailData{ Link: link, Editable: canEdit, + XSRF: xsrftoken.Generate(xsrfKey, cu.login, link.Short), } if canEdit && !ownerExists { data.Link.Owner = cu.login @@ -835,6 +829,16 @@ 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, link.Short) { + http.Error(w, "invalid XSRF token", http.StatusBadRequest) + return + } + if err := db.Delete(short); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -844,6 +848,7 @@ func serveDelete(w http.ResponseWriter, r *http.Request) { deleteTmpl.Execute(w, deleteData{ Short: link.Short, Long: link.Long, + XSRF: xsrftoken.Generate(xsrfKey, cu.login, newShortName), }) } @@ -881,15 +886,20 @@ func serveSave(w http.ResponseWriter, r *http.Request) { return } - // Prevent accidental overwrites of existing links. - // If the link already exists, make sure this request is an intentional update. - if link != nil && r.FormValue("update") == "" { - http.Error(w, "link already exists", http.StatusForbidden) + if !canEditLink(r.Context(), link, cu) { + http.Error(w, fmt.Sprintf("cannot update link owned by %q", link.Owner), http.StatusForbidden) return } - if !canEditLink(r.Context(), link, cu) { - http.Error(w, fmt.Sprintf("cannot update link owned by %q", link.Owner), http.StatusForbidden) + // short name to use for XSRF token. + // For new link creation, the special newShortName value is used. + tokenShortName := newShortName + if link != nil { + tokenShortName = link.Short + } + + if !isRequestAuthorized(r, cu, tokenShortName) { + http.Error(w, "invalid XSRF token", http.StatusBadRequest) return } @@ -1067,3 +1077,14 @@ 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 + } + + return xsrftoken.Valid(r.PostFormValue("xsrf"), xsrfKey, u.login, short) +} diff --git a/golink_test.go b/golink_test.go index 8d6f1f5..5842112 100644 --- a/golink_test.go +++ b/golink_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "golang.org/x/net/xsrftoken" "tailscale.com/tstest" "tailscale.com/types/ptr" "tailscale.com/util/must" @@ -22,75 +23,6 @@ func init() { *dev = ":8080" } -func TestEnforceSecFetchSiteOrSecGolink(t *testing.T) { - mux := http.NewServeMux() - mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }) - handler := EnforceSecFetchSiteOrSecGolink(mux) - - tests := []struct { - name string - method string - withSameOrigin bool - withCrossSite bool - expectSuccess bool - withSecGolink bool - }{ - { - name: "GET without header succeeds", - method: http.MethodGet, - withSameOrigin: false, - expectSuccess: true, - }, - { - name: "POST without header fails", - method: http.MethodPost, - }, - { - name: "POST with same-origin header succeeds", - method: http.MethodPost, - withSameOrigin: true, - expectSuccess: true, - }, - { - name: "POST with cross-site header fails", - method: http.MethodPost, - withCrossSite: true, - }, - { - name: "POST with sec-golink header succeeds", - method: http.MethodPost, - withSecGolink: true, - expectSuccess: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - r := httptest.NewRequest(tt.method, "/", nil) - if tt.withSameOrigin { - r.Header.Set(secFetchSite, "same-origin") - } - if tt.withCrossSite { - r.Header.Set(secFetchSite, "cross-site") - } - if tt.withSecGolink { - r.Header.Set(secGolink, "true") - } - - w := httptest.NewRecorder() - handler.ServeHTTP(w, r) - - if w.Code != http.StatusOK && tt.expectSuccess { - t.Errorf("expected status OK, got %d", w.Code) - } else if w.Code == http.StatusOK && !tt.expectSuccess { - t.Errorf("expected non-OK status, got %d", w.Code) - } - }) - } -} - func TestServeGo(t *testing.T) { var err error db, err = NewSQLiteDB(":memory:") @@ -226,11 +158,18 @@ func TestServeSave(t *testing.T) { } 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 - update bool allowUnknownUsers bool currentUser func(*http.Request) (user, error) wantStatus int @@ -250,25 +189,14 @@ func TestServeSave(t *testing.T) { { name: "save simple link", short: "who", + xsrf: fooXSRF(newShortName), long: "http://who/", wantStatus: http.StatusOK, }, - { - name: "disallow accidental updates", - short: "who", - long: "http://who2/", - wantStatus: http.StatusForbidden, - }, - { - name: "allow intentional updates", - short: "who", - long: "http://who/", - update: true, - 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, @@ -276,22 +204,23 @@ 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", - update: true, currentUser: func(*http.Request) (user, error) { return user{login: "bar@example.com"}, nil }, wantStatus: http.StatusOK, }, { name: "admins can edit any link", short: "who", + xsrf: barXSRF("who"), long: "http://who/", - update: true, currentUser: func(*http.Request) (user, error) { return user{login: "bar@example.com", isAdmin: true}, nil }, wantStatus: http.StatusOK, }, { 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, @@ -304,6 +233,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 { @@ -320,15 +256,11 @@ func TestServeSave(t *testing.T) { *allowUnknownUsers = tt.allowUnknownUsers t.Cleanup(func() { *allowUnknownUsers = oldAllowUnknownUsers }) - v := url.Values{ + r := httptest.NewRequest("POST", "/", strings.NewReader(url.Values{ "short": {tt.short}, "long": {tt.long}, - } - if tt.update { - v.Set("update", "1") - } - - r := httptest.NewRequest("POST", "/", strings.NewReader(v.Encode())) + "xsrf": {tt.xsrf}, + }.Encode())) r.Header.Set("Content-Type", "application/x-www-form-urlencoded") w := httptest.NewRecorder() serveSave(w, r) @@ -350,9 +282,14 @@ func TestServeDelete(t *testing.T) { db.Save(&Link{Short: "foo", Owner: "foo@example.com"}) db.Save(&Link{Short: "link-owned-by-tagged-devices", Long: "/before", Owner: "tagged-devices"}) + xsrf := func(short string) string { + return xsrftoken.Generate(xsrfKey, "foo@example.com", short) + } + tests := []struct { name string short string + xsrf string currentUser func(*http.Request) (user, error) wantStatus int }{ @@ -374,14 +311,28 @@ func TestServeDelete(t *testing.T) { { name: "allow deleting link owned by tagged-devices", short: "link-owned-by-tagged-devices", + xsrf: xsrf("link-owned-by-tagged-devices"), wantStatus: http.StatusOK, }, { name: "admin can delete unowned link", short: "a", currentUser: func(*http.Request) (user, error) { return user{login: "foo@example.com", isAdmin: true}, nil }, + xsrf: xsrf("a"), wantStatus: http.StatusOK, }, + { + name: "invalid xsrf", + short: "foo", + xsrf: xsrf("invalid"), + wantStatus: http.StatusBadRequest, + }, + { + name: "valid xsrf", + short: "foo", + xsrf: xsrf("foo"), + wantStatus: http.StatusOK, + }, } for _, tt := range tests { @@ -394,7 +345,9 @@ func TestServeDelete(t *testing.T) { }) } - r := httptest.NewRequest("POST", "/.delete/"+tt.short, nil) + r := httptest.NewRequest("POST", "/.delete/"+tt.short, strings.NewReader(url.Values{ + "xsrf": {tt.xsrf}, + }.Encode())) r.Header.Set("Content-Type", "application/x-www-form-urlencoded") w := httptest.NewRecorder() serveDelete(w, r) diff --git a/tmpl/delete.html b/tmpl/delete.html index 144049d..b84edc4 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 744807a..95adb19 100644 --- a/tmpl/detail.html +++ b/tmpl/detail.html @@ -3,6 +3,7 @@

Link Details

{{ if .Editable }} +
@@ -26,13 +27,13 @@

Link Details

{{.Link.LastEdit.Format "Jan _2, 2006 3:04pm MST"}}
-

Danger Zone

+
diff --git a/tmpl/help.html b/tmpl/help.html index ea4a998..7b66b61 100644 --- a/tmpl/help.html +++ b/tmpl/help.html @@ -139,8 +139,5 @@

Application Programming Interface (API)

{{`{"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"}`}} -

-To update an existing link, also include `-d update=1`. - {{ end }} diff --git a/tmpl/home.html b/tmpl/home.html index c7a10cf..1e39b81 100644 --- a/tmpl/home.html +++ b/tmpl/home.html @@ -8,6 +8,7 @@

Create a new link

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

{{ end }}
+