Skip to content

fix(security): prevent Solr local-param injection in SearchService.search #122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adityamparikh wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(security): prevent Solr local-param injection in SearchService.search #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions src/main/java/org/apache/solr/mcp/server/search/SearchService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,14 @@ private static Map<String, Map<String, Long>> 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
Expand Down Expand Up @@ -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
Expand Down
113 changes: 110 additions & 3 deletions src/test/java/org/apache/solr/mcp/server/search/SearchServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Expand All @@ -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<SolrQuery> 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<SolrQuery> captor = ArgumentCaptor.forClass(SolrQuery.class);
when(mockClient.query(eq("test_collection"), captor.capture())).thenReturn(mockResponse);

String malicious = "{!xmlparser v='<root/>'}";
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<SolrQuery> 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<SolrQuery> 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\"";
Expand All @@ -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);
Expand Down Expand Up @@ -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());
Expand Down