You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2016/12/16 23:57:54 UTC
[Bug 60490] New: Several improvements to the ErrorReportValve
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
Bug ID: 60490
Summary: Several improvements to the ErrorReportValve
Product: Tomcat 8
Version: 8.5.x-trunk
Hardware: All
OS: All
Status: NEW
Severity: enhancement
Priority: P2
Component: Catalina
Assignee: dev@tomcat.apache.org
Reporter: 1983-01-06@gmx.net
Target Milestone: ----
Created attachment 34531
--> https://bz.apache.org/bugzilla/attachment.cgi?id=34531&action=edit
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
--- Comment #8 from Michael Osipov <19...@gmx.net> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
--- Comment #7 from Christopher Schultz <ch...@christopherschultz.net> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
--- Comment #1 from Christopher Schultz <ch...@christopherschultz.net> ---
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
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
--- Comment #4 from Michael Osipov <19...@gmx.net> ---
Anyone willing to take a look? The longer the patch remains stale the more
merge conflicts are likely.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
Michael Osipov <19...@gmx.net> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #34531|0 |1
is obsolete| |
--- Comment #3 from Michael Osipov <19...@gmx.net> ---
Created attachment 34537
--> https://bz.apache.org/bugzilla/attachment.cgi?id=34537&action=edit
Patch for ErrorReportValve
Updated patch
* Line endings normalized
* message is already escaped at the start of report()
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
--- Comment #2 from Michael Osipov <19...@gmx.net> ---
(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
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
--- Comment #9 from Mark Thomas <ma...@apache.org> ---
Further back-ported to:
- 8.0.x for 8.0.53 onwards
- 7.0.x for 7.0.89 onwards
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #5 from Mark Thomas <ma...@apache.org> ---
Thanks for the patch.
Fixed in:
- trunk for 9.0.0.M18 onwards
- 8.5.x for 8.5.12 onwards
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 60490] Several improvements to the ErrorReportValve
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=60490
--- Comment #6 from Michael Osipov <19...@gmx.net> ---
(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
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org