mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-24 16:41:30 +02:00
[management] Expose the ETag response header to browser clients
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.
This commit is contained in:
@@ -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,
|
||||
})
|
||||
}
|
||||
|
||||
88
management/server/http/handler_test.go
Normal file
88
management/server/http/handler_test.go
Normal file
@@ -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")
|
||||
}
|
||||
Reference in New Issue
Block a user