feat(exception): add optimistic-lock backstop returning generic 409
Centralized @ExceptionHandler(ObjectOptimisticLockingFailureException) net so any write path losing a @Version race becomes a generic 409 (CONFLICT code) — never a 500 + Sentry + Hibernate internals (CWE-209). No Sentry, class-name- only parameterized logging, body free of id/version/class. Entity-agnostic by design (no switch on getPersistentClassName); the service catch keeps the precise TIMELINE_EVENT_CONFLICT. Per #775 Q2/R4/R8. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -104,6 +104,30 @@ public class GlobalExceptionHandler {
|
|||||||
return "unknown";
|
return "unknown";
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Generic backstop for optimistic-locking conflicts that escape a service-level catch. A
|
||||||
|
* conflict is a 409, not a system fault — so, like {@link #handleDataIntegrityViolation}, it
|
||||||
|
* must NOT fire Sentry and must NOT leak Hibernate internals (CWE-209): the response carries
|
||||||
|
* only the generic {@link ErrorCode#CONFLICT} code and a generic message — no entity id, no
|
||||||
|
* version, no persistent-class name.
|
||||||
|
*
|
||||||
|
* <p>Deliberately code-GENERIC: do NOT {@code switch} on {@code getPersistentClassName()} to map
|
||||||
|
* back to a per-entity code. Unlike {@link #handleDataIntegrityViolation}, which branches on
|
||||||
|
* stable schema constraint NAMES, persistent-class names are not a contract. The precise,
|
||||||
|
* code-carrying path is the service catch (e.g. {@code TIMELINE_EVENT_CONFLICT}); this is only
|
||||||
|
* the net that keeps any current or future write path from regressing to a 500.
|
||||||
|
*/
|
||||||
|
@ExceptionHandler(org.springframework.orm.ObjectOptimisticLockingFailureException.class)
|
||||||
|
public ResponseEntity<ErrorResponse> handleOptimisticLock(
|
||||||
|
org.springframework.orm.ObjectOptimisticLockingFailureException ex) {
|
||||||
|
// Log the persistent-class name ONLY (schema metadata, safe for Loki). Never `ex` /
|
||||||
|
// ex.getMessage(): those embed the entity id + version (CWE-209). No Sentry: it's a 409.
|
||||||
|
log.warn("Rejected a write that lost an optimistic-lock race on: {}", ex.getPersistentClassName());
|
||||||
|
return ResponseEntity.status(409)
|
||||||
|
.body(new ErrorResponse(ErrorCode.CONFLICT,
|
||||||
|
"The resource was modified concurrently. Please reload and try again."));
|
||||||
|
}
|
||||||
|
|
||||||
@ExceptionHandler(Exception.class)
|
@ExceptionHandler(Exception.class)
|
||||||
public ResponseEntity<ErrorResponse> handleGeneric(Exception ex) {
|
public ResponseEntity<ErrorResponse> handleGeneric(Exception ex) {
|
||||||
Sentry.captureException(ex);
|
Sentry.captureException(ex);
|
||||||
|
|||||||
@@ -14,6 +14,9 @@ import org.slf4j.LoggerFactory;
|
|||||||
import org.springframework.dao.DataIntegrityViolationException;
|
import org.springframework.dao.DataIntegrityViolationException;
|
||||||
import org.springframework.dao.IncorrectResultSizeDataAccessException;
|
import org.springframework.dao.IncorrectResultSizeDataAccessException;
|
||||||
import org.springframework.http.ResponseEntity;
|
import org.springframework.http.ResponseEntity;
|
||||||
|
import org.springframework.orm.ObjectOptimisticLockingFailureException;
|
||||||
|
|
||||||
|
import java.util.UUID;
|
||||||
|
|
||||||
import static org.assertj.core.api.Assertions.assertThat;
|
import static org.assertj.core.api.Assertions.assertThat;
|
||||||
import static org.mockito.Mockito.mockStatic;
|
import static org.mockito.Mockito.mockStatic;
|
||||||
@@ -103,6 +106,49 @@ class GlobalExceptionHandlerTest {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void handleOptimisticLock_returns409_genericConflict_noSentry_noLeak() {
|
||||||
|
// CWE-209 regression: an optimistic-lock failure escaping a service catch must become a
|
||||||
|
// generic 409, never a 500 + Sentry + Hibernate internals. The generic CONFLICT code keeps
|
||||||
|
// it entity-agnostic — NOT TIMELINE_EVENT_CONFLICT, or every future entity's conflict is
|
||||||
|
// mislabeled a timeline one.
|
||||||
|
UUID entityId = UUID.randomUUID();
|
||||||
|
ObjectOptimisticLockingFailureException ex =
|
||||||
|
new ObjectOptimisticLockingFailureException("com.example.SomeEntity", entityId);
|
||||||
|
|
||||||
|
Logger handlerLogger = (Logger) LoggerFactory.getLogger(GlobalExceptionHandler.class);
|
||||||
|
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||||
|
appender.start();
|
||||||
|
handlerLogger.addAppender(appender);
|
||||||
|
|
||||||
|
try (MockedStatic<Sentry> sentryMock = mockStatic(Sentry.class)) {
|
||||||
|
ResponseEntity<GlobalExceptionHandler.ErrorResponse> response = handler.handleOptimisticLock(ex);
|
||||||
|
|
||||||
|
assertThat(response.getStatusCode().value()).isEqualTo(409);
|
||||||
|
assertThat(response.getBody()).isNotNull();
|
||||||
|
assertThat(response.getBody().code()).isEqualTo(ErrorCode.CONFLICT);
|
||||||
|
// Body echoes no persistent-class name, no entity id, no version (enumeration aids).
|
||||||
|
assertThat(response.getBody().message())
|
||||||
|
.doesNotContain("SomeEntity")
|
||||||
|
.doesNotContain(entityId.toString());
|
||||||
|
|
||||||
|
// A conflict is not a system fault — no fabricated Sentry alert.
|
||||||
|
sentryMock.verifyNoInteractions();
|
||||||
|
} finally {
|
||||||
|
handlerLogger.detachAppender(appender);
|
||||||
|
}
|
||||||
|
|
||||||
|
assertThat(appender.list)
|
||||||
|
.as("WARN names the persistent class for debuggability")
|
||||||
|
.anySatisfy(e -> {
|
||||||
|
assertThat(e.getLevel()).isEqualTo(Level.WARN);
|
||||||
|
assertThat(e.getFormattedMessage()).contains("com.example.SomeEntity");
|
||||||
|
});
|
||||||
|
assertThat(appender.list)
|
||||||
|
.as("but never the entity id (would leak via getMessage())")
|
||||||
|
.noneSatisfy(e -> assertThat(e.getFormattedMessage()).contains(entityId.toString()));
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void handleDataIntegrityViolation_logsConstraintName_butNotTheSql() {
|
void handleDataIntegrityViolation_logsConstraintName_butNotTheSql() {
|
||||||
// Debuggability (DevOps): the WARN must name *which* constraint fired so an
|
// Debuggability (DevOps): the WARN must name *which* constraint fired so an
|
||||||
|
|||||||
Reference in New Issue
Block a user