diff --git a/src/contractTest/resources/application.properties b/src/contractTest/resources/application.properties index 064ab7e8e2..f16b8169ab 100644 --- a/src/contractTest/resources/application.properties +++ b/src/contractTest/resources/application.properties @@ -3,7 +3,7 @@ server.port=4452 server.servlet.contextPath= server.compression.enabled=true server.compression.mime-types=application/json,application/xml,text/html,text/xml,text/plain -server.error.include-message=always +server.error.include-message=never server.max-http-header-size=100KB spring.config.import=optional:configtree:/mnt/secrets/ccd/ diff --git a/src/main/java/uk/gov/hmcts/ccd/config/SafeErrorAttributes.java b/src/main/java/uk/gov/hmcts/ccd/config/SafeErrorAttributes.java new file mode 100644 index 0000000000..0387bbdac0 --- /dev/null +++ b/src/main/java/uk/gov/hmcts/ccd/config/SafeErrorAttributes.java @@ -0,0 +1,38 @@ +package uk.gov.hmcts.ccd.config; + +import org.springframework.boot.web.error.ErrorAttributeOptions; +import org.springframework.boot.web.servlet.error.DefaultErrorAttributes; +import org.springframework.http.HttpStatus; +import org.springframework.stereotype.Component; +import org.springframework.web.context.request.WebRequest; +import uk.gov.hmcts.ccd.domain.model.common.HttpError; + +import java.util.Map; + +@Component +public class SafeErrorAttributes extends DefaultErrorAttributes { + + private static final String ACCESS_DENIED_MESSAGE = "Access Denied"; + + @Override + public Map getErrorAttributes(WebRequest webRequest, ErrorAttributeOptions options) { + Map errorAttributes = super.getErrorAttributes(webRequest, options); + errorAttributes.put("message", safeMessage(errorAttributes.get("status"))); + errorAttributes.remove("exception"); + return errorAttributes; + } + + String safeMessage(Object status) { + if (status instanceof Number statusCode) { + HttpStatus httpStatus = HttpStatus.resolve(statusCode.intValue()); + if (HttpStatus.FORBIDDEN.equals(httpStatus)) { + return ACCESS_DENIED_MESSAGE; + } + if (httpStatus != null) { + return httpStatus.getReasonPhrase(); + } + } + + return HttpError.DEFAULT_ERROR; + } +} diff --git a/src/main/java/uk/gov/hmcts/ccd/domain/model/common/HttpError.java b/src/main/java/uk/gov/hmcts/ccd/domain/model/common/HttpError.java index dd9b63f0a3..837f6ac5dd 100644 --- a/src/main/java/uk/gov/hmcts/ccd/domain/model/common/HttpError.java +++ b/src/main/java/uk/gov/hmcts/ccd/domain/model/common/HttpError.java @@ -34,7 +34,7 @@ public HttpError(Exception exception, HttpServletRequest request) { this.timestamp = LocalDateTime.now(ZoneOffset.UTC); this.status = getStatusFromResponseStatus(responseStatus); this.error = getErrorReason(responseStatus); - this.message = exception.getMessage(); + this.message = error; this.path = UriUtils.encodePath(request.getRequestURI(), StandardCharsets.UTF_8); } @@ -45,7 +45,7 @@ public HttpError(Exception exception, String path, HttpStatus status) { this.timestamp = LocalDateTime.now(ZoneOffset.UTC); this.status = getStatusFromResponseStatus(responseStatus, status); this.error = getErrorReason(responseStatus, status); - this.message = exception.getMessage(); + this.message = error; this.path = UriUtils.encodePath(path, StandardCharsets.UTF_8); } diff --git a/src/main/java/uk/gov/hmcts/ccd/endpoint/exceptions/RestExceptionHandler.java b/src/main/java/uk/gov/hmcts/ccd/endpoint/exceptions/RestExceptionHandler.java index d0976a488b..5891987da7 100644 --- a/src/main/java/uk/gov/hmcts/ccd/endpoint/exceptions/RestExceptionHandler.java +++ b/src/main/java/uk/gov/hmcts/ccd/endpoint/exceptions/RestExceptionHandler.java @@ -80,6 +80,7 @@ public ResponseEntity handleCaseValidationException(HttpServletReques exception.getFields(), exception); appInsights.trackException(exception, customProperties, SeverityLevel.Warning); final HttpError error = new HttpError<>(exception, request) + .withMessage(exception.getMessage()) .withDetails(exception.getDetails()); return ResponseEntity .status(error.getStatus()) diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index e6a04cd0d6..b6420f0a84 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -3,7 +3,7 @@ server.port=4452 server.servlet.contextPath= server.compression.enabled=true server.compression.mime-types=application/json,application/xml,text/html,text/xml,text/plain -server.error.include-message=always +server.error.include-message=never server.max-http-request-header-size=100KB spring.config.import=optional:configtree:/mnt/secrets/ccd/ diff --git a/src/test/java/uk/gov/hmcts/ccd/ElasticsearchIT.java b/src/test/java/uk/gov/hmcts/ccd/ElasticsearchIT.java index a82c18b4ed..d6624e0343 100644 --- a/src/test/java/uk/gov/hmcts/ccd/ElasticsearchIT.java +++ b/src/test/java/uk/gov/hmcts/ccd/ElasticsearchIT.java @@ -69,13 +69,11 @@ import static org.elasticsearch.index.query.QueryBuilders.boolQuery; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.index.query.QueryBuilders.matchQuery; -import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.hasItem; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.hasProperty; -import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertAll; import static org.mockito.Mockito.doReturn; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; @@ -644,7 +642,7 @@ void shouldReturnErrorWithUnsupportedUseCase() throws Exception { assertAll( () -> assertThat(exceptionNode.get(ERROR_MESSAGE).asText(), - is("The provided use case 'INVALID' is unsupported for case type 'AAT'.")) + is("Bad Request")) ); } @@ -657,7 +655,7 @@ void shouldReturnErrorWithMissingQueryField() throws Exception { assertAll( () -> assertThat(exceptionNode.get(ERROR_MESSAGE).asText(), - is("missing required field 'query'")) + is("Bad Request")) ); } @@ -672,7 +670,7 @@ void shouldReturnErrorWhenElasticsearchErrors() throws Exception { assertAll( () -> assertThat(exceptionNode.get(ERROR_MESSAGE).asText(), - containsString("No mapping found for [invalid.keyword] in order to sort on")) + is("Bad Request")) ); } @@ -693,7 +691,7 @@ void shouldReturnErrorWhenInvalidJsonIsSent() throws Exception { assertAll( () -> assertThat(exceptionNode.get(ERROR_MESSAGE).asText(), - containsString("Request requires correctly formatted JSON")) + is("Bad Request")) ); } @@ -1589,7 +1587,7 @@ void shouldErrorWhenInvalidCaseTypeIsProvided() throws Exception { assertAll( () -> assertThat(exceptionNode.get("message").asText(), - startsWith("Resource not found when getting case type definition for INVALID")) + is("Not Found")) ); } diff --git a/src/test/java/uk/gov/hmcts/ccd/config/SafeErrorAttributesTest.java b/src/test/java/uk/gov/hmcts/ccd/config/SafeErrorAttributesTest.java new file mode 100644 index 0000000000..5d17af9866 --- /dev/null +++ b/src/test/java/uk/gov/hmcts/ccd/config/SafeErrorAttributesTest.java @@ -0,0 +1,57 @@ +package uk.gov.hmcts.ccd.config; + +import org.junit.jupiter.api.Test; +import org.springframework.boot.web.error.ErrorAttributeOptions; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.http.HttpStatus; +import org.springframework.web.context.request.ServletWebRequest; +import uk.gov.hmcts.ccd.domain.model.common.HttpError; + +import jakarta.servlet.RequestDispatcher; +import java.util.Map; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; + +class SafeErrorAttributesTest { + + private final SafeErrorAttributes safeErrorAttributes = new SafeErrorAttributes(); + + @Test + void shouldUseAccessDeniedForForbiddenResponses() { + assertThat(safeErrorAttributes.safeMessage(HttpStatus.FORBIDDEN.value()), is("Access Denied")); + } + + @Test + void shouldUseReasonPhraseForNonForbiddenResponses() { + assertThat(safeErrorAttributes.safeMessage(HttpStatus.BAD_REQUEST.value()), is("Bad Request")); + } + + @Test + void shouldUseDefaultErrorWhenStatusCannotBeResolved() { + assertThat(safeErrorAttributes.safeMessage(null), is(HttpError.DEFAULT_ERROR)); + } + + @Test + void shouldOverrideDefaultErrorAttributesWithSafeMessageAndRemoveException() { + Map errorAttributes = safeErrorAttributes.getErrorAttributes( + errorRequest(HttpStatus.BAD_REQUEST, "Raw exception message"), + ErrorAttributeOptions.defaults().including( + ErrorAttributeOptions.Include.MESSAGE, + ErrorAttributeOptions.Include.EXCEPTION + ) + ); + + assertThat(errorAttributes.get("message"), is("Bad Request")); + assertThat(errorAttributes.containsKey("exception"), is(false)); + } + + private ServletWebRequest errorRequest(HttpStatus status, String message) { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setRequestURI("/some/path"); + request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE, status.value()); + request.setAttribute(RequestDispatcher.ERROR_MESSAGE, message); + request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, new IllegalArgumentException(message)); + return new ServletWebRequest(request); + } +} diff --git a/src/test/java/uk/gov/hmcts/ccd/domain/model/common/HttpErrorTest.java b/src/test/java/uk/gov/hmcts/ccd/domain/model/common/HttpErrorTest.java index 508300168e..a9b3c2f9ad 100644 --- a/src/test/java/uk/gov/hmcts/ccd/domain/model/common/HttpErrorTest.java +++ b/src/test/java/uk/gov/hmcts/ccd/domain/model/common/HttpErrorTest.java @@ -97,11 +97,11 @@ public void shouldExtractErrorFromExceptionAnnotation_default() { } @Test - public void shouldExtractMessageFromException() { + public void shouldUseSafeDefaultMessageFromException() { final uk.gov.hmcts.ccd.domain.model.common.HttpError error = new uk.gov.hmcts.ccd.domain.model.common.HttpError(new IllegalArgumentException(MESSAGE), request); - assertThat(error.getMessage(), is(equalTo(MESSAGE))); + assertThat(error.getMessage(), is(equalTo(uk.gov.hmcts.ccd.domain.model.common.HttpError.DEFAULT_ERROR))); } @Test diff --git a/src/test/java/uk/gov/hmcts/ccd/endpoint/exceptions/RestExceptionHandlerTest.java b/src/test/java/uk/gov/hmcts/ccd/endpoint/exceptions/RestExceptionHandlerTest.java index 03e9ebd7c4..bae3f3b072 100644 --- a/src/test/java/uk/gov/hmcts/ccd/endpoint/exceptions/RestExceptionHandlerTest.java +++ b/src/test/java/uk/gov/hmcts/ccd/endpoint/exceptions/RestExceptionHandlerTest.java @@ -763,6 +763,7 @@ private void assertHttpErrorResponse(ResultActions result, Exception expectedExc // NB: as we cannot mock HttpError generate an equivalent and compare to response final HttpError expectedError = new HttpError<>(expectedException, mock(HttpServletRequest.class)); + withExplicitClientVisibleMessage(expectedError, expectedException); assertBasicHttpErrorResponseProperties(result, expectedException, expectedError); } @@ -776,10 +777,17 @@ private void assertHttpErrorResponse(ResultActions result, Exception expectedExc // NB: as we cannot mock HttpError generate an equivalent and compare to response final HttpError expectedError = new HttpError<>(expectedException, mock(HttpServletRequest.class), expectedHttpStatus); + withExplicitClientVisibleMessage(expectedError, expectedException); assertBasicHttpErrorResponseProperties(result, expectedException, expectedError); } + private void withExplicitClientVisibleMessage(HttpError expectedError, Exception expectedException) { + if (expectedException instanceof CaseValidationException) { + expectedError.withMessage(expectedException.getMessage()); + } + } + private void assertBasicHttpErrorResponseProperties(ResultActions result, Exception expectedException, HttpError expectedHttpError) throws Exception { @@ -788,7 +796,7 @@ private void assertBasicHttpErrorResponseProperties(ResultActions result, Except result.andExpect(jsonPath("$.exception").value(expectedException.getClass().getName())); // check a bit more - result.andExpect(jsonPath("$.message").value(expectedException.getMessage())); + result.andExpect(jsonPath("$.message").value(expectedHttpError.getMessage())); } private void assertExtraApiExceptionResponseProperties(ResultActions result, ApiException expectedException) diff --git a/src/test/java/uk/gov/hmcts/ccd/endpoint/std/CallbackTest.java b/src/test/java/uk/gov/hmcts/ccd/endpoint/std/CallbackTest.java index 6d19ad1090..52bdf5a36f 100644 --- a/src/test/java/uk/gov/hmcts/ccd/endpoint/std/CallbackTest.java +++ b/src/test/java/uk/gov/hmcts/ccd/endpoint/std/CallbackTest.java @@ -620,8 +620,7 @@ public void shouldReturn422WhenPostCreateCaseWithCallbackOverridingDataWithMissi ).andReturn(); assertEquals("Incorrect Response Status Code", 422, mvcResult.getResponse().getStatus()); - assertEquals("Incorrect Error Message", "\"The event cannot be complete due to a callback returned data " - + "validation error (c)\"", + assertEquals("Incorrect Error Message", "\"Unprocessable Entity\"", mapper.readTree(mvcResult.getResponse().getContentAsString()).get("message").toString()); } diff --git a/src/test/java/uk/gov/hmcts/ccd/endpoint/std/DraftsEndpointIT.java b/src/test/java/uk/gov/hmcts/ccd/endpoint/std/DraftsEndpointIT.java index f66b0b5c80..ae4f813183 100644 --- a/src/test/java/uk/gov/hmcts/ccd/endpoint/std/DraftsEndpointIT.java +++ b/src/test/java/uk/gov/hmcts/ccd/endpoint/std/DraftsEndpointIT.java @@ -162,8 +162,7 @@ public void shouldReturn404WhenUpdateDraftForCaseworkerWithDraftIdNotFound() thr assertEquals("Incorrect Response Status Code", 404, mvcResult.getResponse().getStatus()); String actualResponse = mapper.readTree(mvcResult.getResponse().getContentAsString()).toString(); - assertThat(actualResponse, containsString("\"message\":" + - "\"No draft found ( draft reference = '7578590391163133' )\"")); + assertThat(actualResponse, containsString("\"message\":\"Not Found\"")); } @Test @@ -299,8 +298,7 @@ public void shouldReturn404WhenGetInvalidDraft() throws Exception { assertEquals("Incorrect Response Status Code", 404, mvcResult.getResponse().getStatus()); String actualResponse = mapper.readTree(mvcResult.getResponse().getContentAsString()).toString(); - assertThat(actualResponse, containsString("\"message\":" + - "\"No draft found ( draft reference = '7578590391163133' )\"")); + assertThat(actualResponse, containsString("\"message\":\"Not Found\"")); } @Test