Skip to content

Commit 23fc8b0

Browse files
seanmonstarseanmonstar
committed
fix(client): allow client GET requests with explicit body headers
Closes #1925
1 parent a1609fb commit 23fc8b0

File tree

2 files changed

+221
-45
lines changed

2 files changed

+221
-45
lines changed

‎src/proto/h1/role.rs

Lines changed: 48 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -781,31 +781,44 @@ impl Client {
781781

782782
impl Client {
783783
fn set_length(head: &mut RequestHead, body: Option<BodyLength>) -> Encoder {
784-
if let Some(body) = body {
785-
let can_chunked = head.version == Version::HTTP_11
786-
&& (head.subject.0 != Method::HEAD)
787-
&& (head.subject.0 != Method::GET)
788-
&& (head.subject.0 != Method::CONNECT);
789-
set_length(&mut head.headers, body, can_chunked)
784+
let body = if let Some(body) = body {
785+
body
790786
} else {
791787
head.headers.remove(header::TRANSFER_ENCODING);
792-
Encoder::length(0)
793-
}
794-
}
795-
}
788+
return Encoder::length(0)
789+
};
790+
791+
// HTTP/1.0 doesn't know about chunked
792+
let can_chunked = head.version == Version::HTTP_11;
793+
let headers = &mut head.headers;
794+
795+
// If the user already set specific headers, we should respect them, regardless
796+
// of what the Payload knows about itself. They set them for a reason.
796797

797-
fn set_length(headers: &mut HeaderMap, body: BodyLength, can_chunked: bool) -> Encoder {
798-
// If the user already set specific headers, we should respect them, regardless
799-
// of what the Payload knows about itself. They set them for a reason.
798+
// Because of the borrow checker, we can't check the for an existing
799+
// Content-Length header while holding an `Entry` for the Transfer-Encoding
800+
// header, so unfortunately, we must do the check here, first.
800801

801-
// Because of the borrow checker, we can't check the for an existing
802-
// Content-Length header while holding an `Entry` for the Transfer-Encoding
803-
// header, so unfortunately, we must do the check here, first.
802+
let existing_con_len = headers::content_length_parse_all(headers);
803+
let mut should_remove_con_len = false;
804804

805-
let existing_con_len = headers::content_length_parse_all(headers);
806-
let mut should_remove_con_len = false;
805+
if !can_chunked {
806+
// Chunked isn't legal, so if it is set, we need to remove it.
807+
if headers.remove(header::TRANSFER_ENCODING).is_some() {
808+
trace!("removing illegal transfer-encoding header");
809+
}
810+
811+
return if let Some(len) = existing_con_len {
812+
Encoder::length(len)
813+
} else if let BodyLength::Known(len) = body {
814+
set_content_length(headers, len)
815+
} else {
816+
// HTTP/1.0 client requests without a content-length
817+
// cannot have any body at all.
818+
Encoder::length(0)
819+
};
820+
}
807821

808-
if can_chunked {
809822
// If the user set a transfer-encoding, respect that. Let's just
810823
// make sure `chunked` is the final encoding.
811824
let encoder = match headers.entry(header::TRANSFER_ENCODING)
@@ -841,9 +854,22 @@ fn set_length(headers: &mut HeaderMap, body: BodyLength, can_chunked: bool) -> E
841854
if let Some(len) = existing_con_len {
842855
Some(Encoder::length(len))
843856
} else if let BodyLength::Unknown = body {
844-
should_remove_con_len = true;
845-
te.insert(HeaderValue::from_static("chunked"));
846-
Some(Encoder::chunked())
857+
// GET, HEAD, and CONNECT almost never have bodies.
858+
//
859+
// So instead of sending a "chunked" body with a 0-chunk,
860+
// assume no body here. If you *must* send a body,
861+
// set the headers explicitly.
862+
match head.subject.0 {
863+
Method::GET |
864+
Method::HEAD |
865+
Method::CONNECT => {
866+
Some(Encoder::length(0))
867+
},
868+
_ => {
869+
te.insert(HeaderValue::from_static("chunked"));
870+
Some(Encoder::chunked())
871+
},
872+
}
847873
} else {
848874
None
849875
}
@@ -869,27 +895,6 @@ fn set_length(headers: &mut HeaderMap, body: BodyLength, can_chunked: bool) -> E
869895
};
870896

871897
set_content_length(headers, len)
872-
} else {
873-
// Chunked isn't legal, so if it is set, we need to remove it.
874-
// Also, if it *is* set, then we shouldn't replace with a length,
875-
// since the user tried to imply there isn't a length.
876-
let encoder = if headers.remove(header::TRANSFER_ENCODING).is_some() {
877-
trace!("removing illegal transfer-encoding header");
878-
should_remove_con_len = true;
879-
Encoder::close_delimited()
880-
} else if let Some(len) = existing_con_len {
881-
Encoder::length(len)
882-
} else if let BodyLength::Known(len) = body {
883-
set_content_length(headers, len)
884-
} else {
885-
Encoder::close_delimited()
886-
};
887-
888-
if should_remove_con_len && existing_con_len.is_some() {
889-
headers.remove(header::CONTENT_LENGTH);
890-
}
891-
892-
encoder
893898
}
894899
}
895900

‎tests/client.rs

