From b00919e3491f19b37962474bbe365f1a5cf84509 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Mon, 19 Feb 2024 16:20:03 -0800 Subject: [PATCH 1/4] Add tests for existing safety checks --- tests/unit/models/callStack.test.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 tests/unit/models/callStack.test.js diff --git a/tests/unit/models/callStack.test.js b/tests/unit/models/callStack.test.js new file mode 100644 index 00000000..3e5ddf90 --- /dev/null +++ b/tests/unit/models/callStack.test.js @@ -0,0 +1,27 @@ +const callStack = require("../../../src/models/callStack.js"); + +describe("Sanitizes content as expected", () => { + test("Leaves safe data unmodified", () => { + const cs = new callStack(); + + const before = { + value: "Safe data" + }; + + const after = cs.sanitize(before); + + expect(after).toEqual(before); + }); + + test("Removes value of a key starting with 'token'", () => { + const cs = new callStack(); + + const before = { + token: "super_secret" + }; + + const after = cs.sanitize(before); + + expect(after).toEqual({ token: "*****" }); + }); +}); From 19415e3cb7f2ce352a0235031c088f143a155971 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Mon, 19 Feb 2024 16:48:28 -0800 Subject: [PATCH 2/4] Add support for every type of value, and fully test --- src/models/callStack.js | 68 ++++++++++++++++++++++++----- tests/unit/models/callStack.test.js | 57 +++++++++++++++++++++++- 2 files changed, 112 insertions(+), 13 deletions(-) diff --git a/src/models/callStack.js b/src/models/callStack.js index 66545e20..6deaa180 100644 --- a/src/models/callStack.js +++ b/src/models/callStack.js @@ -20,21 +20,65 @@ module.exports = class CallStack { // Attempts to remove any sensitive data that may be found within sanitize(content) { - if (typeof content !== "object") { - return content; - } + const hideString = "*****"; let outContent = {}; + let type = typeof content; + + // Since JavaScript `typeof` will assign an array as "object" as well as null + // we will extend this typeof check to add those as different types, to ease + // the complexity of the below switch statement + if (Array.isArray(content)) { + type = "array"; + } + if (content === null) { + type = "null"; + } + + switch(type) { + case "object": + for (const key in content) { + // Match different possible keys that represent sensitive data + switch(key) { + case "token": + outContent[key] = hideString; + break; + default: + outContent[key] = this.sanitize(content[key]); + break; + } + } + break; + case "string": + // Match different strings of sensitive data - for (const key in content) { - switch (key) { - case "token": - outContent[key] = "*****"; - break; - default: - outContent[key] = content[key]; - break; - } + // https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/about-authentication-to-github#githubs-token-formats + if (content.startsWith("gho_")) { + outContent = hideString; + } else if (content.startsWith("ghp_")) { + outContent = hideString; + } else if (content.startsWith("github_pat_")) { + outContent = hideString; + } else if (content.startsWith("ghu_")) { + outContent = hideString; + } else if (content.startsWith("ghs_")) { + outContent = hideString; + } else if (content.startsWith("ghr_")) { + outContent = hideString; + } else { + // String seems safe + outContent = content; + } + break; + case "array": + let tmpArr = []; + for (let i = 0; i < content.length; i++) { + tmpArr.push(this.sanitize(content[i])); + } + outContent = tmpArr; + break; + default: + outContent = content; } return outContent; diff --git a/tests/unit/models/callStack.test.js b/tests/unit/models/callStack.test.js index 3e5ddf90..54363965 100644 --- a/tests/unit/models/callStack.test.js +++ b/tests/unit/models/callStack.test.js @@ -1,4 +1,5 @@ const callStack = require("../../../src/models/callStack.js"); +const hideValue = "*****"; describe("Sanitizes content as expected", () => { test("Leaves safe data unmodified", () => { @@ -22,6 +23,60 @@ describe("Sanitizes content as expected", () => { const after = cs.sanitize(before); - expect(after).toEqual({ token: "*****" }); + expect(after).toEqual({ token: hideValue }); + }); + + test("Removes value of an unsafe string", () => { + const cs = new callStack(); + + const before = "gho_value"; + + const after = cs.sanitize(before); + + expect(after).toEqual(hideValue); + }); + + test("Removes deeply nested unsafe string", () => { + const cs = new callStack(); + + const before = { value: { value: { value: { value: "github_pat_value" }}}}; + + const after = cs.sanitize(before); + + expect(after).toEqual({ value: { value: { value: { value: hideValue }}}}); + }); + + test("Removes unsafe value from array", () => { + const cs = new callStack(); + + const before = [{}, {}, { token: "super_secret" }]; + const after = cs.sanitize(before); + + expect(after).toEqual([{}, {}, { token: hideValue }]); + }); + + test("Doesn't break on 'null'", () => { + const cs = new callStack(); + + const before = null; + const after = cs.sanitize(before); + + expect(after).toEqual(null); + }); + + test("Removes all known github token formats", () => { + const cs = new callStack(); + + const before = [ + { v1: "ghp_personal_access_token_classic" }, + { v2: "github_pat_fine_grained_personal_access_token" }, + { v3: "gho_oauth_access_token" }, + { v4: "ghu_user_access_token_for_github_app" }, + { v5: "ghs_installation_access_token" }, + { v6: "ghr_refresh_token_for_github_app" } + ]; + const after = cs.sanitize(before); + + expect(after).toEqual([{v1: hideValue}, {v2: hideValue}, {v3: hideValue}, {v4: hideValue}, {v5: hideValue}, {v6: hideValue}]); }); }); From e3c7933c3bc28b8f92651dc40f49816a092d3836 Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Mon, 19 Feb 2024 16:59:37 -0800 Subject: [PATCH 3/4] Remove temporary variable within `switch` as it's scope is unclear --- src/models/callStack.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/models/callStack.js b/src/models/callStack.js index 6deaa180..4cbc2abd 100644 --- a/src/models/callStack.js +++ b/src/models/callStack.js @@ -71,11 +71,10 @@ module.exports = class CallStack { } break; case "array": - let tmpArr = []; + outContent = []; for (let i = 0; i < content.length; i++) { - tmpArr.push(this.sanitize(content[i])); + outContent.push(this.sanitize(content[i])); } - outContent = tmpArr; break; default: outContent = content; From 8186d21d36cc665a5de8bce3706041331b036cff Mon Sep 17 00:00:00 2001 From: confused-Techie Date: Mon, 19 Feb 2024 21:14:41 -0800 Subject: [PATCH 4/4] Optimize code, additional keys --- src/models/callStack.js | 46 ++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/src/models/callStack.js b/src/models/callStack.js index 4cbc2abd..3aec037d 100644 --- a/src/models/callStack.js +++ b/src/models/callStack.js @@ -21,6 +21,16 @@ module.exports = class CallStack { // Attempts to remove any sensitive data that may be found within sanitize(content) { + const badKeys = [ + "token", + "password", + "pass", + "auth", + "secret", + "passphrase", + "card" + ]; + const githubTokenReg = /(?:gho_|ghp_|github_pat_|ghu_|ghs_|ghr_)/; const hideString = "*****"; let outContent = {}; let type = typeof content; @@ -28,24 +38,22 @@ module.exports = class CallStack { // Since JavaScript `typeof` will assign an array as "object" as well as null // we will extend this typeof check to add those as different types, to ease // the complexity of the below switch statement - if (Array.isArray(content)) { - type = "array"; - } - if (content === null) { - type = "null"; + if (type === "object") { + if (Array.isArray(content)) { + type = "array"; + } else if (content === null) { + type = "null"; + } } switch(type) { case "object": for (const key in content) { // Match different possible keys that represent sensitive data - switch(key) { - case "token": - outContent[key] = hideString; - break; - default: - outContent[key] = this.sanitize(content[key]); - break; + if (badKeys.includes(key)) { + outContent[key] = hideString; + } else { + outContent[key] = this.sanitize(content[key]); } } break; @@ -53,19 +61,9 @@ module.exports = class CallStack { // Match different strings of sensitive data // https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/about-authentication-to-github#githubs-token-formats - if (content.startsWith("gho_")) { - outContent = hideString; - } else if (content.startsWith("ghp_")) { - outContent = hideString; - } else if (content.startsWith("github_pat_")) { - outContent = hideString; - } else if (content.startsWith("ghu_")) { - outContent = hideString; - } else if (content.startsWith("ghs_")) { - outContent = hideString; - } else if (content.startsWith("ghr_")) { + if (githubTokenReg.test(content)) { outContent = hideString; - } else { + } else { // More strings to test can be added here // String seems safe outContent = content; }