From 89fa46bb3943da0bd2ab8417a7d70cd9798d8b95 Mon Sep 17 00:00:00 2001 From: adityamparikh Date: Sat, 2 May 2026 16:59:46 -0400 Subject: [PATCH] fix(security): bind user query via Solr parameter dereferencing to prevent local-param injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SearchService.search previously passed user input directly to Solr's standard query parser via solrQuery.setQuery(query). Solr's syntax supports a {!parser arg=val} prefix that switches parsers mid-query, enabling {!xmlparser} (historical XXE/RCE — CVE-2017-12629), {!join} (cross-collection access), and {!func}/_val_ (DoS-grade plans). Bind user input to a separate qq parameter and reference it from a constant q value: q = {!edismax v=$qq} qq = eDisMax does not honor {!parser ...} prefixes inside its input, so local-param injection is defanged. The user's query is treated as keywords/field names, not as a parser-switch directive. Adds unit tests asserting that XML parser, join parser, and function query injection attempts are all bound into qq instead of being interpreted by the standard parser. Refs: - CWE-943: Improper Neutralization of Special Elements in Data Query Logic - Solr Reference Guide — Parameter Substitution - CVE-2017-12629 (historical xmlparser RCE) Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: adityamparikh --- .../solr/mcp/server/search/SearchService.java | 21 +++- .../mcp/server/search/SearchServiceTest.java | 113 +++++++++++++++++- 2 files changed, 128 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java index c29ccb6a..18016b35 100644 --- a/src/main/java/org/apache/solr/mcp/server/search/SearchService.java +++ b/src/main/java/org/apache/solr/mcp/server/search/SearchService.java @@ -196,8 +196,14 @@ private static Map> getFacets(QueryResponse queryRespo * @param collection * The Solr collection to query * @param query - * The Solr query string (q parameter). Defaults to "*:*" if not - * specified + * The user-supplied search expression. To prevent local-param + * injection (e.g. {@code {!xmlparser ...}}, {@code {!join ...}}, + * {@code {!func}}/{@code _val_}), this value is bound to the + * {@code qq} request parameter and dereferenced from a constant + * {@code q={!edismax v=$qq}}. The eDisMax parser is forced and does + * not honor a {@code {!parser ...}} prefix in its input, so any such + * prefix is treated as literal characters. Defaults to {@code *:*} + * if not specified * @param filterQueries * List of filter queries (fq parameter) * @param facetFields @@ -251,9 +257,18 @@ public SearchResponse search(@McpToolParam(description = "Solr collection to que throws SolrServerException, IOException { // query + // + // Security: bind user input via Solr parameter dereferencing so that the + // standard query parser never sees raw user input. The actual q value is + // the constant "{!edismax v=$qq}", which tells Solr to parse the value of + // the qq parameter using eDisMax. eDisMax does not honor a {!parser ...} + // local-param prefix inside its input, so injection attempts such as + // {!xmlparser ...}, {!join ...}, or {!func}/_val_ are treated as literal + // characters rather than parser switches. See CWE-943. final SolrQuery solrQuery = new SolrQuery("*:*"); if (StringUtils.hasText(query)) { - solrQuery.setQuery(query); + solrQuery.setQuery("{!edismax v=$qq}"); + solrQuery.set("qq", query); } // filter queries diff --git a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java index e12d9d12..773b666e 100644 --- a/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java +++ b/src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java @@ -34,6 +34,7 @@ import org.apache.solr.common.SolrDocumentList; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.condition.DisabledInNativeImage; +import org.mockito.ArgumentCaptor; /** * Unit tests for SearchService with mocked SolrClient. @@ -57,6 +58,7 @@ void search_WithNullQuery_ShouldDefaultToMatchAll() throws Exception { when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenAnswer(invocation -> { SolrQuery q = invocation.getArgument(1); assertEquals("*:*", q.getQuery()); + assertNull(q.get("qq")); return mockResponse; }); SearchService localService = new SearchService(mockClient); @@ -65,7 +67,110 @@ void search_WithNullQuery_ShouldDefaultToMatchAll() throws Exception { } @Test - void search_WithCustomQuery_ShouldUseProvidedQuery() throws Exception { + void search_WithBlankQuery_ShouldDefaultToMatchAllAndNotBindQq() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + SolrDocumentList mockDocuments = createMockDocumentList(); + when(mockResponse.getResults()).thenReturn(mockDocuments); + when(mockResponse.getFacetFields()).thenReturn(null); + when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenAnswer(invocation -> { + SolrQuery q = invocation.getArgument(1); + assertEquals("*:*", q.getQuery()); + assertNull(q.get("qq")); + return mockResponse; + }); + SearchService localService = new SearchService(mockClient); + SearchResponse result = localService.search("test_collection", " ", null, null, null, null, null); + assertNotNull(result); + } + + @Test + void search_WithSimpleQueryString_ShouldBindToQqAndForceEdismax() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + SolrDocumentList mockDocuments = createMockDocumentList(); + when(mockResponse.getResults()).thenReturn(mockDocuments); + when(mockResponse.getFacetFields()).thenReturn(null); + ArgumentCaptor captor = ArgumentCaptor.forClass(SolrQuery.class); + when(mockClient.query(eq("test_collection"), captor.capture())).thenReturn(mockResponse); + + SearchService localService = new SearchService(mockClient); + SearchResponse result = localService.search("test_collection", "laptop", null, null, null, null, null); + assertNotNull(result); + + SolrQuery captured = captor.getValue(); + assertEquals("{!edismax v=$qq}", captured.getQuery()); + assertEquals("laptop", captured.get("qq")); + } + + @Test + void search_WithXmlParserInjection_ShouldBindMaliciousStringToQq() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + SolrDocumentList mockDocuments = createMockDocumentList(); + when(mockResponse.getResults()).thenReturn(mockDocuments); + when(mockResponse.getFacetFields()).thenReturn(null); + ArgumentCaptor captor = ArgumentCaptor.forClass(SolrQuery.class); + when(mockClient.query(eq("test_collection"), captor.capture())).thenReturn(mockResponse); + + String malicious = "{!xmlparser v=''}"; + SearchService localService = new SearchService(mockClient); + SearchResponse result = localService.search("test_collection", malicious, null, null, null, null, null); + assertNotNull(result); + + SolrQuery captured = captor.getValue(); + // The constant q forces eDisMax — the malicious string MUST end up in qq, + // NOT in q where the standard parser would honor the {!xmlparser ...} prefix. + assertEquals("{!edismax v=$qq}", captured.getQuery()); + assertEquals(malicious, captured.get("qq")); + assertFalse(captured.getQuery().contains("xmlparser"), + "q must not contain user-supplied parser switch directives"); + } + + @Test + void search_WithJoinParserInjection_ShouldBindMaliciousStringToQq() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + SolrDocumentList mockDocuments = createMockDocumentList(); + when(mockResponse.getResults()).thenReturn(mockDocuments); + when(mockResponse.getFacetFields()).thenReturn(null); + ArgumentCaptor captor = ArgumentCaptor.forClass(SolrQuery.class); + when(mockClient.query(eq("test_collection"), captor.capture())).thenReturn(mockResponse); + + String malicious = "{!join from=id fromIndex=other to=id}*:*"; + SearchService localService = new SearchService(mockClient); + SearchResponse result = localService.search("test_collection", malicious, null, null, null, null, null); + assertNotNull(result); + + SolrQuery captured = captor.getValue(); + assertEquals("{!edismax v=$qq}", captured.getQuery()); + assertEquals(malicious, captured.get("qq")); + assertFalse(captured.getQuery().contains("join"), "q must not allow cross-collection {!join ...} injection"); + } + + @Test + void search_WithFunctionQueryInjection_ShouldBindMaliciousStringToQq() throws Exception { + SolrClient mockClient = mock(SolrClient.class); + QueryResponse mockResponse = mock(QueryResponse.class); + SolrDocumentList mockDocuments = createMockDocumentList(); + when(mockResponse.getResults()).thenReturn(mockDocuments); + when(mockResponse.getFacetFields()).thenReturn(null); + ArgumentCaptor captor = ArgumentCaptor.forClass(SolrQuery.class); + when(mockClient.query(eq("test_collection"), captor.capture())).thenReturn(mockResponse); + + String malicious = "_val_:recip(rord(id),1,1000,1000)"; + SearchService localService = new SearchService(mockClient); + SearchResponse result = localService.search("test_collection", malicious, null, null, null, null, null); + assertNotNull(result); + + SolrQuery captured = captor.getValue(); + assertEquals("{!edismax v=$qq}", captured.getQuery()); + assertEquals(malicious, captured.get("qq")); + assertFalse(captured.getQuery().contains("_val_"), "q must not allow function-query (_val_) injection"); + } + + @Test + void search_WithCustomQuery_ShouldBindToQqParameter() throws Exception { SolrClient mockClient = mock(SolrClient.class); QueryResponse mockResponse = mock(QueryResponse.class); String customQuery = "name:\"Spring Boot\""; @@ -74,7 +179,8 @@ void search_WithCustomQuery_ShouldUseProvidedQuery() throws Exception { when(mockResponse.getFacetFields()).thenReturn(null); when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenAnswer(invocation -> { SolrQuery q = invocation.getArgument(1); - assertEquals(customQuery, q.getQuery()); + assertEquals("{!edismax v=$qq}", q.getQuery()); + assertEquals(customQuery, q.get("qq")); return mockResponse; }); SearchService localService = new SearchService(mockClient); @@ -165,7 +271,8 @@ void search_WithAllParameters_ShouldCombineAllOptions() throws Exception { when(mockResponse.getFacetFields()).thenReturn(createMockFacetFields()); when(mockClient.query(eq("test_collection"), any(SolrQuery.class))).thenAnswer(invocation -> { SolrQuery captured = invocation.getArgument(1); - assertEquals(query, captured.getQuery()); + assertEquals("{!edismax v=$qq}", captured.getQuery()); + assertEquals(query, captured.get("qq")); assertArrayEquals(filterQueries.toArray(), captured.getFilterQueries()); assertNotNull(captured.getFacetFields()); assertEquals(start, captured.getStart());