From 2876c6e6a0c9eb5004a6518b246ca0c16d8cd8a6 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 7 Feb 2022 15:13:35 -0700 Subject: [PATCH 01/10] feat: add isAddressInfo helper function --- src/node/util.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/node/util.ts b/src/node/util.ts index b53a4ecf6182..6432bb1ac84c 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -485,6 +485,15 @@ export function isNodeJSErrnoException(error: unknown): error is NodeJS.ErrnoExc return error !== undefined && (error as NodeJS.ErrnoException).code !== undefined } +/** + * A helper function which returns a boolean indicating whether + * the given address is AddressInfo and has .address + * and a .port property. + */ +export function isAddressInfo(address: unknown): address is net.AddressInfo { + return address !== null && typeof address !== "string" && (address as net.AddressInfo).port !== undefined +} + // TODO: Replace with proper templating system. export const escapeJSON = (value: cp.Serializable) => JSON.stringify(value).replace(/"/g, """) From 4c42a315737d5f79a8704681d923ec30cfabf714 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 7 Feb 2022 15:14:09 -0700 Subject: [PATCH 02/10] feat(update): add test for rejection UpdateProvider --- test/unit/node/update.test.ts | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index 49c938b125a0..0c2f5efe0935 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -1,7 +1,10 @@ import * as http from "http" +import { logger } from "@coder/logger" +import { AddressInfo } from "net" import * as path from "path" import { SettingsProvider, UpdateSettings } from "../../../src/node/settings" import { LatestResponse, UpdateProvider } from "../../../src/node/update" +import { isAddressInfo } from "../../../src/node/util" import { clean, mockLogger, tmpdir } from "../../utils/helpers" describe("update", () => { @@ -23,6 +26,11 @@ describe("update", () => { return response.end(JSON.stringify(latest)) } + if (request.url === "/reject-status-code") { + response.writeHead(500) + return response.end("rejected status code test") + } + // Anything else is a 404. response.writeHead(404) response.end("not found") @@ -37,6 +45,7 @@ describe("update", () => { } let _provider: UpdateProvider | undefined + let _address: string | AddressInfo | null const provider = (): UpdateProvider => { if (!_provider) { throw new Error("Update provider has not been created") @@ -62,12 +71,12 @@ describe("update", () => { }) }) - const address = server.address() - if (!address || typeof address === "string" || !address.port) { + _address = server.address() + if (!isAddressInfo(_address)) { throw new Error("unexpected address") } - _provider = new UpdateProvider(`http://${address.address}:${address.port}/latest`, _settings) + _provider = new UpdateProvider(`http://${_address?.address}:${_address?.port}/latest`, _settings) }) afterAll(() => { @@ -170,4 +179,19 @@ describe("update", () => { expect(update.checked < Date.now() && update.checked >= now).toEqual(true) expect(update.version).toStrictEqual("unknown") }) + + it("should reject if response has status code 500", async () => { + if (isAddressInfo(_address)) { + const mockURL = `http://${_address.address}:${_address.port}/reject-status-code` + let provider = new UpdateProvider(mockURL, settings()) + let update = await provider.getUpdate(true) + + expect(update.version).toBe("unknown") + expect(logger.error).toHaveBeenCalled() + expect(logger.error).toHaveBeenCalledWith("Failed to get latest version", { + identifier: "error", + value: `${mockURL}: 500`, + }) + } + }) }) From c04ddcdeaf3a827c24d7f6ff1a9e311880ad64a5 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 7 Feb 2022 16:18:25 -0700 Subject: [PATCH 03/10] feat: add more tests for UpdateProvider --- test/unit/node/update.test.ts | 87 +++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index 0c2f5efe0935..cd0012dd3849 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -31,6 +31,51 @@ describe("update", () => { return response.end("rejected status code test") } + if (request.url === "/no-location-header") { + response.writeHead(301) + response.writeHead(301, "testing", { + location: "", + }) + return response.end("rejected status code test") + } + + if (request.url === "/location-redirect") { + const latest: LatestResponse = { + name: "4.0.2", + } + response.writeHead(200) + return response.end(JSON.stringify(latest)) + } + + if (request.url === "/with-location-header") { + response.writeHead(301) + response.writeHead(301, "testing", { + location: "/location-redirect", + }) + + return response.end() + } + + // NOTES subpath match /redirect/${number} + // if number === 0, resolve it + if (request.url.match(/\/redirect\/[0-9]/)) { + if (request.url === "/redirect/0") { + response.writeHead(200) + return response.end("done") + } + + // Subtract 1 from the current redirect number + // i.e. /redirect/10 -> /redirect/9 -> /redirect/8 + const urlSplit = request.url.split("/") + const currentRedirectNumber = urlSplit[urlSplit.length - 1] + const newRedirectNumber = parseInt(currentRedirectNumber) + 1 + + response.writeHead(302, "testing", { + location: `/redirect/${String(newRedirectNumber)}`, + }) + return response.end("") + } + // Anything else is a 404. response.writeHead(404) response.end("not found") @@ -84,6 +129,7 @@ describe("update", () => { }) beforeEach(() => { + jest.clearAllMocks() spy = [] }) @@ -194,4 +240,45 @@ describe("update", () => { }) } }) + + it("should reject if no location header provided", async () => { + if (isAddressInfo(_address)) { + const mockURL = `http://${_address.address}:${_address.port}/no-location-header` + let provider = new UpdateProvider(mockURL, settings()) + let update = await provider.getUpdate(true) + + expect(update.version).toBe("unknown") + expect(logger.error).toHaveBeenCalled() + expect(logger.error).toHaveBeenCalledWith("Failed to get latest version", { + identifier: "error", + value: `received redirect with no location header`, + }) + } + }) + + it("should resolve the request with response.headers.location", async () => { + if (isAddressInfo(_address)) { + const mockURL = `http://${_address.address}:${_address.port}/with-location-header` + let provider = new UpdateProvider(mockURL, settings()) + let update = await provider.getUpdate(true) + + expect(logger.error).not.toHaveBeenCalled() + expect(update.version).toBe("4.0.2") + } + }) + + it("should reject if more than 10 redirects", async () => { + if (isAddressInfo(_address)) { + const mockURL = `http://${_address.address}:${_address.port}/redirect/10` + let provider = new UpdateProvider(mockURL, settings()) + let update = await provider.getUpdate(true) + + expect(update.version).toBe("unknown") + expect(logger.error).toHaveBeenCalled() + expect(logger.error).toHaveBeenCalledWith("Failed to get latest version", { + identifier: "error", + value: `reached max redirects`, + }) + } + }) }) From 50dbf8b0bd77484b005a8005c73feabdd8ff4f91 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 8 Feb 2022 10:48:57 -0700 Subject: [PATCH 04/10] fixup! move isAddressInfo, add .address check --- src/node/util.ts | 9 --------- test/unit/node/update.test.ts | 3 +-- test/utils/helpers.ts | 14 ++++++++++++++ 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/node/util.ts b/src/node/util.ts index 6432bb1ac84c..b53a4ecf6182 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -485,15 +485,6 @@ export function isNodeJSErrnoException(error: unknown): error is NodeJS.ErrnoExc return error !== undefined && (error as NodeJS.ErrnoException).code !== undefined } -/** - * A helper function which returns a boolean indicating whether - * the given address is AddressInfo and has .address - * and a .port property. - */ -export function isAddressInfo(address: unknown): address is net.AddressInfo { - return address !== null && typeof address !== "string" && (address as net.AddressInfo).port !== undefined -} - // TODO: Replace with proper templating system. export const escapeJSON = (value: cp.Serializable) => JSON.stringify(value).replace(/"/g, """) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index cd0012dd3849..d59ab1565c7b 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -4,8 +4,7 @@ import { AddressInfo } from "net" import * as path from "path" import { SettingsProvider, UpdateSettings } from "../../../src/node/settings" import { LatestResponse, UpdateProvider } from "../../../src/node/update" -import { isAddressInfo } from "../../../src/node/util" -import { clean, mockLogger, tmpdir } from "../../utils/helpers" +import { clean, isAddressInfo, mockLogger, tmpdir } from "../../utils/helpers" describe("update", () => { let version = "1.0.0" diff --git a/test/utils/helpers.ts b/test/utils/helpers.ts index 92e06fbcf204..be8f3a90da4d 100644 --- a/test/utils/helpers.ts +++ b/test/utils/helpers.ts @@ -105,3 +105,17 @@ export function idleTimer(message: string, reject: (error: Error) => void, delay }, } } + +/** + * A helper function which returns a boolean indicating whether + * the given address is AddressInfo and has .address + * and a .port property. + */ +export function isAddressInfo(address: unknown): address is net.AddressInfo { + return ( + address !== null && + typeof address !== "string" && + (address as net.AddressInfo).port !== undefined && + (address as net.AddressInfo).address !== undefined + ) +} From fa822be2a490e624a3cfb1a06212c98a5f1ec408 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 8 Feb 2022 10:49:58 -0700 Subject: [PATCH 05/10] fixup! remove extra writeHead --- test/unit/node/update.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index d59ab1565c7b..b8910f0f3716 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -31,7 +31,6 @@ describe("update", () => { } if (request.url === "/no-location-header") { - response.writeHead(301) response.writeHead(301, "testing", { location: "", }) @@ -47,7 +46,6 @@ describe("update", () => { } if (request.url === "/with-location-header") { - response.writeHead(301) response.writeHead(301, "testing", { location: "/location-redirect", }) From 8b23fb896fadb403f409ab743077f8d964c01cc5 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 8 Feb 2022 10:50:39 -0700 Subject: [PATCH 06/10] fixup! use -1 in redirect logic --- test/unit/node/update.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index b8910f0f3716..d4ecb9e05ff3 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -65,7 +65,7 @@ describe("update", () => { // i.e. /redirect/10 -> /redirect/9 -> /redirect/8 const urlSplit = request.url.split("/") const currentRedirectNumber = urlSplit[urlSplit.length - 1] - const newRedirectNumber = parseInt(currentRedirectNumber) + 1 + const newRedirectNumber = parseInt(currentRedirectNumber) - 1 response.writeHead(302, "testing", { location: `/redirect/${String(newRedirectNumber)}`, From 9302a24997d65694011a99bcefaa565fb8cbcdc2 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 8 Feb 2022 10:51:07 -0700 Subject: [PATCH 07/10] fixup! remove unnecessary String call --- test/unit/node/update.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index d4ecb9e05ff3..eb7139faa8a6 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -68,7 +68,7 @@ describe("update", () => { const newRedirectNumber = parseInt(currentRedirectNumber) - 1 response.writeHead(302, "testing", { - location: `/redirect/${String(newRedirectNumber)}`, + location: `/redirect/${newRedirectNumber}`, }) return response.end("") } From 0db85dce775885f665dae98c696342066eeee966 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 8 Feb 2022 11:16:46 -0700 Subject: [PATCH 08/10] fixup! use /latest for redirect --- test/unit/node/update.test.ts | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index eb7139faa8a6..0ef6b4300769 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -37,17 +37,9 @@ describe("update", () => { return response.end("rejected status code test") } - if (request.url === "/location-redirect") { - const latest: LatestResponse = { - name: "4.0.2", - } - response.writeHead(200) - return response.end(JSON.stringify(latest)) - } - if (request.url === "/with-location-header") { response.writeHead(301, "testing", { - location: "/location-redirect", + location: "/latest", }) return response.end() @@ -68,7 +60,7 @@ describe("update", () => { const newRedirectNumber = parseInt(currentRedirectNumber) - 1 response.writeHead(302, "testing", { - location: `/redirect/${newRedirectNumber}`, + location: `/redirect/${String(newRedirectNumber)}`, }) return response.end("") } @@ -254,13 +246,14 @@ describe("update", () => { }) it("should resolve the request with response.headers.location", async () => { + version = "4.1.1" if (isAddressInfo(_address)) { const mockURL = `http://${_address.address}:${_address.port}/with-location-header` let provider = new UpdateProvider(mockURL, settings()) let update = await provider.getUpdate(true) expect(logger.error).not.toHaveBeenCalled() - expect(update.version).toBe("4.0.2") + expect(update.version).toBe("4.1.1") } }) From 2443ed7a1521539735f228c6528e5438259d8771 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Tue, 8 Feb 2022 11:18:10 -0700 Subject: [PATCH 09/10] fixup! use match group for regex --- test/unit/node/update.test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index 0ef6b4300769..b8a17e8abb9d 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -45,9 +45,11 @@ describe("update", () => { return response.end() } - // NOTES subpath match /redirect/${number} - // if number === 0, resolve it - if (request.url.match(/\/redirect\/[0-9]/)) { + // Checks if url matches /redirect/${number} + // with optional trailing slash + let redirectSlashNumberRegExp = new RegExp(/\/redirect\/([0-9]+)(\/)?$/) + + if (request.url.match(redirectSlashNumberRegExp)) { if (request.url === "/redirect/0") { response.writeHead(200) return response.end("done") @@ -259,7 +261,7 @@ describe("update", () => { it("should reject if more than 10 redirects", async () => { if (isAddressInfo(_address)) { - const mockURL = `http://${_address.address}:${_address.port}/redirect/10` + const mockURL = `http://${_address.address}:${_address.port}/redirect/11` let provider = new UpdateProvider(mockURL, settings()) let update = await provider.getUpdate(true) From 99db1b7867804262b07bcc980df97254122cb770 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Fri, 11 Feb 2022 13:45:10 -0700 Subject: [PATCH 10/10] fixup!: replace match/split logic --- test/unit/node/update.test.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/test/unit/node/update.test.ts b/test/unit/node/update.test.ts index b8a17e8abb9d..b900d8ae7c0d 100644 --- a/test/unit/node/update.test.ts +++ b/test/unit/node/update.test.ts @@ -47,9 +47,8 @@ describe("update", () => { // Checks if url matches /redirect/${number} // with optional trailing slash - let redirectSlashNumberRegExp = new RegExp(/\/redirect\/([0-9]+)(\/)?$/) - - if (request.url.match(redirectSlashNumberRegExp)) { + const match = request.url.match(/\/redirect\/([0-9]+)\/?$/) + if (match) { if (request.url === "/redirect/0") { response.writeHead(200) return response.end("done") @@ -57,9 +56,8 @@ describe("update", () => { // Subtract 1 from the current redirect number // i.e. /redirect/10 -> /redirect/9 -> /redirect/8 - const urlSplit = request.url.split("/") - const currentRedirectNumber = urlSplit[urlSplit.length - 1] - const newRedirectNumber = parseInt(currentRedirectNumber) - 1 + const currentRedirectNumber = parseInt(match[1]) + const newRedirectNumber = currentRedirectNumber - 1 response.writeHead(302, "testing", { location: `/redirect/${String(newRedirectNumber)}`,