From ff7cfd4b1a8b651129e908edfb23ff97b88a4396 Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 1 Jun 2026 11:03:04 +0200 Subject: [PATCH] fix(exception): log the violated constraint name at WARN (#678) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Tobias's review concern: the generic DataIntegrityViolation backstop turned every integrity violation into a silent 400 with no constraint name, no stack, no Sentry — an unanticipated write bug would fail invisibly in production. Now extract the constraint NAME from the cause chain (schema metadata, safe for Loki) and log it parameterized at WARN, so the failure is debuggable. Still never pass `ex`/`getMessage()` (SQL + values, CWE-209) and still no Sentry — the response stays generic, so the response logic is not brittle. New test proves the WARN names the constraint but never carries the SQL. Co-Authored-By: Claude Opus 4.8 --- .../exception/GlobalExceptionHandler.java | 28 +++++++++--- .../exception/GlobalExceptionHandlerTest.java | 43 ++++++++++++++++++- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java b/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java index 7a029955..686ef457 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java @@ -68,19 +68,35 @@ public class GlobalExceptionHandler { /** * Backstop for any database integrity violation that slips past the explicit upstream * guards (e.g. a future constraint, or the import path emitting a bad range). Turns it into - * a clean 400 instead of a 500 + Sentry alert. Deliberately generic: it does NOT inspect the - * exception to identify the constraint — that inspection is the brittle thing we avoid, and the - * known date-range cases are already caught upstream and never reach here. + * a clean 400 instead of a 500 + Sentry alert. The known date-range cases are caught upstream + * and never reach here; this only catches the unanticipated ones — so it logs the constraint + * NAME at WARN to stay debuggable, without re-leaking SQL and without branching the response + * on it (the response stays generic, which is the non-brittle part). */ @ExceptionHandler(DataIntegrityViolationException.class) public ResponseEntity handleDataIntegrityViolation(DataIntegrityViolationException ex) { - // Fixed message only: ex.getMessage() embeds the constraint name + SQL (CWE-209), and - // passing ex to the logger would dump the same into Loki — so do neither, and no Sentry. - log.warn("Rejected a request that violated a database integrity constraint"); + // Log the constraint NAME only — schema metadata, safe for Loki, and enough to tell which + // constraint fired at 2am. Never pass `ex` / `ex.getMessage()`: those embed the SQL + the + // offending values (CWE-209). No Sentry: an integrity violation is a 400, not a system fault. + log.warn("Rejected a request that violated a database integrity constraint: {}", constraintNameOf(ex)); return ResponseEntity.badRequest() .body(new ErrorResponse(ErrorCode.VALIDATION_ERROR, "The submitted data violated a database constraint")); } + /** + * Returns the offending constraint's name from the cause chain, or {@code "unknown"}. + * Reads only the name (a non-sensitive schema identifier) — never the SQL or the values. + */ + private static String constraintNameOf(Throwable ex) { + for (Throwable t = ex; t != null && t != t.getCause(); t = t.getCause()) { + if (t instanceof org.hibernate.exception.ConstraintViolationException cve + && cve.getConstraintName() != null) { + return cve.getConstraintName(); + } + } + return "unknown"; + } + @ExceptionHandler(Exception.class) public ResponseEntity handleGeneric(Exception ex) { Sentry.captureException(ex); diff --git a/backend/src/test/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandlerTest.java b/backend/src/test/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandlerTest.java index 3a51993f..c75e9fae 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandlerTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandlerTest.java @@ -72,9 +72,48 @@ class GlobalExceptionHandlerTest { .as("logs a WARN line") .anySatisfy(e -> assertThat(e.getLevel()).isEqualTo(Level.WARN)); assertThat(appender.list) - .as("never logs the constraint name / SQL (would re-leak to Loki)") + .as("never logs the SQL statement / values (would re-leak to Loki)") .noneSatisfy(e -> { - assertThat(e.getFormattedMessage()).contains("chk_"); + assertThat(e.getFormattedMessage()).contains("could not execute statement"); }); } + + @Test + void handleDataIntegrityViolation_logsConstraintName_butNotTheSql() { + // Debuggability (DevOps): the WARN must name *which* constraint fired so an + // unanticipated violation isn't a silent mystery — but it must carry the name only, + // never the SQL statement or the offending values that the SQLException message holds. + java.sql.SQLException sql = new java.sql.SQLException( + "ERROR: violates check constraint; could not execute statement; values (1917-01-10)"); + org.hibernate.exception.ConstraintViolationException cve = + new org.hibernate.exception.ConstraintViolationException( + "constraint violation", sql, "chk_meta_date_end_after_start"); + DataIntegrityViolationException ex = new DataIntegrityViolationException("wrapper", cve); + + Logger handlerLogger = (Logger) LoggerFactory.getLogger(GlobalExceptionHandler.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + handlerLogger.addAppender(appender); + + try (MockedStatic sentryMock = mockStatic(Sentry.class)) { + ResponseEntity response = + handler.handleDataIntegrityViolation(ex); + + // Response stays generic and leak-free (CWE-209) regardless of what we log. + assertThat(response.getStatusCode().value()).isEqualTo(400); + assertThat(response.getBody().message()) + .doesNotContain("chk_") + .doesNotContain("meta_date"); + sentryMock.verifyNoInteractions(); + } finally { + handlerLogger.detachAppender(appender); + } + + assertThat(appender.list) + .as("WARN names the constraint for debuggability") + .anySatisfy(e -> assertThat(e.getFormattedMessage()).contains("chk_meta_date_end_after_start")); + assertThat(appender.list) + .as("but never the SQL statement or values") + .noneSatisfy(e -> assertThat(e.getFormattedMessage()).contains("could not execute statement")); + } }