From c47191b4255534771dc320960c39a25645823800 Mon Sep 17 00:00:00 2001 From: Paul Faid Date: Sat, 1 May 2021 09:04:49 +1200 Subject: [PATCH] Port redirect policy changes from azure-sdk-for-js --- Changelog.md | 4 +- lib/fetchHttpClient.ts | 7 +- lib/policies/redirectPolicy.ts | 39 +++- lib/serviceClient.ts | 16 +- lib/util/constants.ts | 2 +- package.json | 2 +- test/policies/redirectPolicyTests.ts | 274 +++++++++++++++++++++++++++ test/redirectLimitTests.ts | 186 +++++++++--------- 8 files changed, 413 insertions(+), 117 deletions(-) create mode 100644 test/policies/redirectPolicyTests.ts diff --git a/Changelog.md b/Changelog.md index c09c625..3efcf38 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,6 +1,8 @@ # Changelog -## 2.3.1 - UNRELEASED +## 2.5.0 - UNRELEASED + - Add WebResource.redirectLimit: Limit the number of redirects followed for this request. If set to 0, redirects will not be followed. +- Port changes to redirect policy from [azure-sdk-for-js](https://github.com/Azure/azure-sdk-for-js/pull/11863/files] ## 2.4.0 - 2021-04-19 diff --git a/lib/fetchHttpClient.ts b/lib/fetchHttpClient.ts index 5994f1c..d28b011 100644 --- a/lib/fetchHttpClient.ts +++ b/lib/fetchHttpClient.ts @@ -130,11 +130,6 @@ export abstract class FetchHttpClient implements HttpClient { body = uploadReportStream; } - const redirectInit: Partial = {}; - if (httpRequest.redirectLimit !== undefined) { - redirectInit.redirect = "manual"; - } - const platformSpecificRequestInit: Partial = await this.prepareRequest( httpRequest ); @@ -144,7 +139,7 @@ export abstract class FetchHttpClient implements HttpClient { headers: httpRequest.headers.rawHeaders(), method: httpRequest.method, signal: abortController.signal, - ...redirectInit, + redirect: "manual", ...platformSpecificRequestInit, }; diff --git a/lib/policies/redirectPolicy.ts b/lib/policies/redirectPolicy.ts index a2b025a..5d6b47d 100644 --- a/lib/policies/redirectPolicy.ts +++ b/lib/policies/redirectPolicy.ts @@ -11,6 +11,27 @@ import { RequestPolicyOptionsLike, } from "./requestPolicy"; +/** + * Options for how redirect responses are handled. + */ +export interface RedirectOptions { + /* + * When true, redirect responses are followed. Defaults to true. + */ + handleRedirects: boolean; + + /* + * The maximum number of times the redirect URL will be tried before + * failing. Defaults to 20. + */ + maxRetries?: number; +} + +export const DefaultRedirectOptions: RedirectOptions = { + handleRedirects: true, + maxRetries: 20, +}; + export function redirectPolicy(maximumRetries = 20): RequestPolicyFactory { return { create: (nextPolicy: RequestPolicy, options: RequestPolicyOptionsLike) => { @@ -44,12 +65,14 @@ function handleRedirect( const locationHeader = response.headers.get("location"); if ( locationHeader && - (status === 300 || status === 302 || status === 307 || (status === 303 && request.method === "POST")) && - ( - (request.redirectLimit !== undefined && currentRetries < request.redirectLimit) - || - (request.redirectLimit === undefined && currentRetries < policy.maxRetries) - )) { + (status === 300 || + (status === 301 && ["GET", "HEAD"].includes(request.method)) || + (status === 302 && ["GET", "POST", "HEAD"].includes(request.method)) || + (status === 303 && "POST" === request.method) || + status === 307) && + ((request.redirectLimit !== undefined && currentRetries < request.redirectLimit) || + (request.redirectLimit === undefined && currentRetries < policy.maxRetries)) + ) { const builder = URLBuilder.parse(request.url); builder.setPath(locationHeader); request.url = builder.toString(); @@ -57,8 +80,9 @@ function handleRedirect( // POST request with Status code 302 and 303 should be converted into a // redirected GET request if the redirect url is present in the location header // reference: https://tools.ietf.org/html/rfc7231#page-57 && https://fetch.spec.whatwg.org/#http-redirect-fetch - if (status === 302 || status === 303) { + if ((status === 302 || status === 303) && request.method === "POST") { request.method = "GET"; + delete request.body; } return policy._nextPolicy @@ -78,4 +102,3 @@ function recordRedirect(response: HttpOperationResponse, redirect: string): Http } return response; } - diff --git a/lib/serviceClient.ts b/lib/serviceClient.ts index 9ff8b0a..8161adc 100644 --- a/lib/serviceClient.ts +++ b/lib/serviceClient.ts @@ -26,7 +26,7 @@ import { getDefaultUserAgentHeaderName, getDefaultUserAgentValue, } from "./policies/userAgentPolicy"; -import { redirectPolicy } from "./policies/redirectPolicy"; +import { DefaultRedirectOptions, RedirectOptions, redirectPolicy } from "./policies/redirectPolicy"; import { RequestPolicy, RequestPolicyFactory, @@ -135,6 +135,10 @@ export interface ServiceClientOptions { * Proxy settings which will be used for every HTTP request (Node.js only). */ proxySettings?: ProxySettings; + /** + * Options for how redirect responses are handled. + */ + redirectOptions?: RedirectOptions; /** * HTTP and HTTPS agents which will be used for every HTTP request (Node.js only). */ @@ -581,7 +585,15 @@ function createDefaultRequestPolicyFactories( if (userAgentHeaderName && userAgentHeaderValue) { factories.push(userAgentPolicy({ key: userAgentHeaderName, value: userAgentHeaderValue })); } - factories.push(redirectPolicy()); + + const redirectOptions = { + ...DefaultRedirectOptions, + ...options.redirectOptions, + }; + if (redirectOptions.handleRedirects) { + factories.push(redirectPolicy(redirectOptions.maxRetries)); + } + factories.push(rpRegistrationPolicy(options.rpRegistrationRetryTimeout)); if (!options.noRetryPolicy) { diff --git a/lib/util/constants.ts b/lib/util/constants.ts index 5437b78..dce9325 100644 --- a/lib/util/constants.ts +++ b/lib/util/constants.ts @@ -7,7 +7,7 @@ export const Constants = { * @const * @type {string} */ - msRestVersion: "2.4.0", + msRestVersion: "2.5.0", /** * Specifies HTTP. diff --git a/package.json b/package.json index 80a4f7a..56d86d5 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "email": "azsdkteam@microsoft.com", "url": "https://github.com/Azure/ms-rest-js" }, - "version": "2.4.0", + "version": "2.5.0", "description": "Isomorphic client Runtime for Typescript/node.js/browser javascript client libraries generated using AutoRest", "tags": [ "isomorphic", diff --git a/test/policies/redirectPolicyTests.ts b/test/policies/redirectPolicyTests.ts new file mode 100644 index 0000000..9595580 --- /dev/null +++ b/test/policies/redirectPolicyTests.ts @@ -0,0 +1,274 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import { assert } from "chai"; +import { RedirectPolicy } from "../../lib/policies/redirectPolicy"; +import { WebResource } from "../../lib/webResource"; +import { HttpOperationResponse } from "../../lib/httpOperationResponse"; +import { HttpHeaders } from "../../lib/httpHeaders"; +import { RequestPolicyOptions } from "../../lib/policies/requestPolicy"; + +describe("RedirectPolicy", () => { + it("should not follow redirect if no location header", async () => { + const responseCodes = [301]; + const request = new WebResource("https://example.com", "GET"); + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, 301); + }); + + it("should not follow POST 301 redirect", async function () { + const expectedStatusCode = 301; + const responseCodes = [301]; + const request = new WebResource("https://example.com", "POST"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should follow GET 301 redirect", async function () { + const expectedStatusCode = 200; + const responseCodes = [301]; + const request = new WebResource("https://example.com", "GET"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should follow HEAD 301 redirect", async function () { + const expectedStatusCode = 200; + const responseCodes = [301]; + const request = new WebResource("https://example.com", "HEAD"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should follow POST 302 redirect", async function () { + const expectedStatusCode = 200; + const responseCodes = [302]; + const request = new WebResource("https://example.com", "POST"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should follow GET 302 redirect", async function () { + const expectedStatusCode = 200; + const responseCodes = [302]; + const request = new WebResource("https://example.com", "GET"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should follow HEAD 302 redirect", async function () { + const expectedStatusCode = 200; + const responseCodes = [302]; + const request = new WebResource("https://example.com", "HEAD"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should follow POST 303 redirect", async function () { + const expectedStatusCode = 200; + const responseCodes = [303]; + const request = new WebResource("https://example.com", "POST"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + if (!responseCodes.length) { + assert.strictEqual(_requestToSend.method, "GET", "Expected second request to be GET"); + } + + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should not follow GET 303 redirect", async function () { + const expectedStatusCode = 303; + const responseCodes = [303]; + const request = new WebResource("https://example.com", "GET"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should follow GET 307 redirect", async function () { + const expectedStatusCode = 200; + const responseCodes = [307]; + const request = new WebResource("https://example.com", "GET"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should follow GET 300 redirect", async function () { + const expectedStatusCode = 200; + const responseCodes = [300]; + const request = new WebResource("https://example.com", "GET"); + const headers = [{ location: "https://example.com/new" }]; + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders(headers.shift() || {}), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should only try maxretries", async function () { + const expectedStatusCode = 300; + const maxretries = 1; + const responseCodes = [300, 300, 200]; + const request = new WebResource("https://example.com", "GET"); + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + return Promise.resolve({ + status: responseCodes.shift() || 200, + request: _requestToSend, + headers: new HttpHeaders({ location: "https://example.com/new" }), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions(), maxretries); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + }); + + it("should try to redirect 3 times by default", async function () { + const expectedStatusCode = 300; + let callCount = 0; + const request = new WebResource("https://example.com", "GET"); + const nextPolicy = { + sendRequest: (_requestToSend: WebResource): Promise => { + callCount++; + return Promise.resolve({ + status: 300, + request: _requestToSend, + headers: new HttpHeaders({ location: "https://example.com/new" }), + }); + }, + }; + const policy = new RedirectPolicy(nextPolicy, new RequestPolicyOptions()); + const result = await policy.sendRequest(request); + + assert.strictEqual(result.status, expectedStatusCode); + assert.strictEqual(callCount, 21); + }); +}); diff --git a/test/redirectLimitTests.ts b/test/redirectLimitTests.ts index fc20467..5198ab6 100644 --- a/test/redirectLimitTests.ts +++ b/test/redirectLimitTests.ts @@ -5,7 +5,6 @@ import { expect } from "chai"; import sinon from "sinon"; import { DefaultHttpClient } from "../lib/defaultHttpClient"; -import { WebResource } from "../lib/webResource"; import { getHttpMock, HttpMockFacade } from "./mockHttp"; import { CommonResponse } from "../lib/fetchHttpClient"; import { ServiceClient } from "../lib/serviceClient"; @@ -16,13 +15,10 @@ import { redirectPolicy } from "../lib/policies/redirectPolicy"; const nodeIt = (isNode ? it : it.skip) as TestFunction; describe("redirectLimit", function () { - let httpMock: HttpMockFacade; - let capturedRedirectInit: string | undefined; beforeEach(() => { httpMock = getHttpMock(); httpMock.setup(); - capturedRedirectInit = undefined; }); afterEach(() => httpMock.teardown()); @@ -31,7 +27,6 @@ describe("redirectLimit", function () { const fetchMock = httpMock.getFetch(); if (fetchMock) { sinon.stub(httpClient, "fetch").callsFake(async (input, init) => { - capturedRedirectInit = init?.redirect; const response = await fetchMock(input, init); return (response as unknown) as CommonResponse; }); @@ -39,130 +34,125 @@ describe("redirectLimit", function () { return httpClient; } - async function executeRequestWithRedirectLimit(redirectLimit?: number) { - const resourceUrl = "/resource"; - - httpMock.get(resourceUrl, async () => { - return { status: 200 }; - }); - - const httpClient = getMockedHttpClient(); - const request = new WebResource().prepare({ url: resourceUrl, method: "GET", redirectLimit}); - - // Act - await httpClient.sendRequest(request); - } - - nodeIt("should initiate fetch without overriding redirect when redirectLimit is undefined", async function () { - await executeRequestWithRedirectLimit(undefined); - - expect(capturedRedirectInit).to.be.undefined; - }); - - nodeIt("should initiate fetch with manual redirect when redirectLimit is 0", async function () { - await executeRequestWithRedirectLimit(0); - - expect(capturedRedirectInit).to.equal("manual"); - }); - - nodeIt("should initiate fetch with manual redirect when redirectLimit is greater than 0", async function () { - await executeRequestWithRedirectLimit(3); - - expect(capturedRedirectInit).to.equal("manual"); - }); - const resourceUrl = "/resource"; const redirectedUrl_1 = "/redirected_1"; const redirectedUrl_2 = "/redirected_2"; function configureMockRedirectResponses() { httpMock.get(resourceUrl, async () => { - return { status: 300, headers : {"location": redirectedUrl_1} }; + return { status: 300, headers: { location: redirectedUrl_1 } }; }); httpMock.get(redirectedUrl_1, async () => { - return { status: 300, headers : {"location": redirectedUrl_2} }; + return { status: 300, headers: { location: redirectedUrl_2 } }; }); httpMock.get(redirectedUrl_2, async () => { return { status: 200 }; }); } - nodeIt("of 20 should follow redirects and return last visited url in response.url", async function () { - configureMockRedirectResponses(); + nodeIt( + "of 20 should follow redirects and return last visited url in response.url", + async function () { + configureMockRedirectResponses(); - const client = new ServiceClient(undefined, { - httpClient: getMockedHttpClient() - }); + const client = new ServiceClient(undefined, { + httpClient: getMockedHttpClient(), + }); - // Act - const response = await client.sendRequest({ url: resourceUrl, method: "GET", redirectLimit: 20}); + // Act + const response = await client.sendRequest({ + url: resourceUrl, + method: "GET", + redirectLimit: 20, + }); - expect(response.status).to.equal(200); - expect(response.redirected).to.be.true; - expect(response.url).to.equal(redirectedUrl_2); - }); + expect(response.status).to.equal(200); + expect(response.redirected).to.be.true; + expect(response.url).to.equal(redirectedUrl_2); + } + ); - nodeIt("of 0 should not follow redirects and should return last visited url in response.url", async function () { - configureMockRedirectResponses(); + nodeIt( + "of 0 should not follow redirects and should return last visited url in response.url", + async function () { + configureMockRedirectResponses(); - const client = new ServiceClient(undefined, { - httpClient: getMockedHttpClient() - }); + const client = new ServiceClient(undefined, { + httpClient: getMockedHttpClient(), + }); - // Act - const response = await client.sendRequest({ url: resourceUrl, method: "GET", redirectLimit: 0}); + // Act + const response = await client.sendRequest({ + url: resourceUrl, + method: "GET", + redirectLimit: 0, + }); - expect(response.status).to.equal(300); - expect(response.headers.get("location")).to.equal(redirectedUrl_1); - expect(response.redirected).to.be.false; - expect(response.url).to.equal(resourceUrl); - }); + expect(response.status).to.equal(300); + expect(response.headers.get("location")).to.equal(redirectedUrl_1); + expect(response.redirected).to.be.false; + expect(response.url).to.equal(resourceUrl); + } + ); - nodeIt("of 1 should follow 1 redirect and return last visited url in response.url", async function () { - configureMockRedirectResponses(); + nodeIt( + "of 1 should follow 1 redirect and return last visited url in response.url", + async function () { + configureMockRedirectResponses(); - const client = new ServiceClient(undefined, { - httpClient: getMockedHttpClient() - }); + const client = new ServiceClient(undefined, { + httpClient: getMockedHttpClient(), + }); - // Act - const response = await client.sendRequest({ url: resourceUrl, method: "GET", redirectLimit: 1}); + // Act + const response = await client.sendRequest({ + url: resourceUrl, + method: "GET", + redirectLimit: 1, + }); - expect(response.status).to.equal(300); - expect(response.headers.get("location")).to.equal(redirectedUrl_2); - expect(response.redirected).to.be.true; - expect(response.url).to.equal(redirectedUrl_1); - }); + expect(response.status).to.equal(300); + expect(response.headers.get("location")).to.equal(redirectedUrl_2); + expect(response.redirected).to.be.true; + expect(response.url).to.equal(redirectedUrl_1); + } + ); - nodeIt("of undefined should follow redirects and return last visited url in response.url", async function () { - configureMockRedirectResponses(); + nodeIt( + "of undefined should follow redirects and return last visited url in response.url", + async function () { + configureMockRedirectResponses(); - const client = new ServiceClient(undefined, { - httpClient: getMockedHttpClient() - }); + const client = new ServiceClient(undefined, { + httpClient: getMockedHttpClient(), + }); - // Act - const response = await client.sendRequest({ url: resourceUrl, method: "GET" }); + // Act + const response = await client.sendRequest({ url: resourceUrl, method: "GET" }); - expect(response.status).to.equal(200); - expect(response.redirected).to.be.true; - expect(response.url).to.equal(redirectedUrl_2); - }); + expect(response.status).to.equal(200); + expect(response.redirected).to.be.true; + expect(response.url).to.equal(redirectedUrl_2); + } + ); - nodeIt("of undefinded with policy limit of 1 should follow 1 redirect and return last visited url in response.url", async function () { - configureMockRedirectResponses(); + nodeIt( + "of undefinded with policy limit of 1 should follow 1 redirect and return last visited url in response.url", + async function () { + configureMockRedirectResponses(); - const client = new ServiceClient(undefined, { - httpClient: getMockedHttpClient(), - requestPolicyFactories: [redirectPolicy(1)], - }); + const client = new ServiceClient(undefined, { + httpClient: getMockedHttpClient(), + requestPolicyFactories: [redirectPolicy(1)], + }); - // Act - const response = await client.sendRequest({ url: resourceUrl, method: "GET" }); + // Act + const response = await client.sendRequest({ url: resourceUrl, method: "GET" }); - expect(response.status).to.equal(300); - expect(response.headers.get("location")).to.equal(redirectedUrl_2); - expect(response.redirected).to.be.true; - expect(response.url).to.equal(redirectedUrl_1); - }); + expect(response.status).to.equal(300); + expect(response.headers.get("location")).to.equal(redirectedUrl_2); + expect(response.redirected).to.be.true; + expect(response.url).to.equal(redirectedUrl_1); + } + ); });