fix: Escape version in NSIS Updater during replace

[burnhamup]

The updater derives the path to the old blockmap file by using the url of the new blockmap file and changing the version. The code uses a RegEx with the replace function in order to replace all occurrences of the version number.

The problem is that the version number is a string like '0.1.9-2+273e2eb' and contains characters like '.' and '+' that get interpreted as regex characters. This works in most cases since a simple version string like '0.1.9' will accidentally match a typical filename. However it would aggressively match a filename like 'My-Application0A1B9-Setup-0.1.9.exe', and will fail with versions containing plus signs.

I initially tried using String.ReplaceAll but it was only recently added in Chrome 85. Some sort of escaping needs to happen, so I used lodash.escapeRegExp since the project already seems to be using lodash. If there's a problem with the dependency, we can use simpler escaping since I think the '.' and '+' are the only valid semver characters that cause regex problems.

[mmaietta]

@burnhamup thank you!

Would you be willing to add a test case for this?

There are currently these two files that might be leverageable
https://github.com/electron-userland/electron-builder/blob/master/test/src/updater/nsisUpdaterTest.ts
https://github.com/electron-userland/electron-builder/blob/master/test/src/updater/differentialUpdateTest.ts

I'll also think about how some tests could be added here. AFAICT, there's not an immediate manner with which to verify the blockmap regexes

[burnhamup]

It looks like nsisUpdateTest would be the best candidate for a test. A number of those tests call the differentialDownloadInstaller() function that I altered. The function appears stubbed out in tests, depending on what the _testOnlyOptions are set to.

I sort of see how to toggle this, because the differentialUpdateTest changes isUseDifferentialDownload, but that file doesn't call the differentialDownloadInstaller function.

I'm also not sure how to unstub that function, since it wants to actually download the two blockmaps. Would those blockmaps need to be created and uploaded to develar.s3.amazonaws.com/test ?

[mmaietta]

Hmmmm. What if we shortcut this and just extracted the blockmap name construction to a utility:

blockmapFiles(fileInfo: ResolvedUpdateFileInfo, downloadUpdateOptions: DownloadUpdateOptions): URL[]

Or if unable to mock those objects easily, then the param could just be of type info: UpdateInfo for downloadUpdateOptions.updateInfoAndProvider.info

Then later in the function it'd be

const blockMapDataList = await Promise.all(blockmapFileUrls.map(u => downloadBlockMap(u)))

That way we can only test explicitly what you're changing with simple unit tests.

I also just found this file that we could add your unit tests to:
https://github.com/electron-userland/electron-builder/blob/master/test/src/urlUtilTest.ts

What are your thoughts?

[burnhamup]

I made an attempt at adding test cases - wasn't expecting my push to automatically show up here yet.

The test I added was passing, and failing if I removed the regexpEscape call.

I see the posix smart unpack is failing, and I'm trying to understand if that was somehow impacted by my change.

[mmaietta]

Great work on this.

You can ignore the failing test. It's unrelated to your changes and is unstable, I'm still trying to figure out how to fix it.

[mmaietta]

@burnhamup, sorry for the delay on this. Can you please rebase this off latest master then I can merge

[burnhamup]

Okay, I rebased things. It looks like some of the url functions were moved from main to util, so I placed my new function there as well.

[mmaietta]

Note:The failing test is unrelated and something I'm working on in another branch.

PR LGTM. Merging