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

Honor H2 request limits #3892

Closed
wants to merge 2 commits into from
Closed

Conversation

walid-git
Copy link
Member

This PR honors h2 request limits for http_req_size, http_req_hdr_len, and h2_max_header_list_size
Fixes #3709

Currently, varnish is not honoring request limits for h2. This commit checks that the total uncompressed headers size
does not exceed http_req_size or h2_max_header_list_size, otherwise it responds with HTTP 431 and resets the stream
with ENHANCE_YOUR_CALM error.
@nigoroll
Copy link
Member

bugwash:

  • please improve test coverage - even without this patch it was not too good
  • please test with an actual browser
  • do we need to reconsider parameter defaults?
  • test on a live server if possible

Honor http_req_hdr_len by checking the size of each individual header field. In case a header exceeds the limit, we make sure
to process all the headers before failing the stream to keep a consistent HPACK table state for the connection.
@walid-git walid-git force-pushed the h2_req_limits branch 2 times, most recently from 3255f13 to 9a73b57 Compare May 29, 2023 13:01
@Dridi Dridi closed this in 7ccffe7 Apr 4, 2024
Dridi added a commit that referenced this pull request Apr 4, 2024
Dridi added a commit that referenced this pull request Apr 4, 2024
Dridi added a commit that referenced this pull request Apr 4, 2024
Refs #3709
Refs #3892

Conflicts:
	bin/varnishd/http2/cache_http2_hpack.c
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.

h2: Currently not honoring request limits
2 participants