Add @elastic/safer-lodash-set as an alternative to lodash.set #67452

watson · 2020-05-27T10:31:04Z

This PR adds new ESLint rules that instructs developers to not use set and setWith from the lodash module, but instead use the equivalent functions from a new @elastic/safer-lodash-set module.

This new module is located in packages/elastic-safer-lodash-set. This module is going to be published to npm as well once everyone is happy with it.

This PR both adds the ESLint rules, the @elastic/safer-lodash-set module, and updates any code that violates those new ESLint rules. Since we use set/setWith from lodash all over the code-base, this PR touches upon a lot of files. This can seem a bit daunting, but to ease the review process, you might want to first check out the files inside of packages/elastic-safer-lodash-set. These can be viewed as a standalone module with its own documentation, tests, etc. So I suggest reading the README.md to get a better understanding of what it's doing.

Todo

Fix lodash/fp ESLint errors (two files only: x-pack/plugins/siem/server/lib/hosts/elasticsearch_adapter.ts and x-pack/plugins/siem/server/lib/timeline/routes/utils/common.ts)
Ensure package tests run as part of CI
Ensure LICENSE file is ok
Either publish to npm or update README.md file to say it's not on npm and add "private": true to package.json

elasticmachine · 2020-05-27T10:31:09Z

Pinging @elastic/uptime (Team:uptime)

x-pack/package.json

watson · 2020-05-27T14:33:28Z

@elastic/siem-security-team I need your help verifying something in this PR:

This PR adds a safer version of lodash.set and lodash.setWith. Unfortunately, lodash also ships with a "functional programming" version of this function under the lodash/fp namespace. This PR does not provide a safe version of these fp functions.

As far as I can see, the only place where we use any of those functions are in the SIEM plugin as evident by these ESLint errors:

x-pack/plugins/siem/server/lib/hosts/elasticsearch_adapter.ts
  7:1  error  'lodash/fp' import is restricted from being used. Please use @elastic/safer-lodash-set instead  no-restricted-imports

x-pack/plugins/siem/server/lib/timeline/routes/utils/common.ts
  6:1  error  'lodash/fp' import is restricted from being used. Please use @elastic/safer-lodash-set instead  no-restricted-imports

Creating an fp version of these functions is proving difficult, so instead, I'd rather just add these two locations to a whitelist so that ESLint ignores that they use lodash.set. Before I do this, I want to make sure that we don't rely on user-provided data for the 1st argument (the path) being passed to the lodash.set function?

For reference, here are the places where you use lodash.set:

kibana/x-pack/plugins/siem/server/lib/hosts/elasticsearch_adapter.ts

Line 122 in b7e393d

return set(`node.${fieldName}`, fieldValue, flattenedFields);

kibana/x-pack/plugins/siem/server/lib/hosts/elasticsearch_adapter.ts

Line 139 in b7e393d

return set(fieldName, fieldValue, flattenedFields);

kibana/x-pack/plugins/siem/server/lib/timeline/routes/utils/common.ts

Lines 21 to 29 in b7e393d

    
           return set<FrameworkRequest>( 
        
             'user', 
        
             user, 
        
             set<KibanaRequest & { context: RequestHandlerContext }>( 
        
               'context.core.savedObjects.client', 
        
               savedObjectsClient, 
        
               request 
        
             ) 
        
           );

watson · 2020-05-27T14:38:42Z

@elastic/kibana-operations This PR adds a new package under the packages directory (packages/kbn-safer-lodash-set). If you're inside that directory you can run npm test to run the tests for the package:

kibana/packages/kbn-safer-lodash-set/package.json

Lines 11 to 12 in 3619c8e

    
           "lint": "dependency-check --no-dev package.json set.js setWith.js", 
        
           "test": "npm run lint && node test.js",

I'm not sure if our CI system runs these tests automatically, or if I need to do something specific to ensure that the package tests are included in the regular CI run?

elasticmachine · 2020-05-27T14:48:06Z

Pinging @elastic/kibana-security (Team:Security)

legrego

