Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

Fix that some response cause decompress fail #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

EltonZhong
Copy link

[2018-09-07T12:27:17.742] [FATAL] mapping - { Error: unexpected end of file
at Zlib.zlibOnError [as onerror] (zlib.js:153:17)
at processChunkSync (zlib.js:481:12)
at zlibBufferSync (zlib.js:139:12)
at Object.gunzipSync (zlib.js:695:14)
at /root/mockingbirds/65003/mockingbird/node_modules/koa-better-http-proxy/app/steps/decorateUserRes.js:12:46

Due to this problem.

@keymandll
Copy link

Just had a look into this. I found traces of the problem from 2004 and indeed, curl had some workaround implemented such as Z_SYNC_FLUSH. But, on top of that, from the diff of the recommended workaround, it seemed to me there may have been some need to fake some headers there too for the workaround to be complete.
I tested with Apache/2.4.54 (released on 2022-06-08) and things were working perfectly fine without the changes proposed by this MR.

@jeremy-daley-kr
Copy link

@keymandll I know that it's definitely not working perfectly fine in my case, where a HEAD request is proxied. A HEAD response does not include a body, so what @EltonZhong posted happens for me. Not sure if his PR would fix my issue, but a byteLength zero Buffer probably shouldn't be gunzipped.

@keymandll
Copy link

keymandll commented Feb 24, 2023

@jeremy-daley-kr It's been a while since I had a look into this so I did another round of testing again and was playing around with the Content-Encoding and the body of the HEAD response that I then proxied with koa-better-http-proxy. I could not find a test case that would produce the exception in the PR.

I'm using Node v18.12.1. Which version do you use? I'm wondering if upgrading to a more recent Node version would help. (Maybe they added some checks to the Zlib implementation)
(edit) Also, which version of koa-better-http-proxy do you use?

Anyway, I'd love to see that raw HTTP HEAD response if you could share it, please.

@jeremy-daley-kr
Copy link

jeremy-daley-kr commented Feb 24, 2023

@keymandll I really don't think Node version is of consequence. Can you please check my PR here: #54

If you remove the byteLength check from my branch and run the test, you should see the error mentioned. Take note of the agent().head(...) in the test, which returns a zero-byte response.

@keymandll
Copy link

@jeremy-daley-kr You don't think or do you know for sure? The reason why I asked for your node version and the version of koa-better-http-proxy you are using is that even with your test I could not get the proxy to throw the exception in question. It seems to me there is a difference between your system and mine which contributes to the problem you are experiencing.

On a side note: I'm not a maintainer of this lib, I have my own copy I use and maintain. This is why I'm interested in this.

@jeremy-daley-kr
Copy link

@keymandll I appreciate the interest and feedback. The .nvmrc of the project is set to v14.15.4, but even testing with your Node version, I get the same result:

image

I'm trying to prove out the test, 'cause I'm obviously seeing the error... I'm just more familiar with Jest than I am mocha =/

@keymandll
Copy link

Update. While I'm still unable to reproduce with HEAD requests, I could reproduce with a GET request.

So, a raw HTTP response example to a GET request that could trigger the issue was:

HTTP/1.1 200 OK
Content-Encoding: gzip
Connection: close
Content-Type: text/plain; charset=utf-8
Content-Length: 0
Date: Fri, 24 Feb 2023 17:56:55 GMT

@jeremy-daley-kr
Copy link

@keymandll I just updated my test btw. Took me a minute to figure out sinon spying, but I've confirmed test failure when I remove the if condition.

@keymandll
Copy link

@jeremy-daley-kr I was also looking into it as the tests needed some changes to work as expected. Anyway, as things are not moving with this project, I applied the fix and wrote the test for my fork of the library.

If you are interested, you can find the:

Source at: https://gitlab.com/Keymandll/koa-even-better-http-proxy
NPM: https://www.npmjs.com/package/koa-even-better-http-proxy

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants