net: http_server: capturing request headers does not correctly account for concurrent HTTP2 streams #82273
Comments
I think we can still break the API if needed, it is not considered stable API yet. We need to document these API changes for example in migration guide. |
In that case I think something like: would be a good option, with the |
That sounds reasonable, you're right that the client should not mixup header frames from different streams - if one header frame does not carry a full set of headers, it has to be followed with continuation on the same stream.
I like the idea, seems pretty consistent. |
Concurrent HTTP POST requests on different HTTP2 concurrent streams require that the client's header_capture_context is re-used to capture headers on a second stream before all of the body data has been received (and sent to the application) on the first stream. As a result, any captured headers must be sent to the application callback before any headers can be received on a different stream. In practice this means that for HTTP2 the application callback is called for the first time on receiving a headers frame, before any data frames are received. All subsequent application callbacks will not include the request header data. While this mechanism is not necessary for HTTP1, it is also updated to only send headers in the first application callback for consistency. Fixes zephyrproject-rtos#82273 Signed-off-by: Matt Rodgers <[email protected]>
Concurrent HTTP POST requests on different HTTP2 concurrent streams require that the client's header_capture_context is re-used to capture headers on a second stream before all of the body data has been received (and sent to the application) on the first stream. As a result, any captured headers must be sent to the application callback before any headers can be received on a different stream. In practice this means that for HTTP2 the application callback is called for the first time on receiving a headers frame, before any data frames are received. All subsequent application callbacks will not include the request header data. While this mechanism is not necessary for HTTP1, it is also updated to only send headers in the first application callback for consistency. Fixes zephyrproject-rtos#82273 Signed-off-by: Matt Rodgers <[email protected]>
Concurrent HTTP POST requests on different HTTP2 concurrent streams require that the client's header_capture_context is re-used to capture headers on a second stream before all of the body data has been received (and sent to the application) on the first stream. As a result, any captured headers must be sent to the application callback before any headers can be received on a different stream. In practice this means that for HTTP2 the application callback is called for the first time on receiving a headers frame, before any data frames are received. All subsequent application callbacks will not include the request header data. While this mechanism is not necessary for HTTP1, it is also updated to only send headers in the first application callback for consistency. Fixes zephyrproject-rtos#82273 Signed-off-by: Matt Rodgers <[email protected]>
Concurrent HTTP POST requests on different HTTP2 concurrent streams require that the client's header_capture_context is re-used to capture headers on a second stream before all of the body data has been received (and sent to the application) on the first stream. As a result, any captured headers must be sent to the application callback before any headers can be received on a different stream. In practice this means that for HTTP2 the application callback is called for the first time on receiving a headers frame, before any data frames are received. All subsequent application callbacks will not include the request header data. While this mechanism is not necessary for HTTP1, it is also updated to only send headers in the first application callback for consistency. Fixes zephyrproject-rtos#82273 Signed-off-by: Matt Rodgers <[email protected]>
Describe the bug
The header capture feature of the HTTP server (
CONFIG_HTTP_SERVER_CAPTURE_HEADERS) does not correctly handle HTTP2 concurrent streams. In the case of a dynamic POST request, headers associated with one request could sometimes be accessed by an application callback associated with a different request.To Reproduce
I think it would actually be quite difficult to create this situation in "real" usage of the HTTP server, but the situation would be:
Perhaps the easiest way to demonstrate this is the failing test case I've created in this branch: https://github.com/mrodgers-witekio/zephyr/tree/http2_request_headers_concurrent_streams
Expected behavior
Each request should call the dynamic response callback with the correct headers associated with that request.
Impact
Minimal, since concurrent HTTP2 POST requests with the header capture feature enabled and different headers in each request is unlikely to happen often. The issue shouldn't happen with concurrent GET requests, since the header frames of two different streams cannot be interleaved, only header frames with data frames.
Additional context
I didn't consider this situation when implementing the header capture feature, and only just noticed the issue now. I have a few ideas on how to fix, but it would be good to get some thoughts on the right way to go before I submit a PR.
Based on RFC9113 section 4.3 I think that headers from different streams cannot not be interleaved (ie. HEADERS_START_1, HEADERS_START2, CONTINUATION_1, ...). If my understanding is correct, it makes fixing the issue significantly simpler since we do not need to simultaneously store headers for multiple different streams.
So for HTTP/2 we could call the application callback as soon as the headers are parsed to pass the headers to the application, even if no data is sent yet. Then the headers can be cleared ready for a request on a different stream. Any subsequent callbacks providing data to the application would not contain the header data which was passed in the first callback.
Since the application callback currently just accesses the headers from the
client_ctxstruct this cannot currently work, so a different way of passing the headers to the application would be needed. Maybe an additional parameter to the callback for request headers, or combine the request headers and data into astruct http_request_ctxsimilar to thestruct htp_response_ctxused to pass response data back in the other direction?Both of these would require a change to the dynamic resource callback signature which would break existing applications. If we want to avoid this, then maybe adding a pointer to the headers in the
client_ctxstruct, which points to the existingheader_capture_ctxif the headers are valid for a given callback, or is set toNULLif not.The text was updated successfully, but these errors were encountered: