From b7ead5ff140c36293b0eb42428d41969b8d8d4a3 Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 5 Nov 2019 16:55:57 +0100 Subject: [PATCH] Allow method rewriting on redirects (#913) --- readme.md | 2 + source/request-as-event-emitter.ts | 75 ++++++++++++++-------------- test/arguments.ts | 64 ++++++++++++++++++++++++ test/redirects.ts | 80 ++++++++++++++++++++++++++---- 4 files changed, 174 insertions(+), 47 deletions(-) diff --git a/readme.md b/readme.md index 5faff6f95..4897c4c1f 100644 --- a/readme.md +++ b/readme.md @@ -368,6 +368,8 @@ Defines if redirect responses should be followed automatically. Note that if a `303` is sent by the server in response to any request type (`POST`, `DELETE`, etc.), Got will automatically request the resource pointed to in the location header via `GET`. This is in accordance with [the spec](https://tools.ietf.org/html/rfc7231#section-6.4.4). +This supports [method rewriting](https://tools.ietf.org/html/rfc7231#section-6.4). For example, when sending a POST request and receiving a `302`, it will resend that request to the new location. + ###### maxRedirects Type: `number`
diff --git a/source/request-as-event-emitter.ts b/source/request-as-event-emitter.ts index c13c8d8dc..99653b3d2 100644 --- a/source/request-as-event-emitter.ts +++ b/source/request-as-event-emitter.ts @@ -20,13 +20,8 @@ import urlToOptions from './utils/url-to-options'; import {RequestFunction, NormalizedOptions, Response, ResponseObject, AgentByProtocol} from './utils/types'; import dynamicRequire from './utils/dynamic-require'; -export type GetMethodRedirectCodes = 300 | 301 | 302 | 303 | 304 | 305 | 307 | 308; -export type AllMethodRedirectCodes = 300 | 303 | 307 | 308; -export type WithoutBody = 'GET' | 'HEAD'; - -const getMethodRedirectCodes: ReadonlySet = new Set([300, 301, 302, 303, 304, 305, 307, 308]); -const allMethodRedirectCodes: ReadonlySet = new Set([300, 303, 307, 308]); -const withoutBody: ReadonlySet = new Set(['GET', 'HEAD']); +const redirectCodes: ReadonlySet = new Set([300, 301, 302, 303, 304, 307, 308]); +const withoutBody: ReadonlySet = new Set(['GET', 'HEAD']); export interface RequestAsEventEmitter extends EventEmitter { retry: (error: T) => boolean; @@ -143,46 +138,50 @@ export default (options: NormalizedOptions, input?: TransformStream) => { await Promise.all(promises); } - if (options.followRedirect && 'location' in typedResponse.headers) { - if (allMethodRedirectCodes.has(statusCode as AllMethodRedirectCodes) || (getMethodRedirectCodes.has(statusCode as GetMethodRedirectCodes) && (options.method === 'GET' || options.method === 'HEAD'))) { - typedResponse.resume(); // We're being redirected, we don't care about the response. - - if (statusCode === 303) { - // Server responded with "see other", indicating that the resource exists at another location, - // and the client should request it from that location via GET or HEAD. - options.method = 'GET'; - } + if (options.followRedirect && Reflect.has(typedResponse.headers, 'location') && redirectCodes.has(statusCode)) { + typedResponse.resume(); // We're being redirected, we don't care about the response. - if (redirects.length >= options.maxRedirects) { - throw new MaxRedirectsError(typedResponse, options.maxRedirects, options); - } + if (statusCode === 303 && options.method !== 'GET' && options.method !== 'HEAD') { + // Server responded with "see other", indicating that the resource exists at another location, + // and the client should request it from that location via GET or HEAD. + options.method = 'GET'; - // Handles invalid URLs. See https://github.com/sindresorhus/got/issues/604 - const redirectBuffer = Buffer.from(typedResponse.headers.location, 'binary').toString(); - const redirectURL = new URL(redirectBuffer, currentUrl); - redirectString = redirectURL.toString(); + delete options.json; + delete options.form; + delete options.body; + } - redirects.push(redirectString); + if (redirects.length >= options.maxRedirects) { + throw new MaxRedirectsError(typedResponse, options.maxRedirects, options); + } - const redirectOptions = { - ...options, - port: undefined, - auth: undefined, - ...urlToOptions(redirectURL) - }; + // Handles invalid URLs. See https://github.com/sindresorhus/got/issues/604 + const redirectBuffer = Buffer.from(typedResponse.headers.location, 'binary').toString(); + const redirectURL = new URL(redirectBuffer, currentUrl); + redirectString = redirectURL.toString(); - for (const hook of options.hooks.beforeRedirect) { - // eslint-disable-next-line no-await-in-loop - await hook(redirectOptions, typedResponse); - } + redirects.push(redirectString); - emitter.emit('redirect', response, redirectOptions); + const redirectOptions = { + ...options, + port: undefined, + auth: undefined, + ...urlToOptions(redirectURL) + }; - await get(redirectOptions); - return; + for (const hook of options.hooks.beforeRedirect) { + // eslint-disable-next-line no-await-in-loop + await hook(redirectOptions, typedResponse); } + + emitter.emit('redirect', response, redirectOptions); + + await get(redirectOptions); + return; } + delete options.body; + getResponse(typedResponse, options, emitter); } catch (error) { emitError(error); @@ -362,7 +361,7 @@ export default (options: NormalizedOptions, input?: TransformStream) => { const isForm = !is.nullOrUndefined(options.form); const isJSON = !is.nullOrUndefined(options.json); const isBody = !is.nullOrUndefined(body); - if ((isBody || isForm || isJSON) && withoutBody.has(options.method as WithoutBody)) { + if ((isBody || isForm || isJSON) && withoutBody.has(options.method)) { throw new TypeError(`The \`${options.method}\` method cannot be used with a body`); } diff --git a/test/arguments.ts b/test/arguments.ts index 547da096a..14041f5e4 100644 --- a/test/arguments.ts +++ b/test/arguments.ts @@ -313,3 +313,67 @@ test('`context` option is accessible when extending instances', t => { t.is(instance.defaults.options.context, context); t.false({}.propertyIsEnumerable.call(instance.defaults.options, 'context')); }); + +test('`options.body` is cleaned up when retrying - `options.json`', withServer, async (t, server, got) => { + let first = true; + server.post('/', (_request, response) => { + if (first) { + first = false; + + response.statusCode = 401; + } + + response.end(); + }); + + await t.notThrowsAsync(got.post('', { + hooks: { + afterResponse: [ + async (response, retryWithMergedOptions) => { + if (response.statusCode === 401) { + return retryWithMergedOptions(); + } + + t.is(response.request.options.body, undefined); + + return response; + } + ] + }, + json: { + some: 'data' + } + })); +}); + +test('`options.body` is cleaned up when retrying - `options.form`', withServer, async (t, server, got) => { + let first = true; + server.post('/', (_request, response) => { + if (first) { + first = false; + + response.statusCode = 401; + } + + response.end(); + }); + + await t.notThrowsAsync(got.post('', { + hooks: { + afterResponse: [ + async (response, retryWithMergedOptions) => { + if (response.statusCode === 401) { + return retryWithMergedOptions(); + } + + t.is(response.request.options.body, undefined); + + return response; + } + ] + }, + form: { + some: 'data' + } + })); +}); diff --git a/test/redirects.ts b/test/redirects.ts index e47debf6f..0146b75b8 100644 --- a/test/redirects.ts +++ b/test/redirects.ts @@ -120,18 +120,30 @@ test('hostname + path are not breaking redirects', withServer, async (t, server, })).body, 'reached'); }); -test('redirects only GET and HEAD requests', withServer, async (t, server, got) => { - server.post('/', relativeHandler); +test('redirects GET and HEAD requests', withServer, async (t, server, got) => { + server.get('/', (_request, response) => { + response.writeHead(308, { + location: '/' + }); + response.end(); + }); - const error = await t.throwsAsync(got.post({body: 'wow'}), { - instanceOf: got.HTTPError, - message: 'Response code 302 (Found)' + await t.throwsAsync(got.get(''), { + instanceOf: got.MaxRedirectsError }); +}); - // @ts-ignore - t.is(error.options.path, '/'); - // @ts-ignore - t.is(error.response.statusCode, 302); +test('redirects POST requests', withServer, async (t, server, got) => { + server.post('/', (_request, response) => { + response.writeHead(308, { + location: '/' + }); + response.end(); + }); + + await t.throwsAsync(got.post({body: 'wow'}), { + instanceOf: got.MaxRedirectsError + }); }); test('redirects on 303 response even on post, put, delete', withServer, async (t, server, got) => { @@ -279,3 +291,53 @@ test('port is reset on redirect', withServer, async (t, server, got) => { const {body} = await got(''); t.is(body, 'ok'); }); + +test('body is reset on GET redirect', withServer, async (t, server, got) => { + server.post('/', (_request, response) => { + response.writeHead(303, { + location: '/' + }); + response.end(); + }); + + server.get('/', (_request, response) => { + response.end(); + }); + + await got.post('', { + body: 'foobar', + hooks: { + beforeRedirect: [ + options => { + t.is(options.body, undefined); + } + ] + } + }); +}); + +test('body is passed on POST redirect', withServer, async (t, server, got) => { + server.post('/redirect', (_request, response) => { + response.writeHead(302, { + location: '/' + }); + response.end(); + }); + + server.post('/', (request, response) => { + request.pipe(response); + }); + + const {body} = await got.post('redirect', { + body: 'foobar', + hooks: { + beforeRedirect: [ + options => { + t.is(options.body, 'foobar'); + } + ] + } + }); + + t.is(body, 'foobar'); +});