From 992806fe2ad4e1f1e74a90ff63a21de6abb6c143 Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Wed, 23 Sep 2020 16:37:36 -0400 Subject: [PATCH 1/4] WIP: Properly encode URL params from custom routes --- .../lib/router/utils/path-match.ts | 23 ++++--------------- .../lib/router/utils/prepare-destination.ts | 11 ++++----- .../next/next-server/server/next-server.ts | 19 +++++---------- packages/next/server/next-dev-server.ts | 4 ++-- 4 files changed, 18 insertions(+), 39 deletions(-) diff --git a/packages/next/next-server/lib/router/utils/path-match.ts b/packages/next/next-server/lib/router/utils/path-match.ts index cbf785b5c6510..8633a35cc5fb4 100644 --- a/packages/next/next-server/lib/router/utils/path-match.ts +++ b/packages/next/next-server/lib/router/utils/path-match.ts @@ -2,13 +2,14 @@ import * as pathToRegexp from 'next/dist/compiled/path-to-regexp' export { pathToRegexp } -export const matcherOptions = { +export const matcherOptions: pathToRegexp.TokensToRegexpOptions & + pathToRegexp.ParseOptions = { sensitive: false, delimiter: '/', - decode: decodeParam, } -export const customRouteMatcherOptions = { +export const customRouteMatcherOptions: pathToRegexp.TokensToRegexpOptions & + pathToRegexp.ParseOptions = { ...matcherOptions, strict: true, } @@ -21,11 +22,7 @@ export default (customRoute = false) => { keys, customRoute ? customRouteMatcherOptions : matcherOptions ) - const matcher = pathToRegexp.regexpToFunction( - matcherRegex, - keys, - matcherOptions - ) + const matcher = pathToRegexp.regexpToFunction(matcherRegex, keys) return (pathname: string | null | undefined, params?: any) => { const res = pathname == null ? false : matcher(pathname) @@ -47,13 +44,3 @@ export default (customRoute = false) => { } } } - -function decodeParam(param: string) { - try { - return decodeURIComponent(param) - } catch (_) { - const err: Error & { code?: string } = new Error('failed to decode param') - err.code = 'DECODE_FAILED' - throw err - } -} diff --git a/packages/next/next-server/lib/router/utils/prepare-destination.ts b/packages/next/next-server/lib/router/utils/prepare-destination.ts index 72c51f1b3adbb..76dff836d48cc 100644 --- a/packages/next/next-server/lib/router/utils/prepare-destination.ts +++ b/packages/next/next-server/lib/router/utils/prepare-destination.ts @@ -54,16 +54,15 @@ export default function prepareDestination( const destPathParams = destPathParamKeys.map((key) => key.name) - let destinationCompiler = pathToRegexp.compile( - destPath, + let destinationCompiler = pathToRegexp.compile(destPath, { // we don't validate while compiling the destination since we should // have already validated before we got to this point and validating // breaks compiling destinations with named pattern params from the source // e.g. /something:hello(.*) -> /another/:hello is broken with validation // since compile validation is meant for reversing and not for inserting // params from a separate path-regex into another - { validate: false } - ) + validate: false, + }) let newUrl // update any params in query values @@ -97,8 +96,8 @@ export default function prepareDestination( const shouldAddBasePath = destination.startsWith('/') && basePath try { - newUrl = `${shouldAddBasePath ? basePath : ''}${encodeURI( - destinationCompiler(params) + newUrl = `${shouldAddBasePath ? basePath : ''}${destinationCompiler( + params )}` const [pathname, hash] = newUrl.split('#') diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 268a3c9f11d24..e36e19e3ee855 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -352,11 +352,7 @@ export default class Server { match: route('/static/:path*'), name: 'static catchall', fn: async (req, res, params, parsedUrl) => { - const p = join( - this.dir, - 'static', - ...(params.path || []).map(encodeURIComponent) - ) + const p = join(this.dir, 'static', ...params.path) await this.serveStatic(req, res, p, parsedUrl) return { finished: true, @@ -428,11 +424,7 @@ export default class Server { // re-create page's pathname const pathname = getRouteFromAssetPath( - `/${params.path - // we need to re-encode the params since they are decoded - // by path-match and we are re-building the URL - .map((param: string) => encodeURIComponent(param)) - .join('/')}`, + `/${params.path.join('/')}`, '.json' ) @@ -775,7 +767,9 @@ export default class Server { protected generatePublicRoutes(): Route[] { const publicFiles = new Set( - recursiveReadDirSync(this.publicDir).map((p) => p.replace(/\\/g, '/')) + recursiveReadDirSync(this.publicDir).map((p) => + encodeURI(p.replace(/\\/g, '/')) + ) ) return [ @@ -798,8 +792,7 @@ export default class Server { await this.serveStatic( req, res, - // we need to re-encode it since send decodes it - join(this.publicDir, ...pathParts.map(encodeURIComponent)), + join(this.publicDir, ...pathParts), parsedUrl ) return { diff --git a/packages/next/server/next-dev-server.ts b/packages/next/server/next-dev-server.ts index f8c0042cb255d..bd52c3a613b5d 100644 --- a/packages/next/server/next-dev-server.ts +++ b/packages/next/server/next-dev-server.ts @@ -336,7 +336,7 @@ export default class DevServer extends Server { const path = `/${pathParts.join('/')}` // check for a public file, throwing error if there's a // conflicting page - if (await this.hasPublicFile(path)) { + if (await this.hasPublicFile(decodeURIComponent(path))) { if (await this.hasPage(pathname!)) { const err = new Error( `A conflicting public file and page file was found for path ${pathname} https://err.sh/vercel/next.js/conflicting-public-file-page` @@ -660,7 +660,7 @@ export default class DevServer extends Server { res: ServerResponse, pathParts: string[] ): Promise { - const p = pathJoin(this.publicDir, ...pathParts.map(encodeURIComponent)) + const p = pathJoin(this.publicDir, ...pathParts) return this.serveStatic(req, res, p) } From 63ff43a0314472085c42eaed5c5adb2057ca7a3d Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 23 Sep 2020 16:55:01 -0500 Subject: [PATCH 2/4] Update to pass through query values for custom routes --- .../lib/router/utils/prepare-destination.ts | 7 +++-- .../next/next-server/server/next-server.ts | 28 ++++++++++++++++++- packages/next/server/hot-reloader.ts | 4 ++- .../production/pages/invalid-param/[slug].js | 13 +++++++++ .../integration/production/test/index.test.js | 2 +- 5 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 test/integration/production/pages/invalid-param/[slug].js diff --git a/packages/next/next-server/lib/router/utils/prepare-destination.ts b/packages/next/next-server/lib/router/utils/prepare-destination.ts index 76dff836d48cc..90df77c446c5b 100644 --- a/packages/next/next-server/lib/router/utils/prepare-destination.ts +++ b/packages/next/next-server/lib/router/utils/prepare-destination.ts @@ -54,15 +54,16 @@ export default function prepareDestination( const destPathParams = destPathParamKeys.map((key) => key.name) - let destinationCompiler = pathToRegexp.compile(destPath, { + let destinationCompiler = pathToRegexp.compile( + destPath, // we don't validate while compiling the destination since we should // have already validated before we got to this point and validating // breaks compiling destinations with named pattern params from the source // e.g. /something:hello(.*) -> /another/:hello is broken with validation // since compile validation is meant for reversing and not for inserting // params from a separate path-regex into another - validate: false, - }) + { validate: false } + ) let newUrl // update any params in query values diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index e36e19e3ee855..415f463cdd4e0 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -4,7 +4,11 @@ import chalk from 'next/dist/compiled/chalk' import { IncomingMessage, ServerResponse } from 'http' import Proxy from 'next/dist/compiled/http-proxy' import { join, relative, resolve, sep } from 'path' -import { parse as parseQs, ParsedUrlQuery } from 'querystring' +import { + parse as parseQs, + stringify as stringifyQs, + ParsedUrlQuery, +} from 'querystring' import { format as formatUrl, parse as parseUrl, UrlWithParsedQuery } from 'url' import { PrerenderManifest } from '../../build' import { @@ -551,6 +555,14 @@ export default class Server { false, getCustomRouteBasePath(redirectRoute) ) + + const { query } = parsedDestination + delete parsedDestination.query + + parsedDestination.search = stringifyQs(query, undefined, undefined, { + encodeURIComponent: (str: string) => str, + } as any) + const updatedDestination = formatUrl(parsedDestination) res.setHeader('Location', updatedDestination) @@ -589,6 +601,15 @@ export default class Server { // external rewrite, proxy it if (parsedDestination.protocol) { + const { query } = parsedDestination + delete parsedDestination.query + parsedDestination.search = stringifyQs( + query, + undefined, + undefined, + { encodeURIComponent: (str) => str } + ) + const target = formatUrl(parsedDestination) const proxy = new Proxy({ target, @@ -1316,6 +1337,11 @@ export default class Server { } } catch (err) { this.logError(err) + + if (err.code === 'DECODE_FAILED') { + res.statusCode = 400 + return await this.renderErrorToHTML(err, req, res, pathname, query) + } res.statusCode = 500 return await this.renderErrorToHTML(err, req, res, pathname, query) } diff --git a/packages/next/server/hot-reloader.ts b/packages/next/server/hot-reloader.ts index abeb3872300c9..01f27aa254b1f 100644 --- a/packages/next/server/hot-reloader.ts +++ b/packages/next/server/hot-reloader.ts @@ -215,7 +215,9 @@ export default class HotReloader { return {} } - const page = denormalizePagePath(`/${params.path.join('/')}`) + const page = denormalizePagePath( + `/${params.path.map((param) => decodeURIComponent(param)).join('/')}` + ) if (page === '/_error' || BLOCKED_PAGES.indexOf(page) === -1) { try { await this.ensurePage(page) diff --git a/test/integration/production/pages/invalid-param/[slug].js b/test/integration/production/pages/invalid-param/[slug].js new file mode 100644 index 0000000000000..9f287061d65fc --- /dev/null +++ b/test/integration/production/pages/invalid-param/[slug].js @@ -0,0 +1,13 @@ +import { useRouter } from 'next/router' + +export default function Page() { + return

hello {useRouter().query}

+} + +export const getServerSideProps = () => { + return { + props: { + hello: 'world', + }, + } +} diff --git a/test/integration/production/test/index.test.js b/test/integration/production/test/index.test.js index 467eeb42c114d..5a53721540439 100644 --- a/test/integration/production/test/index.test.js +++ b/test/integration/production/test/index.test.js @@ -710,7 +710,7 @@ describe('Production Usage', () => { }) it('should handle failed param decoding', async () => { - const html = await renderViaHTTP(appPort, '/%DE~%C7%1fY/') + const html = await renderViaHTTP(appPort, '/invalid-param/%DE~%C7%1fY/') expect(html).toMatch(/400/) expect(html).toMatch(/Bad Request/) }) From c6ac70902d558e5f4c65fac1b602cf7e1845cb4d Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 23 Sep 2020 20:19:36 -0500 Subject: [PATCH 3/4] Update decode error in dev server --- packages/next/server/next-dev-server.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/next/server/next-dev-server.ts b/packages/next/server/next-dev-server.ts index bd52c3a613b5d..8bddcb130f023 100644 --- a/packages/next/server/next-dev-server.ts +++ b/packages/next/server/next-dev-server.ts @@ -336,7 +336,17 @@ export default class DevServer extends Server { const path = `/${pathParts.join('/')}` // check for a public file, throwing error if there's a // conflicting page - if (await this.hasPublicFile(decodeURIComponent(path))) { + let decodedPath: string + + try { + decodedPath = decodeURIComponent(path) + } catch (_) { + const err: Error & { code?: string } = new Error('failed to decode param') + err.code = 'DECODE_FAILED' + throw err + } + + if (await this.hasPublicFile(decodedPath)) { if (await this.hasPage(pathname!)) { const err = new Error( `A conflicting public file and page file was found for path ${pathname} https://err.sh/vercel/next.js/conflicting-public-file-page` From 9c47c22af6bce4c3bb7e3e19ccb1e4e5620088f2 Mon Sep 17 00:00:00 2001 From: JJ Kasper Date: Wed, 23 Sep 2020 20:54:00 -0500 Subject: [PATCH 4/4] Update decode error in hot-reloader --- .../next/next-server/server/next-server.ts | 2 +- packages/next/server/hot-reloader.ts | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/next/next-server/server/next-server.ts b/packages/next/next-server/server/next-server.ts index 415f463cdd4e0..b005f719114af 100644 --- a/packages/next/next-server/server/next-server.ts +++ b/packages/next/next-server/server/next-server.ts @@ -1338,7 +1338,7 @@ export default class Server { } catch (err) { this.logError(err) - if (err.code === 'DECODE_FAILED') { + if (err && err.code === 'DECODE_FAILED') { res.statusCode = 400 return await this.renderErrorToHTML(err, req, res, pathname, query) } diff --git a/packages/next/server/hot-reloader.ts b/packages/next/server/hot-reloader.ts index 01f27aa254b1f..0885e0c84fd8f 100644 --- a/packages/next/server/hot-reloader.ts +++ b/packages/next/server/hot-reloader.ts @@ -215,9 +215,22 @@ export default class HotReloader { return {} } - const page = denormalizePagePath( - `/${params.path.map((param) => decodeURIComponent(param)).join('/')}` - ) + let decodedPagePath: string + + try { + decodedPagePath = `/${params.path + .map((param) => decodeURIComponent(param)) + .join('/')}` + } catch (_) { + const err: Error & { code?: string } = new Error( + 'failed to decode param' + ) + err.code = 'DECODE_FAILED' + throw err + } + + const page = denormalizePagePath(decodedPagePath) + if (page === '/_error' || BLOCKED_PAGES.indexOf(page) === -1) { try { await this.ensurePage(page)