Handle subdirectories when extracting attachment ZIP files

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'

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

@@ -728,8 +728,8 @@ Zotero.Sync.Storage.Local = {
 			return false;
 		}
 		
-		var parentDir = Zotero.Attachments.getStorageDirectory(item);
+		var parentDir = Zotero.Attachments.getStorageDirectory(item).path;
-		if (!parentDir.exists()) {
+		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')) {
+			if (filePath.startsWith('.zotero')) {
-				Zotero.debug("Skipping " + fileName);
+				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
+			// Make sure all components of the path are valid, in case an invalid character somehow made
-			// somehow make it into the ZIP (e.g., from before we checked for them)
+			// it into the ZIP (e.g., from before we checked for them)
-			//
+			var filteredPath = filePath.split('/').map(part => Zotero.File.getValidFileName(part)).join('/');
-			// Do this before trying to use the relative descriptor, since otherwise
+			if (filteredPath != filePath) {
-			// it might fail silently and select the parent directory
+				Zotero.debug("Filtering filename '" + filePath + "' to '" + filteredPath + "'");
-			var filteredName = Zotero.File.getValidFileName(fileName);
+				filePath = filteredPath;
-			if (filteredName != fileName) {
-				Zotero.debug("Filtering filename '" + fileName + "' to '" + filteredName + "'");
-				fileName = filteredName;
 				filtered = true;
 			}
 			
-			// Name in ZIP is a relative descriptor, so file has to be reconstructed
+			var destPath = OS.Path.join(parentDir, ...filePath.split('/'));
-			// using setRelativeDescriptor()
-			var destFile = parentDir.clone();
-			destFile.QueryInterface(Components.interfaces.nsILocalFile);
-			destFile.setRelativeDescriptor(parentDir, fileName);
-			
-			fileName = destFile.leafName;
 			
 			// 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) {
+				if (itemFileName != filePath) {
-					Zotero.debug("Renaming single file '" + fileName + "' in ZIP to known filename '" + itemFileName + "'", 2);
+					let msg = "Renaming single file '" + filePath + "' in ZIP to known filename '" + itemFileName + "'";
-					Components.utils.reportError("Renaming single file '" + fileName + "' in ZIP to known filename '" + itemFileName + "'");
+					Zotero.debug(msg, 2);
-					fileName = itemFileName;
+					Components.utils.reportError(msg);
-					destFile.leafName = fileName;
+					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()) {
+			if (yield OS.File.exists(destPath)) {
-				var msg = "ZIP entry '" + fileName + "' " + "already exists";
+				var msg = "ZIP entry '" + filePath + "' already exists";
-				Zotero.debug(msg, 2);
+				Zotero.logError(msg);
-				Components.utils.reportError(msg + " in " + funcName);
+				Zotero.debug(destPath);
-				Zotero.debug(destFile.path);
 				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);
+				Zotero.logError(e);
-				Components.utils.reportError(e);
 				
 				zipReader.close();
 				
-				Zotero.File.checkFileAccessError(e, destFile, 'create');
+				Zotero.File.checkFileAccessError(e, destPath, 'create');
 			}
 			
-			if (destFile.leafName != fileName) {
+			if (OS.Path.basename(destPath) != shortened) {
-				Zotero.debug("Changed filename '" + fileName + "' to '" + destFile.leafName + "'");
+				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();

test/tests/storageLocalTest.js

@@ -105,17 +105,24 @@ describe("Zotero.Sync.Storage.Local", function () {
 	describe("#processDownload()", function () {
 		var file1Name = 'index.html';
 		var file1Contents = '<html><body>Test</body></html>';
-		var file2Name = 'test.txt';
+		var file2Name = 'aux1.txt';
-		var file2Contents = 'Test';
+		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
+			);
 		})
 	})