Fix sync error due to invalid attachment filename

A stored-file path of 'storage:/' -- left behind by the 128 schema step,
which skipped paths with no basename -- triggered an
NS_ERROR_FILE_UNRECOGNIZED_PATH that aborted the whole
checkForUpdatedFiles() loop, so no files synced in the library. Skip an
attachment that throws instead of failing the whole library.

Also apply the setter's directory-path rule in getFilePath[Async]() to
avoid errors elsewhere.

https://forums.zotero.org/discussion/133523/synchronize-issue

(cherry picked from commit 661843b03f)

chrome/content/zotero/xpcom/data/item.js

@@ -2875,6 +2875,22 @@ Zotero.Item.prototype.getFile = function () {
 }
 
 
+/**
+ * Check whether a stored file's filename contains a directory path -- a forward slash (never
+ * valid in a filename) or a Windows absolute path (drive-letter or UNC prefix). Bare
+ * backslashes are technically valid on Linux/macOS and present in existing filenames (e.g.,
+ * LaTeX in titles), so they're allowed here.
+ *
+ * @param {String} filename
+ * @return {Boolean}
+ */
+function filenameContainsPath(filename) {
+	return filename.includes('/')
+		|| /^[a-zA-Z]:[\\/]/.test(filename)
+		|| filename.startsWith('\\\\');
+}
+
+
 /**
  * Get the absolute file path for the attachment
  *
@@ -2914,6 +2930,13 @@ Zotero.Item.prototype.getFilePath = function () {
 		// Strip "storage:"
 		path = path.substr(8);
 		
+		// A stored file's path is just a filename, so a directory path means it's invalid
+		if (filenameContainsPath(path)) {
+			Zotero.logError("Invalid stored-file attachment filename '" + path + "'");
+			this._updateAttachmentStates(false);
+			return false;
+		}
+		
 		// Ignore .zotero* files that were relinked before we started blocking them
 		if (path.startsWith(".zotero")) {
 			Zotero.debug("Ignoring attachment file " + path, 2);
@@ -3008,6 +3031,13 @@ Zotero.Item.prototype.getFilePathAsync = async function () {
 		// Strip "storage:"
 		path = path.substr(8);
 		
+		// A stored file's path is just a filename, so a directory path means it's invalid
+		if (filenameContainsPath(path)) {
+			Zotero.logError("Invalid stored-file attachment filename '" + path + "'");
+			this._updateAttachmentStates(false);
+			return false;
+		}
+		
 		// Ignore .zotero* files that were relinked before we started blocking them
 		if (path.startsWith(".zotero")) {
 			Zotero.debug("Ignoring attachment file " + path, 2);
@@ -3671,15 +3701,8 @@ Zotero.defineProperty(Zotero.Item.prototype, 'attachmentPath', {
 				}
 				val = 'storage:' + PathUtils.filename(val);
 			}
-			// A leaked directory path -- a forward slash (never valid in a filename) or
-			// a Windows absolute path (drive-letter or UNC prefix). Bare backslashes
-			// are technically valid on Linux/macOS and present in existing filenames
-			// (e.g., LaTeX in titles), so allow here for now to avoid breaking things,
-			// but getValidFileName() should be used elsewhere to strip them.
-			let filename = val.substr(8);
-			if (filename.includes('/')
-					|| /^[a-zA-Z]:[\\/]/.test(filename)
-					|| filename.startsWith('\\\\')) {
+			// getValidFileName() should be used elsewhere to strip backslashes
+			if (filenameContainsPath(val.substr(8))) {
 				throw new Error(`Stored-file filename cannot contain a directory path -- got '${val}'`);
 			}
 		}

chrome/content/zotero/xpcom/storage/storageLocal.js

@@ -289,8 +289,16 @@ Zotero.Sync.Storage.Local = {
 		var changed = false;
 		var statesToSet = {};
 		for (let item of items) {
-			// TODO: Catch error?
-			let state = await this._checkForUpdatedFile(item, attachmentData[item.id]);
+			let state;
+			try {
+				state = await this._checkForUpdatedFile(item, attachmentData[item.id]);
+			}
+			catch (e) {
+				// Don't let one broken attachment stop the rest of the library from syncing
+				Zotero.logError("Error checking attachment file for item " + item.libraryKey);
+				Zotero.logError(e);
+				continue;
+			}
 			if (state !== false) {
 				if (!statesToSet[state]) {
 					statesToSet[state] = [];

test/tests/itemTest.js

@@ -1067,6 +1067,30 @@ describe("Zotero.Item", function () {
 				attachment.getFilePath()
 			);
 		});
+		
+		it("should return false for a stored file with a directory path instead of a filename", async function () {
+			var item = await importTextAttachment();
+			// Corrupt paths from a third-party tool that bypassed the setter
+			for (let path of ["storage:/", "storage:D:\\foo\\bar\\", "storage:foo/bar.pdf"]) {
+				item._attachmentPath = path;
+				assert.isFalse(item.getFilePath(), path);
+				assert.isFalse(await item.getFilePathAsync(), path);
+			}
+		});
+		
+		it("should return a path for a stored file with a backslash in the filename", async function () {
+			// Backslashes are only valid in filenames on Linux/macOS
+			if (Zotero.isWin) this.skip();
+			
+			var item = await importTextAttachment();
+			var storageDir = Zotero.getStorageDirectory().path;
+			item._attachmentPath = "storage:foo\\bar.txt";
+			
+			assert.equal(
+				item.getFilePath(),
+				OS.Path.join(storageDir, item.key, "foo\\bar.txt")
+			);
+		});
 	});
 	
 	

test/tests/storageLocalTest.js

@@ -31,6 +31,47 @@ describe("Zotero.Sync.Storage.Local", function () {
 			assert.equal(item.attachmentSyncState, Zotero.Sync.Storage.Local.SYNC_STATE_TO_UPLOAD);
 		});
 		
+		it("should keep checking files after an error on one attachment", async function () {
+			var item1 = await importTextAttachment();
+			var item2 = await importTextAttachment();
+			var hash = await item2.attachmentHash;
+			// Set file mtime to the past (without milliseconds, which aren't used on OS X)
+			var mtime = (Math.floor(new Date().getTime() / 1000) * 1000) - 1000;
+			await OS.File.setDates(((await item2.getFilePathAsync())), null, mtime);
+			
+			// Mark as synced, so they will be checked
+			item1.attachmentSyncState = "in_sync";
+			await item1.saveTx({ skipAll: true });
+			item2.attachmentSyncedModificationTime = mtime;
+			item2.attachmentSyncedHash = hash;
+			item2.attachmentSyncState = "in_sync";
+			await item2.saveTx({ skipAll: true });
+			
+			// Update mtime and contents of the second file
+			var path = await item2.getFilePathAsync();
+			await OS.File.setDates(path);
+			await Zotero.File.putContentsAsync(path, Zotero.Utilities.randomString());
+			
+			var local = Zotero.Sync.Storage.Local;
+			var stub = sinon.stub(local, '_checkForUpdatedFile');
+			stub.withArgs(sinon.match(item => item.id == item1.id)).throws(new Error("Test error"));
+			stub.callThrough();
+			
+			var libraryID = Zotero.Libraries.userLibraryID;
+			try {
+				var changed = await local.checkForUpdatedFiles(libraryID, [item1.id, item2.id]);
+			}
+			finally {
+				stub.restore();
+			}
+			
+			await item1.eraseTx();
+			await item2.eraseTx();
+			
+			assert.isTrue(changed);
+			assert.equal(item2.attachmentSyncState, local.SYNC_STATE_TO_UPLOAD);
+		});
+		
 		it("should skip a file if mod time hasn't changed", async function () {
 			// Create attachment
 			let item = await importTextAttachment();