Skip to content

Commit

Permalink
fix: return boolean from safeStorage.isEncryptionAvailable() instea…
Browse files Browse the repository at this point in the history
…d of crashing

On Linux, `isEncryptionAvailable()` was crashing instead of returning a
boolean before the 'ready' event was emitted by the app. The reason of
the crash is that [`CreateKeyStorage()`](https://source.chromium.org/chromium/chromium/src/+/main:components/os_crypt/os_crypt_linux.cc;l=74;drc=35be6215ec8f09e50176f36753c68f26c63d1885;bpv=1;bpt=0)
expects the config to be set but the function responsible for setting the
config, [`SetConfig()`](https://source.chromium.org/chromium/chromium/src/+/main:components/os_crypt/os_crypt_linux.cc;l=237;drc=35be6215ec8f09e50176f36753c68f26c63d1885;bpv=1;bpt=0),
is called only after the app is ready inside [`PostCreateMainMessageLoop()`](https://github.com/electron/electron/blob/main/shell/browser/electron_browser_main_parts.cc#L499).
So this change patches [`IsEncryptionAvailable()`](https://source.chromium.org/chromium/chromium/src/+/main:components/os_crypt/os_crypt_linux.cc;l=245;drc=35be6215ec8f09e50176f36753c68f26c63d1885;bpv=1;bpt=0)
to return false if the config hasn't been set already.

Fixes: #32206
Signed-off-by: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
RaisinTen committed Apr 25, 2022
1 parent f3e0517 commit 656f235
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 3 deletions.
4 changes: 2 additions & 2 deletions docs/api/safe-storage.md
Expand Up @@ -18,8 +18,8 @@ The `safeStorage` module has the following methods:

Returns `boolean` - Whether encryption is available.

On Linux, returns true if the secret key is
available. On MacOS, returns true if Keychain is available.
On Linux, returns true if the app has emitted the `ready` event and the secret key is available.
On MacOS, returns true if Keychain is available.
On Windows, returns true once the app has emitted the `ready` event.

### `safeStorage.encryptString(plainText)`
Expand Down
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -108,3 +108,4 @@ fix_non-client_mouse_tracking_and_message_bubbling_on_windows.patch
build_make_libcxx_abi_unstable_false_for_electron.patch
introduce_ozoneplatform_electron_can_call_x11_property.patch
make_gtk_getlibgtk_public.patch
fix_return_boolean_from_safestorage_isencryptionavailable_instead_of_crashing.patch
@@ -0,0 +1,30 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Darshan Sen <raisinten@gmail.com>
Date: Mon, 25 Apr 2022 17:03:34 +0530
Subject: fix: return boolean from `safeStorage.isEncryptionAvailable()` instead of crashing

On Linux, `isEncryptionAvailable()` was crashing instead of returning a
boolean before the 'ready' event was emitted by the app. The reason of
the crash is that [`CreateKeyStorage()`](https://source.chromium.org/chromium/chromium/src/+/main:components/os_crypt/os_crypt_linux.cc;l=74;drc=35be6215ec8f09e50176f36753c68f26c63d1885;bpv=1;bpt=0)
expects the config to be set but the function responsible for setting the
config, [`SetConfig()`](https://source.chromium.org/chromium/chromium/src/+/main:components/os_crypt/os_crypt_linux.cc;l=237;drc=35be6215ec8f09e50176f36753c68f26c63d1885;bpv=1;bpt=0),
is called only after the app is ready inside [`PostCreateMainMessageLoop()`](https://github.com/electron/electron/blob/main/shell/browser/electron_browser_main_parts.cc#L499).
So this change patches [`IsEncryptionAvailable()`](https://source.chromium.org/chromium/chromium/src/+/main:components/os_crypt/os_crypt_linux.cc;l=245;drc=35be6215ec8f09e50176f36753c68f26c63d1885;bpv=1;bpt=0)
to return false if the config hasn't been set already.

Fixes: https://github.com/electron/electron/issues/32206
Signed-off-by: Darshan Sen <raisinten@gmail.com>

diff --git a/os_crypt/os_crypt_linux.cc b/os_crypt/os_crypt_linux.cc
index 07da4e55f1788..2781d060bbe1d 100644
--- a/os_crypt/os_crypt_linux.cc
+++ b/os_crypt/os_crypt_linux.cc
@@ -242,7 +242,7 @@ void OSCrypt::SetConfig(std::unique_ptr<os_crypt::Config> config) {

// static
bool OSCrypt::IsEncryptionAvailable() {
- return g_get_password[Version::V11]();
+ return g_cache.Get().config && g_get_password[Version::V11]();
}

// static
19 changes: 18 additions & 1 deletion spec-main/api-safe-storage-spec.ts
@@ -1,6 +1,6 @@
import * as cp from 'child_process';
import * as path from 'path';
import { safeStorage } from 'electron/main';
import { app, safeStorage } from 'electron/main';
import { expect } from 'chai';
import { emittedOnce } from './events-helpers';
import { ifdescribe } from './spec-helpers';
Expand All @@ -12,8 +12,25 @@ import * as fs from 'fs';
*
* Because all encryption methods are gated by isEncryptionAvailable, the methods will never return the correct values
* when run on CI and linux.
* Refs: https://github.com/electron/electron/issues/30424.
*/

describe('SafeStorage.isEncryptionAvailable() before and after app is ready', () => {
it('isEncryptionAvailable() returns false before app is ready', async () => {
if (!app.isReady()) {
// isEncryptionAvailable() returns false before the app is ready on
// Linux: https://github.com/electron/electron/issues/32206
// and
// Windows: https://github.com/electron/electron/issues/33640.
expect(safeStorage.isEncryptionAvailable()).to.equal(process.platform === 'darwin');
}
await app.whenReady();
// isEncryptionAvailable() will always return false on CI due to a mocked
// dbus as mentioned above.
expect(safeStorage.isEncryptionAvailable()).to.equal(process.platform !== 'linux');
});
});

ifdescribe(process.platform !== 'linux')('safeStorage module', () => {
after(async () => {
const pathToEncryptedString = path.resolve(__dirname, 'fixtures', 'api', 'safe-storage', 'encrypted.txt');
Expand Down

0 comments on commit 656f235

Please sign in to comment.