-
Notifications
You must be signed in to change notification settings - Fork 298
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 new ESLint plugin: @datadog/eslint-plugin #4539
base: master
Are you sure you want to change the base?
Conversation
Overall package sizeSelf size: 6.85 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.0.1 | 15.59 MB | 15.6 MB | | @datadog/native-iast-taint-tracking | 3.0.0 | 11.14 MB | 11.15 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 6.56 MB | | @datadog/native-iast-rewriter | 2.3.1 | 2.15 MB | 2.24 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.8.1 | 71.67 kB | 785.15 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | path-to-regexp | 0.1.7 | 6.78 kB | 6.78 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
@@ -63,7 +63,7 @@ function scrubChildProcessCmd (expression) { | |||
|
|||
if (token === null) { | |||
continue | |||
} else if (typeof token === 'object') { | |||
} else if (typeof token === 'object') { // eslint-disable-line @datadog/safe-typeof-object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a limitation of the plugin to me, as there is a check for null
. If that check is removed for any reason, because of this comment the rule won't catch the now invalid code.
@@ -25,6 +25,7 @@ function taintObject (iastContext, object, type) { | |||
} else { | |||
parent[key] = tainted | |||
} | |||
// eslint-disable-next-line @datadog/safe-typeof-object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just add the check for null
instead of disabling the rule? Same question for pretty much all other occurrences.
(Adding my slack comment here to avoid the retention clean) In web-ui we also use eslint 8, and we have local custom rules without issues. Example in
There's also stuffs like https://github.com/taskworld/eslint-plugin-local or I don't think we should publish a public package just for this, especially with the current effort in https://datadoghq.atlassian.net/wiki/spaces/FRON/pages/3017179854/2023-07-26+-+Common+eslint+and+prettier+config+for+JS+TS+projects (but you probably were not aware of it) |
Add a new ESLint plugin: @datadog/eslint-plugin
This plugin currently only contains a single rule,
@datadog/safer-typeof-object
, which will ensure that anytypeof x === 'object'
is preceded by a not-null check:Bad:
Good:
This is because
typeof null === 'object'
, so any operation on the variable that doesn't expect anull
value will at best lead to unexpected behaviour, and at works to an uncaught exception crashing the process.The source code previously contained a lot of instances violating this code, but this was recently fixed in #4517. This PR is an attempt to ensure no new violations sneak in.
Note to reviewers
I'm working on publishing
@datadog/eslint-plugin
to npm (but need access first). But since it's not really an issue to have a dev-dependency that points to a GitHub repo, I think it's safe to merge this as-is. Once the package is published to npm, I'll make a follow up PR to update the dependency to use that version.