From 1ecab988698cbad7f46b953e7b80d31629820a6a Mon Sep 17 00:00:00 2001 From: Daniel Tschinder <231804+danez@users.noreply.github.com> Date: Fri, 26 Aug 2022 17:43:43 +0200 Subject: [PATCH 1/2] fix: fix redirects to external URLs --- src/utils/proxy.js | 15 +- tests/integration/0.command.dev.test.js | 40 +-- tests/integration/400.command.dev.test.js | 350 ---------------------- tests/integration/500.command.dev.test.js | 27 +- 4 files changed, 44 insertions(+), 388 deletions(-) diff --git a/src/utils/proxy.js b/src/utils/proxy.js index 0dd2e8640d1..122f9b43e6d 100644 --- a/src/utils/proxy.js +++ b/src/utils/proxy.js @@ -70,7 +70,7 @@ const proxyToExternalUrl = function ({ dest, destURL, req, res }) { pathRewrite: () => destURL, ...(Buffer.isBuffer(req.originalBody) && { buffer: toReadableStream(req.originalBody) }), }) - return handler(req, res, {}) + return handler(req, res, () => {}) } const handleAddonUrl = function ({ addonUrl, req, res }) { @@ -185,7 +185,7 @@ const serveRedirect = async function ({ match, options, proxy, req, res }) { const staticFile = await getStatic(decodeURIComponent(reqUrl.pathname), options.publicFolder) if (staticFile) { - req.url = encodeURIComponent(staticFile) + reqUrl.search + req.url = encodeURI(staticFile) + reqUrl.search // if there is an existing static file and it is not a forced redirect, return the file if (!match.force) { return proxy.web(req, res, { ...options, staticFile }) @@ -214,18 +214,25 @@ const serveRedirect = async function ({ match, options, proxy, req, res }) { }) } - const destURL = stripOrigin(dest) + let destURL = stripOrigin(dest) if (isExternal(match)) { - return proxyToExternalUrl({ req, res, dest, destURL }) + if (isRedirect(match)) { + // This is a redirect, so we set the complete external URL as destination + destURL = `${dest}` + } else { + return proxyToExternalUrl({ req, res, dest, destURL }) + } } if (isRedirect(match)) { + console.log(`${NETLIFYDEVLOG} Redirecting ${req.url} to ${destURL}`) res.writeHead(match.status, { Location: destURL, 'Cache-Control': 'no-cache', }) res.end(`Redirecting to ${destURL}`) + return } diff --git a/tests/integration/0.command.dev.test.js b/tests/integration/0.command.dev.test.js index 4809920d3c3..4c32d55eab7 100644 --- a/tests/integration/0.command.dev.test.js +++ b/tests/integration/0.command.dev.test.js @@ -1,6 +1,5 @@ // Handlers are meant to be async outside tests const { promises: fs } = require('fs') -const http = require('http') const { join } = require('path') // eslint-disable-next-line ava/use-test @@ -166,7 +165,7 @@ test('should source redirects file from publish directory', async (t) => { }) }) -test('should redirect requests to an external server', async (t) => { +test('should rewrite requests to an external server', async (t) => { await withSiteBuilder('site-redirects-file-to-external', async (builder) => { const externalServer = startExternalServer() const { port } = externalServer.address() @@ -186,6 +185,7 @@ test('should redirect requests to an external server', async (t) => { 'Content-Type': 'application/x-www-form-urlencoded', }, body: 'param=value', + followRedirect: false, }) .json() t.deepEqual(postResponse, { body: { param: 'value' }, method: 'POST', url: '/ping' }) @@ -206,15 +206,18 @@ test('should follow 301 redirect to an external server', async (t) => { await builder.buildAsync() await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/api/ping`).json() - t.deepEqual(response, { body: {}, method: 'GET', url: '/ping' }) + const response = await got(`${server.url}/api/ping`, { followRedirect: false }) + t.is(response.headers.location, `http://localhost:${port}/ping`) + + const body = await got(`${server.url}/api/ping`).json() + t.deepEqual(body, { body: {}, method: 'GET', url: '/ping' }) }) externalServer.close() }) }) -test('should redirect POST request if content-type is missing', async (t) => { +test('should rewrite POST request if content-type is missing', async (t) => { await withSiteBuilder('site-with-post-no-content-type', async (builder) => { builder.withNetlifyToml({ config: { @@ -226,27 +229,16 @@ test('should redirect POST request if content-type is missing', async (t) => { await builder.buildAsync() await withDevServer({ cwd: builder.directory }, async (server) => { - const options = { - host: server.host, - port: server.port, - path: '/api/echo', - method: 'POST', - } - let data = '' - await new Promise((resolve) => { - const callback = (response) => { - response.on('data', (chunk) => { - data += chunk - }) - response.on('end', resolve) - } - const req = http.request(options, callback) - req.write('param=value') - req.end() - }) + const error = await t.throwsAsync( + async () => + await got.post(`${server.url}/api/echo`, { + body: 'param=value', + followRedirect: false, + }), + ) // we're testing Netlify Dev didn't crash - t.is(data, 'Method Not Allowed') + t.is(error.message, 'Response code 405 (Method Not Allowed)') }) }) }) diff --git a/tests/integration/400.command.dev.test.js b/tests/integration/400.command.dev.test.js index d0434f61915..253a2bbe061 100644 --- a/tests/integration/400.command.dev.test.js +++ b/tests/integration/400.command.dev.test.js @@ -1,6 +1,5 @@ // Handlers are meant to be async outside tests /* eslint-disable require-await */ -const path = require('path') const process = require('process') // eslint-disable-next-line ava/use-test @@ -363,353 +362,4 @@ test('should handle multipart form data when redirecting', async (t) => { }) }) }) - -test('should return 404 when redirecting to a non existing function', async (t) => { - await withSiteBuilder('site-with-missing-function', async (builder) => { - builder.withNetlifyToml({ - config: { - functions: { directory: 'functions' }, - redirects: [{ from: '/api/*', to: '/.netlify/functions/:splat', status: 200 }], - }, - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got - .post(`${server.url}/api/none`, { - body: 'nothing', - }) - .catch((error) => error.response) - - t.is(response.statusCode, 404) - }) - }) -}) - -test('should parse function query parameters using simple parsing', async (t) => { - await withSiteBuilder('site-with-multi-part-function', async (builder) => { - builder - .withNetlifyToml({ - config: { - functions: { directory: 'functions' }, - }, - }) - .withFunction({ - path: 'echo.js', - handler: async (event) => ({ - statusCode: 200, - body: JSON.stringify(event), - }), - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response1 = await got(`${server.url}/.netlify/functions/echo?category[SOMETHING][]=something`).json() - const response2 = await got(`${server.url}/.netlify/functions/echo?category=one&category=two`).json() - - t.deepEqual(response1.queryStringParameters, { 'category[SOMETHING][]': 'something' }) - t.deepEqual(response2.queryStringParameters, { category: 'one, two' }) - }) - }) -}) - -test('should handle form submission', async (t) => { - await withSiteBuilder('site-with-form', async (builder) => { - builder - .withContentFile({ - path: 'index.html', - content: '

⊂◉‿◉つ

', - }) - .withNetlifyToml({ - config: { - functions: { directory: 'functions' }, - }, - }) - .withFunction({ - path: 'submission-created.js', - handler: async (event) => ({ - statusCode: 200, - body: JSON.stringify(event), - }), - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const form = new FormData() - form.append('some', 'thing') - const response = await got - .post(`${server.url}/?ding=dong`, { - body: form, - }) - .json() - - const body = JSON.parse(response.body) - const expectedBody = { - payload: { - created_at: body.payload.created_at, - data: { - ip: '::ffff:127.0.0.1', - some: 'thing', - user_agent: 'got (https://github.com/sindresorhus/got)', - }, - human_fields: { - Some: 'thing', - }, - ordered_human_fields: [ - { - name: 'some', - title: 'Some', - value: 'thing', - }, - ], - site_url: '', - }, - } - - t.is(response.headers.host, `${server.host}:${server.port}`) - t.is(response.headers['content-length'], JSON.stringify(expectedBody).length.toString()) - t.is(response.headers['content-type'], 'application/json') - t.is(response.httpMethod, 'POST') - t.is(response.isBase64Encoded, false) - t.is(response.path, '/') - t.deepEqual(response.queryStringParameters, { ding: 'dong' }) - t.deepEqual(body, expectedBody) - }) - }) -}) - -test('should handle form submission with a background function', async (t) => { - await withSiteBuilder('site-with-form-background-function', async (builder) => { - await builder - .withContentFile({ - path: 'index.html', - content: '

⊂◉‿◉つ

', - }) - .withNetlifyToml({ - config: { - functions: { directory: 'functions' }, - }, - }) - .withFunction({ - path: 'submission-created-background.js', - handler: async (event) => ({ - statusCode: 200, - body: JSON.stringify(event), - }), - }) - .buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const form = new FormData() - form.append('some', 'thing') - const response = await got.post(`${server.url}/?ding=dong`, { - body: form, - }) - t.is(response.statusCode, 202) - t.is(response.body, '') - }) - }) -}) - -test('should not handle form submission when content type is `text/plain`', async (t) => { - await withSiteBuilder('site-with-form-text-plain', async (builder) => { - builder - .withContentFile({ - path: 'index.html', - content: '

⊂◉‿◉つ

', - }) - .withNetlifyToml({ - config: { - functions: { directory: 'functions' }, - }, - }) - .withFunction({ - path: 'submission-created.js', - handler: async (event) => ({ - statusCode: 200, - body: JSON.stringify(event), - }), - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got - .post(`${server.url}/?ding=dong`, { - body: 'Something', - headers: { - 'content-type': 'text/plain', - }, - }) - .catch((error) => error.response) - t.is(response.body, 'Method Not Allowed') - }) - }) -}) - -test('should return existing local file even when rewrite matches when force=false', async (t) => { - await withSiteBuilder('site-with-shadowing-force-false', async (builder) => { - builder - .withContentFile({ - path: 'foo.html', - content: '

foo', - }) - .withContentFile({ - path: path.join('not-foo', 'index.html'), - content: '

not-foo', - }) - .withNetlifyToml({ - config: { - redirects: [{ from: '/foo', to: '/not-foo', status: 200, force: false }], - }, - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/foo?ping=pong`).text() - t.is(response, '

foo') - }) - }) -}) - -test('should return existing local file even when redirect matches when force=false', async (t) => { - await withSiteBuilder('site-with-shadowing-force-false', async (builder) => { - builder - .withContentFile({ - path: 'foo.html', - content: '

foo', - }) - .withContentFile({ - path: path.join('not-foo', 'index.html'), - content: '

not-foo', - }) - .withNetlifyToml({ - config: { - redirects: [{ from: '/foo', to: '/not-foo', status: 301, force: false }], - }, - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/foo?ping=pong`).text() - t.is(response, '

foo') - }) - }) -}) - -test('should ignore existing local file when redirect matches and force=true', async (t) => { - await withSiteBuilder('site-with-shadowing-force-true', async (builder) => { - builder - .withContentFile({ - path: 'foo.html', - content: '

foo', - }) - .withContentFile({ - path: path.join('not-foo', 'index.html'), - content: '

not-foo', - }) - .withNetlifyToml({ - config: { - redirects: [{ from: '/foo', to: '/not-foo', status: 200, force: true }], - }, - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/foo`).text() - t.is(response, '

not-foo') - }) - }) -}) - -test('should use existing file when rule contains file extension and force=false', async (t) => { - await withSiteBuilder('site-with-shadowing-file-extension-force-false', async (builder) => { - builder - .withContentFile({ - path: 'foo.html', - content: '

foo', - }) - .withContentFile({ - path: path.join('not-foo', 'index.html'), - content: '

not-foo', - }) - .withNetlifyToml({ - config: { - redirects: [{ from: '/foo.html', to: '/not-foo', status: 200, force: false }], - }, - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/foo.html`).text() - t.is(response, '

foo') - }) - }) -}) - -test('should redirect when rule contains file extension and force=true', async (t) => { - await withSiteBuilder('site-with-shadowing-file-extension-force-true', async (builder) => { - builder - .withContentFile({ - path: 'foo.html', - content: '

foo', - }) - .withContentFile({ - path: path.join('not-foo', 'index.html'), - content: '

not-foo', - }) - .withNetlifyToml({ - config: { - redirects: [{ from: '/foo.html', to: '/not-foo', status: 200, force: true }], - }, - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/foo.html`).text() - t.is(response, '

not-foo') - }) - }) -}) - -test('should redirect from sub directory to root directory', async (t) => { - await withSiteBuilder('site-with-shadowing-sub-to-root', async (builder) => { - builder - .withContentFile({ - path: 'foo.html', - content: '

foo', - }) - .withContentFile({ - path: path.join('not-foo', 'index.html'), - content: '

not-foo', - }) - .withNetlifyToml({ - config: { - redirects: [{ from: '/not-foo', to: '/foo', status: 200, force: true }], - }, - }) - - await builder.buildAsync() - - await withDevServer({ cwd: builder.directory }, async (server) => { - const response1 = await got(`${server.url}/not-foo`).text() - const response2 = await got(`${server.url}/not-foo/`).text() - - // TODO: check why this doesn't redirect - const response3 = await got(`${server.url}/not-foo/index.html`).text() - - t.is(response1, '

foo') - t.is(response2, '

foo') - t.is(response3, '

not-foo') - }) - }) -}) /* eslint-enable require-await */ diff --git a/tests/integration/500.command.dev.test.js b/tests/integration/500.command.dev.test.js index 1c17fdb820a..e0b835e38cf 100644 --- a/tests/integration/500.command.dev.test.js +++ b/tests/integration/500.command.dev.test.js @@ -267,15 +267,18 @@ test('should ignore existing local file when redirect matches and force=true', a }) .withNetlifyToml({ config: { - redirects: [{ from: '/foo', to: '/not-foo', status: 200, force: true }], + redirects: [{ from: '/foo', to: '/not-foo', status: 301, force: true }], }, }) await builder.buildAsync() await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/foo`).text() - t.is(response, '

not-foo') + const response = await got(`${server.url}/foo`, { followRedirect: false }) + t.is(response.headers.location, `/not-foo`) + + const body = await got(`${server.url}/foo`).text() + t.is(body, '

not-foo') }) }) }) @@ -293,15 +296,16 @@ test('should use existing file when rule contains file extension and force=false }) .withNetlifyToml({ config: { - redirects: [{ from: '/foo.html', to: '/not-foo', status: 200, force: false }], + redirects: [{ from: '/foo.html', to: '/not-foo', status: 301, force: false }], }, }) await builder.buildAsync() await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/foo.html`).text() - t.is(response, '

foo') + const response = await got(`${server.url}/foo.html`, { followRedirect: false }) + t.is(response.headers.location, undefined) + t.is(response.body, '

foo') }) }) }) @@ -319,15 +323,18 @@ test('should redirect when rule contains file extension and force=true', async ( }) .withNetlifyToml({ config: { - redirects: [{ from: '/foo.html', to: '/not-foo', status: 200, force: true }], + redirects: [{ from: '/foo.html', to: '/not-foo', status: 301, force: true }], }, }) await builder.buildAsync() await withDevServer({ cwd: builder.directory }, async (server) => { - const response = await got(`${server.url}/foo.html`).text() - t.is(response, '

not-foo') + const response = await got(`${server.url}/foo.html`, { followRedirect: false }) + t.is(response.headers.location, `/not-foo`) + + const body = await got(`${server.url}/foo.html`).text() + t.is(body, '

not-foo') }) }) }) @@ -381,7 +388,7 @@ test('Runs build plugins with the `onPreDev` event', async (t) => { onPreDev: () => { const server = http.createServer((_, res) => res.end("Hello world")); - + server.listen(${userServerPort}, "localhost", () => { console.log("Server is running on port ${userServerPort}"); }); From 3565dce3343f57595d5156757b66196521095b5a Mon Sep 17 00:00:00 2001 From: Daniel Tschinder <231804+danez@users.noreply.github.com> Date: Mon, 29 Aug 2022 14:23:03 +0200 Subject: [PATCH 2/2] chore: fix nit from review --- tests/integration/0.command.dev.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/0.command.dev.test.js b/tests/integration/0.command.dev.test.js index 4c32d55eab7..f4fdeca02ed 100644 --- a/tests/integration/0.command.dev.test.js +++ b/tests/integration/0.command.dev.test.js @@ -217,7 +217,7 @@ test('should follow 301 redirect to an external server', async (t) => { }) }) -test('should rewrite POST request if content-type is missing', async (t) => { +test('should rewrite POST request if content-type is missing and not crash dev server', async (t) => { await withSiteBuilder('site-with-post-no-content-type', async (builder) => { builder.withNetlifyToml({ config: { @@ -237,8 +237,8 @@ test('should rewrite POST request if content-type is missing', async (t) => { }), ) - // we're testing Netlify Dev didn't crash - t.is(error.message, 'Response code 405 (Method Not Allowed)') + // Method Not Allowed + t.is(error.response.statusCode, 405) }) }) })