Desktop, Cli: Fix ENEX import failing on Evernote's unescaped ampersands (#16580)

packages/app-cli/tests/enex_to_md/unescaped_ampersand.enex

@@ -0,0 +1,29 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE en-export SYSTEM "http://xml.evernote.com/pub/evernote-export4.dtd">
+<en-export export-date="20260914T100000Z" application="Evernote" version="10.0">
+  <note>
+    <title>Unescaped ampersand</title>
+    <content>
+      <![CDATA[<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE en-note SYSTEM "http://xml.evernote.com/pub/enml2.dtd">
+<en-note><div>Mobilize &amp; Measure</div></en-note>
+]]>
+    </content>
+    <created>20260914T100000Z</created>
+    <updated>20260914T100000Z</updated>
+    <note-attributes>
+      <application-data key="evernote.caption">Phase 1: Mobilize & Measure, which involves forming an advocacy group.</application-data>
+    </note-attributes>
+  </note>
+  <note>
+    <title>Note after the bad one</title>
+    <content>
+      <![CDATA[<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE en-note SYSTEM "http://xml.evernote.com/pub/enml2.dtd">
+<en-note><div>This note must still be imported</div></en-note>
+]]>
+    </content>
+    <created>20260914T100000Z</created>
+    <updated>20260914T100000Z</updated>
+  </note>
+</en-export>

packages/app-desktop/gui/WindowCommandsAndDialogs/commands/importFrom.ts

@@ -10,6 +10,7 @@ import Setting from '@joplin/lib/models/Setting';
 import { PackageInfo } from '@joplin/lib/versionInfo';
 import shim from '@joplin/lib/shim';
 import { ImportModule } from '@joplin/lib/services/interop/Module';
+import { isRecoverableError } from '@joplin/lib/import-enex';
 import Logger from '@joplin/utils/Logger';
 const packageInfo: PackageInfo = require('../../../packageInfo.js');
 
@@ -136,7 +137,9 @@ export const runtime = (control: WindowControl): CommandRuntime => {
 					void CommandService.instance().execute('showModalMessage', `${modalMessage}\n\n${statusStrings.join('\n')}`);
 				},
 				onError: (error: string|Error) => {
-					errors.push(error);
+					// Recovered errors are logged but not reported, since the
+					// notes were imported successfully anyway.
+					if (!isRecoverableError(error)) errors.push(error);
 					console.warn(error);
 				},
 				destinationFolderId: options.destinationFolderId,

packages/lib/import-enex.ts

@@ -273,6 +273,12 @@ interface NoteResourceRecognition {
 	objID?: string;
 }
 
