-
Notifications
You must be signed in to change notification settings - Fork 511
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
fix: provide fetch utils to cacheEventHandler
event
#1968
Conversation
src/runtime/cache.ts
Outdated
@@ -349,6 +349,8 @@ export function defineCachedEventHandler< | |||
|
|||
// Call handler | |||
const event = createEvent(reqProxy, resProxy); | |||
event.fetch = incomingEvent.fetch; | |||
event.$fetch = incomingEvent.$fetch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shall use global fetch
and $fetch
since internal one has access to the headers and we don't want them to be leaked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated, would you mind explaining how one would leak the headers though?
As far as I can see only the headers of the response are cached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an incoming request comes with for example auth Cookies (incoming headers)(, we start rendering process with a mocked event
that has access to limited headers. So the rendered response can be shared across users. event.$fetch
inherits the context and headers (https://github.com/unjs/h3/blob/29195307e03c5d85fe965e5067bdc36e563a6344/src/utils/proxy.ts#L119). So during SSR rendering, while main headers are masked, calling event.$fetch
can wrongly cause making an internal authenticated fetch and keeping it in cache.
Probably a better solution could create a similar wrapper (https://github.com/unjs/nitro/blob/main/src/runtime/app.ts#L131) but with this new event in L351 that already masked headers so we can preserve async context, etc but not leak headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah interesting! Thank you for explaining, honestly, I would have expected something like this to be done in user-land (i.e don't cache routes that are auth'd), it's not that intuitive but I think it's a great safeguard.
For some context, I switched to using event.$fetch to solve an issue with not being able to resolve the host header when doing nested internal calls, so nice to learn more about what's going on.
I'll take a look at changing it to a wrapper tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey guys, thanks for taking a look at this! I did some testing against my project and while using incomingEvent.$fetch
fixes the issue, with globalEvent.$fetch
the context is still lost on nested requests. Would the wrapper approach fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made an attempt to use the wrapper approach here, trying to mimic how it is done in app.ts. In my testing, it appears to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Happy to close this if that solves it better
Thanks! Merged via #2066 |
π Linked issue
resolves #1848
β Type of change
π Description
I have an endpoint that may or may not be cached that is relying on
event.$fetch
for internal fetch's, when cached using route rules, it will throw an error.I don't see why the mock event can't have these, let me know if I missed something though.
π Checklist