node_gyp_bins causes build failures in downstream packages

[VerteDinde]

• Node Version: Node 16.13.0 and NPM 8.1.0
• Node-Gyp Version: >= v9.0.0
• Platform: Mac & Linux
• Module: electron-packager & electron-rebuild

Hey folks! I'm a maintainer of electron-packager and electron-rebuild. We're seeing an issue with rebuilding native modules using node-gyp, that we think we introduced by this change: b9ddcd5 .

When trying to rebuild a native module using Electron and node-gyp 9.0.0+, rebuilding will fail for older versions of python or folks using a python version manager with this error:

An unhandled rejection has occurred inside Forge:
Error: /var/folders/sp/141453k53mg70282wr9vkn_h0000gp/T/electron-packager/darwin-x64/test-darwin-x64/Electron.app/Contents/Resources/app/node_modules/ffi-napi/build/node_gyp_bins/python3:
file links out of the package

The link to the specific issue, with more details, is here: electron/rebuild#1024

We can handle this error inside electron-packager by ignoring the directory, but would it be possible to move this symlink directory outside of node_modules and into something like a tmp dir?

I wasn't sure if the directory being in node_modules may cause other issues with other downstream users/packages, and thought we'd err on the side of reporting it 🙂 If there's a technical reason why it needs to be included in node_modules, please feel free to close this.

Thanks for your time!

[rickymohk]

I am working on an electron project and running into this issue too. Though this only occurs when I use electron-packager (via electorn-forge) with asar enabled. Any temporary workaround?

[pimterry]

I'm also hitting this issue in an Electron project, it's breaking my build completely due to this dangling symlink.

As an alternative to moving this path, could this symlink just be cleaned up after the build is completed? I can see how this is useful during the build process, but it doesn't seem like there's any good reason for it to persist afterwards.

[pimterry]

Another report of problems caused by this issue from stack overflow: https://stackoverflow.com/questions/73216989/electron-forge-throws-python3-8-links-out-of-the-package-error-when-makin

[cclauss]

From 2016: electron-userland/electron-builder#675 -- file links out of the package

https://www.npmjs.com/package/asar-dev

Purpose of this package is to offer a solution to the issue electron-userland/electron-builder#675.
Error: /Users/user1/myapp/node_modules/myapp-lib: file links out of the package

Can we please see the command typed and the full error log?

[pimterry]

In my case this I am building an Electron app and using Electron Builder, but the failure doesn't come from there at all, it comes from codesign - Apple's code signing tool from XCode - so although that bug above is a similar issue I don't think the cause is directly related @cclauss. It's just another example of symlink + bundling issues.

In my case, the specific command that finally fails for me is:

codesign --verify --deep --strict --verbose=2 \
    /Users/runner/work/httptoolkit-desktop/httptoolkit-desktop/dist/mac/HTTP Toolkit.app

You can see the full error log in my CI build here: https://github.com/httptoolkit/httptoolkit-desktop/runs/7883734525?check_suite_focus=true.

In this case, my .app folder includes an separate subcomponent, whose build includes an npm install that builds native modules, and whose built output (e.g. the tarballs here) is just directly copied into my codebase.

The node_modules folder for this subcomponent ends up including a [...]/build/node_gyp_bins/python3 link to /usr/bin/python3, which (AFAICT) is created by node-gyp during the npm install step.

It's impossible to codesign any app for Mac that contains symlinks outside the codebase like this, so these links completely break my build. I think Apple is acting reasonably in rejecting this, I agree that a distributable app generally shouldn't contain absolute links to external locations on the filesystem. Even if they're weren't correct though, Apple is not likely to change this behaviour any time soon.

This will affect any node_module folder that eventually gets code signed like this, and it'll also affect many other tools that attempt to bundle a directory including node_modules, because they will all (sensibly) refuse to bundle /usr/bin/python into the app itself.

For now I've fixed this by manually scanning for all node_gyp_bins directories and deleting them during my install process. That solves the issue, but it would be much better if node-gyp cleaned up after itself instead.

[cclauss]

I would recommend reading thru https://github.com/txoof/codesign (which is written in Python) and then opening an issue on that repo if you still do not have a resolution.

