Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jetty doesn't set the date header on error responses #9685

Closed
markslater opened this issue Apr 27, 2023 · 4 comments · Fixed by #9687 or #9719
Closed

Jetty doesn't set the date header on error responses #9685

markslater opened this issue Apr 27, 2023 · 4 comments · Fixed by #9687 or #9719
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@markslater
Copy link
Contributor

Jetty version(s)
11.0.15

Java version/vendor (use: java -version)
openjdk version "11.0.18" 2023-01-17
OpenJDK Runtime Environment (build 11.0.18+10-post-Ubuntu-0ubuntu120.04.1)
OpenJDK 64-Bit Server VM (build 11.0.18+10-post-Ubuntu-0ubuntu120.04.1, mixed mode, sharing)

OS type/version

Linux 5.4.0-147-generic

Description

Jetty's default error responses don't include the date header, but RFC 9110 specifies that it's mandatory for an "origin server with a clock".

How to reproduce?

Run the following class:

package com.example;

import org.eclipse.jetty.server.Server;

public class ErrorServer {
    public static void main(String[] args) throws Exception {
        new Server(8080).start();
    }
}

Navigate to http://localhost:8080 in a browser, and view the response headers. I get:

Cache-Control: must-revalidate,no-cache,no-store
Content-Length: 435
Content-Type: text/html;charset=iso-8859-1
Server: Jetty(11.0.15)

This appears to be caused by a call to Response.resetContent() in the default error handling flow, which erases the date header that had previously been set by HttpChannel.onRequest(MetaData.Request).

@markslater markslater added the Bug For general bugs on Jetty side label Apr 27, 2023
@gregw gregw self-assigned this Apr 28, 2023
@gregw
Copy link
Contributor

gregw commented Apr 28, 2023

@markslater your analysis of the cause is indeed correct. We've been aware for a while that resetContent does reset headers that the server itself has generated.... but we'd not acted because it was not clear if that really was the intended semantic of reset or not. However, as you say, RFC9110 makes it pretty clear that we should be including at least the Date header.

Now we have a choice between:

  1. preserving it as it was (but an app may have modified it)
  2. re-adding server headers in reset.
  3. changing to only generate the server headers during commit.

I think 2. is least change and probably the correct semantic for when to set the date header.

@sbordet thoughts?

@gregw
Copy link
Contributor

gregw commented Apr 28, 2023

Ah, we had already changed resetContent to only reset specific headers:

                case CONTENT_LENGTH:
                case CONTENT_ENCODING:
                case CONTENT_LANGUAGE:
                case CONTENT_RANGE:
                case CONTENT_MD5:
                case CONTENT_LOCATION:
                case TRANSFER_ENCODING:
                case CACHE_CONTROL:
                case LAST_MODIFIED:
                case EXPIRES:
                case ETAG:
                case DATE:
                case VARY:

So why is DATE considered a content header? We should definitely take it out of the list.

I think that would be enough. I'm not sure we need to worry about applications that deliberately remove the Date header.

gregw added a commit that referenced this issue Apr 29, 2023
resetContent does not reset Date Header

Signed-off-by: gregw <[email protected]>
gregw added a commit that referenced this issue Apr 29, 2023
resetContent does not reset Date Header

Signed-off-by: gregw <[email protected]>
@gregw
Copy link
Contributor

gregw commented Apr 29, 2023

This has been fixed in jetty 10,11 and the ee environments of 12.

However, in the core APIs, there is only a response reset method that removes all headers. We probably need to modify that to restore (or not remove) any server provided headers.

@gregw gregw reopened this Apr 29, 2023
gregw added a commit that referenced this issue May 1, 2023
Merging #9685 to 12 broke the build as it turned out that the error handling was depending on the attempt to remove the Date header to detect if the response was already committed during a sendError.   This fix makes the test for committed explicit, it also fixes which exception is passed to abort.
gregw added a commit that referenced this issue May 1, 2023
sendError checks for committed and throws ISE (as per spec javadoc)
gregw added a commit that referenced this issue May 1, 2023
Merging #9685 to 12 broke the build as it turned out that the error handling was depending on the attempt to remove the Date header to detect if the response was already committed during a sendError.
@gregw gregw linked a pull request May 2, 2023 that will close this issue
gregw added a commit that referenced this issue May 3, 2023
* Issue #9648 ServletApiResponse.sendError must check if already committed.

* Fix #9685 merge to 12

Merging #9685 to 12 broke the build as it turned out that the error handling was depending on the attempt to remove the Date header to detect if the response was already committed during a sendError.   This fix makes the test for committed explicit, it also fixes which exception is passed to abort.

* Fix #9685 merge to 12

Merging #9685 to 12 broke the build as it turned out that the error handling was depending on the attempt to remove the Date header to detect if the response was already committed during a sendError.

---------

Co-authored-by: Jan Bartel <[email protected]>
@joakime
Copy link
Contributor

joakime commented May 10, 2023

Closing as it is merged

@joakime joakime closed this as completed May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
3 participants