Ensure process exits if a process warning is emitted by watson · Pull Request #59651 · elastic/kibana

watson · 2020-03-09T13:36:46Z

What

Crash Kibana in dev/CI if a process warning is detected. This does not influence how Kibana behaves in production when run via ./bin/kibana as there the --no-warnings flag is used. We will detect this flag and as a result, disable this behavior.

Why

Because it's way too easy to miss a process warning being printed in the console. Less warnings => Better code => More secure code.

Details

Previously we used the flags --trace-warnings --throw-deprecation when you started Kibana via yarn start and we used --throw-deprecation in CI. This meant that we would only crash on deprecation warnings and log a stack trace for all other types of warnings.

There were a couple of drawbacks to this approach:

If the deprecated API was called in a place enclosed in a try-catch (or inside of a Promise or an async/await context), the throw would be caught and the program would not crash.
If you ran ./scripts/kibana directly instead of running yarn start, you would not get these flags automatically.
If you ran any of our tests locally you would not get the standard CI flags. This meant something that might seem to pass locally would not pass in CI.

This PR changes this behavior by ensuring:

That we always crash - no matter if the offending code is surrounded by a try-catch (etc).
That you always get the same behavior whether you run yarn start or ./scripts/kibana.
That you always get the same behavior in CI or if you run individual tests locally.

Furthermore, we now crash for all types of warnings - not only deprecation warnings (except MaxListenersExceededWarning, though that's just because I didn't want to spend too much time getting that to work).

Closes #59646

elasticmachine · 2020-03-09T13:36:48Z

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

watson · 2020-03-10T14:46:08Z

src/dev/exit_on_warning.js

+ * under the License.
+ */
+
+const ignore = ['MaxListenersExceededWarning'];


I'm ignoring MaxListenersExceededWarning because one of our tests (src/cli/cluster/cluster_manager.test.ts) emitted this warning and I don't consider these a security risk.

We could consider "fixing" the test so it doesn't emit this warning, but it's not super important. However, if we did, it would probably improve the codebase as new warnings of this type would not sneak in as those are usually a result of poorly written code that can hide other bugs. So if it's easy to do we should consider it (I spent a few hours trying to fix it without any luck).

kibanamachine · 2020-03-10T18:56:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7f7d69d

History

💚 Build #32048 succeeded 83702d8
💔 Build #31867 failed 7ce6cb470224031c718d109dbf66ecf5a065c638
💔 Build #31838 failed 2a4d6100ae01e26013205d97f44751eb677ac36a
💔 Build #31829 failed be397e3292b9b59621ffbce535c572455709c48a
💔 Build #31739 failed 04cfdf9ee9355846e07aff29394aa6d3ef4f2506

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

spalger

LGTM, Very excited about getting these benefits when using node scripts/kibana

src/setup_node_env/index.js

mistic

LGTM

legrego · 2020-03-11T18:30:52Z

src/setup_node_env/exit_on_warning.js

