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.

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

Refactor avoidCapture function #1399

Merged
merged 9 commits into from Jul 6, 2021
Merged

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented Jul 3, 2021

Reason behind this change, the new version of ESLint allows ecmaVersion: "latest", and this part don't understand it.

Let's not care about ecmaVersion and strict mode, just use the safest strategy.

WDYT? @futpib

@fisker fisker changed the title Simplify avoidCapture function Simplify avoidCapture function Jul 3, 2021
@fisker
Copy link
Collaborator Author

fisker commented Jul 3, 2021

On second thought, it's not safe anyway, running ESLint with specific ecma version, doesn't mean scripts always run on the same environment. I always use my shared config with ecmaVersion: 2021, but my code can run on old engine.

@fisker fisker marked this pull request as draft July 3, 2021 17:19
@fisker fisker marked this pull request as ready for review July 4, 2021 07:15
@futpib
Copy link
Collaborator

futpib commented Jul 5, 2021

My implementation tried to match the target ecma version closely with regards to reserved words and allowed arguments as a variable name where language allows it without shadowing. This rewrite bans reserved words regardless of ecma version and bans arguments name ignoring complex rules around it.

If original approach is not a valuable enough feature, this PR provides a more practical approach that will be easier to maintain and should be merged. Yet it seems original code could have been fixed to support 'latest' ecma version with a one line patch.

@fisker fisker marked this pull request as draft July 5, 2021 11:20
@fisker
Copy link
Collaborator Author

fisker commented Jul 5, 2021

I also found potential threat, for example we used it in catch-error-name, if some bad shared config, config it as

"unicorn/catch-error-name": [
	"error",
	{
		"name": "){} evilCode; if(false"
	}
]
try {} catch (e) {}

will fix to

try {} catch (){} evilCode; if(false) {}

And it may run into infinity loop in all used rules,

for example:

"unicorn/prevent-abbreviations": [
	"error",
	{
		"replacements":  {
			"a" : {
				123: true
			}
		}
	}
]

current implementation will not find a usable variable name and hang forever, I'll fix.

@sindresorhus
Copy link
Owner

I also found potential threat, for example we used it in catch-error-name, if some bad shared config, config it as

I wouldn't consider that a vulnerability. It applies to a lot of ESLint rules. Rules make no guarantee about being resistant to evil config, nor should they.

@fisker
Copy link
Collaborator Author

fisker commented Jul 5, 2021

But the infinity one, people may config it by mistake, for example, added a space after the replacement.

@sindresorhus
Copy link
Owner

The infinity case I agree we should fix.

@fisker fisker changed the title Simplify avoidCapture function Refactor avoidCapture function Jul 5, 2021
@fisker fisker marked this pull request as ready for review July 5, 2021 17:28
@sindresorhus sindresorhus merged commit e8dc017 into sindresorhus:main Jul 6, 2021
@fisker fisker deleted the avoid-capture branch July 6, 2021 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants