Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow paths to asar archives to contain the .asar extension in directories #20342

Merged
merged 1 commit into from Oct 2, 2019

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Sep 25, 2019

Description of Change

There is an issue with the code parsing paths to determine whether it's an ASAR archive:

const ASAR_EXTENSION = '.asar'
// Separate asar package's path from full path.
const splitPath = archivePathOrBuffer => {
// Shortcut for disabled asar.
if (isAsarDisabled()) return { isAsar: false }
// Check for a bad argument type.
let archivePath = archivePathOrBuffer
if (Buffer.isBuffer(archivePathOrBuffer)) {
archivePath = archivePathOrBuffer.toString()
}
if (typeof archivePath !== 'string') return { isAsar: false }
if (archivePath.endsWith(ASAR_EXTENSION)) {
return { isAsar: true, asarPath: archivePath, filePath: '' }
}
archivePath = path.normalize(archivePath)
const index = archivePath.lastIndexOf(`${ASAR_EXTENSION}${path.sep}`)
if (index === -1) return { isAsar: false }
// E.g. for "//some/path/to/archive.asar/then/internal.file"...
return {
isAsar: true,
// "//some/path/to/archive.asar"
asarPath: archivePath.substr(0, index + ASAR_EXTENSION.length),
// "then/internal.file" (with a path separator excluded)
filePath: archivePath.substr(index + ASAR_EXTENSION.length + 1)
}
}

It can handle //some/path.asar/to/archive.asar/then/internal.file, however if the file ends up being mapped to //some/path.asar/to/archive.asar.unpacked/then/internal.file, it incorrectly returns:

{
  isAsar: true,
  asarPath: "//some/path.asar",
  filePath: "to/archive.asar.unpacked/then/internal.file"
}

This PR:

  • moves the core of the splitPath implementation to native code reusing asar::GetAsarArchivePath() to eliminate code duplication
  • no longer considers directories with .asar extension an archive (the results of is directory check need to be cached for performance reasons)

Checklist

Release Notes

Notes: Fixed parsing of paths with .asar in directory name extensions.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 25, 2019
@miniak miniak changed the title fix: handle "//some/path.asar/to/archive.asar.unpacked/then/internal.file" paths fix: parsing of paths where file.asar.unpacked file is placed inside of dir.asar Sep 25, 2019
@miniak
Copy link
Contributor Author

miniak commented Sep 25, 2019

@zcbenz we have an issue with a customer, who has the app installed to
C:\Users\xyz.asar\AppData\Local\Microsoft\Teams

Loading a node module, which is located in app.asar.unpacked fails with this exception:

Uncaught Exception:
Error: Invalid package \\?\C:\Users\xyz.asar
    at invalidArchiveError (ELECTRON_ASAR.js:147:19)
    at process.module.(anonymous function) [as dlopen] (ELECTRON_ASAR.js:171:9)
    at Object.Module._extensions..node (internal/modules/cjs/loader.js:740:18)
    at Object.module.(anonymous function) [as .node] (ELECTRON_ASAR.js:180:18)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:559:12)
    at Function.Module._load (internal/modules/cjs/loader.js:551:3)
    at Module.require (internal/modules/cjs/loader.js:658:17)
    at require (C:\Users\t.asar\AppData\Local\Microsoft\Teams\current\resources\app.asar\external\v8-compile-cache\v8-compile-cache.js:173:28)
    at Object.<anonymous> (C:\Users\t.asar\AppData\Local\Microsoft\Teams\current\resources\app.asar\node_modules\native-utils\index.js:1:173)

@miniak
Copy link
Contributor Author

miniak commented Sep 25, 2019

@zcbenz I am not sure what's the best solution. Ideally only files would be treated as ASAR archives, not directories. But we would need to check the type every time we parse the path, which would be inefficient.

@miniak miniak self-assigned this Sep 25, 2019
@zcbenz
Copy link
Member

zcbenz commented Sep 26, 2019

@miniak I think it is not just a problem with reading file under archive.asar.unpacked, it seems that reading any normal file under C:\Users\xyz.asar\ would result in the same error?

The //some/path.asar/to/archive.asar/then/internal.file works because the path is parsed backwards, paths like //some/path.asar/to/file would fail if path.asar is a directory.

But we would need to check the type every time we parse the path, which would be inefficient.

Checking type every time seems to be only correct solution. We can probably use caching to avoid performance problem:

if (cachedArchives.has(archivePath))
  isAsar = true
else if (cacedFolders.has(archivePath))
  isAsar = false
else {
  if (fs.isFolder(archivePath)) {
    cacedFolders.add(archivePath)
    isAsar = false
  } else
    isAsar = true
}

we would have problems when user replaces a folder with a real asar archive though, but I think we don't need to be prepared for that.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 26, 2019
@miniak miniak changed the title fix: parsing of paths where file.asar.unpacked file is placed inside of dir.asar fix: only consider files to be asar archives Sep 27, 2019
@miniak miniak changed the title fix: only consider files to be asar archives fix: allow paths to asar archives to contain the .asar extension in directories Sep 27, 2019
@miniak miniak marked this pull request as ready for review September 27, 2019 06:13
@miniak miniak requested a review from zcbenz September 27, 2019 06:15
@miniak
Copy link
Contributor Author

miniak commented Sep 27, 2019

@zcbenz I've added the check for directories with caching. I've also moved some of the parsing code to native to eliminate code duplication.

@zcbenz zcbenz merged commit bf978e0 into master Oct 2, 2019
@trop
Copy link
Contributor

trop bot commented Oct 2, 2019

@miniak has manually backported this PR to "7-0-x", please check out #20401

@trop
Copy link
Contributor

trop bot commented Oct 2, 2019

@miniak has manually backported this PR to "6-0-x", please check out #20402

@trop
Copy link
Contributor

trop bot commented Oct 2, 2019

@miniak has manually backported this PR to "5-0-x", please check out #20403

zcbenz pushed a commit that referenced this pull request Oct 2, 2019
zcbenz pushed a commit that referenced this pull request Oct 2, 2019
zcbenz pushed a commit that referenced this pull request Oct 2, 2019
@sofianguy sofianguy added this to 7.0.0-beta.6 in 7.2.x Oct 9, 2019
@sofianguy sofianguy added this to Fixed in 6.0.12 in 6.1.x Oct 22, 2019
@sofianguy sofianguy added this to Fixed in 5.0.12 in 5.0.x Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
5.0.x
Fixed in 5.0.12
6.1.x
Fixed in 6.0.12
7.2.x
7.0.0-beta.6
Development

Successfully merging this pull request may close these issues.

None yet

3 participants