feat(helper/proxy): introduce proxy helper #3589

usualoma · 2024-10-30T12:43:03Z

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 · 2024-10-30T12:46:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.36%. Comparing base (d40fffb) to head (2e8e250).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3589      +/-   ##
==========================================
+ Coverage   91.32%   91.36%   +0.04%     
==========================================
  Files         161      162       +1     
  Lines       10242    10290      +48     
  Branches     2889     2976      +87     
==========================================
+ Hits         9353     9401      +48     
  Misses        888      888              
  Partials        1        1

☔ 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 · 2024-10-30T13:22:38Z

src/helper/proxy/index.ts

+ * * Content-Length
+ * * Content-Range
+ */
+const forceDeleteResponseHeaderNames = ['Content-Encoding', 'Content-Length', 'Content-Range']


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:

if the original request contains Range header, fast-fail with 400 status code

strip Accept-Range from response header

indicate upstream error if response header contain Content-Range

Content-Length should be left alone in all case IMO

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.

Fixed in 5998770 and 3a5ef0f.

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.

Co-authored-by: Haochen M. Kotoi-Xie <[email protected]>

…ponse is uncompressed

haochenx

Looks good to me.

yusukebe · 2024-11-05T07:17:41Z

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

haochenx · 2024-11-06T06:59:23Z

@usualoma @yusukebe many thanks for your amazing works again!

just fyi we ended up using a pre/process combo like the following to mitigate

