[meeting demo] Upgrade eslint and fix security alerts #117

devalevenkatesh · 2021-12-28T03:07:28Z

Issue #:

Get react meeting demo security vulnerability alerts to 0.

Description of changes:

Upgraded ES Lint version and fixed all warnings and errors for the added rules.
Removed unused packages.
Upgrades react-dev-utils to latest major version. This does not affect
us as we use a plugin which does not have any breaking changes.
Now npm run build will also lint the changes.

Testing

How did you test these changes?

Ran locally and did sanity checks using 2 attendees.
Deployed the demo app serverlessly and again did the sanity checks.
Everything worked fine as expected with no console level errors.

Can these changes be tested using one of the demo application? If yes, which demo application can be used to test it?

meeting demo.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

1. Upgraded ES Lint version and rules. 2. Removed unused packages. 3. Upgrades react-dev-utils to latest major version. This does not affect us as we use a plugin which does not have any breaking changes. 4. Now npm run build will also lint the changes.

xuesichao · 2021-12-28T03:24:06Z

Shall we change single quote to double quote? Maybe we should still use singe quote rule to keep consistent.

devalevenkatesh · 2021-12-28T17:41:15Z

Shall we change single quote to double quote? Maybe we should still use singe quote rule to keep consistent.

I knew I would get this comment lol, sorry dint fixed in first and thanks for the review! I have fixed it.

ltrung · 2021-12-28T18:09:16Z

apps/meeting/package.json

    "css-loader": "^6.2.0",
-    "eslint": "^5.16.0",
-    "eslint-config-airbnb": "^17.1.1",


Is there any reason we don't want to use this?

I configured the eslint using command line where it asks for whether you want to use any preferred eslint configs like if to follow airbnb and so on. I checked the last deleted eslint config file and did not find any reference to airbnb rules hence removed it.

We were also not running lint so Instead of choosing the set solution, I used our own with basic set of rules. With this setup I fixed around (12 errors and 34 warnings). I think we can make them more stricter as we want later. Still should I try airbnb config?

no just want to check whether you think we should follow airbnb as it is quite popular. Anyway, we can always revisit this when we have a single centralize eslint config :)

Let me do a quick check with airbnb setup and get back if I can fix all lets use it as a standard solution for now, if overwhelming errors and warning I will keep this setup for now 😅

So, I got below errors with airbnb style guide:
✖ 447 problems (437 errors, 10 warnings)

We can re-visit this later 😅 .

apps/meeting/package.json

xuesichao

We still have many any type in our code, instead of disable eslint for those lines, I would prefer to disable no-explicit-any rule in our config.

rules: {
    "@typescript-eslint/no-explicit-any": "off"
  },

Maybe we should also disable @typescript-eslint/no-empty-function rule.

apps/meeting/src/constants/index.ts

apps/meeting/src/containers/EndMeetingControl/index.tsx

apps/meeting/src/containers/MeetingRoster.tsx

ltrung · 2021-12-28T18:37:15Z

We still have many any type in our code, instead of disable eslint for those lines, I would prefer to disable no-explicit-any rule in our config.
rules: {
    "@typescript-eslint/no-explicit-any": "off"
  },
Maybe we should also disable @typescript-eslint/no-empty-function rule.

We should enforce it and remove as we move along. Disable it entirely just open for more any type :)

apps/meeting/.eslintrc.json

xuesichao

LGTM, but you may want to get another approval.
Thanks for upgrading these. 👍

