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

setRequestHeaderSize and setResponseHeaderSize apply to all headers, not one #6204

Closed
mikekap opened this issue Apr 21, 2021 · 10 comments
Closed
Assignees
Labels
Stale For auto-closed stale issues and pull requests

Comments

@mikekap
Copy link

mikekap commented Apr 21, 2021

Jetty version Jetty 9.4.31

Java version 14.0.2

OS type/version Mac OS

Description
This is either a bug with the documentation or the implementation. Theses are the comments in HTTPConfiguration.java: https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java#L401-L422 .

The documentation says that these control "the maximum size in bytes of the ... header." MDN defines a header as ONE key-value pair - i.e. just one Header: Value.

Unfortunately in Jetty, these configuration options actually control the size of the HTTP Header "Area" (not sure what to call it). Setting either of these controls the total length of ALL the (request/response) headers, not just the maximum of one header. I'm not sure if that's intentional, but if it is, it would be great to improve the documentation to say that it's the total header length, not a single header length.

@joakime
Copy link
Contributor

joakime commented Apr 21, 2021

It's a buffer configuration, it controls the buffer used for the header section of the HTTP protocol.
Not just a specific header.

Per RFC7230 (HTTP/1.1 spec), you have ...

  1. Start Line
  2. Header
  3. Message Body

Those configurations control the buffer used in section 2 above.

@mikekap
Copy link
Author

mikekap commented Apr 21, 2021

Right that's true - it's currently meant to be interpreted as the "protocol header". However that usage of just "header" conflicts with the usage of "http header" as most people understand it - even in the same file:
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java#L266
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java#L296

Even in RFC7230 you have the distinction:

All HTTP/1.1 messages consist of a start-line followed by a sequence
of octets in a format similar to the Internet Message Format
[RFC5322]: zero or more header fields (collectively referred to as
the "headers" or the "header section"), an empty line indicating the
end of the header section, and an optional message body.

@joakime
Copy link
Contributor

joakime commented Apr 21, 2021

The documentation says that these control "the maximum size in bytes of the ... header." MDN defines a header as ONE key-value pair - i.e. just one Header: Value.

That's actually a "Field".

Also, I would caution relying on MDN, as its information is not up to date and frequently stagnates.
Example: it's information on x-forwarding-* headers is incomplete and years out of date, so much so that there are now several dozen competing interpretations that are nearly all incompatible with each other, causing RFC7239 to be created (in 2014) to solve for this. It's references on how to handle IPv6 addresses/hosts is also referencing old recommendations that have been superceded years ago by ietf specs that bring up the flaw/issues with the old recommendations and now considers most of those old IPv6 recommendations invalid.

As for the naming in use by Jetty, think of it this way ...

  • "Header" is the high level section of the protocol, what the buffer we are talking about operates on. for requests that buffer controls the start line + headers, for responses that buffer controls the response line + headers.
  • "Headers" or "Header Section" is the generic name of the collection of fields also known as "HTTP Fields", which is what our class is called.
  • "Field" is the specific entry, which is what our class is called.
  • "Name" is the name of the entry.
  • "Value" is the 0...n values of the field belonging to the name.

I know it's nuanced, but lots of things in HTTP/1.x are nuanced like that.
(Never mind the intentional misspellings in the spec - eg: "Referer")

Take this from the HTTP/1.1 spec: Section 3 (yes, I know you copy/pasted this section too, I just decided to link it and include the section header so others in the future that come across this issue can read it for themselves) ...

3.  Message Format

   All HTTP/1.1 messages consist of a start-line followed by a sequence
   of octets in a format similar to the Internet Message Format
   [RFC5322]: zero or more header fields (collectively referred to as
   the "headers" or the "header section"), an empty line indicating the
   end of the header section, and an optional message body.

The use of "headers" (plural) is actually not used much within the HTTP/1.1 spec itself. (There's 5 hits, 1 is the use in section above, 3 are shorthand references to "message headers" IANA registry called "Permanent Message Header Field Names", and the last hit is in the index at the bottom to reference above).

For the purposes of the HttpConfiguration ...
In HTTP/1.x the "Header" buffer controls the buffer used in the parsing of the sections including the start-line/response-line, and the fields, but before the message body/trailers.
In HTTP/2 the "Header" buffer controls the parsing of the the HEADERS + CONTINUATION frames, which contains the request line + headers, but not the data or trailers.

Changing this to be "headers" plural is actually limiting the scope of the buffer configuration (as that name would exclude the start-line/response-line, which is part of this buffer configuration)

I feel we are consistent to both of the HTTP specs, their chosen naming, and the true scope of that configuration within Jetty.
But I'll leave this open for others to chime in.

@gregw
Copy link
Contributor

gregw commented Apr 21, 2021

The correct description is definitely "headers size" or something to that effect.
However, that Javadoc is not really very good anyway as it just talks about the ramifications without really saying what is being set. We could clarify it by saying that it is setting the maximum buffer size for the headers section of the request/response.

@mikekap
Copy link
Author

mikekap commented May 4, 2021

My sense is the intersection of the sets {websites that users of Jetty visit} and {websites that refer to the HTTP header in the protocol sense} is low. Though I'm happy to be proven otherwise - I thought MDN was fairly authoritative in terms of "enough people look at this" but of course it's not by any means authoritative in terms of current standard tracks. I doubt most users of Jetty read the HTTP spec though - that's why they use Jetty 😄.

@gregw that seems reasonable - anything that communicates "this is not the same as what Apache/Nginx/etc use to limit line length." Where would be a better place for this documentation? My first thought was Javadoc because that's how I got to the comment. Jetty suddenly started returning 5xxs from my requests and I just followed the code to how Jetty is configured. Happy to send PRs to update all the places.

@gregw
Copy link
Contributor

gregw commented May 4, 2021

I think we should just improve the javadoc to say it is a buffer size for all the header fields.

@github-actions
Copy link

github-actions bot commented May 5, 2022

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label May 5, 2022
@sbordet sbordet removed the Stale For auto-closed stale issues and pull requests label May 10, 2022
@github-actions
Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label May 11, 2023
@sbordet sbordet removed the Stale For auto-closed stale issues and pull requests label May 13, 2023
@sbordet sbordet self-assigned this May 13, 2023
Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label May 13, 2024
Copy link

This issue has been closed due to it having no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale For auto-closed stale issues and pull requests
Projects
None yet
Development

No branches or pull requests

4 participants