[GitHub] [solr] cpoerschke commented on a diff in pull request #1632: SOLR-14886 : suppress stack traces in query response

["cpoerschke (via GitHub)" <gi...@apache.org>]

cpoerschke commented on code in PR #1632:
URL: https://github.com/apache/solr/pull/1632#discussion_r1192139537


##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -296,6 +296,8 @@ && getZkController().getOverseer() != null
 
   private final Set<Path> allowPaths;
 
+  private final boolean hideStackTrace;
+
   private final AllowListUrlChecker allowListUrlChecker;

Review Comment:
   minor: consider locating `hideStackTrace` after the two `allow...` to match protected constructor init order
   ```suggestion
     private final AllowListUrlChecker allowListUrlChecker;
   
     private final boolean hideStackTrace;
   ```



##########
solr/core/src/java/org/apache/solr/servlet/ResponseUtils.java:
##########
@@ -70,10 +90,13 @@ public static int getErrorInfo(Throwable ex, NamedList<Object> info, Logger log)
 
     // For any regular code, don't include the stack trace
     if (code == 500 || code < 100) {
-      StringWriter sw = new StringWriter();
-      ex.printStackTrace(new PrintWriter(sw));
-      log.error("500 Exception", ex);
-      info.add("trace", sw.toString());
+      // hide all stack traces, as configured
+      if (!hideStackTrace) {
+        StringWriter sw = new StringWriter();
+        ex.printStackTrace(new PrintWriter(sw));
+        log.error("500 Exception", ex);
+        info.add("trace", sw.toString());

Review Comment:
   Wondering if only the `info.add` here could be guarded by the `!hideStackTrace` hide condition i.e. it's logged but not returned in the response?



##########
solr/core/src/java/org/apache/solr/rest/BaseSolrResource.java:
##########
@@ -143,7 +143,11 @@ protected void handleException(Logger log) {
     Exception exception = getSolrResponse().getException();
     if (null != exception) {
       NamedList<Object> info = new SimpleOrderedMap<>();
-      this.statusCode = ResponseUtils.getErrorInfo(exception, info, log);
+      boolean hideStackTrace =
+          solrCore != null
+              ? solrCore.getCoreContainer().hideStackTrace()
+              : Boolean.parseBoolean(System.getProperty("solr.hideStackTrace"));

Review Comment:
   subjective: with multiple places have this logic, perhaps there could be a common utility method somewhere
   ```suggestion
         boolean hideStackTrace = SomeWhereUtils.getHideStrackTrace(solrCore);
   ```



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org