From e59a1a58cb464d830fe46bd77ae7697c54ba6a5d Mon Sep 17 00:00:00 2001 From: Jean <110341611+jean-michelet@users.noreply.github.com> Date: Sun, 28 Jan 2024 21:50:26 +0100 Subject: [PATCH] refactor: runASTAnalysis functions to use class AstAnalyser (#216) * refactor: create and use AstAnalyser class to handle ast analysis * refactor: remove useless JSDoc * fix: ESTree type from meriyah * fix: add JSDoc again * fix: optimize regular expression by removing non-capturing group * fix: revert regex to the original pattern * fix: declare class AstAnalyser instead of interface * fix: replace Parser type by SourceParser type * fix: parsers should return body directly --- index.d.ts | 4 + index.js | 94 +++--------- src/AstAnalyser.js | 129 ++++++++++++++++ src/JsSourceParser.js | 12 +- src/SourceParser.js | 34 ----- test/AstAnalyser.spec.js | 237 ++++++++++++++++++++++++++++++ test/JsSourceParser.spec.js | 8 +- test/SourceParser.spec.js | 80 ---------- test/fixtures/FakeSourceParser.js | 5 + test/runASTAnalysis.spec.js | 149 +++---------------- test/runASTAnalysisOnFile.spec.js | 47 ++++-- types/api.d.ts | 25 ++-- 12 files changed, 472 insertions(+), 352 deletions(-) create mode 100644 src/AstAnalyser.js delete mode 100644 src/SourceParser.js create mode 100644 test/AstAnalyser.spec.js delete mode 100644 test/SourceParser.spec.js create mode 100644 test/fixtures/FakeSourceParser.js diff --git a/index.d.ts b/index.d.ts index 7714596..e29aa50 100644 --- a/index.d.ts +++ b/index.d.ts @@ -1,4 +1,6 @@ import { + AstAnalyser, + SourceParser, runASTAnalysis, runASTAnalysisOnFile, Report, @@ -20,6 +22,8 @@ declare const warnings: Record)/g, ""); + } +} diff --git a/src/JsSourceParser.js b/src/JsSourceParser.js index 35586b4..2b0ac1a 100644 --- a/src/JsSourceParser.js +++ b/src/JsSourceParser.js @@ -1,9 +1,6 @@ // Import Third-party Dependencies import * as meriyah from "meriyah"; -// Import Internal Dependencies -import { SourceParser } from "./SourceParser.js"; - // CONSTANTS const kParsingOptions = { next: true, @@ -12,19 +9,19 @@ const kParsingOptions = { jsx: true }; -export class JsSourceParser extends SourceParser { +export class JsSourceParser { /** * @param {object} options * @param {boolean} options.isEcmaScriptModule */ - parseScript(options = {}) { + parse(source, options = {}) { const { isEcmaScriptModule } = options; try { const { body } = meriyah.parseScript( - this.source, + source, { ...kParsingOptions, module: isEcmaScriptModule, @@ -43,7 +40,7 @@ export class JsSourceParser extends SourceParser { isIllegalReturn )) { const { body } = meriyah.parseScript( - this.source, + source, { ...kParsingOptions, module: true, @@ -58,4 +55,3 @@ export class JsSourceParser extends SourceParser { } } } - diff --git a/src/SourceParser.js b/src/SourceParser.js deleted file mode 100644 index 0eb0894..0000000 --- a/src/SourceParser.js +++ /dev/null @@ -1,34 +0,0 @@ -export class SourceParser { - /** - * @param {!string} source - * @param {object} options - * @param {boolean} [options.removeHTMLComments=false] - */ - constructor(source, options = {}) { - if (typeof source !== "string") { - throw new TypeError("source must be a string"); - } - const { removeHTMLComments = false } = options; - - this.raw = source; - - /** - * if the file start with a shebang then we remove it because meriyah.parseScript fail to parse it. - * @example - * #!/usr/bin/env node - */ - const rawNoShebang = source.charAt(0) === "#" ? - source.slice(source.indexOf("\n") + 1) : source; - - this.source = removeHTMLComments ? - this.#removeHTMLComment(rawNoShebang) : rawNoShebang; - } - - /** - * @param {!string} str - * @returns {string} - */ - #removeHTMLComment(str) { - return str.replaceAll(/)/g, ""); - } -} diff --git a/test/AstAnalyser.spec.js b/test/AstAnalyser.spec.js new file mode 100644 index 0000000..fc58856 --- /dev/null +++ b/test/AstAnalyser.spec.js @@ -0,0 +1,237 @@ +// Import Node.js Dependencies +import { describe, it } from "node:test"; +import assert from "node:assert"; +import { readFileSync } from "node:fs"; + +// Import Internal Dependencies +import { AstAnalyser } from "../src/AstAnalyser.js"; +import { JsSourceParser } from "../src/JsSourceParser.js"; +import { getWarningKind } from "./utils/index.js"; + +// CONSTANTS +const FIXTURE_URL = new URL("fixtures/searchRuntimeDependencies/", import.meta.url); + +describe("AstAnalyser", (t) => { + describe("analyse", () => { + it("should return all dependencies required at runtime", () => { + const { dependencies, warnings } = getAnalyser().analyse(` + const http = require("http"); + const net = require("net"); + const fs = require("fs").promises; + + require("assert").strictEqual; + require("timers"); + require("./aFile.js"); + + const myVar = "path"; + require(myVar); + `, { module: false }); + + assert.strictEqual(warnings.length, 0); + assert.deepEqual([...dependencies.keys()], + ["http", "net", "fs", "assert", "timers", "./aFile.js", "path"] + ); + }); + + it("should throw a 'suspicious-literal' warning when given a code with a suspicious string", () => { + const suspectString = readFileSync( + new URL("suspect-string.js", FIXTURE_URL), + "utf-8" + ); + const { warnings, stringScore } = getAnalyser().analyse(suspectString); + + assert.deepEqual( + getWarningKind(warnings), + ["suspicious-literal"].sort() + ); + assert.strictEqual(stringScore, 8); + }); + + it("should throw a 'suspicious-file' warning because the file contains to much encoded-literal warnings", () => { + const suspectString = readFileSync( + new URL("suspiciousFile.js", FIXTURE_URL), + "utf-8" + ); + const { warnings } = getAnalyser().analyse(suspectString); + + assert.deepEqual( + getWarningKind(warnings), + ["suspicious-file"].sort() + ); + }); + + it("should combine same encoded-literal as one warning with multiple locations", () => { + const { warnings } = getAnalyser().analyse(` + const foo = "18c15e5c5c9dac4d16f9311a92bb8331"; + const bar = "18c15e5c5c9dac4d16f9311a92bb8331"; + const xd = "18c15e5c5c9dac4d16f9311a92bb8331"; + `); + + assert.strictEqual(warnings.length, 1); + assert.deepEqual( + getWarningKind(warnings), + ["encoded-literal"].sort() + ); + + const [encodedLiteral] = warnings; + assert.strictEqual(encodedLiteral.location.length, 3); + }); + + it("should be capable to follow a malicious code with hexa computation and reassignments", () => { + const { warnings, dependencies } = getAnalyser().analyse(` + function unhex(r) { + return Buffer.from(r, "hex").toString(); + } + + const g = eval("this"); + const p = g["pro" + "cess"]; + + const evil = p["mainMod" + "ule"][unhex("72657175697265")]; + const work = evil(unhex("2e2f746573742f64617461")); + `); + + assert.deepEqual(getWarningKind(warnings), [ + "encoded-literal", + "unsafe-import", + "unsafe-stmt" + ].sort()); + assert.deepEqual([...dependencies.keys()], ["./test/data"]); + }); + + it("should throw a 'short-identifiers' warning for a code with only one-character identifiers", () => { + const { warnings } = getAnalyser().analyse(` + var a = 0, b, c, d; + for (let i = 0; i < 10; i++) { + a += i; + } + let de = "foo"; + let x, z; + `); + + assert.deepEqual(getWarningKind(warnings), ["short-identifiers"].sort()); + }); + + it("should detect dependency required under a TryStatement", () => { + const { dependencies } = getAnalyser().analyse(` + try { + require("http"); + } + catch {} + `); + + assert.ok(dependencies.has("http")); + assert.ok(dependencies.get("http").inTry); + }); + + it("should return isOneLineRequire true given a single line CJS export", () => { + const { dependencies, isOneLineRequire } = getAnalyser().analyse( + "module.exports = require('foo');" + ); + + assert.ok(isOneLineRequire); + assert.deepEqual([...dependencies.keys()], ["foo"]); + }); + + it("should be capable to extract dependencies name for ECMAScript Modules (ESM)", () => { + const { dependencies, warnings } = getAnalyser().analyse(` + import * as http from "http"; + import fs from "fs"; + import { foo } from "xd"; + `, { module: true }); + + assert.strictEqual(warnings.length, 0); + assert.deepEqual( + [...dependencies.keys()].sort(), + ["http", "fs", "xd"].sort() + ); + }); + }); + + it("remove the packageName from the dependencies list", async() => { + const result = await getAnalyser().analyseFile( + new URL("depName.js", FIXTURE_URL), + { module: false, packageName: "foobar" } + ); + + assert.ok(result.ok); + assert.strictEqual(result.warnings.length, 0); + assert.deepEqual([...result.dependencies.keys()], + ["open"] + ); + }); + + it("should fail with a parsing error", async() => { + const result = await getAnalyser().analyseFile( + new URL("parsingError.js", FIXTURE_URL), + { module: false, packageName: "foobar" } + ); + + assert.strictEqual(result.ok, false); + assert.strictEqual(result.warnings.length, 1); + + const parsingError = result.warnings[0]; + assert.strictEqual(parsingError.kind, "parsing-error"); + }); + + describe("prepareSource", () => { + it("should remove shebang at the start of the file", (t) => { + const source = "#!/usr/bin/env node\nconst hello = \"world\";"; + const preparedSource = getAnalyser().prepareSource(source); + + assert.strictEqual( + preparedSource, + "const hello = \"world\";" + ); + }); + + it("should not remove shebang if not at the start (that's an illegal code)", () => { + const source = "const hello = \"world\";\n#!/usr/bin/env node"; + const preparedSource = getAnalyser().prepareSource(source); + + assert.strictEqual( + preparedSource, + source + ); + }); + + it("should remove singleline HTML comment from source code when removeHTMLComments is enabled", () => { + const preparedSource = getAnalyser().prepareSource("", { + removeHTMLComments: true + }); + + assert.strictEqual(preparedSource, ""); + }); + + it("should remove multiline HTML comment from source code when removeHTMLComments is enabled", () => { + const preparedSource = getAnalyser().prepareSource(` + + `, { + removeHTMLComments: true + }); + + assert.strictEqual(preparedSource.trim(), ""); + }); + + it("should remove multiple HTML comments", () => { + const preparedSource = getAnalyser().prepareSource( + "\nconst yo = 'foo'\n", { + removeHTMLComments: true + }); + + assert.strictEqual(preparedSource, "\nconst yo = 'foo'\n"); + }); + }); +}); + +let analyser = null; +function getAnalyser() { + if (!analyser) { + analyser = new AstAnalyser(new JsSourceParser()); + } + + return analyser; +} diff --git a/test/JsSourceParser.spec.js b/test/JsSourceParser.spec.js index 8715711..a8352c7 100644 --- a/test/JsSourceParser.spec.js +++ b/test/JsSourceParser.spec.js @@ -5,15 +5,15 @@ import { describe, it } from "node:test"; import { JsSourceParser } from "../src/JsSourceParser.js"; describe("JsSourceParser", () => { - describe("parseScript", () => { + describe("parse", () => { it("should not crash even if isEcmaScriptModule 'false' is provided (import keyword)", () => { - new JsSourceParser("import * as foo from \"foo\";").parseScript({ + new JsSourceParser().parse("import * as foo from \"foo\";", { isEcmaScriptModule: false }); }); it("should not crash even if isEcmaScriptModule 'false' is provided (export keyword)", () => { - new JsSourceParser("export const foo = 5;").parseScript({ + new JsSourceParser().parse("export const foo = 5;", { isEcmaScriptModule: false }); }); @@ -25,7 +25,7 @@ describe("JsSourceParser", () => { return {children({ ...props, open })}; });`; - new JsSourceParser(code).parseScript({ + new JsSourceParser().parse(code, { isEcmaScriptModule: false }); }); diff --git a/test/SourceParser.spec.js b/test/SourceParser.spec.js deleted file mode 100644 index 4301638..0000000 --- a/test/SourceParser.spec.js +++ /dev/null @@ -1,80 +0,0 @@ -// Import Node.js Dependencies -import { describe, it } from "node:test"; -import assert from "node:assert"; - -// Import Internal Dependencies -import { SourceParser } from "../src/SourceParser.js"; - -describe("SourceParser", () => { - describe("constructor", () => { - it("should throw a TypeError if source is not a string", () => { - assert.throws( - () => new SourceParser(10), - { message: "source must be a string" } - ); - }); - - it("should not update the content of raw", () => { - const expectedStr = "hello world"; - - assert.strictEqual( - new SourceParser(expectedStr).raw, - expectedStr - ); - - assert.strictEqual( - new SourceParser(expectedStr, { removeHTMLComments: true }).raw, - expectedStr - ); - }); - - it("should remove shebang at the start of the file", () => { - const sp = new SourceParser("#!/usr/bin/env node\nconst hello = \"world\";"); - - assert.strictEqual( - sp.source, - "const hello = \"world\";" - ); - }); - - it("should not remove shebang if not at the start (that's an illegal code)", () => { - const source = "const hello = \"world\";\n#!/usr/bin/env node"; - const sp = new SourceParser(source); - - assert.strictEqual( - sp.source, - source - ); - }); - - it("should remove singleline HTML comment from source code when removeHTMLComments is enabled", () => { - const sp = new SourceParser("", { - removeHTMLComments: true - }); - - assert.strictEqual(sp.source, ""); - }); - - it("should remove multiline HTML comment from source code when removeHTMLComments is enabled", () => { - const sp = new SourceParser(` - - `, { - removeHTMLComments: true - }); - - assert.strictEqual(sp.source.trim(), ""); - }); - - it("should remove multiple HTML comments", () => { - const sp = new SourceParser("\nconst yo = 'foo'\n", { - removeHTMLComments: true - }); - - assert.strictEqual(sp.source, "\nconst yo = 'foo'\n"); - }); - }); -}); diff --git a/test/fixtures/FakeSourceParser.js b/test/fixtures/FakeSourceParser.js new file mode 100644 index 0000000..af51c98 --- /dev/null +++ b/test/fixtures/FakeSourceParser.js @@ -0,0 +1,5 @@ +export class FakeSourceParser { + parse(str, options) { + return [{ type: "LiteralExpression" }]; + } +} diff --git a/test/runASTAnalysis.spec.js b/test/runASTAnalysis.spec.js index 791c5c6..318ff24 100644 --- a/test/runASTAnalysis.spec.js +++ b/test/runASTAnalysis.spec.js @@ -1,144 +1,39 @@ // Import Node.js Dependencies -import { readFileSync } from "node:fs"; -import { test } from "node:test"; +import { it } from "node:test"; import assert from "node:assert"; // Import Internal Dependencies import { runASTAnalysis } from "../index.js"; -import { getWarningKind } from "./utils/index.js"; +import { AstAnalyser } from "../src/AstAnalyser.js"; +import { JsSourceParser } from "../src/JsSourceParser.js"; +import { FakeSourceParser } from "./fixtures/FakeSourceParser.js"; -// CONSTANTS -const FIXTURE_URL = new URL("fixtures/searchRuntimeDependencies/", import.meta.url); +it("should call AstAnalyser.analyse with the expected arguments", (t) => { + t.mock.method(AstAnalyser.prototype, "analyse"); -test("it should return all dependencies required at runtime", () => { - const { dependencies, warnings } = runASTAnalysis(` - const http = require("http"); - const net = require("net"); - const fs = require("fs").promises; + const source = "const http = require(\"http\");"; + runASTAnalysis(source, { module: true, removeHTMLComments: true }); - require("assert").strictEqual; - require("timers"); - require("./aFile.js"); + const source2 = "const fs = require(\"fs\");"; + runASTAnalysis(source2, { module: false, removeHTMLComments: false }); - const myVar = "path"; - require(myVar); - `, { module: false }); + const calls = AstAnalyser.prototype.analyse.mock.calls; + assert.strictEqual(calls.length, 2); - assert.strictEqual(warnings.length, 0); - assert.deepEqual([...dependencies.keys()], - ["http", "net", "fs", "assert", "timers", "./aFile.js", "path"] - ); + assert.deepEqual(calls[0].arguments, [source, { module: true, removeHTMLComments: true }]); + assert.deepEqual(calls[1].arguments, [source2, { module: false, removeHTMLComments: false }]); }); -test("it should throw a 'suspicious-literal' warning when given a code with a suspicious string", () => { - const suspectString = readFileSync( - new URL("suspect-string.js", FIXTURE_URL), - "utf-8" - ); - const { warnings, stringScore } = runASTAnalysis(suspectString); - - assert.deepEqual( - getWarningKind(warnings), - ["suspicious-literal"].sort() - ); - assert.strictEqual(stringScore, 8); -}); +it("should instantiate AstAnalyser with the expected parser", (t) => { + t.mock.method(JsSourceParser.prototype, "parse"); + t.mock.method(FakeSourceParser.prototype, "parse"); -test("it should throw a 'suspicious-file' warning because the file contains to much encoded-literal warnings", () => { - const suspectString = readFileSync( - new URL("suspiciousFile.js", FIXTURE_URL), - "utf-8" - ); - const { warnings } = runASTAnalysis(suspectString); + runASTAnalysis("const http = require(\"http\");", { module: true, removeHTMLComments: true }); - assert.deepEqual( - getWarningKind(warnings), - ["suspicious-file"].sort() + runASTAnalysis("const fs = require(\"fs\");", + { module: false, removeHTMLComments: false, customParser: new FakeSourceParser() } ); -}); - -test("it should combine same encoded-literal as one warning with multiple locations", () => { - const { warnings } = runASTAnalysis(` - const foo = "18c15e5c5c9dac4d16f9311a92bb8331"; - const bar = "18c15e5c5c9dac4d16f9311a92bb8331"; - const xd = "18c15e5c5c9dac4d16f9311a92bb8331"; - `); - - assert.strictEqual(warnings.length, 1); - assert.deepEqual( - getWarningKind(warnings), - ["encoded-literal"].sort() - ); - - const [encodedLiteral] = warnings; - assert.strictEqual(encodedLiteral.location.length, 3); -}); - -test("it should be capable to follow a malicious code with hexa computation and reassignments", () => { - const { warnings, dependencies } = runASTAnalysis(` - function unhex(r) { - return Buffer.from(r, "hex").toString(); - } - - const g = eval("this"); - const p = g["pro" + "cess"]; - - const evil = p["mainMod" + "ule"][unhex("72657175697265")]; - const work = evil(unhex("2e2f746573742f64617461")); - `); - assert.deepEqual(getWarningKind(warnings), [ - "encoded-literal", - "unsafe-import", - "unsafe-stmt" - ].sort()); - assert.deepEqual([...dependencies.keys()], ["./test/data"]); -}); - -test("it should throw a 'short-identifiers' warning for a code with only one-character identifiers", () => { - const { warnings } = runASTAnalysis(` - var a = 0, b, c, d; - for (let i = 0; i < 10; i++) { - a += i; - } - let de = "foo"; - let x, z; - `); - - assert.deepEqual(getWarningKind(warnings), ["short-identifiers"].sort()); -}); - -test("it should detect dependency required under a TryStatement", () => { - const { dependencies } = runASTAnalysis(` - try { - require("http"); - } - catch {} - `); - - assert.ok(dependencies.has("http")); - assert.ok(dependencies.get("http").inTry); -}); - -test("it should return isOneLineRequire true given a single line CJS export", () => { - const { dependencies, isOneLineRequire } = runASTAnalysis( - "module.exports = require('foo');" - ); - - assert.ok(isOneLineRequire); - assert.deepEqual([...dependencies.keys()], ["foo"]); -}); - -test("it should be capable to extract dependencies name for ECMAScript Modules (ESM)", () => { - const { dependencies, warnings } = runASTAnalysis(` - import * as http from "http"; - import fs from "fs"; - import { foo } from "xd"; - `, { module: true }); - - assert.strictEqual(warnings.length, 0); - assert.deepEqual( - [...dependencies.keys()].sort(), - ["http", "fs", "xd"].sort() - ); + assert.strictEqual(JsSourceParser.prototype.parse.mock.calls.length, 1); + assert.strictEqual(FakeSourceParser.prototype.parse.mock.calls.length, 1); }); diff --git a/test/runASTAnalysisOnFile.spec.js b/test/runASTAnalysisOnFile.spec.js index 542439a..42a018d 100644 --- a/test/runASTAnalysisOnFile.spec.js +++ b/test/runASTAnalysisOnFile.spec.js @@ -1,35 +1,52 @@ // Import Node.js Dependencies -import { test } from "node:test"; +import { it } from "node:test"; import assert from "node:assert"; // Import Internal Dependencies import { runASTAnalysisOnFile } from "../index.js"; +import { AstAnalyser } from "../src/AstAnalyser.js"; +import { FakeSourceParser } from "./fixtures/FakeSourceParser.js"; +import { JsSourceParser } from "../src/JsSourceParser.js"; // CONSTANTS const FIXTURE_URL = new URL("fixtures/searchRuntimeDependencies/", import.meta.url); -test("it remove the packageName from the dependencies list", async() => { - const result = await runASTAnalysisOnFile( - new URL("depName.js", FIXTURE_URL), +it("should call AstAnalyser.analyseFile with the expected arguments", async(t) => { + t.mock.method(AstAnalyser.prototype, "analyseFile"); + + const url = new URL("depName.js", FIXTURE_URL); + await runASTAnalysisOnFile( + url, { module: false, packageName: "foobar" } ); - assert.ok(result.ok); - assert.strictEqual(result.warnings.length, 0); - assert.deepEqual([...result.dependencies.keys()], - ["open"] + const url2 = new URL("parsingError.js", FIXTURE_URL); + await runASTAnalysisOnFile( + url, + { module: true, packageName: "foobar2" } ); + + const calls = AstAnalyser.prototype.analyseFile.mock.calls; + assert.strictEqual(calls.length, 2); + + assert.deepEqual(calls[0].arguments, [url, { module: false, packageName: "foobar" }]); + assert.deepEqual(calls[1].arguments, [url2, { module: true, packageName: "foobar2" }]); }); -test("it should fail with a parsing error", async() => { - const result = await runASTAnalysisOnFile( - new URL("parsingError.js", FIXTURE_URL), +it("should instantiate AstAnalyser with the expected parser", async(t) => { + t.mock.method(JsSourceParser.prototype, "parse"); + t.mock.method(FakeSourceParser.prototype, "parse"); + + await runASTAnalysisOnFile( + new URL("depName.js", FIXTURE_URL), { module: false, packageName: "foobar" } ); - assert.strictEqual(result.ok, false); - assert.strictEqual(result.warnings.length, 1); + await runASTAnalysisOnFile( + new URL("parsingError.js", FIXTURE_URL), + { module: true, packageName: "foobar2", customParser: new FakeSourceParser() } + ); - const parsingError = result.warnings[0]; - assert.strictEqual(parsingError.kind, "parsing-error"); + assert.strictEqual(JsSourceParser.prototype.parse.mock.calls.length, 1); + assert.strictEqual(FakeSourceParser.prototype.parse.mock.calls.length, 1); }); diff --git a/types/api.d.ts b/types/api.d.ts index 5f6a3af..5d05248 100644 --- a/types/api.d.ts +++ b/types/api.d.ts @@ -1,6 +1,9 @@ import { Warning } from "./warnings.js"; +import { Statement } from "meriyah/dist/src/estree.js"; export { + AstAnalyser, + SourceParser, runASTAnalysis, runASTAnalysisOnFile, @@ -44,6 +47,8 @@ interface RuntimeOptions { * @default false */ removeHTMLComments?: boolean; + + customParser?: SourceParser; } interface Report { @@ -54,16 +59,8 @@ interface Report { isOneLineRequire: boolean; } -interface RuntimeFileOptions { +interface RuntimeFileOptions extends Omit { packageName?: string; - /** - * @default true - */ - module?: boolean; - /** - * @default false - */ - removeHTMLComments?: boolean; } type ReportOnFile = { @@ -76,5 +73,15 @@ type ReportOnFile = { warnings: Warning[]; } +interface SourceParser { + parse(source: string, options: unknown): Statement[]; +} + +declare class AstAnalyser { + constructor(parser: SourceParser); + analyse: (str: string, options?: Omit) => Report; + analyzeFile(pathToFile: string, options?: Omit): Promise; +} + declare function runASTAnalysis(str: string, options?: RuntimeOptions): Report; declare function runASTAnalysisOnFile(pathToFile: string, options?: RuntimeFileOptions): Promise;