feat(serve-static): support absolute root #3420

yusukebe · 2024-09-17T07:54:30Z

This PR enables the serve static middleware to support an absolute path for root. This change affects the serveStatic from Bun and Deno adapter.

import { serveStatic } from 'hono/bun'

// ...

app.all(
  '/static/*',
  serveStatic({ root: '/home/hono/app/static', allowAbsoluteRoot: true })
)

To use an absolute path for root, you should set allowAbsoluteRoot as true. This is because it prevents security issues. If the user does not know a root can have an absolute path, an intended string that includes an absolute path is set; it will cause unexpected behavior for them. And using an absolute path has a risk of accessing the whole of the system. So, setting the flag explicitly is a good design.

We should implement the same feature for the Node.js adapter later.

Related to #3383 #3108

Closes #3107

The author should do the following, if applicable

Add tests
Run tests
bun run format:fix && bun run lint:fix to format the code
Add TSDoc/JSDoc to document the code

codecov · 2024-09-17T07:56:36Z

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.78%. Comparing base (dfbd717) to head (6365ba9).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
src/adapter/deno/serve-static.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3420   +/-   ##
=======================================
  Coverage   95.77%   95.78%           
=======================================
  Files         155      155           
  Lines        9310     9325   +15     
  Branches     2725     2808   +83     
=======================================
+ Hits         8917     8932   +15     
  Misses        393      393

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yusukebe · 2024-09-20T10:08:10Z

Hey @usualoma ! Can you review this?

usualoma

I think it's a good change!

However, although it will be a breaking change, I think that for users who are using it for the first time, they will expect that specifying it as follows will result in an absolute path, so I think it is a difficult question as to whether or not allowAbsoluteRoot is necessary.

serveStatic({ root: '/home/hono/app/static' })

runtime-tests/deno/middleware.test.tsx

usualoma · 2024-09-20T11:49:45Z

src/utils/filepath.ts

+ // assets => /assets
+ path = path.replace(/^(?!\/)/, '/')
+ // Using URL to normalize the path.
+ const url = new URL(`file://${path}`)


