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

Issue #11292 - Missing Date Response Header. #11293

Merged
merged 2 commits into from Jan 25, 2024

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Jan 19, 2024

  • Revert changes to boolean values in HttpConfiguration from commit 103e1d9
  • Add Unit Tests for Date Response header in ee10 and DistributionTests

Fixes: #11292

* Revert changes to boolean values in HttpConfiguration from commit 103e1d9
* Add Unit Tests for Date Response header
  in ee10 and DistributionTests
@joakime joakime added the Bug For general bugs on Jetty side label Jan 19, 2024
@joakime joakime added this to the 12.0.x milestone Jan 19, 2024
@joakime joakime self-assigned this Jan 19, 2024
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

There's no need to change from <Property> to the attribute property, and would change the default behaviour of jetty. The <Property> element allows a default value to be specified, whereas using the attribute property means that the setter won't be called unless a value is specified for the property., leaving the default value up to the code instead. As the server.mod has commented out value for sendDateHeader property, the default will revert to the code, which is true. Usng the <Property> element with a default of false means it defaults to false, which seems to have been the case for a long time time, at least from jetty-10 onwards.

So I wouldn't change the xml, but a unit test might be useful to ensure that setting the property values is working as expected.

@joakime
Copy link
Contributor Author

joakime commented Jan 20, 2024

There's no need to change from <Property> to the attribute property, and would change the default behaviour of jetty. The <Property> element allows a default value to be specified, whereas using the attribute property means that the setter won't be called unless a value is specified for the property., leaving the default value up to the code instead.

This is exactly what we want.
All of the other properties in that file are also using the code default, not the XML default.
The ini sections in our mod files have commented out properties and are set to the code defaults.

As the server.mod has commented out value for sendDateHeader property, the default will revert to the code, which is true. Usng the <Property> element with a default of false means it defaults to false, which seems to have been the case for a long time time, at least from jetty-10 onwards.

This means there is now a difference in default behavior between standalone and embedded.
Something we never want.

The XML and mod+ini sections should match the code defaults (like all of the other defaults on all of the other XML / INI+MOD sections)
I'll have to update my ini/mod/xml/property scanning tool on monday to check this more thoroughly.

So I wouldn't change the xml, but a unit test might be useful to ensure that setting the property values is working as expected.

The setSendDateHeader is default true in code and the commented out property.
Jetty versions 10.0.0 thru 10.0.4 have this configuration as true.
You changed it in commit 103e1d9 (for Jetty 10.0.5)

The older <Set name=""><Property name="" default=""></Set> techniques are undesired, that's why we have the newer <Set name="" property="">, as the older mechanism is subject to this mismatch between the code and the XML. Don't even call the setter if the property is undefined, use code defaults.

@janbartel
Copy link
Contributor

There's no need to change from <Property> to the attribute property, and would change the default behaviour of jetty. The <Property> element allows a default value to be specified, whereas using the attribute property means that the setter won't be called unless a value is specified for the property., leaving the default value up to the code instead.

This is exactly what we want. All of the other properties in that file are also using the code default, not the XML default. The ini sections in our mod files have commented out properties and are set to the code defaults.

Yes, however when I reviewed the xml, the property settings and the code defaults in #6348 it was apparent that some property defaults had been different than the code defaults, so we decided to retain that behaviour, and not change to the code defaults. Changing now to default to true will mean that every distro user will suddenly have the Date header being sent and I think we'll get a lot of questions about that. However, it would be nice to make the embedded usage == to the distro usage. @gregw what are your thoughts?

@gregw
Copy link
Contributor

gregw commented Jan 20, 2024

I've not drilled down on this in detail....
But in general we do have a few defaults that are different for embedded usage vs xml usage, so this is not unique.
I don't think we can change this without breaking half our users... either embedded or xml based ones will see a change if we change this.

I recall looking at this a while ago, as the RFC says that we should always send a Date header, so we are being a little bad by having the default false (can't remember which half of the users it is false for). At the time, I wanted to change to having the default true everywhere.... but then decided we didn't want to make a change that affected half our users.

Ultimately, I think this is a case of if it is not really badly broken, then let's not fix it.

@gregw
Copy link
Contributor

gregw commented Jan 20, 2024

OK, I've looked a bit deeper now and it looks like we had a bit of a long discussion about this 3 years ago in #6348. Is there any reason to re litigate the rough consensus that was determined then?

Although I do recall a recent security issue that suggested we change default for allowing relative redirection, as that can prevent internal details from being disclosed in some situations. So that's the type of thing that should provoke a re-review of #6348.

In the case of sending Date headers, I think the embedded code does that because that is most correct by the RFCs... but the distro doesn't because standards be damned, lets run fast!
I guess we could(should?) revisit this default in 12.1.0, but not in a 12.0.x

As for the original issue, I don't see how the default can affect the behavior with regards to a host header?

@joakime
Copy link
Contributor Author

joakime commented Jan 20, 2024

As for the original issue, I don't see how the default can affect the behavior with regards to a host header?

Yeah! this part of the issue is still a total mystery.

@joakime joakime merged commit b571c6a into jetty-12.0.x Jan 25, 2024
8 checks passed
@joakime joakime deleted the fix/12.0.x/response-date-header-missing branch January 25, 2024 13:42
@chiqiu
Copy link

chiqiu commented Mar 28, 2024

In the jetty.xml , in the httpconfiguration, sendDateHeader default value needs to be set to true

@sbordet
Copy link
Contributor

sbordet commented Mar 28, 2024

In the jetty.xml , in the httpconfiguration, sendDateHeader default value needs to be set to true

Why? Do you have authoritative references that require so?

@joakime
Copy link
Contributor Author

joakime commented Mar 28, 2024

@sbordet https://datatracker.ietf.org/doc/html/rfc9110#section-6.6.1

An origin server with a clock (as defined in Section 5.6.7) MUST generate a Date header field in all 2xx (Successful), 3xx (Redirection), and 4xx (Client Error) responses, and MAY generate a Date header field in 1xx (Informational) and 5xx (Server Error) responses.

An origin server without a clock MUST NOT generate a Date header field.

@sbordet
Copy link
Contributor

sbordet commented Mar 28, 2024

In jetty.xml, sendDateHeader was true, then was removed in e09d401, and then added back as false in c302a40.

I'm fine restoring whatever the code has, with the new syntax of using the property attribute.

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
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Jetty12 doesn't set the Date response header when using localhost
5 participants