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

Upgrade Joi version or remove dependency on it #84624

Closed
mshustov opened this issue Dec 1, 2020 · 8 comments · Fixed by #99899
Closed

Upgrade Joi version or remove dependency on it #84624

mshustov opened this issue Dec 1, 2020 · 8 comments · Fixed by #99899
Labels
Feature:Dependencies Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Dec 1, 2020

@watson update Hapi to v18 in #54168 However, the core team code still relies on an old joi version. We should consider updating our code to use a newer version or get rid of the package.

@mshustov mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Dependencies labels Dec 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@legrego
Copy link
Member

legrego commented May 12, 2021

Our old 13.x version of Joi depends on a version of hoek that is now vulnerable to a prototype pollution attack. Fortunately, we are not impacted, but this appears on our scanners now, and is something we'll have to continue to monitor: snyk.io/vuln/SNYK-JS-HAPIHOEK-548452

The upgrade to newer versions of Joi is non-trivial. There are a lot of breaking changes from 13.x -> 17.x, and my very brief look into this suggests that we'll require extensive changes to @kbn/config-schema in order to function correctly. The bulk of the breaking changes appear to have happened at 16.0: the release notes indicate that this will not be an easy transition.

@afharo
Copy link
Member

afharo commented May 13, 2021

AFAIK, @pgayvallet has almost completed the upgrade.

Apart from the security benefits mentioned by @legrego, we also noticed considerable improvements in performance (gcanti/io-ts-benchmarks#6)

@pgayvallet
Copy link
Contributor

Added link to the upgrade PR: #99899

@watson
Copy link
Contributor

watson commented May 19, 2021

Besides joi 13 there's a few other dependencies that depend on hoek. I've just created two PRs that should get rid of them all (besides joi which is dealt with in #99899). With these we'll be 100% free of any non-@hapi-namespace packages from the hapi ecosystem.

@watson
Copy link
Contributor

watson commented May 19, 2021

Btw, in case anybody needs this in the future, I have been using a regex to find non-@hapi-namespaces packages from the hapi package ecosystem:

(yar|wreck|vision|vise|topo|teamwork|subtext|statehood|somever|shot|scooter|podium|pez|oppsy|nigel|nes|mimos|lab|iron|inert|hoek|heavy|h2o2|glue|eslint-plugin-hapi|cryptiles|crumb|hapi-auth-cookie|content|code|catbox-redis|catbox-memory|catbox-memcached|catbox|call|bourne|bounce|bossy|boom|bell|hapi-auth-basic|b64|ammo|accept|hapi)

For searching in yarn.lock it can be modified as such:

^(yar|wreck|vision|vise|topo|teamwork|subtext|statehood|somever|shot|scooter|podium|pez|oppsy|nigel|nes|mimos|lab|iron|inert|hoek|heavy|h2o2|glue|eslint-plugin-hapi|cryptiles|crumb|hapi-auth-cookie|content|code|catbox-redis|catbox-memory|catbox-memcached|catbox|call|bourne|bounce|bossy|boom|bell|hapi-auth-basic|b64|ammo|accept|hapi)@

@pgayvallet
Copy link
Contributor

Besides joi 13 there's a few other dependencies that depend on hoek

I can confirm that after rebasing my PR, hoek got removed from the package-lock, so I guess we're good now.

@legrego
Copy link
Member

legrego commented May 20, 2021

Fantastic, thanks @pgayvallet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dependencies Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants