search

doyensec / electronegativity

Electronegativity is a tool to identify misconfigurations and security anti-patterns in Electron applications.
Apache License 2.0
970 stars 66 forks source link

Bugfix: Don't fail in version check if Electron version is unknown #65

Closed baltpeter closed 4 years ago

baltpeter commented 4 years ago

The AvailableSecurityFixesGlobalCheck was failing if it encountered an unknown Electron release. The bug is in this code:

https://github.com/doyensec/electronegativity/blob/7cfc8d3e12cb6f70deb1c26c56b1ef15aaa9e4a2/src/finder/checks/GlobalChecks/AvailableSecurityFixesGlobalCheck.js#L58-L60

This assumes that we can always find a latestRelease, which isn't necessarily the case leading to an unhandled promise rejection.

As an example of this bug:

  1. git clone https://github.com/zadam/trilium.git
  2. electronegativity -i trilium

Instead of producing a result, Electronegativity fails with the following error:

Fetching Electron's new releases, this may take a while...
Updated releases list to v8.2.5!
(node:956086) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'version' of undefined
    at AvailableSecurityFixesGlobalCheck.checkSecurityFixes (/home/benni/coding/uni/electronegativity/dist/finder/checks/GlobalChecks/AvailableSecurityFixesGlobalCheck.js:122:64)
    at AvailableSecurityFixesGlobalCheck.perform (/home/benni/coding/uni/electronegativity/dist/finder/checks/GlobalChecks/AvailableSecurityFixesGlobalCheck.js:86:56)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async GlobalChecks.getResults (/home/benni/coding/uni/electronegativity/dist/finder/globalchecks.js:119:20)
    at async run (/home/benni/coding/uni/electronegativity/dist/runner.js:217:12)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:956086) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 27)
(node:956086) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

This PR fixes that bug by only executing the remaining check code if we did in fact find a latestRelease. Otherwise, we inform the user and report available security fixes with a tentative confidence.
To do so, the checkSecurityFixes() function now returns a confidence instead of true in case of potential available fixes.

I was thinking of reporting an error in this cases but if I understand your code correctly, those are only meant for parser errors. And if we cannot update the releases list, we only fail with a warning as well, so this approach seems reasonable to me. Let me know, if you prefer a different apporach, though.

phosphore commented 4 years ago

Good catch! I think that the best approach here is just issuing a warning hinting for manual review, just as you designed. I'll merge this to master now, but it will ship with v1.6.1 as soon as we fix something major like #62. Thanks again for the PR :)