Bare carriage returns in header line endings are still forwarded when separated from the CRLF by whitespace
- Dominant language
- C++
- Stars
- 2k
- Forks
- 874
- Avg merge
- 6d 15h
- Merged PRs (30d)
- 46
Description
Follow-up to #12195 and #13595.
#13595 stops the raw input line from being preserved when the byte immediately before the terminating CRLF is a carriage return. That covers the line reported in #12195, but not the general case.
`mime_parser_parse` trims the field value with `field_value.rtrim_if(&ParseRules::is_wslfcr)`, which strips SP and HT as well as CR and LF. So a bare CR only has to be separated from the CRLF by one space or tab to end up back in the raw-print pad and be forwarded unchanged.
Built on master with #13595 applied, output captured from `HTTPHdr::print`:
| request header line | serialized output |
|---|---|
| `Extra-CRs: \r\r\r\r\n` | `Extra-CRs: \r\n` (fixed by #13595) |
| `Extra-CRs: \r \r\n` | `Extra-CRs: \r \r\n` |
| `Extra-CRs: \r\r \r\n` | `Extra-CRs: \r\r \r\n` |
| `Extra-CRs: bar\r \r\n` | `Extra-CRs: bar\r \r\n` |
| `Extra-CRs: \r\t\r\n` | `Extra-CRs: \r\t\r\n` |
RFC 9112 section 2.2 says a recipient of a bare CR must either consider the element invalid or replace each bare CR with SP before processing or forwarding the message. The last four rows do neither.
A positional check will keep missing spellings of this. Making it structural works and keeps the fast path: once the value is trimmed, require that everything between the end of the value and the end of the line is optional whitespace followed by exactly one CRLF.
```cpp
// find value first
field_value.ltrim_if(&ParseRules::is_ws);
field_value.rtrim_if(&ParseRules::is_wslfcr);
if (raw_print_field) {
// Raw printing copies the original input bytes instead of re-serializing, so it would
// replay this line ending verbatim. Everything between the end of the trimmed value and
// the end of the line must be optional whitespace followed by exactly one CRLF; a bare
// CR in there is malformed (RFC 9112 section 2.2) and must not be forwarded.
TextView tail{field_value.data() + field_value.size(), parsed.data() + parsed.size()};
if (tail.suffix(2) != "\r\n" || tail.remove_suffix(2).find_first_of("\r\n") != TextView::npos) {
raw_print_field = false;
}
}
```
With that in place all five rows normalize to a single CRLF and `test_proxy_hdrs` stays green. Well-formed fields keep the raw fast path: `X: a \r\n`, `X:\ta\r\n` and a folded line are all still emitted byte for byte. The `else if` on the line ending goes away, as does the need for the `size() > 2` guard, since `suffix()` clamps.
Three smaller items for the same change:
1. The unit test fixture added in #13595 produces a raw-print pad of exactly 7, and `mime_field_name_value_set` only honors raw printing when the pad is 7 or less. One more CR and the pre-existing pad gate suppresses raw printing on its own, so the test would pass without any fix at all. It needs a comment saying so, plus a case that is not on the boundary.
2. Only the request direction is covered. The same function serves responses, so a `parse_resp` case is cheap.
3. `tests/gold_tests/headers/field_name_space.test.py` covers the sibling whitespace-before-colon case end to end. Because `MIMEFieldBlockImpl::move_strings` clears the raw flag on any string-heap relocation, whether these bytes reach the origin depends on heap state a unit test does not exercise, so a gold test with a raw-socket client would pin the actual wire contract.
This reproduces on 10.2.x and 10.1.x as well.
Contributor guide
Research direction
Start in mime_parser_parse and review the raw-print handling described in the issue, then run test_proxy_hdrs. Check request and parse_resp cases for the five malformed line endings, while confirming well-formed and folded fields retain byte-for-byte output. Use tests/gold_tests/headers/field_name_space.test.py as the end-to-end reference for the wire-level behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- networking
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100