From 9b0b7f44275a3f83879d5ddc441e7073f33ff02c Mon Sep 17 00:00:00 2001 From: Adin Schmahmann Date: Mon, 25 Nov 2024 12:01:22 -0500 Subject: [PATCH] Error instead of dropping unauthorized headers (#201) * fix: returns an HTTP 401 if authorization headers are required, but are not there or incorrect rather than silently ignoring those headers --- CHANGELOG.md | 1 + docs/headers.md | 6 +++--- handler_test.go | 14 ++++---------- handlers.go | 11 +++-------- 4 files changed, 11 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f4262c..cb7e1c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The following emojis are used to highlight certain changes: - boxo [v0.24.3](https://github.com/ipfs/boxo/releases/tag/v0.24.3) - go-libp2p-kad-dht [v0.28.1](https://github.com/libp2p/go-libp2p-kad-dht/releases/tag/v0.28.1) +- passing headers that require authorization but are not authorized now results in an HTTP 401 instead of ignoring those headers ### Removed diff --git a/docs/headers.md b/docs/headers.md index 9a57829..2a28226 100644 --- a/docs/headers.md +++ b/docs/headers.md @@ -8,7 +8,7 @@ See [`RAINBOW_TRACING_AUTH`](./environment-variables.md#rainbow_tracing_auth) Optional. Clients may use this header to return a additional vendor-specific trace identification information across different distributed tracing systems. -Currently ignored unless `Authorization` matches [`RAINBOW_TRACING_AUTH`](./environment-variables.md#rainbow_tracing_auth). +Will error unless `Authorization` matches [`RAINBOW_TRACING_AUTH`](./environment-variables.md#rainbow_tracing_auth). > [!TIP] > `Traceparent` value format can be found in [W3C Trace Context Specification](https://www.w3.org/TR/trace-context-1/#trace-context-http-headers-format). @@ -19,7 +19,7 @@ Currently ignored unless `Authorization` matches [`RAINBOW_TRACING_AUTH`](./envi Optional. Clients may use this header to return a additional vendor-specific trace identification information in addition to `Traceparent`. -Currently ignored unless `Authorization` matches [`RAINBOW_TRACING_AUTH`](./environment-variables.md#rainbow_tracing_auth). +Will error unless `Authorization` matches [`RAINBOW_TRACING_AUTH`](./environment-variables.md#rainbow_tracing_auth). > [!TIP] > `Tracestate` value format can be found in [W3C Trace Context Specification](https://www.w3.org/TR/trace-context-1/#trace-context-http-headers-format). @@ -28,6 +28,6 @@ Currently ignored unless `Authorization` matches [`RAINBOW_TRACING_AUTH`](./envi If the value is `true` the associated request will skip the local block cache and leverage a separate in-memory block cache for the request. -This header is not respected unless the request has a valid `Authorization` header +Will error unless the request has a valid `Authorization` header See [`RAINBOW_TRACING_AUTH`](./environment-variables.md#rainbow_tracing_auth) diff --git a/handler_test.go b/handler_test.go index 48c041c..a3cebc5 100644 --- a/handler_test.go +++ b/handler_test.go @@ -158,15 +158,12 @@ func TestNoBlockcacheHeader(t *testing.T) { req, err := http.NewRequest(http.MethodGet, url, nil) require.NoError(t, err) - // Authorization missing, expect NoBlockcacheHeader to be ignored + // Authorization missing, expect NoBlockcacheHeader to result in an error req.Header.Set(NoBlockcacheHeader, "true") res, err := http.DefaultClient.Do(req) assert.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - responseBody, err := io.ReadAll(res.Body) - assert.NoError(t, err) - assert.Equal(t, content, responseBody) + assert.Equal(t, http.StatusUnauthorized, res.StatusCode) }) t.Run("Skipping the cache only works when RAINBOW_TRACING_AUTH is set", func(t *testing.T) { @@ -185,14 +182,11 @@ func TestNoBlockcacheHeader(t *testing.T) { req, err := http.NewRequest(http.MethodGet, url, nil) require.NoError(t, err) - // Authorization missing, expect NoBlockcacheHeader to be ignored + // Authorization missing, expect NoBlockcacheHeader to result in an error req.Header.Set(NoBlockcacheHeader, "true") res, err := http.DefaultClient.Do(req) assert.NoError(t, err) - assert.Equal(t, http.StatusOK, res.StatusCode) - responseBody, err := io.ReadAll(res.Body) - assert.NoError(t, err) - assert.Equal(t, content, responseBody) + assert.Equal(t, http.StatusUnauthorized, res.StatusCode) }) } diff --git a/handlers.go b/handlers.go index 57a989b..923917b 100644 --- a/handlers.go +++ b/handlers.go @@ -363,14 +363,9 @@ func withTracingAndDebug(next http.Handler, authToken string) http.Handler { return http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { // Disable tracing/debug headers if auth token missing or invalid if authToken == "" || request.Header.Get("Authorization") != authToken { - if request.Header.Get("Traceparent") != "" { - request.Header.Del("Traceparent") - } - if request.Header.Get("Tracestate") != "" { - request.Header.Del("Tracestate") - } - if request.Header.Get(NoBlockcacheHeader) != "" { - request.Header.Del(NoBlockcacheHeader) + if request.Header.Get("Traceparent") != "" || request.Header.Get("Tracestate") != "" || request.Header.Get(NoBlockcacheHeader) != "" { + http.Error(writer, "The request is not accompanied by a valid authorization header", http.StatusUnauthorized) + return } }