abumarjikar commented on code in PR #3272:
URL: https://github.com/apache/solr/pull/3272#discussion_r2006372500
##########
solr/modules/scripting/src/java/org/apache/solr/scripting/update/ScriptUpdateProcessorFactory.java:
##########
@@ -216,13 +215,6 @@ void setScriptEngineCustomizer(ScriptEngineCustomizer
scriptEngineCustomizer) {
@Override
public void inform(SolrCore core) {
- if (!core.getCoreDescriptor().isConfigSetTrusted()) {
Review Comment:
I see both sides of this discussion. On one hand, logging a warning could
help raise awareness of potential security risks for users who might not
thoroughly review the documentation. However, I also understand the concern
about unnecessary warnings creating noise or discouraging valid use cases.
Would a compromise be to ensure that security implications are
well-documented and perhaps include an explicit warning in the documentation
instead? That way, we provide clear guidance without adding potentially
alarming log messages. If we agree on this, I’m happy to help improve the
documentation accordingly.
Current warning note on ScriptUpdateProcessor documentation looks casual:
> Being able to run a script of your choice as part of the indexing pipeline
is a really powerful tool, that I sometimes call the Get out of jail free card
because you can solve some problems this way that you can’t in any other way.
However, you are introducing some potential security vulnerabilities.
##########
solr/modules/scripting/src/java/org/apache/solr/scripting/update/ScriptUpdateProcessorFactory.java:
##########
@@ -216,13 +215,6 @@ void setScriptEngineCustomizer(ScriptEngineCustomizer
scriptEngineCustomizer) {
@Override
public void inform(SolrCore core) {
- if (!core.getCoreDescriptor().isConfigSetTrusted()) {
Review Comment:
If we agree to add warn log, i can make changes as suggested.
@gerlowskija @dsmiley @epugh
##########
solr/modules/scripting/src/java/org/apache/solr/scripting/xslt/XSLTUpdateRequestHandler.java:
##########
@@ -93,14 +93,6 @@ public void load(
return;
}
- if (req.getCore().getCoreDescriptor().isConfigSetTrusted() == false) {
Review Comment:
added my comment in ScriptUpdateProcessorFactory
##########
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java:
##########
@@ -140,7 +140,7 @@ public FileVisitResult postVisitDirectory(Path dir,
IOException ioException)
}
@Override
- protected void uploadConfig(String configName, Path source) throws
IOException {
+ public void uploadConfig(String configName, Path source) throws IOException {
Review Comment:
before changing this to public we used another method which was accepting
with trusted flag which was public and that was internally calling this method,
so keeping this protected worked earlier as it was only call from its member
method.
I can add one public method but that would again will be redirecting. Open
for thoughts on this.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
