[Bug 63905] New: ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 63905] New: ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

            Bug ID: 63905
           Summary: ErrorReportValve adds CSS even if both showReport and
                    showServerInfo are set to false
           Product: Tomcat 8
           Version: 8.5.x-trunk
          Hardware: All
                OS: All
            Status: NEW
          Severity: minor
          Priority: P2
         Component: Catalina
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ----

According to the documentation for ErrorReportValve
(https://tomcat.apache.org/tomcat-8.5-doc/config/valve.html#Error_Report_Valve),
disabling both showServerInfo and showReport will only return the HTTP status
code and remove all CSS, however after the fix for 60490
(https://bz.apache.org/bugzilla/show_bug.cgi?id=60490) this is no longer the
case and the error page returns responses like this:

<!doctype html><html lang="en"><head><title>HTTP Status 400 – Bad
Request</title><style type="text/css">h1
{font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:22px;}
h2
{font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:16px;}
h3
{font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;font-size:14px;}
body {font-family:Tahoma,Arial,sans-serif;color:black;background-color:white;}
b {font-family:Tahoma,Arial,sans-serif;color:white;background-color:#525D76;} p
{font-family:Tahoma,Arial,sans-serif;background:white;color:black;font-size:12px;}
a {color:black;} a.name {color:black;} .line
{height:1px;background-color:#525D76;border:none;}</style></head><body><h1>HTTP
Status 400 – Bad Request</h1></body></html>

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

Christopher Schultz <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |Beginner

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

Michael Osipov <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #1 from Michael Osipov <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #2 from Mark Thomas <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #3 from Michael Osipov <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #4 from Christopher Schultz <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #5 from Mark Thomas <[hidden email]> ---
(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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #6 from Michael Osipov <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #7 from Michael Osipov <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #8 from Remy Maucherat <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #9 from Michael Osipov <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #10 from Christopher Schultz <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #11 from Remy Maucherat <[hidden email]> ---
(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 ?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #12 from Mark Thomas <[hidden email]> ---
Probably not. The argument against it was made in bug 56383. I'm not convinced.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #13 from Michael Osipov <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #14 from Christopher Schultz <[hidden email]> ---
(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

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #15 from Michael Osipov <[hidden email]> ---
(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!

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

Michael Osipov <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #16 from Michael Osipov <[hidden email]> ---
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.

[hidden email], 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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #17 from Christopher Schultz <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 63905] ErrorReportValve adds CSS even if both showReport and showServerInfo are set to false

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=63905

--- Comment #18 from Michael Osipov <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]