diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index 35aefe9667..c80df3c129 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/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}'`); } } diff --git a/chrome/content/zotero/xpcom/storage/storageLocal.js b/chrome/content/zotero/xpcom/storage/storageLocal.js index 8e4f698c81..d377168f26 100644 --- a/chrome/content/zotero/xpcom/storage/storageLocal.js +++ b/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] = []; diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index ec35ac3a4c..7be0af6367 100644 --- a/test/tests/itemTest.js +++ b/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") + ); + }); }); diff --git a/test/tests/storageLocalTest.js b/test/tests/storageLocalTest.js index 3d15d249b8..b29c16a0dd 100644 --- a/test/tests/storageLocalTest.js +++ b/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();