Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: allow references to external globals in id-blacklist (fixes #12567) #12987

Merged
merged 2 commits into from Mar 23, 2020
Merged

Fix: allow references to external globals in id-blacklist (fixes #12567) #12987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c1dd9e
Fix: allow references to external globals in id-blacklist (fixes #12567)
mdjermanovic Mar 1, 2020
e961ab0
Simplify global reference check
mdjermanovic Mar 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
240 changes: 138 additions & 102 deletions lib/rules/id-blacklist.js
View file
Open in desktop
Expand Up @@ -6,6 +6,105 @@

"use strict";

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------

/**
* Checks whether the given node represents assignment target in a normal assignment or destructuring.
* @param {ASTNode} node The node to check.
* @returns {boolean} `true` if the node is assignment target.
*/
function isAssignmentTarget(node) {
const parent = node.parent;

return (

// normal assignment
(
parent.type === "AssignmentExpression" &&
parent.left === node
) ||

// destructuring
parent.type === "ArrayPattern" ||
parent.type === "RestElement" ||
(
parent.type === "Property" &&
parent.value === node &&
parent.parent.type === "ObjectPattern"
) ||
(
parent.type === "AssignmentPattern" &&
parent.left === node
)
);
}

/**
* Checks whether the given node represents an imported name that is renamed in the same import/export specifier.
*
* Examples:
* import { a as b } from 'mod'; // node `a` is renamed import
* export { a as b } from 'mod'; // node `a` is renamed import
* @param {ASTNode} node `Identifier` node to check.
* @returns {boolean} `true` if the node is a renamed import.
*/
function isRenamedImport(node) {
const parent = node.parent;

return (
(
parent.type === "ImportSpecifier" &&
parent.imported !== parent.local &&
parent.imported === node
) ||
(
parent.type === "ExportSpecifier" &&
parent.parent.source && // re-export
parent.local !== parent.exported &&
parent.local === node
)
);
}

/**
* Checks whether the given node is a renamed identifier node in an ObjectPattern destructuring.
*
* Examples:
* const { a : b } = foo; // node `a` is renamed node.
* @param {ASTNode} node `Identifier` node to check.
* @returns {boolean} `true` if the node is a renamed node in an ObjectPattern destructuring.
*/
function isRenamedInDestructuring(node) {
const parent = node.parent;

return (
(
!parent.computed &&
parent.type === "Property" &&
parent.parent.type === "ObjectPattern" &&
parent.value !== node &&
parent.key === node
)
);
}

/**
* Checks whether the given node represents shorthand definition of a property in an object literal.
* @param {ASTNode} node `Identifier` node to check.
* @returns {boolean} `true` if the node is a shorthand property definition.
*/
function isShorthandPropertyDefinition(node) {
const parent = node.parent;

return (
parent.type === "Property" &&
parent.parent.type === "ObjectExpression" &&
parent.shorthand
);
}

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -35,88 +134,64 @@ module.exports = {

create(context) {


//--------------------------------------------------------------------------
// Helpers
//--------------------------------------------------------------------------

const blacklist = context.options;
const blacklist = new Set(context.options);
const reportedNodes = new Set();

let globalScope;

/**
* Checks if a string matches the provided pattern
* @param {string} name The string to check.
* @returns {boolean} if the string is a match
* Checks whether the given name is blacklisted.
* @param {string} name The name to check.
* @returns {boolean} `true` if the name is blacklisted.
* @private
*/
function isInvalid(name) {
return blacklist.indexOf(name) !== -1;
function isBlacklisted(name) {
return blacklist.has(name);
}

/**
* Checks whether the given node represents an imported name that is renamed in the same import/export specifier.
*
* Examples:
* import { a as b } from 'mod'; // node `a` is renamed import
* export { a as b } from 'mod'; // node `a` is renamed import
* Checks whether the given node represents a reference to a global variable that is not declared in the source code.
* These identifiers will be allowed, as it is assumed that user has no control over the names of external global variables.
* @param {ASTNode} node `Identifier` node to check.
* @returns {boolean} `true` if the node is a renamed import.
* @returns {boolean} `true` if the node is a reference to a global variable.
*/
function isRenamedImport(node) {
const parent = node.parent;
function isReferenceToGlobalVariable(node) {
const variable = globalScope.set.get(node.name);

return (
(
parent.type === "ImportSpecifier" &&
parent.imported !== parent.local &&
parent.imported === node
) ||
(
parent.type === "ExportSpecifier" &&
parent.parent.source && // re-export
parent.local !== parent.exported &&
parent.local === node
)
);
return variable && variable.defs.length === 0 &&
variable.references.some(ref => ref.identifier === node);
}

/**
* Checks whether the given node is a renamed identifier node in an ObjectPattern destructuring.
*
* Examples:
* const { a : b } = foo; // node `a` is renamed node.
* @param {ASTNode} node `Identifier` node to check.
* @returns {boolean} `true` if the node is a renamed node in an ObjectPattern destructuring.
* Determines whether the given node should be checked.
* @param {ASTNode} node `Identifier` node.
* @returns {boolean} `true` if the node should be checked.
*/
function isRenamedInDestructuring(node) {
function shouldCheck(node) {
const parent = node.parent;

return (
(
!parent.computed &&
parent.type === "Property" &&
parent.parent.type === "ObjectPattern" &&
parent.value !== node &&
parent.key === node
)
);
}

/**
* Verifies if we should report an error or not.
* @param {ASTNode} node The node to check
* @returns {boolean} whether an error should be reported or not
*/
function shouldReport(node) {
const parent = node.parent;
/*
* Member access has special rules for checking property names.
* Read access to a property with a blacklisted name is allowed, because it can be on an object that user has no control over.
* Write access isn't allowed, because it potentially creates a new property with a blacklisted name.
*/
if (
parent.type === "MemberExpression" &&
Copy link
Member

@yeonjuan yeonjuan Mar 11, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, Is this PR intended to ignoring check an identifier in an assignment that has no declaration?

I tested it and recognized it pass the blacklist identifier with assignment in destructuring.

This is the case which I tested.

({a: parseInt} = a); // no warning in this pr version.

// but it warns with the declaration.
var paseInt; ({a: parseInt} = a);  // warning

And it is warned in v6.8.0 (no declaraion)

/*eslint id-blacklist: ["error", "parseInt"] */
({a: parseInt} = foo); // warning

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe, Is this PR intended to ignoring check an identifier in an assignment that has no declaration?

Only if it is a global variable that isn't declared in the same source code. The logic is: if it is declared, then we can assume it's a user-controlled name. Otherwise, it's a built-in/env- specific/third-party global so user can't change the name.

It could be also indeed a user-controlled name declared elsewhere, but in that case id-blacklist will warn in the file where it is declared.

This shouldn't be a warning because parseInt is a global variable that isn't declared in this code:

/*eslint id-blacklist: ["error", "parseInt"] */

({a: parseInt} = a);

This should be warning (2 warnings, one for each Identifier node) because parseInt is declared:

/*eslint id-blacklist: ["error", "parseInt"] */

var parseInt; ({a: parseInt} = a);

This should be also a warning because foo isn't defined in the configuration as a global variable:

/*eslint id-blacklist: ["error", "foo"] */

({a: foo} = a);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdjermanovic

Only if it is a global variable that isn't declared in the same source code. The logic is: if it is declared, then we can assume it's a user-controlled name. Otherwise, it's a built-in/env- specific/third-party global so user can't change the name.

Thanks for the explanation. 👍 it makes sense to me.

parent.property === node &&
!parent.computed
) {
return isAssignmentTarget(parent);
}

return (
parent.type !== "CallExpression" &&
parent.type !== "NewExpression" &&
!isRenamedImport(node) &&
!isRenamedInDestructuring(node) &&
isInvalid(node.name)
!(
isReferenceToGlobalVariable(node) &&
!isShorthandPropertyDefinition(node)
)
);
}

Expand All @@ -141,54 +216,15 @@ module.exports = {

return {

Identifier(node) {

// MemberExpressions get special rules
if (node.parent.type === "MemberExpression") {
const name = node.name,
effectiveParent = node.parent.parent;

// Always check object names
if (node.parent.object.type === "Identifier" &&
node.parent.object.name === name) {
if (isInvalid(name)) {
report(node);
}

// Report AssignmentExpressions only if they are the left side of the assignment
} else if (effectiveParent.type === "AssignmentExpression" &&
(effectiveParent.right.type !== "MemberExpression" ||
effectiveParent.left.type === "MemberExpression" &&
effectiveParent.left.property.name === name)) {
if (isInvalid(name)) {
report(node);
}

// Report the last identifier in an ObjectPattern destructuring.
} else if (
(
effectiveParent.type === "Property" &&
effectiveParent.value === node.parent &&
effectiveParent.parent.type === "ObjectPattern"
) ||
effectiveParent.type === "RestElement" ||
effectiveParent.type === "ArrayPattern" ||
(
effectiveParent.type === "AssignmentPattern" &&
effectiveParent.left === node.parent
)
) {
if (isInvalid(name)) {
report(node);
}
}
Program() {
globalScope = context.getScope();
},

} else if (shouldReport(node)) {
Identifier(node) {
if (isBlacklisted(node.name) && shouldCheck(node)) {
report(node);
}
}

};

}
};