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());