From b6f0f6c0ba1963fcbce8617e4fe1deec7eaff028 Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Wed, 16 Sep 2026 16:05:36 -0400 Subject: [PATCH] Show login manager error when a credential write fails outside the keystore The login manager can fail to store a value even when the OS keystore works -- e.g., if key4.db is read-only, which Firefox 153 triggers because NSS now creates a new AES key on the first write. Show the existing corrupted-logins instructions instead of the keystore alert or the unencrypted-storage offer, neither of which can help. Also update the corrupted-logins dialog to add an "Open Profile Directory" button --- chrome/content/zotero/xpcom/osKeyStore.js | 13 +++- chrome/content/zotero/xpcom/storage/webdav.js | 13 +++- chrome/content/zotero/xpcom/sync/syncLocal.js | 47 ++++++++++-- chrome/locale/en-US/zotero/zotero.ftl | 2 + test/tests/syncLocalTest.js | 76 +++++++++++++++++-- test/tests/webdavTest.js | 19 +++-- 6 files changed, 149 insertions(+), 21 deletions(-) diff --git a/chrome/content/zotero/xpcom/osKeyStore.js b/chrome/content/zotero/xpcom/osKeyStore.js index 657d519a1b..e60a575e35 100644 --- a/chrome/content/zotero/xpcom/osKeyStore.js +++ b/chrome/content/zotero/xpcom/osKeyStore.js @@ -58,6 +58,12 @@ Zotero.OSKeyStore = { isEncrypted: function (value) { return typeof value == 'string' && value.startsWith(this._prefix); }, + + // Whether an error from encrypting or decrypting a credential came from the key store itself, + // as opposed to the login manager entry that holds the encrypted value + isKeyStoreError: function (e) { + return e instanceof Zotero.Error && !!e.keyStoreError; + }, // The settings window, where credentials are usually saved from, or the main window when // they're saved during a sync @@ -171,7 +177,7 @@ Zotero.OSKeyStore = { encrypt: async function (plaintext) { let mod = this._load(); if (!mod) { - throw new Error("OSKeyStore unavailable"); + throw await this._error(new Error("OSKeyStore unavailable"), 'os-keystore-save-failed'); } let ciphertext; try { @@ -193,7 +199,10 @@ Zotero.OSKeyStore = { } let mod = this._load(); if (!mod) { - throw new Error("OSKeyStore unavailable but stored value is encrypted"); + throw await this._error( + new Error("OSKeyStore unavailable but stored value is encrypted"), + 'os-keystore-read-failed' + ); } // OSKeyStore.encrypt() encodes the string as UTF-8 before encrypting, but // OSKeyStore.decrypt() returns the decrypted bytes as a binary string, so diff --git a/chrome/content/zotero/xpcom/storage/webdav.js b/chrome/content/zotero/xpcom/storage/webdav.js index ba59f65602..bf844593d7 100644 --- a/chrome/content/zotero/xpcom/storage/webdav.js +++ b/chrome/content/zotero/xpcom/storage/webdav.js @@ -261,7 +261,12 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { catch (e) { Zotero.logError(e); if (!Zotero.Sync.Runner.backgroundSync) { - Zotero.OSKeyStore.alertMigrateFailed(); + if (Zotero.OSKeyStore.isKeyStoreError(e)) { + Zotero.OSKeyStore.alertMigrateFailed(); + } + else { + await Zotero.Sync.Data.Local.alertLoginManagerCorrupted(); + } } } } @@ -394,6 +399,12 @@ Zotero.Sync.Storage.Mode.WebDAV.prototype = { Zotero.logError(e); } } + // The login manager can fail to store a value even when the keystore works -- e.g., if + // the key database is read-only -- and storing the password unencrypted wouldn't help + if (!Zotero.OSKeyStore.isKeyStoreError(error)) { + await Zotero.Sync.Data.Local.alertLoginManagerCorrupted(); + throw error; + } // A password that would read back as ciphertext can't be stored unencrypted if (Zotero.OSKeyStore.isEncrypted(password)) { throw error; diff --git a/chrome/content/zotero/xpcom/sync/syncLocal.js b/chrome/content/zotero/xpcom/sync/syncLocal.js index 560336d912..d54a113675 100644 --- a/chrome/content/zotero/xpcom/sync/syncLocal.js +++ b/chrome/content/zotero/xpcom/sync/syncLocal.js @@ -82,7 +82,12 @@ Zotero.Sync.Data.Local = { catch (e) { Zotero.logError(e); if (!Zotero.Sync.Runner.backgroundSync) { - Zotero.OSKeyStore.alertMigrateFailed(); + if (Zotero.OSKeyStore.isKeyStoreError(e)) { + Zotero.OSKeyStore.alertMigrateFailed(); + } + else { + await this.alertLoginManagerCorrupted(); + } } } } @@ -206,6 +211,14 @@ Zotero.Sync.Data.Local = { Zotero.logError(e); } } + // The login manager can fail to store a value even when the keystore works -- e.g., if + // the key database is read-only -- and storing the key unencrypted wouldn't help + if (!Zotero.OSKeyStore.isKeyStoreError(error)) { + if (!Zotero.Sync.Runner.backgroundSync) { + await this.alertLoginManagerCorrupted(); + } + throw error; + } // An automatic sync can reach this via the legacy credential upgrade in // _getAPIKeyFromLogin(), so don't interrupt one with a dialog if (Zotero.Sync.Runner.backgroundSync || !Zotero.OSKeyStore.confirmUnencryptedFallback()) { @@ -593,13 +606,7 @@ Zotero.Sync.Data.Local = { if (await this.repairLoginManager()) { return false; } - if (!this._lastLoginManagerErrorTime - || this._lastLoginManagerErrorTime < Date.now() - 60000) { - let msg = Zotero.getString('sync.error.loginManagerCorrupted1', Zotero.appName) + "\n\n" - + Zotero.getString('sync.error.loginManagerCorrupted2', Zotero.appName); - Zotero.alert(null, Zotero.getString('general.error'), msg); - this._lastLoginManagerErrorTime = Date.now(); - } + await this.alertLoginManagerCorrupted(); return false; } @@ -608,6 +615,30 @@ Zotero.Sync.Data.Local = { }, + /** + * Tell the user how to fix a login manager that can't read or write credentials, at most + * once a minute, and offer to show the files to delete + */ + alertLoginManagerCorrupted: async function () { + if (this._lastLoginManagerErrorTime + && this._lastLoginManagerErrorTime >= Date.now() - 60000) { + return; + } + this._lastLoginManagerErrorTime = Date.now(); + let index = Zotero.Prompt.confirm({ + title: Zotero.getString('general.error'), + text: Zotero.getString('sync.error.loginManagerCorrupted1', Zotero.appName) + "\n\n" + + Zotero.getString('sync.error.loginManagerCorrupted2', Zotero.appName), + button0: Zotero.getString('login-manager-open-profile-directory'), + button1: Zotero.Prompt.BUTTON_TITLE_CANCEL + }); + if (index != 0) { + return; + } + Zotero.launchFile(Zotero.Profile.dir); + }, + + /** * @return {Promise} */ diff --git a/chrome/locale/en-US/zotero/zotero.ftl b/chrome/locale/en-US/zotero/zotero.ftl index 8b11e9cf83..0bad3e2b73 100644 --- a/chrome/locale/en-US/zotero/zotero.ftl +++ b/chrome/locale/en-US/zotero/zotero.ftl @@ -1190,6 +1190,8 @@ data-dir-unsupported-storage = This can happen if the { -app-name } data directo login-manager-reset = { -app-name } was unable to read your saved login information, so it has been reset. Please log in again in the { preferences-pane-account } pane of the { -app-name } settings. +login-manager-open-profile-directory = Open Profile Directory + os-keystore-save-failed = { PLATFORM() -> [macos] { -app-name } couldn’t access the { -os-name } Keychain to securely save your credentials. Make sure your Keychain is accessible and try again. diff --git a/test/tests/syncLocalTest.js b/test/tests/syncLocalTest.js index f497aaf975..cd7d94c893 100644 --- a/test/tests/syncLocalTest.js +++ b/test/tests/syncLocalTest.js @@ -1,6 +1,15 @@ "use strict"; describe("Zotero.Sync.Data.Local", function () { + function keyStoreError() { + return new Zotero.Error( + "Key store unavailable", + 0, + { keyStoreError: new Error("User canceled OS unlock entry") } + ); + } + + describe("#getAPIKey()/#setAPIKey()", function () { it("should get and set an API key", async function () { var apiKey1 = Zotero.Utilities.randomString(24); @@ -23,7 +32,7 @@ describe("Zotero.Sync.Data.Local", function () { it("should store the key without encryption if the keystore is unusable and the user agrees", async function () { var apiKey = Zotero.Utilities.randomString(24); var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") .returns(true); try { @@ -44,7 +53,7 @@ describe("Zotero.Sync.Data.Local", function () { it("shouldn't store the key if the keystore is unusable and the user declines", async function () { var apiKey = Zotero.Utilities.randomString(24); var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") .returns(false); try { @@ -63,7 +72,7 @@ describe("Zotero.Sync.Data.Local", function () { it("should prompt before storing the key without encryption", async function () { var apiKey = Zotero.Utilities.randomString(24); var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); var promptStub = sinon.stub(Zotero.Prompt, "confirm").returns(0); try { await Zotero.Sync.Data.Local.setAPIKey(apiKey); @@ -81,7 +90,7 @@ describe("Zotero.Sync.Data.Local", function () { it("shouldn't prompt to save the key unencrypted during an automatic sync", async function () { var apiKey = Zotero.Utilities.randomString(24); var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); var promptStub = sinon.stub(Zotero.Prompt, "confirm").returns(0); var backgroundStub = sinon.stub(Zotero.Sync.Runner, "backgroundSync").get(() => true); try { @@ -99,6 +108,63 @@ describe("Zotero.Sync.Data.Local", function () { }) + it("should show the login manager error if the key can't be stored", async function () { + var apiKey = Zotero.Utilities.randomString(24); + var writeStub = sinon.stub(Zotero.Sync.Data.Local, "_writeAPIKey") + .rejects(new Error("User canceled primary password entry")); + var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") + .returns(true); + var promptStub = sinon.stub(Zotero.Prompt, "confirm").returns(1); + Zotero.Sync.Data.Local._lastLoginManagerErrorTime = null; + try { + var e = await getPromiseError(Zotero.Sync.Data.Local.setAPIKey(apiKey)); + assert.ok(e); + // Storing the key unencrypted wouldn't help, so don't offer it + assert.isFalse(confirmStub.called); + assert.ok(promptStub.calledOnce); + assert.include(promptStub.firstCall.args[0].text, "key4.db"); + } + finally { + writeStub.restore(); + confirmStub.restore(); + promptStub.restore(); + } + }) + + + it("should show the login manager error if a legacy key can't be mirrored", async function () { + var apiKey = Zotero.Utilities.randomString(24); + var nsLoginInfo = new Components.Constructor("@mozilla.org/login-manager/loginInfo;1", + Components.interfaces.nsILoginInfo, "init"); + await Services.logins.addLoginAsync(new nsLoginInfo( + Zotero.Sync.Data.Local._loginManagerHost, + null, + Zotero.Sync.Data.Local._loginManagerRealmLegacy, + 'API Key', + apiKey, + '', + '' + )); + var writeStub = sinon.stub(Zotero.Sync.Data.Local, "_writeAPIKey") + .rejects(new Error("User canceled primary password entry")); + var migrateAlertStub = sinon.stub(Zotero.OSKeyStore, "alertMigrateFailed"); + var promptStub = sinon.stub(Zotero.Prompt, "confirm").returns(1); + Zotero.Sync.Data.Local._mirroredAPIKey = false; + Zotero.Sync.Data.Local._lastLoginManagerErrorTime = null; + try { + assert.equal(await Zotero.Sync.Data.Local.getAPIKey(), apiKey); + assert.isFalse(migrateAlertStub.called); + assert.ok(promptStub.calledOnce); + } + finally { + writeStub.restore(); + migrateAlertStub.restore(); + promptStub.restore(); + await Zotero.Sync.Data.Local.setAPIKey(""); + } + }) + + it("should encrypt a key stored without encryption in a later session", async function () { if (!Zotero.OSKeyStore.available) { this.skip(); @@ -106,7 +172,7 @@ describe("Zotero.Sync.Data.Local", function () { var apiKey = Zotero.Utilities.randomString(24); // Store it with an unusable keystore var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") .returns(true); try { diff --git a/test/tests/webdavTest.js b/test/tests/webdavTest.js index 2ed7dd8af4..2116cf60ff 100644 --- a/test/tests/webdavTest.js +++ b/test/tests/webdavTest.js @@ -267,9 +267,9 @@ describe("Zotero.Sync.Storage.Mode.WebDAV", function () { } await controller.setPassword("password"); var decryptStub = sinon.stub(Zotero.OSKeyStore._module, "decrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); var encryptStub = sinon.stub(Zotero.OSKeyStore._module, "encrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); try { let e = await getPromiseError(controller.getPassword()); assert.equal(e.message, Zotero.getString('os-keystore-read-failed')); @@ -287,7 +287,7 @@ describe("Zotero.Sync.Storage.Mode.WebDAV", function () { } await controller.setPassword("password"); var stub = sinon.stub(Zotero.OSKeyStore._module, "decrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); try { let e = await getPromiseError(controller.getPassword()); assert.equal(e.message, Zotero.getString('os-keystore-read-unrecoverable')); @@ -298,9 +298,18 @@ describe("Zotero.Sync.Storage.Mode.WebDAV", function () { }) + function keyStoreError() { + return new Zotero.Error( + "Key store unavailable", + 0, + { keyStoreError: new Error("User canceled OS unlock entry") } + ); + } + + it("shouldn't store a password that would read back as encrypted", async function () { var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") .returns(true); try { @@ -317,7 +326,7 @@ describe("Zotero.Sync.Storage.Mode.WebDAV", function () { it("should return a password stored without encryption after a keystore failure", async function () { var password = "p\u00e4ssw\u20acrd"; var encryptStub = sinon.stub(Zotero.OSKeyStore, "encrypt") - .rejects(new Error("User canceled OS unlock entry")); + .rejects(keyStoreError()); var confirmStub = sinon.stub(Zotero.OSKeyStore, "confirmUnencryptedFallback") .returns(true); try {