Port redirect policy changes from azure-sdk-for-js

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
 

lib/fetchHttpClient.ts

@@ -130,11 +130,6 @@ export abstract class FetchHttpClient implements HttpClient {
       body = uploadReportStream;
     }
 
-    const redirectInit: Partial<RequestInit> = {};
-    if (httpRequest.redirectLimit !== undefined) {
-      redirectInit.redirect = "manual";
-    }
-
     const platformSpecificRequestInit: Partial<RequestInit> = 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,
     };
 

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")) &&
+    (status === 300 ||
-    (
+      (status === 301 && ["GET", "HEAD"].includes(request.method)) ||
-      (request.redirectLimit !== undefined && currentRetries < request.redirectLimit)
+      (status === 302 && ["GET", "POST", "HEAD"].includes(request.method)) ||
-      ||
+      (status === 303 && "POST" === request.method) ||
-      (request.redirectLimit === undefined && currentRetries < policy.maxRetries)
+      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;
 }
-

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) {

lib/util/constants.ts

@@ -7,7 +7,7 @@ export const Constants = {
    * @const
    * @type {string}
    */
-  msRestVersion: "2.4.0",
+  msRestVersion: "2.5.0",
 
   /**
    * Specifies HTTP.

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",

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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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<HttpOperationResponse> => {
+        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);
+  });
+});

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,106 +34,97 @@ 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 () {
+  nodeIt(
+    "of 20 should follow redirects and return last visited url in response.url",
+    async function () {
       configureMockRedirectResponses();
 
       const client = new ServiceClient(undefined, {
-      httpClient: getMockedHttpClient()
+        httpClient: getMockedHttpClient(),
       });
 
       // Act
-    const response = await client.sendRequest({ url: resourceUrl, method: "GET", redirectLimit: 20});
+      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);
-  });
+    }
+  );
 
-  nodeIt("of 0 should not follow redirects and should return last visited url in response.url", async function () {
+  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()
+        httpClient: getMockedHttpClient(),
       });
 
       // Act
-    const response = await client.sendRequest({ url: resourceUrl, method: "GET", redirectLimit: 0});
+      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);
-  });
+    }
+  );
 
-  nodeIt("of 1 should follow 1 redirect and return last visited url in response.url", async function () {
+  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()
+        httpClient: getMockedHttpClient(),
       });
 
       // Act
-    const response = await client.sendRequest({ url: resourceUrl, method: "GET", redirectLimit: 1});
+      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);
-  });
+    }
+  );
 
-  nodeIt("of undefined should follow redirects and return last visited url in response.url", async function () {
+  nodeIt(
+    "of undefined should follow redirects and return last visited url in response.url",
+    async function () {
       configureMockRedirectResponses();
 
       const client = new ServiceClient(undefined, {
-      httpClient: getMockedHttpClient()
+        httpClient: getMockedHttpClient(),
       });
 
       // Act
@@ -147,9 +133,12 @@ describe("redirectLimit", function () {
       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 () {
+  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, {
@@ -164,5 +153,6 @@ describe("redirectLimit", function () {
       expect(response.headers.get("location")).to.equal(redirectedUrl_2);
       expect(response.redirected).to.be.true;
       expect(response.url).to.equal(redirectedUrl_1);
-  });
+    }
+  );
 });