From 9bfc5beade66b16648b7ef16a42391ca6339da3b Mon Sep 17 00:00:00 2001 From: KaKa Date: Mon, 11 Oct 2021 23:13:49 +0800 Subject: [PATCH 1/5] fix: redirection handling --- index.js | 15 ++++++++++++--- test/static.test.js | 7 ++++--- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/index.js b/index.js index ae4365a0..6169aac9 100644 --- a/index.js +++ b/index.js @@ -447,12 +447,21 @@ function getEncodingExtension (encoding) { } function getRedirectUrl (url) { - if (url.startsWith('//') || url.startsWith('/\\')) { - // malicous redirect - return '/' + // malicous redirect detection + // we stripe it into a single slash + while (url.startsWith('//') || url.startsWith('/\\')) { + if (url.startsWith('//')) { + // we replace the first two slash only + url = url.replace('//', '/') + } + if (url.startsWith('/\\')) { + // we replace the leading malicous part only + url = url.replace('/\\', '/') + } } try { const parsed = new URL(url, 'http://localhost.com/') + console.log(url, parsed.href) return parsed.pathname + (parsed.pathname[parsed.pathname.length - 1] !== '/' ? '/' : '') + (parsed.search || '') } catch (error) { const err = new Error(`Invalid redirect URL: ${url}`) diff --git a/test/static.test.js b/test/static.test.js index fdd59acc..67335eda 100644 --- a/test/static.test.js +++ b/test/static.test.js @@ -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) From d52513a3bbbe5ebc2fffa3be5a8cf5ffee182f7a Mon Sep 17 00:00:00 2001 From: KaKa Date: Mon, 11 Oct 2021 23:16:13 +0800 Subject: [PATCH 2/5] chore: remove console.log --- index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/index.js b/index.js index 6169aac9..1c0f3d67 100644 --- a/index.js +++ b/index.js @@ -461,7 +461,6 @@ function getRedirectUrl (url) { } try { const parsed = new URL(url, 'http://localhost.com/') - console.log(url, parsed.href) return parsed.pathname + (parsed.pathname[parsed.pathname.length - 1] !== '/' ? '/' : '') + (parsed.search || '') } catch (error) { const err = new Error(`Invalid redirect URL: ${url}`) From e358026c1041d6686188a0865b2a740a50bf6e0b Mon Sep 17 00:00:00 2001 From: KaKa Date: Mon, 11 Oct 2021 23:27:13 +0800 Subject: [PATCH 3/5] refactor: use a single for-loop --- index.js | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/index.js b/index.js index 1c0f3d67..272fab22 100644 --- a/index.js +++ b/index.js @@ -446,19 +446,16 @@ function getEncodingExtension (encoding) { } } +// in here the url is already santitize once +// all the / and \ are equal to / function getRedirectUrl (url) { - // malicous redirect detection - // we stripe it into a single slash - while (url.startsWith('//') || url.startsWith('/\\')) { - if (url.startsWith('//')) { - // we replace the first two slash only - url = url.replace('//', '/') - } - if (url.startsWith('/\\')) { - // we replace the leading malicous part only - url = url.replace('/\\', '/') - } + let i = 0 + // we detech how many slash before a valid path + for (i; i < url.length; i++) { + if (url[i] !== '/') break } + // turns all leading / to 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 || '') From aa797bc8da538d7853a65e2595a7962ede19c306 Mon Sep 17 00:00:00 2001 From: KaKa Date: Tue, 12 Oct 2021 00:11:25 +0800 Subject: [PATCH 4/5] fix: backward slash is not handle --- index.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index 272fab22..36efa0b2 100644 --- a/index.js +++ b/index.js @@ -446,15 +446,13 @@ function getEncodingExtension (encoding) { } } -// in here the url is already santitize once -// all the / and \ are equal to / function getRedirectUrl (url) { let i = 0 // we detech how many slash before a valid path for (i; i < url.length; i++) { - if (url[i] !== '/') break + if (url[i] !== '/' && url[i] !== '\\') break } - // turns all leading / to a single / + // turns all leading / or \ into a single / url = '/' + url.substr(i) try { const parsed = new URL(url, 'http://localhost.com/') From 16eef579bbe5a7352e4968e610f9b52dbc5a77d4 Mon Sep 17 00:00:00 2001 From: KaKa Date: Tue, 12 Oct 2021 12:12:45 +0800 Subject: [PATCH 5/5] test: 100% coverage --- index.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/index.js b/index.js index 36efa0b2..10b04f04 100644 --- a/index.js +++ b/index.js @@ -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 { @@ -458,8 +460,12 @@ function getRedirectUrl (url) { 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 + /* istanbul ignore next */ const err = new Error(`Invalid redirect URL: ${url}`) + /* istanbul ignore next */ err.statusCode = 400 + /* istanbul ignore next */ throw err } }