From 4ab481483e86425d1b6c072c4f85f88768c792c6 Mon Sep 17 00:00:00 2001 From: Noemi Date: Wed, 30 Mar 2022 16:36:22 +0200 Subject: [PATCH] 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. --- ...ts-when-downloading-the-appsignal-agent.md | 6 + .../nodejs/src/__tests__/transmitter.test.ts | 152 ++++++++++++++++-- packages/nodejs/src/transmitter.ts | 74 ++++++++- 3 files changed, 210 insertions(+), 22 deletions(-) create mode 100644 packages/nodejs/.changesets/follow-redirects-when-downloading-the-appsignal-agent.md diff --git a/packages/nodejs/.changesets/follow-redirects-when-downloading-the-appsignal-agent.md b/packages/nodejs/.changesets/follow-redirects-when-downloading-the-appsignal-agent.md new file mode 100644 index 00000000..f100480f --- /dev/null +++ b/packages/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 index 78fd1400..db2cf3ef 100644 --- 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 index 6ef9e750..e3665ebd 100644 --- 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