From e8a2b87de29100d716cc2f7af0c2767dfb887d6d Mon Sep 17 00:00:00 2001 From: Steven van der Vegt Date: Mon, 27 Jul 2026 13:59:47 +0000 Subject: [PATCH] fix(auth): never reflect fetched response bodies in API responses (#4421) * fix(auth): never reflect fetched response bodies in API responses Several callback handlers formatted err.Error() (or a raw response body) into the OAuth2 error_description or RFC7807 detail returned to the caller. This can be used in an attack chain to disclose internal response data. This PR fixes that by not including the error in the response body but instead log it in truncated form to the debug logs. Assisted-by: AI --- auth/api/iam/openid4vci.go | 14 ++++++++----- auth/api/iam/openid4vci_test.go | 25 ++++++++++++++++++----- auth/api/iam/openid4vp.go | 4 ++-- auth/api/iam/openid4vp_test.go | 3 ++- auth/client/iam/client.go | 11 +++++----- auth/client/iam/client_test.go | 23 +++++++++++++++++++-- core/http_client.go | 24 ++++++++++++++-------- core/http_client_test.go | 20 +++++++++++++++--- discovery/api/server/client/http.go | 26 +++++++----------------- discovery/api/server/client/http_test.go | 21 +++++++++++-------- docs/pages/release_notes.rst | 7 +++++++ vdr/didweb/web.go | 6 +++++- vdr/didweb/web_test.go | 3 ++- 13 files changed, 126 insertions(+), 61 deletions(-) diff --git a/auth/api/iam/openid4vci.go b/auth/api/iam/openid4vci.go index 952a42f39f..042183c833 100644 --- a/auth/api/iam/openid4vci.go +++ b/auth/api/iam/openid4vci.go @@ -30,6 +30,7 @@ import ( "github.com/lestrrat-go/jwx/v3/jwt" "github.com/nuts-foundation/go-did/did" "github.com/nuts-foundation/go-did/vc" + "github.com/nuts-foundation/nuts-node/auth/log" "github.com/nuts-foundation/nuts-node/auth/oauth" "github.com/nuts-foundation/nuts-node/core" "github.com/nuts-foundation/nuts-node/crypto" @@ -144,27 +145,30 @@ func (r Wrapper) handleOpenID4VCICallback(ctx context.Context, authorizationCode // use code to request access token from remote token endpoint response, err := r.auth.IAMClient().AccessToken(ctx, authorizationCode, oauthSession.TokenEndpoint, checkURL.String(), *oauthSession.OwnSubject, clientID, oauthSession.PKCEParams.Verifier, false) if err != nil { - return nil, withCallbackURI(oauthError(oauth.AccessDenied, fmt.Sprintf("error while fetching the access_token from endpoint: %s, error: %s", oauthSession.TokenEndpoint, err.Error())), appCallbackURI) + return nil, withCallbackURI(oauthError(oauth.AccessDenied, fmt.Sprintf("failed to retrieve access token from %s", oauthSession.TokenEndpoint), err), appCallbackURI) } // make proof and collect credential proofJWT, err := r.openid4vciProof(ctx, *oauthSession.OwnDID, oauthSession.IssuerURL, response.Get(oauth.CNonceParam)) if err != nil { - return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("error building proof to fetch the credential from endpoint %s, error: %s", oauthSession.IssuerCredentialEndpoint, err.Error())), appCallbackURI) + return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("failed to build proof for the credential request to %s", oauthSession.IssuerCredentialEndpoint), err), appCallbackURI) } credentials, err := r.auth.IAMClient().VerifiableCredentials(ctx, oauthSession.IssuerCredentialEndpoint, response.AccessToken, proofJWT) if err != nil { - return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("error while fetching the credential from endpoint %s, error: %s", oauthSession.IssuerCredentialEndpoint, err.Error())), appCallbackURI) + return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("failed to retrieve the credential from %s", oauthSession.IssuerCredentialEndpoint), err), appCallbackURI) } // validate credential // TODO: check that issued credential is bound to DID that requested it (OwnDID)??? credential, err := vc.ParseVerifiableCredential(credentials.Credential) if err != nil { - return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("error while parsing the credential: %s, error: %s", credentials.Credential, err.Error())), appCallbackURI) + // Debug-log the (truncated) credential for diagnostics, but never reflect it: it is + // fetched from the issuer's (attacker-influenceable) credential endpoint. + log.Logger().Debugf("credential returned by %s could not be parsed (credential: %q)", oauthSession.IssuerCredentialEndpoint, core.TruncateHTTPBody([]byte(credentials.Credential))) + return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("failed to parse the credential returned by %s", oauthSession.IssuerCredentialEndpoint), err), appCallbackURI) } err = r.vcr.Verifier().Verify(*credential, true, true, nil) if err != nil { - return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("error while verifying the credential from issuer: %s, error: %s", credential.Issuer.String(), err.Error())), appCallbackURI) + return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("the credential returned by issuer %s failed verification", credential.Issuer.String()), err), appCallbackURI) } // store credential in wallet err = r.vcr.Wallet().Put(ctx, *credential) diff --git a/auth/api/iam/openid4vci_test.go b/auth/api/iam/openid4vci_test.go index 5f4b31ef41..d0356c035e 100644 --- a/auth/api/iam/openid4vci_test.go +++ b/auth/api/iam/openid4vci_test.go @@ -246,7 +246,8 @@ func TestWrapper_handleOpenID4VCICallback(t *testing.T) { assert.Error(t, err) assert.Nil(t, callback) - assert.Equal(t, "access_denied - error while fetching the access_token from endpoint: https://auth.server/token, error: FAIL", err.Error()) + assert.ErrorContains(t, err, "failed to retrieve access token from https://auth.server/token") + assert.ErrorContains(t, err, "FAIL") }) t.Run("fail_credential_response", func(t *testing.T) { ctx := newTestClient(t) @@ -258,7 +259,8 @@ func TestWrapper_handleOpenID4VCICallback(t *testing.T) { callback, err := ctx.client.handleOpenID4VCICallback(nil, code, &session) assert.Nil(t, callback) - assert.EqualError(t, err, "server_error - error while fetching the credential from endpoint https://auth.server/credz, error: FAIL") + assert.ErrorContains(t, err, "failed to retrieve the credential from https://auth.server/credz") + assert.ErrorContains(t, err, "FAIL") }) t.Run("err - invalid credential", func(t *testing.T) { ctx := newTestClient(t) @@ -266,13 +268,25 @@ func TestWrapper_handleOpenID4VCICallback(t *testing.T) { ctx.keyResolver.EXPECT().ResolveKey(holderDID, nil, resolver.NutsSigningKeyType).Return("kid", nil, nil) ctx.jwtSigner.EXPECT().SignJWT(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return("signed-proof", nil) ctx.iamClient.EXPECT().VerifiableCredentials(nil, credEndpoint, accessToken, "signed-proof").Return(&iam.CredentialResponse{ - Credential: "super invalid", + Credential: "SENTINEL-CREDENTIAL-BODY", }, nil) callback, err := ctx.client.handleOpenID4VCICallback(nil, code, &session) assert.Nil(t, callback) - assert.EqualError(t, err, "server_error - error while parsing the credential: super invalid, error: jwt.Parse: failed to parse token: unknown payload type (payload is not JWT?)") + require.Error(t, err) + var oauthErr oauth.OAuth2Error + require.ErrorAs(t, err, &oauthErr) + // It failed specifically because the returned credential could not be parsed: + // the static description identifies that branch, so the test can't pass on an + // unrelated earlier error. + assert.Equal(t, oauth.ServerError, oauthErr.Code) + assert.Contains(t, oauthErr.Description, "failed to parse the credential") + // The parse failure detail is still available for diagnostics ... + require.NotNil(t, oauthErr.InternalError) + assert.ErrorContains(t, err, "failed to parse token") + // ... but the raw credential body is never reflected into the response. + assert.NotContains(t, oauthErr.Description, "SENTINEL-CREDENTIAL-BODY") }) t.Run("fail_verify", func(t *testing.T) { ctx := newTestClient(t) @@ -285,7 +299,8 @@ func TestWrapper_handleOpenID4VCICallback(t *testing.T) { callback, err := ctx.client.handleOpenID4VCICallback(nil, code, &session) assert.Nil(t, callback) - assert.EqualError(t, err, "server_error - error while verifying the credential from issuer: did:web:example.com:iam:issuer, error: FAIL") + assert.ErrorContains(t, err, "the credential returned by issuer did:web:example.com:iam:issuer failed verification") + assert.ErrorContains(t, err, "FAIL") }) t.Run("error - key not found", func(t *testing.T) { ctx := newTestClient(t) diff --git a/auth/api/iam/openid4vp.go b/auth/api/iam/openid4vp.go index 96f123732f..aafa082ec5 100644 --- a/auth/api/iam/openid4vp.go +++ b/auth/api/iam/openid4vp.go @@ -737,12 +737,12 @@ func (r Wrapper) handleCallback(ctx context.Context, authorizationCode string, o // use code to request access token from remote token endpoint tokenResponse, err := r.auth.IAMClient().AccessToken(ctx, authorizationCode, oauthSession.TokenEndpoint, checkURL.String(), *oauthSession.OwnSubject, clientID, oauthSession.PKCEParams.Verifier, oauthSession.UseDPoP) if err != nil { - return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("failed to retrieve access token: %s", err.Error())), appCallbackURI) + return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("failed to retrieve access token from %s", oauthSession.TokenEndpoint), err), appCallbackURI) } // update TokenResponse using session.SessionID tokenResponse = tokenResponse.With("status", oauth.AccessTokenRequestStatusActive) if err = r.accessTokenClientStore().Put(oauthSession.SessionID, tokenResponse); err != nil { - return nil, withCallbackURI(oauthError(oauth.ServerError, fmt.Sprintf("failed to store access token: %s", err.Error())), appCallbackURI) + return nil, withCallbackURI(oauthError(oauth.ServerError, "failed to store access token", err), appCallbackURI) } return Callback302Response{ Headers: Callback302ResponseHeaders{Location: appCallbackURI.String()}, diff --git a/auth/api/iam/openid4vp_test.go b/auth/api/iam/openid4vp_test.go index 66859269dd..61471a92fd 100644 --- a/auth/api/iam/openid4vp_test.go +++ b/auth/api/iam/openid4vp_test.go @@ -757,7 +757,8 @@ func Test_handleCallback(t *testing.T) { _, err := ctx.client.handleCallback(nil, code, &session) - requireOAuthError(t, err, oauth.ServerError, "failed to retrieve access token: assert.AnError general error for testing") + // The remote error must not be reflected into the description; only static context (the endpoint) is kept. + requireOAuthError(t, err, oauth.ServerError, "failed to retrieve access token from https://example.com/token") }) } diff --git a/auth/client/iam/client.go b/auth/client/iam/client.go index 88e4075c7d..5c3b94e1c6 100644 --- a/auth/client/iam/client.go +++ b/auth/client/iam/client.go @@ -211,12 +211,11 @@ func (hb HTTPClient) AccessToken(ctx context.Context, tokenEndpoint string, data return token, fmt.Errorf("unable to read response: %w", err) } if err = json.Unmarshal(responseData, &token); err != nil { - // Cut off the response body to 100 characters max to prevent logging of large responses - responseBodyString := string(responseData) - if len(responseBodyString) > core.HttpResponseBodyLogClipAt { - responseBodyString = responseBodyString[:core.HttpResponseBodyLogClipAt] + "...(clipped)" - } - return token, fmt.Errorf("unable to unmarshal response: %w, %s", err, responseBodyString) + // Debug-log the (truncated) body for diagnostics, but never return it: the token + // endpoint may have been redirected to an attacker-influenced target, and the body + // must not be reflected back to the caller. + log.Logger().Debugf("token endpoint returned an unparseable response (body: %q)", core.TruncateHTTPBody(responseData)) + return token, fmt.Errorf("unable to unmarshal response: %w", err) } return token, nil } diff --git a/auth/client/iam/client_test.go b/auth/client/iam/client_test.go index 18828df60c..b14b3a49dc 100644 --- a/auth/client/iam/client_test.go +++ b/auth/client/iam/client_test.go @@ -31,9 +31,12 @@ import ( nutsCrypto "github.com/nuts-foundation/nuts-node/crypto" test2 "github.com/nuts-foundation/nuts-node/crypto/test" "github.com/nuts-foundation/nuts-node/vdr/resolver" + "github.com/sirupsen/logrus" + logrustest "github.com/sirupsen/logrus/hooks/test" "net/http" "net/http/httptest" "net/url" + "strings" "testing" "time" @@ -188,13 +191,29 @@ func TestHTTPClient_AccessToken(t *testing.T) { assert.Equal(t, "offline", string(httpError.ResponseBody)) }) t.Run("error - invalid response", func(t *testing.T) { - handler := http2.Handler{StatusCode: http.StatusOK, ResponseData: "}"} + hook := logrustest.NewGlobal() + oldLevel := logrus.GetLevel() + logrus.SetLevel(logrus.DebugLevel) + t.Cleanup(func() { logrus.SetLevel(oldLevel) }) + + handler := http2.Handler{StatusCode: http.StatusOK, ResponseData: "SECRET-RESPONSE-BODY"} tlsServer, client := testServerAndClient(t, &handler) _, err := client.AccessToken(ctx, tlsServer.URL, data, dpopHeader) require.Error(t, err) - assert.EqualError(t, err, "unable to unmarshal response: invalid character '}' looking for beginning of value, }") + assert.ErrorContains(t, err, "unable to unmarshal response") + // The raw response body must not be reflected in the returned error, since it can + // carry content fetched from an attacker-influenced endpoint. + assert.NotContains(t, err.Error(), "SECRET-RESPONSE-BODY") + // It is debug-logged (truncated) for diagnostics instead. + var logged bool + for _, entry := range hook.AllEntries() { + if strings.Contains(entry.Message, "SECRET-RESPONSE-BODY") { + logged = true + } + } + assert.True(t, logged, "response body should be debug-logged") }) } diff --git a/core/http_client.go b/core/http_client.go index f5acec4ed2..faccae6e10 100644 --- a/core/http_client.go +++ b/core/http_client.go @@ -33,9 +33,22 @@ import ( // This callback pattern avoids circular imports between core and tracing packages. var TracingHTTPTransport func(http.RoundTripper) http.RoundTripper -// HttpResponseBodyLogClipAt is the maximum length of a response body to log. +// HttpResponseBodyLogTruncateAt is the maximum length of a response body to log. // If the response body is longer than this, it will be truncated. -const HttpResponseBodyLogClipAt = 200 +const HttpResponseBodyLogTruncateAt = 200 + +// TruncateHTTPBody returns the response body truncated to at most HttpResponseBodyLogTruncateAt bytes, +// safe to write to a (debug) log. It exists so an unexpected response can be logged for +// diagnostics without ever placing the full, possibly attacker-influenced, body into an +// error message that could be reflected back to a caller. +func TruncateHTTPBody(body []byte) string { + if len(body) <= HttpResponseBodyLogTruncateAt { + return string(body) + } + // The cut may split a multi-byte UTF-8 character; that is fine for logging, + // callers log the result with %q which escapes any invalid bytes. + return string(body[:HttpResponseBodyLogTruncateAt]) + "...(truncated)" +} // HttpError describes an error returned when invoking a remote server. type HttpError struct { @@ -56,13 +69,8 @@ func TestResponseCodeWithLog(expectedStatusCode int, response *http.Response, lo if response.StatusCode != expectedStatusCode { responseData, _ := io.ReadAll(response.Body) if log != nil { - // Cut off the response body to 100 characters max to prevent logging of large responses - responseBodyString := string(responseData) - if len(responseBodyString) > HttpResponseBodyLogClipAt { - responseBodyString = responseBodyString[:HttpResponseBodyLogClipAt] + "...(clipped)" - } log.WithField("http_request_path", response.Request.URL.Path). - Infof("Unexpected HTTP response (len=%d): %s", len(responseData), responseBodyString) + Infof("Unexpected HTTP response (len=%d): %q", len(responseData), TruncateHTTPBody(responseData)) } return HttpError{ error: fmt.Errorf("server returned HTTP %d (expected: %d)", response.StatusCode, expectedStatusCode), diff --git a/core/http_client_test.go b/core/http_client_test.go index 1bc9e31835..13dc285d4d 100644 --- a/core/http_client_test.go +++ b/core/http_client_test.go @@ -106,6 +106,20 @@ func TestTestResponseCode(t *testing.T) { }) } +func TestTruncateHTTPBody(t *testing.T) { + t.Run("short body is returned as-is", func(t *testing.T) { + assert.Equal(t, "hello", TruncateHTTPBody([]byte("hello"))) + }) + t.Run("body at the limit is returned as-is", func(t *testing.T) { + body := strings.Repeat("a", HttpResponseBodyLogTruncateAt) + assert.Equal(t, body, TruncateHTTPBody([]byte(body))) + }) + t.Run("longer body is truncated", func(t *testing.T) { + body := strings.Repeat("a", HttpResponseBodyLogTruncateAt+1) + assert.Equal(t, strings.Repeat("a", HttpResponseBodyLogTruncateAt)+"...(truncated)", TruncateHTTPBody([]byte(body))) + }) +} + func TestTestResponseCodeWithLog(t *testing.T) { t.Run("ok", func(t *testing.T) { logger := logrus.New() @@ -130,9 +144,9 @@ func TestTestResponseCodeWithLog(t *testing.T) { assert.Len(t, hook.Entries, 1) assert.Equal(t, logrus.InfoLevel, hook.LastEntry().Level) - assert.Equal(t, "Unexpected HTTP response (len=13): hello, world!", hook.LastEntry().Message) + assert.Equal(t, `Unexpected HTTP response (len=13): "hello, world!"`, hook.LastEntry().Message) }) - t.Run("large response body (>200), clipped", func(t *testing.T) { + t.Run("large response body (>200), truncated", func(t *testing.T) { data := strings.Repeat("a", 201) status := stdHttp.StatusUnauthorized logger := logrus.New() @@ -142,7 +156,7 @@ func TestTestResponseCodeWithLog(t *testing.T) { _ = TestResponseCodeWithLog(stdHttp.StatusOK, &stdHttp.Response{StatusCode: status, Body: readCloser(data), Request: request}, logger.WithFields(nil)) - assert.Equal(t, "Unexpected HTTP response (len=201): "+strings.Repeat("a", HttpResponseBodyLogClipAt)+"...(clipped)", hook.LastEntry().Message) + assert.Equal(t, `Unexpected HTTP response (len=201): "`+strings.Repeat("a", HttpResponseBodyLogTruncateAt)+`...(truncated)"`, hook.LastEntry().Message) }) } diff --git a/discovery/api/server/client/http.go b/discovery/api/server/client/http.go index b819be4c92..802dcba13f 100644 --- a/discovery/api/server/client/http.go +++ b/discovery/api/server/client/http.go @@ -64,8 +64,9 @@ func (h DefaultHTTPClient) Register(ctx context.Context, serviceEndpointURL stri } defer httpResponse.Body.Close() if err := core.TestResponseCodeWithLog(201, httpResponse, log.Logger()); err != nil { - httpErr := err.(core.HttpError) // TestResponseCodeWithLog always returns an HttpError - return fmt.Errorf("non-OK response from remote Discovery Service (url=%s): %s", serviceEndpointURL, problemResponseToError(httpErr)) + // The response body is not part of the returned error: it comes from a remote server + // and must not be reflected in our own API responses. TestResponseCodeWithLog logs it (truncated). + return fmt.Errorf("non-OK response from remote Discovery Service (url=%s): %w", serviceEndpointURL, err) } return nil } @@ -82,9 +83,10 @@ func (h DefaultHTTPClient) Get(ctx context.Context, serviceEndpointURL string, t return nil, "", 0, fmt.Errorf("failed to invoke remote Discovery Service (url=%s): %w", serviceEndpointURL, err) } defer httpResponse.Body.Close() - if err := core.TestResponseCode(200, httpResponse); err != nil { - httpErr := err.(core.HttpError) // TestResponseCodeWithLog always returns an HttpError - return nil, "", 0, fmt.Errorf("non-OK response from remote Discovery Service (url=%s): %s", serviceEndpointURL, problemResponseToError(httpErr)) + if err := core.TestResponseCodeWithLog(200, httpResponse, log.Logger()); err != nil { + // The response body is not part of the returned error: it comes from a remote server + // and must not be reflected in our own API responses. TestResponseCodeWithLog logs it (truncated). + return nil, "", 0, fmt.Errorf("non-OK response from remote Discovery Service (url=%s): %w", serviceEndpointURL, err) } responseData, err := io.ReadAll(httpResponse.Body) if err != nil { @@ -96,17 +98,3 @@ func (h DefaultHTTPClient) Get(ctx context.Context, serviceEndpointURL string, t } return result.Entries, result.Seed, result.Timestamp, nil } - -// problemResponseToError converts a Problem Details response to an error. -// It creates an error with the given string concatenated with the title and detail fields of the problem details. -func problemResponseToError(httpErr core.HttpError) string { - var problemDetails struct { - Title string `json:"title"` - Description string `json:"detail"` - Status int `json:"status"` - } - if err := json.Unmarshal(httpErr.ResponseBody, &problemDetails); err != nil { - return fmt.Sprintf("%s: %s", httpErr.Error(), httpErr.ResponseBody) - } - return fmt.Sprintf("server returned HTTP status code %d: %s: %s", problemDetails.Status, problemDetails.Title, problemDetails.Description) -} diff --git a/discovery/api/server/client/http_test.go b/discovery/api/server/client/http_test.go index ac40dd8552..513699bea2 100644 --- a/discovery/api/server/client/http_test.go +++ b/discovery/api/server/client/http_test.go @@ -50,24 +50,27 @@ func TestHTTPInvoker_Register(t *testing.T) { assert.Equal(t, vpData, handler.RequestData) }) t.Run("non-ok with problem details", func(t *testing.T) { - server := httptest.NewServer(&testHTTP.Handler{StatusCode: http.StatusBadRequest, ResponseData: `{"title":"missing credentials", "status":400, "detail":"could not resolve DID"}`}) + server := httptest.NewServer(&testHTTP.Handler{StatusCode: http.StatusBadRequest, ResponseData: `{"title":"SENTINEL-TITLE", "status":400, "detail":"SENTINEL-DETAIL"}`}) client := New(time.Minute) err := client.Register(context.Background(), server.URL, vp) assert.ErrorContains(t, err, "non-OK response from remote Discovery Service") - assert.ErrorContains(t, err, "server returned HTTP status code 400") - assert.ErrorContains(t, err, "missing credentials: could not resolve DID") + assert.ErrorContains(t, err, "server returned HTTP 400") + // The remote response body (including parsed problem details) must not be reflected in the error, + // since the error ends up in our own API responses. It's only logged. + assert.NotContains(t, err.Error(), "SENTINEL-TITLE") + assert.NotContains(t, err.Error(), "SENTINEL-DETAIL") }) t.Run("non-ok other", func(t *testing.T) { - server := httptest.NewServer(&testHTTP.Handler{StatusCode: http.StatusNotFound, ResponseData: `not found`}) + server := httptest.NewServer(&testHTTP.Handler{StatusCode: http.StatusNotFound, ResponseData: `SENTINEL-RAW-BODY`}) client := New(time.Minute) err := client.Register(context.Background(), server.URL, vp) assert.ErrorContains(t, err, "non-OK response from remote Discovery Service") assert.ErrorContains(t, err, "server returned HTTP 404") - assert.ErrorContains(t, err, "not found") + assert.NotContains(t, err.Error(), "SENTINEL-RAW-BODY") }) } @@ -128,15 +131,17 @@ func TestHTTPInvoker_Get(t *testing.T) { assert.True(t, strings.HasPrefix(capturedRequest.Header.Get("X-Forwarded-Host"), "127.0.0.1")) }) t.Run("server returns invalid status code", func(t *testing.T) { - handler := &testHTTP.Handler{StatusCode: http.StatusInternalServerError, ResponseData: `{"title":"internal server error", "status":500, "detail":"db not found"}`} + handler := &testHTTP.Handler{StatusCode: http.StatusInternalServerError, ResponseData: `{"title":"SENTINEL-TITLE", "status":500, "detail":"SENTINEL-DETAIL"}`} server := httptest.NewServer(handler) client := New(time.Minute) _, _, _, err := client.Get(context.Background(), server.URL, 0) assert.ErrorContains(t, err, "non-OK response from remote Discovery Service") - assert.ErrorContains(t, err, "server returned HTTP status code 500") - assert.ErrorContains(t, err, "internal server error: db not found") + assert.ErrorContains(t, err, "server returned HTTP 500") + // The remote response body must not be reflected in the error (it ends up in our own API responses). + assert.NotContains(t, err.Error(), "SENTINEL-TITLE") + assert.NotContains(t, err.Error(), "SENTINEL-DETAIL") }) t.Run("server does not return JSON", func(t *testing.T) { handler := &testHTTP.Handler{StatusCode: http.StatusOK} diff --git a/docs/pages/release_notes.rst b/docs/pages/release_notes.rst index 5223c0b9d9..736e007435 100644 --- a/docs/pages/release_notes.rst +++ b/docs/pages/release_notes.rst @@ -2,6 +2,13 @@ Release notes ############# +**************** +Unreleased +**************** + +## Security +* #4421: Stop reflecting fetched HTTP response bodies in API responses. The OAuth2 and OpenID4VCI callback handlers no longer place a remote endpoint's response body or error text into the returned ``error_description``, the did:web resolver no longer returns the fetched document body in its parse error, and the Discovery Service client no longer includes the remote server's error response in errors returned through the discovery APIs. Such content is now logged (truncated) for diagnostics instead. Static context such as the endpoint that failed is retained. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4421 + ***************** Peanut (v6.2.10) ***************** diff --git a/vdr/didweb/web.go b/vdr/didweb/web.go index 25f78ca10a..1749b03e53 100644 --- a/vdr/didweb/web.go +++ b/vdr/didweb/web.go @@ -24,6 +24,7 @@ import ( "github.com/nuts-foundation/go-did/did" "github.com/nuts-foundation/nuts-node/core" "github.com/nuts-foundation/nuts-node/http/client" + "github.com/nuts-foundation/nuts-node/vdr/log" "github.com/nuts-foundation/nuts-node/vdr/resolver" "io" "mime" @@ -106,7 +107,10 @@ func (w Resolver) Resolve(id did.DID, _ *resolver.ResolveMetadata) (*did.Documen var document did.Document err = document.UnmarshalJSON(data) if err != nil { - return nil, nil, fmt.Errorf("did:web JSON unmarshal error: %w", err) + // Debug-log the (truncated) body for diagnostics, but do not return it: the document + // is fetched from an externally-controlled URL and must not be reflected to the caller. + log.Logger().Debugf("did:web document at %s could not be parsed (error: %s, body: %q)", targetURL, err, core.TruncateHTTPBody(data)) + return nil, nil, errors.New("did:web document could not be parsed as JSON") } if !document.ID.Equals(id) { diff --git a/vdr/didweb/web_test.go b/vdr/didweb/web_test.go index b887de4096..e3ce623b6f 100644 --- a/vdr/didweb/web_test.go +++ b/vdr/didweb/web_test.go @@ -168,7 +168,8 @@ func TestResolver_Resolve(t *testing.T) { id := did.MustParseDID(baseDID.String() + ":invalid-json") doc, md, err := resolver.Resolve(id, nil) - assert.EqualError(t, err, "did:web JSON unmarshal error: invalid character '\\x01' looking for beginning of value") + // The fetched (unparseable) body must not be reflected into the returned error. + assert.EqualError(t, err, "did:web document could not be parsed as JSON") assert.Nil(t, md) assert.Nil(t, doc) })