Merge pull request #14351 from Expensify/yuwen-sanitizeStringForJSONP…

…arse

.github/actions/javascript/createOrUpdateStagingDeploy/index.js

@@ -194,6 +194,7 @@ module.exports = run;
 
 const _ = __nccwpck_require__(3571);
 const {spawn} = __nccwpck_require__(3129);
+const sanitizeStringForJSONParse = __nccwpck_require__(9338);
 
 /**
  * Get merge logs between two refs (inclusive) as a JavaScript object.
@@ -229,14 +230,11 @@ function getMergeLogsAsJSON(fromRef, toRef) {
         spawnedProcess.on('error', err => reject(err));
     })
         .then((stdout) => {
-            // Remove any double-quotes from commit subjects
-            let sanitizedOutput = stdout.replace(/(?<="subject": ").*(?="})/g, subject => subject.replace(/"/g, "'"));
+            // Sanitize just the text within commit subjects as that's the only potentially un-parseable text.
+            const sanitizedOutput = stdout.replace(/(?<="subject": ").*?(?="})/g, subject => sanitizeStringForJSONParse(subject));
 
-            // Also remove any newlines and escape backslashes
-            sanitizedOutput = sanitizedOutput.replace(/(\r\n|\n|\r)/gm, '').replace(/\\/g, '\\\\');
-
-            // Then format as JSON and convert to a proper JS object
-            const json = `[${sanitizedOutput}]`.replace('},]', '}]');
+            // Then remove newlines, format as JSON and convert to a proper JS object
+            const json = `[${sanitizedOutput}]`.replace(/(\r\n|\n|\r)/gm, '').replace('},]', '}]');
 
             return JSON.parse(json);
         });
@@ -861,6 +859,39 @@ module.exports.ISSUE_OR_PULL_REQUEST_REGEX = ISSUE_OR_PULL_REQUEST_REGEX;
 module.exports.POLL_RATE = POLL_RATE;
 
 
+/***/ }),
+
+/***/ 9338:
+/***/ ((module) => {
+
+const replacer = str => ({
+    '\\': '\\\\',
+    '\t': '\\t',
+    '\n': '\\n',
+    '\r': '\\r',
+    '\f': '\\f',
+    '"': '\\"',
+}[str]);
+
+/**
+ * Replace any characters in the string that will break JSON.parse for our Git Log output
+ *
+ * Solution partly taken from SO user Gabriel Rodríguez Flores 🙇
+ * https://stackoverflow.com/questions/52789718/how-to-remove-special-characters-before-json-parse-while-file-reading
+ *
+ * @param {String} inputString
+ * @returns {String}
+ */
+module.exports = function (inputString) {
+    if (typeof inputString !== 'string') {
+        throw new TypeError('Input must me of type String');
+    }
+
+    // Replace any newlines and escape backslashes
+    return inputString.replace(/\\|\t|\n|\r|\f|"/g, replacer);
+};
+
+
 /***/ }),
 
 /***/ 7351:

.github/actions/javascript/getDeployPullRequestList/index.js

@@ -127,6 +127,7 @@ module.exports = {
 
 const _ = __nccwpck_require__(3571);
 const {spawn} = __nccwpck_require__(3129);
+const sanitizeStringForJSONParse = __nccwpck_require__(9338);
 
 /**
  * Get merge logs between two refs (inclusive) as a JavaScript object.
@@ -162,14 +163,11 @@ function getMergeLogsAsJSON(fromRef, toRef) {
         spawnedProcess.on('error', err => reject(err));
     })
         .then((stdout) => {
-            // Remove any double-quotes from commit subjects
-            let sanitizedOutput = stdout.replace(/(?<="subject": ").*(?="})/g, subject => subject.replace(/"/g, "'"));
+            // Sanitize just the text within commit subjects as that's the only potentially un-parseable text.
+            const sanitizedOutput = stdout.replace(/(?<="subject": ").*?(?="})/g, subject => sanitizeStringForJSONParse(subject));
 
-            // Also remove any newlines and escape backslashes
-            sanitizedOutput = sanitizedOutput.replace(/(\r\n|\n|\r)/gm, '').replace(/\\/g, '\\\\');
-
-            // Then format as JSON and convert to a proper JS object
-            const json = `[${sanitizedOutput}]`.replace('},]', '}]');
+            // Then remove newlines, format as JSON and convert to a proper JS object
+            const json = `[${sanitizedOutput}]`.replace(/(\r\n|\n|\r)/gm, '').replace('},]', '}]');
 
             return JSON.parse(json);
         });
@@ -794,6 +792,39 @@ module.exports.ISSUE_OR_PULL_REQUEST_REGEX = ISSUE_OR_PULL_REQUEST_REGEX;
 module.exports.POLL_RATE = POLL_RATE;
 
 
+/***/ }),
+
+/***/ 9338:
+/***/ ((module) => {
+
+const replacer = str => ({
+    '\\': '\\\\',
+    '\t': '\\t',
+    '\n': '\\n',
+    '\r': '\\r',
+    '\f': '\\f',
+    '"': '\\"',
+}[str]);
+
+/**
+ * Replace any characters in the string that will break JSON.parse for our Git Log output
+ *
+ * Solution partly taken from SO user Gabriel Rodríguez Flores 🙇
+ * https://stackoverflow.com/questions/52789718/how-to-remove-special-characters-before-json-parse-while-file-reading
+ *
+ * @param {String} inputString
+ * @returns {String}
+ */
+module.exports = function (inputString) {
+    if (typeof inputString !== 'string') {
+        throw new TypeError('Input must me of type String');
+    }
+
+    // Replace any newlines and escape backslashes
+    return inputString.replace(/\\|\t|\n|\r|\f|"/g, replacer);
+};
+
+
 /***/ }),
 
 /***/ 7351:

.github/libs/GitUtils.js

@@ -1,5 +1,6 @@
 const _ = require('underscore');
 const {spawn} = require('child_process');
+const sanitizeStringForJSONParse = require('./sanitizeStringForJSONParse');
 
 /**
  * Get merge logs between two refs (inclusive) as a JavaScript object.
@@ -35,14 +36,11 @@ function getMergeLogsAsJSON(fromRef, toRef) {
         spawnedProcess.on('error', err => reject(err));
     })
         .then((stdout) => {
-            // Remove any double-quotes from commit subjects
-            let sanitizedOutput = stdout.replace(/(?<="subject": ").*(?="})/g, subject => subject.replace(/"/g, "'"));
+            // Sanitize just the text within commit subjects as that's the only potentially un-parseable text.
+            const sanitizedOutput = stdout.replace(/(?<="subject": ").*?(?="})/g, subject => sanitizeStringForJSONParse(subject));
 
-            // Also remove any newlines and escape backslashes
-            sanitizedOutput = sanitizedOutput.replace(/(\r\n|\n|\r)/gm, '').replace(/\\/g, '\\\\');
-
-            // Then format as JSON and convert to a proper JS object
-            const json = `[${sanitizedOutput}]`.replace('},]', '}]');
+            // Then remove newlines, format as JSON and convert to a proper JS object
+            const json = `[${sanitizedOutput}]`.replace(/(\r\n|\n|\r)/gm, '').replace('},]', '}]');
 
             return JSON.parse(json);
         });

.github/libs/sanitizeStringForJSONParse.js

@@ -0,0 +1,26 @@
+const replacer = str => ({
+    '\\': '\\\\',
+    '\t': '\\t',
+    '\n': '\\n',
+    '\r': '\\r',
+    '\f': '\\f',
+    '"': '\\"',
+}[str]);
+
+/**
+ * Replace any characters in the string that will break JSON.parse for our Git Log output
+ *
+ * Solution partly taken from SO user Gabriel Rodríguez Flores 🙇
+ * https://stackoverflow.com/questions/52789718/how-to-remove-special-characters-before-json-parse-while-file-reading
+ *
+ * @param {String} inputString
+ * @returns {String}
+ */
+module.exports = function (inputString) {
+    if (typeof inputString !== 'string') {
+        throw new TypeError('Input must me of type String');
+    }
+
+    // Replace any newlines and escape backslashes
+    return inputString.replace(/\\|\t|\n|\r|\f|"/g, replacer);
+};

tests/unit/sanitizeStringForJSONParseTest.js

@@ -0,0 +1,57 @@
+import sanitizeStringForJSONParse from '../../.github/libs/sanitizeStringForJSONParse';
+
+// Bad inputs should cause an error to be thrown
+const badInputs = [
+    null,
+    undefined,
+    42,
+    true,
+];
+
+// Invalid JSON Data should be able to get parsed and the parsed result should match the input text.
+const invalidJSONData = [
+    ['Hello \t world!', 'Hello \t world!'],
+    ['Hello \n world!', 'Hello \n world!'],
+    ['Hello \n\tworld!', 'Hello \n\tworld!'],
+    ['"Hello world!"', '"Hello world!"'],
+    ['Test "', 'Test "'],
+    ['something `\\ something', 'something `\\ something'],
+
+    // Real-life examples from git commits that broke getMergeLogsAsJSON
+    // From https://github.com/Expensify/App/commit/e472470893867648cfbd85a5c2c5d24da1efece6
+    ['Add \\', 'Add \\'],
+
+    // From https://github.com/Expensify/App/pull/13500/commits/b730d5c43643f32baa3b189f0238f4de61aae0b7
+    ['Prevent commit messages that end in `\\` from breaking `getMergeLogsAsJSON()`', 'Prevent commit messages that end in `\\` from breaking `getMergeLogsAsJSON()`'],
+];
+
+// Valid JSON Data should be able to get parsed and the input text should be unmodified.
+const validJSONData = [
+    ['', ''],
+    ['Hello world!', 'Hello world!'],
+    ['Hello\\\\world!', 'Hello\\\\world!'],
+];
+
+describe('santizeStringForJSONParse', () => {
+    describe.each(badInputs)('willDetectBadInputs', (input) => {
+        test('sanitizeStringForJSONParse', () => {
+            expect(() => sanitizeStringForJSONParse(input)).toThrow();
+        });
+    });
+
+    describe.each(invalidJSONData)('canHandleInvalidJSON', (input, expectedOutput) => {
+        test('sanitizeStringForJSONParse', () => {
+            const badJSON = `{"key": "${input}"}`;
+            expect(() => JSON.parse(badJSON)).toThrow();
+            const goodJSON = JSON.parse(`{"key": "${sanitizeStringForJSONParse(input)}"}`);
+            expect(goodJSON.key).toStrictEqual(expectedOutput);
+        });
+    });
+
+    describe.each(validJSONData)('canHandleValidJSON', (input, expectedOutput) => {
+        test('sanitizeStringForJSONParse', () => {
+            const goodJSON = JSON.parse(`{"key": "${sanitizeStringForJSONParse(input)}"}`);
+            expect(goodJSON.key).toStrictEqual(expectedOutput);
+        });
+    });
+});