From 676f820f87d68bf52905f2d1d473b69b0c47ea7a Mon Sep 17 00:00:00 2001 From: Abe Jellinek Date: Sat, 22 Jul 2023 03:30:28 -0400 Subject: [PATCH] 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 | 5 +-- chrome/content/zotero/xpcom/data/item.js | 18 +++++--- chrome/content/zotero/xpcom/data/items.js | 19 ++++++-- chrome/content/zotero/xpcom/file.js | 2 + chrome/content/zotero/xpcom/reader.js | 10 ++--- test/tests/attachmentsTest.js | 7 +++ test/tests/itemTest.js | 50 ++++++++++++++++++++++ 7 files changed, 92 insertions(+), 19 deletions(-) diff --git a/chrome/content/zotero/xpcom/attachments.js b/chrome/content/zotero/xpcom/attachments.js index 4034bb8366..9d61f0f3e0 100644 --- a/chrome/content/zotero/xpcom/attachments.js +++ b/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 }; diff --git a/chrome/content/zotero/xpcom/data/item.js b/chrome/content/zotero/xpcom/data/item.js index c37de5900b..ecbdd5612f 100644 --- a/chrome/content/zotero/xpcom/data/item.js +++ b/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); } diff --git a/chrome/content/zotero/xpcom/data/items.js b/chrome/content/zotero/xpcom/data/items.js index e3c25bada5..a72cc81375 100644 --- a/chrome/content/zotero/xpcom/data/items.js +++ b/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'); diff --git a/chrome/content/zotero/xpcom/file.js b/chrome/content/zotero/xpcom/file.js index 433ac3d89d..7cc97bcd62 100644 --- a/chrome/content/zotero/xpcom/file.js +++ b/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 diff --git a/chrome/content/zotero/xpcom/reader.js b/chrome/content/zotero/xpcom/reader.js index 639a6c9d63..d33c6b1aef 100644 --- a/chrome/content/zotero/xpcom/reader.js +++ b/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); } diff --git a/test/tests/attachmentsTest.js b/test/tests/attachmentsTest.js index df4a45d4a3..19e38d22a9 100644 --- a/test/tests/attachmentsTest.js +++ b/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 () { diff --git a/test/tests/itemTest.js b/test/tests/itemTest.js index a04f9c07e3..5eedebeab9 100644 --- a/test/tests/itemTest.js +++ b/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 () {