somehow (I guess when http/2 is enabled? we observed this when enabling https on vite's devserver) creating RequestInit using { ...resq } (also for ResponseInit) does not work sometime, and fields needs to be assembled manually
"connection" and "transfer-encoding" header needs to be filtered to avoid node throwing ERR_HTTP2_INVALID_CONNECTION_HEADERS

we are only using this particular proxying method in dev server, so there might be other catches..

    const response = await fetch(
      new Request(target, {
        body: c.req.raw.body,
        method: c.req.raw.method,
        headers: [...c.req.raw.headers.entries()].filter(
          ([k]) => !["connection", "accept-encoding"].includes(k.toLocaleLowerCase()),
        ),
      }),
    );
    return new Response(response.body, {
      ...response,
      status: response.status,
      statusText: response.statusText,
      headers: [...response.headers.entries()].filter(
        ([k]) =>
          !["content-encoding", "transfer-encoding", "content-length"].includes(
            k.toLocaleLowerCase(),
          ),
      ),
    });

haochenx · 2024-11-06T07:29:51Z

also, on node v22.11.0, it seems that duplex: "half" is needed on the RequestInit passed to fetch when the incoming request contains body (e.g. a POST request)

usualoma · 2024-11-07T01:27:48Z

@haochenx Thank you!

Note: We probably need to delete “Hop-by-hop Headers."
https://datatracker.ietf.org/doc/html/rfc2616#section-13.5.1

john-griffin · 2024-11-19T16:19:47Z

Looking forward to this one, we use http-proxy-middleware in express and it's super helpful.

yusukebe · 2024-11-19T22:20:04Z

@john-griffin Thanks for the information! We can refer to it.

jbergstroem · 2025-01-02T19:29:18Z

Happy to sponsor to see this land; it would help me migrate an app to Hono.

yusukebe · 2025-01-19T09:11:14Z

Hi @usualoma

Sorry for my super late response!

I've said, "Proxy helper is good," but is there any reason not to make it as middleware? I think making it as middleware is good because the user should not pass the params such as c.req.raw:

import { Hono } from 'hono'
import { createProxy } from 'hono/proxy'

const app = new Hono()

app.use(
  '/proxy/*',
  createProxy({
    target: 'https://example.com/',
  })
)

This API is inspired by http-proxy-middleware. What do you think of this?

usualoma · 2025-01-20T07:25:38Z

Hi @yusukebe

Well, proxying is essentially a pretty dangerous operation, and if the implementation is such that c.req.raw is forwarded by default, I think that if you use it carelessly, your cookies and Authorization headers will leak to external sites.

I didn't want that to happen, so I made it so that you could ‘explicitly see that c.req.raw is being passed’.

That said, in general, proxy servers pass things on as they are, so I think it's fine to say that if there's an accident, it's the developer's responsibility.

My concern is that if this is provided as middleware, there will be cases where it would have been sufficient (and safe) to simply use fetch

app.get(‘/’, () => fetch(‘https://example.com/’))

there will be users who use middleware without thinking carefully and pass on unnecessary information.

With this PR's implementation, if there is no need to pass c.req.raw, it is not necessary to do so. The aim is to solve issues such as range when using fetch to proxy.

usualoma · 2025-01-20T07:25:49Z

This was the idea behind the creation of this PR, but even so, I don't think that explicitly passing c.req.raw makes it any more secure.

There is no problem with proxying to the backend you manage, so if the use case you have in mind is something like that, there is no problem even if c.req.raw is forwarded, and as you pointed out, I think it would be a good API design to implement it as middleware.

yusukebe · 2025-01-20T08:02:22Z

@usualoma

Thank you for the explanation! I was able to understand your concern well. I'll consider it.

yusukebe · 2025-01-20T08:37:28Z

src/helper/proxy/index.ts

+ * @example
+ * ```ts
+ * app.get('/proxy/:path', (c) => {
+ *   return proxyFetch(new Request(`http://${originServer}/${c.req.param('path')}`, c.req.raw), {


I like not using new Request() style like this:

app.get('/proxy/:path', (c) => { return proxyFetch(`https://example.com/${c.req.param('path')}`, { ...c.req.raw, proxySetRequestHeaders: { 'X-Forwarded-For': '127.0.0.1', }, }) })

Then, if you have the following code:

app.get('/proxy/:path', (c) => proxyFetch( new Request(`https://example.com/${c.req.param('path')}`, { headers: { 'X-Request-Id': '123', 'Accept-Encoding': 'gzip', }, }), { proxyDeleteResponseHeaderNames: ['X-Response-Id'], } ) )

you can also write it with not using new Request() style:

app.get('/proxy/:path', (c) => proxyFetch(`https://example.com/${c.req.param('path')}`, { headers: { 'X-Request-Id': '123', 'Accept-Encoding': 'gzip', }, proxyDeleteResponseHeaderNames: ['X-Response-Id'], }) )

These are short and user does not have to call new Request(). Both are okay, but I would like to write all cases in the code or examples with not using new Request() style. What do you think of it?

@yusukebe
Sorry for the late reply. I understand your point.

I also think that it would be better not to show the raw part of c.req.raw if possible. I think that there is a risk in the statement that ‘all headers are transferred by default’, but I think that it would be best not to write raw.

And, because proxySetRequestHeaders and proxyDeleteResponseHeaderNames are also difficult to understand, I feel that it would be good to be able to write the following for proxies for GET requests.

app.get('/proxy/:path', (c) => { return proxyFetch(`http://${originServer}/${c.req.param('path')}`, { headers: { ...c.req.header(), // optional, specify only when header forwarding is truly necessary. 'X-Forwarded-For': '127.0.0.1', 'X-Forwarded-Host': c.req.header('host'), Authorization: undefined, // do not propagate request headers contained in c.req.header('Authorization') }, }).then((res) => { res.headers.delete('Cookie') return res }) })

I wish it were possible to write a proxy for arbitrary requests, including POST, in a simpler way.

yusukebe · 2025-01-20T08:47:26Z

Hey @usualoma

I've considered it, and I found out that this Proxy Helper is so good! This is because it is just a wrapper of fetch but will resolve @haochenx 's problem simply. As you said, the help does not always need c.req.raw or a Context, so this should not be middleware.

I've left a comment. Please check it!

usualoma · 2025-01-26T02:51:18Z

Hi @yusukebe

I've reconsidered the API, and I think it would be good to add b6eb510 and fb54227 so that it can be written as follows.

I was worried about confusion with the Proxy object when I was writing this issue, but I think it's fine now, and I think it's easier to write with a short name.

app.get('/proxy/:path', (c) =>
  proxy(`http://${originServer}/${c.req.param('path')}`, {
    headers: {
      ...c.req.header(), // optional, specify only when forwarding all the request data (including credentials) is necessary.
      'X-Forwarded-For': '127.0.0.1',
      'X-Forwarded-Host': c.req.header('host'),
      Authorization: undefined, // do not propagate request headers contained in c.req.header('Authorization')
    },
  }).then((res) => {
    res.headers.delete('Cookie')
    return res
  })

app.any('/proxy/:path', (c) =>
  proxy(`http://${originServer}/${c.req.param('path')}`, {
    ...c.req, // optional, specify only when forwarding all the request data (including credentials) is necessary
    headers: {
      ...c.req.header(),
      'X-Forwarded-For': '127.0.0.1',
      'X-Forwarded-Host': c.req.header('host'),
      Authorization: undefined, // do not propagate request headers contained in c.req.header('Authorization')
    },
  })

usualoma · 2025-01-26T03:14:08Z

dc24de3 also allows you to write as follows.

app.any('/proxy/:path', (c) => proxy(`http://${originServer}/${c.req.param('path')}`, c.req))

usualoma · 2025-01-26T03:18:31Z

But now that I think about it, since serveStatic is middleware, it might be appropriate to think of proxy as middleware too.

usualoma · 2025-01-26T12:07:38Z

No, I was wrong, serveStatic wasn't in the list of middleware.

If that's the case, I think it's more appropriate to think of proxy in this category as a helper rather than middleware, so I think the way of this PR, which can simply replace the fetch API, is good.

usualoma · 2025-01-26T12:34:24Z

usualoma · 2025-01-26T12:41:38Z

It is now ready to be merged.

yusukebe · 2025-01-26T12:48:48Z

@usualoma Thanks! I'll check it tomorrow.

usualoma · 2025-01-27T00:04:53Z

🙇 One refactoring commit has been added.

2e8e250

usualoma added 2 commits October 30, 2024 21:40

feat(helper/proxy): introduce proxy helper

dcfa1a5

chore(helper/proxy): expose proxy helper

64cdd41

usualoma mentioned this pull request Oct 30, 2024

Proxying compressed response do not work out of the box #3518

Open

test(helper/proxy): fix test name

ad17d90

haochenx reviewed Oct 30, 2024

View reviewed changes

usualoma and others added 2 commits October 31, 2024 06:27

fix(helper/proxy): return Content-Range header as it is

5998770

Co-authored-by: Haochen M. Kotoi-Xie <[email protected]>

refactor(helper/proxy): return the original content-length if the res…

3a5ef0f

…ponse is uncompressed

haochenx approved these changes Nov 1, 2024

View reviewed changes

yusukebe mentioned this pull request Nov 9, 2024

[Document] Highlight Cloudflare Workers Environment Differences #3646

Closed

This was referenced Nov 9, 2024

behind a reverse proxy, X-Forwarded-Proto isn't used and plain http is used for the signin action honojs/middleware#537

Closed

Potential Security Vulnerability in X-Forwarded-* Headers for auth-js Introduced in #549 honojs/middleware#817

Open

yusukebe added the v4.7 label Jan 20, 2025

yusukebe reviewed Jan 20, 2025

View reviewed changes

usualoma added 2 commits January 26, 2025 11:39

feat(middleware): enable to pass ...c.req to init options

b6eb510

refactor(middleware/proxy): rename proxyFetch to proxy

fb54227

docs(helper/proxy): update example

e08c602

usualoma added 2 commits January 26, 2025 11:57

docs(helper/proxy): fix typo

eafc02a

refactor(helper/proxy): also accept HonoRequest instance as request init

dc24de3

fix(helper/proxy): remove hop-by-hop headers

833b8e2

Merge branch 'main' into feat-helper-proxy

d1ed0bd

refactor(helper/proxy): build request init from request

2e8e250

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(helper/proxy): introduce proxy helper #3589

feat(helper/proxy): introduce proxy helper #3589

usualoma commented Oct 30, 2024

codecov bot commented Oct 30, 2024 •

edited

Loading

haochenx left a comment •

edited

Loading

haochenx Oct 30, 2024

usualoma Oct 30, 2024

usualoma Oct 30, 2024

haochenx Nov 1, 2024

usualoma Nov 1, 2024

haochenx left a comment

yusukebe commented Nov 5, 2024

haochenx commented Nov 6, 2024 •

edited

Loading

haochenx commented Nov 6, 2024

usualoma commented Nov 7, 2024

john-griffin commented Nov 19, 2024

yusukebe commented Nov 19, 2024

jbergstroem commented Jan 2, 2025

yusukebe commented Jan 19, 2025

usualoma commented Jan 20, 2025

usualoma commented Jan 20, 2025

yusukebe commented Jan 20, 2025

yusukebe Jan 20, 2025

usualoma Jan 26, 2025

yusukebe commented Jan 20, 2025

usualoma commented Jan 26, 2025 •

edited

Loading

usualoma commented Jan 26, 2025

usualoma commented Jan 26, 2025

usualoma commented Jan 26, 2025

usualoma commented Jan 26, 2025

usualoma commented Jan 26, 2025

yusukebe commented Jan 26, 2025

usualoma commented Jan 27, 2025

feat(helper/proxy): introduce proxy helper #3589

Are you sure you want to change the base?

feat(helper/proxy): introduce proxy helper #3589

Conversation

usualoma commented Oct 30, 2024

Naming

Usage

The author should do the following, if applicable

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

haochenx left a comment • edited Loading

Choose a reason for hiding this comment

haochenx Oct 30, 2024

Choose a reason for hiding this comment

usualoma Oct 30, 2024

Choose a reason for hiding this comment

Content-Length

Content-Range

usualoma Oct 30, 2024

Choose a reason for hiding this comment

haochenx Nov 1, 2024

Choose a reason for hiding this comment

usualoma Nov 1, 2024

Choose a reason for hiding this comment

haochenx left a comment

Choose a reason for hiding this comment

yusukebe commented Nov 5, 2024

haochenx commented Nov 6, 2024 • edited Loading

haochenx commented Nov 6, 2024

usualoma commented Nov 7, 2024

john-griffin commented Nov 19, 2024

yusukebe commented Nov 19, 2024

jbergstroem commented Jan 2, 2025

yusukebe commented Jan 19, 2025

usualoma commented Jan 20, 2025

usualoma commented Jan 20, 2025

yusukebe commented Jan 20, 2025

yusukebe Jan 20, 2025

Choose a reason for hiding this comment

usualoma Jan 26, 2025

Choose a reason for hiding this comment

yusukebe commented Jan 20, 2025

usualoma commented Jan 26, 2025 • edited Loading

usualoma commented Jan 26, 2025

usualoma commented Jan 26, 2025

usualoma commented Jan 26, 2025

usualoma commented Jan 26, 2025

usualoma commented Jan 26, 2025

yusukebe commented Jan 26, 2025

usualoma commented Jan 27, 2025

codecov bot commented Oct 30, 2024 •

edited

Loading

haochenx left a comment •

edited

Loading

haochenx commented Nov 6, 2024 •

edited

Loading

usualoma commented Jan 26, 2025 •

edited

Loading