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

Http upgrades #203

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
d73822b
refactor request queue mechanics
dpatti Jan 13, 2020
94e4480
refactor-request-queue: read loop no longer needs to wake writer
dpatti May 16, 2020
cce55fd
refactor-request-queue: fixes
dpatti Apr 2, 2021
0bab909
Implement HTTP upgrades.
dhouse-js Apr 26, 2021
b761c7e
Implement HTTP upgrades.
dhouse-js Apr 26, 2021
a49ea3c
Merge remote-tracking branch 'upstream/refactor-request-queue' into r…
dhouse-js Apr 27, 2021
949d7d9
Merge branch 'refactor-request-queue' into http-upgrades
dhouse-js Apr 27, 2021
124eed8
Fix requests where the client requests an upgrade but the server decl…
dhouse-js Apr 29, 2021
004e128
refactor
dhouse-js Apr 29, 2021
d1ff221
More fixes for when the upgrade request is declined
dhouse-js Apr 29, 2021
60f137b
Unwind changes to parser
dhouse-js Apr 30, 2021
2a36491
Merge branch 'master' into http-upgrades
dhouse-js May 24, 2021
d45b85c
Fix compilation of examples
dhouse-js May 24, 2021
05c797d
Implement HTTP upgrades.
dhouse-js Apr 26, 2021
e6cda20
Implement HTTP upgrades.
dhouse-js Apr 26, 2021
9ccf77b
Fix requests where the client requests an upgrade but the server decl…
dhouse-js Apr 29, 2021
40ae33c
refactor
dhouse-js Apr 29, 2021
5fcb69a
More fixes for when the upgrade request is declined
dhouse-js Apr 29, 2021
681d363
Unwind changes to parser
dhouse-js Apr 30, 2021
90de89e
Fix compilation of examples
dhouse-js May 24, 2021
203f06c
update reader coercion in client tests
dpatti Jun 3, 2021
cee330c
http-upgrades: require no request body
dpatti Jun 5, 2021
81b4808
http-upgrades: refactor input_state logic
dpatti Jun 5, 2021
9faf49c
http-upgrades: add tests and fix read loop issue
dpatti Jun 5, 2021
a978744
resolve conflicts
dhouse-js Jun 7, 2021
6167c51
merge with master
dhouse-js Jun 7, 2021
d3377dd
fix build
dhouse-js Jun 7, 2021
5264434
improve comment
dhouse-js Jun 7, 2021
052bdcf
fix conflicts
dhouse-js Jun 21, 2021
31a6c1a
minor refactoring
dpatti Jan 12, 2022
0c2ede1
expose is_upgrade and fix up documentation
dpatti Jan 12, 2022
0da6f10
Merge remote-tracking branch 'origin/master' into http-upgrades
dpatti Jan 17, 2022
1384050
fix httpaf-async upgrade implementation
dpatti Jan 17, 2022
5294105
fix lwt implementation and add upgrade helper
dpatti Mar 7, 2022
c7eeda5
switch async upgrade to pass the socket
dpatti Mar 7, 2022
71e0486
fix lwt_echo_upgrade example
dpatti Mar 12, 2022
4944d00
move reader yields to separate function
dpatti Jun 22, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix requests where the client requests an upgrade but the server decl…
…ines.

In these cases, the correct behaviour is to send the response back to
the client and then close the connection. The client may have sent us
bytes after the request header which are not HTTP, so we have no hope
of recovering this connection.

This is achieved by modifying the parser to stop if it detects that
the client has requested an upgrade. This ensures that, regardless of
whether or not [Reqd.input_state] is [Upgrade], the parser will not
attempt to consume bytes that might not be HTTP.
dhouse-js authored and dpatti committed Jun 3, 2021
commit 9ccf77b724716ec3eab51cf55c167411ac78081d
2 changes: 1 addition & 1 deletion lib/message.ml
Original file line number Diff line number Diff line change
@@ -41,7 +41,7 @@ let persistent_connection ?(proxy=false) version headers =
(* XXX(seliopou): use proxy argument in the case of HTTP/1.0 as per
https://tools.ietf.org/html/rfc7230#section-6.3 *)
match Headers.get headers "connection" with
| Some "close" -> false
| Some ("close" | "upgrade") -> false
| Some "keep-alive" -> Version.(compare version v1_0) >= 0
| _ -> Version.(compare version v1_1) >= 0

47 changes: 28 additions & 19 deletions lib/parse.ml
Original file line number Diff line number Diff line change
@@ -212,13 +212,22 @@ module Reader = struct
| `Invalid_response_body_length of Response.t
| `Parse of string list * string ]

type parser_result =
| Can_continue
| Stop

