Properly alert receivers when using non-default dynamic table sizes for HPACK #495
Conversation
CelestiaTheDryad
force-pushed
the
header-max-size-fix
branch
from
ba2453a
to
d988854
Compare
|
Rebased onto master. |
|
@CelestiaTheDryad This is big. I will need some time to review the changes. Do you think there is a possibility to put together a test case to verify the fix? |
| @@ -261,6 +262,11 @@ void encodeHeader( | |||
| void encodeHeader( | |||
| final ByteArrayBuffer dst, final String name, final String value, final boolean sensitive, | |||
| final boolean noIndexing, final boolean useHuffman) throws CharacterCodingException { | |||
| //send receiver the updated dynamic table size | |||
| if (maxTableSize != this.dynamicTable.getMaxSize()) { | |||
There was a problem hiding this comment.
@CelestiaTheDryad This looks like the most essential change in the change-set. Is there any chance you could add a unit test to TestHPackCoding for this condition and will happily merge your changes
There was a problem hiding this comment.
I'll add some unit tests today.
CelestiaTheDryad
force-pushed
the
header-max-size-fix
branch
from
d988854
to
79c8fca
Compare
|
Added unit tests for several behaviors relating to dynamic table size and the size change signal. This includes one logical change to the encoder, replacing a |
|
Cherry-picked to I am baffled though as to how we never came across this problem before. Do I understand it correctly you have been using HttpCore on the server side and common browsers on the client side? |
|
That's correct. We use HttpCore to serve the files for our web app and the REST API that controls the back-end server. We encountered this issue when upgrading to HttpCore 5.x to leverage HTTP/2. The 4096 octets that browsers default to is large enough that low-usage connections wont even reach the limit. It's also plausible that the maximum size used by browsers was changed recently. In any case it was immediately apparent to us because of the size of some headers we use. |
|
@CelestiaTheDryad There will likely be a bug fix release next week (v5.3.1). Please do try to test your code with the latest snapshot from 5.3.x branch and let me know if everything works as expected. |
https://datatracker.ietf.org/doc/html/rfc7541#section-6.3
I discovered while using this library that common web browsers (chrome, edge, firefox) encountered HTTP2 protocol/HPACK compression errors when communicating with servers from this library. This was eventually traced to the browser and server using different values for the dynamic table size. I determined this was because this library does not send dynamic table size updates, and instead assumed both sides would immediately use the receiver's maximum table size. After adding encoder support for the dynamic table size update header signal, communication between browser and server is now error-free.