[FIX] manifestEnhancer: Only use valid files for supportedLocales (#1080)

This fixes two problems that could have occurred:

A properties file with an invalid locale was still taken into the list
of supported locales, which then caused a runtime exception in the
ResourceBundle as it validates the input.

Another problem was that properties files could have a valid name
according to BCP47, but the file won't be ever requested with that name.
This is due to the fact that the ResourceBundle does use the legacy Java
locale format (using underscores instead of dashes) for the request URL.

In both cases, the properties file is now ignored and no entry for the
supportedLocales is created.

Only locales that are valid according to the legacy Java locale format
are considered.
However, there is one special case: sr_Latn is also requested by the UI5
runtime, although it contains a BCP47 script, which is not valid
according to the legacy Java locale format.

---------

Co-authored-by: Merlin Beutlberger <m.beutlberger@sap.com>

Author: matz3

lib/processors/manifestEnhancer.js

@@ -7,6 +7,58 @@ const log = getLogger("builder:processors:manifestEnhancer");
 
 const APP_DESCRIPTOR_V22 = new Version("1.21.0");
 
+/*
+ * Matches a legacy Java locale string, which is the format used by the UI5 Runtime (ResourceBundle)
+ * to load i18n properties files.
+ * Special case: "sr_Latn" is also supported, although the BCP47 script part is not supported by the Java locale format.
+ *
+ * Variants are limited to the format from BCP47, but with underscores instead of hyphens.
+ */
+//                          [     language     ]    [    region    ][                variants               ]
+const rLegacyJavaLocale = /^([a-z]{2,3}|sr_Latn)(?:_([A-Z]{2}|\d{3})((?:_[0-9a-zA-Z]{5,8}|_[0-9][0-9a-zA-Z]{3})*)?)?$/;
+
+// See https://github.com/SAP/openui5/blob/d7ecf2792788719d35b4eee3085a327d545bab24/src/sap.ui.core/src/sap/base/i18n/LanguageFallback.js#L10
+const sapSupportabilityVariants = ["saptrc", "sappsd", "saprigi"];
+
+function getBCP47LocaleFromPropertiesFilename(locale) {
+	const match = rLegacyJavaLocale.exec(locale);
+	if (!match) {
+		return null;
+	}
+	let [, language, region, variants] = match;
+	let script;
+
+	variants = variants?.slice(1); // Remove leading underscore
+
+	// Special handling of sr_Latn (see regex above)
+	// Note: This needs to be in sync with the runtime logic:
+	// https://github.com/SAP/openui5/blob/d7ecf2792788719d35b4eee3085a327d545bab24/src/sap.ui.core/src/sap/base/i18n/LanguageFallback.js#L87
+	if (language === "sr_Latn") {
+		language = "sr";
+		script = "Latn";
+	}
+
+	if (language === "en" && region === "US" && sapSupportabilityVariants.includes(variants)) {
+		// Convert to private use section
+		// Note: This needs to be in sync with the runtime logic:
+		// https://github.com/SAP/openui5/blob/d7ecf2792788719d35b4eee3085a327d545bab24/src/sap.ui.core/src/sap/base/i18n/LanguageFallback.js#L75
+		variants = `x-${variants}`;
+	}
+
+	let bcp47Locale = language;
+	if (script) {
+		bcp47Locale += `-${script}`;
+	}
+	if (region) {
+		bcp47Locale += `-${region}`;
+	}
+	if (variants) {
+		// Convert to BCP47 variant format
+		bcp47Locale += `-${variants.replace(/_/g, "-")}`;
+	}
+	return bcp47Locale;
+}
+
 function isAbsoluteUrl(url) {
 	if (url.startsWith("/")) {
 		return true;
@@ -132,6 +184,8 @@ class ManifestEnhancer {
 
 		this.isModified = false;
 		this.runInvoked = false;
+
+		this.supportedLocalesCache = new Map();
 	}
 
 	markModified() {
@@ -151,7 +205,14 @@ class ManifestEnhancer {
 		}
 	}
 
-	async findSupportedLocales(i18nBundleUrl) {
+	findSupportedLocales(i18nBundleUrl) {
+		if (!this.supportedLocalesCache.has(i18nBundleUrl)) {
+			this.supportedLocalesCache.set(i18nBundleUrl, this._findSupportedLocales(i18nBundleUrl));
+		}
+		return this.supportedLocalesCache.get(i18nBundleUrl);
+	}
+
+	async _findSupportedLocales(i18nBundleUrl) {
 		const i18nBundleName = path.basename(i18nBundleUrl, ".properties");
 		const i18nBundlePrefix = `${i18nBundleName}_`;
 		const i18nBundleDir = path.dirname(i18nBundleUrl);
@@ -165,8 +226,13 @@ class ManifestEnhancer {
 			if (fileNameWithoutExtension === i18nBundleName) {
 				supportedLocales.push("");
 			} else if (fileNameWithoutExtension.startsWith(i18nBundlePrefix)) {
-				const locale = fileNameWithoutExtension.replace(i18nBundlePrefix, "");
-				supportedLocales.push(locale);
+				const fileNameLocale = fileNameWithoutExtension.replace(i18nBundlePrefix, "");
+				const bcp47Locale = getBCP47LocaleFromPropertiesFilename(fileNameLocale);
+				if (bcp47Locale) {
+					supportedLocales.push(bcp47Locale);
+				} else {
+					log.warn(`Ignoring unexpected locale in filename '${fileName}' for bundle '${i18nBundleUrl}'`);
+				}
 			}
 		});
 		return supportedLocales.sort();

test/expected/build/application.o/dest/Component-preload.js

@@ -0,0 +1 @@
+welcome=Hello EN US saptrc world

test/expected/build/application.o/dest/i18n/i18n_en_US_sapprc.properties

@@ -12,8 +12,8 @@
       "supportedLocales": [
         "",
         "en",
-        "en_US",
-        "en_US_sapprc"
+        "en-US",
+        "en-US-x-saptrc"
       ]
     }
   },
@@ -26,8 +26,8 @@
           "supportedLocales": [
             "",
             "en",
-            "en_US",
-            "en_US_sapprc"
+            "en-US",
+            "en-US-x-saptrc"
           ]
         }
       },
