feat(serve-static): support absolute path #257
Conversation
| @@ -54,7 +54,7 @@ | |||
| } | |||
| }, | |||
| "scripts": { | |||
| "test": "node --expose-gc ./node_modules/.bin/jest", | |||
| "test": "node --expose-gc node_modules/jest/bin/jest.js", | |||
There was a problem hiding this comment.
To support Windows on CI.
|
Hi @usualoma Can you review this, please? |
| @@ -68,7 +68,9 @@ describe('Serve Static Middleware', () => { | |||
| expect(res.status).toBe(200) | |||
| expect(res.text).toBe('<h1>Hello Hono</h1>') | |||
| expect(res.headers['content-type']).toBe('text/html; charset=utf-8') | |||
| expect(res.headers['x-custom']).toBe('Found the file at ./test/assets/static/index.html') | |||
| expect(res.headers['x-custom']).toMatch( | |||
There was a problem hiding this comment.
To simplify implementation, make the path passed to onFound an absolute path.
app.use(
'/static/*',
serveStatic({
root: './test/assets',
onFound: (path, c) => {
// path is an absolute path
c.header('X-Custom', `Found the file at ${path}`)
},
})
)
|
Thank you for reviewing. I made it simple using Node.js API. What do you think of this? |
src/serve-static.ts
Outdated
| path = addCurrentDirPrefix(path) | ||
| } else { | ||
| // Security check: prevent path traversal attacks | ||
| if (!optionPath && !path.startsWith(rootResolved)) { |
There was a problem hiding this comment.
👍
path.startsWith(rootResolved)
src/serve-static.ts
Outdated
| path = addCurrentDirPrefix(path) | ||
| if (optionPath) { | ||
| // Use path option directly if specified | ||
| path = resolve(optionPath) |
There was a problem hiding this comment.
In the current main branch, I think "root" is also used here.
| path = resolve(optionPath) | |
| path = resolve(join(optionRoot, optionPath)) |
There was a problem hiding this comment.
@yusukebe Thank you!
The behavior when specifying an absolute path in path will change from before.
app.use('*', serveStatic({
root: 'public',
path: '/file.html',
}))Previously, it behaved as if it joined to root. If this is an intentional change in behavior, I think it's OK.
There was a problem hiding this comment.
Co-authored-by: Taku Amano <[email protected]>
Co-authored-by: Taku Amano <[email protected]>
yusukebe
force-pushed
the
feat/serve-static-absolute-path
branch
from
18ebc11 to
8f15053
Compare
|
@usualoma Thank you for reviewing! Landing it. |
Fixies #187
Related to #203 #238
This PR enables serve-static to support absolute paths, but both
rootandpathoptions.I considered that we should move the serve-static feature to the main
honojs/honocore related to #203 honojs/hono#3483. However, if so, we need to update the peer dependencies forhonoso that we can add this feature to this project separately fromhonojs/hono.