httptoolkit / mockttp

Powerful friendly HTTP mock server & proxy library
https://httptoolkit.com
Apache License 2.0
786 stars 88 forks source link

[BUG] Overriding request headers cause requests to fail #154

Closed kspeers-r7 closed 1 year ago

kspeers-r7 commented 1 year ago

The following proxy script can be used to demonstrate an issue overriding headers:

// This bypasses self signed certs, when proxy chaining
process.env['NODE_TLS_REJECT_UNAUTHORIZED'] = 0;
const port = 8886;
(async () => {
 const mockttp = require('mockttp');
 const https = await mockttp.generateCACertificate();
 let proxy = mockttp.getLocal({ https, http2: true });
 await proxy.start(port);
 console.log(`Proxy Server is listening on port ${port}`);
 // For all requests
 proxy.forAnyRequest().thenPassThrough({
  beforeRequest: ({ headers }) => {
   // optional - modify headers
   return { headers };
  },
  trustAdditionalCAs: [{ cert: https.cert }]
 });
})();

After running the above script:

pimterry commented 1 year ago

Ah, interesting! That's certainly a bug, thanks for the clear repro!

It seems like this is due to HTTP/1 fallback logic. Notably, www.imdb.com supports HTTP/2 but imdb.com does not (you can test with the same commands without the -x ... argument to send requests directly).

When Mockttp receives an HTTP/2 request, it tests the upstream server, and sends HTTP/2 if possible, or falls back to HTTP/1 if not. That's powered by http2-wrapper which provides a single API to optimistically connect to HTTP/2 and fallback where it's unavailable.

To make this work, we normally strip out HTTP/2 headers entirely, so they're auto-generated during the request, but we can't do that if you might have modified them (you really might want to set a modified value) as in the beforeRequest callback here.

It turns out that in this case, http2-wrapper doesn't fall back to HTTP/1 cleanly, and it tries to send HTTP/2-only headers to the HTTP/1 server, which breaks.

I've filed a bug there to look at this: https://github.com/szmarczak/http2-wrapper/issues/98. I'll open a PR to do that there, if they're open to it, TBC.

Once that's fixed, this will work automatically. Until then, we could work around this automatically for some cases (adding logic to detect when you haven't really modified the pseudo headers, and so we can drop them) but it will always be broken for the case of HTTP/2 requests, where your callback modifies an HTTP/2-only header, but the server only supports HTTP/1. Somehow we have to convert the headers appropriately after testing the server for HTTP/2 support, and right now there's no way to do that.

kspeers-r7 commented 1 year ago

thanks for such a quick response - I'll keep an eye on the http2-wrapper issue you raised as well

pimterry commented 1 year ago

This is now fixed @kspeers-r7, in a patch bump for http2-wrapper. Can you try updating that to v2.2.1 and see if you still have the same issue?

kspeers-r7 commented 1 year ago

Thanks for the update. I'll get v2.2.1 tested and confirm if the fix works

kspeers-r7 commented 1 year ago

Confirmed that v2.2.1 fixes the problem we were seeing. Many thanks.

pimterry commented 1 year ago

Great! I'll close this now. Thanks for reporting this, glad we could get that sorted :+1: