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

Consecutive "/" at the beginning of a path causes an H2 protocol error. #3911

Closed
xcir opened this issue Apr 1, 2023 · 0 comments · Fixed by #3913
Closed

Consecutive "/" at the beginning of a path causes an H2 protocol error. #3911

xcir opened this issue Apr 1, 2023 · 0 comments · Fixed by #3913

Comments

@xcir
Copy link
Contributor

xcir commented Apr 1, 2023

Expected Behavior

If "/" is consecutively at the beginning of a path, it does not cause an error.

Current Behavior

22f666f

In this commit, if the first two characters of the path header are "//", an H2 protocol error will now occur.

Possible Solution

I believe that the "/" check on the second character should be removed.

https://github.com/varnishcache/varnish-cache/blob/master/bin/varnishd/http2/cache_http2_hpack.c#L139-L140

139,140c139
<                       if (((len > 0 && *b != '/') ||
<                           (len > 1 && *(b+1) == '/')) &&
---
>                       if ((len > 0 && *b != '/') &&

Steps to Reproduce (for bugs)

  1. Access with a path starting with "//" at http2 ( https://varnish-cache.org//index.html etc..)

Context

RFC7540#8.1.2.3 in the comment says "the "path-absolute" production and optionally~", which has been corrected to "absolute-path" in the revised RFC9113#8.3.1.

absolute-path is defined in RFC9110#4.1 and "//" does not appear to be prohibited.

This modification was made in

httpwg/http2-spec#906
httpwg/http2-spec#910

It is discussed in the above issue, but states that it was an unintentional mistake to prohibit "//".
Therefore, I believe it is a mistake to assume it is a protocol error.

However, multiple slashes are not desirable and it would be nice and kind to include them in the documentation. (or default.vcl)

Your Environment

  • Version used: 7.3.0
  • Operating System and version: Ubuntu 22.04
daghf added a commit to daghf/varnish-cache that referenced this issue Apr 3, 2023
This requirement was dropped in the updated rfc 9113.

Fixes: varnishcache#3911
daghf added a commit to daghf/varnish-cache that referenced this issue Apr 4, 2023
This requirement was dropped in the updated rfc 9113.

Fixes: varnishcache#3911
Dridi pushed a commit to Dridi/varnish-cache that referenced this issue Apr 20, 2023
This requirement was dropped in the updated rfc 9113.

Fixes: varnishcache#3911
nigoroll pushed a commit that referenced this issue Apr 24, 2023
This requirement was dropped in the updated rfc 9113.

Fixes: #3911
Dridi pushed a commit that referenced this issue Oct 25, 2023
This requirement was dropped in the updated rfc 9113.

Fixes: #3911
Dridi pushed a commit that referenced this issue Apr 4, 2024
This requirement was dropped in the updated rfc 9113.

Fixes: #3911
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 a pull request may close this issue.

1 participant