Follow redirects on transmitter requests

Implement following redirects for `Transmitter` requests. As per the HTTP spec and common browser behaviour, redirects using status codes 301, 302 and 303 have the method changed to `GET`, and redirection loops are detected.
appsignal · Mar 30, 2022 · 4ab4814 · 4ab4814
1 parent f3eaf18
commit 4ab4814
Show file tree

Hide file tree

Showing 3 changed files with 210 additions and 22 deletions.
diff --git a/...ges/nodejs/.changesets/follow-redirects-when-downloading-the-appsignal-agent.md b/...ges/nodejs/.changesets/follow-redirects-when-downloading-the-appsignal-agent.md
@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "fix"
+---
+
+Follow redirects when downloading the AppSignal agent. This fixes an issue where the redirect page would be downloaded instead of the agent, causing the agent installation to fail.
diff --git a/packages/nodejs/src/__tests__/transmitter.test.ts b/packages/nodejs/src/__tests__/transmitter.test.ts
@@ -15,6 +15,7 @@ describe("Transmitter", () => {
       nock.activate()
     }
 
+    nock.cleanAll()
     nock.disableNetConnect()
 
     originalEnv = { ...process.env }
@@ -263,6 +264,136 @@ describe("Transmitter", () => {
       expect(callback).not.toHaveBeenCalled()
     })
 
+    it("follows redirects", async () => {
+      nock("http://example.invalid")
+        .get("/301")
+        .reply(301, undefined, {
+          Location: "http://example.invalid/302"
+        })
+        .get("/302")
+        .reply(302, undefined, {
+          Location: "http://example.invalid/303"
+        })
+        .get("/303")
+        .reply(303, undefined, {
+          Location: "http://example.invalid/307"
+        })
+        .get("/307")
+        .reply(307, undefined, {
+          Location: "http://example.invalid/308"
+        })
+        .get("/308")
+        .reply(308, undefined, {
+          Location: "http://example.invalid/foo"
+        })
+        .get("/foo")
+        .reply(200, "response body")
+
+      const { callback, onData, onError } = await transmitterRequest(
+        "GET",
+        "http://example.invalid/301"
+      )
+
+      expect(callback).toHaveBeenCalledWith(
+        expect.objectContaining({ statusCode: 200 })
+      )
+      expect(callback).toHaveBeenCalledTimes(1)
+
+      expect(onData).toHaveBeenCalledWith(Buffer.from("response body"))
+      expect(onError).not.toHaveBeenCalled()
+    })
+
+    it("redirects to GET method for status code 301/302/303", async () => {
+      nock("http://example.invalid")
+        .post("/301", "request body")
+        .reply(301, undefined, {
+          Location: "http://example.invalid/foo"
+        })
+        .post("/302", "request body")
+        .reply(302, undefined, {
+          Location: "http://example.invalid/foo"
+        })
+        .post("/303", "request body")
+        .reply(303, undefined, {
+          Location: "http://example.invalid/foo"
+        })
+        .get("/foo")
+        .times(3)
+        .reply(200, "response body")
+
+      for (const statusCode of [301, 302, 303]) {
+        const { callback, onData, onError } = await transmitterRequest(
+          "POST",
+          `http://example.invalid/${statusCode}`,
+          "request body"
+        )
+
+        expect(callback).toHaveBeenCalledWith(
+          expect.objectContaining({ statusCode: 200 })
+        )
+
+        expect(onData).toHaveBeenCalledWith(Buffer.from("response body"))
+        expect(onError).not.toHaveBeenCalled()
+      }
+    })
+
+    it("redirects to the same method for status code 307/308", async () => {
+      nock("http://example.invalid")
+        .post("/307", "request body")
+        .reply(307, undefined, {
+          Location: "http://example.invalid/foo"
+        })
+        .post("/308", "request body")
+        .reply(308, undefined, {
+          Location: "http://example.invalid/foo"
+        })
+        .post("/foo", "request body")
+        .times(2)
+        .reply(200, "response body")
+
+      for (const statusCode of [307, 308]) {
+        const { callback, onData, onError } = await transmitterRequest(
+          "POST",
+          `http://example.invalid/${statusCode}`,
+          "request body"
+        )
+
+        expect(callback).toHaveBeenCalledWith(
+          expect.objectContaining({ statusCode: 200 })
+        )
+
+        expect(onData).toHaveBeenCalledWith(Buffer.from("response body"))
+        expect(onError).not.toHaveBeenCalled()
+      }
+    })
+
+    it("throws an error on a redirect loop", async () => {
+      nock("http://example.invalid")
+        .persist()
+        .get("/foo")
+        .reply(302, undefined, {
+          Location: "http://example.invalid/bar"
+        })
+        .get("/bar")
+        .reply(302, undefined, {
+          Location: "http://example.invalid/foo"
+        })
+
+      const { callback, onData, onError } = await transmitterRequest(
+        "GET",
+        `http://example.invalid/foo`
+      )
+
+      expect(callback).not.toHaveBeenCalled()
+      expect(onData).not.toHaveBeenCalled()
+      expect(onError).toHaveBeenCalledWith(
+        expect.objectContaining({
+          name: "MaxRedirectsError",
+          message: "Maximum number of redirects reached"
+        })
+      )
+    })
+
     it("uses the CA file from the config", async () => {
       // disable nock, so we can mock the https library ourselves
       nock.restore()
@@ -277,16 +408,13 @@ describe("Transmitter", () => {
 
       const mock = mockRequest(https)
 
-      const { callback } = await transmitterRequest(
-        "GET",
-        "https://example.invalid"
-      )
+      await transmitterRequest("GET", "https://example.invalid")
 
       expect(fsReadFileSyncMock).toHaveBeenCalledWith("/foo/bar", "utf-8")
 
       expect(mock).toHaveBeenCalledWith(
         expect.objectContaining({ ca: "ca file contents" }),
-        callback
+        expect.any(Function)
       )
     })
 
@@ -308,14 +436,11 @@ describe("Transmitter", () => {
 
       const mock = mockRequest(https)
 
-      const { callback } = await transmitterRequest(
-        "GET",
-        "https://example.invalid"
-      )
+      await transmitterRequest("GET", "https://example.invalid")
 
       expect(mock).toHaveBeenCalledWith(
         expect.not.objectContaining({ ca: expect.anything() }),
-        callback
+        expect.any(Function)
       )
 
       expect(consoleWarnMock).toHaveBeenCalledTimes(1)
@@ -331,15 +456,12 @@ describe("Transmitter", () => {
       const httpMock = mockRequest(http)
       const httpsMock = mockRequest(https)
 
-      const { callback } = await transmitterRequest(
-        "GET",
-        "http://example.invalid"
-      )
+      await transmitterRequest("GET", "http://example.invalid")
 
       expect(httpsMock).not.toHaveBeenCalled()
       expect(httpMock).toHaveBeenCalledWith(
         expect.not.objectContaining({ ca: expect.anything() }),
-        callback
+        expect.any(Function)
       )
     })
   })

diff --git a/packages/nodejs/src/transmitter.ts b/packages/nodejs/src/transmitter.ts
@@ -5,13 +5,33 @@ import http from "http"
 import { Configuration } from "./config"
 import { URL, URLSearchParams } from "url"
 
+const REDIRECT_COUNT = Symbol("redirect-count")
+
 type TransmitterRequestOptions = {
   method: string
   params?: URLSearchParams
-  callback: (stream: http.IncomingMessage) => void
+  callback: ((stream: http.IncomingMessage) => void) & {
+    [REDIRECT_COUNT]?: number
+  }
   onError: (error: Error) => void
 }
 
+const REDIRECT_STATUS_CODES = [301, 302, 303, 307, 308]
+const REDIRECT_GET_STATUS_CODES = [301, 302, 303]
+const MAX_REDIRECTS = 20
+
+class MaxRedirectsError extends Error {
+  constructor() {
+    super("Maximum number of redirects reached")
+
+    if (Error.captureStackTrace) {
+      Error.captureStackTrace(this, MaxRedirectsError)
+    }
+
+    this.name = "MaxRedirectsError"
+  }
+}
+
 export class Transmitter {
   #config: Configuration
   #url: string
@@ -79,12 +99,9 @@ export class Transmitter {
     })
   }
 
-  public request({
-    method,
-    params = new URLSearchParams(),
-    callback,
-    onError
-  }: TransmitterRequestOptions) {
+  public request(requestOptions: TransmitterRequestOptions) {
+    const { method, params = new URLSearchParams(), onError } = requestOptions
+
     const initialOptions = {
       method,
       ...this.urlRequestOptions()
@@ -101,6 +118,8 @@ export class Transmitter {
 
     const module = this.requestModule(protocol ?? "")
 
+    const callback = this.handleRedirectsCallback(requestOptions)
+
     const request = module.request(options, callback)
 
     request.on("error", onError)
@@ -109,6 +128,47 @@ export class Transmitter {
     request.end()
   }
 
+  private handleRedirectsCallback({
+    method,
+    params,
+    callback,
+    onError
+  }: TransmitterRequestOptions): (stream: http.IncomingMessage) => void {
+    return stream => {
+      const responseStatus = stream.statusCode ?? 999
+      const isRedirect = REDIRECT_STATUS_CODES.indexOf(responseStatus) !== -1
+      const newURL = stream.headers?.location
+
+      if (isRedirect && typeof newURL !== "undefined") {
+        const redirectCount = callback[REDIRECT_COUNT] ?? 0
+
+        if (redirectCount >= MAX_REDIRECTS) {
+          onError(new MaxRedirectsError())
+        } else {
+          callback[REDIRECT_COUNT] = redirectCount + 1
+
+          let newMethod = method
+          const isGetRedirect =
+            REDIRECT_GET_STATUS_CODES.indexOf(responseStatus) !== -1
+
+          if (isGetRedirect) {
+            newMethod = "GET"
+          }
+
+          const newTransmitter = new Transmitter(newURL, this.#body)
+          newTransmitter.request({
+            method: newMethod,
+            params,
+            callback,
+            onError
+          })
+        }
+      } else {
+        callback(stream)
+      }
+    }
+  }
+
   private configParams(): URLSearchParams {
     const config_data = this.#config.data