fix: ignore symlink file if it links outside module

[beyondkmp]

If there is a symlink file in some node module that points to ../../../../../../etc/passwd, this would cause the system's passwd file to be packaged along with it.

The solution is to restrict symlinks within modules to only link to other files within their directory. Any symlinks pointing outside of this directory will be ignored, and a warning log will be generated.

[changeset-bot]

🦋 Changeset detected

Latest commit: 5eef967

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
app-builder-lib	Patch
dmg-builder	Patch
electron-builder-squirrel-windows	Patch
electron-builder	Patch
electron-forge-maker-appimage	Patch
electron-forge-maker-nsis-web	Patch
electron-forge-maker-nsis	Patch
electron-forge-maker-snap	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[netlify]

✅ Deploy Preview for car-park-attendant-cleat-11576 ready!

Name	Link
🔨 Latest commit	5eef967
🔍 Latest deploy log	https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/6704039e2109e60008a8cfa9
😎 Deploy Preview	https://deploy-preview-8568--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mmaietta]

We already have a test that should be failing if the symlink is outside of the project dir (doesn't need to be a node_module specifically, just any symlinked file)

electron-builder/test/src/globTest.ts

Lines 107 to 125 in cd1e3b0

	test.ifNotWindows(
	"outside link",
	app(
	{
	targets: Platform.LINUX.createTarget(DIR_TARGET),
	},
	{
	projectDirCreated: async (projectDir, tmpDir) => {
	const tempDir = await tmpDir.getTempDir()
	await outputFile(path.join(tempDir, "foo"), "data")
	await fs.symlink(tempDir, path.join(projectDir, "o-dir"))
	},
	packed: async context => {
	const file = (await readAsar(path.join(context.getResources(Platform.LINUX), "app.asar"))).getFile("o-dir/foo", false)
	expect(removeUnstableProperties(file)).toMatchSnapshot()
	},
	}
	)
	)

I don't think we need this "ignore symlink file if it links outside node_modules" test and can instead just reuse the "outside link" test instead

[mmaietta]

I think this logic needs to be in asarUtil instead of NodeModuleCopyHelper as the symlink outside the project dir can't be allowed for both file assets or node modules.

Let me finish up the electron/asar migration and I can incorporate your changes here into that PR branch

[beyondkmp]

If it's done in asarUtil, any files linked to other workspaces within the project will all fail. I'm not sure how common this usage pattern is.

[mmaietta]

any files linked to other workspaces within the project will all fail

Can you elaborate on this? Are you referring to a two-package.json project structure?

I'm not sure how common this usage pattern is.

It's less so about how common it is and more so about enforcing best practices. For instance, electron/asar already validates making sure symlinks are within the package during asar extraction: https://github.com/electron/asar/blob/464e83436967438c74d8ac184b088fc780706b2d/src/asar.ts#L249-L253

[mmaietta]

I think we can close this PR in favor of electron/asar PR #8570 implementation of this symlink logic:

electron-builder/packages/app-builder-lib/src/asar/asarUtil.ts

Lines 101 to 109 in 9fc967e

	const realPathRelative = path.relative(this.config.appDir, realPathFile)
	const symlinkTarget = path.resolve(this.rootForAppFilesWithoutAsar, realPathRelative)
	const isOutsidePackage = realPathRelative.startsWith("../")
	if (isOutsidePackage) {
	log.error({ source: log.filePath(source), realPathFile: log.filePath(realPathFile) }, `unable to copy, file is symlinked outside the package`)
	throw new Error(
	`Cannot copy file (${path.basename(source)}) symlinked to file (${path.basename(realPathFile)}) outside the package as that violates asar security integrity`
	)
	}

[beyondkmp]

Agreeded. I will close it.