+if (process.noProcessWarnings !== true) {
+  var ignore = ['MaxListenersExceededWarning'];
+
+  process.on('warning', function(warn) {


If I'm understanding correctly, we were previously crashing on any deprecation warnings, but now, we will crash on all warnings except MaxListenersExceededWarning (including any custom warnings emitted by dependencies or our own code). Is this intended?

Previously we used the flags --trace-warnings --throw-deprecation when you started Kibana via yarn start and we used --throw-deprecation in CI. This meant that we would only crash on deprecation warnings and log a stack trace for all other types of warnings (MaxListenersExceededWarning belongs to the latter group).

There were a couple of drawbacks to this approach:

If the deprecated API was called in a place enclosed in a try-catch (or inside of a Promise or an async/await context), the throw would be caught and the program would not crash.

If you ran ./scripts/kibana directly instead of running yarn start, you would not get these flags automatically.

If you ran any of our tests locally you would not get the standard CI flags. This meant something that might seem to pass locally would not pass in CI.

This PR changes this behavior by ensuring:

Crash for all types of warnings - not only deprecation warnings (except MaxListenersExceededWarning, though that's just because I didn't want to spend too much time getting that to work).

That we always crash - no matter if the offending code is surrounded by a try-catch (etc).

That you always get the same behavior whether you run yarn start or ./scripts/kibana.

That you always get the same behavior in CI or if you run individual tests locally.

Update: I'll add this to the PR description

Thanks for explaining, that makes sense to me now

legrego

tested locally, LGTM!

Crash Kibana in dev/CI if a process warning is detected. This does not influence how Kibana behaves in production when run via `./bin/kibana` as there the `--no-warnings` flag is used. We will detect this flag and as a result, disable this behavior. Previously we used the flags `--trace-warnings --throw-deprecation` when you started Kibana via `yarn start` and we used `--throw-deprecation` in CI. This meant that we would only crash on deprecation warnings and log a stack trace for all other types of warnings. There were a couple of drawbacks to this approach: 1. If the deprecated API was called in a place enclosed in a try-catch (or inside of a Promise or an async/await context), the throw would be caught and the program would not crash. 2. If you ran `./scripts/kibana` directly instead of running `yarn start`, you would not get these flags automatically. 3. If you ran any of our tests locally you would not get the standard CI flags. This meant something that might seem to pass locally would not pass in CI. This commit changes this behavior by ensuring: - That we always crash - no matter if the offending code is surrounded by a try-catch (etc). - That you always get the same behavior whether you run `yarn start` or `./scripts/kibana`. - That you always get the same behavior in CI or if you run individual tests locally. Furthermore, we now crash for all types of warnings - not only deprecation warnings (except `MaxListenersExceededWarning`). Closes elastic#59646

Crash Kibana in dev/CI if a process warning is detected. This does not influence how Kibana behaves in production when run via `./bin/kibana` as there the `--no-warnings` flag is used. We will detect this flag and as a result, disable this behavior. Previously we used the flags `--trace-warnings --throw-deprecation` when you started Kibana via `yarn start` and we used `--throw-deprecation` in CI. This meant that we would only crash on deprecation warnings and log a stack trace for all other types of warnings. There were a couple of drawbacks to this approach: 1. If the deprecated API was called in a place enclosed in a try-catch (or inside of a Promise or an async/await context), the throw would be caught and the program would not crash. 2. If you ran `./scripts/kibana` directly instead of running `yarn start`, you would not get these flags automatically. 3. If you ran any of our tests locally you would not get the standard CI flags. This meant something that might seem to pass locally would not pass in CI. This commit changes this behavior by ensuring: - That we always crash - no matter if the offending code is surrounded by a try-catch (etc). - That you always get the same behavior whether you run `yarn start` or `./scripts/kibana`. - That you always get the same behavior in CI or if you run individual tests locally. Furthermore, we now crash for all types of warnings - not only deprecation warnings (except `MaxListenersExceededWarning`). Closes #59646

watson added Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 Feature:Hardening Harding of Kibana from a security perspective labels Mar 9, 2020

watson self-assigned this Mar 9, 2020

watson force-pushed the more-process-warnings branch from 2a4d610 to 7ce6cb4 Compare March 9, 2020 18:22

watson added 4 commits March 10, 2020 13:40

Ensure process exits if a process warning is emitted

0fcd1fb

Also run during CI

e8a1e38

use full path

ae065c1

Ignore MaxListenersExceededWarning

83702d8

watson force-pushed the more-process-warnings branch from 9cc5423 to 83702d8 Compare March 10, 2020 12:41

watson commented Mar 10, 2020

View reviewed changes

watson added 2 commits March 10, 2020 18:04

Ensure process exits on warning in all envs except prod

50c09f5

process.noProcessWarnings is never false

7f7d69d

watson marked this pull request as ready for review March 10, 2020 21:08

watson requested a review from a team as a code owner March 10, 2020 21:08

watson requested a review from a team March 10, 2020 21:09

spalger approved these changes Mar 10, 2020

View reviewed changes

jbudz reviewed Mar 10, 2020

View reviewed changes

src/setup_node_env/index.js Show resolved Hide resolved

mistic approved these changes Mar 11, 2020

View reviewed changes

legrego reviewed Mar 11, 2020

View reviewed changes

legrego approved these changes Mar 11, 2020

View reviewed changes

watson merged commit ec78348 into elastic:master Mar 12, 2020

watson deleted the more-process-warnings branch March 12, 2020 06:21

watson mentioned this pull request Mar 12, 2020

[7.x] Ensure process exits if a process warning is emitted (#59651) #59980

Merged

lukeelmers mentioned this pull request Feb 4, 2021

[core.logging] Fix issue where logs fail to calculate size of gunzip streams. #90353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure process exits if a process warning is emitted#59651

Ensure process exits if a process warning is emitted#59651
watson merged 6 commits intoelastic:masterfrom
watson:more-process-warnings

watson commented Mar 9, 2020 •

edited

Loading

Uh oh!

elasticmachine commented Mar 9, 2020

Uh oh!

watson Mar 10, 2020

Uh oh!

kibanamachine commented Mar 10, 2020

Uh oh!

spalger left a comment •

edited

Loading

Uh oh!

Uh oh!

mistic left a comment

Uh oh!

legrego Mar 11, 2020

Uh oh!

watson Mar 11, 2020 •

edited

Loading

Uh oh!

legrego Mar 11, 2020

Uh oh!

legrego left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

watson commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Details

Uh oh!

elasticmachine commented Mar 9, 2020

Uh oh!

watson Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 10, 2020

💚 Build Succeeded

History

Uh oh!

spalger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mistic left a comment

Choose a reason for hiding this comment

Uh oh!

legrego Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

watson Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legrego Mar 11, 2020

Choose a reason for hiding this comment

Uh oh!

legrego left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

watson commented Mar 9, 2020 •

edited

Loading

spalger left a comment •

edited

Loading

watson Mar 11, 2020 •

edited

Loading