Thanks for this, @watson!
While I'm reviewing the rest, I wanted to mention a couple comments I had left on the original review:

Can we make sure that the existing lodash set tests are included in our test suite as well?
We should provide type definitions as part of this package, so that consumers can have type safety without having to manually cast.

watson · 2020-05-28T15:02:48Z

@legrego:

Can we make sure that the existing lodash set tests are included in our test suite as well?

The tests you link to are actually from the upcoming v5.0.0 and currently doesn't actually work as far as I can see. The file isn't even in v4.x, in which all tests for all lodash functions are just in one huge file as all tests have been completely refactored in master 😞

Here's a snippet from their CONTRIBUTING file:

Pardon the mess. The master branch is in flux while we work on Lodash v5. This means things like npm scripts, which we encourage using for contributions, may not be working. Thank you for your patience.

I took a look to see if we could somehow re-use this, but it can't be easily done. Not without rewriting all the setup and test-harness code at least. So I don't think it's worth it currently, what do you think?

We should provide type definitions as part of this package, so that consumers can have type safety without having to manually cast.

Good idea 👍

legrego · 2020-05-28T15:36:56Z

I took a look to see if we could somehow re-use this, but it can't be easily done. Not without rewriting all the setup and test-harness code at least. So I don't think it's worth it currently, what do you think?

Ah sorry, I failed to notice that master wasn't at all close to the 4.x release. I agree it isn't worth it to try to port the harness over, but there may be test cases that we want to replicate. I can take a closer look in a bit

FrankHassanabad

LGTM for just the one spot that it references within security_solution

thomasneirynck

