check body_filter rulesets even without content-length and chunked da…#307
check body_filter rulesets even without content-length and chunked da…#307sandromodarelli wants to merge 1 commit intop0pr0ck5:masterfrom sandromodarelli:patch-1
Conversation
…ta transmission I reviewed how collections.RESPONSE_BODY have been filled. Added also a ctx.short_circuit = false to ensure rule processing. res_body_max_size check is compared with total buffer length, so body_filter phase's rulesets can be processed even for dynamic pages (Transfer-Encoding: chunked)
|
@p0pr0ck5 any news about this pull request? |
p0pr0ck5
left a comment
p0pr0ck5
left a comment
There was a problem hiding this comment.
Sorry for the delayed response, and thanks for the patch. I think there may be a bug in the logic here, but I don't have time to test at this point.
| ctx.res_length = ctx.res_length + string.len(data) | ||
| end | ||
|
|
||
| if eof or ctx.res_length > waf._res_body_max_size then |
There was a problem hiding this comment.
what happens when the Nginx body filter runs a chunk again after this expression evaluates to true (ctx.res_length > waf._res_body_max_size)? from what i can tell, ctx.skip_buffering will be true because we set it in a previous body_filter run in this block. therefore, ngx.arg[1] will not be set to nil (as it is above on line 95), leading to mangled response bodies.
There was a problem hiding this comment.
when ctx.res_length > waf._res_body_max_size is true the response will be the concatenation of all chunks buffered.
The next chunk will not be processed because of ctx.skip_buffering on line 91: this means that ngx.arg[1] won't be touched and will contain the entire current chunk, ctx.short_circuit=true will be applied and chunk will be sent to client without lost of datas
There was a problem hiding this comment.
Thanks. Do you have a test case to demonstrate this behavior?
We will need regression and integration tests for this change anyhow. Thanks!
There was a problem hiding this comment.
@sandromodarelli im still waiting on a response for the following:
Do you have a test case to demonstrate this behavior?
We will need regression and integration tests for this change anyhow. Thanks!
3 participants
…ta transmission
I reviewed how collections.RESPONSE_BODY have been filled. Added also a ctx.short_circuit = false to ensure rule processing.
res_body_max_size check is compared with total buffer length, so body_filter phase's rulesets can be processed even for dynamic pages (Transfer-Encoding: chunked)