Uh oh!
There was an error while loading. Please reload this page.
bitreq: Add RFC 9112 compliance test vectors - #660
Conversation
Exercise the HTTP/1.1 wire grammar so request generation and response parsing gaps are visible before behavior is changed. Co-Authored-By: HAL 9000
Cover response length and chunk decoding rules so incomplete or ambiguous messages cannot pass unnoticed. Co-Authored-By: HAL 9000
Cover persistence, closure, and pipelining rules so connection reuse can be corrected against explicit expectations. Refs: rust-bitcoin#659 Co-Authored-By: HAL 9000
| } | ||
| #[test] | ||
| #[ignore = "TODO: reject bare CR in request protocol elements (RFC 9112 Section 2.2)"] |
There was a problem hiding this comment.
Do we actually intend to fix all these? I'm happy to see them fixed if we care, but its not clear to me that we do/should, at which point adding tests for them is just noise.
There was a problem hiding this comment.
Yes, I think we should. #661 is the start, but I intend to tackle these bit-by-bit in follow-ups.
There was a problem hiding this comment.
FWIW I don't think we should bother trying to be super strict on what the calling code passes. In this case specifically where the request is invalid, I think we should just accept it. The only exception would be if the request is bad in a way that will break our request code, ie if it might make two requests because it adds a \r\n\r\nGET... or something similar. I think that basically just means rejecting \r\ns and maybe rejecting standalone \r and \ns
TheBlueMatt
left a comment
There was a problem hiding this comment.
I skimmed about the first half of these. I think most of them its not worth enforcing - stuff where the server or client are maybe a bit confused but we can parse the resulting stream fine I really don't think we should add code to be strict on the RFC for.
| } | ||
| #[test] | ||
| #[ignore = "TODO: reject bare CR in request protocol elements (RFC 9112 Section 2.2)"] |
There was a problem hiding this comment.
FWIW I don't think we should bother trying to be super strict on what the calling code passes. In this case specifically where the request is invalid, I think we should just accept it. The only exception would be if the request is bad in a way that will break our request code, ie if it might make two requests because it adds a \r\n\r\nGET... or something similar. I think that basically just means rejecting \r\ns and maybe rejecting standalone \r and \ns
| } | ||
| #[test] | ||
| #[ignore = "TODO: reject invalid request field names (RFC 9112 Sections 2.2 and 5)"] |
There was a problem hiding this comment.
I don't think we should bother rejecting this.
| } | ||
| #[test] | ||
| #[ignore = "TODO: reject conflicting Host fields (RFC 9112 Section 3.2)"] |
There was a problem hiding this comment.
I don't think we should worry about stuff like this.
| #[test] | ||
| #[ignore = "TODO: validate response status-line grammar (RFC 9112 Sections 2.3 and 4)"] | ||
| fn section_4_invalid_status_lines_are_rejected() { |
There was a problem hiding this comment.
I don't think we should enforce this.
| } | ||
| #[test] | ||
| #[ignore = "TODO: reject or ignore whitespace-prefixed fields (RFC 9112 Section 2.2)"] |
There was a problem hiding this comment.
I don't see why we should enforce this.
| #[test] | ||
| #[ignore = "TODO: reject repeated chunked transfer coding (RFC 9112 Section 6.1)"] | ||
| fn section_6_1_sender_rejects_repeated_chunked_coding() { | ||
| // RFC 9112 Section 6.1: a sender MUST NOT apply chunked transfer coding more than once. |
There was a problem hiding this comment.
I don't see why we should reject this.
| } | ||
| #[test] | ||
| #[ignore = "TODO: exclude chunked from TE requests (RFC 9112 Section 7.4)"] |
There was a problem hiding this comment.
Let's please not add parsing of random client headers just to enforce them.
| } | ||
| #[test] | ||
| #[ignore = "TODO: add the TE connection option when sending TE (RFC 9112 Section 7.4)"] |
| #[ignore = "TODO: treat successful CONNECT responses as tunnels (RFC 9112 Section 6.3)"] | ||
| fn section_6_3_successful_connect_ignores_framing_fields() { | ||
| // RFC 9112 Section 6.3: a client receiving a successful CONNECT response MUST ignore | ||
| // Content-Length and Transfer-Encoding because the connection becomes a tunnel. |
There was a problem hiding this comment.
Are we even going to support parsing this? Its not clear how to parse this if we're pipelining and if a proxy is this broken kinda who cares?
| #[ignore = "TODO: reject Transfer-Encoding in HTTP/1.0 responses (RFC 9112 Section 6.1)"] | ||
| fn section_6_1_http_1_0_transfer_encoding_is_faulty() { | ||
| // RFC 9112 Section 6.1: a client receiving Transfer-Encoding in HTTP/1.0 MUST treat framing as | ||
| // faulty and close the connection. |
There was a problem hiding this comment.
Why is it worth our time to enforce this?
Please note development has migrated to https://git.rust-bitcoin.org/rust-bitcoin/corepc. Any further comments or pushes here on github may be ignored or lost. Closing since I know @tnull is ok with the migration. |
In #659 we recently discovered that we aren't fully compliant with RFC 9112 / HTTP 1.1.
As a first step of mitigation, we here add some test vectors that cover the central points. We opted to mark the (33!) failing checks as
ignored, so we can revisit and fix them in following PRs (instead of cramming everything into one PR).And more minor: