Parser only stores standard trailer fields

[ashtum]

No description provided.

[sehe]

Is there a linked issue for this change?

Specifically, I am concerned this will break custom trailer headers for existing users of Beast, see e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Trailer.

As you can see it's not a feature browsers typically support, but we're typically not writing/dealing with browsers here, so the feature might be used (for things like debugging, telemetry, custom digests).

[ashtum]

Is there a linked issue for this change?

This is actually a security concern and stems from the fact that we are storing trailer fields and initial fields in the same container. As a result, it is possible for a chunked message to add arbitrary fields to a message at the end.

Specifically, I am concerned this will break custom trailer headers for existing users of Beast, see e.g. https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Trailer.

As you can see it's not a feature browsers typically support, but we're typically not writing/dealing with browsers here, so the feature might be used (for things like debugging, telemetry, custom digests).

For more advanced use cases, users can create their own parser class derived from basic_parser. Another alternative could be to add a virtual function to the parser class (not basic_parser) that determines whether a trailer field should be stored, with a default implementation that allows only the standard ones. However, considering how rarely trailer headers are used (if used at all), I think we don't need to make this customization easy.

By the way, this is a draft PR, so we can consider all the alternatives and decide.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.18%. Comparing base (72dc439) to head (a92c5ce).
⚠️ Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3042   +/-   ##
========================================
  Coverage    93.17%   93.18%           
========================================
  Files          176      176           
  Lines        13689    13690    +1     
========================================
+ Hits         12755    12757    +2     
+ Misses         934      933    -1     

Files with missing lines	Coverage Δ
include/boost/beast/http/basic_parser.hpp	94.11% <ø> (ø)
include/boost/beast/http/impl/basic_parser.ipp	97.14% <100.00%> (+0.01%)	⬆️
include/boost/beast/http/impl/field.ipp	98.59% <100.00%> (+0.94%)	⬆️
include/boost/beast/http/parser.hpp	89.02% <100.00%> (+2.06%)	⬆️

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72dc439...a92c5ce. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ashtum]

Another option could be to check if a trailer field is listed in the Trailer field and then store it. This approach assumes that the user checks the Trailer field when the headers are received, so it would not be safe by default, which is not ideal and does not change the status quo.

[sehe]

This approach assumes that the user checks the Trailer field when the headers are received, so it would not be safe by default, which is not ideal and does not change the status quo.

I think it's how the RFC is intended, though. Also, it would allow you to check that disallowed headers are not named, or no trailing headers would overwrite existing headers. That way, it would already safer by default.

I can see room for a flag "allow non-standard trailers". I'm not sure how feasible it would be for a derived parser to override the on-trailer-header handler, but that idea could work.

[ashtum]

Also, it would allow you to check that disallowed headers are not named

The problem is there isn’t a concrete list of disallowed headers, that’s why I went with a whitelist instead.

or no trailing headers would overwrite existing headers. That way, it would already safer by default.

Yes, this would make it much safer, although it would still allow an attacker to append arbitrary fields (e.g. X-Forwarded-For).

I can see room for a flag "allow non-standard trailers". I'm not sure how feasible it would be for a derived parser to override the on-trailer-header handler, but that idea could work.

This depends on how often trailer headers are used in the wild, and if they are, how many of these usages don’t fall into the whitelist we already have. Considering trailer fields are disallowed in browsers (except Server-Timing in Firefox) and most HTTP libraries, I don’t expect any usage in the wild (this is just an assumption though). Unless there are cases that rely on libraries for both client and server sides that allows arbitrary trailer fields. In that case, users can implement their own parser from basic_parser (starting by copying the existing parser implementation in Beast).

So, my attempt is to avoid complicating the current interface unless we have evidence that the feature is used in the wild. The closest thing I found was gRPC’s grpc-status and grpc-message, but gRPC is built on top of HTTP/2.

[sehe]

Also, it would allow you to check that disallowed headers are not named

The problem is there isn’t a concrete list of disallowed headers, that’s why I went with a whitelist instead.

Honoring the Trailer header is a whitelist. My point was that you can "check that disallowed headers are not named" (in the Trailer header)

[sehe]

or no trailing headers would overwrite existing headers. That way, it would already safer by default.

Yes, this would make it much safer, although it would still allow an attacker to append arbitrary fields (e.g. X-Forwarded-For).

Only iff it were named in a Trailer header value. This will vastly reduce the opportunity for abuse in most server architectures.

I can see room for a flag "allow non-standard trailers". I'm not sure how feasible it would be for a derived parser to override the on-trailer-header handler, but that idea could work.

This depends on how often trailer headers are used in the wild, and if they are, how many of these usages don’t fall into the whitelist we already have.

Again, my concern is the opposite. Currently, users of the library can rely on this functionality. It should be reasonably possible to achieve the same result with the changes. Again:

I'm not sure how feasible it would be for a derived parser to override the on-trailer-header handler, but that idea could work.

If it's trivial, a 16-word sketch in the release notes could do

[ashtum]

@sehe, the behavior has changed as follows:

• By default, only standard trailer headers that are listed in the Trailer field are merged.
• When parser::merge_all_trailers(true) is set, all trailers listed in the Trailer field are merged.

Regardless of the settings, any trailer header not listed in the Trailer field is always discarded.

[cppalliance-bot]

An automated preview of the documentation is available at https://3042.beast.prtest.cppalliance.org/libs/beast/doc/html/index.html

If more commits are pushed to the pull request, the docs will rebuild at the same URL.

2025-10-16 12:04:57 UTC

[sehe]

@sehe, the behavior has changed as follows:

Awesome. That looks like it covers all purposes nicely.

My only review comment would be that the comment I'm replying to does a FAR better job documenting the behavior than the doxygen. Notably, on_trailer_field_impl docs don't mention any conditions.

merge_all_trailers could be better named (it never merges all trailers, after all). Perhaps merge_non_standard_tailers? Or slightly less clunky when inverted: merge_standard_trailers_only?

[ashtum]

My only review comment would be that the comment I'm replying to does a FAR better job documenting the behavior than the doxygen. Notably, on_trailer_field_impl docs don't mention any conditions.

basic_parser calls on_trailer_field_impl unconditionally, it is the parser class that does the filtering. so you can find related docs here:
http::parser::merge_all_trailers (2 of 2 overloads)

merge_all_trailers could be better named (it never merges all trailers, after all). Perhaps merge_non_standard_tailers? Or slightly less clunky when inverted: merge_standard_trailers_only?

I first went with merge_nonstandard_trailers but it's not correct either, because there's not an standard whitelist either, it is more like a well-know list or something.