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

Fix #8897 Ignore conditional headers as per RFC7232 #8899

Merged
merged 2 commits into from
Nov 17, 2022

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Nov 14, 2022

Ignore date based headers if etag ones are present. Also avoid parsing dates unless necessary.

Signed-off-by: Greg Wilkins [email protected]

Ignore date based headers if etag ones are present.
Also avoid parsing dates unless necessary.

Signed-off-by: Greg Wilkins <[email protected]>
@@ -604,10 +604,14 @@ protected boolean passConditionalHeaders(HttpServletRequest request, HttpServlet
}

// Parse the if[un]modified dates and compare to resource
if (ifums != -1 && content.getResource().lastModified() / 1000 > ifums / 1000)
if (ifums != null && ifm == null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the ResourceService (handling only GET and HEAD), the If-Unmodified-* headers are never going to be seen here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it "never" or just very VERY rarely?
I'm inclined to leave support in for 10/11 and remove support in 12.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to RFC9110, the If-Unmodified-Since header was for unsafe methods (like POST) and had no meaning for safe methods (like GET).

But now https://www.rfc-editor.org/rfc/rfc9110#name-if-unmodified-since says ..

A client MAY send an If-Unmodified-Since header field in a GET request to indicate that it would prefer a 412 (Precondition Failed) response if the selected representation has been modified. However, this is only useful in range requests (Section 14) for completing a previously received partial representation when there is no desire for a new representation. If-Range (Section 13.1.5) is better suited for range requests when the client prefers to receive a new representation.

So I'm actually thinking we should instead add back support in Jetty 12.

@gregw gregw requested review from joakime and lorban November 15, 2022 01:38
@@ -595,7 +595,7 @@ protected boolean passConditionalHeaders(HttpServletRequest request, HttpServlet
return false;
}

long ifmsl = request.getDateHeader(HttpHeader.IF_MODIFIED_SINCE.asString());
long ifmsl = DateParser.parseDate(ifms);
if (ifmsl != -1 && content.getResource().lastModified() / 1000 <= ifmsl / 1000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per https://www.rfc-editor.org/rfc/rfc9110#section-13.1.3-7

A recipient MUST ignore the If-Modified-Since header field if the resource does not have a modification date available.

The value of content.getResource().lastModified() should be checked to know if the resource has a modification date and skip this check if it has no modification date.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Check a resource has a lastModified date

Signed-off-by: Greg Wilkins <[email protected]>
@gregw gregw requested a review from joakime November 16, 2022 05:30
@gregw gregw merged commit 55e9f73 into jetty-10.0.x Nov 17, 2022
@gregw gregw deleted the jetty-10.0.x-8897-ConditionalHeadersIgnored branch November 17, 2022 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants