Skip to content

Forward the client's Accept-Encoding to the proxy backend, and handle a brotli response (#265) - #509

Open
wakqasahmed wants to merge 4 commits into
litespeedtech:masterfrom
wakqasahmed:fix/issue-265-proxy-accept-encoding
Open

Forward the client's Accept-Encoding to the proxy backend, and handle a brotli response (#265)#509
wakqasahmed wants to merge 4 commits into
litespeedtech:masterfrom
wakqasahmed:fix/issue-265-proxy-accept-encoding

Conversation

@wakqasahmed

Copy link
Copy Markdown
Contributor

Closes #265.

OLS overwrites the client's Accept-Encoding with gzip before proxying, so a backend that can serve brotli never gets the chance to. The first commit drops that rewrite and forwards the header as the client sent it, still falling back to gzip when the client sent none at all.

@litespeedtech pointed out that on its own this breaks cached pages. That's right, and it turned out to break more than the cache — nothing on the proxy response path knew what br was. HttpCgiTool::processHeaderLine2() matched only gzip and deflate in an upstream Content-Encoding, so a brotli response left the request's compression flags untouched. HttpSession::setupGzipFilter() then saw gz == GZIP_REQUIRED for the usual gzip, deflate, br client and gzipped the brotli bytes, with addGzipEncodingHeader() merging the header into Content-Encoding: br,gzip. With the cache on it got worse: cacheHeader() decides whether to compress from get_resp_buffer_compress_method(), which returned LSI_NO_COMPRESS, so the module gzipped the brotli body and stored the entry as CEH_GZIP. A hit then set Content-Encoding: gzip over the stored br, and the client gunzipped its way to garbage.

The rest of the commits make that path brotli-aware. parseContentEncoding() factors the value matching out of processHeaderLine2() and adds br, setting UPSTREAM_BR. setupGzipFilter() returns early for a brotli body instead of stacking gzip on it, and marks the buffer with HSF_RESP_BODY_BRCOMPRESSED.

That flag is what fixes the cache, without touching cache.cpp. get_resp_buffer_compress_method() now reports LSI_BR_COMPRESS, so cacheHeader()'s needGzip is false, markReady() stores the entry as CEH_BR, and handlerProcess() already has the branch that serves Content-Encoding: br from the stored bytes. toggleGzipState() already returns early for anything that isn't none or gzip, and buildCacheKey() already varies on Accept-Encoding through getVaryFlag(), so no new vary dimension is needed either.

One more thing fell out of it. contentEncodingFixup() ran modgzip's decompress filter over whatever Content-Encoding was present whenever the client accepted neither gzip nor br. That filter is zlib only, so it would inflate brotli — or zstd — into garbage. It's now restricted to gzip and deflate. There is no brotli decoder wired into the response filter chain, so if a backend returns brotli to a client that never asked for it, OLS passes it through untouched rather than mangling it. That's what it already does for any other encoding it can't undo, and it's only reachable if the backend ignores the Accept-Encoding it was sent.

I still don't have a working build of this tree here (empty lsquic submodule, brotli and boringssl need the fetch scripts), so I can't claim a compiled or runtime test. What I did check: parseContentEncoding() compiled and run standalone against the cases in the new unit test, and the two edited HttpSession bodies re-compiled as a standalone translation unit with stubbed dependencies to catch syntax and arity mistakes. The test case lives in test/http/httpcgitooltest.cpp, which was already in the test build but had every case commented out.

Previously proxyconn.cpp always rewrote the client's Accept-Encoding
header to 'gzip' before forwarding it to a proxy backend, clobbering
brotli ('br') and any other encoding the client actually requested.
Only forward the client's original header, still defaulting to gzip
when the client sent none, matching the existing backend<->OLS
decompression path (HttpSession::setupGzipFilter handles only
gzip/deflate from upstream).
…espeedtech#265)

processHeaderLine2() only looked for gzip and deflate, so a 'br' response
from a proxy backend left the request's compression flags untouched and the
rest of the response path treated the body as uncompressed.

Factor the value matching out into HttpCgiTool::parseContentEncoding() so
the same classification can be reused elsewhere, and set UPSTREAM_BR when
the backend used brotli.
…edtech#265)

setupGzipFilter() never checked whether the body it was about to compress
was already encoded. With UPSTREAM_BR now set for a brotli backend response,
gz still equalled GZIP_REQUIRED for the usual 'gzip, deflate, br' client, so
OLS gzipped brotli bytes and addGzipEncodingHeader() merged the header into
'Content-Encoding: br,gzip'.

Bail out early for a brotli body instead and record it via
HSF_RESP_BODY_BRCOMPRESSED. That also makes get_resp_buffer_compress_method()
report LSI_BR_COMPRESS, which is what the cache module keys its own
compression decisions off: cacheHeader() then skips its gzip pass and stores
the entry as CEH_BR, and a hit serves 'Content-Encoding: br' from the stored
bytes rather than replacing the header with gzip.

contentEncodingFixup() had the mirror-image problem: for a client that
accepts neither gzip nor br it ran modgzip's zlib decompress filter over
whatever encoding was present. Restrict that to gzip and deflate, the only
two it can actually decode.
…tech#265)

httpcgitooltest.cpp is already in the unit test build but was entirely
commented out. Add a case for HttpCgiTool::parseContentEncoding() covering
gzip, deflate, none and br, plus the encodings the response filters cannot
decode, since a wrong answer there is what leads to a body being compressed
twice.
@wakqasahmed

Copy link
Copy Markdown
Contributor Author

@litespeedtech your cache concern is handled now, so you don't have to work it back out of the diff.

The reason the cache re-gzipped a brotli body was that get_resp_buffer_compress_method() returned LSI_NO_COMPRESS for it — no core path ever set HSF_RESP_BODY_BRCOMPRESSED, it was only ever set through the module API. cacheHeader() therefore took needGzip, compressed the already-brotli bytes and stored the entry as CEH_GZIP, and the hit path replaced the stored br with Content-Encoding: gzip.

setupGzipFilter() now sets that flag when the upstream response was brotli, which is enough on its own: needGzip goes false, markReady() stores CEH_BR, and handlerProcess()'s existing LSI_BR_COMPRESS branch serves it back with the right header. cache.cpp is untouched. toggleGzipState() already bails on anything that isn't none or gzip, and getVaryFlag()/buildCacheKey() already key on Accept-Encoding, so a br entry can't be served to a client that didn't ask for one.

I had to open this as a new PR — GitHub kept refusing to reopen #504 ("Could not open the pull request") even though the branch merges cleanly. Same branch, three commits on top of the original one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gzip encoding enforced for proxy

1 participant