search

laurent22 / joplin

Joplin - the secure note taking and to-do app with synchronisation capabilities for Windows, macOS, Linux, Android and iOS.
https://joplinapp.org
Other
44.35k stars 4.79k forks source link

Plugins: settings fields that are secure are unable to be set on keychain #10526

Open alondmnt opened 1 month ago

alondmnt commented 1 month ago

Operating system

Windows

Joplin version

2.14.22

Desktop version info

Joplin 2.14.22 (prod, win32) Client ID: 19f20674be77445bbd5535190049d93c Sync Version: 3 Profile Version: 46 Keychain Supported: Yes Revision: e579eb9

Current behaviour

This was first reported by @djsudduth in this Jarvis plugin issue.

  1. The plugin tries to set API keys (secure: true fields) into the ketchain service.
  2. In the development tools console the following exception appears:

models/Setting: Could not set setting on the keychain. Will be saved to database instead: plugin-joplin.plugin.alondmnt.jarvis.springer_api_key: Error: Password is required.

Setting secure: false resolves the error.

Expected behaviour

No error.

Logs

models/Setting: Could not set setting on the keychain. Will be saved to database instead: plugin-joplin.plugin.alondmnt.jarvis.hf_api_key: Error: Password is required. at checkRequired (C:\Users\myusername\AppD…\lib\keytar.js:5:11) at Object.setPassword (C:\Users\myusername\AppD…\lib\keytar.js:20:5) at KeychainServiceDriver. (C:\Users\myusername\AppD…river.node.js:19:43) at Generator.next () at C:\Users\myusername\AppD…Driver.node.js:8:71 at new Promise () at __awaiter (C:\Users\myusername\AppD…Driver.node.js:4:12) at KeychainServiceDriver.setPassword (C:\Users\myusername\AppD…river.node.js:16:16) at KeychainService. (C:\Users\myusername\AppD…ainService.js:51:32) at Generator.next ()

JackGruber commented 1 month ago

I assume you have not set a password for your user in Windows? Therefore, the KEychain service cannot be used and the credentials is therefore stored in the Joplin database. The same happens if you use the Protable app.

I would say that the behavior is exactly as intended and no Joplin error/bug.

When you set the field to secure: false than the credentials are stored in the Joplin database or in the Json file in cleartext.

Simply suppressing this error would not be a good idea in my opinion ... Since it is assumed that the credentials are stored securely, which it is not and I think this is an error.

djsudduth commented 1 month ago

@JackGruber - nice to hear from you! No, the user account has a password in Windows and other apps that use the keystore on the same PC are working without errors. So, not sure why the error is being thrown. I agree the error should not be suppressed, that's why I raised it in @alondmnt 's plugin issues. (I'll try it on another PC to see if the error occurs on it)

Side note - I am concerned that Joplin is using keytar.js 7.9.0 who's repo has been archived. Another keystore library should be used. I would be curious if Electron's safeStorage could be the possible choice.

alondmnt commented 1 month ago

  1. Thanks @JackGruber I agree that such errors should not be simply suppressed (these fields were set to be secure because they hold API keys), but at present these errors are hidden in the log. If @djsudduth hadn't looked at the developer tools console he wouldn't have been aware of it. (Setting secure to false was only a test to validate that these fields are the cause for the error, not a workaround.)

  2. As @djsudduth wrote, it's unclear why the keychain is inaccessible. Is it possible that you're running the Joplin desktop app in dev mode? I noticed that even though I don't see the exception when running in normal mode, it does appear in dev mode.

JackGruber commented 1 month ago

Has Joplin on the System Keychain Support? grafik

laurent22 commented 1 month ago

I don't know about Windows but on macOS sometimes the keychain is in state that prevents it from saving passwords to it. And to fix it you need to close and reopen the keychain.

Maybe something like this needs to be done in Windows too? You can find the keychain somewhere in the settings of internet explorer or edge

djsudduth commented 1 month ago

@JackGruber @alondmnt - I hadn't noticed the keychain message in the About page - mine does say "Keychain Supported: Yes"

I also tried it on a different PC and the error still occurs when toggling dev tools. But both PCs show "Yes" to Keychain supported in Help (both are Windows 10).

@laurent22 - there's nothing on Windows to change, my Python applications have no issue accessing the keychain either in runtime or debugging mode. I would hazard to guess there's something in the way Electron accesses the keychain in the dev tools console mode.

I saw this online - no idea if this has any bearing: "When using Electron, you have to use keytar through the main process. Calling keytar from the render process won't work." https://stackoverflow.com/questions/56565138/unhandled-rejection-typeerror-keytar-setpassword-is-not-a-function

Maybe a clue?

This seems to be a similar issue: https://discourse.joplinapp.org/t/could-not-set-setting-on-the-keychain/34355

djsudduth commented 1 month ago

@laurent22 @JackGruber @alondmnt - After some more debugging, I've discovered the problem. Turns out keytar setPassword cannot accept empty or null values for secure attributes. I only had one secure API key set of the five in the Jarvis plugin - the rest were blank. When I set all five of the secure Jarvis API keys with some random text, the the keytar error goes away (on Windows).

See this issue: https://github.com/atom/node-keytar/issues/86 for reference.