From fd91ce76ce50cd0c37d73985b7abef978eaa0269 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 5 Aug 2015 12:57:04 -0400 Subject: [PATCH] Fix #192 - Correctly unescape search parameters in the server when they have a trailing comma or an escaped backslash --- .travis.yml | 3 +- .../fhir/rest/method/QualifiedParamList.java | 78 +++++++++++++------ .../ca/uhn/fhir/rest/param/ParameterUtil.java | 1 + .../fhir/rest/server/TokenParameterTest.java | 78 ++++++++++++++++++- src/changes/changes.xml | 5 ++ 5 files changed, 139 insertions(+), 26 deletions(-) diff --git a/.travis.yml b/.travis.yml index 127349dc524..e98f5c574d7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,12 +14,11 @@ cache: install: /bin/true +# This seems to be required to get travis to set Xmx4g, per https://github.com/travis-ci/travis-ci/issues/3893 before_script: - export MAVEN_SKIP_RC=true script: - - export MAVEN_OPTS="-XX:MaxPermSize=512m -Xmx4g" - mvn -B clean install && cd hapi-fhir-cobertura && mvn -B -DTRAVIS_JOB_ID=$TRAVIS_JOB_ID -P COBERTURA clean cobertura:cobertura coveralls:report -# - mvn -B clean install -Dcobertura.skip=true && mvn -B -DTRAVIS_JOB_ID=$TRAVIS_JOB_ID -P COBERTURA clean cobertura:cobertura coveralls:report diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/QualifiedParamList.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/QualifiedParamList.java index addf5d7b275..f1bda5042e1 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/QualifiedParamList.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/method/QualifiedParamList.java @@ -1,5 +1,7 @@ package ca.uhn.fhir.rest.method; +import static org.apache.commons.lang3.StringUtils.isBlank; + /* * #%L * HAPI FHIR - Core Library @@ -29,21 +31,21 @@ import ca.uhn.fhir.model.api.IQueryParameterType; public class QualifiedParamList extends ArrayList { private static final long serialVersionUID = 1L; - + private String myQualifier; public QualifiedParamList() { super(); } - + public QualifiedParamList(int theCapacity) { super(theCapacity); } public QualifiedParamList(IQueryParameterOr theNextOr) { for (IQueryParameterType next : theNextOr.getValuesAsQueryTokens()) { - if (myQualifier==null) { - myQualifier=next.getQueryParameterQualifier(); + if (myQualifier == null) { + myQualifier = next.getQueryParameterQualifier(); } add(next.getValueAsQueryToken()); } @@ -60,36 +62,68 @@ public class QualifiedParamList extends ArrayList { public static QualifiedParamList singleton(String theParamValue) { return singleton(null, theParamValue); } - + public static QualifiedParamList singleton(String theQualifier, String theParamValue) { QualifiedParamList retVal = new QualifiedParamList(1); retVal.setQualifier(theQualifier); retVal.add(theParamValue); return retVal; } - - - public static QualifiedParamList splitQueryStringByCommasIgnoreEscape(String theQualifier, String theParams){ - QualifiedParamList retVal = new QualifiedParamList(); - retVal.setQualifier(theQualifier); - - StringTokenizer tok = new StringTokenizer(theParams,","); - String prev=null; + + public static QualifiedParamList splitQueryStringByCommasIgnoreEscape(String theQualifier, String theParams) { + QualifiedParamList retVal = new QualifiedParamList(); + retVal.setQualifier(theQualifier); + + StringTokenizer tok = new StringTokenizer(theParams, ",", true); + String prev = null; while (tok.hasMoreElements()) { String str = tok.nextToken(); - if (prev!=null&&prev.endsWith("\\")) { - int idx = retVal.size()-1; - String existing = retVal.get(idx); - retVal.set(idx, existing.substring(0, existing.length()-1) + "," + str); - }else { - retVal.add(str); + if (isBlank(str)) { + prev = null; + continue; } - prev=str; + if (str.equals(",")) { + if (countTrailingSlashes(prev) % 2 == 1) { + int idx = retVal.size() - 1; + String existing = retVal.get(idx); + prev = existing.substring(0, existing.length() - 1) + ','; + retVal.set(idx, prev); + } else { + prev = null; + } + continue; + } + + if (prev != null && prev.length() > 0 && prev.charAt(prev.length() - 1) == ',') { + int idx = retVal.size() - 1; + String existing = retVal.get(idx); + prev = existing + str; + retVal.set(idx, prev); + } else { + retVal.add(str); + prev = str; + } + + } + + return retVal; + } + + private static int countTrailingSlashes(String theString) { + if(theString==null) { + return 0; + } + int retVal = 0; + for (int i = theString.length() - 1; i >= 0; i--) { + char nextChar = theString.charAt(i); + if (nextChar != '\\') { + break; + } else { + retVal++; + } } - return retVal; } - } diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/ParameterUtil.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/ParameterUtil.java index 87ded1d6a3c..a63ea0c73a0 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/ParameterUtil.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/param/ParameterUtil.java @@ -255,6 +255,7 @@ public class ParameterUtil { case '$': case ',': case '|': + case '\\': continue; default: b.append(next); diff --git a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/TokenParameterTest.java b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/TokenParameterTest.java index 716e6df33b9..0412c17b164 100644 --- a/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/TokenParameterTest.java +++ b/hapi-fhir-structures-dstu/src/test/java/ca/uhn/fhir/rest/server/TokenParameterTest.java @@ -49,16 +49,90 @@ public class TokenParameterTest { * Test #192 */ @Test - public void testOrListWithEscapedValue() throws Exception { + public void testOrListWithEscapedValue1() throws Exception { HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=" + UrlUtil.escape("system|code-include-but-not-end-with-comma\\,suffix")); HttpResponse status = ourClient.execute(httpGet); IOUtils.closeQuietly(status.getEntity().getContent()); assertEquals(200, status.getStatusLine().getStatusCode()); + assertEquals("system", ourLastOrList.getListAsCodings().get(0).getSystemElement().getValue()); + assertEquals("code-include-but-not-end-with-comma,suffix", ourLastOrList.getListAsCodings().get(0).getCodeElement().getValue()); + assertEquals(1, ourLastOrList.getListAsCodings().size()); + } + + /** + * Test #192 + */ + @Test + public void testOrListWithEscapedValue2() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=" + UrlUtil.escape("system|code-include-end-with-comma\\,")); + HttpResponse status = ourClient.execute(httpGet); + IOUtils.closeQuietly(status.getEntity().getContent()); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertEquals(1, ourLastOrList.getListAsCodings().size()); assertEquals("system", ourLastOrList.getListAsCodings().get(0).getSystemElement().getValue()); - assertEquals("code-include-but-not-end-with-comma,suffix", ourLastOrList.getListAsCodings().get(0).getCodeElement().getValue()); + assertEquals("code-include-end-with-comma,", ourLastOrList.getListAsCodings().get(0).getCodeElement().getValue()); + } + + /** + * Test #192 + */ + @Test + public void testOrListWithEscapedValue3() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=" + UrlUtil.escape("system|code-include-end-with-comma1,system|code-include-end-with-comma2,,,,,")); + HttpResponse status = ourClient.execute(httpGet); + IOUtils.closeQuietly(status.getEntity().getContent()); + + assertEquals(200, status.getStatusLine().getStatusCode()); + + assertEquals(2, ourLastOrList.getListAsCodings().size()); + assertEquals("system", ourLastOrList.getListAsCodings().get(0).getSystemElement().getValue()); + assertEquals("code-include-end-with-comma1", ourLastOrList.getListAsCodings().get(0).getCodeElement().getValue()); + assertEquals("system", ourLastOrList.getListAsCodings().get(1).getSystemElement().getValue()); + assertEquals("code-include-end-with-comma2", ourLastOrList.getListAsCodings().get(1).getCodeElement().getValue()); + } + + /** + * Test #192 + */ + @Test + public void testOrListWithEscapedValue4() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=" + UrlUtil.escape("\\,\\,\\,value1\\,\\,\\,with\\,\\,\\,commas\\,\\,\\,,,,\\,\\,\\,value2\\,\\,\\,with\\,\\,\\,commas,,,\\,")); + HttpResponse status = ourClient.execute(httpGet); + IOUtils.closeQuietly(status.getEntity().getContent()); + + assertEquals(200, status.getStatusLine().getStatusCode()); + + assertEquals(null, ourLastOrList.getListAsCodings().get(0).getSystemElement().getValue()); + assertEquals(",,,value1,,,with,,,commas,,,", ourLastOrList.getListAsCodings().get(0).getCodeElement().getValue()); + assertEquals(null, ourLastOrList.getListAsCodings().get(1).getSystemElement().getValue()); + assertEquals(",,,value2,,,with,,,commas", ourLastOrList.getListAsCodings().get(1).getCodeElement().getValue()); + assertEquals(null, ourLastOrList.getListAsCodings().get(2).getSystemElement().getValue()); + assertEquals(",", ourLastOrList.getListAsCodings().get(2).getCodeElement().getValue()); + assertEquals(3, ourLastOrList.getListAsCodings().size()); + } + + /** + * Test #192 + */ + @Test + public void testOrListWithEscapedValue5() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=" + UrlUtil.escape("A\\\\,B,\\$")); + HttpResponse status = ourClient.execute(httpGet); + IOUtils.closeQuietly(status.getEntity().getContent()); + + assertEquals(200, status.getStatusLine().getStatusCode()); + + assertEquals(null, ourLastOrList.getListAsCodings().get(0).getSystemElement().getValue()); + assertEquals("A\\", ourLastOrList.getListAsCodings().get(0).getCodeElement().getValue()); + assertEquals(null, ourLastOrList.getListAsCodings().get(1).getSystemElement().getValue()); + assertEquals("B", ourLastOrList.getListAsCodings().get(1).getCodeElement().getValue()); + assertEquals(null, ourLastOrList.getListAsCodings().get(2).getSystemElement().getValue()); + assertEquals("$", ourLastOrList.getListAsCodings().get(2).getCodeElement().getValue()); + assertEquals(3, ourLastOrList.getListAsCodings().size()); } @AfterClass diff --git a/src/changes/changes.xml b/src/changes/changes.xml index d59e05eca4b..76a8f470965 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -67,6 +67,11 @@ JPA server and generic client now both support the _tag search parameter + + Server was not correctly unescaping URL parameter values with + a trailing comma or an escaped backslash. Thanks to GitHub user + @SherryH for all of her help in diagnosing this issue! +