From 35137326f73b7bef7098cf1d5aec412a7d7deff8 Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Tue, 11 Aug 2026 11:51:00 +0200 Subject: [PATCH] [management] Expose the ETag response header to browser clients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ETag is not a CORS-safelisted response header, so JavaScript cannot read it unless the server names it in Access-Control-Expose-Headers. Without that the API hands a browser client a validator it has no way to see, and conditional requests are available to the CLI, the REST client and Terraform but silently not to the dashboard — the one client where a lost update is a person's work disappearing rather than a plan reverting. The library offers no way to extend cors.AllowAll(), so the policy is spelled out verbatim with ExposedHeaders added. That is a wider blast radius than the one field deserves, which is why the tests assert the rest of the policy too: any origin, no credentials, the full method set. If-Match needs no grant of its own, since AllowedHeaders is already "*" — the preflight test guards against a later narrowing that would leave writes readable but not conditional. Collapsing this back to cors.AllowAll() is the obvious tidy-up and would silently undo it, which is what the tests are there to catch. --- management/server/http/handler.go | 31 ++++++++- management/server/http/handler_test.go | 88 ++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 management/server/http/handler_test.go diff --git a/management/server/http/handler.go b/management/server/http/handler.go index a57f44b3c..98c54c8e8 100644 --- a/management/server/http/handler.go +++ b/management/server/http/handler.go @@ -98,7 +98,7 @@ func NewAPIHandler(ctx context.Context, router *mux.Router, accountManager accou isValidChildAccount, ) - corsMiddleware := cors.AllowAll() + corsMiddleware := newCORSMiddleware() metricsMiddleware := appMetrics.HTTPMiddleware() @@ -145,3 +145,32 @@ func NewAPIHandler(ctx context.Context, router *mux.Router, accountManager accou return router, nil } + +// newCORSMiddleware builds the API's CORS policy: cors.AllowAll() plus ETag in +// ExposedHeaders. +// +// The addition is what makes conditional requests usable from a browser. A +// response header that is not CORS-safelisted is invisible to JavaScript +// unless it is named in Access-Control-Expose-Headers, and ETag is not on that +// list — so without this the server can hand a browser client a validator it +// has no way to read, leaving conditional requests to non-browser clients +// only. If-Match needs nothing further, since AllowedHeaders is already "*". +// +// Everything else mirrors cors.AllowAll() exactly. It is spelled out rather +// than called because the library offers no way to extend it. +func newCORSMiddleware() *cors.Cors { + return cors.New(cors.Options{ + AllowedOrigins: []string{"*"}, + AllowedMethods: []string{ + http.MethodHead, + http.MethodGet, + http.MethodPost, + http.MethodPut, + http.MethodPatch, + http.MethodDelete, + }, + AllowedHeaders: []string{"*"}, + ExposedHeaders: []string{"ETag"}, + AllowCredentials: false, + }) +} diff --git a/management/server/http/handler_test.go b/management/server/http/handler_test.go new file mode 100644 index 000000000..4314eca9c --- /dev/null +++ b/management/server/http/handler_test.go @@ -0,0 +1,88 @@ +package http + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCORSExposesETag pins the reason this policy is spelled out instead of +// being cors.AllowAll(). ETag is not a CORS-safelisted response header, so +// without it named in Access-Control-Expose-Headers a browser client is handed +// a validator it cannot read — conditional requests would work for the CLI, +// the REST client and Terraform, and silently not for the dashboard. +// +// Collapsing this back to cors.AllowAll() is exactly the simplification that +// would reintroduce that, which is what this test is here to catch. +func TestCORSExposesETag(t *testing.T) { + handler := newCORSMiddleware().Handler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("ETag", `"9f86d081884c7d65"`) + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest(http.MethodGet, "/api/agent-network/settings", nil) + req.Header.Set("Origin", "https://app.netbird.io") + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + + // Compared canonicalized: the library normalizes the name it echoes, so + // this reads "Etag" rather than "ETag". Browsers match the exposed-header + // list case-insensitively, so the spelling does not matter — but asserting + // it byte-exactly would fail for a reason that has nothing to do with the + // behaviour being pinned. + assert.Equal(t, http.CanonicalHeaderKey("ETag"), + http.CanonicalHeaderKey(rec.Header().Get("Access-Control-Expose-Headers")), + "browser clients must be allowed to read the validator they are sent") +} + +// TestCORSAllowsIfMatchPreflight covers the request half. It needs nothing +// beyond the wildcard AllowedHeaders that was already there, so this is a +// regression guard rather than a new grant: narrowing AllowedHeaders to a list +// later must not drop If-Match and leave writes readable but not conditional. +func TestCORSAllowsIfMatchPreflight(t *testing.T) { + handler := newCORSMiddleware().Handler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest(http.MethodOptions, "/api/agent-network/settings", nil) + req.Header.Set("Origin", "https://app.netbird.io") + req.Header.Set("Access-Control-Request-Method", http.MethodPut) + req.Header.Set("Access-Control-Request-Headers", "If-Match") + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Contains(t, rec.Header().Get("Access-Control-Allow-Headers"), "If-Match", + "a conditional write must survive preflight") + assert.Contains(t, rec.Header().Get("Access-Control-Allow-Methods"), http.MethodPut, + "the conditional write's method must survive preflight") +} + +// TestCORSMatchesAllowAllOtherwise pins the rest of the policy, which is a +// verbatim copy of cors.AllowAll(). Spelling the options out is what let ETag +// be added; it also means a change to the library's defaults no longer reaches +// this API, so the settings that matter are asserted here rather than assumed. +func TestCORSMatchesAllowAllOtherwise(t *testing.T) { + handler := newCORSMiddleware().Handler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest(http.MethodOptions, "/api/peers", nil) + req.Header.Set("Origin", "https://anywhere.example.com") + req.Header.Set("Access-Control-Request-Method", http.MethodDelete) + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, "*", rec.Header().Get("Access-Control-Allow-Origin"), "any origin must still be allowed") + assert.Empty(t, rec.Header().Get("Access-Control-Allow-Credentials"), + "credentials must stay disallowed — allowing them alongside a wildcard origin would be a real weakening") + assert.Contains(t, rec.Header().Get("Access-Control-Allow-Methods"), http.MethodDelete, + "the full method set must still be allowed") +}