diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/BaseValidatingInterceptor.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/BaseValidatingInterceptor.java index f7a641b9c0a..f70ba6837e3 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/BaseValidatingInterceptor.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/BaseValidatingInterceptor.java @@ -244,6 +244,10 @@ abstract class BaseValidatingInterceptor extends InterceptorAdapter { } } + if (theRequest == null) { + return; + } + ValidationResult validationResult = doValidate(validator, theRequest); if (myAddResponseIssueHeaderOnSeverity != null) { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java index 033abb89547..29104c664d2 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java @@ -120,7 +120,6 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { TestUtil.clearAllStaticFieldsForUnitTest(); } - @Override public void before() throws Exception { super.before(); @@ -295,7 +294,7 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { cs.setUrl("http://urn/system"); cs.addConcept().setCode("code0"); ourClient.create().resource(cs).execute(); - + ValueSet options = new ValueSet(); options.getCompose().addInclude().setSystem("http://urn/system"); IIdType optId = ourClient.create().resource(options).execute().getId(); @@ -438,8 +437,7 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { ourLog.info(responseString); assertEquals(400, response.getStatusLine().getStatusCode()); OperationOutcome oo = myFhirCtx.newXmlParser().parseResource(OperationOutcome.class, responseString); - assertEquals("Can not create resource with ID \"2\", ID must not be supplied on a create (POST) operation (use an HTTP PUT / update operation if you wish to supply an ID)", - oo.getIssue().get(0).getDiagnostics()); + assertEquals("Can not create resource with ID \"2\", ID must not be supplied on a create (POST) operation (use an HTTP PUT / update operation if you wish to supply an ID)", oo.getIssue().get(0).getDiagnostics()); } finally { response.getEntity().getContent().close(); @@ -522,8 +520,7 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { //@formatter:on fail(); } catch (PreconditionFailedException e) { - assertEquals("HTTP 412 Precondition Failed: Failed to DELETE resource with match URL \"Patient?identifier=testDeleteConditionalMultiple\" because this search matched 2 resources", - e.getMessage()); + assertEquals("HTTP 412 Precondition Failed: Failed to DELETE resource with match URL \"Patient?identifier=testDeleteConditionalMultiple\" because this search matched 2 resources", e.getMessage()); } // Not deleted yet.. @@ -568,6 +565,27 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { } } + /** + * Test for #345 + */ + @Test + public void testDeleteNormal() throws IOException { + Patient p = new Patient(); + p.addName().addFamily("FAM"); + IIdType id = ourClient.create().resource(p).execute().getId().toUnqualifiedVersionless(); + + ourClient.read().resource(Patient.class).withId(id).execute(); + + ourClient.delete().resourceById(id).execute(); + + try { + ourClient.read().resource(Patient.class).withId(id).execute(); + fail(); + } catch (ResourceGoneException e) { + // good + } + } + @Test public void testDeleteResourceConditional1() throws IOException { String methodName = "testDeleteResourceConditional1"; @@ -634,7 +652,8 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { } /* - * Try it with a raw socket call. The Apache client won't let us use the unescaped "|" in the URL but we want to make sure that works too.. + * Try it with a raw socket call. The Apache client won't let us use the unescaped "|" in the URL but we want to + * make sure that works too.. */ Socket sock = new Socket(); sock.setSoTimeout(3000); @@ -1168,8 +1187,7 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { response.close(); } - get = new HttpGet(ourServerBase + "/Patient/" + pId.getIdPart() + "/$everything?_lastUpdated=%3E" + new InstantType(new Date(time2)).getValueAsString() + "&_lastUpdated=%3C" - + new InstantType(new Date(time3)).getValueAsString()); + get = new HttpGet(ourServerBase + "/Patient/" + pId.getIdPart() + "/$everything?_lastUpdated=%3E" + new InstantType(new Date(time2)).getValueAsString() + "&_lastUpdated=%3C" + new InstantType(new Date(time3)).getValueAsString()); response = ourHttpClient.execute(get); try { assertEquals(200, response.getStatusLine().getStatusCode()); @@ -1761,8 +1779,7 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { Patient patient = new Patient(); patient.addIdentifier().setSystem("urn:system").setValue("testSearchTokenParam001"); patient.addName().addFamily("Tester").addGiven("testSearchTokenParam1"); - patient.addCommunication().getLanguage().setText("testSearchTokenParamComText").addCoding().setCode("testSearchTokenParamCode").setSystem("testSearchTokenParamSystem") - .setDisplay("testSearchTokenParamDisplay"); + patient.addCommunication().getLanguage().setText("testSearchTokenParamComText").addCoding().setCode("testSearchTokenParamCode").setSystem("testSearchTokenParamSystem").setDisplay("testSearchTokenParamDisplay"); myPatientDao.create(patient, mySrd); patient = new Patient(); @@ -2474,8 +2491,7 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { ourLog.info(responseString); assertEquals(400, response.getStatusLine().getStatusCode()); OperationOutcome oo = myFhirCtx.newXmlParser().parseResource(OperationOutcome.class, responseString); - assertThat(oo.getIssue().get(0).getDiagnostics(), containsString( - "Can not update resource, resource body must contain an ID element which matches the request URL for update (PUT) operation - Resource body ID of \"333\" does not match URL ID of \"2\"")); + assertThat(oo.getIssue().get(0).getDiagnostics(), containsString("Can not update resource, resource body must contain an ID element which matches the request URL for update (PUT) operation - Resource body ID of \"333\" does not match URL ID of \"2\"")); } finally { response.close(); } @@ -2499,10 +2515,8 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { ourLog.info(resp); assertEquals(412, response.getStatusLine().getStatusCode()); assertThat(resp, not(containsString("Resource has no id"))); - assertThat(resp, - stringContainsInOrder(">ERROR<", "/f:Patient/f:contact", "
SHALL at least contain a contact's details or a reference to an organization
", "", - "", "", - "")); + assertThat(resp, stringContainsInOrder(">ERROR<", "/f:Patient/f:contact", "
SHALL at least contain a contact's details or a reference to an organization
", "", "", + "", "")); } finally { IOUtils.closeQuietly(response.getEntity().getContent()); response.close(); @@ -2585,8 +2599,7 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { assertEquals(200, response.getStatusLine().getStatusCode()); assertThat(resp, not(containsString("Resource has no id"))); assertThat(resp, containsString("
No issues detected during validation
")); - assertThat(resp, - stringContainsInOrder("", "", "", "", "")); + assertThat(resp, stringContainsInOrder("", "", "", "", "")); } finally { IOUtils.closeQuietly(response.getEntity().getContent()); response.close(); @@ -2611,8 +2624,7 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { assertEquals(200, response.getStatusLine().getStatusCode()); assertThat(resp, not(containsString("Resource has no id"))); assertThat(resp, containsString("
No issues detected during validation
")); - assertThat(resp, - stringContainsInOrder("", "", "", "", "")); + assertThat(resp, stringContainsInOrder("", "", "", "", "")); } finally { IOUtils.closeQuietly(response.getEntity().getContent()); response.close(); diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/RequestValidatingInterceptorDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/RequestValidatingInterceptorDstu3Test.java index 9b389631cff..1c8ae155567 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/RequestValidatingInterceptorDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/RequestValidatingInterceptorDstu3Test.java @@ -11,6 +11,8 @@ import java.util.concurrent.TimeUnit; import org.apache.commons.io.IOUtils; import org.apache.http.HttpResponse; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpDelete; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpPost; import org.apache.http.entity.ContentType; @@ -22,9 +24,9 @@ import org.eclipse.jetty.server.Server; import org.eclipse.jetty.servlet.ServletHandler; import org.eclipse.jetty.servlet.ServletHolder; import org.hl7.fhir.dstu3.hapi.validation.FhirInstanceValidator; +import org.hl7.fhir.dstu3.model.Enumerations.AdministrativeGender; import org.hl7.fhir.dstu3.model.IdType; import org.hl7.fhir.dstu3.model.Patient; -import org.hl7.fhir.dstu3.model.Enumerations.AdministrativeGender; import org.hl7.fhir.instance.model.api.IBaseResource; import org.junit.AfterClass; import org.junit.Before; @@ -35,6 +37,7 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.model.api.IResource; import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.annotation.Create; +import ca.uhn.fhir.rest.annotation.Delete; import ca.uhn.fhir.rest.annotation.IdParam; import ca.uhn.fhir.rest.annotation.OptionalParam; import ca.uhn.fhir.rest.annotation.ResourceParam; @@ -49,7 +52,7 @@ import ca.uhn.fhir.validation.ResultSeverityEnum; public class RequestValidatingInterceptorDstu3Test { private static CloseableHttpClient ourClient; - + private static FhirContext ourCtx = FhirContext.forDstu3(); private static boolean ourLastRequestWasSearch; private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(RequestValidatingInterceptorDstu3Test.class); @@ -58,23 +61,22 @@ public class RequestValidatingInterceptorDstu3Test { private static Server ourServer; private static RestfulServer ourServlet; - + private RequestValidatingInterceptor myInterceptor; - @Before public void before() { ourLastRequestWasSearch = false; while (ourServlet.getInterceptors().size() > 0) { ourServlet.unregisterInterceptor(ourServlet.getInterceptors().get(0)); } - + myInterceptor = new RequestValidatingInterceptor(); -// myInterceptor.setFailOnSeverity(ResultSeverityEnum.ERROR); -// myInterceptor.setAddResponseHeaderOnSeverity(ResultSeverityEnum.INFORMATION); -// myInterceptor.setResponseHeaderName("X-RESP"); -// myInterceptor.setResponseHeaderValue(RequestValidatingInterceptor.DEFAULT_RESPONSE_HEADER_VALUE); - + // myInterceptor.setFailOnSeverity(ResultSeverityEnum.ERROR); + // myInterceptor.setAddResponseHeaderOnSeverity(ResultSeverityEnum.INFORMATION); + // myInterceptor.setResponseHeaderName("X-RESP"); + // myInterceptor.setResponseHeaderValue(RequestValidatingInterceptor.DEFAULT_RESPONSE_HEADER_VALUE); + ourServlet.registerInterceptor(myInterceptor); } @@ -88,7 +90,7 @@ public class RequestValidatingInterceptorDstu3Test { patient.setGender(AdministrativeGender.MALE); patient.addContact().addRelationship().setText("FOO"); String encoded = ourCtx.newJsonParser().encodeResourceToString(patient); - + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/Patient"); httpPost.setEntity(new StringEntity(encoded, ContentType.create(Constants.CT_FHIR_JSON, "UTF-8"))); @@ -108,13 +110,13 @@ public class RequestValidatingInterceptorDstu3Test { @Test public void testCreateJsonInvalidNoValidatorsSpecified() throws Exception { myInterceptor.setAddResponseHeaderOnSeverity(ResultSeverityEnum.INFORMATION); - + Patient patient = new Patient(); patient.addIdentifier().setValue("002"); patient.setGender(AdministrativeGender.MALE); patient.addContact().addRelationship().setText("FOO"); String encoded = ourCtx.newJsonParser().encodeResourceToString(patient); - + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/Patient"); httpPost.setEntity(new StringEntity(encoded, ContentType.create(Constants.CT_FHIR_JSON, "UTF-8"))); @@ -137,7 +139,7 @@ public class RequestValidatingInterceptorDstu3Test { patient.addIdentifier().setValue("002"); patient.setGender(AdministrativeGender.MALE); String encoded = ourCtx.newJsonParser().encodeResourceToString(patient); - + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/Patient"); httpPost.setEntity(new StringEntity(encoded, ContentType.create(Constants.CT_FHIR_JSON, "UTF-8"))); @@ -162,7 +164,7 @@ public class RequestValidatingInterceptorDstu3Test { patient.addIdentifier().setValue("002"); patient.setGender(AdministrativeGender.MALE); String encoded = ourCtx.newJsonParser().encodeResourceToString(patient); - + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/Patient"); httpPost.setEntity(new StringEntity(encoded, ContentType.create(Constants.CT_FHIR_JSON, "UTF-8"))); @@ -177,7 +179,7 @@ public class RequestValidatingInterceptorDstu3Test { assertEquals(201, status.getStatusLine().getStatusCode()); assertThat(status.toString(), (containsString("X-FHIR-Request-Validation: NO ISSUES"))); } - + @Test public void testCreateXmlInvalidInstanceValidator() throws Exception { IValidatorModule module = new FhirInstanceValidator(); @@ -190,7 +192,7 @@ public class RequestValidatingInterceptorDstu3Test { patient.setGender(AdministrativeGender.MALE); patient.addContact().addRelationship().setText("FOO"); String encoded = ourCtx.newXmlParser().encodeResourceToString(patient); - + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/Patient"); httpPost.setEntity(new StringEntity(encoded, ContentType.create(Constants.CT_FHIR_XML, "UTF-8"))); @@ -215,7 +217,7 @@ public class RequestValidatingInterceptorDstu3Test { patient.setGender(AdministrativeGender.MALE); patient.addContact().addRelationship().setText("FOO"); String encoded = ourCtx.newXmlParser().encodeResourceToString(patient); - + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/Patient"); httpPost.setEntity(new StringEntity(encoded, ContentType.create(Constants.CT_FHIR_XML, "UTF-8"))); @@ -230,7 +232,7 @@ public class RequestValidatingInterceptorDstu3Test { assertEquals(422, status.getStatusLine().getStatusCode()); assertThat(status.toString(), containsString("X-FHIR-Request-Validation")); } - + @Test public void testCreateXmlInvalidNoValidatorsSpecifiedOutcomeHeader() throws Exception { myInterceptor.setAddResponseHeaderOnSeverity(null); @@ -242,7 +244,7 @@ public class RequestValidatingInterceptorDstu3Test { patient.setGender(AdministrativeGender.MALE); patient.addContact().addRelationship().setText("FOO"); String encoded = ourCtx.newXmlParser().encodeResourceToString(patient); - + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/Patient"); httpPost.setEntity(new StringEntity(encoded, ContentType.create(Constants.CT_FHIR_XML, "UTF-8"))); @@ -258,14 +260,13 @@ public class RequestValidatingInterceptorDstu3Test { assertThat(status.toString(), containsString("X-FHIR-Request-Validation: {\"resourceType\":\"OperationOutcome")); } - @Test public void testCreateXmlValidNoValidatorsSpecified() throws Exception { Patient patient = new Patient(); patient.addIdentifier().setValue("002"); patient.setGender(AdministrativeGender.MALE); String encoded = ourCtx.newXmlParser().encodeResourceToString(patient); - + HttpPost httpPost = new HttpPost("http://localhost:" + ourPort + "/Patient"); httpPost.setEntity(new StringEntity(encoded, ContentType.create(Constants.CT_FHIR_XML, "UTF-8"))); @@ -281,12 +282,33 @@ public class RequestValidatingInterceptorDstu3Test { assertThat(status.toString(), not(containsString("X-FHIR-Request-Validation"))); } + /** + * Test for #345 + */ + @Test + public void testDelete() throws Exception { + myInterceptor.setFailOnSeverity(null); + myInterceptor.setAddResponseHeaderOnSeverity(ResultSeverityEnum.INFORMATION); + + HttpDelete httpDelete = new HttpDelete("http://localhost:" + ourPort + "/Patient/123"); + + CloseableHttpResponse status = ourClient.execute(httpDelete); + try { + ourLog.info("Response was:\n{}", status); + + assertEquals(204, status.getStatusLine().getStatusCode()); + assertThat(status.toString(), not(containsString("X-FHIR-Request-Validation"))); + } finally { + IOUtils.closeQuietly(status); + } + } + @Test public void testFetchMetadata() throws Exception { myInterceptor.setAddResponseHeaderOnSeverity(ResultSeverityEnum.INFORMATION); - + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/metadata"); - + // This header caused a crash httpGet.addHeader("Content-Type", "application/xml+fhir"); @@ -301,7 +323,7 @@ public class RequestValidatingInterceptorDstu3Test { assertEquals(200, status.getStatusLine().getStatusCode()); assertThat(responseContent, containsString("Conformance")); } - + @Test public void testSearch() throws Exception { HttpGet httpPost = new HttpGet("http://localhost:" + ourPort + "/Patient?foo=bar"); @@ -318,13 +340,13 @@ public class RequestValidatingInterceptorDstu3Test { assertThat(status.toString(), not(containsString("X-FHIR-Request-Validation"))); assertEquals(true, ourLastRequestWasSearch); } - + @AfterClass public static void afterClassClearContext() throws Exception { ourServer.stop(); TestUtil.clearAllStaticFieldsForUnitTest(); } - + @BeforeClass public static void beforeClass() throws Exception { ourPort = PortUtil.findFreePort(); @@ -346,21 +368,26 @@ public class RequestValidatingInterceptorDstu3Test { ourClient = builder.build(); } - public static class PatientProvider implements IResourceProvider { + public static class PatientProvider implements IResourceProvider { @Create() public MethodOutcome createPatient(@ResourceParam Patient thePatient, @IdParam IdType theIdParam) { return new MethodOutcome(new IdDt("Patient/001/_history/002")); } + @Delete + public MethodOutcome delete(@IdParam IdType theId) { + return new MethodOutcome(theId.withVersion("2")); + } + @Override public Class getResourceType() { return Patient.class; } - + @Search - public List search(@OptionalParam(name="foo") StringParam theString) { + public List search(@OptionalParam(name = "foo") StringParam theString) { ourLastRequestWasSearch = true; return new ArrayList(); } diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/ResponseValidatingInterceptorDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/ResponseValidatingInterceptorDstu3Test.java index c8379530005..26b367c9c07 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/ResponseValidatingInterceptorDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/ResponseValidatingInterceptorDstu3Test.java @@ -12,6 +12,8 @@ import java.util.concurrent.TimeUnit; import org.apache.commons.io.IOUtils; import org.apache.http.HttpResponse; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpDelete; import org.apache.http.client.methods.HttpGet; import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; @@ -20,6 +22,7 @@ import org.eclipse.jetty.server.Server; import org.eclipse.jetty.servlet.ServletHandler; import org.eclipse.jetty.servlet.ServletHolder; import org.hl7.fhir.dstu3.hapi.validation.FhirInstanceValidator; +import org.hl7.fhir.dstu3.model.IdType; import org.hl7.fhir.dstu3.model.Patient; import org.hl7.fhir.dstu3.model.Enumerations.AdministrativeGender; import org.hl7.fhir.dstu3.model.Identifier.IdentifierUse; @@ -31,8 +34,11 @@ import org.junit.Ignore; import org.junit.Test; import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.rest.annotation.Delete; +import ca.uhn.fhir.rest.annotation.IdParam; import ca.uhn.fhir.rest.annotation.OptionalParam; import ca.uhn.fhir.rest.annotation.Search; +import ca.uhn.fhir.rest.api.MethodOutcome; import ca.uhn.fhir.rest.api.RestOperationTypeEnum; import ca.uhn.fhir.rest.param.StringParam; import ca.uhn.fhir.rest.server.interceptor.ResponseValidatingInterceptor; @@ -67,6 +73,28 @@ public class ResponseValidatingInterceptorDstu3Test { ourServlet.registerInterceptor(myInterceptor); } + + /** + * Test for #345 + */ + @Test + public void testDelete() throws Exception { + myInterceptor.setFailOnSeverity(null); + myInterceptor.setAddResponseHeaderOnSeverity(ResultSeverityEnum.INFORMATION); + + HttpDelete httpDelete = new HttpDelete("http://localhost:" + ourPort + "/Patient/123"); + + CloseableHttpResponse status = ourClient.execute(httpDelete); + try { + ourLog.info("Response was:\n{}", status); + + assertEquals(204, status.getStatusLine().getStatusCode()); + assertThat(status.toString(), not(containsString("X-FHIR-Request-Validation"))); + } finally { + IOUtils.closeQuietly(status); + } + } + @Test public void testLongHeaderTruncated() throws Exception { @@ -351,6 +379,11 @@ public class ResponseValidatingInterceptorDstu3Test { public static class PatientProvider implements IResourceProvider { + @Delete + public MethodOutcome delete(@IdParam IdType theId) { + return new MethodOutcome(theId.withVersion("2")); + } + @Override public Class getResourceType() { return Patient.class; diff --git a/src/changes/changes.xml b/src/changes/changes.xml index a5ebaf0f66f..5b2326180ff 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -6,7 +6,13 @@ HAPI FHIR Changelog - + + + ResponseValidatingInterceptor threw an InternalErrorException (HTTP 500) for operations + that do not return any content (e.g. delete). Thanks to Mohammad Jafari for reporting! + + + Security Fix: XML parser was vulnerable to XXE (XML External Entity) processing, which could result in local files on disk being disclosed.