dynamic_modules: fix stale decodingBuffer causing empty body with ext_authz#44043
dynamic_modules: fix stale decodingBuffer causing empty body with ext_authz#44043douyux wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
…_authz (envoyproxy#43063) When ext_authz buffers HTTP/2 request body during async auth, decodingBuffer() remains non-null after draining. A downstream dynamic module filter's decodeData() would then incorrectly call addDecodedData() on the final end_of_stream chunk, moving its content into the stale buffer and leaving an empty chunk for the router — causing upstream request timeout. Fix: track whether THIS filter is actively buffering via request_body_buffering_/response_body_buffering_ flags, updated after each decodeData/encodeData callback. Only call addDecodedData/addEncodedData when the flag is true. Includes regression tests for both decode and encode paths. AI-assisted: This change was developed with Claude AI assistance. Signed-off-by: bxwu <douyubox@gmail.com>
douyux
requested review from
agrawroh,
mathetake,
mattklein123 and
wbpcode
as code owners
douyux
had a problem deploying
to
external-contributors
GitHub Actions
Failure
|
Hi @douyux, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
| if (end_of_stream && decoder_callbacks_->decodingBuffer()) { | ||
| // To make the very last chunk of the body available to the filter when buffering is enabled, | ||
| // we need to call addDecodedData. See the code comment there for more details. | ||
| if (end_of_stream && request_body_buffering_ && decoder_callbacks_->decodingBuffer()) { | ||
| // When THIS filter is actively buffering (previously returned StopIterationAndBuffer or | ||
| // StopIterationAndWatermark), merge the final chunk into the buffered data so the module can | ||
| // see the complete body via BufferedRequestBody. We must only do this when this filter itself | ||
| // is buffering — not when decodingBuffer() is stale non-null from another filter (e.g. | ||
| // ext_authz). See https://github.com/envoyproxy/envoy/issues/43063. | ||
| decoder_callbacks_->addDecodedData(chunk, false); |
There was a problem hiding this comment.
I think we only need to remove the addDecodedData logic now because now we will access the buffered body and the received body with different ABI correctly. The addDecodedData (which is designed for previous legacy ABI) now should be unnecessary.
There was a problem hiding this comment.
Thanks for the quick review and attention to this issue! That makes sense — I was also puzzled by the addDecodedData logic since the ABI already provides separate access to buffered and received body. I'm
not deeply familiar with the codebase, so I'd prefer to leave the fix to you. Happy to close this PR.
There was a problem hiding this comment.
OK, I will create a PR to fix it. :)
|
See #44081 |
4 participants
Commit Message: dynamic_modules: fix stale decodingBuffer causing empty body with ext_authz
Additional Description:
When ext_authz buffers HTTP/2 request body during async auth,
decodingBuffer()remainsnon-null after draining. A downstream dynamic module filter's
decodeData(end_of_stream=true)then incorrectly calls
addDecodedData(), moving the final chunk's content into the stalebuffer — the router receives an empty body and the upstream request times out.
The fix adds
request_body_buffering_/response_body_buffering_flags that track whetherTHIS filter previously returned
StopIterationAndBufferorStopIterationAndWatermark.The
addDecodedData/addEncodedDatacalls are now guarded by these flags.Risk Level: Low
Testing: Unit tests added (
DecodeDataStaleDecodingBufferBug,EncodeDataStaleEncodingBufferBug). Updated existingBodyCallbackstest.Fixes: #43063
API Considerations: This change was developed with Claude AI assistance.