feat: add electron.safeStorage encryption API
#30020
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
electron.safeStorage cookie encryption APIelectron.safeStorage encryption API
MarshallOfSound
requested changes
Jul 5, 2021
electron.safeStorage encryption APIelectron.safeStorage encryption API
8c868f9 to
7a86391
Compare
nornagon
suggested changes
Jul 9, 2021
|
|
||
| std::string SafeStorage::DecryptString(v8::Isolate* isolate, | ||
| v8::Local<v8::Value> buffer) { | ||
| if (!node::Buffer::HasInstance(buffer)) { |
There was a problem hiding this comment.
We should accept any Uint8Array here, and not require Buffer (which is a subclass of Uint8Array).
There was a problem hiding this comment.
I think Buffer is the better move here, as you note Buffer is a loose wrapper around Uint8Array, in fact you can do zero-cost conversions into a Buffer class or just pass a Uint8Array to anything that supports a node::Buffer.
Node documents things as taking Buffer's even if they also support Uint8Array's.
There was a problem hiding this comment.
node::Buffer::HasInstance will return false for a Uint8Array though?
There was a problem hiding this comment.
@MarshallOfSound @nornagon Sorry to gravedig this one, just wondering if there was any resolution to this conversation/ and if there were any strong feelings against keeping Buffer as the input? 🙇
There was a problem hiding this comment.
I think for consistency we should use buffer, we don't have any other apis that take uint8array but we do work with buffers a bunch.
There was a problem hiding this comment.
I still think we should accept Uint8Array even if we document this as accepting Buffer. https://en.wikipedia.org/wiki/Robustness_principle
I won't block the PR on it though.
e72bd7c to
fdd4b9e
Compare
4fe1c3a to
cb6eb32
Compare
e34ac71 to
67fd1fd
Compare
|
|
||
| // ensures an error is thrown in Mac or Linux on | ||
| // decryption failure, rather than failing silently | ||
| std::vector<std::string> kEncryptionVersionPrefix{"v10", "v11"}; |
There was a problem hiding this comment.
This could be defined outside of this method, preferably as two unique char* so that we aren't statically allocating strings or vectors
| gin_helper::ErrorThrower(isolate).ThrowError( | ||
| "Error while decrypting the ciphertext provided to " | ||
| "safeStorage.decryptString. " | ||
| "Ciphertext may not be encrypted."); |
There was a problem hiding this comment.
Suggested change
| "Ciphertext may not be encrypted."); | |
| "Ciphertext does not appear to be encrypted."); |
557c4dc to
59f2c7a
Compare
Vikeeyy
approved these changes
Aug 4, 2021
nornagon
reviewed
Aug 5, 2021
| #ifndef SHELL_BROWSER_API_ELECTRON_API_SAFE_STORAGE_H_ | ||
| #define SHELL_BROWSER_API_ELECTRON_API_SAFE_STORAGE_H_ | ||
|
|
||
| #include <string> |
There was a problem hiding this comment.
Suggested change
| #include <string> |
Comment on lines
+9
to
+14
| /* isEncryptionAvailable returns false in Linux when running CI due to a mocked dbus. This stops | ||
| * Chrome from reaching the system's keyring or libsecret. When running the tests with config.store | ||
| * set to basic-text, a nullptr is returned from chromium, defaulting the available encryption to false. | ||
| * | ||
| * Because all encryption methods are gated by isEncryptionAvailable, the methods will never return the correct values | ||
| * when run on CI and linux. |
There was a problem hiding this comment.
This is a shame, it would be a nice followup to have the keyring stuff mocked out in dbus in a way that would allow the tests to run. But I think it's OK to ship without this.
There was a problem hiding this comment.
created a follow-up issue so this doesn't get lost :)
spec-main/api-safe-storage-spec.ts
Outdated
|
|
||
| ifdescribe(process.platform !== 'linux')('safeStorage module', () => { | ||
| after(async () => { | ||
| if (process.platform === 'linux') return; |
There was a problem hiding this comment.
handled by ifdescribe above.
Suggested change
| if (process.platform === 'linux') return; |
spec-main/api-safe-storage-spec.ts
Outdated
| encryptAppProcess.stderr.on('data', data => { stdout += data; }); | ||
|
|
||
| // this setTimeout is left here for QOL in case we need to debug this test in the future | ||
| setTimeout(() => { console.log(stdout); }, 3000); |
There was a problem hiding this comment.
Hm, this setTimeout is a bit weird... it might make the stdout look like it was coming from a different test. Maybe instead something like:
try {
// ... rest of the test ...
} catch (e) {
console.log(stdout)
throw e
}
?
c1cd352 to
eaf33c4
Compare
3 tasks
nornagon
approved these changes
Aug 5, 2021
|
Linux asan failure is fixed in main. |
|
Release Notes Persisted
|
|
I was unable to backport this PR to "14-x-y" cleanly; |
trop bot
pushed a commit
that referenced
this pull request
Aug 5, 2021
* feat: add SafeStorage api; first commit * chore: rename files to fit semantically * chore: add linkedBindings * chore: fix function signatures * chore: refactor eisCookieEncryptionEnabled() fuse * chore: create test file * chore: add tests and documentation * chore: add copyright and lint * chore: add additional tests * chore: fix constructor * chore: commit for pair programming * wip: commit for keeley pairing * chore: docs change and code cleanup * chore: add linux import * chore: add description to documentation * chore: fixing tests * chore: modify behaviour to not allow unencrypted strings as decyption input * fix add patch for enabling default v11 encryption on Linux * chore: remove file after each test * chore: fix patch * chore: remove chromium patch * chore: add linux specific tests * chore: fix path * chore: add checker for linuux file deletion * chore: add dcheck back * chore: remove reference to headless mode * chore: remove tests for linux * chore: edit commit message * chore: refactor safeStorage to not be a class * chore: remove static variable from header * chore: spec file remove settimeout Co-authored-by: VerteDinde <keeleymhammond@gmail.com>
|
I have automatically backported this PR to "15-x-y", please check out #30430 |
3 tasks
VerteDinde
added a commit
that referenced
this pull request
Aug 12, 2021
* feat: add SafeStorage api; first commit * chore: rename files to fit semantically * chore: add linkedBindings * chore: fix function signatures * chore: refactor eisCookieEncryptionEnabled() fuse * chore: create test file * chore: add tests and documentation * chore: add copyright and lint * chore: add additional tests * chore: fix constructor * chore: commit for pair programming * wip: commit for keeley pairing * chore: docs change and code cleanup * chore: add linux import * chore: add description to documentation * chore: fixing tests * chore: modify behaviour to not allow unencrypted strings as decyption input * fix add patch for enabling default v11 encryption on Linux * chore: remove file after each test * chore: fix patch * chore: remove chromium patch * chore: add linux specific tests * chore: fix path * chore: add checker for linuux file deletion * chore: add dcheck back * chore: remove reference to headless mode * chore: remove tests for linux * chore: edit commit message * chore: refactor safeStorage to not be a class * chore: remove static variable from header * chore: spec file remove settimeout Co-authored-by: VerteDinde <keeleymhammond@gmail.com>
VerteDinde
added a commit
that referenced
this pull request
Aug 23, 2021
* feat: add SafeStorage api; first commit * chore: rename files to fit semantically * chore: add linkedBindings * chore: fix function signatures * chore: refactor eisCookieEncryptionEnabled() fuse * chore: create test file * chore: add tests and documentation * chore: add copyright and lint * chore: add additional tests * chore: fix constructor * chore: commit for pair programming * wip: commit for keeley pairing * chore: docs change and code cleanup * chore: add linux import * chore: add description to documentation * chore: fixing tests * chore: modify behaviour to not allow unencrypted strings as decyption input * fix add patch for enabling default v11 encryption on Linux * chore: remove file after each test * chore: fix patch * chore: remove chromium patch * chore: add linux specific tests * chore: fix path * chore: add checker for linuux file deletion * chore: add dcheck back * chore: remove reference to headless mode * chore: remove tests for linux * chore: edit commit message * chore: refactor safeStorage to not be a class * chore: remove static variable from header * chore: spec file remove settimeout Co-authored-by: VerteDinde <keeleymhammond@gmail.com> Co-authored-by: George Xu <33054982+georgexu99@users.noreply.github.com> Co-authored-by: VerteDinde <keeleymhammond@gmail.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
BlackHole1
pushed a commit
to BlackHole1/electron
that referenced
this pull request
Aug 30, 2021
* feat: add SafeStorage api; first commit * chore: rename files to fit semantically * chore: add linkedBindings * chore: fix function signatures * chore: refactor eisCookieEncryptionEnabled() fuse * chore: create test file * chore: add tests and documentation * chore: add copyright and lint * chore: add additional tests * chore: fix constructor * chore: commit for pair programming * wip: commit for keeley pairing * chore: docs change and code cleanup * chore: add linux import * chore: add description to documentation * chore: fixing tests * chore: modify behaviour to not allow unencrypted strings as decyption input * fix add patch for enabling default v11 encryption on Linux * chore: remove file after each test * chore: fix patch * chore: remove chromium patch * chore: add linux specific tests * chore: fix path * chore: add checker for linuux file deletion * chore: add dcheck back * chore: remove reference to headless mode * chore: remove tests for linux * chore: edit commit message * chore: refactor safeStorage to not be a class * chore: remove static variable from header * chore: spec file remove settimeout Co-authored-by: VerteDinde <keeleymhammond@gmail.com>
Ovenoboyo
added a commit
to Moosync/Moosync-electron
that referenced
this pull request
Oct 11, 2021
|
Will i.e. Do alternate security measures need anticipating just at startup or at anytime? |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Change
add a cookie encryption api to allow users to encrypt, decrypt strings and check whether encryption is available.
we recently added experimental cookie encryption support #27524. This API exposes three methods from Chromium, but only when os_crypto is available.
Checklist
npm testpassesRelease Notes
Notes: add
safeStoragestring encryption API