NONE: small fixes of connect screen (#1600)

* NONE: fixing timeout and status code messages

* NONE: improve URL validator

* NONE: add more debug info for GHE connect page backend

* NONE: refactor GHE connect page backend validation

* NONE: frontend fixes

* NONE: add error handling to jira-server-url; use hostname for checks

* NONE: PR comments

src/routes/jira/connect/enterprise/jira-connect-enterprise-post.test.ts

@@ -43,12 +43,43 @@ describe("POST /jira/connect/enterprise", () => {
 		});
 	});
 
+	afterEach(() => {
+		delete process.env.JIRA_CONNECT_ENTERPRISE_POST_TIMEOUT_MSEC;
+	});
+
 	it("POST Jira Connect Enterprise - invalid URL", async () => {
 		const response = mockResponse();
 		await JiraConnectEnterprisePost(mockRequest("Random string!!"), response);
 
 		expect(response.status).toHaveBeenCalledWith(200);
-		expect(response.send).toHaveBeenCalledWith({ success: false, errors: [{ code: "GHE_ERROR_INVALID_URL", message: "Invalid URL" }] });
+		expect(response.send).toHaveBeenCalledWith({ success: false, errors: [{ code: "GHE_ERROR_INVALID_URL", reason: undefined }] });
+	});
+
+	it("POST Jira Connect Enterprise - invalid URL (port)", async () => {
+		const response = mockResponse();
+		await JiraConnectEnterprisePost(mockRequest("http://foobar.com:12345"), response);
+
+		expect(response.status).toHaveBeenCalledWith(200);
+		expect(response.send).toHaveBeenCalledWith({
+			success: false,
+			errors: [{
+				code: "GHE_ERROR_INVALID_URL",
+				reason: "only the following ports are allowed: 80, 8080, 443, 6017, 8443, 8444, 7990, 8090, 8085, 8060, 8900, 9900"
+			}]
+		});
+	});
+
+	it("POST Jira Connect Enterprise - GitHub cloud", async () => {
+		const response = mockResponse();
+		await JiraConnectEnterprisePost(mockRequest("https://github.com:8090"), response);
+
+		expect(response.status).toHaveBeenCalledWith(200);
+		expect(response.send).toHaveBeenCalledWith({
+			success: false,
+			errors: [{
+				code: "GHE_ERROR_GITHUB_CLOUD_HOST"
+			}]
+		});
 	});
 
 	it("POST Jira Connect Enterprise - valid existing URL", async () => {
@@ -78,4 +109,31 @@ describe("POST /jira/connect/enterprise", () => {
 		expect(response.status).toHaveBeenCalledWith(200);
 		expect(response.send).toHaveBeenCalledWith({ success: true, appExists: false });
 	});
-});
+
+	it("POST Jira Connect Enterprise - URL timed out", async () => {
+		process.env.JIRA_CONNECT_ENTERPRISE_POST_TIMEOUT_MSEC = "100";
+		const response = mockResponse();
+		gheNock.get("/").delayConnection(2000).reply(200);
+		await JiraConnectEnterprisePost(mockRequest(gheUrl), response);
+		expect(response.status).toHaveBeenCalledWith(200);
+		expect(response.send).toHaveBeenCalledWith({
+			success: false, errors: [{
+				code: "GHE_ERROR_CANNOT_CONNECT",
+				reason: "ETIMEDOUT"
+			}]
+		});
+	});
+
+	it("POST Jira Connect Enterprise - invalid status code", async () => {
+		const response = mockResponse();
+		gheNock.get("/").reply(500);
+		await JiraConnectEnterprisePost(mockRequest(gheUrl), response);
+		expect(response.status).toHaveBeenCalledWith(200);
+		expect(response.send).toHaveBeenCalledWith({
+			success: false, errors: [{
+				code: "GHE_ERROR_CANNOT_CONNECT",
+				reason: "received 500 response"
+			}]
+		});
+	});
+});

src/routes/jira/connect/enterprise/jira-connect-enterprise-post.ts