maps/* code review. lgtm. Thank you!

stephmilovic

Thanks for fixing the type issues with SIEM. LGTM 🏁

watson · 2020-07-09T22:17:50Z

@elasticmachine merge upstream

watson · 2020-07-10T05:56:55Z

@elasticmachine merge upstream

gtback

LGTM from the licensing perspective.

azasypkin · 2020-07-15T05:53:30Z

CI failure doesn't look related to the PR and seems master isn't stable either. Let's try again.

azasypkin · 2020-07-15T05:53:51Z

@elasticmachine merge upstream

kibanamachine · 2020-07-15T07:55:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1b4d708

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
beats_management	233	+58	175
canvas	884	+58	826
core	434	+58	376
data	263	+58	205
esUiShared	175	+58	117
infra	658	+58	600
kibanaLegacy	86	+58	28
monitoring	275	+58	217
savedObjectsManagement	73	+58	15
securitySolution	933	+169	764
upgradeAssistant	84	+58	26
visTypeTimeseries	327	+4	323
visTypeVislib	296	+58	238
visualizations	77	+58	19
watcher	107	+58	49
total	-	+927	-

async chunks size

id	value	diff	baseline
beats_management	535.6KB	+53.0B	535.6KB
canvas	2.1MB	-7.0B	2.1MB
infra	4.7MB	+26.0KB	4.7MB
kibanaLegacy	189.6KB	+156.0B	189.4KB
monitoring	1.5MB	+24.8KB	1.5MB
savedObjectsManagement	232.9KB	+25.3KB	207.6KB
securitySolution	9.1MB	+73.7KB	9.0MB
upgradeAssistant	443.2KB	+22.4KB	420.8KB
visTypeTimeseries	2.4MB	+2.2KB	2.4MB
visTypeVislib	64.4KB	+3.0B	64.4KB
watcher	795.5KB	+25.0KB	770.5KB
total	-	+199.7KB	-

miscellaneous assets size

id	value	diff	baseline
beats_management	224.1KB	+5.1KB	219.0KB
canvas	521.0KB	+5.4KB	515.6KB
core	463.9KB	+5.2KB	458.7KB
data	400.5KB	+5.4KB	395.2KB
esUiShared	395.1KB	+5.5KB	389.6KB
infra	118.0KB	+57.0B	118.0KB
kibanaLegacy	1.0MB	+4.9KB	1.0MB
monitoring	104.2KB	+55.0B	104.2KB
savedObjectsManagement	88.5KB	+3.0B	88.4KB
securitySolution	660.2KB	+5.8KB	654.4KB
upgradeAssistant	22.6KB	-4.0B	22.6KB
visTypeTimeseries	107.7KB	-81.0B	107.7KB
visTypeVislib	505.4KB	+5.3KB	500.1KB
visualizations	143.8KB	+5.2KB	138.6KB
watcher	15.7KB	+14.0B	15.6KB
total	-	+47.8KB	-

page load bundle size

id	value	diff	baseline
beats_management	580.2KB	+17.4KB	562.8KB
canvas	1.4MB	+17.5KB	1.4MB
core	1.3MB	+17.5KB	1.2MB
data	1.4MB	+17.5KB	1.4MB
esUiShared	1006.2KB	+17.4KB	988.8KB
kibanaLegacy	232.9KB	+17.4KB	215.6KB
securitySolution	885.5KB	+17.7KB	867.8KB
visTypeVislib	1.3MB	+17.5KB	1.3MB
visualizations	421.3KB	+17.4KB	403.8KB
watcher	37.3KB	+2.0B	37.3KB
total	-	+157.3KB	-

History

💔 Build #61813 failed 415fdd2
💔 Build #61801 failed 9b99a23
💚 Build #61628 succeeded daf8eb0
💚 Build #61480 succeeded 41c6acf
💔 Build #60582 failed 7dd9e3e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

azasypkin · 2020-07-15T08:29:16Z

Okay, I see end2end-for-apm-ui is failing, but it's not marking CI red and hence not blocking this PR. I'm not sure if these tests are just flaky or not, didn't manage to run them locally and for some reason I see this test suite runs only for certain PRs. Pinged @elastic/apm-ui to help to figure that out. If it happens to be related to this PR we'll handle that in a follow-up that is much simpler to keep in sync with the constantly changing master.

UPDATE: 7.x CI run didn't have any failures.

azasypkin · 2020-07-15T10:20:03Z

7.x/7.9.0: 2764abe

* master: [APM] Add error rates to Service Map popovers (elastic#69520) [Security Solution][Detection Engine] - Update exceptions logic (elastic#71512) [Security Solution] Full screen timeline, Collapse event (elastic#71786) [Security Solution][Exception Modal] Create endpoint exception list if it doesn't already exist (elastic#71807) [Detection Rules] Add 7.9 rules (elastic#71808) [Search] Add telemetry for data plugin search service (elastic#70677) Add @elastic/safer-lodash-set as an alternative to lodash.set (elastic#67452) [tests] Temporarily skipped to promote snapshot

watson · 2020-08-05T09:08:40Z

Thank you @azasypkin for all your help landing and backporting this PR 🙏

watson added 4 commits May 27, 2020 10:16

Initial inline of @elastic/safer-lodash-set

05d9664

Comply with Kibana monorepo rules andi styling

fafb173

Refactor all use of lodash.set to @elastic/safer-lodash-set

8358e2b

Add lodash/fp to ESLint blacklist

f4f7eeb

watson added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels May 27, 2020

watson requested a review from legrego May 27, 2020 10:31

watson self-assigned this May 27, 2020

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label May 27, 2020

Remove standardjs badge from README.md

4d6d865

watson commented May 27, 2020

View reviewed changes

x-pack/package.json Show resolved Hide resolved

watson added 2 commits May 27, 2020 16:05

Rename @elastic/safer-lodash-set to @kbn/safer-lodash-set

d41353c

For now we don't want to publish this module

3619c8e

watson changed the title ~~Add @elastic/safer-lodash-set as an alternative to lodash.set~~ Add @kbn/safer-lodash-set as an alternative to lodash.set May 27, 2020

watson added the Feature:Hardening Harding of Kibana from a security perspective label May 27, 2020

watson added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label May 27, 2020

legrego reviewed May 28, 2020

View reviewed changes

elastic deleted a comment from elasticmachine Jun 10, 2020

watson added 2 commits June 10, 2020 15:45

Merge branch 'master' into safer-lodash-set

49d8875

Remove duplicate entry in x-pack/package.json

e68344a

FrankHassanabad approved these changes Jun 24, 2020

View reviewed changes

watson added 2 commits June 25, 2020 14:04

Merge branch 'master' into safer-lodash-set

8dd6a76

Rename @kbn namespace back to @elastic

8e54d46

thomasneirynck approved these changes Jul 9, 2020

View reviewed changes

Merge branch 'master' into safer-lodash-set

34316df

stephmilovic approved these changes Jul 9, 2020

View reviewed changes

Whitelist files for casing

a33c7ed

Merge branch 'master' into safer-lodash-set

93ff996

Merge branch 'master' into safer-lodash-set

7dd9e3e

gtback approved these changes Jul 13, 2020

View reviewed changes

gtback requested review from tomcallahan and carlspataro July 13, 2020 15:09

azasypkin added 4 commits July 14, 2020 17:50

Merge branch 'master' into pr-67452-safer

fb7c63d

Disable ESLint var warning to be closer to the lodash source code.

41c6acf

Merge branch 'master' into pr-67452-safer

daf8eb0

Merge branch 'master' into pr-67452-safer

9b99a23

elasticmachine and others added 2 commits July 14, 2020 23:53

Merge branch 'master' into safer-lodash-set

415fdd2

Update yarn.lock.

1b4d708

azasypkin merged commit fc5bc6b into elastic:master Jul 15, 2020

azasypkin mentioned this pull request Jul 15, 2020

[7.x] Add @elastic/safer-lodash-set as an alternative to lodash.set (#67452) #71823

Merged

azasypkin added the backported label Jul 15, 2020

watson deleted the safer-lodash-set branch August 5, 2020 09:08

legrego mentioned this pull request Aug 26, 2020

[DISCUSS] lodash get / set should be avoided, we should introduce an eslint rule #75915

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add @elastic/safer-lodash-set as an alternative to lodash.set #67452

Add @elastic/safer-lodash-set as an alternative to lodash.set #67452

watson commented May 27, 2020 •

edited

Loading

elasticmachine commented May 27, 2020

watson commented May 27, 2020 •

edited

Loading

watson commented May 27, 2020 •

edited

Loading

elasticmachine commented May 27, 2020

legrego left a comment

watson commented May 28, 2020

legrego commented May 28, 2020

FrankHassanabad left a comment

thomasneirynck left a comment

stephmilovic left a comment

watson commented Jul 9, 2020

watson commented Jul 10, 2020

gtback left a comment •

edited

Loading

azasypkin commented Jul 15, 2020

azasypkin commented Jul 15, 2020

kibanamachine commented Jul 15, 2020

azasypkin commented Jul 15, 2020 •

edited

Loading

azasypkin commented Jul 15, 2020

watson commented Aug 5, 2020

Add @elastic/safer-lodash-set as an alternative to lodash.set #67452

Add @elastic/safer-lodash-set as an alternative to lodash.set #67452

Conversation

watson commented May 27, 2020 • edited Loading

Todo

elasticmachine commented May 27, 2020

watson commented May 27, 2020 • edited Loading

watson commented May 27, 2020 • edited Loading

elasticmachine commented May 27, 2020

legrego left a comment

Choose a reason for hiding this comment

watson commented May 28, 2020

legrego commented May 28, 2020

FrankHassanabad left a comment

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

stephmilovic left a comment

Choose a reason for hiding this comment

watson commented Jul 9, 2020

watson commented Jul 10, 2020

gtback left a comment • edited Loading

Choose a reason for hiding this comment

azasypkin commented Jul 15, 2020

azasypkin commented Jul 15, 2020

kibanamachine commented Jul 15, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

miscellaneous assets size

page load bundle size

History

azasypkin commented Jul 15, 2020 • edited Loading

azasypkin commented Jul 15, 2020

watson commented Aug 5, 2020

watson commented May 27, 2020 •

edited

Loading

watson commented May 27, 2020 •

edited

Loading

watson commented May 27, 2020 •

edited

Loading

gtback left a comment •

edited

Loading

azasypkin commented Jul 15, 2020 •

edited

Loading