* Update meeting app to inclue CPU utilization options for background blur * Add onDeviceReplacement in meeting demo * Prepare dev branch * Conditionally show the background blur checkbox depending on if background blur is enabled * Improve code style of api.ts file (#95) * Remove tarball and do not change package.json (#98) * Fix the display bug of zoom in/zoom out label (#99) * Update react component library to latest released version (#101) * Update webpack config to fix infinite reloading issue (#110) * Update the uuid import statement (#111) * Add /end API to dev server proxy (#114) * Allow to enable simulcast and priority video policies (#113) * [meeting demo] Upgrade eslint and fix security alerts (#117) * [meeting demo] Upgrade eslint and fix security alerts 1. Upgraded ES Lint version and rules. 2. Removed unused packages. 3. Upgrades react-dev-utils to latest major version. This does not affect us as we use a plugin which does not have any breaking changes. 4. Now npm run build will also lint the changes. * Use single over double quotes * Add rule for trailing commas * Use trailing commas for multi-line only * Fixed Web Audio not being enabled properly, disabled the ability to have web audio and disable audio video enabled at once because of a bug and reduntant logic * Version bump for latest release (#121) * Resolve meeting-dev to main conflicts (#123) * Update main meeting (#102) * Update meeting app to inclue CPU utilization options for background blur * Add onDeviceReplacement in meeting demo * Prepare dev branch * Conditionally show the background blur checkbox depending on if background blur is enabled * Improve code style of api.ts file (#95) * Remove tarball and do not change package.json (#98) * Update react component library to latest released version Co-authored-by: David Collette <[email protected]> Co-authored-by: chnjing <[email protected]> Co-authored-by: Michael Hyun <[email protected]> Co-authored-by: Sichao Xue (sichax) <[email protected]> * Add contributing guidelines for apps/meeting demo (#105) * Add contributing guidelines for apps/meeting demo * Address comments * [amplify-demo] Fix security alerts (#115) * Fix security alerts for amplify demo * NPM Update tmpl for fixing security vulnerability * Address README comments * Fix duplicate files in gitignore * [real-time-collaboration] security alerts fix (#116) * [real-time-chat-collaboration-demo] fix security alerts * Fix eslint import/extensions error by removing "js" extension * [moderated-chat-and-sentiment-analysis] security alert fix (#119) Co-authored-by: Venkatesh Devale <[email protected]> Co-authored-by: David Collette <[email protected]> Co-authored-by: chnjing <[email protected]> Co-authored-by: Michael Hyun <[email protected]> Co-authored-by: Sichao Xue (sichax) <[email protected]> Co-authored-by: David Collette <[email protected]> Co-authored-by: chnjing <[email protected]> Co-authored-by: Venkatesh Devale <[email protected]> Co-authored-by: Michael Hyun <[email protected]> Co-authored-by: Sichao Xue (sichax) <[email protected]> Co-authored-by: Venkatesh Devale <[email protected]> Co-authored-by: Daitarn <[email protected]> Co-authored-by: Trung Le <[email protected]>

devalevenkatesh requested a review from a team as a code owner December 28, 2021 03:07

devalevenkatesh force-pushed the meeting-demo-security-alerts branch from 52859ef to b046739 Compare December 28, 2021 17:36

Use single over double quotes

75d5ad9

devalevenkatesh force-pushed the meeting-demo-security-alerts branch from b046739 to 75d5ad9 Compare December 28, 2021 17:39

ltrung reviewed Dec 28, 2021

View reviewed changes

michhyun1 reviewed Dec 28, 2021

View reviewed changes

apps/meeting/package.json Outdated Show resolved Hide resolved

xuesichao reviewed Dec 28, 2021

View reviewed changes

apps/meeting/src/constants/index.ts Outdated Show resolved Hide resolved

apps/meeting/src/containers/EndMeetingControl/index.tsx Outdated Show resolved Hide resolved

apps/meeting/src/containers/MeetingRoster.tsx Outdated Show resolved Hide resolved

Add rule for trailing commas

4be4caf

devalevenkatesh force-pushed the meeting-demo-security-alerts branch from 3de2acc to 4be4caf Compare December 28, 2021 19:32

xuesichao reviewed Dec 28, 2021

View reviewed changes

apps/meeting/.eslintrc.json Outdated Show resolved Hide resolved

xuesichao reviewed Dec 28, 2021

View reviewed changes

apps/meeting/.eslintrc.json Outdated Show resolved Hide resolved

Use trailing commas for multi-line only

b771d4d

devalevenkatesh force-pushed the meeting-demo-security-alerts branch from cf41a80 to b771d4d Compare December 28, 2021 21:57

xuesichao approved these changes Dec 28, 2021

View reviewed changes

ltrung approved these changes Dec 28, 2021

View reviewed changes

devalevenkatesh merged commit dad3304 into aws-samples:meeting-dev Dec 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[meeting demo] Upgrade eslint and fix security alerts #117

[meeting demo] Upgrade eslint and fix security alerts #117

devalevenkatesh commented Dec 28, 2021

xuesichao commented Dec 28, 2021

devalevenkatesh commented Dec 28, 2021

ltrung Dec 28, 2021

devalevenkatesh Dec 28, 2021

ltrung Dec 28, 2021

devalevenkatesh Dec 28, 2021

devalevenkatesh Dec 28, 2021

xuesichao left a comment •

edited

Loading

ltrung commented Dec 28, 2021

xuesichao left a comment

[meeting demo] Upgrade eslint and fix security alerts #117

[meeting demo] Upgrade eslint and fix security alerts #117

Conversation

devalevenkatesh commented Dec 28, 2021

xuesichao commented Dec 28, 2021

devalevenkatesh commented Dec 28, 2021

ltrung Dec 28, 2021

Choose a reason for hiding this comment

devalevenkatesh Dec 28, 2021

Choose a reason for hiding this comment

ltrung Dec 28, 2021

Choose a reason for hiding this comment

devalevenkatesh Dec 28, 2021

Choose a reason for hiding this comment

devalevenkatesh Dec 28, 2021

Choose a reason for hiding this comment

xuesichao left a comment • edited Loading

Choose a reason for hiding this comment

ltrung commented Dec 28, 2021

xuesichao left a comment

Choose a reason for hiding this comment

xuesichao left a comment •

edited

Loading