Upgrade xml2js to address security vulnerability

[karlhorky]

Why

Address security vulnerability in xml2js below version 0.5.0

Ref: Leonidas-from-XIV/node-xml2js#663 (comment)

How

Upgrade xml2js version

Test Plan

Checklist

• Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md).
• Conforms with the Documentation Writing Style Guide
• This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

[karlhorky]

Ah, seems maybe this is already done by the bot over here:

• Bump xml2js from 0.4.23 to 0.5.0 #22071

[expo-bot]

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries

Your changes should be noted in the changelog. Read Updating Changelogs guide and consider adding an appropriate entry to the following changelogs:

• packages/@expo/config-plugins/CHANGELOG.md

---

Generated by ExpoBot 🤖 against fb7efb6

[Simek]

Hello @karlhorky, thank you for preparing a bump PR! 👍 Unfortunately, it looks like some tests are failing after the package upgrade:

• https://github.com/expo/expo/actions/runs/4665172035/jobs/8258221627?pr=22080

Would you like to look at the failures and try to find the root cause of that? Maybe there were some breaking changes in new version, besides the vulnerability fix, since they have bumped the minor?

[tambor81]

is anybody looking at these failing tests?
the fix seems to be good on xml2js on 0.5.0 but the vulnerability is still failing in our daily builds

[rikur]

Could this be addressed? Using resolutions to fix the vulnerability (which will trigger a pentest failure) breaks expo-plugins:

TypeError: [android.manifest]: withAndroidManifestBaseMod: androidManifest.manifest.hasOwnProperty is not a function

@EvanBacon

[brentsmyth]

It seems a primary change in xml2js 0.5.0 was to start using Object.create(null); vs {} to resolve a CVE where properties like __proto__ or hasOwnProperty could be passed via the XML. If any logic is relying on these traditional JS properties (which looks to be the case) then it would need cleaning up.

See: Leonidas-from-XIV/node-xml2js#603

[brentsmyth]

It appears that many tests are failing because they parse XML to JS objects (with null prototype) and then feed those objects back through logic like this which expects properties like isString:

expo/packages/@expo/config-plugins/src/utils/XML.ts

Lines 61 to 78 in 0746a58

	} else if (manifest.toString) {
	const builder = new Builder({
	headless: true,
	});
	// For strings.xml
	if (Array.isArray(manifest?.resources?.string)) {
	for (const string of manifest?.resources?.string) {
	if (string.$.translatable === 'false' || string.$.translatable === false) {
	continue;
	}
	string._ = escapeAndroidString(string._);
	}
	}
	xmlInput = builder.buildObject(manifest);
	return xmlInput;

Might be a bit tricky to fix this without pulling some things apart. Possibly might also be worth considering an alternative like https://github.com/NaturalIntelligence/fast-xml-parser. They seemed to have recently addressed the issue in a slightly more pragmatic manner ... NaturalIntelligence/fast-xml-parser@2b032a4

[brentvatne]

we use xml2js@0.6.0 now, thanks for the PR though! sorry i didn't follow up here