feat: add support for HTTP redirect (#341)

Corepack will now follow the `Location` header when receiving a 30x HTTP status code. This should allow Corepack to work with proxies that redirect to a different URL.
nodejs · Dec 29, 2023 · 6df5063 · 6df5063
1 parent e8ae337
commit 6df5063
Show file tree

Hide file tree

Showing 2 changed files with 115 additions and 12 deletions.
diff --git a/sources/httpUtils.ts b/sources/httpUtils.ts
@@ -1,6 +1,6 @@
-import {UsageError}      from 'clipanion';
-import {RequestOptions}  from 'https';
-import {IncomingMessage} from 'http';
+import {UsageError}                     from 'clipanion';
+import {RequestOptions}                 from 'https';
+import {IncomingMessage, ClientRequest} from 'http';
 
 export async function fetchUrlStream(url: string, options: RequestOptions = {}) {
   if (process.env.COREPACK_ENABLE_NETWORK === `0`)
@@ -13,17 +13,25 @@ export async function fetchUrlStream(url: string, options: RequestOptions = {})
   const proxyAgent = new ProxyAgent();
 
   return new Promise<IncomingMessage>((resolve, reject) => {
-    const request = https.get(url, {...options, agent: proxyAgent}, response => {
-      const statusCode = response.statusCode;
-      if (statusCode != null && statusCode >= 200 && statusCode < 300)
-        return resolve(response);
+    const createRequest = (url: string) => {
+      const request: ClientRequest = https.get(url, {...options, agent: proxyAgent}, response => {
+        const statusCode = response.statusCode;
 
-      return reject(new Error(`Server answered with HTTP ${statusCode} when performing the request to ${url}; for troubleshooting help, see https://github.com/nodejs/corepack#troubleshooting`));
-    });
+        if ([301, 302, 307, 308].includes(statusCode as number) && response.headers.location)
+          return createRequest(response.headers.location as string);
 
-    request.on(`error`, err => {
-      reject(new Error(`Error when performing the request to ${url}; for troubleshooting help, see https://github.com/nodejs/corepack#troubleshooting`));
-    });
+        if (statusCode != null && statusCode >= 200 && statusCode < 300)
+          return resolve(response);
+
+        return reject(new Error(`Server answered with HTTP ${statusCode} when performing the request to ${url}; for troubleshooting help, see https://github.com/nodejs/corepack#troubleshooting`));
+      });
+
+      request.on(`error`, err => {
+        reject(new Error(`Error when performing the request to ${url}; for troubleshooting help, see https://github.com/nodejs/corepack#troubleshooting`));
+      });
+    };
+
+    createRequest(url);
   });
 }
 

diff --git a/tests/httpUtils.test.ts b/tests/httpUtils.test.ts
@@ -0,0 +1,95 @@
+import {jest, describe, beforeEach, beforeAll, it, expect} from '@jest/globals';
+
+import {fetchUrlStream}                                    from '../sources/httpUtils';
+
+
+describe(`http utils fetchUrlStream`, () => {
+  const getUrl = (statusCode: number | string, redirectCode?: number | string) =>
+    `https://registry.example.org/answered/${statusCode}${redirectCode ? `?redirectCode=${redirectCode}` : ``}`;
+
+  const httpsGetFn = jest.fn((url: string, _, callback: (response: any) => void) => {
+    const parsedURL = new URL(url);
+    const statusCode = parsedURL.pathname.slice(parsedURL.pathname.lastIndexOf(`/`) + 1);
+    const response = {url, statusCode: +statusCode};
+    const errorCallbacks: Array<(err: string) => void> = [];
+
+    if ([301, 302, 307, 308].includes(+statusCode)) {
+      const redirectCode = parsedURL.searchParams.get(`redirectCode`)!;
+      // mock response.headers.location
+      if (redirectCode) {
+        Reflect.set(response, `headers`, {location: getUrl(redirectCode)});
+      }
+    }
+
+    // handle request.on('error', err => ...)
+    if (statusCode === `error`)
+      process.nextTick(() => errorCallbacks.forEach(cb => cb(`Test internal error`)));
+    else
+      callback(response);
+
+    return {
+      on: (type: string, callback: (err: string) => void) => {
+        if (type === `error`) {
+          errorCallbacks.push(callback);
+        }
+      },
+    };
+  });
+
+  beforeAll(() => {
+    jest.doMock(`https`, () => ({
+      get: httpsGetFn,
+      Agent: class Agent {},
+    }));
+  });
+
+  beforeEach(() => {
+    httpsGetFn.mockClear();
+  });
+
+  it(`correct response answered statusCode should be >= 200 and < 300`, async () => {
+    await expect(fetchUrlStream(getUrl(200))).resolves.toMatchObject({
+      statusCode: 200,
+    });
+
+    await expect(fetchUrlStream(getUrl(299))).resolves.toMatchObject({
+      statusCode: 299,
+    });
+
+    expect(httpsGetFn).toHaveBeenCalledTimes(2);
+  });
+
+  it(`bad response`, async () => {
+    await expect(fetchUrlStream(getUrl(300))).rejects.toThrowError();
+    await expect(fetchUrlStream(getUrl(199))).rejects.toThrowError();
+  });
+
+  it(`redirection with correct response`, async () => {
+    await expect(fetchUrlStream(getUrl(301, 200))).resolves.toMatchObject({
+      statusCode: 200,
+    });
+
+    expect(httpsGetFn).toHaveBeenCalledTimes(2);
+
+    await expect(fetchUrlStream(getUrl(308, 299))).resolves.toMatchObject({
+      statusCode: 299,
+    });
+
+    expect(httpsGetFn).toHaveBeenCalledTimes(4);
+  });
+
+  it(`redirection with bad response`, async () => {
+    await expect(fetchUrlStream(getUrl(301, 300))).rejects.toThrowError();
+    await expect(fetchUrlStream(getUrl(308, 199))).rejects.toThrowError();
+    await expect(fetchUrlStream(getUrl(301, 302))).rejects.toThrowError();
+    await expect(fetchUrlStream(getUrl(307))).rejects.toThrowError();
+  });
+
+  it(`rejects with error`, async () => {
+    await expect(fetchUrlStream(getUrl(`error`))).rejects.toThrowError();
+  });
+
+  it(`rejects when redirection with error`, async () => {
+    await expect(fetchUrlStream(getUrl(307, `error`))).rejects.toThrowError();
+  });
+});