From 813f05a3392d589285830f9fcd2007e621465f78 Mon Sep 17 00:00:00 2001 From: Jacob Bandes-Storch Date: Thu, 25 Feb 2021 18:50:38 -0600 Subject: [PATCH 1/6] Do nothing when fix range is invalid --- lib/linter/source-code-fixer.js | 5 +++++ tests/lib/linter/source-code-fixer.js | 10 ++++++++++ 2 files changed, 15 insertions(+) diff --git a/lib/linter/source-code-fixer.js b/lib/linter/source-code-fixer.js index 53dc1dc6be7..f94fec32fb1 100644 --- a/lib/linter/source-code-fixer.js +++ b/lib/linter/source-code-fixer.js @@ -88,6 +88,11 @@ SourceCodeFixer.applyFixes = function(sourceText, messages, shouldFix) { const start = fix.range[0]; const end = fix.range[1]; + // Protect against invalid values produced by plugins + if (typeof start !== "number" || typeof end !== "number") { + return false; + } + // Remain it as a problem if it's overlapped or it's a negative range if (lastPos >= start || start > end) { remainingMessages.push(problem); diff --git a/tests/lib/linter/source-code-fixer.js b/tests/lib/linter/source-code-fixer.js index 15df35cb987..261df2dd14a 100644 --- a/tests/lib/linter/source-code-fixer.js +++ b/tests/lib/linter/source-code-fixer.js @@ -127,6 +127,10 @@ const INSERT_AT_END = { range: [3, 0], text: " " } + }, + INVALID_FIX = { + message: "bad fix", + fix: { range: [null, null], text: "bad fix" } }; //------------------------------------------------------------------------------ @@ -383,6 +387,12 @@ describe("SourceCodeFixer", () => { assert.isTrue(result.fixed); }); + it("should do nothing when given an invalid fix", () => { + const result = SourceCodeFixer.applyFixes(TEST_CODE, [INVALID_FIX]); + + assert.strictEqual(result.output, TEST_CODE); + }); + }); describe("BOM manipulations", () => { From 3b6d404c564d8e262fe18c0764873d397625b79a Mon Sep 17 00:00:00 2001 From: Jacob Bandes-Storch Date: Sun, 28 Feb 2021 18:33:29 -0600 Subject: [PATCH 2/6] Move validation to rule-fixer and add error logging --- lib/linter/rule-fixer.js | 29 ++++++++++++++++- lib/linter/source-code-fixer.js | 5 --- tests/lib/linter/rule-fixer.js | 45 ++++++++++++++++++++++++++- tests/lib/linter/source-code-fixer.js | 10 ------ 4 files changed, 72 insertions(+), 17 deletions(-) diff --git a/lib/linter/rule-fixer.js b/lib/linter/rule-fixer.js index bdd80d13b16..a30ec250771 100644 --- a/lib/linter/rule-fixer.js +++ b/lib/linter/rule-fixer.js @@ -8,12 +8,27 @@ // Requirements //------------------------------------------------------------------------------ -// none! +const log = require("../shared/logging"); //------------------------------------------------------------------------------ // Helpers //------------------------------------------------------------------------------ + +/** + * Validates that a range contains two numbers. + * @param {int[]} range The range to validate. + * @returns {bool} True if the range was valid, false otherwise. + * @private + */ +function validateRange(range) { + if (typeof range[0] !== "number" || typeof range[1] !== "number") { + log.error("Invalid text range:", range); + return false; + } + return true; +} + /** * Creates a fix command that inserts text at the specified index in the source text. * @param {int} index The 0-based index at which to insert the new text. @@ -58,6 +73,9 @@ const ruleFixer = Object.freeze({ * @returns {Object} The fix command. */ insertTextAfterRange(range, text) { + if (!validateRange(range)) { + return null; + } return insertTextAt(range[1], text); }, @@ -81,6 +99,9 @@ const ruleFixer = Object.freeze({ * @returns {Object} The fix command. */ insertTextBeforeRange(range, text) { + if (!validateRange(range)) { + return null; + } return insertTextAt(range[0], text); }, @@ -104,6 +125,9 @@ const ruleFixer = Object.freeze({ * @returns {Object} The fix command. */ replaceTextRange(range, text) { + if (!validateRange(range)) { + return null; + } return { range, text @@ -128,6 +152,9 @@ const ruleFixer = Object.freeze({ * @returns {Object} The fix command. */ removeRange(range) { + if (!validateRange(range)) { + return null; + } return { range, text: "" diff --git a/lib/linter/source-code-fixer.js b/lib/linter/source-code-fixer.js index f94fec32fb1..53dc1dc6be7 100644 --- a/lib/linter/source-code-fixer.js +++ b/lib/linter/source-code-fixer.js @@ -88,11 +88,6 @@ SourceCodeFixer.applyFixes = function(sourceText, messages, shouldFix) { const start = fix.range[0]; const end = fix.range[1]; - // Protect against invalid values produced by plugins - if (typeof start !== "number" || typeof end !== "number") { - return false; - } - // Remain it as a problem if it's overlapped or it's a negative range if (lastPos >= start || start > end) { remainingMessages.push(problem); diff --git a/tests/lib/linter/rule-fixer.js b/tests/lib/linter/rule-fixer.js index a70e735509c..846f15e6d65 100644 --- a/tests/lib/linter/rule-fixer.js +++ b/tests/lib/linter/rule-fixer.js @@ -9,13 +9,27 @@ //------------------------------------------------------------------------------ const assert = require("chai").assert, - ruleFixer = require("../../../lib/linter/rule-fixer"); + sinon = require("sinon"); + +const proxyquire = require("proxyquire").noCallThru().noPreserveCache(); //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ describe("RuleFixer", () => { + const log = { + info: sinon.spy(), + error: sinon.spy() + }; + const ruleFixer = proxyquire("../../../lib/linter/rule-fixer", { + "../shared/logging": log + }); + + afterEach(() => { + log.info.resetHistory(); + log.error.resetHistory(); + }); describe("insertTextBefore", () => { @@ -138,4 +152,33 @@ describe("RuleFixer", () => { }); + describe("validation", () => { + it("should return null given non-numeric indices", () => { + const invalidRanges = [ + [0, null], + ["1", 2], + [void 0, void 0], + [1] + ]; + const functions = [ + (range, text) => ruleFixer.insertTextBefore({ range }, text), + (range, text) => ruleFixer.insertTextBeforeRange(range, text), + (range, text) => ruleFixer.insertTextAfter({ range }, text), + (range, text) => ruleFixer.insertTextAfterRange(range, text), + range => ruleFixer.remove({ range }), + range => ruleFixer.removeRange(range), + range => ruleFixer.replaceText({ range }), + range => ruleFixer.replaceTextRange(range) + ]; + + for (const range of invalidRanges) { + for (const fn of functions) { + assert.isNull(fn(range, "x")); + assert.isTrue(log.error.called); + log.error.resetHistory(); + } + } + }); + }); + }); diff --git a/tests/lib/linter/source-code-fixer.js b/tests/lib/linter/source-code-fixer.js index 261df2dd14a..15df35cb987 100644 --- a/tests/lib/linter/source-code-fixer.js +++ b/tests/lib/linter/source-code-fixer.js @@ -127,10 +127,6 @@ const INSERT_AT_END = { range: [3, 0], text: " " } - }, - INVALID_FIX = { - message: "bad fix", - fix: { range: [null, null], text: "bad fix" } }; //------------------------------------------------------------------------------ @@ -387,12 +383,6 @@ describe("SourceCodeFixer", () => { assert.isTrue(result.fixed); }); - it("should do nothing when given an invalid fix", () => { - const result = SourceCodeFixer.applyFixes(TEST_CODE, [INVALID_FIX]); - - assert.strictEqual(result.output, TEST_CODE); - }); - }); describe("BOM manipulations", () => { From bed945c2328247a783ae090e20ed520e83f11bf7 Mon Sep 17 00:00:00 2001 From: Jacob Bandes-Storch Date: Mon, 1 Mar 2021 12:33:34 -0600 Subject: [PATCH 3/6] Move validation to report-translator --- lib/linter/report-translator.js | 17 ++++++++++ lib/linter/rule-fixer.js | 29 +---------------- tests/lib/linter/report-translator.js | 16 ++++++++++ tests/lib/linter/rule-fixer.js | 45 +-------------------------- 4 files changed, 35 insertions(+), 72 deletions(-) diff --git a/lib/linter/report-translator.js b/lib/linter/report-translator.js index bed5af81e5d..f7e7d172bbe 100644 --- a/lib/linter/report-translator.js +++ b/lib/linter/report-translator.js @@ -115,6 +115,17 @@ function normalizeReportLoc(descriptor) { return descriptor.node.loc; } +/** + * Check that a fix has a valid range. + * @param {Fix|null} fix The fix to validate. + * @returns {void} + */ +function validateFix(fix) { + if (fix) { + assert(typeof fix.range[0] === "number" && typeof fix.range[1] === "number", `Fix has invalid range: ${fix.range}`); + } +} + /** * Compares items in a fixes array by range. * @param {Fix} a The first message. @@ -133,6 +144,10 @@ function compareFixesByRange(a, b) { * @returns {{text: string, range: number[]}} The merged fixes */ function mergeFixes(fixes, sourceCode) { + for (const fix of fixes) { + validateFix(fix); + } + if (fixes.length === 0) { return null; } @@ -181,6 +196,8 @@ function normalizeFixes(descriptor, sourceCode) { if (fix && Symbol.iterator in fix) { return mergeFixes(Array.from(fix), sourceCode); } + + validateFix(fix); return fix; } diff --git a/lib/linter/rule-fixer.js b/lib/linter/rule-fixer.js index a30ec250771..bdd80d13b16 100644 --- a/lib/linter/rule-fixer.js +++ b/lib/linter/rule-fixer.js @@ -8,27 +8,12 @@ // Requirements //------------------------------------------------------------------------------ -const log = require("../shared/logging"); +// none! //------------------------------------------------------------------------------ // Helpers //------------------------------------------------------------------------------ - -/** - * Validates that a range contains two numbers. - * @param {int[]} range The range to validate. - * @returns {bool} True if the range was valid, false otherwise. - * @private - */ -function validateRange(range) { - if (typeof range[0] !== "number" || typeof range[1] !== "number") { - log.error("Invalid text range:", range); - return false; - } - return true; -} - /** * Creates a fix command that inserts text at the specified index in the source text. * @param {int} index The 0-based index at which to insert the new text. @@ -73,9 +58,6 @@ const ruleFixer = Object.freeze({ * @returns {Object} The fix command. */ insertTextAfterRange(range, text) { - if (!validateRange(range)) { - return null; - } return insertTextAt(range[1], text); }, @@ -99,9 +81,6 @@ const ruleFixer = Object.freeze({ * @returns {Object} The fix command. */ insertTextBeforeRange(range, text) { - if (!validateRange(range)) { - return null; - } return insertTextAt(range[0], text); }, @@ -125,9 +104,6 @@ const ruleFixer = Object.freeze({ * @returns {Object} The fix command. */ replaceTextRange(range, text) { - if (!validateRange(range)) { - return null; - } return { range, text @@ -152,9 +128,6 @@ const ruleFixer = Object.freeze({ * @returns {Object} The fix command. */ removeRange(range) { - if (!validateRange(range)) { - return null; - } return { range, text: "" diff --git a/tests/lib/linter/report-translator.js b/tests/lib/linter/report-translator.js index 6c7252b48f6..b31dec17851 100644 --- a/tests/lib/linter/report-translator.js +++ b/tests/lib/linter/report-translator.js @@ -1028,5 +1028,21 @@ describe("createReportTranslator", () => { "Node must be provided when reporting error if location is not provided" ); }); + + it("should throw an error if fix range is invalid", () => { + for (const badRange of [[0], [0, null], [null, 0], []]) { + assert.throws( + // eslint-disable-next-line no-loop-func + () => translateReport( + { + node, + messageId: "testMessage", + fix: () => ({ range: badRange, text: "foo" }) + } + ), + "Fix has invalid range" + ); + } + }); }); }); diff --git a/tests/lib/linter/rule-fixer.js b/tests/lib/linter/rule-fixer.js index 846f15e6d65..a70e735509c 100644 --- a/tests/lib/linter/rule-fixer.js +++ b/tests/lib/linter/rule-fixer.js @@ -9,27 +9,13 @@ //------------------------------------------------------------------------------ const assert = require("chai").assert, - sinon = require("sinon"); - -const proxyquire = require("proxyquire").noCallThru().noPreserveCache(); + ruleFixer = require("../../../lib/linter/rule-fixer"); //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ describe("RuleFixer", () => { - const log = { - info: sinon.spy(), - error: sinon.spy() - }; - const ruleFixer = proxyquire("../../../lib/linter/rule-fixer", { - "../shared/logging": log - }); - - afterEach(() => { - log.info.resetHistory(); - log.error.resetHistory(); - }); describe("insertTextBefore", () => { @@ -152,33 +138,4 @@ describe("RuleFixer", () => { }); - describe("validation", () => { - it("should return null given non-numeric indices", () => { - const invalidRanges = [ - [0, null], - ["1", 2], - [void 0, void 0], - [1] - ]; - const functions = [ - (range, text) => ruleFixer.insertTextBefore({ range }, text), - (range, text) => ruleFixer.insertTextBeforeRange(range, text), - (range, text) => ruleFixer.insertTextAfter({ range }, text), - (range, text) => ruleFixer.insertTextAfterRange(range, text), - range => ruleFixer.remove({ range }), - range => ruleFixer.removeRange(range), - range => ruleFixer.replaceText({ range }), - range => ruleFixer.replaceTextRange(range) - ]; - - for (const range of invalidRanges) { - for (const fn of functions) { - assert.isNull(fn(range, "x")); - assert.isTrue(log.error.called); - log.error.resetHistory(); - } - } - }); - }); - }); From 8062391e66333c2027342bf8dc3212d171fa6ee2 Mon Sep 17 00:00:00 2001 From: Jacob Bandes-Storch Date: Mon, 8 Mar 2021 16:49:56 -0600 Subject: [PATCH 4/6] Update from review --- lib/linter/report-translator.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/linter/report-translator.js b/lib/linter/report-translator.js index f7e7d172bbe..6b797c64983 100644 --- a/lib/linter/report-translator.js +++ b/lib/linter/report-translator.js @@ -120,9 +120,9 @@ function normalizeReportLoc(descriptor) { * @param {Fix|null} fix The fix to validate. * @returns {void} */ -function validateFix(fix) { +function assertValidFix(fix) { if (fix) { - assert(typeof fix.range[0] === "number" && typeof fix.range[1] === "number", `Fix has invalid range: ${fix.range}`); + assert(typeof fix.range[0] === "number" && typeof fix.range[1] === "number", `Fix has invalid range: ${JSON.stringify(fix, null, 2)}`); } } @@ -145,7 +145,7 @@ function compareFixesByRange(a, b) { */ function mergeFixes(fixes, sourceCode) { for (const fix of fixes) { - validateFix(fix); + assertValidFix(fix); } if (fixes.length === 0) { @@ -197,7 +197,7 @@ function normalizeFixes(descriptor, sourceCode) { return mergeFixes(Array.from(fix), sourceCode); } - validateFix(fix); + assertValidFix(fix); return fix; } From d7cefee640385d15fb0a1f49863ec222703a3db4 Mon Sep 17 00:00:00 2001 From: Jacob Bandes-Storch Date: Tue, 9 Mar 2021 10:51:25 -0600 Subject: [PATCH 5/6] Test ranges with undefined; test fix merging --- tests/lib/linter/report-translator.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/lib/linter/report-translator.js b/tests/lib/linter/report-translator.js index b31dec17851..9a95837b375 100644 --- a/tests/lib/linter/report-translator.js +++ b/tests/lib/linter/report-translator.js @@ -1030,7 +1030,7 @@ describe("createReportTranslator", () => { }); it("should throw an error if fix range is invalid", () => { - for (const badRange of [[0], [0, null], [null, 0], []]) { + for (const badRange of [[0], [0, null], [null, 0], [void 0, 1], [0, void 0], [void 0, void 0], []]) { assert.throws( // eslint-disable-next-line no-loop-func () => translateReport( @@ -1042,6 +1042,22 @@ describe("createReportTranslator", () => { ), "Fix has invalid range" ); + + assert.throws( + // eslint-disable-next-line no-loop-func + () => translateReport( + { + node, + messageId: "testMessage", + fix: () => [ + { range: [0, 0], text: "foo" }, + { range: badRange, text: "bar" }, + { range: [1, 1], text: "baz" } + ] + } + ), + "Fix has invalid range" + ); } }); }); From c37294ef26019cea7e1ac30715018fc919d46913 Mon Sep 17 00:00:00 2001 From: Jacob Bandes-Storch Date: Tue, 9 Mar 2021 10:53:25 -0600 Subject: [PATCH 6/6] Assert & test missing range --- lib/linter/report-translator.js | 2 +- tests/lib/linter/report-translator.js | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/linter/report-translator.js b/lib/linter/report-translator.js index 6b797c64983..75005c16e58 100644 --- a/lib/linter/report-translator.js +++ b/lib/linter/report-translator.js @@ -122,7 +122,7 @@ function normalizeReportLoc(descriptor) { */ function assertValidFix(fix) { if (fix) { - assert(typeof fix.range[0] === "number" && typeof fix.range[1] === "number", `Fix has invalid range: ${JSON.stringify(fix, null, 2)}`); + assert(fix.range && typeof fix.range[0] === "number" && typeof fix.range[1] === "number", `Fix has invalid range: ${JSON.stringify(fix, null, 2)}`); } } diff --git a/tests/lib/linter/report-translator.js b/tests/lib/linter/report-translator.js index 9a95837b375..c6dd242e330 100644 --- a/tests/lib/linter/report-translator.js +++ b/tests/lib/linter/report-translator.js @@ -1030,15 +1030,16 @@ describe("createReportTranslator", () => { }); it("should throw an error if fix range is invalid", () => { + assert.throws( + () => translateReport({ node, messageId: "testMessage", fix: () => ({ text: "foo" }) }), + "Fix has invalid range" + ); + for (const badRange of [[0], [0, null], [null, 0], [void 0, 1], [0, void 0], [void 0, void 0], []]) { assert.throws( // eslint-disable-next-line no-loop-func () => translateReport( - { - node, - messageId: "testMessage", - fix: () => ({ range: badRange, text: "foo" }) - } + { node, messageId: "testMessage", fix: () => ({ range: badRange, text: "foo" }) } ), "Fix has invalid range" );