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: fix symlink pointing error in filesystem.insertLink #301

Merged

Conversation

jrainlau
Copy link
Contributor

@jrainlau jrainlau commented Feb 1, 2024

In MacOS, if I run asar.createPackage() to pack a folder into .asar, which contains "symlink", it would get wrong after I run asar.extractAll() from the generated .asar.

Let's look at this Beyond Compare result:

企业微信截图_0ef755c7-3d85-4037-8d72-906e533b1fe1

The folder in left is the origin one, and the folder in right was generated by using asar.extractAll() from the origin one's .asar ( folderLeft --> folderLeft.asar --> folderRight).

Inside the left folder, file Resources were pointed to Versions/Current/Resources, but the same file inside the right folder were pointed to Versions/A/Resources, it's wrong.

I fixed this bug and added a test unit. I create a new folder in test/input/packthis-with-symlink, then create a /Current symlink folder from folder /A, then create a real.txt from folder /Current:

image

We could see that the new real.txt were pointing to Current/real.txt.

After preparing these materials, I made a new test unit:

it('should extract an archive with symlink', async () => {
    await asar.createPackageWithOptions('test/input/packthis-with-symlink/', 'tmp/packthis-with-symlink.asar', { dot: false })
    asar.extractAll('tmp/packthis-with-symlink.asar', 'tmp/packthis-with-symlink/')
    return compFiles('tmp/packthis-with-symlink/real.txt', 'test/input/packthis-with-symlink/real.txt')
  })

and updated the compFiles() function:

module.exports = async function (actualFilePath, expectedFilePath) {
  if (process.env.ELECTRON_ASAR_SPEC_UPDATE) {
    await fs.writeFile(expectedFilePath, await fs.readFile(actualFilePath))
  }
  const [actualFileContent, expectedFileContent] = await Promise.all([fs.readFile(actualFilePath, 'utf8'), fs.readFile(expectedFilePath, 'utf8')])
  assert.strictEqual(actualFileContent, expectedFileContent)

  const [actualIsSymlink, expectedIsSymlink] = [isSymbolicLinkSync(actualFilePath), isSymbolicLinkSync(expectedFilePath)]
  assert.strictEqual(actualIsSymlink, expectedIsSymlink)

  if (actualIsSymlink && expectedIsSymlink) {
    const [actualSymlinkPointer, expectedSymlinkPointer] = [fs.readlinkSync(actualFilePath), fs.readlinkSync(expectedFilePath)]
    assert.strictEqual(actualSymlinkPointer, expectedSymlinkPointer)
  }
}

function isSymbolicLinkSync (path) {
  const stats = fs.lstatSync(path)
  return stats.isSymbolicLink()
}

The new compFiles() function would detect whether the file is a symlink, and compare the real pointing by fs.readlinkSync(). With the bug in filesystem.js before I fixed, the test would throw an error:

image

After fixing this bug, everything runs well.

image

@jrainlau jrainlau requested a review from a team as a code owner February 1, 2024 06:29
@BlackHole1
Copy link
Member

Thank you for your contribution.


I have two questions regarding the current modifications:

  1. In what scenarios do you need to put *.framework into app.asar? (In my opinion, the correct approach would be to put it in the app.asar.unpacked folder or the Resources folder.)
  2. Your screenshot confuses me. Why isn’t there a Current file in the left-hand file tree? Shouldn't the correct file tree be like this?
    00-18 14 00@2x

If you are using electron-builder / electron-forge, you can use the extraResources configuration to place *.framework files in the Resources folder.

@jrainlau
Copy link
Contributor Author

Happy Chinese new year to @BlackHole1! Let me answer the two questions you asked.

In our scenarios, we use .asar just for "archiving", such as "tar" or "zip", etc. However, when we try to archiving the same folder into .tar or .zip, the final product would got different MD5 from different OS, different archive time, and so on. We must keep the product's MD5 consistent across all archiving enviroment. After some test, .asar would be a good choice to us.

Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for answering my questions. I don't think addressing your "requirement" is the responsibility of asar, but I believe your PR is still meaningful because it does address a bug.

