Bug 60490 - Several improvements to the ErrorReportValve
Summary: Several improvements to the ErrorReportValve
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.5.x-trunk
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-16 23:57 UTC by Michael Osipov
Modified: 2017-01-24 16:52 UTC (History)
0 users



Attachments
Patch for ErrorReportValve (39.12 KB, patch)
2016-12-16 23:57 UTC, Michael Osipov
Details | Diff
Patch for ErrorReportValve (39.62 KB, patch)
2016-12-20 11:00 UTC, Michael Osipov
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Osipov 2016-12-16 23:57:54 UTC
Created attachment 34531 [details]
Patch for ErrorReportValve

Please find attached a patch (against 8.5.x trunk) for the current ErrorReportValve. It is a slightly modified version of my publically available EnhancedErrorReportValve.

What has been changed and why:
* Properties:
** Translate 'Type' to French and Spanish
** Apply proper case (upper case) for titles and labels according to Chicago Manual of Style
** Add stylistically correct en dash in English, semirraya/raya menor in Spanish and tiret demi-cadratin in French
** Remove all status codes below 400 because they aren't errors and can't be used with this valve anyway due to line 149/150
   (Cross-references aren't present to that bundle)
** Remove reference to org/apache/tomcat/util/http/res/LocalStrings.properties, this bundle does not exist any more
** Update all descriptions in English by the most recent versions of today's RFCs (through IANA listings/references)
** Split status codes to reasons and descriptions based on RFCs

* Java:
** Import TomcatCSS class for better readablity of the code
** Obtain reason and description from properties
** Align HTML code to HTML5 style
** Change head/title to status code and reason. The user (of a browser) does not care about the server running this webapp.
   Rather inform the user upfront that an HTTP error has happened and what its phrase is. More details are on the page itself.
** Have statusHeader contain only general information, details are layed out in the body
** Don't show in statusHeader message if showReport is false (applies to the issues above)
** Always include the CSS style because even if showReport is false, it still remains HTML and a few elements are displayed
** Use boolean getters rather than the fields directly
** Use translated label for 'Type'
** Don't abuse div as hr
** Don't use underline and bold, it is considered as bad style
** Omit the message line if there isn't any
** Don't print server info with rootCauseInLogs as it is confusing with the version and duplicates the information with the subsequent line
** Use System.lineSeparator() instead of \n

The goal is to clean up old strings, remove duplication and add consistency.
Comment 1 Christopher Schultz 2016-12-19 21:32:51 UTC
Some comments.

- Line endings should be either be CRLF or LF. On certain systems, System.lineSeparator may return CR only which could cause some problems[1]. I'd recommend reverting that particular change, or using CRLF.

- All of the various messages here need to be HTML-escaped before being dropped-into the HTML document. Specific examples: reason phrase, error message and description, root cause, and stack trace elements. You might consider this out-of-scope for your patch, which is okay.

[1] http://stackoverflow.com/questions/5916340/using-only-cr-as-linebreak-inside-pre-tag-doesnt-work
Comment 2 Michael Osipov 2016-12-19 22:39:28 UTC
(In reply to Christopher Schultz from comment #1)
> Some comments.
> 
> - Line endings should be either be CRLF or LF. On certain systems,
> System.lineSeparator may return CR only which could cause some problems[1].
> I'd recommend reverting that particular change, or using CRLF.

Thanks for that, looks like an oversight from me. I will rework on Tuesday. It was previously \n. How can that method return \r? Documentation says LF on Unix, CRLF on Windows.

> - All of the various messages here need to be HTML-escaped before being
> dropped-into the HTML document. Specific examples: reason phrase, error
> message and description, root cause, and stack trace elements. You might
> consider this out-of-scope for your patch, which is okay.

The messages are in our control, nothing which needs to be escaped. The stacktrace gets escaped already by RequestUtil#filter().
Why should everything but stacktrace be espaced if there is nothing unsafe for HTML? I do agree that "message" has to be escaped, yes!
 
> [1]
> http://stackoverflow.com/questions/5916340/using-only-cr-as-linebreak-inside-
> pre-tag-doesnt-work
Comment 3 Michael Osipov 2016-12-20 11:00:47 UTC
Created attachment 34537 [details]
Patch for ErrorReportValve

Updated patch

* Line endings normalized
* message is already escaped at the start of report()
Comment 4 Michael Osipov 2017-01-20 13:35:02 UTC
Anyone willing to take a look? The longer the patch remains stale the more merge conflicts are likely.
Comment 5 Mark Thomas 2017-01-23 09:50:44 UTC
Thanks for the patch.

Fixed in:
- trunk for 9.0.0.M18 onwards
- 8.5.x for 8.5.12 onwards
Comment 6 Michael Osipov 2017-01-23 11:47:07 UTC
(In reply to Mark Thomas from comment #5)
> Thanks for the patch.
> 
> Fixed in:
> - trunk for 9.0.0.M18 onwards
> - 8.5.x for 8.5.12 onwards

Thank you very much, I can finally discontinue my public version: http://mo-tomcat-ext.sourceforge.net/xref/net/sf/michaelo/tomcat/extras/valves/EnhancedErrorReportValve.html
Comment 7 Christopher Schultz 2017-01-24 16:35:16 UTC
(In reply to Michael Osipov from comment #2)
> How can that method return \r? Documentation says LF
> on Unix, CRLF on Windows.

Mac OS used to use \r for line endings. It's possible that there were never any JVMs that ran on it before they switched to a BSD base, and switched to \n.
Comment 8 Michael Osipov 2017-01-24 16:52:18 UTC
(In reply to Christopher Schultz from comment #7)
> (In reply to Michael Osipov from comment #2)
> > How can that method return \r? Documentation says LF
> > on Unix, CRLF on Windows.
> 
> Mac OS used to use \r for line endings. It's possible that there were never
> any JVMs that ran on it before they switched to a BSD base, and switched to
> \n.

Classic, more than 15 years ago. Fully neglectable.