Skip to content
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.

Sign up for GitHub

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: url handle #247

Merged
merged 5 commits into from
Oct 12, 2021
Merged

fix: url handle #247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bfc5be
fix: redirection handling
climba03003 Oct 11, 2021
d52513a
chore: remove console.log
climba03003 Oct 11, 2021
e358026
refactor: use a single for-loop
climba03003 Oct 11, 2021
aa797bc
fix: backward slash is not handle
climba03003 Oct 11, 2021
16eef57
test: 100% coverage
climba03003 Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ async function fastifyStatic (fastify, opts) {
try {
reply.redirect(301, getRedirectUrl(request.raw.url))
} catch (error) {
// the try-catch here is actually unreachable, but we keep it for safety and prevent DoS attack
/* istanbul ignore next */
reply.send(error)
}
} else {
Expand Down Expand Up @@ -447,16 +449,23 @@ function getEncodingExtension (encoding) {
}

function getRedirectUrl (url) {
if (url.startsWith('//') || url.startsWith('/\\')) {
// malicous redirect
return '/'
let i = 0
// we detech how many slash before a valid path
for (i; i < url.length; i++) {
if (url[i] !== '/' && url[i] !== '\\') break
}
// turns all leading / or \ into a single /
url = '/' + url.substr(i)
try {
const parsed = new URL(url, 'http://localhost.com/')
return parsed.pathname + (parsed.pathname[parsed.pathname.length - 1] !== '/' ? '/' : '') + (parsed.search || '')
} catch (error) {
// the try-catch here is actually unreachable, but we keep it for safety and prevent DoS attack
Copy link
Member Author

@climba03003 climba03003 Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In here, the catch branch is actually unreachable after we convert all leading slash to a single /. But, it may be changed in future and URL will be crashed on some weird case.

So, I will keep the try {} catch {} here and ignore the coverage.

Why it is unreachable?
When we specify the host for in URL, the path can be either a full url or path. After we stripe the leading slash and it can only be path or schema://something in this case. And URL do not check the invalid encoded component. Thus, it is not able to crash anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was reachable for this command:

curl --path-as-is "http://localhost:3000//^/.."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http://localhost:3000//^/.. is actually doing new URL('/^/..', 'http://localhost.com/') and it is not throwing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on node 16.8 I get (see the double slash - this trigger the issue but your check hide this case)

➜  fastify-static git:(PULL_247) ✗ node       
Welcome to Node.js v16.8.0.
Type ".help" for more information.
> new URL('//^/..', 'http://localhost.com/')
Uncaught TypeError [ERR_INVALID_URL]: Invalid URL
    at __node_internal_captureLargerStackTrace (node:internal/errors:464:5)
    at new NodeError (node:internal/errors:371:5)
    at onParseError (node:internal/url:552:9)
    at new URL (node:internal/url:628:5) {
  input: '//^/..',
  code: 'ERR_INVALID_URL'
}

/* istanbul ignore next */
const err = new Error(`Invalid redirect URL: ${url}`)
/* istanbul ignore next */
err.statusCode = 400
/* istanbul ignore next */
throw err
}
}
Expand Down
7 changes: 4 additions & 3 deletions test/static.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3288,12 +3288,13 @@ t.test('should not redirect to protocol-relative locations', (t) => {
['//^/..', '/', 301],
['//^/.', null, 404], // it is NOT recognized as a directory by pillarjs/send
['//:/..', '/', 301],
['/\\\\a//google.com/%2e%2e%2f%2e%2e', '/', 301],
['//a//youtube.com/%2e%2e%2f%2e%2e', '/', 301],
['/\\\\a//google.com/%2e%2e%2f%2e%2e', '/a//google.com/%2e%2e%2f%2e%2e/', 301],
['//a//youtube.com/%2e%2e%2f%2e%2e', '/a//youtube.com/%2e%2e%2f%2e%2e/', 301],
['/^', null, 404], // it is NOT recognized as a directory by pillarjs/send
['//google.com/%2e%2e', '/', 301],
['//users/%2e%2e', '/', 301],
['//users', null, 404]
['//users', null, 404],
['///deep/path//for//test//index.html', null, 200]
]

t.plan(1 + urls.length * 2)
Expand Down