In the original implementation, if the case of A -> B -> C exists, asar would lose B (due to fs.realpathSync returning the last non-symlink path).

In your fix, you use fs.readlinkSync to circumvent this issue, and I think this solution is sound.

PTAL @erickzhao @dsanders11

@dsanders11 dsanders11 merged commit a9b4e7b into electron:main Mar 5, 2024
4 checks passed
Copy link

🎉 This PR is included in version 3.2.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

@viplifes
Copy link

viplifes commented Mar 7, 2024

Hi, after this change I can't package my project

[FAILED] /var/folders/nr/c9zr4xxd6sxfcj2rq3z7vnb80000gn/T/electron-packager/tmp-ehj9F8/Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/.bin/is-docker: file "../../../../../../../../../../../../var/folders/nr/c9zr4xxd6sxfcj2rq3z7vnb80000gn/T/electron-packager/tmp-ehj9F8/Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/is-docker/cli.js" links out of the package
[FAILED] /var/folders/nr/c9zr4xxd6sxfcj2rq3z7vnb80000gn/T/electron-packager/tmp-ehj9F8/Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/.bin/is-docker: file "../../../../../../../../../../../../var/folders/nr/c9zr4xxd6sxfcj2rq3z7vnb80000gn/T/electron-packager/tmp-ehj9F8/Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/is-docker/cli.js" links out of the package
[FAILED] /var/folders/nr/c9zr4xxd6sxfcj2rq3z7vnb80000gn/T/electron-packager/tmp-ehj9F8/Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/.bin/is-docker: file "../../../../../../../../../../../../var/folders/nr/c9zr4xxd6sxfcj2rq3z7vnb80000gn/T/electron-packager/tmp-ehj9F8/Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/is-docker/cli.js" links out of the package

An unhandled rejection has occurred inside Forge:
Error: /var/folders/nr/c9zr4xxd6sxfcj2rq3z7vnb80000gn/T/electron-packager/tmp-ehj9F8/Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/.bin/is-docker: file "../../../../../../../../../../../../var/folders/nr/c9zr4xxd6sxfcj2rq3z7vnb80000gn/T/electron-packager/tmp-ehj9F8/Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/is-docker/cli.js" links out of the package
at Filesystem.insertLink (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/asar/lib/filesystem.js:106:13)
    at handleFile (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/asar/lib/asar.js:132:20)
    at next (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/asar/lib/asar.js:148:11)
    at next (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/asar/lib/asar.js:149:12)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async MacApp.asarApp (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/packager/src/platform.ts:245:5)
    at async MacApp.buildApp (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/packager/src/platform.ts:150:5)
    at async MacApp.initialize (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/packager/src/platform.ts:141:7)
    at async MacApp.create (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/packager/src/mac.ts:435:5)
    at async Promise.all (index 0)
    at async packager (/Users/dimir/projects/simulator/simulator-launcher/app/node_modules/@electron/packager/src/packager.ts:246:20)

@erickzhao
Copy link
Member

@viplifes thanks for the report, we'll take a look.

For the time being, you can downgrade the @electron/asar version in your project's lockfile to v3.2.8.

@dsanders11
Copy link
Member

@viplifes, I'm not able to repro this locally. Could you give more detailed information about the versions of packages you're using? Specifically versions of Electron Forge and @electron/packager.

@viplifes
Copy link

viplifes commented Mar 8, 2024

@dsanders11 Here is an example of a project that reproduces this problem https://github.com/viplifes/testapp_asar

I think there is a problem module: open -> is-inside-container -> is-docker

@dsanders11
Copy link
Member

Thank you @viplifes! That repro case is very helpful.

It appears the issue on macOS is one of the paths starting with /var/ and the other /private/var/ resulting in the confusion on relative paths. A potential fix is just using fs.realpathSync on parentPath so that they both end up with /private/var/. @jrainlau, any thoughts on this potential fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants