3274 transactions with full request url should be rejected (#3280)

* 3274 throw if url is full url

* 3274 will throw on invalid urls

* 3274 fixing the tests

* 3274 cleanup

* anonymize the data

* review fixes

* 3274 fixed for backward compatibility

* added review fixes

* fixing tests

* some more test fixes

* test fixes

* 3274 fixing the check to only look for resource at begining

* 3274 fixed test

* test fixes

Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-MacBook-Pro.local>
This commit is contained in:
TipzCM 2022-01-17 11:46:32 -05:00 committed by GitHub
parent 456afa17e9
commit 221d7ed5b7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
17 changed files with 161 additions and 61 deletions

View File

@ -0,0 +1,4 @@
---
issue: 3274
title: "Transactions with entries that have request.url values that are fully
qualified urls will now be rejected."

View File

@ -42,6 +42,7 @@ import ca.uhn.fhir.rest.param.TokenParamModifier;
import ca.uhn.fhir.rest.param.UriParam;
import ca.uhn.fhir.rest.param.UriParamQualifierEnum;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.util.UrlUtil;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang3.StringUtils;
import org.hl7.fhir.dstu3.model.Appointment;
@ -94,7 +95,6 @@ import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.instance.model.api.IIdType;
import org.hl7.fhir.instance.model.api.IPrimitiveType;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.springframework.test.context.TestPropertySource;
import org.springframework.transaction.TransactionDefinition;
@ -110,7 +110,6 @@ import java.util.Date;
import java.util.List;
import java.util.Set;
import java.util.TreeSet;
import java.util.stream.Collectors;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
@ -272,7 +271,8 @@ public class FhirResourceDaoDstu3SearchNoFtTest extends BaseJpaDstu3Test {
Set<String> allIds = new TreeSet<>();
for (BundleEntryComponent nextEntry : inputBundle.getEntry()) {
nextEntry.getRequest().setMethod(HTTPVerb.PUT);
nextEntry.getRequest().setUrl(nextEntry.getResource().getId());
UrlUtil.UrlParts parts = UrlUtil.parseUrl(nextEntry.getResource().getId());
nextEntry.getRequest().setUrl(parts.getResourceType() + "/" + parts.getResourceId());
allIds.add(nextEntry.getResource().getIdElement().toUnqualifiedVersionless().getValue());
}

View File

@ -1108,21 +1108,6 @@ public class FhirSystemDaoDstu3Test extends BaseJpaDstu3SystemTest {
}
}
@Test
public void testTransactionCreateWithPutUsingAbsoluteUrl() {
String methodName = "testTransactionCreateWithPutUsingAbsoluteUrl";
Bundle request = new Bundle();
request.setType(BundleType.TRANSACTION);
Patient p = new Patient();
p.addIdentifier().setSystem("urn:system").setValue(methodName);
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.PUT).setUrl("http://localhost/server/base/Patient/" + methodName);
mySystemDao.transaction(mySrd, request);
myPatientDao.read(new IdType("Patient/" + methodName), mySrd);
}
@Test
public void testTransactionCreateWithPutUsingUrl() {
String methodName = "testTransactionCreateWithPutUsingUrl";
@ -2685,7 +2670,7 @@ public class FhirSystemDaoDstu3Test extends BaseJpaDstu3SystemTest {
Patient p = new Patient();
p.setActive(true);
p.setId(IdType.newRandomUuid());
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl(p.getId());
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl("Patient/" + p.getId());
Observation o = new Observation();
o.getCode().setText("Some Observation");
@ -2765,7 +2750,7 @@ public class FhirSystemDaoDstu3Test extends BaseJpaDstu3SystemTest {
Patient p = new Patient();
p.setActive(true);
p.setId(IdType.newRandomUuid());
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl(p.getId());
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST);
Observation o = new Observation();
o.getCode().setText("Some Observation");

View File

@ -8,6 +8,8 @@ import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
import ca.uhn.fhir.model.primitive.IdDt;
import ca.uhn.fhir.rest.api.server.IBundleProvider;
import ca.uhn.fhir.rest.param.TokenParam;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
import ca.uhn.fhir.util.BundleBuilder;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.instance.model.api.IIdType;
@ -27,6 +29,7 @@ import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Arrays;
import java.util.Date;
@ -43,6 +46,7 @@ import static org.hamcrest.Matchers.matchesPattern;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoR4VersionedReferenceTest.class);
@ -832,6 +836,36 @@ public class FhirResourceDaoR4VersionedReferenceTest extends BaseJpaR4Test {
assertThat(versionedPatientReference, is(equalTo("Patient/RED/_history/1")));
}
@Test
public void bundleTransaction_withRequestUrlNotRelativePath_doesNotProcess() throws IOException {
Bundle bundle = loadResourceFromClasspath(Bundle.class, "/transaction-bundles/transaction-with-full-request-url.json");
try {
// test
mySystemDao.transaction(new ServletRequestDetails(),
bundle);
fail("We expect invalid full urls to fail");
} catch (InvalidRequestException ex) {
Assertions.assertTrue(ex.getMessage().contains("Unable to perform POST, URL provided is invalid:"));
}
}
@Test
public void bundleTransaction_withRequestURLWithPrecedingSlash_processesAsExpected() throws IOException {
Bundle bundle = loadResourceFromClasspath(Bundle.class, "/transaction-bundles/transaction-with-preceding-slash-request-url.json");
// test
Bundle outcome = mySystemDao.transaction(new SystemRequestDetails(),
bundle);
// verify it was created
Assertions.assertEquals(1, outcome.getEntry().size());
IdType idType = new IdType(bundle.getEntry().get(0)
.getResource().getId());
// the bundle above contains an observation, so we'll verify it was created here
Observation obs = myObservationDao.read(idType);
Assertions.assertNotNull(obs);
}
@Test
@DisplayName("Bundle transaction with AutoVersionReferenceAtPath on and with existing Patient resource should create")

View File

@ -2094,31 +2094,16 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest {
request.addEntry()
.setResource(o)
.getRequest().setUrl("A").setMethod(HTTPVerb.PUT);
.getRequest().setUrl("Observation").setMethod(HTTPVerb.PUT);
try {
mySystemDao.transaction(mySrd, request);
fail();
} catch (InvalidRequestException e) {
assertEquals("Invalid match URL[Observation?A] - URL has no search parameters", e.getMessage());
assertEquals("Invalid match URL[Observation?Observation] - URL has no search parameters", e.getMessage());
}
}
@Test
public void testTransactionCreateWithPutUsingAbsoluteUrl() {
String methodName = "testTransactionCreateWithPutUsingAbsoluteUrl";
Bundle request = new Bundle();
request.setType(BundleType.TRANSACTION);
Patient p = new Patient();
p.addIdentifier().setSystem("urn:system").setValue(methodName);
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.PUT).setUrl("http://localhost/server/base/Patient/" + methodName);
mySystemDao.transaction(mySrd, request);
myPatientDao.read(new IdType("Patient/" + methodName), mySrd);
}
@Test
public void testTransactionCreateWithPutUsingUrl() {
String methodName = "testTransactionCreateWithPutUsingUrl";
@ -4084,7 +4069,7 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest {
Patient p = new Patient();
p.setActive(true);
p.setId(IdType.newRandomUuid());
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl(p.getId());
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl("Patient/" + p.getId());
Observation o = new Observation();
o.getCode().setText("Some Observation");
@ -4164,7 +4149,7 @@ public class FhirSystemDaoR4Test extends BaseJpaR4SystemTest {
Patient p = new Patient();
p.setActive(true);
p.setId(IdType.newRandomUuid());
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl(p.getId());
request.addEntry().setResource(p).getRequest().setMethod(HTTPVerb.POST).setUrl("Patient/" + p.getId());
Observation o = new Observation();
o.getCode().setText("Some Observation");

View File

@ -5,7 +5,6 @@ import ca.uhn.fhir.jpa.dao.data.ISearchDao;
import ca.uhn.fhir.jpa.entity.Search;
import ca.uhn.fhir.jpa.provider.r4.ResourceProviderR4Test;
import ca.uhn.fhir.jpa.search.SearchCoordinatorSvcImpl;
import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
import ca.uhn.fhir.model.api.TemporalPrecisionEnum;
import ca.uhn.fhir.model.primitive.InstantDt;
import ca.uhn.fhir.model.primitive.UriDt;
@ -15,7 +14,6 @@ import ca.uhn.fhir.rest.api.CacheControlDirective;
import ca.uhn.fhir.rest.api.Constants;
import ca.uhn.fhir.rest.api.MethodOutcome;
import ca.uhn.fhir.rest.api.SummaryEnum;
import ca.uhn.fhir.rest.api.server.IBundleProvider;
import ca.uhn.fhir.rest.client.api.IClientInterceptor;
import ca.uhn.fhir.rest.client.api.IGenericClient;
import ca.uhn.fhir.rest.client.api.IHttpRequest;
@ -28,7 +26,6 @@ import ca.uhn.fhir.rest.param.ParamPrefixEnum;
import ca.uhn.fhir.rest.param.StringAndListParam;
import ca.uhn.fhir.rest.param.StringOrListParam;
import ca.uhn.fhir.rest.param.StringParam;
import ca.uhn.fhir.rest.param.TokenParam;
import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException;
@ -122,7 +119,6 @@ import org.hl7.fhir.dstu3.model.Task;
import org.hl7.fhir.dstu3.model.UnsignedIntType;
import org.hl7.fhir.dstu3.model.ValueSet;
import org.hl7.fhir.dstu3.model.codesystems.DeviceStatus;
import org.hl7.fhir.instance.model.api.IAnyResource;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.instance.model.api.IIdType;
import org.junit.jupiter.api.AfterEach;
@ -1818,7 +1814,8 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test {
Set<String> allIds = new TreeSet<String>();
for (BundleEntryComponent nextEntry : inputBundle.getEntry()) {
nextEntry.getRequest().setMethod(HTTPVerb.PUT);
nextEntry.getRequest().setUrl(nextEntry.getResource().getId());
UrlUtil.UrlParts parts = UrlUtil.parseUrl(nextEntry.getResource().getId());
nextEntry.getRequest().setUrl(parts.getResourceType() + "/" + parts.getResourceId());
allIds.add(nextEntry.getResource().getIdElement().toUnqualifiedVersionless().getValue());
}

View File

@ -1040,7 +1040,7 @@ public class ResourceProviderDstu3ValueSetVersionedTest extends BaseResourceProv
.setResource(updatedValueSet_v1)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl("ValueSet/" + updatedValueSet_v1.getIdElement().getIdPart());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
ourClient.transaction().withBundle(bundle).execute();
@ -1062,7 +1062,7 @@ public class ResourceProviderDstu3ValueSetVersionedTest extends BaseResourceProv
.setResource(updatedValueSet_v2)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl("ValueSet/" + updatedValueSet_v2.getIdElement().getIdPart());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
ourClient.transaction().withBundle(bundle).execute();

View File

@ -1163,7 +1163,6 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
*/
@Test
public void testInstanceRuleOkForResourceWithNoId() {
ourRestServer.registerInterceptor(new AuthorizationInterceptor(PolicyEnum.DENY) {
@Override
public List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
@ -1183,8 +1182,11 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
Patient p = new Patient();
p.setActive(true);
p.setId(IdType.newRandomUuid());
request.addEntry().setResource(p).getRequest().setMethod(Bundle.HTTPVerb.POST).setUrl(p.getId());
p.setId("123");
request.addEntry().setResource(p).getRequest().setMethod(Bundle.HTTPVerb.POST)
.setUrl(
"Patient/"+
p.getId());
Observation o = new Observation();
o.getCode().setText("Some Observation");

View File

@ -886,7 +886,7 @@ public class ResourceProviderR4ValueSetNoVerCSNoVerTest extends BaseResourceProv
.setResource(updatedValueSet)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl(myExtensionalVsId.getValueAsString());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
myClient.transaction().withBundle(bundle).execute();

View File

@ -776,7 +776,7 @@ public class ResourceProviderR4ValueSetVerCSNoVerTest extends BaseResourceProvid
.setResource(updatedValueSet)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl(myExtensionalVsId.getValueAsString());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
myClient.transaction().withBundle(bundle).execute();

View File

@ -995,7 +995,7 @@ public class ResourceProviderR4ValueSetVerCSVerTest extends BaseResourceProvider
.setResource(updatedValueSet_v1)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl(myExtensionalVsId_v1.getValueAsString());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
myClient.transaction().withBundle(bundle).execute();
@ -1017,7 +1017,7 @@ public class ResourceProviderR4ValueSetVerCSVerTest extends BaseResourceProvider
.setResource(updatedValueSet_v2)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl(myExtensionalVsId_v2.getValueAsString());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
myClient.transaction().withBundle(bundle).execute();

View File

@ -1094,7 +1094,7 @@ public class ResourceProviderR5ValueSetTest extends BaseResourceProviderR5Test {
.setResource(updatedValueSet)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl(myExtensionalVsId.getValueAsString());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
myClient.transaction().withBundle(bundle).execute();

View File

@ -1025,7 +1025,7 @@ public class ResourceProviderR5ValueSetVersionedTest extends BaseResourceProvide
.setResource(updatedValueSet_v1)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl(myExtensionalVsId_v1.getValueAsString());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
myClient.transaction().withBundle(bundle).execute();
@ -1047,7 +1047,7 @@ public class ResourceProviderR5ValueSetVersionedTest extends BaseResourceProvide
.setResource(updatedValueSet_v2)
.getRequest()
.setMethod(Bundle.HTTPVerb.PUT)
.setUrl(url);
.setUrl(myExtensionalVsId_v2.getValueAsString());
ourLog.info("Transaction Bundle:\n" + myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle));
myClient.transaction().withBundle(bundle).execute();

View File

@ -0,0 +1,16 @@
{
"resourceType": "Bundle",
"type": "transaction",
"entry": [
{
"request": {
"method": "POST",
"url": "http://localhost:8000/fhir/Goal"
},
"resource": {
"resourceType": "Goal",
"id": "4bebccef-de86-4b49-9ca8-7567bffcc6f5"
}
}
]
}

View File

@ -0,0 +1,16 @@
{
"resourceType": "Bundle",
"type": "transaction",
"entry": [
{
"request": {
"method": "POST",
"url": "/Observation"
},
"resource": {
"resourceType": "Observation",
"id": "4bebccef-de86-4b49-9ca8-7567bffcc6f5"
}
}
]
}

View File

@ -894,6 +894,16 @@ public abstract class BaseTransactionProcessor {
switch (verb) {
case "POST": {
// CREATE
/*
* To preserve existing functionality,
* we will only verify that the request url is
* valid if it's provided at all.
* Otherwise, we'll ignore it
*/
String url = myVersionAdapter.getEntryRequestUrl(nextReqEntry);
if (isNotBlank(url)) {
extractAndVerifyTransactionUrlForEntry(nextReqEntry, verb);
}
validateResourcePresent(res, order, verb);
@SuppressWarnings("rawtypes")
IFhirResourceDao resourceDao = getDaoOrThrowException(res.getClass());
@ -920,7 +930,7 @@ public abstract class BaseTransactionProcessor {
}
case "DELETE": {
// DELETE
String url = extractTransactionUrlOrThrowException(nextReqEntry, verb);
String url = extractAndVerifyTransactionUrlForEntry(nextReqEntry, verb);
UrlUtil.UrlParts parts = UrlUtil.parseUrl(url);
IFhirResourceDao<? extends IBaseResource> dao = toDao(parts, verb, url);
int status = Constants.STATUS_HTTP_204_NO_CONTENT;
@ -959,7 +969,7 @@ public abstract class BaseTransactionProcessor {
@SuppressWarnings("rawtypes")
IFhirResourceDao resourceDao = getDaoOrThrowException(res.getClass());
String url = extractTransactionUrlOrThrowException(nextReqEntry, verb);
String url = extractAndVerifyTransactionUrlForEntry(nextReqEntry, verb);
DaoMethodOutcome outcome;
UrlUtil.UrlParts parts = UrlUtil.parseUrl(url);
@ -1003,7 +1013,7 @@ public abstract class BaseTransactionProcessor {
// PATCH
validateResourcePresent(res, order, verb);
String url = extractTransactionUrlOrThrowException(nextReqEntry, verb);
String url = extractAndVerifyTransactionUrlForEntry(nextReqEntry, verb);
UrlUtil.UrlParts parts = UrlUtil.parseUrl(url);
String matchUrl = toMatchUrl(nextReqEntry);
@ -1543,6 +1553,58 @@ public abstract class BaseTransactionProcessor {
myContext = theContext;
}
/**
* Extracts the transaction url from the entry and verifies it's:
* * not null or bloack
* * is a relative url matching the resourceType it is about
*
* Returns the transaction url (or throws an InvalidRequestException if url is not valid)
*/
private String extractAndVerifyTransactionUrlForEntry(IBase theEntry, String theVerb) {
String url = extractTransactionUrlOrThrowException(theEntry, theVerb);
if (!isValidResourceTypeUrl(url)) {
ourLog.debug("Invalid url. Should begin with a resource type: {}", url);
String msg = myContext.getLocalizer().getMessage(BaseStorageDao.class, "transactionInvalidUrl", theVerb, url);
throw new InvalidRequestException(msg);
}
return url;
}
/**
* Returns true if the provided url is a valid entry request.url.
*
* This means:
* a) not an absolute url (does not start with http/https)
* b) starts with either a ResourceType or /ResourceType
*/
private boolean isValidResourceTypeUrl(@Nonnull String theUrl) {
if (UrlUtil.isAbsolute(theUrl)) {
return false;
} else {
int queryStringIndex = theUrl.indexOf("?");
String url;
if (queryStringIndex > 0) {
url = theUrl.substring(0, theUrl.indexOf("?"));
} else {
url = theUrl;
}
String[] parts;
if (url.startsWith("/")) {
parts = url.substring(1).split("/");
} else {
parts = url.split("/");
}
Set<String> allResourceTypes = myContext.getResourceTypes();
return allResourceTypes.contains(parts[0]);
}
}
/**
* Extracts the transaction url from the entry and verifies that it is not null/blank
* and returns it
*/
private String extractTransactionUrlOrThrowException(IBase nextEntry, String verb) {
String url = myVersionAdapter.getEntryRequestUrl(nextEntry);
if (isBlank(url)) {

View File

@ -32,7 +32,6 @@ import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.Stack;
import static org.junit.jupiter.api.Assertions.assertEquals;
@ -99,7 +98,7 @@ public class JsonLikeParserTest {
assertEquals("x1", extension.get("url"), "Expecting '/extension[]/url' = 'x1'; found '"+extension.get("url")+"'");
}
/**
* Repeat the "View" tests with custom JSON-Like structure
*/