From 6330b9e4d9a5dbef8caaad57a350595243b30f1c Mon Sep 17 00:00:00 2001 From: David Smiley Date: Wed, 12 Aug 2026 18:09:29 -0400 Subject: [PATCH 1/3] SOLR-18345: ClientUtils.encodeLocalParamVal() can produces lossy/invalid encodings Affects faceting with a custom facet response key. Affects the SQL module for LIKE queries. --- .../solr/client/solrj/util/ClientUtils.java | 27 ++++++++---- .../client/solrj/util/ClientUtilsTest.java | 41 ++++++++++++++++++- 2 files changed, 59 insertions(+), 9 deletions(-) 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..6a9f7ba48dbb 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 @@ -234,24 +234,37 @@ public static String escapeQueryChars(String s) { */ public static String encodeLocalParamVal(String val) { int len = val.length(); - if (0 == len) return "''"; // quoted empty string + // An empty value needs no quoting: QueryParsing#parseLocalParams reads an unquoted value up to + // whitespace/the end char, so a zero-length span there is already unambiguous. int i = 0; - if (len > 0 && val.charAt(0) != '$') { - for (; i < len; i++) { - char ch = val.charAt(i); - if (Character.isWhitespace(ch) || ch == '}') break; + if (len > 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; + } } } 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..b3b67004f02f 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,42 @@ public void testEscapeQuery() { assertEquals("h\\~\\!", ClientUtils.escapeQueryChars("h~!")); } + public void testEncodeLocalParamValEmptyIsUnquoted() { + assertEquals("", ClientUtils.encodeLocalParamVal("")); + } + + 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("'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")); + } + @Test public void testDeterminesWhenToUseDefaultCollection() { final var noDefaultNeededRequest = new CollectionAdminRequest.List(); @@ -55,7 +93,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(); From 38fd44d583fbe9f8c2cd03f5f3143de29b1f8040 Mon Sep 17 00:00:00 2001 From: David Smiley Date: Wed, 12 Aug 2026 18:12:48 -0400 Subject: [PATCH 2/3] CHANGELOG --- .../unreleased/SOLR-18345-encodeLocalParamVal.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changelog/unreleased/SOLR-18345-encodeLocalParamVal.yml 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 From aaa144ea00cd36bfb56c3beb061eac3f3a36729a Mon Sep 17 00:00:00 2001 From: David Smiley Date: Thu, 13 Aug 2026 00:24:30 -0400 Subject: [PATCH 3/3] put back empty string case --- .../solr/client/solrj/util/ClientUtils.java | 30 ++++++++++--------- .../client/solrj/util/ClientUtilsTest.java | 15 +++++++--- 2 files changed, 27 insertions(+), 18 deletions(-) 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 6a9f7ba48dbb..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 @@ -234,22 +234,24 @@ public static String escapeQueryChars(String s) { */ 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. - // An empty value needs no quoting: QueryParsing#parseLocalParams reads an unquoted value up to - // whitespace/the end char, so a zero-length span there is already unambiguous. int i = 0; - if (len > 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; - } + 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; } } 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 b3b67004f02f..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 @@ -39,14 +39,12 @@ public void testEscapeQuery() { assertEquals("h\\~\\!", ClientUtils.escapeQueryChars("h~!")); } - public void testEncodeLocalParamValEmptyIsUnquoted() { - assertEquals("", ClientUtils.encodeLocalParamVal("")); - } - + // 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"); @@ -73,6 +71,15 @@ private void assertRoundTrips(String original) throws Exception { "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