-
Notifications
You must be signed in to change notification settings - Fork 493
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(cache): try decode path #2658
base: v2
Are you sure you want to change the base?
Conversation
src/runtime/internal/cache.ts
Outdated
@@ -221,7 +221,7 @@ export function defineCachedEventHandler< | |||
const _path = | |||
event.node.req.originalUrl || event.node.req.url || event.path; | |||
const _pathname = | |||
escapeKey(decodeURI(parseURL(_path).pathname)).slice(0, 16) || "index"; | |||
escapeKey(decodePath(parseURL(_path).pathname)).slice(0, 16) || "index"; |
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.
This is arguably less safe because decodePath
falls back to user input (which can be dangerous with unseen paths).
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 might have a local try catch system that removes (fallback to empty string or "??" or something like tat) in case of decode failure.
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.
So my original approach looked like:
let _pathname: string;
try {
_pathname = escapeKey(decodeURI(parseURL(_path).pathname)).slice(0, 16) || "index"
} catch {
throw createError({
message: 'Invalid URL',
statusCode: 404,
});
}
But then I changed to match the behavior of the static handler. Would something like that work better?
We might have a local try catch system that removes (fallback to empty string or "??" or something like tat) in case of decode failure.
I don't think we can use a common fallback because then the cache key will be identical for different paths. If the handler that is wrapped by the cache handler doesn't mind the invalid encoding it may return different responses for different paths and expect them to be cached separately.
But maybe there's another way to mitigate the risk? Could you explain where the risk is since I'm not sure I understand? We are already using the user input to generate the cache key when URI decoding does not fail.
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.
the cache key will be identical for different paths
Full path is always added as hash (L225). _pathname
is only a human friendly part to assist identify keys.
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.
Oh, I see now. So you're thinking something like this?
let _pathname: string;
try {
_pathname = escapeKey(decodeURI(parseURL(_path).pathname)).slice(0, 16) || "index"
} catch {
_pathname = "??"
}
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.
Yes.. Althoigh i guess ?
is illegal in windows. Perhaps -
?
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.
Tested it out locally and this approach works as well. π Thanks for the feedback!
π Linked issue
#2657
β Type of change
π Description
This follows the same approach as #1459, replacing the
decodeURI
withdecodePath
fromufo
. This means that an invalid path will no longer throw an error but instead use the undecoded text.π Checklist