-
Notifications
You must be signed in to change notification settings - Fork 78
feat(serve-static): support absolute path #257
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
Changes from 13 commits
8500774
cbfed14
2e9b4e9
e420e47
9a14f20
33f3ccf
22d69c0
54ecac3
d5c1002
52bd0e6
ee6159c
1737089
ff18658
f6f5b90
8f15053
d26e6ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,8 +1,8 @@ | ||||||
| import type { Context, Env, MiddlewareHandler } from 'hono' | ||||||
| import { getFilePath, getFilePathWithoutDefaultDocument } from 'hono/utils/filepath' | ||||||
| import { getMimeType } from 'hono/utils/mime' | ||||||
| import { createReadStream, lstatSync } from 'node:fs' | ||||||
| import type { ReadStream, Stats } from 'node:fs' | ||||||
| import { createReadStream, lstatSync } from 'node:fs' | ||||||
| import { join, resolve } from 'node:path' | ||||||
|
|
||||||
| export type ServeStaticOptions<E extends Env = Env> = { | ||||||
| /** | ||||||
|
|
@@ -44,10 +44,6 @@ const createStreamBody = (stream: ReadStream) => { | |||||
| return body | ||||||
| } | ||||||
|
|
||||||
| const addCurrentDirPrefix = (path: string) => { | ||||||
| return `./${path}` | ||||||
| } | ||||||
|
|
||||||
| const getStats = (path: string) => { | ||||||
| let stats: Stats | undefined | ||||||
| try { | ||||||
|
|
@@ -60,6 +56,9 @@ const getStats = (path: string) => { | |||||
| export const serveStatic = <E extends Env = any>( | ||||||
| options: ServeStaticOptions<E> = { root: '' } | ||||||
| ): MiddlewareHandler<E> => { | ||||||
| const optionRoot = options.root || '.' | ||||||
yusukebe marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| const optionPath = options.path | ||||||
|
|
||||||
| return async (c, next) => { | ||||||
| // Do nothing if Response is already set | ||||||
| if (c.finalized) { | ||||||
|
|
@@ -69,35 +68,44 @@ export const serveStatic = <E extends Env = any>( | |||||
| let filename: string | ||||||
|
|
||||||
| try { | ||||||
| filename = options.path ?? decodeURIComponent(c.req.path) | ||||||
| const rawPath = optionPath ?? c.req.path | ||||||
| // Prevent encoded path traversal attacks | ||||||
| if (!optionPath) { | ||||||
| const decodedPath = decodeURIComponent(rawPath) | ||||||
| if (decodedPath.includes('..')) { | ||||||
| await options.onNotFound?.(rawPath, c) | ||||||
| return next() | ||||||
| } | ||||||
| } | ||||||
| filename = optionPath ?? decodeURIComponent(c.req.path) | ||||||
| } catch { | ||||||
| await options.onNotFound?.(c.req.path, c) | ||||||
| return next() | ||||||
| } | ||||||
|
|
||||||
| let path = getFilePathWithoutDefaultDocument({ | ||||||
| filename: options.rewriteRequestPath ? options.rewriteRequestPath(filename, c) : filename, | ||||||
| root: options.root, | ||||||
| }) | ||||||
| const requestPath = options.rewriteRequestPath | ||||||
| ? options.rewriteRequestPath(filename, c) | ||||||
| : filename | ||||||
| const rootResolved = resolve(optionRoot) | ||||||
| let path: string | ||||||
|
|
||||||
| if (path) { | ||||||
| path = addCurrentDirPrefix(path) | ||||||
| if (optionPath) { | ||||||
| // Use path option directly if specified | ||||||
| path = resolve(optionPath) | ||||||
|
||||||
| path = resolve(optionPath) | |
| path = resolve(join(optionRoot, optionPath)) |
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.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
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.
👍
path.startsWith(rootResolved)
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import { Hono } from 'hono' | ||
|
|
||
| import request from 'supertest' | ||
| import path from 'node:path' | ||
| import { serveStatic } from './../src/serve-static' | ||
| import { createAdaptorServer } from './../src/server' | ||
|
|
||
|
|
@@ -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( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To simplify implementation, make the path passed to app.use(
'/static/*',
serveStatic({
root: './test/assets',
onFound: (path, c) => {
// path is an absolute path
c.header('X-Custom', `Found the file at ${path}`)
},
})
) |
||
| /Found the file at .*[\/\\]test[\/\\]assets[\/\\]static[\/\\]index\.html$/ | ||
| ) | ||
| }) | ||
|
|
||
| it('Should return hono.html', async () => { | ||
|
|
@@ -167,8 +169,8 @@ describe('Serve Static Middleware', () => { | |
| it('Should handle the `onNotFound` option', async () => { | ||
| const res = await request(server).get('/on-not-found/foo.txt') | ||
| expect(res.status).toBe(404) | ||
| expect(notFoundMessage).toBe( | ||
| './not-found/on-not-found/foo.txt is not found, request to /on-not-found/foo.txt' | ||
| expect(notFoundMessage).toMatch( | ||
yusukebe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /.*[\/\\]not-found[\/\\]on-not-found[\/\\]foo\.txt is not found, request to \/on-not-found\/foo\.txt$/ | ||
| ) | ||
| }) | ||
|
|
||
|
|
@@ -226,4 +228,64 @@ describe('Serve Static Middleware', () => { | |
| expect(res.headers['vary']).toBeUndefined() | ||
| expect(res.text).toBe('Hello Not Compressed') | ||
| }) | ||
|
|
||
| describe('Absolute path', () => { | ||
| const rootPaths = [ | ||
| path.join(__dirname, 'assets'), | ||
| __dirname + path.sep + '..' + path.sep + 'test' + path.sep + 'assets', | ||
| ] | ||
| rootPaths.forEach((root) => { | ||
| describe(root, () => { | ||
| const app = new Hono() | ||
| const server = createAdaptorServer(app) | ||
| app.use('/static/*', serveStatic({ root })) | ||
| app.use('/favicon.ico', serveStatic({ path: root + path.sep + 'favicon.ico' })) | ||
|
|
||
| it('Should return index.html', async () => { | ||
| const res = await request(server).get('/static') | ||
| expect(res.status).toBe(200) | ||
| expect(res.headers['content-type']).toBe('text/html; charset=utf-8') | ||
| expect(res.text).toBe('<h1>Hello Hono</h1>') | ||
| }) | ||
|
|
||
| it('Should return correct headers and data for text', async () => { | ||
| const res = await request(server).get('/static/plain.txt') | ||
| expect(res.status).toBe(200) | ||
| expect(res.headers['content-type']).toBe('text/plain; charset=utf-8') | ||
| expect(res.text).toBe('This is plain.txt') | ||
| }) | ||
| it('Should return correct headers for icons', async () => { | ||
| const res = await request(server).get('/favicon.ico') | ||
| expect(res.status).toBe(200) | ||
| expect(res.headers['content-type']).toBe('image/x-icon') | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe('Security tests', () => { | ||
| const app = new Hono() | ||
| const server = createAdaptorServer(app) | ||
| app.use('/static/*', serveStatic({ root: './test/assets' })) | ||
|
|
||
| it('Should prevent path traversal attacks with double dots', async () => { | ||
| const res = await request(server).get('/static/../secret.txt') | ||
| expect(res.status).toBe(404) | ||
| }) | ||
|
|
||
| it('Should prevent path traversal attacks with multiple levels', async () => { | ||
| const res = await request(server).get('/static/../../package.json') | ||
| expect(res.status).toBe(404) | ||
| }) | ||
|
|
||
| it('Should prevent path traversal attacks with mixed separators', async () => { | ||
| const res = await request(server).get('/static/..\\..\\package.json') | ||
| expect(res.status).toBe(404) | ||
| }) | ||
|
|
||
| it('Should prevent path traversal attacks with encoded dots', async () => { | ||
| const res = await request(server).get('/static/%2e%2e%2fsecret.txt') | ||
| expect(res.status).toBe(404) | ||
| }) | ||
| }) | ||
| }) | ||

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.
To support Windows on CI.