Bug 53071 - ErrorReportValve ignores message from throwable
Summary: ErrorReportValve ignores message from throwable
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 6.0.35
Hardware: All All
: P2 normal (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-13 09:34 UTC by Michael Osipov
Modified: 2012-08-27 21:29 UTC (History)
0 users



Attachments
Screen shot from Tomcat 7.0.27 (73.67 KB, image/jpeg)
2012-06-12 19:45 UTC, Konstantin Kolinko
Details
Screen shot from Tomcat trunk (83.38 KB, image/jpeg)
2012-06-12 19:47 UTC, Konstantin Kolinko
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2012-04-13 09:34:03 UTC
When setting

> request.setAttribute(Globals.EXCEPTION_ATTR, e);
> response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);

in a server-side component the throwable#getMessage is completely ignored and only the stacktrace is shown. Message is not retrieved. Line 161 has to be simply replaced with

> if (throwable != null)
>   message = RequestUtil.filter(throwable.getMessage());
> else
>   message = RequestUtil.filter(response.getMessage());

The throwable message is nicely displayed.
Comment 1 Michael Osipov 2012-04-13 09:34:41 UTC
This change could also be applied to Tomcat 7.
Comment 2 Mark Thomas 2012-06-11 09:32:59 UTC
Fixed in trunk and 7.0.x and will be included in 7.0.28 onwards.

Proposed for 6.0.x

Notes:
1. The message from the Throwable was displayed in the stack trace.
2. I modified the patch. The message from the Throwable is only used if no message
   is specified via sendError()
Comment 3 Michael Osipov 2012-06-11 12:03:33 UTC
(In reply to comment #2)
> Fixed in trunk and 7.0.x and will be included in 7.0.28 onwards.
> 
> Proposed for 6.0.x
> 
> Notes:
> 1. The message from the Throwable was displayed in the stack trace.

I am aware of that but I don't want to burden my users with the stack traces, the message is just fine.

> 2. I modified the patch. The message from the Throwable is only used if no
> message
>    is specified via sendError()

This is perfectly fine and does the job.

Thanks for the fix. Looking forward to 6.0.36.
Comment 4 Christopher Schultz 2012-06-11 17:04:17 UTC
Putting this caveat in the bug and not just on the dev list:

This might end up being a security problem, depending on what information is in the exception message. Can we make this a non-default option? Many sites (ours included) attempt to avoid any part of a stack trace (even the message) leaking-out to users.
Comment 5 Mark Thomas 2012-06-11 17:24:24 UTC
Then you have already replaced / modified the error report valve and you won't see this change.
Comment 6 Konstantin Kolinko 2012-06-12 19:45:22 UTC
Created attachment 28920 [details]
Screen shot from Tomcat 7.0.27

Just a screen shot, for comparison. It is before implementing this feature.
It is CookieExample page, when pressing submit button without entering any values.
Comment 7 Konstantin Kolinko 2012-06-12 19:47:00 UTC
Created attachment 28921 [details]
Screen shot from Tomcat trunk

The same with Tomcat trunk after implementing the feature.
Comment 8 Konstantin Kolinko 2012-06-12 20:01:21 UTC
(In reply to comment #4)
> Putting this caveat in the bug and not just on the dev list:

I agree with Mark here.
The change in r1348762 is in the place where ErrorReportValve prepares HTML text of the page. It does not affect other components. It does not affect components that display their own error pages.  The old implementation already displays the exception, so nothing new is revealed.


I personally do not like use of "{0}" in the messages for the "description" field. Especially the ones for 404 and 403. It looks like some unrelated text is inserted into the middle of a sentence. With this change it is printed 4 times on the same page.
Comment 9 Michael Osipov 2012-06-12 20:13:30 UTC
(In reply to comment #8)
> (In reply to comment #4)
> > Putting this caveat in the bug and not just on the dev list:
> 
> I agree with Mark here.
> The change in r1348762 is in the place where ErrorReportValve prepares HTML
> text of the page. It does not affect other components. It does not affect
> components that display their own error pages.  The old implementation
> already displays the exception, so nothing new is revealed.

Exactly, the same information but more prominent.

> I personally do not like use of "{0}" in the messages for the "description"
> field. Especially the ones for 404 and 403. It looks like some unrelated
> text is inserted into the middle of a sentence. With this change it is
> printed 4 times on the same page.

I personally dislike that one too in this valve.

In my opinion, the following should happen:

If message is not given: Message should be from https://tools.ietf.org/html/rfc2616#section-6.1.1 as per status code short name and description should be a copy of the first sentence of the RFC description. Maybe just as HTTPd does.
If message is given: Display message and show RFC description too.

Stack trace should be left as is.

It might make sense to limit the message length in the first line to n chars and show the entire message in the message line. But if and only if the message is to0 long. Otherwise the message line should be omitted.
Comment 10 Mark Thomas 2012-07-04 20:47:48 UTC
Fixed in 6.0.x and will be included in 6.0.36 onwards.
Comment 11 Rainer Jung 2012-07-16 12:36:09 UTC
(In reply to comment #8)
> I personally do not like use of "{0}" in the messages for the "description"
> field. Especially the ones for 404 and 403. It looks like some unrelated
> text is inserted into the middle of a sentence. With this change it is
> printed 4 times on the same page.

I removed the {0} from the "report" in r1361991 (trunk) and r1362000 (tc7).

Motivation:

As far as I can see, those properties are only used in ErrorReportValve. There they are only put into the "report", which is only output in an HTML document directly below the "message" that would have been put into {0}.

In addition the same "message" is again being output as an h1 heading shortly before.

So I see no reason at all to keep the "message" inside the "report".
Comment 12 Michael Osipov 2012-07-20 18:52:21 UTC
(In reply to comment #11)
> (In reply to comment #8)
> > I personally do not like use of "{0}" in the messages for the "description"
> > field. Especially the ones for 404 and 403. It looks like some unrelated
> > text is inserted into the middle of a sentence. With this change it is
> > printed 4 times on the same page.
> 
> I removed the {0} from the "report" in r1361991 (trunk) and r1362000 (tc7).

Rainer,

can you backport to Tomcat 6 too?
Comment 13 Konstantin Kolinko 2012-08-11 22:46:34 UTC
Reopening this. This needs further improvement.

When there is a java compilation error in JSP page, the error page becomes especially ugly. Reproducible in 7.0.29.

To reproduce:
1. Change ROOT/index.jsp introducing a typo in Java code scriptlet, e.g.
s/SimpleDateFormat/SimpleDate Format/

2. Access the page.

Actual result: The whole text of the compilation exception, including a source code fragment, is included in the <h1> header, as well as in the "message" field. That is a lot of text without any formatting, occupying the whole screen. It is ugly.

Expected result: Maybe include just some part of the text, and replace the rest with ellipsis " (...)"? Maybe use just the first line, as line breaks are lost when rendering HTML?


Also,
the changes mentioned in Comment 11 still have to be proposed/fixed in 6.0.
Comment 14 Mark Thomas 2012-08-12 15:45:39 UTC
Jasper issue fixed in trunk and 7.0.x.

Jasper fix and {0} fix proposed for 6.0.c
Comment 15 Mark Thomas 2012-08-27 21:29:04 UTC
Fixed in 6.0.x and will be included in 6.0.36 onwards.