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

@@ -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

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;

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<nsILoginInfo|false>}
 	 */

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.

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 {

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 {