jdaugherty commented on code in PR #15564:
URL: https://github.com/apache/grails-core/pull/15564#discussion_r3124479666
##########
grails-bootstrap/src/main/groovy/org/grails/exceptions/reporting/DefaultStackTraceFilterer.java:
##########
@@ -81,32 +89,32 @@ public Throwable filter(Throwable source, boolean
recursive) {
if (recursive) {
Throwable current = source;
while (current != null) {
- current = filter(current);
+ filter(current);
current = current.getCause();
}
+ return source;
}
return filter(source);
}
public Throwable filter(Throwable source) {
- if (shouldFilter) {
- StackTraceElement[] trace = source.getStackTrace();
- List<StackTraceElement> newTrace = filterTraceWithCutOff(trace,
cutOffPackage);
+ if (!shouldFilter) {
+ return source;
+ }
+ StackTraceElement[] trace = source.getStackTrace();
+ List<StackTraceElement> newTrace = filterTraceWithCutOff(trace,
cutOffPackage);
- if (newTrace.isEmpty()) {
- // filter with no cut-off so at least there is some trace
- newTrace = filterTraceWithCutOff(trace, null);
- }
+ if (newTrace.isEmpty()) {
+ // filter with no cut-off so at least there is some trace
+ newTrace = filterTraceWithCutOff(trace, null);
+ }
- // Only trim the trace if there was some application trace on the
stack
- // if not we will just skip sanitizing and leave it as is
- if (!newTrace.isEmpty()) {
- // We don't want to lose anything, so log it
- STACK_LOG.error(FULL_STACK_TRACE_MESSAGE, source);
Review Comment:
We discussed this and we think this should stay with a configuration setting.
##########
grails-web-mvc/src/main/groovy/org/grails/web/errors/GrailsExceptionResolver.java:
##########
@@ -261,6 +266,70 @@ protected void logStackTrace(Exception e,
HttpServletRequest request) {
LOG.error(getRequestLogMessage(e, request), e);
}
+ /**
+ * When the {@code grails.exceptionresolver.logFullStackTrace} property is
enabled,
+ * emits the unfiltered stack trace to the dedicated {@code StackTrace}
logger.
+ * Must be invoked <em>before</em> {@link #filterStackTrace(Exception)} —
once the
+ * filterer calls {@code setStackTrace(clean)}, the original frames are
gone and
+ * this method can only log the already-trimmed trace.
+ */
+ protected void logFullStackTraceIfEnabled(Exception e) {
+ if (shouldLogFullStackTrace()) {
+
DefaultStackTraceFilterer.STACK_LOG.error(StackTraceFilterer.FULL_STACK_TRACE_MESSAGE,
e);
+ }
+ }
+
+ protected boolean shouldLogFullStackTrace() {
+ Config config = grailsApplication != null ?
grailsApplication.getConfig() : null;
+ return config != null &&
config.getProperty(Settings.SETTING_LOG_FULL_STACKTRACE, Boolean.class, false);
+ }
+
+ protected boolean shouldLogAuditor() {
+ Config config = grailsApplication != null ?
grailsApplication.getConfig() : null;
+ return config != null &&
config.getProperty(Settings.SETTING_LOG_AUDITOR, Boolean.class, true);
+ }
+
+ protected boolean shouldLogRemoteAddr() {
+ Config config = grailsApplication != null ?
grailsApplication.getConfig() : null;
+ return config != null &&
config.getProperty(Settings.SETTING_LOG_REMOTE_ADDR, Boolean.class, true);
+ }
+
+ /**
+ * Resolves the client address to include in the exception log headline.
The default
+ * returns {@link HttpServletRequest#getRemoteAddr()} — the container's
view of the
+ * TCP peer, which reflects forwarded-header handling only when the
servlet container
+ * is configured to trust a proxy chain (for example Spring Boot's
+ * {@code server.forward-headers-strategy}). Subclasses can override this
to apply a
+ * different resolution strategy; the returned value (null or empty to
omit) is
+ * appended verbatim as {@code ip: <value>}.
+ */
+ protected String resolveRemoteAddr(HttpServletRequest request) {
+ return request.getRemoteAddr();
Review Comment:
This can be wrong for proxy based solutions. This can also be considered
PII in some areas so I think we may want to disable this by default.
##########
grails-datamapping-support/src/test/groovy/org/grails/datastore/mapping/core/grailsversion/GrailsVersionSpec.groovy:
##########
@@ -36,7 +36,7 @@ class GrailsVersionSpec extends Specification {
"3.2.0" | true
"3.1.0" | true
"3.3.0" | true
- "7.1.0" | false
+ "7.1.0" | true
Review Comment:
Can you please update this branch? I've fixed this by increasing the value,
we need a "false" case for this test.
--
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]
