adityamparikh opened a new pull request, #127: URL: https://github.com/apache/solr-mcp/pull/127
## Summary The [MCP Tools specification](https://modelcontextprotocol.io/specification/2025-06-18/server/tools#security-considerations) requires servers to *"validate all tool inputs"*. The \`search\` tool previously passed three categories of caller input straight through to Solr without sanitization. This PR fixes all three. ### 1. Filter-query injection (\`fq\`) \`filterQueries: List<String>\` was passed straight to \`solrQuery.addFilterQuery(...)\`. Solr's \`fq\` parameter uses the same standard query parser as \`q\`, so the \`{!xmlparser …}\`, \`{!join …}\`, and \`{!frange …}\` local-param injection vectors that #122 fixed for \`q\` are equally reachable via \`fq\`. Apply the same parameter-dereferencing pattern: bind each user-supplied filter to a separate \`fq{n}\` param and reference it from a constant \`{!edismax v=\$fq{n}}\`. ([CWE-943](https://cwe.mitre.org/data/definitions/943.html)) ### 2. Sort-field injection The \`item\` field of each \`sortClauses\` entry flowed into Solr's \`sort=\` parameter, which accepts function queries (\`sort=if(rord(_val_:…),1,0) asc\`) — enabling expensive query plans (lower-severity DoS). Validate field names against a strict \`^[a-zA-Z_][a-zA-Z0-9_.]*\$\` regex and reject anything else with \`IllegalArgumentException\`. The \`order\` value must parse as a valid \`SolrQuery.ORDER\` (\`asc\`/\`desc\`, case-insensitive). ### 3. Pagination bounds \`rows\` and \`start\` were unbounded — a caller could request \`rows=2_000_000_000\` and OOM the JVM, or set \`start\` deep enough to trigger Solr's own deep-paging DoS surface. Clamp at \`MAX_ROWS=1000\` and \`MAX_START=100_000\` (both private static constants for easy adjustment), with a \`Math.max(value, 0)\` floor for negative inputs. ([CWE-1284](https://cwe.mitre.org/data/definitions/1284.html)) ## Test plan - [x] 6 new unit tests for \`fq\` (plain, xmlparser injection, join injection, frange injection, blank skipped, multiple filters) - [x] 6 new unit tests for sort (valid plain, valid dotted, function-query injection, brace injection, \`_val_\` injection, invalid order) - [x] 3 new unit tests for rows/start clamping (excessive rows clamped, null rows not set, excessive start clamped) - [x] 2 existing tests updated to match the dereferenced form - [x] \`./gradlew build\` passes (full suite, 37s) ## Note on PR ordering Mirrors the parameter-dereferencing pattern from #122 (\`q\` injection). Both PRs touch \`SearchService.search\`; whichever merges later will need a trivial rebase. ## Out of scope Rate limiting — the spec's other input-related \`MUST\`. Tracked separately so it can use a deliberate dependency choice (Bucket4j / resilience4j / Spring AOP). 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
