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

Problem with outdated lodash isFunction #63239

Closed
timroes opened this issue Apr 10, 2020 · 8 comments
Closed

Problem with outdated lodash isFunction #63239

timroes opened this issue Apr 10, 2020 · 8 comments
Labels
Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team

Comments

@timroes
Copy link
Contributor

timroes commented Apr 10, 2020

@sulemanof pointed out an interesting issue we have with our current outdated Lodash version, that will appear when moving over more applications to Kibana Platform. We currently use a fork of lodash, whose isFunction implementation (and we seem to use this in around 100 places around Kibana), is checking the type against the string [object Function]. This worked fine as long as we're transpiling the code. Since code in Kibana Platform will be executed with less transpiling during development, we have seen cases while moving TSVB to KP, where suddenly code breaks, because we checked async functions with _.isFunction, who are stringifying to [object AsyncFunction].

I am not sure if _.isFunction is the only problematic function in our outdated lodash, but since those errors can sometimes be really tricky to find or during moving to KP not appear at all and create buggy releases, I think we should consider: Updating Lodash (at least for isFunction) in our fork, and maybe to an evaluation if any of the other functions we're using could be effected by the switch in compilation.

The newer lodash version doesn't have this problems anymore, because it now also checks for stringified versions of async functions and Proxies.

@timroes timroes added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team Feature:New Platform labels Apr 10, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@sulemanof sulemanof mentioned this issue Apr 13, 2020
7 tasks
@pgayvallet
Copy link
Contributor

@kobelb / @elastic/kibana-security I'm assuming we forked lodash for security reasons regarding prototype pollution?

We are currently at 3.10.1 for our https://github.com/elastic/lodash fork and the origin repo is at 4.17.15

  • Are the security concerns still not addressed in the root repo / is there still a reason to maintain our fork ?
  • I see mostly commits from @kobelb. Which team is responsible to maintain this fork? @elastic/kibana-operations, @elastic/kibana-security or @elastic/kibana-platform ?
  • What is the upstream reintegration/merge policy on our fork?

@kobelb
Copy link
Contributor

kobelb commented Apr 13, 2020

@kobelb / @elastic/kibana-security I'm assuming we forked lodash for security reasons regarding prototype pollution?

That is correct.

Are the security concerns still not addressed in the root repo / is there still a reason to maintain our fork ?

As soon as we can upgrade lodash, there's no reason to maintain the fork. It was intended to be a short-term stopgap.

I see mostly commits from @kobelb. Which team is responsible to maintain this fork? @elastic/kibana-operations, @elastic/kibana-security or @elastic/kibana-platform ?

Since we've only been using the fork to mitigate security vulnerabilities and I think we should keep it this way, the @elastic/kibana-security should own this.

What is the upstream reintegration/merge policy on our fork?

There isn't one at the moment, it was meant to be a temporary solution which never was merged upstream.

This is also being tracked in #62078

@pgayvallet
Copy link
Contributor

As soon as we can upgrade lodash, there's no reason to maintain the fork. It was intended to be a short-term stopgap.

So the correct solution is 'just' to switch back to an up to date version of upstream then.

This is also being tracked in #62078

You mean #7537? This seems to be in operations backlog for 7.9

@kobelb
Copy link
Contributor

kobelb commented Apr 14, 2020

So the correct solution is 'just' to switch back to an up to date version of upstream then.

Correct.

You mean #7537? This seems to be in operations backlog for 7.9

Yup... my copy/paste failed me. Apologies.

@mistic
Copy link
Member

mistic commented Jul 3, 2020

@pgayvallet @timroes FYI we have removed lodash@3 from our repo and upgraded everything to lodash@4

@pgayvallet
Copy link
Contributor

@timroes I guess this can be closed then?

@timroes timroes closed this as completed Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team
Projects
None yet
Development

No branches or pull requests

5 participants