[pimterry]

I would recommend reading thru https://github.com/txoof/codesign (which is written in Python) and then opening an issue on that repo if you still do not have a resolution.

@cclauss I'm not using that package, I'm using Apple's own codesign binary. There's more info here: https://developer.apple.com/library/archive/documentation/Security/Conceptual/CodeSigningGuide/Procedures/Procedures.html#//apple_ref/doc/uid/TP40005929-CH4-SW3.

To be clear: Codesign is working correctly in this case. It is rejecting bad input from node-gyp. I want this error - I do not want to codesign my /usr/bin/python binary. Similarly, people who are bundling node_modules usually do want that to fail if they accidentally include symlinks to other parts of the system outside their codebase.

This is not a problem with codesign or anything else. Node-gyp is now leaving behind absolute symlinks in node_modules. This will cause problems with many tools, but particularly for people building any kind of distributable applications. Node-gyp should not leave absolute symlinks like this in node_modules after the build is completed.

Is there a specific reason that leaving these links after the build completes is useful? Would there be any downside to removing them? I'm happy to open a PR to do so and fix this issue, if that would be accepted.

[cclauss]

Well explained. I now understand. I think your pull request would be the right path forward. Let's try to remove the absolute symlinks in node_modules and see where that takes us.

[pimterry]

Ok, great, I'll do that now

[pimterry]

Create a PR to fix this here: #2721

Not quite as simple as described above, but the same end result. Since the symlinks were created in configure, deleting them at the end of builds would've meant that repeated builds after a single configure could have different behaviour (or might fail entirely) which seemed bad.

Instead, once we calculate the link path in configure we now just store it in the config file, and then the symlink exists only during the build - created at the start using the config value, and deleted straight after.

[cclauss]

There are 14 ffi-napi issues on this repo.

Perhaps hints for a solution could be found by studying them.

[guocaoyi]

so add a whitelist to ignore links out of the package ??

[levpopov]

Thanks for looking at this. This issue also causes some impressive side-effects when using electron-forge and osx codesign. Because node-gyp leaves behind symlinks to python3, electron-builder will use osx codesign to sign the python binary while processing the app, breaking your system python install (because of resulting sig mismatch).

I am wiping node_gyp_bins manually in a build/package hook as a workaround for now, but would be great to get this resolved properly.

[jagthedrummer]

Would love to see a fix for this.

[nipunn1313]

Hello. Thanks for the good work on node-gyp. I wanted the check in on this.

I was finding that the rush package manager's rush deploy command (reasonably) does not like the symlink out of node_modules because it is non-hermetic (pointing out of node-modules). Similar in spirit to the codesign rule discussed above. Would love to see a fix (like #2721) to avoid leaving symlinks pointing out of node_modules.

[cclauss]

When you want a pull request to be merged, please give it a positive review as @DeeDeeG has done at the top right of this page. Every checkmark ✔️ that project maintainers see there gives them confidence that the proposed changes have been looked at and have been deemed both useful and safe to merge into the codebase. Lots of "what is the ETA?" comments are easier for maintainers to ignore than ✔️✔️✔️✔️✔️ from several different reviewers.

Anyone can review a pull request on GitHub. To do so here:

1. Scroll to the top of the pull request page.
2. Click the Files changed tab.
3. Click the Review changes button.
4. Click Approve (or one of the other options) and make comments only if they have not already been stated in the PR.
5. Click Submit review so that your ✔️ can be added to the list.

[nipunn1313]

When you want a pull request to be merged, please give it a positive review as @DeeDeeG has done at the top right of this page. Every checkmark ✔️ that project maintainers see there gives them confidence that the proposed changes have been looked at and have been deemed both useful and safe to merge into the codebase. Lots of "what is the ETA?" comments are easier for maintainers to ignore than ✔️✔️✔️✔️✔️ from several different reviewers.

Sounds good! Happy to help! I know not all maintainers like this, but good to know here.

[cclauss]

I know not all maintainers like this

I have never seen that. If they want to turn off the code review capability for non-maintainers then they are free to do so.