Fix: special EventEmitter keys leak information about other rules #9328

not-an-aardvark · 2017-09-19T00:26:46Z

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix

Tell us about your environment

ESLint Version: master
Node Version: 8.5.0
npm Version: 5.3.0

What parser (default, Babel-ESLint, etc.) are you using?

N/A

Please show your full configuration:

N/A

What did you do? Please include the actual source code causing the issue.

"use strict";

const eslint = require("eslint");
const linter = new eslint.Linter();

linter.defineRule("foo-rule", context => {
    return {
        newListener() {
            assert(false);
        }
    }
});

linter.verify("foo", { rules: { "foo-rule": "error", "no-undef": "error" } });

What did you expect to happen?

I expected the newListener method to never get called, because there are no nodes with type newListener.

What actually happened? Please include the actual, raw output from ESLint.

The newListener method got called because a listener was added from no-undef, and the string "newListener" has special behavior when added to an event emitter.

What changes did you make? (Give an overview)

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.

Is there anything you'd like reviewers to focus on?

Nothing in particular

platinumazure

Left a few notes about sinon usage.

platinumazure · 2017-09-19T03:18:20Z

tests/lib/linter.js

+
+            linter.defineRule("checker", () => ({ Program: spy }));
+            linter.verify("foo", { rules: { checker: "error" } });
+            assert(spy.calledOnce);


I think this would be more readable by adding assert.notOk(spy.firstCall.thisValue) so all of the assertions are in one place. But I won't insist on it.

platinumazure · 2017-09-19T03:19:34Z

tests/lib/linter.js

+
+            linter.defineRule("checker", () => ({ newListener: spy }));
+            linter.verify("foo", { rules: { checker: "error", "no-undef": "error" } });
+            assert(!spy.calledOnce);


This should be assert(!spy.called) or assert(spy.notCalled) or similar. Using calledOnce means a call twice would erroneously pass.

platinumazure

LGTM, just left a few suggestions for improving the inline documentation. Sorry for the delay.

platinumazure · 2017-09-25T13:21:07Z

lib/linter.js

+ * 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.
+ * @returns {{on: Function, emit: Function, eventNames: Function}} An emitter


I'd love to see this document in a JSDoc typedef, but this is nowhere near a blocker.

platinumazure · 2017-09-25T13:22:07Z

lib/linter.js

+ * 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.


I'd love to see an example of 3-argument usage documented somewhere, maybe here, if possible. Not a blocker.

ilyavolodin · 2017-09-27T02:08:46Z

lib/linter.js

@@ -50,6 +49,40 @@ const MAX_AUTOFIX_PASSES = 10;
 //------------------------------------------------------------------------------

 /**
+ * Creates an object which can listen for and emit events.


Can this be moved out into it's own file? Since the behavior doesn't have direct connection to Linter, I'd rather see a separate file, even if it's small.

`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.

eslintbot · 2017-09-27T04:51:39Z

LGTM

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features labels Sep 19, 2017

platinumazure requested changes Sep 19, 2017

View reviewed changes

not-an-aardvark force-pushed the no-emitter-leaks branch from 2a01331 to 2a874d2 Compare September 22, 2017 00:42

not-an-aardvark requested a review from platinumazure September 25, 2017 06:14

platinumazure approved these changes Sep 25, 2017

View reviewed changes

ilyavolodin suggested changes Sep 27, 2017

View reviewed changes

not-an-aardvark added 3 commits September 26, 2017 23:56

Improve sinon usage

3ae72a3

Fix tests on Node 4

f6fe282

not-an-aardvark force-pushed the no-emitter-leaks branch 2 times, most recently from 862dee5 to 9d82dd1 Compare September 27, 2017 04:50

eslint deleted a comment from eslintbot Sep 27, 2017

Move safe-emitter to its own file

0c82e73

not-an-aardvark force-pushed the no-emitter-leaks branch from 9d82dd1 to 0c82e73 Compare September 27, 2017 04:51

eslint deleted a comment from eslintbot Sep 27, 2017

ilyavolodin approved these changes Sep 27, 2017

View reviewed changes

not-an-aardvark merged commit 1c6bc67 into master Sep 27, 2017

not-an-aardvark deleted the no-emitter-leaks branch September 27, 2017 20:34

eslint-deprecated bot locked and limited conversation to collaborators Mar 27, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: special EventEmitter keys leak information about other rules #9328

Fix: special EventEmitter keys leak information about other rules #9328

not-an-aardvark commented Sep 19, 2017

platinumazure left a comment

platinumazure Sep 19, 2017

platinumazure Sep 19, 2017

platinumazure left a comment

platinumazure Sep 25, 2017

platinumazure Sep 25, 2017

ilyavolodin Sep 27, 2017

eslintbot commented Sep 27, 2017

Fix: special EventEmitter keys leak information about other rules #9328

Fix: special EventEmitter keys leak information about other rules #9328

Conversation

not-an-aardvark commented Sep 19, 2017

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure Sep 19, 2017

Choose a reason for hiding this comment

platinumazure Sep 19, 2017

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

platinumazure Sep 25, 2017

Choose a reason for hiding this comment

platinumazure Sep 25, 2017

Choose a reason for hiding this comment

ilyavolodin Sep 27, 2017

Choose a reason for hiding this comment

eslintbot commented Sep 27, 2017