From 1c6bc67ab87913308416f6c01411b7eb1426ae07 Mon Sep 17 00:00:00 2001 From: Teddy Katz Date: Wed, 27 Sep 2017 16:34:45 -0400 Subject: [PATCH] Fix: special EventEmitter keys leak information about other rules (#9328) `Linter` uses Node's `EventEmitter` API to register listeners for rules. However, the `EventEmitter` API has a few problems for this use case: * `EventEmitter` has three "special" events (`newListener`, `removeListener`, and `error`) which are called when something happens with another listener. This is undesirable because `Linter` allows rules to register listeners for arbitrary string events, and we don't want rule listeners to be able to detect each other. * `EventEmitter` calls listeners with a `this` value of the event emitter itself. This is undesirable because this would allow rules to modify or tamper with listeners registered by other rules. This commit fixes the problem by updating `Linter` to use a custom event-emitting object with a similar API, rather than `EventEmitter` itself. --- lib/linter.js | 8 +-- lib/util/node-event-generator.js | 24 ++------- lib/util/safe-emitter.js | 54 +++++++++++++++++++ .../code-path-analysis/code-path-analyzer.js | 4 +- tests/lib/linter.js | 17 ++++++ tests/lib/util/node-event-generator.js | 18 ++++--- tests/lib/util/safe-emitter.js | 43 +++++++++++++++ 7 files changed, 135 insertions(+), 33 deletions(-) create mode 100644 lib/util/safe-emitter.js create mode 100644 tests/lib/util/safe-emitter.js diff --git a/lib/linter.js b/lib/linter.js index a8fa58b9d82..8f1520aafb4 100755 --- a/lib/linter.js +++ b/lib/linter.js @@ -9,8 +9,7 @@ // Requirements //------------------------------------------------------------------------------ -const EventEmitter = require("events").EventEmitter, - eslintScope = require("eslint-scope"), +const eslintScope = require("eslint-scope"), levn = require("levn"), lodash = require("lodash"), blankScriptAST = require("../conf/blank-script.json"), @@ -20,6 +19,7 @@ const EventEmitter = require("events").EventEmitter, validator = require("./config/config-validator"), Environments = require("./config/environments"), applyDisableDirectives = require("./util/apply-disable-directives"), + createEmitter = require("./util/safe-emitter"), NodeEventGenerator = require("./util/node-event-generator"), SourceCode = require("./util/source-code"), Traverser = require("./util/traverser"), @@ -817,7 +817,7 @@ module.exports = class Linter { disableDirectives = []; } - const emitter = new EventEmitter().setMaxListeners(Infinity); + const emitter = createEmitter(); const traverser = new Traverser(); const ecmaFeatures = config.parserOptions.ecmaFeatures || {}; const ecmaVersion = config.parserOptions.ecmaVersion || 5; @@ -860,7 +860,7 @@ module.exports = class Linter { */ _linter: { report() {}, - on: emitter.on.bind(emitter) + on: emitter.on } } ) diff --git a/lib/util/node-event-generator.js b/lib/util/node-event-generator.js index 568a3b7fe10..05b343ae9f2 100644 --- a/lib/util/node-event-generator.js +++ b/lib/util/node-event-generator.js @@ -194,7 +194,7 @@ const parseSelector = lodash.memoize(rawSelector => { * * ```ts * interface EventGenerator { - * emitter: EventEmitter; + * emitter: SafeEmitter; * enterNode(node: ASTNode): void; * leaveNode(node: ASTNode): void; * } @@ -203,8 +203,10 @@ const parseSelector = lodash.memoize(rawSelector => { class NodeEventGenerator { /** - * @param {EventEmitter} emitter - An event emitter which is the destination of events. This emitter must already + * @param {SafeEmitter} emitter + * An SafeEmitter which is the destination of events. This emitter must already * have registered listeners for all of the events that it needs to listen for. + * (See lib/util/safe-emitter.js for more details on `SafeEmitter`.) * @returns {NodeEventGenerator} new instance */ constructor(emitter) { @@ -215,23 +217,7 @@ class NodeEventGenerator { this.anyTypeEnterSelectors = []; this.anyTypeExitSelectors = []; - const eventNames = typeof emitter.eventNames === "function" - - // Use the built-in eventNames() function if available (Node 6+) - ? emitter.eventNames() - - /* - * Otherwise, use the private _events property. - * Using a private property isn't ideal here, but this seems to - * be the best way to get a list of event names without overriding - * addEventListener, which would hurt performance. This property - * is widely used and unlikely to be removed in a future version - * (see https://github.com/nodejs/node/issues/1817). Also, future - * node versions will have eventNames() anyway. - */ - : Object.keys(emitter._events); // eslint-disable-line no-underscore-dangle - - eventNames.forEach(rawSelector => { + emitter.eventNames().forEach(rawSelector => { const selector = parseSelector(rawSelector); if (selector.listenerTypes) { diff --git a/lib/util/safe-emitter.js b/lib/util/safe-emitter.js new file mode 100644 index 00000000000..242cbe42959 --- /dev/null +++ b/lib/util/safe-emitter.js @@ -0,0 +1,54 @@ +/** + * @fileoverview A variant of EventEmitter which does not give listeners information about each other + * @author Teddy Katz + */ + +"use strict"; + +//------------------------------------------------------------------------------ +// Typedefs +//------------------------------------------------------------------------------ + +/** + * An object describing an AST selector + * @typedef {Object} SafeEmitter + * @property {function(eventName: string, listenerFunc: Function): void} on Adds a listener for a given event name + * @property {function(eventName: string, arg1?: any, arg2?: any, arg3?: any)} emit Emits an event with a given name. + * This calls all the listeners that were listening for that name, with `arg1`, `arg2`, and `arg3` as arguments. + * @property {function(): string[]} eventNames Gets the list of event names that have registered listeners. + */ + +/** + * Creates an object which can listen for and emit events. + * This is similar to the EventEmitter API in Node's standard library, but it has a few differences. + * The goal is to allow multiple modules to attach arbitrary listeners to the same emitter, without + * letting the modules know about each other at all. + * 1. It has no special keys like `error` and `newListener`, which would allow modules to detect when + * another module throws an error or registers a listener. + * 2. It calls listener functions without any `this` value. (`EventEmitter` calls listeners with a + * `this` value of the emitter instance, which would give listeners access to other listeners.) + * 3. Events can be emitted with at most 3 arguments. (For example: when using `emitter.emit('foo', a, b, c)`, + * the arguments `a`, `b`, and `c` will be passed to the listener functions.) + * @returns {SafeEmitter} An emitter + */ +module.exports = () => { + const listeners = Object.create(null); + + return Object.freeze({ + on(eventName, listener) { + if (eventName in listeners) { + listeners[eventName].push(listener); + } else { + listeners[eventName] = [listener]; + } + }, + emit(eventName, a, b, c) { + if (eventName in listeners) { + listeners[eventName].forEach(listener => listener(a, b, c)); + } + }, + eventNames() { + return Object.keys(listeners); + } + }); +}; diff --git a/tests/lib/code-path-analysis/code-path-analyzer.js b/tests/lib/code-path-analysis/code-path-analyzer.js index 82384b15731..e9878d6677b 100644 --- a/tests/lib/code-path-analysis/code-path-analyzer.js +++ b/tests/lib/code-path-analysis/code-path-analyzer.js @@ -10,11 +10,11 @@ //------------------------------------------------------------------------------ const assert = require("assert"), - EventEmitter = require("events").EventEmitter, fs = require("fs"), path = require("path"), Linter = require("../../../lib/linter"), EventGeneratorTester = require("../../../tools/internal-testers/event-generator-tester"), + createEmitter = require("../../../lib/util/safe-emitter"), debug = require("../../../lib/code-path-analysis/debug-helpers"), CodePath = require("../../../lib/code-path-analysis/code-path"), CodePathAnalyzer = require("../../../lib/code-path-analysis/code-path-analyzer"), @@ -55,7 +55,7 @@ function getExpectedDotArrows(source) { describe("CodePathAnalyzer", () => { EventGeneratorTester.testEventGeneratorInterface( - new CodePathAnalyzer(new NodeEventGenerator(new EventEmitter())) + new CodePathAnalyzer(new NodeEventGenerator(createEmitter())) ); describe("interface of code paths", () => { diff --git a/tests/lib/linter.js b/tests/lib/linter.js index eda0bd57174..a6352c0f962 100644 --- a/tests/lib/linter.js +++ b/tests/lib/linter.js @@ -100,6 +100,23 @@ describe("Linter", () => { linter.verify(code, config, filename, true); }, "Intentional error."); }); + + it("does not call rule listeners with a `this` value", () => { + const spy = sandbox.spy(); + + linter.defineRule("checker", () => ({ Program: spy })); + linter.verify("foo", { rules: { checker: "error" } }); + assert(spy.calledOnce); + assert.strictEqual(spy.firstCall.thisValue, void 0); + }); + + it("does not allow listeners to use special EventEmitter values", () => { + const spy = sandbox.spy(); + + linter.defineRule("checker", () => ({ newListener: spy })); + linter.verify("foo", { rules: { checker: "error", "no-undef": "error" } }); + assert(spy.notCalled); + }); }); describe("context.getSourceLines()", () => { diff --git a/tests/lib/util/node-event-generator.js b/tests/lib/util/node-event-generator.js index b59fabce7ea..67b6fa53a06 100644 --- a/tests/lib/util/node-event-generator.js +++ b/tests/lib/util/node-event-generator.js @@ -9,11 +9,11 @@ //------------------------------------------------------------------------------ const assert = require("assert"), - EventEmitter = require("events").EventEmitter, sinon = require("sinon"), espree = require("espree"), estraverse = require("estraverse"), EventGeneratorTester = require("../../../tools/internal-testers/event-generator-tester"), + createEmitter = require("../../../lib/util/safe-emitter"), NodeEventGenerator = require("../../../lib/util/node-event-generator"); //------------------------------------------------------------------------------ @@ -30,16 +30,16 @@ const ESPREE_CONFIG = { describe("NodeEventGenerator", () => { EventGeneratorTester.testEventGeneratorInterface( - new NodeEventGenerator(new EventEmitter()) + new NodeEventGenerator(createEmitter()) ); describe("entering a single AST node", () => { let emitter, generator; beforeEach(() => { - emitter = new EventEmitter(); + emitter = Object.create(createEmitter(), { emit: { value: sinon.spy() } }); + ["Foo", "Bar", "Foo > Bar", "Foo:exit"].forEach(selector => emitter.on(selector, () => {})); - emitter.emit = sinon.spy(emitter.emit); generator = new NodeEventGenerator(emitter); }); @@ -82,13 +82,15 @@ describe("NodeEventGenerator", () => { */ function getEmissions(ast, possibleQueries) { const emissions = []; - const emitter = new EventEmitter(); + const emitter = Object.create(createEmitter(), { + emit: { + value: (selector, node) => emissions.push([selector, node]) + } + }); possibleQueries.forEach(query => emitter.on(query, () => {})); const generator = new NodeEventGenerator(emitter); - emitter.emit = (selector, node) => emissions.push([selector, node]); - estraverse.traverse(ast, { enter(node, parent) { node.parent = parent; @@ -308,7 +310,7 @@ describe("NodeEventGenerator", () => { describe("parsing an invalid selector", () => { it("throws a useful error", () => { - const emitter = new EventEmitter(); + const emitter = createEmitter(); emitter.on("Foo >", () => {}); assert.throws( diff --git a/tests/lib/util/safe-emitter.js b/tests/lib/util/safe-emitter.js new file mode 100644 index 00000000000..7b127ea89fd --- /dev/null +++ b/tests/lib/util/safe-emitter.js @@ -0,0 +1,43 @@ +/** + * @fileoverview Tests for safe-emitter + * @author Teddy Katz + */ + +"use strict"; + +const createEmitter = require("../../../lib/util/safe-emitter"); +const assert = require("chai").assert; + +describe("safe-emitter", () => { + describe("emit() and on()", () => { + it("allows listeners to be registered calls them when emitted", () => { + const emitter = createEmitter(); + const colors = []; + + emitter.on("foo", () => colors.push("red")); + emitter.on("foo", () => colors.push("blue")); + emitter.on("bar", () => colors.push("green")); + + emitter.emit("foo"); + assert.deepEqual(colors, ["red", "blue"]); + + emitter.on("bar", color => colors.push(color)); + emitter.emit("bar", "yellow"); + + assert.deepEqual(colors, ["red", "blue", "green", "yellow"]); + }); + + it("calls listeners with no `this` value", () => { + const emitter = createEmitter(); + let called = false; + + emitter.on("foo", function() { + assert.strictEqual(this, void 0); // eslint-disable-line no-invalid-this + called = true; + }); + + emitter.emit("foo"); + assert(called); + }); + }); +});