| Summary: | ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false | ||
|---|---|---|---|
| Product: | Tomcat 8 | Reporter: | awaltman |
| Component: | Catalina | Assignee: | Tomcat Developers Mailing List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | minor | CC: | michaelo |
| Priority: | P2 | Keywords: | Beginner |
| Version: | 8.5.x-trunk | ||
| Target Milestone: | ---- | ||
| Hardware: | All | ||
| OS: | All | ||
|
Description
awaltman
2019-11-05 22:26:06 UTC
The documentation is wrong. If you look at the CSS, you need at least for body and h1. One would need to break up the CSS in parts and include only the required bits. For the record: The CSS was removed when showServerInfo and showReport are both false for debatable security reasons as part of bug 58383. The CSS was restored in all cases as part of bug 60490. I have a preference for fixing the docs but am happy to support any reasonable solution that means the behaviour and the docs are consistent. (In reply to Mark Thomas from comment #2) > For the record: > > The CSS was removed when showServerInfo and showReport are both false for > debatable security reasons as part of bug 58383. Are you certain? That does not look related?! > The CSS was restored in all cases as part of bug 60490. > > I have a preference for fixing the docs but am happy to support any > reasonable solution that means the behaviour and the docs are consistent. Even if you fix the docs, as least a minimal CSS is required. What we can do is to reduce the CSS for starters: * <a> is never used * Setting font-family should be done at best once in the body * background color #525D76 should be done once too I will take care of this, but at low priority. Or we could just drop the CSS because... who cares? If the response entity is <!doctype html><html lang="??"><head><title>Error</title></head><body><h1>Error</h1></body></html> then it's fine. No styling is necessary for such a minimal page. (In reply to Michael Osipov from comment #3) > (In reply to Mark Thomas from comment #2) > > For the record: > > > > The CSS was removed when showServerInfo and showReport are both false for > > debatable security reasons as part of bug 58383. > > Are you certain? That does not look related?! Sorry typo, bug 56383 (In reply to Christopher Schultz from comment #4) > Or we could just drop the CSS because... who cares? If the response entity > is <!doctype html><html > lang="??"><head><title>Error</title></head><body><h1>Error</h1></body></ > html> then it's fine. No styling is necessary for such a minimal page. That would be inconsistent with the rest. The Servlet spec requires an HTML page, we will comply. I see no real fuzz by delivering those bytes given that our error page is minimal already. Please also note that TomcatCSS.TOMCAT_CSS is also used in the DefaultServlet. We either split with common parts or we stay on one and may server rules which do not apply to the error report. I would stick to one because it is plain and simple. I am already working on a simpler approach. (In reply to Mark Thomas from comment #2) > I have a preference for fixing the docs but am happy to support any > reasonable solution that means the behaviour and the docs are consistent. +1 for fixing the docs. I have uploaded a branch for this, changelog edit is pending. Please have a look. It works for me in Firefox and Edge for the Valve and the DefaultServlet with listing on. (In reply to Michael Osipov from comment #6) > (In reply to Christopher Schultz from comment #4) > > Or we could just drop the CSS because... who cares? If the response entity > > is <!doctype html><html > > lang="??"><head><title>Error</title></head><body><h1>Error</h1></body></ > > html> then it's fine. No styling is necessary for such a minimal page. > > That would be inconsistent with the rest. The Servlet spec requires an HTML > page, we will comply. CSS is not a requirement for a valid HTML document. There is no conflict between removing CSS entirely and returning a valid HTML document along with an error report. (In reply to Christopher Schultz from comment #10) > CSS is not a requirement for a valid HTML document. There is no conflict > between removing CSS entirely and returning a valid HTML document along with > an error report. The CSS provides a better user experience for the developer, the cost of having it seems non existent too. Did I miss something ? Probably not. The argument against it was made in bug 56383. I'm not convinced. I don't see how "securing the ErrorReportValve" is related to the served CSS. However, I have found a few more nits I am going through locally now where the CSS will now cleanly apply to the ErrorReportValve as well as the listing of DefaultServlet. (In reply to Michael Osipov from comment #13) > I don't see how "securing the ErrorReportValve" is related to the served CSS. It's a *thin* argument related to fingerprinting the server's version. If you modify the CSS in Tomcat 9.0.28, a client can request a page known to produce this output, check the CSS, and determine if the version is before/after that patch. Well, to some degree of certainty. > However, I have found a few more nits I am going through locally now where > the CSS will now cleanly apply to the ErrorReportValve as well as the > listing of DefaultServlet. This should all really be replaced by external stylesheets, for a few reasons: 1. They are trivially changed by administrators instead of hacking Java code 2. They can be completely blocked, replaced, etc. by a reverse proxy if desired 3. They are more compatible with a desire to reduce response entity byte count 4. They can be used with a "safe" CSF policies[1] [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src (In reply to Christopher Schultz from comment #14) > (In reply to Michael Osipov from comment #13) > > I don't see how "securing the ErrorReportValve" is related to the served CSS. > > It's a *thin* argument related to fingerprinting the server's version. If > you modify the CSS in Tomcat 9.0.28, a client can request a page known to > produce this output, check the CSS, and determine if the version is > before/after that patch. Well, to some degree of certainty. This is likely, but the paranoid should swap the ErrorReportValve for a custom one. > > However, I have found a few more nits I am going through locally now where > > the CSS will now cleanly apply to the ErrorReportValve as well as the > > listing of DefaultServlet. > > This should all really be replaced by external stylesheets, for a few > reasons: > > 1. They are trivially changed by administrators instead of hacking Java code > 2. They can be completely blocked, replaced, etc. by a reverse proxy if > desired > 3. They are more compatible with a desire to reduce response entity byte > count > 4. They can be used with a "safe" CSF policies[1] I agree here, but this is far more work than I do now at the moment. A lot of stuff is hardcoded in Java, burried in static fields. All of our default apps would require a rewrite -- and all of them are optional! Fixed in: - master for 9.0.28 onwards - 8.5.x for 8.5.48 onwards - 7.0.x for 7.0.98 onwards I have opted to fix this for now since I am the one who made the last rewrite of the ErrorReportValve output. I am quite certain that there is still room for improvement and discussion from the other committers. Let's continue this on the mailing list please. awaltman@gmail.com, if you think there is still something open to be fixed, raise this on the mailing list please, I'd be happy to jump into the discussion. (In reply to Michael Osipov from comment #15) > (In reply to Christopher Schultz from comment #14) > > This should all really be replaced by external stylesheets, for a few > > reasons: > > > > 1. They are trivially changed by administrators instead of hacking Java code > > 2. They can be completely blocked, replaced, etc. by a reverse proxy if > > desired > > 3. They are more compatible with a desire to reduce response entity byte > > count > > 4. They can be used with a "safe" CSF policies[1] > > I agree here, but this is far more work than I do now at the moment. Honestly, I see this as easy: 1. Move CSS definitions to [whatever.css] 1. Change all CSS to <link rel="stylesheet" href="[whatever.css]" /> > A lot of stuff is hardcoded in Java, buried in static fields. > All of our default apps would require a rewrite -- and all of them are optional! "Our default apps" = "Tomcat's default apps"? You gotta start somewhere. (In reply to Christopher Schultz from comment #17) > (In reply to Michael Osipov from comment #15) > > (In reply to Christopher Schultz from comment #14) > > > This should all really be replaced by external stylesheets, for a few > > > reasons: > > > > > > 1. They are trivially changed by administrators instead of hacking Java code > > > 2. They can be completely blocked, replaced, etc. by a reverse proxy if > > > desired > > > 3. They are more compatible with a desire to reduce response entity byte > > > count > > > 4. They can be used with a "safe" CSF policies[1] > > > > I agree here, but this is far more work than I do now at the moment. > > Honestly, I see this as easy: > > 1. Move CSS definitions to [whatever.css] > 1. Change all CSS to <link rel="stylesheet" href="[whatever.css]" /> It is not that easy. TOMCAT_CSS is used in multiple locations. You have to make sure that they are present for several components. > > A lot of stuff is hardcoded in Java, buried in static fields. > > All of our default apps would require a rewrite -- and all of them are optional! > > "Our default apps" = "Tomcat's default apps"? Correct! > You gotta start somewhere. Analysis first, then discuss. (In reply to Michael Osipov from comment #18) > (In reply to Christopher Schultz from comment #17) > > (In reply to Michael Osipov from comment #15) > > > (In reply to Christopher Schultz from comment #14) > > > > This should all really be replaced by external stylesheets, for a few > > > > reasons: > > > > > > > > 1. They are trivially changed by administrators instead of hacking Java code > > > > 2. They can be completely blocked, replaced, etc. by a reverse proxy if > > > > desired > > > > 3. They are more compatible with a desire to reduce response entity byte > > > > count > > > > 4. They can be used with a "safe" CSF policies[1] > > > > > > I agree here, but this is far more work than I do now at the moment. > > > > Honestly, I see this as easy: > > > > 1. Move CSS definitions to [whatever.css] > > 1. Change all CSS to <link rel="stylesheet" href="[whatever.css]" /> > > It is not that easy. TOMCAT_CSS is used in multiple locations. You have to > make sure that they are present for several components. So use TOMCAT_EXTERNAL_CSS as a replacement, and only implement it in e.g. DefaultServlet. No need to re-write all of Tomcat's use of CSS at once. |