+interface SaxParser {
+	line: number;
+	column: number;
+	resume: ()=> SaxParser;
+}
+
 const preProcessFile = async (filePath: string): Promise<string> => {
 	// Disabled pre-processing for now because it runs out of memory:
 	// https://github.com/laurent22/joplin/issues/5543
@@ -308,6 +314,27 @@ const preProcessFile = async (filePath: string): Promise<string> => {
 	// return newFilePath;
 };
 
+// Evernote doesn't escape "&" in the "evernote.caption" fields. The parser
+// recovers from this on its own, but in strict mode it's fatal, so one stray
+// character would prevent the whole file from being imported.
+const recoverableParsingErrors = [
+	'Invalid character in entity name',
+	'Invalid character entity',
+];
+
+const isRecoverableParsingError = (error: Error) => {
+	return recoverableParsingErrors.some(m => error.message.includes(m));
+};
+
+// An error the importer recovered from - the import still completed.
+export interface RecoverableError extends Error {
+	recoverable?: boolean;
+}
+
+export const isRecoverableError = (error: Error | string) => {
+	return typeof error !== 'string' && !!(error as RecoverableError).recoverable;
+};
+
 const isEvernoteUrl = (url: string) => {
 	url = url.toLowerCase();
 	return url.startsWith('evernote://') || url.startsWith('https://share.evernote.com/note/');
@@ -419,7 +446,7 @@ const parseNotes = async (parentFolderId: string, filePath: string, importOption
 		const createdNoteIds: string[] = [];
 		const noteTitlesToIds = new Map<string, string[]>();
 
-		const createErrorWithNoteTitle = (fnThis: { _parser?: { line: number; column: number } } | null, error: Error) => {
+		const createErrorWithNoteTitle = (fnThis: { _parser?: SaxParser } | null, error: Error) => {
 			const line = [];
 
 			const parser = fnThis ? fnThis._parser : null;
@@ -439,7 +466,7 @@ const parseNotes = async (parentFolderId: string, filePath: string, importOption
 		};
 
 		stream.on('error', function(error: Error) {
-			importOptions.onError(createErrorWithNoteTitle(this as { _parser?: { line: number; column: number } }, error));
+			importOptions.onError(createErrorWithNoteTitle(this as { _parser?: SaxParser }, error));
 		});
 
 		function currentNodeName() {
@@ -532,8 +559,23 @@ const parseNotes = async (parentFolderId: string, filePath: string, importOption
 			return true;
 		}
 
-		saxStream.on('error', function(error: Error) {
-			importOptions.onError(createErrorWithNoteTitle(this as { _parser?: { line: number; column: number } }, error));
+		saxStream.on('error', function(error: RecoverableError) {
+			const parser = (this as { _parser?: SaxParser })._parser;
+			const canRecover = !!parser && isRecoverableParsingError(error);
+
+			// So that the caller can log it without reporting a failed import.
+			error.recoverable = canRecover;
+
+			importOptions.onError(createErrorWithNoteTitle(this as { _parser?: SaxParser }, error));
+
+			if (canRecover) {
+				// Node disconnects the source stream whenever the destination
+				// emits "error", so without piping it again the rest of the file
+				// - and the "end" event - would never be received.
+				parser.resume();
+				stream.pipe(saxStream);
+				return;
+			}
 
 			// We need to reject the promise here, or parsing will get stuck
 			// ("end" handler will never be called).
@@ -650,7 +692,7 @@ const parseNotes = async (parentFolderId: string, filePath: string, importOption
 				if (notes.length >= importOptions.batchSize) {
 					// eslint-disable-next-line promise/prefer-await-to-then -- Old code before rule was applied
 					processNotes().catch(error => {
-						importOptions.onError(createErrorWithNoteTitle(this as { _parser?: { line: number; column: number } }, error));
+						importOptions.onError(createErrorWithNoteTitle(this as { _parser?: SaxParser }, error));
 					});
 				}
 				note = null;

packages/lib/services/interop/InteropService_Importer_EnexToMd.test.ts

@@ -3,20 +3,22 @@ import { setupDatabaseAndSynchronizer, supportDir, switchClient } from '../../te
 import { ImportModuleOutputFormat, ImportOptions } from './types';
 import InteropService from './InteropService';
 import Folder from '../../models/Folder';
+import { isRecoverableError } from '../../import-enex';
 
-const importFolder = async (path: string) => {
+const importFolder = async (path: string, onError: ImportOptions['onError'] = null) => {
 	const importOptions: ImportOptions = {
 		path: path,
 		format: 'enex',
 		outputFormat: ImportModuleOutputFormat.Markdown,
+		onError,
 	};
 
 	await InteropService.instance().import(importOptions);
 };
 
-const importTestFile = async (name: string) => {
+const importTestFile = async (name: string, onError: ImportOptions['onError'] = null) => {
 	const enexSampleBaseDir = `${supportDir}/../enex_to_md`;
-	await importFolder(`${enexSampleBaseDir}/${name}`);
+	await importFolder(`${enexSampleBaseDir}/${name}`, onError);
 };
 
 describe('InteropService_Importer_EnexToMd', () => {
@@ -54,4 +56,21 @@ describe('InteropService_Importer_EnexToMd', () => {
 		expect(notes[2].body).toContain(`[Test note](:/${notes[3].id})`);
 		expect(notes[4].body).toContain(`[Test](:/${notes[2].id})`);
 	});
+
+	it('should import notes even if the file contains unescaped ampersands', async () => {
+		const errors: Error[] = [];
+		await importTestFile('unescaped_ampersand.enex', error => errors.push(error));
+
+		const notes = await Note.all({ order: [{ by: 'title', dir: 'ASC' }] });
+		expect(notes.map(n => n.title)).toEqual([
+			'Note after the bad one',
+			'Unescaped ampersand',
+		]);
+		expect(notes[0].body).toContain('This note must still be imported');
+		expect(notes[1].body).toContain('Mobilize & Measure');
+
+		// Reported, but flagged so it's not presented as a failed import.
+		expect(errors.length).toBe(1);
+		expect(isRecoverableError(errors[0])).toBe(true);
+	});
 });