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

Cohttp: ensure "host" is the first header in requests and other fixes #986

Merged
merged 9 commits into from
Jul 6, 2023

Conversation

mseri
Copy link
Collaborator

@mseri mseri commented Jun 30, 2023

In addition fixes the test for null-header, which on releases would fail with

ASSERT null content-length header are sent
FAIL null content-length header are sent

   Expected: `"PUT / HTTP/1.1\r\nhost: someuri.com\r\nuser-agent: ocaml-cohttp/\r\ncontent-length: 0\r\n\r\n"'

   Received: `"PUT / HTTP/1.1\r\nhost: someuri.com\r\nuser-agent: ocaml-cohttp/v6.0.0_alpha2\r\ncontent-length: 0\r\n\r\n"'

Raised at Alcotest_engine__Test.check in file "src/alcotest-engine/test.ml", line 200, characters 4-261
Called from Alcotest_engine__Core.Make.protect_test.(fun) in file "src/alcotest-engine/core.ml", line 181, characters 17-23
Called from Alcotest_engine__Monad.Identity.catch in file "src/alcotest-engine/monad.ml", line 24, characters 31-35

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
Followup to mirage#939

From https://www.rfc-editor.org/rfc/rfc9110#section-7.2

    A user agent that sends Host SHOULD send it as the first field in the header section of a request. For example, a GET
    request to the origin server for http://www.example.org/pub/WWW/ would begin with:

    GET /pub/WWW/ HTTP/1.1
    Host: www.example.org

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
The headers are dealt with in reverse order for speed and reversed
once converted back to list. This means that to move an element to
the top of the headers we need to attach it to the bottom of the
list.  The  private method has been mofied accordingly.

The bug was spotted when fixing the null-header test to address the
lack of version in the user-agent header for development versions
of cohttp

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
To guarantee iteration is done following the real header order

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
this guarantees that the correct order is followed.

The commit includes the corrected test output

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@mseri mseri requested a review from rgrinberg June 30, 2023 15:57
Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@mseri
Copy link
Collaborator Author

mseri commented Jun 30, 2023

I would like to draft a new alpha release with these fixes, before we merge #984

Signed-off-by: Marcello Seri <marcello.seri@gmail.com>
@avsm
Copy link
Member

avsm commented Jul 5, 2023

I'm ok with fixing the issue like this, but a simpler fix would be to just track the host header separately in Request outside of the Header module, and not introduce ordering constraints into Header itself. The host header is special and no other cases like it exist, do they?

@mseri
Copy link
Collaborator Author

mseri commented Jul 5, 2023

The header fix (baea762) is only affecting the Request module of cohttp (and fixing the incorrect use in cohttp-eio header writer f317820).

The change in Header is to introduce an API that has predictable iteration behaviour over an header list and document the behaviour (23a7574), but nothing there knows nothing about host.

@mseri mseri merged commit de39002 into mirage:master Jul 6, 2023
17 of 19 checks passed
mseri added a commit to mseri/opam-repository that referenced this pull request Aug 8, 2023
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha2)

CHANGES:

- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995)
- http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986)
- http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986)
- do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985)
- cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976)
- cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982)
- cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
mseri added a commit to mseri/opam-repository that referenced this pull request Aug 8, 2023
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async and cohttp-async (6.0.0~alpha2)

CHANGES:

- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995)
- http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986)
- http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986)
- do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985)
- cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976)
- cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982)
- cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async and cohttp-async (6.0.0~alpha2)

CHANGES:

- cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995)
- http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986)
- http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986)
- do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985)
- cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976)
- cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982)
- cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
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.

None yet

2 participants