feat(helper/proxy): introduce proxy helper #3589
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3589 +/- ##
==========================================
+ Coverage 91.32% 91.37% +0.05%
==========================================
Files 161 163 +2
Lines 10242 10308 +66
Branches 2889 2874 -15
==========================================
+ Hits 9353 9419 +66
Misses 888 888
Partials 1 1 ☔ View full report in Codecov by Sentry. |
src/helper/proxy/index.ts
Outdated
| * * Content-Length | ||
| * * Content-Range | ||
| */ | ||
| const forceDeleteResponseHeaderNames = ['Content-Encoding', 'Content-Length', 'Content-Range'] |
There was a problem hiding this comment.
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
Rangeheader, fast-fail with 400 status code - strip
Accept-Rangefrom response header - indicate upstream error if response header contain
Content-Range
Content-Length should be left alone in all case IMO
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Hey @usualoma ! Thank you for the PR. I'll try to use this and comment later. |
|
@usualoma @yusukebe many thanks for your amazing works again! just fyi we ended up using a pre/process combo like the following to mitigate
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(),
),
),
}); |
|
also, on node v22.11.0, it seems that |
|
@haochenx Thank you! Note: We probably need to delete “Hop-by-hop Headers." |
|
Looking forward to this one, we use |
|
@john-griffin Thanks for the information! We can refer to it. |
|
Happy to sponsor to see this land; it would help me migrate an app to Hono. |
|
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 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 |
|
Hi @yusukebe Well, proxying is essentially a pretty dangerous operation, and if the implementation is such that I didn't want that to happen, so I made it so that you could ‘explicitly see that 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 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 |
|
This was the idea behind the creation of this PR, but even so, I don't think that explicitly passing 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 |
|
Thank you for the explanation! I was able to understand your concern well. I'll consider it. |
src/helper/proxy/index.ts
Outdated
| * @example | ||
| * ```ts | ||
| * app.get('/proxy/:path', (c) => { | ||
| * return proxyFetch(new Request(`http://${originServer}/${c.req.param('path')}`, c.req.raw), { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
|
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 I've left a comment. Please check it! |
|
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 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')
},
}) |
|
dc24de3 also allows you to write as follows. app.any('/proxy/:path', (c) => proxy(`http://${originServer}/${c.req.param('path')}`, c.req)) |
|
But now that I think about it, since |
|
No, I was wrong,
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. |
|
It is now ready to be merged. |
|
@usualoma Thanks! I'll check it tomorrow. |
|
🙇 One refactoring commit has been added. |
|
Hey @usualoma ! Looks good! The only things are about TypeScript-type issues. I've got the following type errors with the examples:
I updated the code, and I think this is good. The diff is the following: diff --git a/src/helper/proxy/index.ts b/src/helper/proxy/index.ts
index 03fd7240..6c0f87e6 100644
--- a/src/helper/proxy/index.ts
+++ b/src/helper/proxy/index.ts
@@ -3,7 +3,7 @@
* Proxy Helper for Hono.
*/
-import type { HonoRequest } from '../../request'
+import type { RequestHeader } from '../../utils/headers'
// https://datatracker.ietf.org/doc/html/rfc2616#section-13.5.1
const hopByHopHeaders = [
@@ -16,16 +16,17 @@ const hopByHopHeaders = [
'transfer-encoding',
]
-interface ProxyRequestInit extends RequestInit {
+interface ProxyRequestInit extends Omit<RequestInit, 'headers'> {
raw?: Request
+ headers?:
+ | HeadersInit
+ | [string, string][]
+ | Record<RequestHeader, string | undefined>
+ | Record<string, string | undefined>
}
interface ProxyFetch {
- (input: RequestInfo | URL, init?: ProxyRequestInit | HonoRequest): Promise<Response>
- (
- input: string | URL | globalThis.Request,
- init?: ProxyRequestInit | HonoRequest
- ): Promise<Response>
+ (input: string | URL | Request, init?: ProxyRequestInit): Promise<Response>
}
const buildRequestInitFromRequest = (
@@ -72,7 +73,7 @@ const buildRequestInitFromRequest = (
* })
* })
*
- * app.any('/proxy/:path', (c) => {
+ * app.all('/proxy/:path', (c) => {
* return proxy(`http://${originServer}/${c.req.param('path')}`, {
* ...c.req, // optional, specify only when forwarding all the request data (including credentials) is necessary.
* headers: {
@@ -88,10 +89,14 @@ const buildRequestInitFromRequest = (
export const proxy: ProxyFetch = async (input, proxyInit) => {
const { raw, ...requestInit } = proxyInit ?? {}
- const req = new Request(input, {
- ...buildRequestInitFromRequest(raw),
- ...requestInit,
- })
+ const req = new Request(
+ input,
+ // @ts-expect-error `headers` in `requestInit` is not compatible with HeadersInit
+ {
+ ...buildRequestInitFromRequest(raw),
+ ...requestInit,
+ }
+ )
req.headers.delete('accept-encoding')
const res = await fetch(req)
const res = await fetch(req)Perhaps you can revise the code patched with this, but it can fix the type error. And the interesting point is using the following types for the Record<RequestHeader, string | undefined>This can be helpful. After merging this PR, we can add |
Co-authored-by: Yusuke Wada <[email protected]>
…ved from the response header
|
@yusukebe Thank you! As you pointed out, the type was not set correctly. Can the header of the Response object we created be changed?When I came to think of it, I made the following specification for deletion without thinking much about it, but can we consider this possible? return 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
})According to MDN, the headers property itself is read-only. However, it does not say whether the headers themselves are immutable. Also, here, if we try to directly modify the headers of the However, the If we don't use this behavior, we may need to consider other ways to remove headers. However, it works fine in all the runtimes I've checked, so I think the current PR method is fine. It's also a clean API. |
|
@usualoma Thank you for handling this! As you said, the headers in the Response are immutable if fetched. // ❌️ headers are immutable
const res = await fetch('https://google.com')
res.headers.delete('server')// ⭕️ OK
const res = new Response('http://foo', { headers: { x: '1' } })
res.headers.delete('x')If the |
|
@yusukebe Thank you for checking. Let's move on with the current content. |
fixes #3518
Naming
The name
proxyFetchseems a little redundant, but I rejected the other candidates for the following reasons.proxy: The nameproxyis simple but is avoided because it is confusing with the JavaScriptProxyobject.fetch: The namefetchis also good. Although it is in thehelper/proxynamespace, so it can be distinguished, when it is used by being incorporated into the application, from the standpoint of reading the code, it looks likeglobalThis.fetchis being called, so I decided to avoid it because of the cognitive load.Usage
The author should do the following, if applicable
bun run format:fix && bun run lint:fixto format the code