type 'error parse_state =
| Done
| Done of parser_result
| Fail of 'error
| Partial of (Bigstringaf.t -> off:int -> len:int -> AU.more -> (unit, 'error) result AU.state)
| Partial of
(Bigstringaf.t
-> off:int
-> len:int
-> AU.more
-> (parser_result, 'error) result AU.state)

type 'error t =
{ parser : (unit, 'error) result Angstrom.t
{ parser : (parser_result, 'error) result Angstrom.t
; mutable parse_state : 'error parse_state
(* The state of the parse for the current request *)
; mutable closed : bool
@@ -231,24 +240,25 @@ module Reader = struct

let create parser =
{ parser
; parse_state = Done
; parse_state = Done Can_continue
; closed = false
}

let ok = return (Ok ())

let request handler =
let parser =
request <* commit >>= fun request ->
match Request.body_length request with
| `Error `Bad_request -> return (Error (`Bad_request request))
| `Error `Bad_request ->
return (Error (`Bad_request request))
| `Fixed 0L ->
handler request Body.empty;
ok
(* If the client has requested an upgrade, then any bytes after the headers are
likely not HTTP, so we should be careful not to try to parse them. *)
return (Ok (if Request.is_upgrade request then Stop else Can_continue))
| `Fixed _ | `Chunked as encoding ->
let request_body = Body.create_reader Bigstringaf.empty in
handler request request_body;
body ~encoding request_body *> ok
body ~encoding request_body *> return (Ok Can_continue)
in
create parser

@@ -261,13 +271,13 @@ module Reader = struct
| `Error `Internal_server_error -> return (Error (`Invalid_response_body_length response))
| `Fixed 0L ->
handler response Body.empty;
ok
return (Ok Can_continue)
| `Fixed _ | `Chunked | `Close_delimited as encoding ->
(* We do not trust the length provided in the [`Fixed] case, as the
client could DOS easily. *)
let response_body = Body.create_reader Bigstringaf.empty in
handler response response_body;
body ~encoding response_body *> ok
body ~encoding response_body *> return (Ok Can_continue)
in
create parser
;;
@@ -278,8 +288,8 @@ module Reader = struct

let transition t state =
match state with
| AU.Done(consumed, Ok ()) ->
t.parse_state <- Done;
| AU.Done(consumed, Ok result) ->
t.parse_state <- Done result;
consumed
| AU.Done(consumed, Error error) ->
t.parse_state <- Fail error;
@@ -303,8 +313,8 @@ module Reader = struct
let rec read_with_more t bs ~off ~len more =
let consumed =
match t.parse_state with
| Fail _ -> 0
| Done ->
| Fail _ | Done Stop -> 0
| Done Can_continue ->
start t (AU.parse t.parser);
read_with_more t bs ~off ~len more;
| Partial continue ->
@@ -324,11 +334,10 @@ module Reader = struct
let next t =
if t.closed
then `Close
else (
else
match t.parse_state with
| Fail err -> `Error err
| Done -> `Read
| Partial _ -> `Read
)
| Done Stop -> `Close
| Done Can_continue | Partial _ -> `Read
;;
end
5 changes: 5 additions & 0 deletions lib/request.ml
Original file line number Diff line number Diff line change
@@ -80,3 +80,8 @@ let persistent_connection ?proxy { version; headers; _ } =
let pp_hum fmt { meth; target; version; headers } =
Format.fprintf fmt "((method \"%a\") (target %S) (version \"%a\") (headers %a))"
Method.pp_hum meth target Version.pp_hum version Headers.pp_hum headers

let is_upgrade t =
match Headers.get t.headers "Connection" with
| None -> false
| Some header_val -> Headers.ci_equal header_val "upgrade"
2 changes: 1 addition & 1 deletion lib/server_connection.ml
Original file line number Diff line number Diff line change
@@ -281,7 +281,7 @@ let rec _next_write_operation t =
(* Even in the Upgrade case, we're still responsible for writing the response
header, so we might have work to do. *)
Writer.next t.writer
else _final_write_operation_for t reqd
else `Upgrade
)

and _final_write_operation_for t reqd =
18 changes: 17 additions & 1 deletion lib_test/test_server_connection.ml
Original file line number Diff line number Diff line change
@@ -950,7 +950,7 @@ let test_shutdown_during_asynchronous_request () =
;;

let test_upgrade () =
let upgrade_headers =["Connection", "Upgrade" ; "Upgrade", "foo"] in
let upgrade_headers =["Connection", "upgrade" ; "Upgrade", "foo"] in
let request_handler reqd =
Reqd.respond_with_upgrade reqd
(Headers.of_list upgrade_headers)
@@ -965,6 +965,21 @@ let test_upgrade () =
Alcotest.check write_operation "Writer is `Upgrade" `Upgrade (current_write_operation t);
;;

let test_upgrade_where_server_does_not_upgrade () =
let upgrade_headers =["Connection", "upgrade" ; "Upgrade", "foo"] in
let response = Response.create `Bad_request ~headers:(Headers.of_list upgrade_headers) in
let request_handler reqd =
Reqd.respond_with_string reqd response ""
in
let t = create request_handler in
read_request t
(Request.create `GET "/"
~headers:(Headers.of_list (("Content-Length", "0") :: upgrade_headers)));
Alcotest.check read_operation "Reader is `Close" `Close (current_read_operation t);
write_response t response;
Alcotest.check write_operation "Writer is `Close" (`Close 0) (current_write_operation t);
;;

let tests =
[ "initial reader state" , `Quick, test_initial_reader_state
; "shutdown reader closed", `Quick, test_reader_is_closed_after_eof
@@ -997,4 +1012,5 @@ let tests =
; "shutdown in request handler", `Quick, test_shutdown_in_request_handler
; "shutdown during asynchronous request", `Quick, test_shutdown_during_asynchronous_request
; "test upgrades", `Quick, test_upgrade
; "test upgrade where server does not upgrade", `Quick, test_upgrade_where_server_does_not_upgrade
]