Strip bidi control characters in filenames and elsewhere (#3208)

Passing unformatted = true to Item#getField() now returns a bidi control
character-less result, and we use that in Reader#updateTitle() and
getFileBaseNameFromItem() to prevent bidi control characters from showing up in
filenames and window titles (the former everywhere, the latter on Windows only).

We also strip bidi control characters in getValidFileName() to be extra safe.

chrome/content/zotero/xpcom/attachments.js

@@ -2390,9 +2390,8 @@ Zotero.Attachments = new function () {
 		}, {});
 
 		const firstCreator = args => common(
-			// 74492e40 adds \u2068 and \u2069 around names in the `firstCreator` field, which we don't want in the filename
-			// We might actually want to move this replacement to getValidFileName
-			item.getField('firstCreator', false, true).replaceAll('\u2068', '').replaceAll('\u2069', ''), args
+			// Pass unformatted = true to omit bidi isolates
+			item.getField('firstCreator', true, true), args
 		);
 
 		const vars = { ...fields, ...creatorFields, firstCreator, itemType, year };

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

@@ -216,12 +216,12 @@ Zotero.Item.prototype._setParentKey = function() {
 // Public Zotero.Item methods
 //
 //////////////////////////////////////////////////////////////////////////////
-/*
+/**
  * Retrieves an itemData field value
  *
  * @param {String|Integer} field fieldID or fieldName
- * @param {Boolean} [unformatted] Skip any special processing of DB value
- *   (e.g. multipart date field)
+ * @param {Boolean} [unformatted] Skip formatting of multipart date fields and
+ * 		omit bidi control characters
  * @param {Boolean} includeBaseMapped If true and field is a base field, returns
  *   value of type-specific field instead
  *   (e.g. 'label' for 'publisher' in 'audioRecording')
@@ -238,11 +238,17 @@ Zotero.Item.prototype.getField = function(field, unformatted, includeBaseMapped)
 	if (field === 'firstCreator' && !this._id) {
 		// Hack to get a firstCreator for an unsaved item
 		let creatorsData = this.getCreators(true);
-		return Zotero.Items.getFirstCreatorFromData(this.itemTypeID, creatorsData);
+		return Zotero.Items.getFirstCreatorFromData(this.itemTypeID, creatorsData,
+			{ omitBidiIsolates: !!unformatted });
 	} else if (field === 'id' || this.ObjectsClass.isPrimaryField(field)) {
 		var privField = '_' + field;
-		//Zotero.debug('Returning ' + (this[privField] ? this[privField] : '') + ' (typeof ' + typeof this[privField] + ')');
-		return this[privField];
+		let value = this[privField];
+		// Bidi isolates
+		if (unformatted && field === 'firstCreator') {
+			value = value.replace(/[\u2068\u2069]/g, '');
+		}
+		//Zotero.debug('Returning ' + (value ? value : '') + ' (typeof ' + typeof value + ')');
+		return value;
 	} else if (field == 'year') {
 		return this.getField('date', true, true).substr(0,4);
 	}

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

@@ -1749,9 +1749,17 @@ Zotero.Items = function() {
 	 *
 	 * @param {Integer} itemTypeID
 	 * @param {Object} creatorData
+	 * @param {Object} [options]
+	 * @param {Boolean} [options.omitBidiIsolates]
 	 * @return {String}
 	 */
-	this.getFirstCreatorFromData = function (itemTypeID, creatorsData) {
+	this.getFirstCreatorFromData = function (itemTypeID, creatorsData, options) {
+		if (!options) {
+			options = {
+				omitBidiIsolates: false
+			};
+		}
+		
 		if (creatorsData.length === 0) {
 			return "";
 		}
@@ -1773,9 +1781,12 @@ Zotero.Items = function() {
 			if (matches.length === 2) {
 				let a = matches[0];
 				let b = matches[1];
-				// \u2068 FIRST STRONG ISOLATE: Isolates the directionality of characters that follow
-				// \u2069 POP DIRECTIONAL ISOLATE: Pops the above isolation
-				return Zotero.getString('general.andJoiner', [`\u2068${a.lastName}\u2069`, `\u2068${b.lastName}\u2069`]);
+				let args = options.omitBidiIsolates
+					? [a.lastName, b.lastName]
+					// \u2068 FIRST STRONG ISOLATE: Isolates the directionality of characters that follow
+					// \u2069 POP DIRECTIONAL ISOLATE: Pops the above isolation
+					: [`\u2068${a.lastName}\u2069`, `\u2068${b.lastName}\u2069`];
+				return Zotero.getString('general.andJoiner', args);
 			}
 			if (matches.length >= 3) {
 				return matches[0].lastName + " " + Zotero.getString('general.etAl');

chrome/content/zotero/xpcom/file.js

@@ -1267,6 +1267,8 @@ Zotero.File = new function(){
 			// Normalize to NFC
 			fileName = fileName.normalize();
 		}
+		// Replace bidi isolation control characters
+		fileName = fileName.replace(/[\u2068\u2069]/g, '');
 		// Don't allow hidden files
 		fileName = fileName.replace(/^\./, '');
 		// Don't allow blank or illegal filenames

chrome/content/zotero/xpcom/reader.js

@@ -181,7 +181,10 @@ class ReaderInstance {
 			let attachment = await parentItem.getBestAttachment();
 			if (attachment && attachment.id === this._itemID) {
 				let parts = [];
-				let creator = parentItem.getField('firstCreator');
+				// Windows displays bidi control characters as placeholders in window titles, so strip them
+				// See https://github.com/mozilla-services/screenshots/issues/4863
+				let unformatted = Zotero.isWin;
+				let creator = parentItem.getField('firstCreator', unformatted);
 				let year = parentItem.getField('year');
 				let title = parentItem.getDisplayTitle();
 				// If creator is missing fall back to titleCreatorYear
@@ -198,11 +201,6 @@ class ReaderInstance {
 				readerTitle = parts.filter(x => x).join(' - ');
 			}
 		}
-		if (Zotero.isWin) {
-			// Windows displays bidi control characters as placeholders in window titles, so strip them
-			// See https://github.com/mozilla-services/screenshots/issues/4863
-			readerTitle = readerTitle.replace(/[\u2068\u2069]/g, '');
-		}
 		this._title = readerTitle;
 		this._setTitleValue(readerTitle);
 	}

test/tests/attachmentsTest.js

@@ -1584,6 +1584,13 @@ describe("Zotero.Attachments", function() {
 				'foo{{ firstCreator }}-{{ title truncate="10" }}-{{ year truncate="2" suffix="00" }}'
 			);
 		});
+
+		it("should strip bidi isolates from firstCreator", async function () {
+			var item = createUnsavedDataObject('item',
+				{ creators: [{ name: 'Foo', creatorType: 'author' }, { name: 'Bar', creatorType: 'author' }] });
+			var str = Zotero.Attachments.getFileBaseNameFromItem(item);
+			assert.equal(str, Zotero.getString('general.andJoiner', ['Foo', 'Bar']) + ' - ');
+		});
 	});
 	
 	describe("#getBaseDirectoryRelativePath()", function () {

test/tests/itemTest.js

@@ -35,6 +35,56 @@ describe("Zotero.Item", function () {
 			]);
 			assert.equal(item.getField('firstCreator'), "B");
 		});
+
+		it("should return a multi-author firstCreator for an unsaved item", async function () {
+			var item = createUnsavedDataObject('item');
+			item.setCreators([
+				{
+					firstName: "A",
+					lastName: "B",
+					creatorType: "author"
+				},
+				{
+					firstName: "C",
+					lastName: "D",
+					creatorType: "author"
+				}
+			]);
+			assert.equal(
+				item.getField('firstCreator'),
+				Zotero.getString('general.andJoiner', ['\u2068B\u2069', '\u2068D\u2069'])
+			);
+		});
+
+		it("should strip bidi isolates from firstCreator when unformatted = true", async function () {
+			var item = createUnsavedDataObject('item');
+			item.setCreators([
+				{
+					firstName: "A",
+					lastName: "B",
+					creatorType: "author"
+				},
+				{
+					firstName: "C",
+					lastName: "D",
+					creatorType: "author"
+				}
+			]);
+			
+			// Test unsaved - uses getFirstCreatorFromData()'s omitBidiIsolates option
+			assert.equal(
+				item.getField('firstCreator', /* unformatted */ true),
+				Zotero.getString('general.andJoiner', ['B', 'D'])
+			);
+			
+			await item.saveTx();
+
+			// Test saved - implemented in getField()
+			assert.equal(
+				item.getField('firstCreator', /* unformatted */ true),
+				Zotero.getString('general.andJoiner', ['B', 'D'])
+			);
+		});
 	});
 	
 	describe("#setField", function () {