fix: escape strings.xml app name

[tiagoappereira]

Platforms affected

Android

Motivation and Context

Error on cordova platform add when the Cordova app name contains the character &. The issue occurs when parsing the strings.xml file. The issue was introduced on cordova-android 10.1.0

Fixes #1380

Description

Simply replaces the & characters in the project name (if present) by an _.

Testing

npm test and project creation successful

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[dpogue]

Wouldn't the correct fix be to XML-escape the & (or other special characters like < and >)?

[codecov-commenter]

Codecov Report

Merging #1384 (64ed3b7) into master (a1ed1c0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1384      +/-   ##
==========================================
+ Coverage   73.15%   73.17%   +0.01%     
==========================================
  Files          21       21              
  Lines        1643     1644       +1     
==========================================
+ Hits         1202     1203       +1     
  Misses        441      441              

Impacted Files	Coverage Δ
lib/create.js	94.28% <100.00%> (+0.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1ed1c0...64ed3b7. Read the comment docs.

[breautek]

Lgtm

[jcesarmobile]

package.json doesn't include lodash.
It works because there are 3 other dependencies that include lodash as dependency themselves but it could break in the future if those dependencies are replaced/removed or they stop using lodash in a newer version.
So I think it should be included in the package.json to prevent that.

[erisu]

I am not infavor for installing the entire lodash package just to use one method.

Either install the exact package, lodash.escape, or write our own escape method please.

Lodash
package size: 319.0 kB
unpacked size: 1.4 MB

Lodash Escape
package size: 3.9 kB
unpacked size: 9.1 kB

[jcesarmobile]

It's already being installed because it's a dependency of other 3 dependencies.
As you can see, the package-lock.json doesn't change because it's already installed.

I would be in favor of not installing lodash, but that would mean updating a lot of our dependencies (including cordova-common as it depends on @netflix/nerror which depends on lodash)

[erisu]

We have been removing lodash from our dependencies because of the vuln. report frequencies.

I have discussed with others on dropping @netflix/nerror because they neglected to update their package over 2 years now and even with a PR open fixing their issues.

Netflix/nerror#18

[breautek]

We have been removing lodash from our dependencies because of the vuln. report frequencies.

I have discussed with others on dropping @netflix/nerror because they neglected to update their package over 2 years now and even with PRs open to resolve some issues.

Netflix/nerror#18

To add onto this, we can probably solve the same issue using the JS native method encodeURIComponent so that we can avoid introducing another lodash dependency in the code. I'm pretty sure the method exists in NodeJS environments.

[jcesarmobile]

encodeURIComponent won't work because it scapes <Incredible&App> to %3CIncredible%26App%3E, which is not valid on the strings.xml.

I solved a similar problem in Capacitor using replace like this
https://github.com/ionic-team/capacitor/pull/5325/files#diff-ed3a1ffe6172324599c40aaeea6832beab33815ec5c28ba7f87e1a37fb55fae1R74-R78

note that it doesn't include > replacement because it wasn't needed in Capacitor, but might still be needed in Cordova as it looks like it does some previous conversion
also note that it includes ", which is not escaped by lodash

[erisu]

Here is what lodash escape does, just for additional details:

  '&': '&',
  '<': '&lt;',
  '>': '&gt;',
  '"': '&quot;',
  "'": '&#39;',
  '`': '&#96;'

[jcesarmobile]

there is some previous conversion somewhere else, because I tested with ' and " and ' was converted to \' and " wasn't converted at all.
Note that for them to work in strings.xml they have to be \' and \", didn't try ``` (by work I mean that the app name is displayed on the phone exactly as on the strings.xml)

[jcesarmobile]

I found where the ' is converted to \', is not done by lodash

cordova-android/lib/prepare.js

Line 266 in 09c7523

strings.find('string[@name="app_name"]').text = name.replace(/'/g, '\\\'');