SOLR-13181: param macro expansion could throw (#1877)
...StringIndexOutOfBoundsException on bad syntax
* failOnMissingParams: should have been returning null (failing) on bad syntax cases
Co-authored-by: Christine Poerschke <cpoerschke@apache.org>
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index f4909e3..109347b 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -244,6 +244,10 @@
* SOLR-14821: {!terms} docValuesTermsFilterTopLevel method now works with single-valued strings (Anatolii Siuniaev
via Jason Gerlowski)
+* SOLR-13181: macro expansion of parameters could result in a StringIndexOutOfBoundsException.
+ Also, params with macros will be processed more strictly by LTR FeatureWeight.
+ (David Smiley, Cesar Rodriguez, Christine Poerschke)
+
Other Changes
---------------------
diff --git a/solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java b/solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
index 67219e5..144ce06 100644
--- a/solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
+++ b/solr/core/src/java/org/apache/solr/request/macro/MacroExpander.java
@@ -119,8 +119,8 @@
int start = 0; // start of the unprocessed part of the string
StringBuilder sb = null;
for (;;) {
+ assert idx >= start;
idx = val.indexOf(macroStart, idx);
- int matchedStart = idx;
// check if escaped
if (idx > 0) {
@@ -135,18 +135,19 @@
}
}
else if (idx < 0) {
- if (sb == null) return val;
- sb.append(val.substring(start));
- return sb.toString();
+ break;
}
// found unescaped "${"
- idx += macroStart.length();
+ final int matchedStart = idx;
- int rbrace = val.indexOf('}', idx);
+ int rbrace = val.indexOf('}', matchedStart + macroStart.length());
if (rbrace == -1) {
// no matching close brace...
- continue;
+ if (failOnMissingParams) {
+ return null;
+ }
+ break;
}
if (sb == null) {
@@ -154,14 +155,14 @@
}
if (matchedStart > 0) {
- sb.append(val.substring(start, matchedStart));
+ sb.append(val, start, matchedStart);
}
// update "start" to be at the end of ${...}
- start = rbrace + 1;
+ idx = start = rbrace + 1;
- // String inbetween = val.substring(idx, rbrace);
- StrParser parser = new StrParser(val, idx, rbrace);
+ // String in-between braces
+ StrParser parser = new StrParser(val, matchedStart + macroStart.length(), rbrace);
try {
String paramName = parser.getId();
String defVal = null;
@@ -187,13 +188,19 @@
}
} catch (SyntaxError syntaxError) {
+ if (failOnMissingParams) {
+ return null;
+ }
// append the part we would have skipped
- sb.append( val.substring(matchedStart, start) );
- continue;
+ sb.append(val, matchedStart, start);
}
+ } // loop idx
+ if (sb == null) {
+ return val;
}
-
+ sb.append(val, start, val.length());
+ return sb.toString();
}
diff --git a/solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java b/solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java
index c6c60cb..99717e2 100644
--- a/solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java
+++ b/solr/core/src/test/org/apache/solr/request/macro/TestMacroExpander.java
@@ -16,14 +16,14 @@
*/
package org.apache.solr.request.macro;
-import java.util.Map;
+import java.util.Collections;
import java.util.HashMap;
-
+import java.util.Map;
import org.apache.solr.SolrTestCase;
import org.junit.Test;
-/*
+/**
* Tests for the MacroExpander
*/
public class TestMacroExpander extends SolrTestCase {
@@ -121,14 +121,13 @@
request.put("expr", new String[] {"${one_ref}"}); // expr is for streaming expressions, no replacement by default
request.put("one_ref",new String[] {"one"});
request.put("three_ref",new String[] {"three"});
- @SuppressWarnings({"rawtypes"})
- Map expanded = MacroExpander.expand(request);
- assertEquals("zero", ((String[])expanded.get("fq"))[0]);
- assertEquals("one", ((String[])expanded.get("fq"))[1]);
- assertEquals("two", ((String[]) expanded.get("fq"))[2]);
- assertEquals("three", ((String[]) expanded.get("fq"))[3]);
+ Map<String, String[]> expanded = MacroExpander.expand(request);
+ assertEquals("zero", expanded.get("fq")[0]);
+ assertEquals("one", expanded.get("fq")[1]);
+ assertEquals("two", expanded.get("fq")[2]);
+ assertEquals("three", expanded.get("fq")[3]);
- assertEquals("${one_ref}", ((String[])expanded.get("expr"))[0]);
+ assertEquals("${one_ref}", expanded.get("expr")[0]);
}
@Test
@@ -143,15 +142,36 @@
String oldVal = System.getProperty("StreamingExpressionMacros","false");
System.setProperty("StreamingExpressionMacros", "true");
try {
- @SuppressWarnings({"rawtypes"})
- Map expanded = MacroExpander.expand(request);
- assertEquals("zero", ((String[])expanded.get("fq"))[0]);
- assertEquals("one", ((String[])expanded.get("fq"))[1]);
- assertEquals("two", ((String[]) expanded.get("fq"))[2]);
- assertEquals("three", ((String[]) expanded.get("fq"))[3]);
- assertEquals("one", ((String[])expanded.get("expr"))[0]);
+ Map<String, String[]> expanded = MacroExpander.expand(request);
+ assertEquals("zero", expanded.get("fq")[0]);
+ assertEquals("one", expanded.get("fq")[1]);
+ assertEquals("two", expanded.get("fq")[2]);
+ assertEquals("three", expanded.get("fq")[3]);
+ assertEquals("one", expanded.get("expr")[0]);
} finally {
System.setProperty("StreamingExpressionMacros", oldVal);
}
}
+
+ @Test
+ public void testUnbalanced() { // SOLR-13181
+ final Map<String, String[]> request = Collections.singletonMap("answer", new String[]{ "42" });
+ final MacroExpander meSkipOnMissingParams = new MacroExpander(request);
+ final MacroExpander meFailOnMissingParams = new MacroExpander(request, true);
+ assertEquals("${noClose", meSkipOnMissingParams.expand("${noClose"));
+ assertEquals("42 ${noClose", meSkipOnMissingParams.expand("${answer} ${noClose"));
+ assertEquals("42 ${noClose fooBar", meSkipOnMissingParams.expand("${answer} ${noClose fooBar"));
+ assertNull(meFailOnMissingParams.expand("${noClose"));
+ assertNull(meFailOnMissingParams.expand("${answer} ${noClose"));
+ assertNull(meFailOnMissingParams.expand("${answer} ${noClose fooBar"));
+
+ assertEquals("${${b}}", meSkipOnMissingParams.expand("${${b}}"));
+ assertNull(meFailOnMissingParams.expand("${${b}}"));
+
+ // Does not register as a syntax failure, although may subjectively look like it.
+ // Consequently, the expression is replaced with nothing in default mode.
+ // It'd be nice if there was a mode to leave un-resolved macros as-is when they don't resolve.
+ assertEquals("preamble ", meSkipOnMissingParams.expand("preamble ${exp${bad}"));
+ assertNull(meFailOnMissingParams.expand("preamble ${exp${bad}"));
+ }
}
