From 3dbef4f33765623d3770c9caa6e2ee80e29ddc12 Mon Sep 17 00:00:00 2001 From: Reza Ebrahimi Date: Sat, 27 May 2023 12:51:57 +0200 Subject: [PATCH 1/8] Add private key validation check for multiline string content --- src/index.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/index.ts b/src/index.ts index 9d9c72a9a..767f184a6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -45,6 +45,14 @@ export function createAppAuth(options: StrategyOptions): AuthInterface { ); } + // This check ensures that private key contains the actual content + // specifically when set using envirnment variables as multiline string. + if (/^-----BEGIN .* PRIVATE KEY-----$/.test(options.privateKey.trim())) { + throw new Error( + "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n" + ); + } + const log = Object.assign( { warn: console.warn.bind(console), From c0537d4fe00ad584d22f8703ff8f8203db736426 Mon Sep 17 00:00:00 2001 From: Reza Ebrahimi Date: Sat, 27 May 2023 13:13:51 +0200 Subject: [PATCH 2/8] fix a typo --- src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.ts b/src/index.ts index 767f184a6..4ce693ed7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -46,7 +46,7 @@ export function createAppAuth(options: StrategyOptions): AuthInterface { } // This check ensures that private key contains the actual content - // specifically when set using envirnment variables as multiline string. + // specifically when set using environment variables as multiline string. if (/^-----BEGIN .* PRIVATE KEY-----$/.test(options.privateKey.trim())) { throw new Error( "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n" From fada9bdd12b25f5ae0f345f554ce9b83638f2747 Mon Sep 17 00:00:00 2001 From: Reza Ebrahimi Date: Sat, 27 May 2023 14:58:02 +0200 Subject: [PATCH 3/8] Fix the regex test expression and update the error message --- src/get-app-authentication.ts | 4 ++-- src/index.ts | 6 ++++-- test/index.test.ts | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/get-app-authentication.ts b/src/get-app-authentication.ts index bb585ce18..74e3f361a 100644 --- a/src/get-app-authentication.ts +++ b/src/get-app-authentication.ts @@ -21,9 +21,9 @@ export async function getAppAuthentication({ expiresAt: new Date(appAuthentication.expiration * 1000).toISOString(), }; } catch (error) { - if (privateKey === "-----BEGIN RSA PRIVATE KEY-----") { + if (/^-----BEGIN .* PRIVATE KEY-----$/.test(privateKey.trim()) === false) { throw new Error( - "The 'privateKey` option contains only the first line '-----BEGIN RSA PRIVATE KEY-----'. If you are setting it using a `.env` file, make sure it is set on a single line with newlines replaced by '\n'" + "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" ); } else { throw error; diff --git a/src/index.ts b/src/index.ts index 4ce693ed7..f86d7e216 100644 --- a/src/index.ts +++ b/src/index.ts @@ -47,9 +47,11 @@ export function createAppAuth(options: StrategyOptions): AuthInterface { // This check ensures that private key contains the actual content // specifically when set using environment variables as multiline string. - if (/^-----BEGIN .* PRIVATE KEY-----$/.test(options.privateKey.trim())) { + if ( + /^-----BEGIN .* PRIVATE KEY-----$/.test(options.privateKey.trim()) === false + ) { throw new Error( - "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n" + "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" ); } diff --git a/test/index.test.ts b/test/index.test.ts index b31a3bb29..b37646822 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -110,7 +110,7 @@ test("throws if incomplete Private Key is provided", async () => { await expect(auth({ type: "app" })).rejects.toEqual( new Error( - "The 'privateKey` option contains only the first line '-----BEGIN RSA PRIVATE KEY-----'. If you are setting it using a `.env` file, make sure it is set on a single line with newlines replaced by '\n'" + "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" ) ); }); From 921981271488419cc8d784f39690ef6341802335 Mon Sep 17 00:00:00 2001 From: Reza Ebrahimi Date: Sat, 27 May 2023 15:45:44 +0200 Subject: [PATCH 4/8] change check from regex to string.includes due to failing an edge case --- src/index.ts | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/index.ts b/src/index.ts index f86d7e216..76da6ef5e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -47,13 +47,22 @@ export function createAppAuth(options: StrategyOptions): AuthInterface { // This check ensures that private key contains the actual content // specifically when set using environment variables as multiline string. - if ( - /^-----BEGIN .* PRIVATE KEY-----$/.test(options.privateKey.trim()) === false - ) { - throw new Error( - "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" - ); + const privateKey: string[] = options.privateKey.trim().split(" "); + if (privateKey.length > 1) { + const protocol = privateKey[1]; + const begin = `-----BEGIN ${protocol} PRIVATE KEY-----`; + const end = `-----END ${protocol} PRIVATE KEY----`; + + if (!(privateKey.includes(begin) && privateKey.includes(end))) { + throw new Error( + "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" + ); + } } + // TODO: tests are failing + // else { + // throw new Error("[@octokit/auth-app] privateKey content is invalid"); + // } const log = Object.assign( { From aab46d2d4ee468d2499a663a7d21fb81ef727cd8 Mon Sep 17 00:00:00 2001 From: Reza Ebrahimi Date: Sat, 27 May 2023 16:31:36 +0200 Subject: [PATCH 5/8] move the logic to new file and write tests for it --- src/get-app-authentication.ts | 3 ++- src/index.ts | 18 +++++-------- src/validate-pk-content.ts | 22 ++++++++++++++++ test/validate-pk-content.test.ts | 44 ++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 13 deletions(-) create mode 100644 src/validate-pk-content.ts create mode 100644 test/validate-pk-content.test.ts diff --git a/src/get-app-authentication.ts b/src/get-app-authentication.ts index 74e3f361a..460fc8060 100644 --- a/src/get-app-authentication.ts +++ b/src/get-app-authentication.ts @@ -1,6 +1,7 @@ import { githubAppJwt } from "universal-github-app-jwt"; import type { AppAuthentication, State } from "./types"; +import { validatePrivatekeyContent } from "./validate-pk-content"; export async function getAppAuthentication({ appId, @@ -21,7 +22,7 @@ export async function getAppAuthentication({ expiresAt: new Date(appAuthentication.expiration * 1000).toISOString(), }; } catch (error) { - if (/^-----BEGIN .* PRIVATE KEY-----$/.test(privateKey.trim()) === false) { + if (!validatePrivatekeyContent(privateKey)) { throw new Error( "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" ); diff --git a/src/index.ts b/src/index.ts index 76da6ef5e..c4fd9ff08 100644 --- a/src/index.ts +++ b/src/index.ts @@ -7,6 +7,7 @@ import { hook } from "./hook"; import { getCache } from "./cache"; import type { AuthInterface, State, StrategyOptions } from "./types"; import { VERSION } from "./version"; +import { validatePrivatekeyContent } from "./validate-pk-content"; export { createOAuthUserAuth } from "@octokit/auth-oauth-user"; export type { @@ -47,19 +48,12 @@ export function createAppAuth(options: StrategyOptions): AuthInterface { // This check ensures that private key contains the actual content // specifically when set using environment variables as multiline string. - const privateKey: string[] = options.privateKey.trim().split(" "); - if (privateKey.length > 1) { - const protocol = privateKey[1]; - const begin = `-----BEGIN ${protocol} PRIVATE KEY-----`; - const end = `-----END ${protocol} PRIVATE KEY----`; - - if (!(privateKey.includes(begin) && privateKey.includes(end))) { - throw new Error( - "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" - ); - } + if (!validatePrivatekeyContent(options.privateKey)) { + throw new Error( + "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" + ); } - // TODO: tests are failing + // TODO: tests are failing... so commenting for now. // else { // throw new Error("[@octokit/auth-app] privateKey content is invalid"); // } diff --git a/src/validate-pk-content.ts b/src/validate-pk-content.ts new file mode 100644 index 000000000..75d42a7f9 --- /dev/null +++ b/src/validate-pk-content.ts @@ -0,0 +1,22 @@ +export function validatePrivatekeyContent(privateKey: string) { + // first check + if (!privateKey.startsWith("----") && !privateKey.endsWith("----")) { + return false; + } + + // second check + const pk: string[] = privateKey.trim().split(" "); + if (pk.length > 1) { + const protocol = pk[1]; + const begin = `-----BEGIN ${protocol} PRIVATE KEY-----`; + const end = `-----END ${protocol} PRIVATE KEY----`; + + if (!(privateKey.includes(begin) && privateKey.includes(end))) { + return false; + } + } + + // TODO: tests are failing... so returning true for now. + // return false; + return true; +} diff --git a/test/validate-pk-content.test.ts b/test/validate-pk-content.test.ts new file mode 100644 index 000000000..51d9126d6 --- /dev/null +++ b/test/validate-pk-content.test.ts @@ -0,0 +1,44 @@ +import { validatePrivatekeyContent } from "../src/validate-pk-content"; + +const VALID_PRIVATE_KEY = `-----BEGIN RSA PRIVATE KEY----- +MIIEpAIBAAKCAQEA1c7+9z5Pad7OejecsQ0bu3aozN3tihPmljnnudb9G3HECdnH +lWu2/a1gB9JW5TBQ+AVpum9Okx7KfqkfBKL9mcHgSL0yWMdjMfNOqNtrQqKlN4kE +p6RD++7sGbzbfZ9arwrlD/HSDAWGdGGJTSOBM6pHehyLmSC3DJoR/CTu0vTGTWXQ +rO64Z8tyXQPtVPb/YXrcUhbBp8i72b9Xky0fD6PkEebOy0Ip58XVAn2UPNlNOSPS +ye+Qjtius0Md4Nie4+X8kwVI2Qjk3dSm0sw/720KJkdVDmrayeljtKBx6AtNQsSX +gzQbeMmiqFFkwrG1+zx6E7H7jqIQ9B6bvWKXGwIDAQABAoIBAD8kBBPL6PPhAqUB +K1r1/gycfDkUCQRP4DbZHt+458JlFHm8QL6VstKzkrp8mYDRhffY0WJnYJL98tr4 +4tohsDbqFGwmw2mIaHjl24LuWXyyP4xpAGDpl9IcusjXBxLQLp2m4AKXbWpzb0OL +Ulrfc1ZooPck2uz7xlMIZOtLlOPjLz2DuejVe24JcwwHzrQWKOfA11R/9e50DVse +hnSH/w46Q763y4I0E3BIoUMsolEKzh2ydAAyzkgabGQBUuamZotNfvJoDXeCi1LD +8yNCWyTlYpJZJDDXooBU5EAsCvhN1sSRoaXWrlMSDB7r/E+aQyKua4KONqvmoJuC +21vSKeECgYEA7yW6wBkVoNhgXnk8XSZv3W+Q0xtdVpidJeNGBWnczlZrummt4xw3 +xs6zV+rGUDy59yDkKwBKjMMa42Mni7T9Fx8+EKUuhVK3PVQyajoyQqFwT1GORJNz +c/eYQ6VYOCSC8OyZmsBM2p+0D4FF2/abwSPMmy0NgyFLCUFVc3OECpkCgYEA5OAm +I3wt5s+clg18qS7BKR2DuOFWrzNVcHYXhjx8vOSWV033Oy3yvdUBAhu9A1LUqpwy +Ma+unIgxmvmUMQEdyHQMcgBsVs10dR/g2xGjMLcwj6kn+xr3JVIZnbRT50YuPhf+ +ns1ScdhP6upo9I0/sRsIuN96Gb65JJx94gQ4k9MCgYBO5V6gA2aMQvZAFLUicgzT +u/vGea+oYv7tQfaW0J8E/6PYwwaX93Y7Q3QNXCoCzJX5fsNnoFf36mIThGHGiHY6 +y5bZPPWFDI3hUMa1Hu/35XS85kYOP6sGJjf4kTLyirEcNKJUWH7CXY+00cwvTkOC +S4Iz64Aas8AilIhRZ1m3eQKBgQCUW1s9azQRxgeZGFrzC3R340LL530aCeta/6FW +CQVOJ9nv84DLYohTVqvVowdNDTb+9Epw/JDxtDJ7Y0YU0cVtdxPOHcocJgdUGHrX +ZcJjRIt8w8g/s4X6MhKasBYm9s3owALzCuJjGzUKcDHiO2DKu1xXAb0SzRcTzUCn +7daCswKBgQDOYPZ2JGmhibqKjjLFm0qzpcQ6RPvPK1/7g0NInmjPMebP0K6eSPx0 +9/49J6WTD++EajN7FhktUSYxukdWaCocAQJTDNYP0K88G4rtC2IYy5JFn9SWz5oh +x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== +-----END RSA PRIVATE KEY-----`; + +test("valid private key must return true", async () => { + const isValid = validatePrivatekeyContent(VALID_PRIVATE_KEY); + await expect(isValid).toStrictEqual(true); +}); + +test("invalid private key INVALID_PRIVATE_KEY", async () => { + const isValid = validatePrivatekeyContent("INVALID_PRIVATE_KEY"); + await expect(isValid).toStrictEqual(false); +}); + +test("invalid private key -----BEGIN RSA PRIVATE KEY-----", async () => { + const isValid = validatePrivatekeyContent("-----BEGIN RSA PRIVATE KEY-----"); + await expect(isValid).toStrictEqual(false); +}); From 2e7c4dd8be859ae93e5a79a26bf50bd580c73abf Mon Sep 17 00:00:00 2001 From: Reza Ebrahimi Date: Sat, 27 May 2023 16:40:08 +0200 Subject: [PATCH 6/8] Add multi-line and single-line valid private key tests --- test/validate-pk-content.test.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/test/validate-pk-content.test.ts b/test/validate-pk-content.test.ts index 51d9126d6..cc9d1acb4 100644 --- a/test/validate-pk-content.test.ts +++ b/test/validate-pk-content.test.ts @@ -1,6 +1,6 @@ import { validatePrivatekeyContent } from "../src/validate-pk-content"; -const VALID_PRIVATE_KEY = `-----BEGIN RSA PRIVATE KEY----- +const VALID_MULTI_LINE_PRIVATE_KEY = `-----BEGIN RSA PRIVATE KEY----- MIIEpAIBAAKCAQEA1c7+9z5Pad7OejecsQ0bu3aozN3tihPmljnnudb9G3HECdnH lWu2/a1gB9JW5TBQ+AVpum9Okx7KfqkfBKL9mcHgSL0yWMdjMfNOqNtrQqKlN4kE p6RD++7sGbzbfZ9arwrlD/HSDAWGdGGJTSOBM6pHehyLmSC3DJoR/CTu0vTGTWXQ @@ -28,8 +28,16 @@ ZcJjRIt8w8g/s4X6MhKasBYm9s3owALzCuJjGzUKcDHiO2DKu1xXAb0SzRcTzUCn x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== -----END RSA PRIVATE KEY-----`; -test("valid private key must return true", async () => { - const isValid = validatePrivatekeyContent(VALID_PRIVATE_KEY); +const VALID_SINGLE_LINE_PRIVATE_KEY = + "-----BEGIN RSA PRIVATE KEY-----\nMIIEpAIBAAKCAQEA1c7+9z5Pad7OejecsQ0bu3aozN3tihPmljnnudb9G3HECdnH\nlWu2/a1gB9JW5TBQ+AVpum9Okx7KfqkfBKL9mcHgSL0yWMdjMfNOqNtrQqKlN4kE\np6RD++7sGbzbfZ9arwrlD/HSDAWGdGGJTSOBM6pHehyLmSC3DJoR/CTu0vTGTWXQ\nrO64Z8tyXQPtVPb/YXrcUhbBp8i72b9Xky0fD6PkEebOy0Ip58XVAn2UPNlNOSPS\nye+Qjtius0Md4Nie4+X8kwVI2Qjk3dSm0sw/720KJkdVDmrayeljtKBx6AtNQsSX\ngzQbeMmiqFFkwrG1+zx6E7H7jqIQ9B6bvWKXGwIDAQABAoIBAD8kBBPL6PPhAqUB\nK1r1/gycfDkUCQRP4DbZHt+458JlFHm8QL6VstKzkrp8mYDRhffY0WJnYJL98tr4\n4tohsDbqFGwmw2mIaHjl24LuWXyyP4xpAGDpl9IcusjXBxLQLp2m4AKXbWpzb0OL\nUlrfc1ZooPck2uz7xlMIZOtLlOPjLz2DuejVe24JcwwHzrQWKOfA11R/9e50DVse\nhnSH/w46Q763y4I0E3BIoUMsolEKzh2ydAAyzkgabGQBUuamZotNfvJoDXeCi1LD\n8yNCWyTlYpJZJDDXooBU5EAsCvhN1sSRoaXWrlMSDB7r/E+aQyKua4KONqvmoJuC\n21vSKeECgYEA7yW6wBkVoNhgXnk8XSZv3W+Q0xtdVpidJeNGBWnczlZrummt4xw3\nxs6zV+rGUDy59yDkKwBKjMMa42Mni7T9Fx8+EKUuhVK3PVQyajoyQqFwT1GORJNz\nc/eYQ6VYOCSC8OyZmsBM2p+0D4FF2/abwSPMmy0NgyFLCUFVc3OECpkCgYEA5OAm\nI3wt5s+clg18qS7BKR2DuOFWrzNVcHYXhjx8vOSWV033Oy3yvdUBAhu9A1LUqpwy\nMa+unIgxmvmUMQEdyHQMcgBsVs10dR/g2xGjMLcwj6kn+xr3JVIZnbRT50YuPhf+\nns1ScdhP6upo9I0/sRsIuN96Gb65JJx94gQ4k9MCgYBO5V6gA2aMQvZAFLUicgzT\nu/vGea+oYv7tQfaW0J8E/6PYwwaX93Y7Q3QNXCoCzJX5fsNnoFf36mIThGHGiHY6\ny5bZPPWFDI3hUMa1Hu/35XS85kYOP6sGJjf4kTLyirEcNKJUWH7CXY+00cwvTkOC\nS4Iz64Aas8AilIhRZ1m3eQKBgQCUW1s9azQRxgeZGFrzC3R340LL530aCeta/6FW\nCQVOJ9nv84DLYohTVqvVowdNDTb+9Epw/JDxtDJ7Y0YU0cVtdxPOHcocJgdUGHrX\nZcJjRIt8w8g/s4X6MhKasBYm9s3owALzCuJjGzUKcDHiO2DKu1xXAb0SzRcTzUCn\n7daCswKBgQDOYPZ2JGmhibqKjjLFm0qzpcQ6RPvPK1/7g0NInmjPMebP0K6eSPx0\n9/49J6WTD++EajN7FhktUSYxukdWaCocAQJTDNYP0K88G4rtC2IYy5JFn9SWz5oh\nx//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w==\n-----END RSA PRIVATE KEY----"; + +test("valid multi-line private key", async () => { + const isValid = validatePrivatekeyContent(VALID_MULTI_LINE_PRIVATE_KEY); + await expect(isValid).toStrictEqual(true); +}); + +test("valid single-line private key", async () => { + const isValid = validatePrivatekeyContent(VALID_SINGLE_LINE_PRIVATE_KEY); await expect(isValid).toStrictEqual(true); }); From 0f992d7e3e069e2f95e754be66cc2dffaf9e3893 Mon Sep 17 00:00:00 2001 From: Reza Ebrahimi Date: Sat, 27 May 2023 16:51:27 +0200 Subject: [PATCH 7/8] remove unrelated comments and update validation for more invalid cases --- src/index.ts | 4 ---- src/validate-pk-content.ts | 7 ++++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/index.ts b/src/index.ts index c4fd9ff08..6cc2bc0f3 100644 --- a/src/index.ts +++ b/src/index.ts @@ -53,10 +53,6 @@ export function createAppAuth(options: StrategyOptions): AuthInterface { "[@octokit/auth-app] privateKey only contains the first line. Try replacing line breaks with \n if you are setting it as multiline string (e.g. environment variable)" ); } - // TODO: tests are failing... so commenting for now. - // else { - // throw new Error("[@octokit/auth-app] privateKey content is invalid"); - // } const log = Object.assign( { diff --git a/src/validate-pk-content.ts b/src/validate-pk-content.ts index 75d42a7f9..a77c2c6dc 100644 --- a/src/validate-pk-content.ts +++ b/src/validate-pk-content.ts @@ -6,7 +6,7 @@ export function validatePrivatekeyContent(privateKey: string) { // second check const pk: string[] = privateKey.trim().split(" "); - if (pk.length > 1) { + if (pk.length > 2) { const protocol = pk[1]; const begin = `-----BEGIN ${protocol} PRIVATE KEY-----`; const end = `-----END ${protocol} PRIVATE KEY----`; @@ -14,9 +14,10 @@ export function validatePrivatekeyContent(privateKey: string) { if (!(privateKey.includes(begin) && privateKey.includes(end))) { return false; } + } else { + // there is no whitespace inside private key content and still invalid + return false; } - // TODO: tests are failing... so returning true for now. - // return false; return true; } From f1cf301921286a2a22eca9183423b7b9dc032c89 Mon Sep 17 00:00:00 2001 From: Reza Ebrahimi Date: Sat, 27 May 2023 17:17:18 +0200 Subject: [PATCH 8/8] fix length check for private key validation --- src/validate-pk-content.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/validate-pk-content.ts b/src/validate-pk-content.ts index a77c2c6dc..eab66b342 100644 --- a/src/validate-pk-content.ts +++ b/src/validate-pk-content.ts @@ -6,7 +6,7 @@ export function validatePrivatekeyContent(privateKey: string) { // second check const pk: string[] = privateKey.trim().split(" "); - if (pk.length > 2) { + if (pk.length > 1) { const protocol = pk[1]; const begin = `-----BEGIN ${protocol} PRIVATE KEY-----`; const end = `-----END ${protocol} PRIVATE KEY----`;