feat(helper/proxy): introduce proxy helper

[usualoma]

fixes #3518

Naming

The name proxyFetch seems a little redundant, but I rejected the other candidates for the following reasons.

• proxy : The name proxy is simple but is avoided because it is confusing with the JavaScript Proxy object.
• fetch : The name fetch is also good. Although it is in the helper/proxy namespace, so it can be distinguished, when it is used by being incorporated into the application, from the standpoint of reading the code, it looks like globalThis.fetch is being called, so I decided to avoid it because of the cognitive load.

Usage

app.get('/proxy/:path', (c) => {
  return proxyFetch(new Request(`http://${originServer}/${c.req.param('path')}`, c.req.raw), {
    proxySetRequestHeaders: {
      'X-Forwarded-For': '127.0.0.1',
      'X-Forwarded-Host': c.req.header('host'),
      Authorization: undefined, // prevent propagating "Authorization" request headers in c.req.raw
    },
    proxyDeleteResponseHeaderNames: ['Cookie'],
  })
})

The author should do the following, if applicable

• Add tests
• Run tests
• bun run format:fix && bun run lint:fix to format the code
• Add TSDoc/JSDoc to document the code

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.73%. Comparing base (caeecf9) to head (3a5ef0f).
Report is 24 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3589      +/-   ##
==========================================
+ Coverage   94.71%   94.73%   +0.01%     
==========================================
  Files         158      159       +1     
  Lines        9557     9587      +30     
  Branches     2826     2834       +8     
==========================================
+ Hits         9052     9082      +30     
  Misses        505      505              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[haochenx]

all looks good to me except regarding the handling of range request and deletion of the Content-Length header from the response

[haochenx]

i'm not sure about Content-Length and Content-Range, as if the original request is for a byte range, the semantics is not preserved. a probable semantically correct implementation:

1. if the original request contains Range header, fast-fail with 400 status code
2. strip Accept-Range from response header
3. indicate upstream error if response header contain Content-Range

Content-Length should be left alone in all case IMO

[usualoma]

Thank you! This certainly needs fixing.

Content-Length

As you can see from the results below, in the case of a compressed response, Content-Length is the “size after compression”, so it cannot be returned as is.

% curl -I 'https://example.com/'
HTTP/2 200
accept-ranges: bytes
age: 526496
cache-control: max-age=604800
content-type: text/html; charset=UTF-8
date: Wed, 30 Oct 2024 21:13:42 GMT
etag: "3147526947"
expires: Wed, 06 Nov 2024 21:13:42 GMT
last-modified: Thu, 17 Oct 2019 07:18:26 GMT
server: ECAcc (lac/55F5)
x-cache: HIT
content-length: 1256
% curl --compressed --raw -i 'https://example.com/'
HTTP/2 200
content-encoding: gzip
age: 527039
cache-control: max-age=604800
content-type: text/html; charset=UTF-8
date: Wed, 30 Oct 2024 21:07:12 GMT
etag: "3147526947+gzip"
expires: Wed, 06 Nov 2024 21:07:12 GMT
last-modified: Thu, 17 Oct 2019 07:18:26 GMT
server: ECAcc (lac/55AA)
vary: Accept-Encoding
x-cache: HIT
content-length: 648

• Delete <- This is the current implementation
• Load the body and reset it <- There is an overhead because you have to clone() and load all the data

Some people may expect the latter, but since hono does not actively add Content-Length to other requests either, I think this is a reasonable response for hono.

However, if there is no Content-Encoding in the first place, there is no need to delete the Content-Length. I think this should be improved.

Content-Range

I think the current implementation is incorrect.
It seems that this will not change depending on Content-Encoding (although in a real environment, it seems that the result of compression will almost never be returned in a request with Range), so I think it is correct to always return it as is.

[usualoma]

Fixed in 5998770 and 3a5ef0f.

[haochenx]

Content-Length is the “size after compression”, so it cannot be returned as is.

Oh, in that case I am in favor of unconditionally deleting it.

It seems that this will not change depending on Content-Encoding (although in a real environment, it seems that the result of compression will almost never be returned in a request with Range), so I think it is correct to always return it as is.

I have no reason to believe the new implementation is incorrect and this seems to be a suitable strategy.

[usualoma]

Thank you!

[haochenx]

Looks good to me.

[yusukebe]

Hey @usualoma ! Thank you for the PR. I'll try to use this and comment later.