-
-
Notifications
You must be signed in to change notification settings - Fork 913
feat(serve-static): use join to correct path resolution
#4291
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
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #4291 +/- ##
==========================================
- Coverage 91.61% 91.51% -0.10%
==========================================
Files 170 171 +1
Lines 10875 10776 -99
Branches 3099 3085 -14
==========================================
- Hits 9963 9862 -101
- Misses 911 913 +2
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| it('Should return index.html', async () => { | ||
| // Serve static on Cloudflare Workers cannot determine whether the target path is a directory or not | ||
| it.skip('Should return index.html', async () => { |
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.
In the previous implementation, if the file was not found, it checked whether index.html existed. This PR, that process has been removed.
Since serve static for Cloudflare Workers is deprecated and not planned to be used, this change is acceptable.
|
Hey @usualoma ! What do you think of this? If it makes sense for you, please review it! |
|
Hi @yusukebe |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -1,3 +1,4 @@ | |||
| import { join } from 'node:path' | |||
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 can use @std/path, but it's better to use node:path because it's built into Deno.
| expect(await res.text()).toBe('Hello in ./static/sub/index.html') | ||
| }) | ||
|
|
||
| it('Should return 200 response - /static/helloworld', async () => { |
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 test is the same as Should return 200 response - /static/sub?? We may remove this.
|
Hi @usualoma Thank you for the accurate comment! For Deno, as you said, we can use For the separators issue, I made What do you think of this? |
|
Hi @usualoma If you already realized this, sorry for that. I updated this PR. Can you review this? |
|
Hi @yusukebe Thanks for the update. The content makes sense! As it stands, the frequency of defaultJoin usage is very low, so I think it's important to keep the code size small, so I think it would be better to do the following. diff --git i/src/middleware/serve-static/path.ts w/src/middleware/serve-static/path.ts
index b8ae7986..0253cf5d 100644
--- i/src/middleware/serve-static/path.ts
+++ w/src/middleware/serve-static/path.ts
@@ -3,39 +3,23 @@
* If you need Windows path support, please use `join` exported from `node:path` etc. instead.
*/
export const defaultJoin = (...paths: string[]): string => {
- if (paths.length === 0) {
- return '.'
- }
- if (paths.length === 1 && paths[0] === '') {
- return '.'
- }
-
// Join non-empty paths with '/'
let result = paths.filter((p) => p !== '').join('/')
// Normalize multiple slashes to single slash
- result = result.replace(/\/+/g, '/')
+ result = result.replace(/(?<=\/)\/+/g, '')
// Handle path resolution (. and ..)
const segments = result.split('/')
const resolved = []
- const isAbsolute = result.startsWith('/')
for (const segment of segments) {
- if (segment === '' || segment === '.') {
- continue
- }
- if (segment === '..') {
- if (resolved.length > 0 && resolved[resolved.length - 1] !== '..') {
- resolved.pop()
- } else if (!isAbsolute) {
- resolved.push('..')
- }
- } else {
+ if (segment === '..' && resolved.length > 0 && resolved.at(-1) !== '..') {
+ resolved.pop()
+ } else if (segment !== '.') {
resolved.push(segment)
}
}
- const final = resolved.join('/')
- return isAbsolute ? '/' + final : final || '.'
+ return resolved.join('/') || '.'
} |
Co-authored-by: Taku Amano <[email protected]>
Bundle size check
Compiler Diagnostics (tsc)
Compiler Diagnostics (typescript-go)
Reported by octocov |
HTTP Performance Benchmark
|
|
Hi @yusukebe |
|
@usualoma Thanks! |
This PR introduces a new mechanism for the serve static middleware. This can fix unintended path resolution.
pathResolveoptionjoinoptionsrc/middleware/serve-static/index.tsdefaultJoin. It is used if thejoinoption is not specifiedIt is inspired by honojs/node-server#261
Problems
In previous implementations, paths starting with
C:\Users\yusuke\on Windows were converted to/Users/yusuke. This caused unintended behavior because the drive name was lost.If we use a function such as
joinexported bypath:node, we can solve this problem and simplify the code. ThepathResolveoption is also unnecessary.Breaking changes?
I changed the test code, but only
pathpassed toonFoundandonNotFoundhas been slightly changed.Other behaviors remain unchanged. Additionally, unexpected path resolution issues should be resolved immediately. Release a minor version without changing the major version.
The author should do the following, if applicable
bun run format:fix && bun run lint:fixto format the code