Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support slightly invalid gzip response #2123

Closed
jimmywarting opened this issue May 14, 2023 · 6 comments · Fixed by #2126
Closed

support slightly invalid gzip response #2123

jimmywarting opened this issue May 14, 2023 · 6 comments · Fixed by #2126
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@jimmywarting
Copy link
Contributor

jimmywarting commented May 14, 2023

Here is one test we have in node-fetch, and i run it on undici fetch but it failed...
our test:

	it.only('should decompress slightly invalid gzip response', async () => {
		const url = `${base}gzip-truncated`;
		const res = await fetch(url);
		expect(res.headers.get('content-type')).to.equal('text/plain');
		const result = await res.text();
		expect(result).to.be.a('string');
		expect(result).to.equal('hello world');
	});
// our response

		if (p === '/gzip-truncated') {
			res.statusCode = 200;
			res.setHeader('Content-Type', 'text/plain');
			res.setHeader('Content-Encoding', 'gzip');
			zlib.gzip('hello world', (err, buffer) => {
				if (err) {
					throw err;
				}

				// Truncate the CRC checksum and size check at the end of the stream
				res.end(buffer.slice(0, -8)); // Buffer.from('H4sIAAAAAAAAE8tIzcnJVyjPL8pJAQA=')
			});
		}

this works fine in browsers and some server ignore this CRC checksum and size at the end

@jimmywarting jimmywarting added the bug Something isn't working label May 14, 2023
@mcollina
Copy link
Member

I'm not sure how we could, this might be something to bring up inside Node.js iteself.

@jimmywarting
Copy link
Contributor Author

jimmywarting commented May 15, 2023

We solved it with some zlibOptions options in node-fetch

const buf = Buffer.from('H4sIAAAAAAAAE8tIzcnJVyjPL8pJAQA=', 'base64')
const readable = stream.Readable.from(buf)

// For Node v6+
// Be less strict when decoding compressed responses, since sometimes
// servers send slightly invalid responses that are still accepted
// by common browsers.
// Always using Z_SYNC_FLUSH is what cURL does.
const zlibOptions = {
  flush: zlib.Z_SYNC_FLUSH,
  finishFlush: zlib.Z_SYNC_FLUSH
}


  // For gzip
  if (codings === 'gzip' || codings === 'x-gzip') {
    const ts = zlib.createGunzip(zlibOptions)
    new Response(readable.pipe(ts)).text().then(console.log)
  }

@jimmywarting
Copy link
Contributor Author

jimmywarting commented May 15, 2023

just tested this and it seems to work okey
relative code is here: https://github.com/node-fetch/node-fetch/blob/7b86e946b02dfdd28f4f8fca3d73a022cbb5ca1e/src/index.js#L291C1-L307

without this zlibOptions it fails

it's described here in nodejs docs to about the use of Z_SYNC_FLUSH
https://nodejs.org/dist/latest-v18.x/docs/api/zlib.html#compressing-http-requests-and-responses

By default, the zlib methods will throw an error when decompressing truncated data. However, if it is known that the data is incomplete, or the desire is to inspect only the beginning of a compressed file, it is possible to suppress the default error handling by changing the flushing method that is used to decompress the last chunk of input data:

@mcollina
Copy link
Member

Then we should likely do the same.

@mcollina mcollina added the good first issue Good for newcomers label May 15, 2023
@KhafraDev
Copy link
Member

Relevant part of fetch:

decoders.push(zlib.createGunzip())

Want to send in a PR adding the options? That test could be added to the node-fetch suite.

@jimmywarting
Copy link
Contributor Author

jimmywarting commented May 15, 2023

Now that NodeJS v16 is the current LTS and having web streams built in i was thinking about actually kind of delete everything in node-fetch (including all the test and so forth) and just bring in a dependency on Undici instead and simply just re-export everything. kind of like an alias for Undici. and piggy back on you to fix all our issues we have :P

setting engine.node version to >=16.7

kind of have this in the pipe-line:

import * as undici from 'undici'

const fetch = globalThis.fetch || undici.fetch
const FormData = globalThis.FormData || undici.FormData
const Headers = globalThis.Headers || undici.Headers
const Request = globalThis.Request || undici.Request
const Response = globalThis.Response || undici.Response

export default fetch
export {
	fetch,
	FormData,
	Headers,
	Request,
	Response
}

export * from 'fetch-blob/from.js'

just tough i would first share a few of our test cases that fails with undici before thinking of actually replacing it with undici.

ref: node-fetch/node-fetch#1742

but sure i could maybe add a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants