diff --git a/changelog/unreleased/SOLR-18345-encodeLocalParamVal.yml b/changelog/unreleased/SOLR-18345-encodeLocalParamVal.yml new file mode 100644 index 000000000000..f176e84b675f --- /dev/null +++ b/changelog/unreleased/SOLR-18345-encodeLocalParamVal.yml @@ -0,0 +1,10 @@ +title: > + SolrJ ClientUtils.encodeLocalParamVal() can produce lossy/invalid encodings with a backslash or leading quotes. + Affects faceting with a custom facet response key. + Affects the SQL module for LIKE queries. +type: fixed +authors: + - name: David Smiley +links: + - name: SOLR-18345 + url: https://issues.apache.org/jira/browse/SOLR-18345 diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java b/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java index bf06c491d032..32d8006cccaf 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java @@ -236,8 +236,19 @@ public static String encodeLocalParamVal(String val) { int len = val.length(); if (0 == len) return "''"; // quoted empty string + // Note: QueryParsing#parseLocalParams's peek() (used to check for a '=' or the closing quote + // char) skips leading whitespace as a side effect, so an unquoted empty value would silently + // absorb the whitespace meant to separate it from the next local param, corrupting parsing of + // everything that follows. Quoting sidesteps this entirely. + int i = 0; - if (len > 0 && val.charAt(0) != '$') { + char first = val.charAt(0); + // A leading '$' would be read back as a param dereference, and a leading quote char would be + // read back as the start of a quoted string (StrParser#getQuotedString accepts both ' and " + // as delimiters); both must be quoted regardless of the rest of the value. + if (first == '$' || first == '\'' || first == '"') { + // leave i == 0 so the quoting branch below is taken + } else { for (; i < len; i++) { char ch = val.charAt(i); if (Character.isWhitespace(ch) || ch == '}') break; @@ -246,12 +257,16 @@ public static String encodeLocalParamVal(String val) { if (i >= len) return val; - // We need to enclose in quotes... but now we need to escape + // We need to enclose in quotes... but now we need to escape. Both the quote delimiter itself + // and a literal backslash must be escaped: StrParser#getQuotedString treats any '\' as the + // start of an escape sequence when reading a quoted value, so an un-escaped '\' here would be + // silently consumed (or worse, combined with the following char into an unintended escape like + // \n) when the value is parsed back. StringBuilder sb = new StringBuilder(val.length() + 4); sb.append('\''); for (i = 0; i < len; i++) { char ch = val.charAt(i); - if (ch == '\'') { + if (ch == '\'' || ch == '\\') { sb.append('\\'); } sb.append(ch); diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java index 2b17ed41386a..da6083b108ec 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java @@ -16,12 +16,14 @@ */ package org.apache.solr.client.solrj.util; +import org.apache.lucene.tests.util.TestUtil; import org.apache.solr.SolrTestCase; import org.apache.solr.client.solrj.request.CollectionAdminRequest; import org.apache.solr.client.solrj.request.HealthCheckRequest; import org.apache.solr.client.solrj.request.QueryRequest; import org.apache.solr.client.solrj.request.UpdateRequest; -import org.apache.solr.client.solrj.request.XMLRequestWriter; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.search.QueryParsing; import org.junit.Test; /** @@ -37,6 +39,49 @@ public void testEscapeQuery() { assertEquals("h\\~\\!", ClientUtils.escapeQueryChars("h~!")); } + // FYI also tested via org.apache.solr.common.params.SolrParamTest.testLocalParamRoundTripParsing + public void testEncodeLocalParamValRoundTrip() throws Exception { + // Values that require quoting (whitespace, '}', or a leading '$') must round-trip through + // Solr's own local-params reader, in particular values containing a literal backslash or + // single quote. + assertRoundTrips(""); + assertRoundTrips("'leadingQuote"); + assertRoundTrips("\"leadingDoubleQuote"); + assertRoundTrips("plain"); + assertRoundTrips("has space"); + assertRoundTrips("trailing}brace"); + assertRoundTrips("has'quote and space"); + assertRoundTrips("has\\backslash and space"); + assertRoundTrips("both\\'kinds together"); + assertRoundTrips("$dollarPrefixed"); + assertRoundTrips("$dollarPrefixed with space"); + assertRoundTrips("$\\'mix of everything"); + + for (int i = 0; i < 100; i++) { + assertRoundTrips(TestUtil.randomUnicodeString(random())); + } + } + + private void assertRoundTrips(String original) throws Exception { + String encoded = ClientUtils.encodeLocalParamVal(original); + String txt = "{!key=" + encoded + "}"; + ModifiableSolrParams target = new ModifiableSolrParams(); + QueryParsing.parseLocalParams(txt, 0, target, null); + assertEquals( + "encodeLocalParamVal(" + original + ") -> " + encoded + " did not round-trip", + original, + target.get("key")); + + // Also confirm the encoded value doesn't swallow whatever follows it. + String txtFollowedByAnother = "{!key=" + encoded + " next=followed}"; + ModifiableSolrParams targetFollowedByAnother = new ModifiableSolrParams(); + QueryParsing.parseLocalParams(txtFollowedByAnother, 0, targetFollowedByAnother, null); + assertEquals( + "encodeLocalParamVal(" + original + ") -> " + encoded + " swallowed the next local param", + "followed", + targetFollowedByAnother.get("next")); + } + @Test public void testDeterminesWhenToUseDefaultCollection() { final var noDefaultNeededRequest = new CollectionAdminRequest.List(); @@ -55,7 +100,6 @@ public void testDeterminesWhenToUseDefaultCollection() { @Test public void testUrlBuilding() throws Exception { - final var rw = new XMLRequestWriter(); // Simple case, non-collection request { final var request = new HealthCheckRequest();