Repository navigation
Conversation
Header or param. Useful to reach a specific node. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
FYI I've been using this for awhile at work. Are there security concerns here? I don't think so. This is live-nodes only. |
code: use Solr's URL param parsing
janhoy
left a comment
There was a problem hiding this comment.
I delegate review to Copilot who is probably better at servlet spec than me. I do like the general solution and I believe the security aspect is handled so so far a LGTM from me from 10.000 ft
janhoy
left a comment
There was a problem hiding this comment.
Some manual review comments as I'm out of copilot credit...
There was a problem hiding this comment.
🟡 Changes recommended
Caller identity preservation and request-routing correctness need correction before approval.
5 open issues
Proxy authentication uses node identity instead of caller identity · New Authentication filter masks proxy errors and response statuses · New Routing directive causes proxy loops on non-replica nodes · New Decoded request paths corrupt upstream URL encoding · New Dotless live-node names are incorrectly expanded · New
Review effort: Balanced
| HttpClient httpClient = | ||
| ((HttpJettySolrClient) cores.getDefaultHttpSolrClient()).getHttpClient(); | ||
| HttpSolrProxy.doHttpProxy(httpClient, req, resp, url); |
There was a problem hiding this comment.
I'm investigating this separately.
Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
…uting loops Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
…icationFilter Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
… caller Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
|
It's really remarkable how much this PR has improved/evolved as a result of such peer-review/scrutiny. |
There was a problem hiding this comment.
technically, this aspect is a small bug fix to possible encoding issue in the path. That's very unlikely to happen in practice IMO.
| // also accept it as a query param on GET requests, for convenience when a header can't | ||
| // easily be set (e.g. a browser address bar) | ||
| String queryString = req.getQueryString(); | ||
| if ("GET".equals(req.getMethod()) | ||
| && queryString != null | ||
| && queryString.contains(TARGET_NODE_HEADER)) { // cheap; avoid double-parse | ||
| SolrParams params = SolrRequestParsers.parseQueryString(queryString); | ||
| if (targetNode != null && params.get(TARGET_NODE_HEADER) != null) { | ||
| res.sendError(400, TARGET_NODE_HEADER + " must EITHER be a header or url param"); | ||
| return; | ||
| } | ||
| targetNode = params.get(TARGET_NODE_HEADER); | ||
| if (targetNode != null) { | ||
| queryString = filterQueryString(params); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
there is non-trivial complexity to support the query string option, particularly for removal. It's a sunk costs as this point -- it works, tested, etc. The motivation is to make this easy for a user with their browser.
| // wrap the request to filter out the header & param. | ||
| // NOTE: we assume downstream processing only accesses such data via what we override !! |
There was a problem hiding this comment.
an alternative would be to enhance HttpSolrProxy to directly do this. The header part would trivially be one line in hop-by-hop headers with a comment justification. I don't love the coupling but it's in the same package.


https://issues.apache.org/jira/browse/SOLR-18537