Lines changed: 173 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ test! {
356356
}
357357

358358
test! {
359-
name: client_get_implicitly_empty,
359+
name: client_get_req_body_implicitly_empty,
360360

361361
server:
362362
expected: "GET / HTTP/1.1\r\nhost: {addr}\r\n\r\n",
@@ -370,9 +370,153 @@ test! {
370370
},
371371
response:
372372
status: OK,
373+
headers: {},
374+
body: None,
375+
}
376+
377+
test! {
378+
name: client_get_req_body_chunked,
379+
380+
server:
381+
expected: "\
382+
GET / HTTP/1.1\r\n\
383+
transfer-encoding: chunked\r\n\
384+
host: {addr}\r\n\
385+
\r\n\
386+
5\r\n\
387+
hello\r\n\
388+
0\r\n\r\n\
389+
",
390+
reply: REPLY_OK,
391+
392+
client:
393+
request: {
394+
method: GET,
395+
url: "http://{addr}/",
373396
headers: {
374-
"Content-Length" => "0",
397+
"transfer-encoding" => "chunked",
398+
},
399+
body: "hello", // not Body::empty
400+
},
401+
response:
402+
status: OK,
403+
headers: {},
404+
body: None,
405+
}
406+
407+
test! {
408+
name: client_get_req_body_chunked_http10,
409+
410+
server:
411+
expected: "\
412+
GET / HTTP/1.0\r\n\
413+
host: {addr}\r\n\
414+
content-length: 5\r\n\
415+
\r\n\
416+
hello\
417+
",
418+
reply: "HTTP/1.0 200 OK\r\ncontent-length: 0\r\n\r\n",
419+
420+
client:
421+
request: {
422+
method: GET,
423+
url: "http://{addr}/",
424+
headers: {
425+
"transfer-encoding" => "chunked",
426+
},
427+
version: HTTP_10,
428+
body: "hello",
429+
},
430+
response:
431+
status: OK,
432+
headers: {},
433+
body: None,
434+
}
435+
436+
test! {
437+
name: client_get_req_body_sized,
438+
439+
server:
440+
expected: "\
441+
GET / HTTP/1.1\r\n\
442+
content-length: 5\r\n\
443+
host: {addr}\r\n\
444+
\r\n\
445+
hello\
446+
",
447+
reply: REPLY_OK,
448+
449+
client:
450+
request: {
451+
method: GET,
452+
url: "http://{addr}/",
453+
headers: {
454+
"Content-Length" => "5",
455+
},
456+
body: (Body::wrap_stream(Body::from("hello"))),
457+
},
458+
response:
459+
status: OK,
460+
headers: {},
461+
body: None,
462+
}
463+
464+
test! {
465+
name: client_get_req_body_unknown,
466+
467+
server:
468+
expected: "\
469+
GET / HTTP/1.1\r\n\
470+
host: {addr}\r\n\
471+
\r\n\
472+
",
473+
reply: REPLY_OK,
474+
475+
client:
476+
request: {
477+
method: GET,
478+
url: "http://{addr}/",
479+
// wrap_steam means we don't know the content-length,
480+
// but we're wrapping a non-empty stream.
481+
//
482+
// But since the headers cannot tell us, and the method typically
483+
// doesn't have a body, the body must be ignored.
484+
body: (Body::wrap_stream(Body::from("hello"))),
485+
},
486+
response:
487+
status: OK,
488+
headers: {},
489+
body: None,
490+
}
491+
492+
test! {
493+
name: client_get_req_body_unknown_http10,
494+
495+
server:
496+
expected: "\
497+
GET / HTTP/1.0\r\n\
498+
host: {addr}\r\n\
499+
\r\n\
500+
",
501+
reply: "HTTP/1.0 200 OK\r\ncontent-length: 0\r\n\r\n",
502+
503+
client:
504+
request: {
505+
method: GET,
506+
url: "http://{addr}/",
507+
headers: {
508+
"transfer-encoding" => "chunked",
375509
},
510+
version: HTTP_10,
511+
// wrap_steam means we don't know the content-length,
512+
// but we're wrapping a non-empty stream.
513+
//
514+
// But since the headers cannot tell us, the body must be ignored.
515+
body: (Body::wrap_stream(Body::from("hello"))),
516+
},
517+
response:
518+
status: OK,
519+
headers: {},
376520
body: None,
377521
}
378522

@@ -434,6 +578,33 @@ test! {
434578
body: None,
435579
}
436580

581+
test! {
582+
name: client_post_unknown,
583+
584+
server:
585+
expected: "\
586+
POST /chunks HTTP/1.1\r\n\
587+
host: {addr}\r\n\
588+
transfer-encoding: chunked\r\n\
589+
\r\n\
590+
B\r\n\
591+
foo bar baz\r\n\
592+
0\r\n\r\n\
593+
",
594+
reply: REPLY_OK,
595+
596+
client:
597+
request: {
598+
method: POST,
599+
url: "http://{addr}/chunks",
600+
body: (Body::wrap_stream(Body::from("foo bar baz"))),
601+
},
602+
response:
603+
status: OK,
604+
headers: {},
605+
body: None,
606+
}
607+
437608
test! {
438609
name: client_post_empty,
439610

0 commit comments

Comments
 (0)