diff --git a/core/echo_errors.go b/core/echo_errors.go index 4848ab207..6eb8a25f7 100644 --- a/core/echo_errors.go +++ b/core/echo_errors.go @@ -50,6 +50,15 @@ const unmappedStatusCode = 0 // CreateHTTPErrorHandler returns an Echo HTTPErrorHandler that logs the error with extra fields and returns it as an HTTP response. func CreateHTTPErrorHandler() echo.HTTPErrorHandler { return func(err error, ctx echo.Context) { + // Echo can invoke the error handler twice for the same request: middleware + // like BodyDump calls c.Error(err) on its way out and still returns the err, + // causing echo's server loop to invoke the error handler a second time. + // Skip the second invocation — the response has already been written and + // the error logged by the first invocation. echo.DefaultHTTPErrorHandler + // behaves the same way. + if ctx.Response().Committed { + return + } // HTTPErrors occur e.g. when a parameter bind fails. We map this to a httpStatusCodeError so its status code // and message get directly mapped to a problem. if echoErr, ok := err.(*echo.HTTPError); ok { @@ -76,19 +85,12 @@ func CreateHTTPErrorHandler() echo.HTTPErrorHandler { } else { logMsg.Warn(title) } - if !ctx.Response().Committed { - errorWriter, _ := ctx.Get(ErrorWriterContextKey).(ErrorWriter) - if errorWriter == nil { - errorWriter = &problemErrorWriter{} - } - writeError := errorWriter.Write(ctx, statusCode, title, err) - if writeError != nil { - logger.Error(err) - } - } else { - logger. - WithError(err). - Warn("Unable to send error back to client, response already committed") + errorWriter, _ := ctx.Get(ErrorWriterContextKey).(ErrorWriter) + if errorWriter == nil { + errorWriter = &problemErrorWriter{} + } + if writeError := errorWriter.Write(ctx, statusCode, title, err); writeError != nil { + logger.Error(err) } } } diff --git a/core/echo_errors_test.go b/core/echo_errors_test.go index baedee9b1..b50e12291 100644 --- a/core/echo_errors_test.go +++ b/core/echo_errors_test.go @@ -21,6 +21,9 @@ package core import ( "errors" "github.com/labstack/echo/v4" + "github.com/labstack/echo/v4/middleware" + "github.com/sirupsen/logrus" + logrustest "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "io" "net/http" @@ -89,6 +92,47 @@ func TestHttpErrorHandler(t *testing.T) { }) } +func TestHttpErrorHandler_idempotent_with_BodyDump(t *testing.T) { + // Echo's BodyDump middleware (and any other middleware that calls c.Error + // on the way out and still returns the error) causes echo to invoke the + // HTTPErrorHandler twice for the same request: once via c.Error and once + // via the err returned to echo's server loop. The second invocation must + // be a no-op so it does not log "response already committed" warnings. + logger, hook := logrustest.NewNullLogger() + logger.SetLevel(logrus.DebugLevel) + prevLogger := logrus.StandardLogger() + logrus.SetOutput(io.Discard) + prevHooks := prevLogger.Hooks + prevLogger.Hooks = make(logrus.LevelHooks) + prevLogger.AddHook(hook) + t.Cleanup(func() { + prevLogger.Hooks = prevHooks + logrus.SetOutput(prevLogger.Out) + }) + + es := echo.New() + es.HTTPErrorHandler = CreateHTTPErrorHandler() + es.Use(middleware.BodyDump(func(c echo.Context, _, _ []byte) {})) + es.Add(http.MethodGet, "/", func(c echo.Context) error { + c.Set(OperationIDContextKey, "test") + return errors.New("upstream failed") + }) + server := httptest.NewServer(es) + t.Cleanup(server.Close) + + resp, err := http.Get(server.URL + "/") + assert.NoError(t, err) + assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) + body, _ := io.ReadAll(resp.Body) + assert.Contains(t, string(body), "upstream failed", + "the error must be written to the response exactly once") + + for _, entry := range hook.AllEntries() { + assert.NotContains(t, entry.Message, "response already committed", + "HTTPErrorHandler must be idempotent when invoked on a committed response") + } +} + func Test_NotFoundError(t *testing.T) { err := NotFoundError("failed: %s", "oops").(httpStatusCodeError) assert.EqualError(t, err, "failed: oops") diff --git a/discovery/module_test.go b/discovery/module_test.go index 56a0a031c..bfa83817b 100644 --- a/discovery/module_test.go +++ b/discovery/module_test.go @@ -483,6 +483,27 @@ func TestModule_Search(t *testing.T) { actualJSON, _ := json.Marshal(results) assert.JSONEq(t, string(expectedJSON), string(actualJSON)) }) + t.Run("retracted presentations are not returned", func(t *testing.T) { + resetStore(t, storageEngine.GetSQLDatabase()) + m, testContext := setupModule(t, storageEngine, func(module *Module) { + module.config.Client.RefreshInterval = 0 + }) + testContext.verifier.EXPECT().VerifyVP(gomock.Any(), true, true, nil).Times(2) + + vpAliceRetract := createPresentationCustom(aliceDID, func(claims map[string]interface{}, vp *vc.VerifiablePresentation) { + vp.Type = append(vp.Type, retractionPresentationType) + claims["retract_jti"] = vpAlice.ID.String() + claims[jwt.AudienceKey] = []string{testServiceID} + }) + + require.NoError(t, m.Register(context.Background(), testServiceID, vpAlice)) + require.NoError(t, m.Register(context.Background(), testServiceID, vpAliceRetract)) + + // Empty query: no credential-join filter, so retraction markers leak through. + results, err := m.Search(testServiceID, map[string]string{}) + require.NoError(t, err) + assert.Empty(t, results, "retraction presentations must not be returned from Search") + }) t.Run("unknown service ID", func(t *testing.T) { m, _ := setupModule(t, storageEngine) _, err := m.Search("unknown", nil) diff --git a/discovery/store.go b/discovery/store.go index 5dfba8298..be65f9211 100644 --- a/discovery/store.go +++ b/discovery/store.go @@ -292,6 +292,10 @@ func (s *sqlStore) search(serviceID string, query map[string]string, allowUnvali if err != nil { return nil, fmt.Errorf("failed to parse presentation '%s': %w", match.PresentationID, err) } + // Retraction markers are stored on the timeline (for Get()) but must not surface in search results. + if presentation.IsType(retractionPresentationType) { + continue + } results = append(results, *presentation) } return results, nil diff --git a/docs/pages/release_notes.rst b/docs/pages/release_notes.rst index a9e17348a..fafd42a2a 100644 --- a/docs/pages/release_notes.rst +++ b/docs/pages/release_notes.rst @@ -2,6 +2,17 @@ Release notes ############# +**************** +Peanut (v6.2.6) +**************** + +Release date: 2026-05-18 + +- Exclude retraction presentations from Discovery Service search results (backport of `#4193 `_). +- Make HTTP error handler idempotent on already-committed responses to prevent double-write attempts (backport of `#4243 `_). + +**Full Changelog**: https://github.com/nuts-foundation/nuts-node/compare/v6.2.5...v6.2.6 + **************** Peanut (v6.2.5) ****************