From 88a43fea31a619a166f1e5aa035a269575d1734f Mon Sep 17 00:00:00 2001 From: Dan Stillman Date: Thu, 2 Jun 2016 03:39:26 -0400 Subject: [PATCH] Handle subdirectories when extracting attachment ZIP files --- chrome/content/zotero/xpcom/file.js | 2 + .../zotero/xpcom/storage/storageLocal.js | 100 +++++++++--------- test/tests/storageLocalTest.js | 43 +++++++- 3 files changed, 93 insertions(+), 52 deletions(-) diff --git a/chrome/content/zotero/xpcom/file.js b/chrome/content/zotero/xpcom/file.js index 8217573ab..185a4584a 100644 --- a/chrome/content/zotero/xpcom/file.js +++ b/chrome/content/zotero/xpcom/file.js @@ -912,6 +912,8 @@ Zotero.File = new function(){ this.checkFileAccessError = function (e, file, operation) { + file = this.pathToFile(file); + var str = 'file.accessError.'; if (file) { str += 'theFile' diff --git a/chrome/content/zotero/xpcom/storage/storageLocal.js b/chrome/content/zotero/xpcom/storage/storageLocal.js index ba9cc01da..d3ba98a62 100644 --- a/chrome/content/zotero/xpcom/storage/storageLocal.js +++ b/chrome/content/zotero/xpcom/storage/storageLocal.js @@ -728,8 +728,8 @@ Zotero.Sync.Storage.Local = { return false; } - var parentDir = Zotero.Attachments.getStorageDirectory(item); - if (!parentDir.exists()) { + var parentDir = Zotero.Attachments.getStorageDirectory(item).path; + if (!(yield OS.File.exists(parentDir))) { yield Zotero.Attachments.createDirectoryForItem(item); } @@ -748,95 +748,95 @@ Zotero.Sync.Storage.Local = { var entries = zipReader.findEntries(null); while (entries.hasMore()) { - count++; var entryName = entries.getNext(); + var entry = zipReader.getEntry(entryName); var b64re = /%ZB64$/; if (entryName.match(b64re)) { - var fileName = Zotero.Utilities.Internal.Base64.decode( + var filePath = Zotero.Utilities.Internal.Base64.decode( entryName.replace(b64re, '') ); } else { - var fileName = entryName; + var filePath = entryName; } - if (fileName.startsWith('.zotero')) { - Zotero.debug("Skipping " + fileName); + if (filePath.startsWith('.zotero')) { + Zotero.debug("Skipping " + filePath); continue; } - Zotero.debug("Extracting " + fileName); + if (entry.isDirectory) { + Zotero.debug("Skipping directory " + filePath); + continue; + } + + count++; + + Zotero.debug("Extracting " + filePath); var primaryFile = false; var filtered = false; var renamed = false; - // Make sure the new filename is valid, in case an invalid character - // somehow make it into the ZIP (e.g., from before we checked for them) - // - // Do this before trying to use the relative descriptor, since otherwise - // it might fail silently and select the parent directory - var filteredName = Zotero.File.getValidFileName(fileName); - if (filteredName != fileName) { - Zotero.debug("Filtering filename '" + fileName + "' to '" + filteredName + "'"); - fileName = filteredName; + // Make sure all components of the path are valid, in case an invalid character somehow made + // it into the ZIP (e.g., from before we checked for them) + var filteredPath = filePath.split('/').map(part => Zotero.File.getValidFileName(part)).join('/'); + if (filteredPath != filePath) { + Zotero.debug("Filtering filename '" + filePath + "' to '" + filteredPath + "'"); + filePath = filteredPath; filtered = true; } - // Name in ZIP is a relative descriptor, so file has to be reconstructed - // using setRelativeDescriptor() - var destFile = parentDir.clone(); - destFile.QueryInterface(Components.interfaces.nsILocalFile); - destFile.setRelativeDescriptor(parentDir, fileName); - - fileName = destFile.leafName; + var destPath = OS.Path.join(parentDir, ...filePath.split('/')); // If only one file in zip and it doesn't match the known filename, // take our chances and use that name if (count == 1 && !entries.hasMore() && itemFileName) { // May not be necessary, but let's be safe itemFileName = Zotero.File.getValidFileName(itemFileName); - if (itemFileName != fileName) { - Zotero.debug("Renaming single file '" + fileName + "' in ZIP to known filename '" + itemFileName + "'", 2); - Components.utils.reportError("Renaming single file '" + fileName + "' in ZIP to known filename '" + itemFileName + "'"); - fileName = itemFileName; - destFile.leafName = fileName; + if (itemFileName != filePath) { + let msg = "Renaming single file '" + filePath + "' in ZIP to known filename '" + itemFileName + "'"; + Zotero.debug(msg, 2); + Components.utils.reportError(msg); + filePath = itemFileName; + destPath = OS.Path.join(OS.Path.dirname(destPath), itemFileName); renamed = true; } } - var primaryFile = itemFileName == fileName; + var primaryFile = itemFileName == filePath; if (primaryFile && filtered) { renamed = true; } - if (destFile.exists()) { - var msg = "ZIP entry '" + fileName + "' " + "already exists"; - Zotero.debug(msg, 2); - Components.utils.reportError(msg + " in " + funcName); - Zotero.debug(destFile.path); + if (yield OS.File.exists(destPath)) { + var msg = "ZIP entry '" + filePath + "' already exists"; + Zotero.logError(msg); + Zotero.debug(destPath); continue; } + let shortened; try { - Zotero.File.createShortened(destFile, Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0644); + shortened = Zotero.File.createShortened( + destPath, Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0644 + ); } catch (e) { - Zotero.debug(e, 1); - Components.utils.reportError(e); + Zotero.logError(e); zipReader.close(); - Zotero.File.checkFileAccessError(e, destFile, 'create'); + Zotero.File.checkFileAccessError(e, destPath, 'create'); } - if (destFile.leafName != fileName) { - Zotero.debug("Changed filename '" + fileName + "' to '" + destFile.leafName + "'"); + if (OS.Path.basename(destPath) != shortened) { + Zotero.debug(`Changed filename '${OS.Path.basename(destPath)}' to '${shortened}'`); // Abort if Windows path limitation would cause filenames to be overly truncated - if (Zotero.isWin && destFile.leafName.length < 40) { + if (Zotero.isWin && shortened < 40) { try { - destFile.remove(false); + yield OS.File.remove(destPath); } catch (e) {} zipReader.close(); @@ -848,17 +848,19 @@ Zotero.Sync.Storage.Local = { throw new Error(msg); } + destPath = OS.Path.join(OS.Path.dirname(destPath, shortened)); + if (primaryFile) { renamed = true; } } try { - zipReader.extract(entryName, destFile); + zipReader.extract(entryName, Zotero.File.pathToFile(destPath)); } catch (e) { try { - destFile.remove(false); + yield OS.File.remove(destPath); } catch (e) {} @@ -866,7 +868,7 @@ Zotero.Sync.Storage.Local = { // destFile.create() works but zipReader.extract() doesn't // when the path length is close to 255. if (destFile.leafName.match(/[a-zA-Z0-9+=]{130,}/)) { - var msg = "Ignoring error extracting '" + destFile.path + "'"; + var msg = "Ignoring error extracting '" + destPath + "'"; Zotero.debug(msg, 2); Zotero.debug(e, 2); Components.utils.reportError(msg + " in " + funcName); @@ -875,14 +877,14 @@ Zotero.Sync.Storage.Local = { zipReader.close(); - Zotero.File.checkFileAccessError(e, destFile, 'create'); + Zotero.File.checkFileAccessError(e, destPath, 'create'); } - destFile.permissions = 0644; + yield OS.File.setPermissions(destPath, { unixMode: 0o644 }); // If we're renaming the main file, processDownload() needs to know if (renamed) { - returnFile = destFile.path; + returnFile = destPath; } } zipReader.close(); diff --git a/test/tests/storageLocalTest.js b/test/tests/storageLocalTest.js index d051c08d2..d3ff5453e 100644 --- a/test/tests/storageLocalTest.js +++ b/test/tests/storageLocalTest.js @@ -105,17 +105,24 @@ describe("Zotero.Sync.Storage.Local", function () { describe("#processDownload()", function () { var file1Name = 'index.html'; var file1Contents = 'Test'; - var file2Name = 'test.txt'; - var file2Contents = 'Test'; + var file2Name = 'aux1.txt'; + var file2Contents = 'Test 1'; + var subDirName = 'sub'; + var file3Name = 'aux2'; + var file3Contents = 'Test 2'; var createZIP = Zotero.Promise.coroutine(function* (zipFile) { var tmpDir = Zotero.getTempDirectory().path; var zipDir = OS.Path.join(tmpDir, Zotero.Utilities.randomString()); yield OS.File.makeDir(zipDir); - yield Zotero.File.putContentsAsync(OS.Path.join(zipDir, file1Name), file1Contents); yield Zotero.File.putContentsAsync(OS.Path.join(zipDir, file2Name), file2Contents); + // Subdirectory + var subDir = OS.Path.join(zipDir, subDirName); + yield OS.File.makeDir(subDir); + yield Zotero.File.putContentsAsync(OS.Path.join(subDir, file3Name), file3Contents); + yield Zotero.File.zipDirectory(zipDir, zipFile); yield OS.File.removeDir(zipDir); }); @@ -129,6 +136,15 @@ describe("Zotero.Sync.Storage.Local", function () { var zipFile = OS.Path.join(tmpDir, key + '.tmp'); yield createZIP(zipFile); + // Create an existing attachment directory (and subdirectory) to replace + var dir = Zotero.Attachments.getStorageDirectoryByLibraryAndKey(libraryID, key).path; + yield OS.File.makeDir( + OS.Path.join(dir, 'subdir'), + { from: Zotero.getZoteroDirectory().path } + ); + yield Zotero.File.putContentsAsync(OS.Path.join(dir, 'A'), ''); + yield Zotero.File.putContentsAsync(OS.Path.join(dir, 'subdir', 'B'), ''); + var md5 = Zotero.Utilities.Internal.md5(Zotero.File.pathToFile(zipFile)); var mtime = 1445667239000; @@ -155,10 +171,31 @@ describe("Zotero.Sync.Storage.Local", function () { }); yield OS.File.remove(zipFile); + var storageDir = Zotero.Attachments.getStorageDirectory(item).path; + + // Make sure previous files don't exist + assert.isFalse(yield OS.File.exists(OS.Path.join(storageDir, 'A'))); + assert.isFalse(yield OS.File.exists(OS.Path.join(storageDir, 'subdir'))); + assert.isFalse(yield OS.File.exists(OS.Path.join(storageDir, 'subdir', 'B'))); + + // Make sure main file matches attachment hash and mtime yield assert.eventually.equal( item.attachmentHash, Zotero.Utilities.Internal.md5(file1Contents) ); yield assert.eventually.equal(item.attachmentModificationTime, mtime); + + // Check second file + yield assert.eventually.equal( + Zotero.File.getContentsAsync(OS.Path.join(storageDir, file2Name)), + file2Contents + ); + + // Check subdirectory and file + assert.isTrue((yield OS.File.stat(OS.Path.join(storageDir, subDirName))).isDir); + yield assert.eventually.equal( + Zotero.File.getContentsAsync(OS.Path.join(storageDir, subDirName, file3Name)), + file3Contents + ); }) })