What was the purpose of using URL? (Sorry, I don't understand.)

With the current code, the following results will be obtained, so I think some kind of modification is necessary.

getFilePathWithoutDefaultDocument({ filename: '/%2e%2e/%2e%2e/%2e%2e/%2e%2e/etc/passwd', root: '/p/p2', allowAbsoluteRoot: true, }) // /etc/passwd

It uses URL to normalize paths like .., but as you said, it's not good! I'll change it.

@usualoma

What about this change? 5f07dd7

Thank you!

If the necessary requirements are as follows,

root : .. refers to the directory above

path: %2e%2e should be left as is

The following code would also achieve this, and I think it would be more performant than resolving .. for each request. Is it possible to meet the requirements in this way?

diff --git a/src/middleware/serve-static/index.ts b/src/middleware/serve-static/index.ts index 49cbcd3c..3517a2e7 100644 --- a/src/middleware/serve-static/index.ts +++ b/src/middleware/serve-static/index.ts @@ -40,6 +40,8 @@ export const serveStatic = <E extends Env = Env>( isDir?: (path: string) => boolean | undefined | Promise<boolean | undefined> } ): MiddlewareHandler => { + const root = new URL(`file://${options.root}`).pathname + return async (c, next) => { // Do nothing if Response is already set if (c.finalized) { @@ -49,7 +51,6 @@ export const serveStatic = <E extends Env = Env>( let filename = options.path ?? decodeURI(c.req.path) filename = options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename - const root = options.root const allowAbsoluteRoot = options.allowAbsoluteRoot ?? false

Ahhhh, goood idea!

@usualoma

I updated it. What do you think of the middleware/serve-static/index.ts?

Well, but

new URL(`file://${options.root}`).pathname

I think the behavior will change if you specify a relative path that includes the parent directory.

serveStatic({ root: '../relative/from/parent' })

The following method may be better.

let root: string = options.root if (root && root.startsWith('/')) { isAbsoluteRoot = true root = new URL(`file://${root}`).pathname }

@usualoma

I've updated the code: 6365ba9

This change includes the fix for #3420 (comment) and added some tests. Could you review it?

usualoma · 2024-09-20T12:57:33Z

src/utils/filepath.ts

@@ -42,6 +44,9 @@ export const getFilePathWithoutDefaultDocument = (
 // /foo.html => foo.html
 filename = filename.replace(/^\.?[\/\\]/, '')


I think it would be better to use /^\.?[\/\\]+/ because it is very dangerous if a / remains at the beginning due to the following changes.

https://github.com/honojs/hono/pull/3420/files#diff-ab73a967275a45e7a4ef6280ce16d384179d552efc1095f067f3a43706fb189eR12

Suggested change

filename = filename.replace(/^\.?[\/\\]/, '')

filename = filename.replace(/^\.?[\/\\]+/, '')

Is the following code one of the dangerous cases? You mean this test should pass, right?

// with the current implementation, the following will fail expect(getFilePathWithoutDefaultDocument({ filename: '///foo.txt' })).toBe('foo.txt')

I see, I'm sorry, the code I suggested might not be good. The case I'm worried about is as follows.

import { serveStatic } from './src/adapter/bun' import { Hono } from './src/hono' const app = new Hono() app.use('/static/*', serveStatic({ root: './' })) app.use('/favicon.ico', serveStatic({ path: './favicon.ico' })) app.get('/', (c) => c.text('You can access: /static/hello.txt')) app.get('*', serveStatic({ root: '.' })) // fallback // or app.get('*', serveStatic({})) export default app

I don't think this is a realistic setting, but it's not an invalid setting, and I think the relative path from the application directory will result in the expected result. With the current code in the feat/serve-static-absolute-root branch, if you access http://localhost:3000///etc/passwd, you can access any path from the absolute path using directory traversal.

I think the following changes will also prevent directory traversal.

diff --git a/src/utils/filepath.ts b/src/utils/filepath.ts index f3f2ea82..61471483 100644 --- a/src/utils/filepath.ts +++ b/src/utils/filepath.ts @@ -52,5 +52,9 @@ export const getFilePathWithoutDefaultDocument = ( let path = root ? root + '/' + filename : filename path = path.replace(/^\.?\//, '') + if (root[0] !== '/' && path[0] === '/') { + return + } + return path }

yusukebe · 2024-09-21T04:11:06Z

@usualoma

The reason I prefer allowAbsoluteRoot is not just that it will introduce a breaking change; I care about the security issue. If it allows an absolute root, the serve static can access any paths in the system like /etc/passwd. Of course, the user sets the option to serve static as an application owner, not an attacker, so the risk is small. But there is a possibility of accessing it. So, to prevent it as much as possible, I think adding the allowAbsoluteRoot is better.

However, this is my current opinion. Is this too much thing?

usualoma · 2024-09-21T05:11:39Z

@yusukebe
Thanks for the explanation!

I can understand that “absolute path specifications are more likely to lead to dangerous situations”. However, on the other hand, I think that “if allowAbsoluteRoot is not specified, paths beginning with / are also processed as relative paths”, in some cases, this can be dangerous.

For example, if you wrote the following expecting it to be treated as an absolute path, but it ended up being treated as a relative path because you didn't specify allowAbsoluteRoot, the result for the user might be that “the static directly under the source code was published unintentionally”.

serveStatic({ root: '/static' })

I think that “unexpected results for users” are the most dangerous.

serveStatic({ root: '/static' }) // throw runtime error
serveStatic({ root: '/static', allowAbsoluteRoot: true }) // served with absolute path

If you specify it like this, instead of processing it as a relative path, how about making it an error at runtime?

yusukebe · 2024-09-21T07:42:36Z

@usualoma

I can understand that “absolute path specifications are more likely to lead to dangerous situations”. However, on the other hand, I think that “if allowAbsoluteRoot is not specified, paths beginning with / are also processed as relative paths”, in some cases, this can be dangerous.

You are right. It's also dangerous. The idea you mentioned below is that throwing an error is good, but I think it's okay not to implement it. It's good to allow an absolute path that starts / and don't add an allowAbsoluteRoot option.

I'll change the code later. Thank you!

…ing `../`

yusukebe · 2024-09-22T01:30:55Z

src/middleware/serve-static/index.test.ts

+ app.get('/static/*', serveStatic)
+
+ const res = await app.request('/static/html/hello.html')
+ expect(await res.text()).toBe('Hello in ./../home/hono/static/html/hello.html')


This behavior is the same as the current main one.

usualoma

Thank you.
LGTM!

feat(serve-static): support absolute root

9e761e2

yusukebe added 3 commits September 17, 2024 17:08

add bun runtime test

46346a4

don't use Request

5d7a2af

add code for deno

2835570

yusukebe marked this pull request as ready for review September 20, 2024 10:06

usualoma reviewed Sep 20, 2024

View reviewed changes

yusukebe added 2 commits September 21, 2024 15:43

remove unnecessay console.log

a93839a

use normalizeFilePath instead of URL

5f07dd7

yusukebe force-pushed the feat/serve-static-absolute-root branch from c055a21 to 613b3c1 Compare September 21, 2024 11:34

use URL for options.root

5dd59c5

yusukebe force-pushed the feat/serve-static-absolute-root branch 3 times, most recently from e33a4cb to a4ecbeb Compare September 22, 2024 01:21

don't allow directory traversal and fix the behavior when root includ…

6365ba9

…ing `../`

yusukebe force-pushed the feat/serve-static-absolute-root branch from a4ecbeb to 6365ba9 Compare September 22, 2024 01:28

yusukebe commented Sep 22, 2024

View reviewed changes

usualoma approved these changes Sep 22, 2024

View reviewed changes

yusukebe added the approved label Sep 22, 2024

This was referenced Sep 23, 2024

Unable to setup fallback when using parent directory in path #3242

Open

Bun "serveStatic" doesn't seem to serve at url #2200

Closed

yusukebe merged commit 246a06a into main Sep 24, 2024
14 checks passed

yusukebe deleted the feat/serve-static-absolute-root branch September 24, 2024 03:23

oon00b mentioned this pull request Sep 24, 2024

fix(serveStatic): support serving file from absolute path for Deno and Bun #3108

Closed

4 tasks

This was referenced Sep 26, 2024

Adding support to absolute path. Tested on Bun. #3383

Closed

feat(serve-static): support absolute path for root honojs/node-server#202

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(serve-static): support absolute root #3420

feat(serve-static): support absolute root #3420

yusukebe commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

yusukebe commented Sep 20, 2024

usualoma left a comment

usualoma Sep 20, 2024

yusukebe Sep 21, 2024

yusukebe Sep 21, 2024 •

edited

Loading

usualoma Sep 21, 2024

yusukebe Sep 21, 2024

yusukebe Sep 21, 2024

usualoma Sep 21, 2024

usualoma Sep 21, 2024

yusukebe Sep 22, 2024

usualoma Sep 20, 2024

yusukebe Sep 21, 2024

usualoma Sep 21, 2024

usualoma Sep 21, 2024

yusukebe commented Sep 21, 2024

usualoma commented Sep 21, 2024

yusukebe commented Sep 21, 2024

yusukebe Sep 22, 2024

usualoma left a comment

		@@ -42,6 +44,9 @@ export const getFilePathWithoutDefaultDocument = (
		// /foo.html => foo.html
		filename = filename.replace(/^\.?[\/\\]/, '')

	filename = filename.replace(/^\.?[\/\\]/, '')
	filename = filename.replace(/^\.?[\/\\]+/, '')

feat(serve-static): support absolute root #3420

feat(serve-static): support absolute root #3420

Conversation

yusukebe commented Sep 17, 2024 • edited Loading

The author should do the following, if applicable

codecov bot commented Sep 17, 2024 • edited Loading

Codecov Report

yusukebe commented Sep 20, 2024

usualoma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusukebe Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusukebe commented Sep 21, 2024

usualoma commented Sep 21, 2024

yusukebe commented Sep 21, 2024

Choose a reason for hiding this comment

usualoma left a comment

Choose a reason for hiding this comment

yusukebe commented Sep 17, 2024 •

edited

Loading

codecov bot commented Sep 17, 2024 •

edited

Loading

yusukebe Sep 21, 2024 •

edited

Loading