From a8ed30ce803a9c5b3b5c04d50f6bd6b94faeee86 Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Fri, 8 Jul 2022 06:00:19 -0400
Subject: [PATCH] Validate fields in ItemTree::getSortFields()

To avoid startup hang if a plugin does something bad:

https://forums.zotero.org/discussion/comment/411843/#Comment_411843

Fixes #2692
---
 chrome/content/zotero/itemTree.jsx | 46 ++++++++++++++++++++++++------
 test/tests/itemTreeTest.js         | 34 ++++++++++++++++++++++
 2 files changed, 72 insertions(+), 8 deletions(-)

diff --git a/chrome/content/zotero/itemTree.jsx b/chrome/content/zotero/itemTree.jsx
index 8f5bbe02c4..f9f114712b 100644
--- a/chrome/content/zotero/itemTree.jsx
+++ b/chrome/content/zotero/itemTree.jsx
@@ -1814,20 +1814,37 @@ var ItemTree = class ItemTree extends LibraryTree {
 		if (secondaryField) {
 			fields.push(secondaryField);
 		}
+		var fallbackFields;
 		try {
-			var fallbackFields = Zotero.Prefs.get('fallbackSort')
+			fallbackFields = Zotero.Prefs.get('fallbackSort')
 				.split(',')
-				.map((x) => x.trim())
-				.filter((x) => x !== '');
+				.map(x => x.trim())
+				.filter(x => x !== '');
+			for (let field of fallbackFields) {
+				if (!this._isValidSortField(field)) {
+					// eslint-disable-next-line no-throw-literal
+					throw `'${field}' is not a valid field in fallbackSort -- resetting`;
+				}
+			}
 		}
 		catch (e) {
-			Zotero.debug(e, 1);
 			Zotero.logError(e);
-			// This should match the default value for the fallbackSort pref
-			var fallbackFields = ['firstCreator', 'date', 'title', 'dateAdded'];
+			Zotero.Prefs.clear('fallbackSort');
+			fallbackFields = Zotero.Prefs.get('fallbackSort').split(',');
 		}
 		fields = Zotero.Utilities.arrayUnique(fields.concat(fallbackFields));
-
+		var validFields = fields.filter(x => this._isValidSortField(x));
+		if (validFields.length) {
+			fields = validFields;
+		}
+		// If no valid fields, use default fallback
+		else {
+			Zotero.logError(`No valid fields in getSortFields() (${fields.join(',')}) `
+				+ '-- resetting');
+			Zotero.Prefs.clear('fallbackSort');
+			fields = Zotero.Prefs.get('fallbackSort').split(',');
+		}
+		
 		// If date appears after year, remove it, unless it's the explicit secondary sort
 		var yearPos = fields.indexOf('year');
 		if (yearPos != -1) {
@@ -1839,7 +1856,13 @@ var ItemTree = class ItemTree extends LibraryTree {
 
 		return fields;
 	}
-
+	
+	_isValidSortField(field) {
+		return field == 'year'
+			|| !!Zotero.ItemFields.getID(field)
+			|| Zotero.Items.primaryFields.includes(field);
+	}
+	
 	/**
 	 * @param index {Integer}
 	 * @param selectAll {Boolean} Whether the selection is part of a select-all event
@@ -3712,6 +3735,13 @@ var ItemTree = class ItemTree extends LibraryTree {
 		if (!secondaryField || secondaryField == primaryField) {
 			return false;
 		}
+		// Make sure specified field is valid
+		if (!this._isValidSortField(secondaryField)) {
+			Zotero.logError(`'${secondaryField}' is not a valid field in `
+				+ `secondarySort.${primaryField} -- resetting`);
+			Zotero.Prefs.clear('secondarySort.' + primaryField);
+			return false;
+		}
 		return secondaryField;
 	}
 	
diff --git a/test/tests/itemTreeTest.js b/test/tests/itemTreeTest.js
index 9e14aae882..ddbdde1c32 100644
--- a/test/tests/itemTreeTest.js
+++ b/test/tests/itemTreeTest.js
@@ -151,6 +151,40 @@ describe("Zotero.ItemTree", function() {
 		})
 	})
 	
+	describe("#getSortFields()", function () {
+		before(async function () {
+			itemsView = zp.itemsView;
+			
+			// Sort by title
+			const colIndex = itemsView.tree._getColumns().findIndex(column => column.dataKey == 'title');
+			await itemsView.tree._columns.toggleSort(colIndex);
+		});
+		
+		after(function () {
+			Zotero.Prefs.clear('fallbackSort');
+		});
+		
+		it("should ignore invalid secondary-sort field", function () {
+			Zotero.Prefs.set('secondarySort.title', 'invalidField');
+			var fields = itemsView.getSortFields();
+			assert.isUndefined(Zotero.Prefs.get('secondarySort.title'));
+			// fallbackSort pref with title moved to beginning
+			assert.sameMembers(['title', 'firstCreator', 'date', 'dateAdded'], fields);
+			
+			Zotero.Prefs.clear('secondarySort.title');
+		});
+		
+		it("should ignore invalid fallback-sort field", function () {
+			Zotero.Prefs.clear('fallbackSort');
+			var originalFallback = Zotero.Prefs.get('fallbackSort');
+			Zotero.Prefs.set('fallbackSort', 'invalidField,' + originalFallback);
+			var fields = itemsView.getSortFields();
+			assert.equal(Zotero.Prefs.get('fallbackSort'), originalFallback);
+			// fallbackSort pref with title moved to beginning
+			assert.sameMembers(['title', 'firstCreator', 'date', 'dateAdded'], fields);
+		});
+	});
+	
 	describe("#notify()", function () {
 		beforeEach(function () {
 			sinon.spy(win.ZoteroPane, "itemSelected");