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

Add eslint-plugin-security and unify .eslintrc.yml #4079

Merged
merged 24 commits into from
Nov 3, 2021

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Oct 28, 2021

Changelog Entry

Added

  • Adds eslint-plugin-security, consolidate .eslintrc.yml at project root, and treat warnings as errors, by @compulim, in PR #4079

Description

We are adding new ESLint rules for security, plus some maintenance work around current ESLint configurations.

The new rules added are warnings, but not errors. As security is top priority for us. We will fail on all warnings and requires an explicit explanation to ignore those warnings.

Also, current ESLint configurations scatter in multiple projects, we are consolidating them into a single set of configurations at the project root for easier maintenance.

We also take some time to clean up the configuration files:

  • Sorts all rules alphabetically
  • Enables @typescript-eslint (rules and parser) only for .ts and .tsx files, but not all files
    • .js will use eslint:no-unused-vars, while .ts will use @typescript-eslint/no-unused-vars

However, we did not resolve #4003, another ESLint related issue, as that one would introduce lot of changes and put a heavier burden to reviewers.

Design

Prevent object injection is square bracket accessors

Using +index

ESLint has an (easy) alarm on array[index]. We turn them into array[+index] to make sure if index is a string like "prototype", we will catch it as NaN and return undefined.

Dealing with the issue

In the order of best practices, we should use the following order when applying the protection.

  1. "Don't use it"
    • Type it out declaratively, don't use map[key]
    • Instead of ['one', 'two'].map(key => map[key]), use the declarative form [map.one, map.two]
  2. Prefixing
    • When using map as a storage, prefix the user-inputted key
    • Instead of storage[theirKey], use storage['id-' + theirKey]
  3. Allowlist
    • Instead of ourMap[theirKey], try to protect it by Object.keys(ourMap).includes(theirKey) && ourMap[theirKey]
  4. Denylist
    • Deny prototype and everything from Object.prototype
    • A new function isForbiddenPropertyNames exposed from core packages will help denylisting
    • This is last resort

Hoisting eslint family of packages to the project root

ESLint resolves dependencies based on where the .eslintrc.yml/plugin is defined. As many of our packages are extending it from the project root .eslintrc.yml, we need to put eslint-plugin-* at the root, where ESLint will resolve them from.

We are removing eslint* packages from our individual package, and hoist them to the project root.

One exception is directlinespeech as we try our best to isolate it out of our monorepo story and make it easy to eject.

Converging into a single .eslintrc.yml

We attempted to converge all ESLint configurations into a single .eslintrc.yml and use file extension approach to differentiate them. Say, .jsx will have React ruleset enabled, etc. But we failed to do so because:

  • Some .js files are custom hooks and should have React ruleset enabled for them
  • Some .js files are CJS and some are ESM, requires slightly different modification to the configuration (i.e. parserOptions)

To-do

  • Add --report-unused-disable-directives to ESLint
  • Add --max-warnings 0 to ESLint
    • Verify ESLint error out with warnings (i.e. non-fatal) from eslint-plugin-security
  • Sort rules in .eslintrc.yml
  • Update verbiage of CHANGELOG.md

Specific Changes

  • Re-run Prettier
  • Fixed all ESLint issues with existing rules
  • Removed all unused ESLint directives (after adding --report-unused-disable-directives)
  • ESLint will now fail on any warnings (--max-warnings 0)
  • Added eslint-plugin-security/recommended
  • eslint and related dependencies is now installed at root, instead of per package
    • Except directlinespeech, which we want to make it easily eject-able from our monorepo
  • Converged all .eslintrc.yml into 3 versions located at the root
    • .eslintrc.yml is the base configuration and platform neutral
      • Contains section for TypeScript (.ts, .tsx) and Jest (__tests__, *.spec.js, *.spec.ts)
    • .eslintrc.react.yml is for projects with React or React Hooks
    • .eslintrc.node.yml is for CLI projects
  • Enable no-undefined rule
  • Removed unused npm run prettier script
  • Added function names to anonymous functions
  • Updated to use non-deprecated API from event-target-shim
    • page-object/src/globals/testHelpers/speech/speechSynthesis/MockAudioContext.js
  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

@compulim compulim marked this pull request as ready for review November 2, 2021 20:03
@compulim compulim merged commit b6bc206 into microsoft:main Nov 3, 2021
@compulim compulim deleted the feat-add-eslint-security branch November 3, 2021 02:48
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.

ESLint is not properly configured for TypeScript
2 participants