@@ -1,64 +1,71 @@
 import { Request, Response } from "express";
 import { GitHubServerApp } from "models/github-server-app";
-import { isValidUrl } from "utils/is-valid-url";
+import { validateUrl } from "utils/validate-url";
 import { statsd } from "config/statsd";
 import { metricError } from "config/metric-names";
 import { sendAnalytics } from "utils/analytics-client";
 import { AnalyticsEventTypes, AnalyticsTrackEventsEnum } from "interfaces/common";
 import { createAnonymousClient } from "utils/get-github-client-config";
 
-const TIMEOUT_PERIOD_MS = 30 * 1000;
+const GITHUB_CLOUD_HOSTS = ["github.com", "www.github.com"];
 
-interface MessageAndCode {
-	errorCode: string;
-	message: string;
+enum ErrorResponseCode {
+	INVALID_URL = "GHE_ERROR_INVALID_URL",
+	CLOUD_HOST = "GHE_ERROR_GITHUB_CLOUD_HOST",
+	CANNOT_CONNECT = "GHE_ERROR_CANNOT_CONNECT"
 }
 
-interface GheServerUrlErrors {
-	[key: string | number]: MessageAndCode;
+function isInteger(n: string) {
+	return !isNaN(Number(n));
 }
 
-const GHE_ERROR_UNREACHABLE = {
-	errorCode: "GHE_ERROR_ENOTFOUND",
-	message: "Request to URL failed"
-};
-
-export const gheServerUrlErrors: GheServerUrlErrors = {
-	invalidUrl: {
-		errorCode: "GHE_ERROR_INVALID_URL",
-		message: "Invalid URL"
-	},
-	ENOTFOUND: GHE_ERROR_UNREACHABLE,
-	DEPTH_ZERO_SELF_SIGNED_CERT: GHE_ERROR_UNREACHABLE,
-	403: GHE_ERROR_UNREACHABLE,
-	502: {
-		errorCode: "GHE_SERVER_BAD_GATEWAY",
-		message: "Bad gateway"
-	},
-	ECONNABORTED: {
-		errorCode: "GHE_ERROR_CONNECTION_TIMED_OUT",
-		message: "Connection timed out"
+function sendErrorMetricAndAnalytics(jiraHost: string, errorCode: ErrorResponseCode, maybeStatus: string | undefined = undefined) {
+	const errorCodeAndStatusObj: { errorCode: string, status?: string } = { errorCode };
+	if (maybeStatus) {
+		errorCodeAndStatusObj.status = maybeStatus;
 	}
-};
+	statsd.increment(metricError.gheServerUrlError, errorCodeAndStatusObj);
+
+	sendAnalytics(AnalyticsEventTypes.TrackEvent, {
+		name: AnalyticsTrackEventsEnum.GitHubServerUrlErrorTrackEventName,
+		jiraHost,
+		...errorCodeAndStatusObj
+	});
+}
 
 export const JiraConnectEnterprisePost = async (
 	req: Request,
 	res: Response
 ): Promise<void> => {
+
+	// Must be configurable and re-evaluated on each execution for testing, therefore
+	// inside the handler
+	const TIMEOUT_PERIOD_MS = parseInt(process.env.JIRA_CONNECT_ENTERPRISE_POST_TIMEOUT_MSEC || "30000");
+
 	const { gheServerURL } = req.body;
 	const { id: installationId } = res.locals.installation;
 
+	const jiraHost = res.locals.jiraHost;
+
 	req.log.debug(`Verifying provided GHE server url ${gheServerURL} is a valid URL`);
-	const isGheUrlValid = isValidUrl(gheServerURL);
+	const urlValidationResult = validateUrl(gheServerURL);
 
-	if (!isGheUrlValid) {
-		const { errorCode, message } = gheServerUrlErrors.invalidUrl;
-		res.status(200).send({ success: false, errors: [{ code: errorCode, message }] });
-		req.log.error(`The entered URL is not valid. ${gheServerURL} is not a valid url`);
+	if (!urlValidationResult.isValidUrl) {
+		res.status(200).send({
+			success: false,
+			errors: [{ code: ErrorResponseCode.INVALID_URL, reason: urlValidationResult.reason }]
+		});
+		req.log.info(`The entered URL is not valid. ${gheServerURL} is not a valid url`);
+		sendErrorMetricAndAnalytics(jiraHost, ErrorResponseCode.INVALID_URL);
 		return;
 	}
 
-	const jiraHost = res.locals.jiraHost;
+	if (GITHUB_CLOUD_HOSTS.includes(new URL(gheServerURL).hostname)) {
+		res.status(200).send({ success: false, errors: [ { code: ErrorResponseCode.CLOUD_HOST } ] });
+		req.log.info("The entered URL is GitHub cloud site, return error");
+		sendErrorMetricAndAnalytics(jiraHost, ErrorResponseCode.CLOUD_HOST);
+		return;
+	}
 
 	try {
 		const gitHubServerApps = await GitHubServerApp.getAllForGitHubBaseUrlAndInstallationId(gheServerURL, installationId);
@@ -80,20 +87,18 @@ export const JiraConnectEnterprisePost = async (
 			jiraHost: jiraHost
 		});
 	} catch (err) {
-		req.log.error({ err, gheServerURL }, `Something went wrong`);
-		const codeOrStatus = err.code || err.response.status;
-		const serverError = gheServerUrlErrors[codeOrStatus];
-		const errorCode = serverError?.errorCode || codeOrStatus;
-		const message = serverError?.message;
-
-		res.status(200).send({ success: false, errors: [{ code: errorCode, message }] });
-		statsd.increment(metricError.gheServerUrlError, { errorCode, status: err.response?.status });
-
-		sendAnalytics(AnalyticsEventTypes.TrackEvent, {
-			name: AnalyticsTrackEventsEnum.GitHubServerUrlErrorTrackEventName,
-			jiraHost,
-			errorCode,
-			errorMessage: message
+		req.log.warn({ err, gheServerURL }, `Couldn't access GHE host`);
+		const codeOrStatus = "" + (err.code || err.response.status);
+
+		res.status(200).send({
+			success: false, errors: [{
+				code: ErrorResponseCode.CANNOT_CONNECT,
+				reason:
+					isInteger(codeOrStatus)
+						? `received ${codeOrStatus} response`
+						: codeOrStatus
+			}]
 		});
+		sendErrorMetricAndAnalytics(jiraHost, ErrorResponseCode.CANNOT_CONNECT, codeOrStatus);
 	}
 };

src/util/get-github-client-config.test.ts

@@ -10,7 +10,7 @@ jest.mock("../config/feature-flags");
 describe('get-github-client-config', () => {
 	const uuid = newUUID();
 	const APP_ID = 123;
-	const GHES_HOSTNAME = "myinternalserver.com";
+	const GHES_HOSTNAME = "myinternalserver.com:8090";
 	let gitHubServerApp: GitHubServerApp;
 	beforeEach(async () => {
 		const payload = {
@@ -52,7 +52,16 @@ describe('get-github-client-config', () => {
 			expect(config.proxyBaseUrl).toBeUndefined();
 		});
 
-		it('skips proxy if GHES URL is in the skiplist', async () => {
+		it('skips proxy if GHES hostname is in the skiplist without port', async () => {
+			when(stringFlag)
+				.calledWith(StringFlags.OUTBOUND_PROXY_SKIPLIST, expect.anything(), jiraHost)
+				.mockResolvedValue(new URL("http://" + GHES_HOSTNAME).hostname);
+
+			const config = await getGitHubClientConfigFromAppId(gitHubServerApp.id, getLogger('test'), jiraHost);
+			expect(config.proxyBaseUrl).toBeUndefined();
+		});
+
+		it('skips proxy if GHES URL is in the skiplist with/without port', async () => {
 			when(stringFlag)
 				.calledWith(StringFlags.OUTBOUND_PROXY_SKIPLIST, expect.anything(), jiraHost)
 				.mockResolvedValue("http://" + GHES_HOSTNAME);

src/util/get-github-client-config.ts

@@ -54,8 +54,8 @@ async function calculateProxyBaseUrl(jiraHost: string, gitHubBaseUrl: string | u
 				.filter(hostname => !!hostname)
 				.map(hostname => hostname.trim())
 				.map(hostname => hostname.indexOf("://") >= 0 ? hostname : "http://" + hostname)
-				.map(hostname => new URL(hostname).host.toLowerCase())
-				.indexOf(new URL(gitHubBaseUrl).host.trim().toLowerCase()) >= 0;
+				.map(hostname => new URL(hostname).hostname.toLowerCase())
+				.indexOf(new URL(gitHubBaseUrl).hostname.trim().toLowerCase()) >= 0;
 		} catch (err) {
 			logger.error({ err }, "Cannot evaluate skiplist because of a error, opting for outboundproxy for good");
 			skipOutboundProxy = false;

src/util/validate-url.test.ts

@@ -0,0 +1,40 @@
+import { validateUrl } from "utils/validate-url";
+
+describe("validateUrl", () => {
+	it("should return VALID for valid URLs", async () => {
+		expect(validateUrl("http://myvalidurl.com").isValidUrl).toBeTruthy();
+		expect(validateUrl("https://myvalidurl.net").isValidUrl).toBeTruthy();
+		expect(validateUrl("https://192.213.23.12/branch/mypage").isValidUrl).toBeTruthy();
+		expect(validateUrl("https://192.213.23.12:8080/branch/mypage").isValidUrl).toBeTruthy();
+	});
+
+	it("should return INVALID for unsupported protocol", async () => {
+		expect(validateUrl("accd://thatsnotavalidprotocol")).toEqual({
+			isValidUrl: false,
+			reason: "unsupported protocol, only HTTP and HTTPS are allowed"
+		});
+	});
+
+	it("should return INVALID for corrupted URL", () => {
+		expect(validateUrl("thisaintaurlatall")).toEqual({
+			isValidUrl: false
+		});
+		expect(validateUrl("www.wheresmyprotocol.com")).toEqual({
+			isValidUrl: false
+		});
+	});
+
+	it("should return INVALID invalid port", () => {
+		expect(validateUrl("http://foo.com:12345")).toEqual({
+			isValidUrl: false,
+			reason: "only the following ports are allowed: 80, 8080, 443, 6017, 8443, 8444, 7990, 8090, 8085, 8060, 8900, 9900"
+		});
+	});
+
+	it("should return INVALID if url has query parameters", async () => {
+		expect(validateUrl("https://192.213.23.12/branch/mypage/?foo=bar")).toEqual({
+			isValidUrl: false,
+			reason: "query parameters are not allowed"
+		});
+	});
+});

src/util/validate-url.ts

@@ -0,0 +1,38 @@
+interface UrlValidationResult {
+	isValidUrl: boolean,
+	reason?: string
+}
+
+// See https://stash.atlassian.com/projects/EDGE/repos/edge-proxies/browse/sceptre/data/squid_outbound-proxy.conf.jinja2#43
+const ALLOWED_PORTS = [80, 8080, 443, 6017, 8443, 8444, 7990, 8090, 8085, 8060, 8900, 9900];
+
+export const validateUrl = (url: string): UrlValidationResult => {
+	try {
+		const { protocol, port } = new URL(url);
+		if (port && !ALLOWED_PORTS.includes(parseInt(port))) {
+			return {
+				isValidUrl: false,
+				reason: "only the following ports are allowed: " + ALLOWED_PORTS.join(", ")
+			};
+		}
+		if  (!(/^https?:$/.test(protocol))) {
+			return {
+				isValidUrl: false,
+				reason: "unsupported protocol, only HTTP and HTTPS are allowed"
+			};
+		}
+		if (url.includes("?")) {
+			return {
+				isValidUrl: false,
+				reason: "query parameters are not allowed"
+			};
+		}
+	} catch (err) {
+		return {
+			isValidUrl: false
+		};
+	}
+	return {
+		isValidUrl: true
+	};
+};