quiche: enforce content-length header consistency#18459
quiche: enforce content-length header consistency#18459alyssawilk merged 7 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
|
/assign @alyssawilk |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Thanks for picking this one up! just 2 nits and you're good to go
| if (received_content_bytes_ > content_length_.value() || | ||
| (end_stream && received_content_bytes_ != content_length_.value())) { | ||
| // Reset instead of closing the connection to align with nghttp2. | ||
| onStreamError(false); |
There was a problem hiding this comment.
can we make sure the stream would have informative error details here?
There was a problem hiding this comment.
added inconsistent_content_length response code detail
| } | ||
| } | ||
|
|
||
| TEST_P(DownstreamProtocolIntegrationTest, ContentLengthLargerThanPayload) { |
There was a problem hiding this comment.
What do you think of one additional test in multiplexed integration test where we have an incorrect body length and trailers, just to test the accounting you added there?
|
one more thing sorry, can you add the TODO to clean up once we enforce upstream? |
repokitteh-read-only
bot
added
the
deps
danzh2010
force-pushed
the
contentlength
branch
from
83c154b to
a097a9d
Compare
done |
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
cc @wrowe could you do a quick pass and let me know if there's anything other than 304/HEAD that content length validation doesn't apply to?
|
/assign @wrowe |
|
Guess not :-/ |
4 participants
Signed-off-by: Dan Zhang danzh@google.com
Commit Message: If request/response has content-length header, its value must be the equal to the actually body length. Otherise reset the stream as nghttp2 does.
Risk Level: low
Testing: added integration tests