@@ -38,8 +38,8 @@
           "supportedLocales": [
             "",
             "en",
-            "en_US",
-            "en_US_sapprc"
+            "en-US",
+            "en-US-x-saptrc"
           ]
         }
       }

test/expected/build/application.o/dest/i18n/i18n_en_US_saptrc.properties

@@ -0,0 +1 @@
+welcome=Hello EN US saptrc world

test/expected/build/application.o/dest/manifest.json

@@ -2,6 +2,34 @@ import test from "ava";
 import sinonGlobal from "sinon";
 import esmock from "esmock";
 
+function isValidBCP47Locale(locale) {
+	if (locale === "") {
+		// Special handling of empty string, as this marks the developer locale (without locale information)
+		return true;
+	}
+
+	// See https://github.com/SAP/openui5/blob/a8f36e430f1fac172eb705811da4a2af25483408/src/sap.ui.core/src/sap/base/i18n/ResourceBundle.js#L30
+	/**
+	 * A regular expression that describes language tags according to BCP-47.
+	 *
+	 * @see BCP47 "Tags for Identifying Languages" (http://www.ietf.org/rfc/bcp/bcp47.txt)
+	 *
+	 * The matching groups are
+	 *  0=all
+	 *  1=language (shortest ISO639 code + ext. language sub tags | 4digits (reserved) | registered language sub tags)
+	 *  2=script (4 letters)
+	 *  3=region (2letter language or 3 digits)
+	 *  4=variants (separated by '-', Note: capturing group contains leading '-' to shorten the regex!)
+	 *  5=extensions (including leading singleton, multiple extensions separated by '-')
+	 *  6=private use section (including leading 'x', multiple sections separated by '-')
+	 */
+
+	// eslint-disable-next-line max-len
+	//                [-------------------- language ----------------------][--- script ---][------- region --------][------------- variants --------------][----------- extensions ------------][------ private use -------]
+	const rLocale = /^((?:[A-Z]{2,3}(?:-[A-Z]{3}){0,3})|[A-Z]{4}|[A-Z]{5,8})(?:-([A-Z]{4}))?(?:-([A-Z]{2}|[0-9]{3}))?((?:-[0-9A-Z]{5,8}|-[0-9][0-9A-Z]{3})*)((?:-[0-9A-WYZ](?:-[0-9A-Z]{2,8})+)*)(?:-(X(?:-[0-9A-Z]{1,8})+))?$/i;
+	return rLocale.test(locale);
+}
+
 test.beforeEach(async (t) => {
 	const sinon = t.context.sinon = sinonGlobal.createSandbox();
 	t.context.logWarnSpy = sinon.spy();
@@ -3032,20 +3060,143 @@ test("manifestEnhancer#getSupportedLocales", async (t) => {
 	fs.readdir.withArgs("/i18n")
 		.callsArgWith(1, null, [
 			"i18n.properties",
-			"i18n_en.properties"
+			"i18n_ar_001.properties",
+			"i18n_ar_001_variant.properties",
+			"i18n_crn.properties",
+			"i18n_en.properties",
+			"i18n_en_US.properties",
+			"i18n_en_US_saptrc.properties",
+			"i18n_en_US_saprigi.properties",
+			"i18n_sr_Latn_RS.properties",
+			"i18n_sr_Latn_RS_variant.properties"
 		]);
 
-	t.deepEqual(await manifestEnhancer.getSupportedLocales("./i18n/i18n.properties"), ["", "en"]);
-	t.deepEqual(await manifestEnhancer.getSupportedLocales("i18n/../i18n/i18n.properties"), ["", "en"]);
-	t.deepEqual(await manifestEnhancer.getSupportedLocales("ui5://sap/ui/demo/app/i18n/i18n.properties"), ["", "en"]);
+	const expectedLocales = [
+		"",
+		"ar-001",
+		"ar-001-variant",
+		"crn",
+		"en",
+		"en-US",
+		"en-US-x-saprigi",
+		"en-US-x-saptrc",
+		"sr-Latn-RS",
+		"sr-Latn-RS-variant",
+	];
+
+	const generatedLocales = await manifestEnhancer.getSupportedLocales("./i18n/i18n.properties");
+
+	t.deepEqual(generatedLocales, expectedLocales);
+	t.deepEqual(await manifestEnhancer.getSupportedLocales("i18n/../i18n/i18n.properties"), expectedLocales);
+	t.deepEqual(await manifestEnhancer.getSupportedLocales("ui5://sap/ui/demo/app/i18n/i18n.properties"), expectedLocales);
 
 	// Path traversal to root and then into application namespace
 	// This works, but is not recommended at all! It also likely fails at runtime
 	t.deepEqual(await manifestEnhancer.getSupportedLocales(
 		"../../../../../../../../../../../../resources/sap/ui/demo/app/i18n/i18n.properties"
-	), ["", "en"]);
+	), expectedLocales);
+
+	// findSupportedLocales caches requests, so for the same bundle readdir is only called once
+	t.is(fs.readdir.callCount, 1);
+
+	t.true(t.context.logVerboseSpy.notCalled, "No verbose messages should be logged");
+	t.true(t.context.logWarnSpy.notCalled, "No warnings should be logged");
+	t.true(t.context.logErrorSpy.notCalled, "No errors should be logged");
+
+	// Check whether generated locales are valid BCP47 locales, as the UI5 runtime
+	// fails if a locale is not a valid
+	generatedLocales.forEach((locale) => {
+		t.true(isValidBCP47Locale(locale), `Generated locale '${locale}' should be a valid BCP47 locale`);
+	});
+});
+
+test("manifestEnhancer#getSupportedLocales (invalid file names)", async (t) => {
+	const {fs} = t.context;
+	const {ManifestEnhancer} = t.context.__internals__;
+
+	const manifest = JSON.stringify({
+		"_version": "1.58.0",
+		"sap.app": {
+			"id": "sap.ui.demo.app"
+		}
+	});
+	const filePath = "/manifest.json";
+
+	const manifestEnhancer = new ManifestEnhancer(manifest, filePath, fs);
+
+	const fileNames = [
+		"i18n.properties",
+		"i18n_en.properties",
+
+		// Invalid: Should be "en_US"
+		"i18n_en-US.properties",
 
-	t.is(fs.readdir.callCount, 4);
+		// Invalid: Should be "zh_CN"
+		"i18n_zh_CN_.properties",
+
+		// Invalid: Script section is only supported for "sr_Latn"
+		"i18n_en_Latn_US.properties",
+
+		// Invalid: Legacy Java locale format does have a BCP47 "extension" section
+		"i18n_sr_Latn_RS_variant_f_11.properties",
+
+		// Invalid: Legacy Java locale format does have a BCP47 "private use" section
+		"i18n_sr_Latn_RS_variant_x_private.properties",
+
+		// Invalid: Legacy Java locale format does have BCP47 "extension" / "private use" sections
+		"i18n_sr_Latn_RS_variant_f_11_x_private.properties",
+
+		// Invalid: Invalid variant length (too short)
+		"i18n_de_CH_var.properties",
+
+		// Invalid: Invalid variant length (too long)
+		"i18n_de_CH_variant11.properties",
+
+		// Invalid: Invalid variant length (too long)
+		"i18n_de_CH_001FOOBAR.properties",
+
+		// Invalid: Should be "en_US_saprigi"
+		"i18n_en_US_x_saprigi.properties"
+	];
+
+	fs.readdir.withArgs("/i18n")
+		.callsArgWith(1, null, fileNames);
+
+	const expectedLocales = [
+		"",
+		"en"
+	];
+
+	t.deepEqual(await manifestEnhancer.getSupportedLocales("./i18n/i18n.properties"), expectedLocales);
+
+	t.is(fs.readdir.callCount, 1);
+
+	t.is(t.context.logWarnSpy.callCount, 10);
+	t.is(t.context.logWarnSpy.getCall(0).args[0],
+		"Ignoring unexpected locale in filename 'i18n_en-US.properties' for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(1).args[0],
+		"Ignoring unexpected locale in filename 'i18n_zh_CN_.properties' for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(2).args[0],
+		"Ignoring unexpected locale in filename 'i18n_en_Latn_US.properties' for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(3).args[0],
+		"Ignoring unexpected locale in filename 'i18n_sr_Latn_RS_variant_f_11.properties' " +
+		"for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(4).args[0],
+		"Ignoring unexpected locale in filename 'i18n_sr_Latn_RS_variant_x_private.properties' " +
+		"for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(5).args[0],
+		"Ignoring unexpected locale in filename 'i18n_sr_Latn_RS_variant_f_11_x_private.properties' " +
+		"for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(6).args[0],
+		"Ignoring unexpected locale in filename 'i18n_de_CH_var.properties' for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(7).args[0],
+		"Ignoring unexpected locale in filename 'i18n_de_CH_variant11.properties' for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(8).args[0],
+		"Ignoring unexpected locale in filename 'i18n_de_CH_001FOOBAR.properties' for bundle 'i18n/i18n.properties'");
+	t.is(t.context.logWarnSpy.getCall(9).args[0],
+		"Ignoring unexpected locale in filename 'i18n_en_US_x_saprigi.properties' for bundle 'i18n/i18n.properties'");
+	t.true(t.context.logVerboseSpy.notCalled, "No verbose messages should be logged");
+	t.true(t.context.logErrorSpy.notCalled, "No errors should be logged");
 });
 
 test("manifestEnhancer#getSupportedLocales (absolute / invalid URLs)", async (t) => {
@@ -3076,13 +3227,24 @@ test("manifestEnhancer#getSupportedLocales (absolute / invalid URLs)", async (t)
 	t.deepEqual(await manifestEnhancer.getSupportedLocales("sftp:i18n.properties"), []);
 	t.deepEqual(await manifestEnhancer.getSupportedLocales("file://i18n.properties"), []);
 
+	t.is(t.context.logVerboseSpy.callCount, 0, "No verbose messages should be logged");
+
 	// Path traversal to root
 	t.deepEqual(await manifestEnhancer.getSupportedLocales("../../../../../../../../../../../../i18n.properties"), []);
 
+	t.is(t.context.logVerboseSpy.callCount, 1, "One verbose message should be logged from previous call");
+	t.is(t.context.logVerboseSpy.getCall(0).args[0],
+		"/manifest.json: bundleUrl '../../../../../../../../../../../../i18n.properties' " +
+		"points to a bundle outside of the current namespace 'sap.ui.demo.app', enhancement of " +
+		"'supportedLocales' is skipped");
+
 	// Relative ui5-protocol URL
 	t.deepEqual(await manifestEnhancer.getSupportedLocales("ui5:i18n.properties"), []);
 
 	t.is(fs.readdir.callCount, 0, "readdir should not be called for any absolute / invalid URL");
+	t.is(t.context.logVerboseSpy.callCount, 1, "No additional verbose messages should be logged");
+	t.true(t.context.logWarnSpy.notCalled, "No warnings should be logged");
+	t.true(t.context.logErrorSpy.notCalled, "No errors should be logged");
 });
 
 test("manifestEnhancer#getSupportedLocales (error handling)", async (t) => {
@@ -3120,6 +3282,10 @@ test("manifestEnhancer#getSupportedLocales (error handling)", async (t) => {
 	});
 
 	t.is(fs.readdir.callCount, 2, "readdir should be called once");
+
+	t.true(t.context.logVerboseSpy.notCalled, "No verbose messages should be logged");
+	t.true(t.context.logWarnSpy.notCalled, "No warnings should be logged");
+	t.true(t.context.logErrorSpy.notCalled, "No errors should be logged");
 });
 
 test("getRelativeBundleUrlFromName", (t) => {