-
Notifications
You must be signed in to change notification settings - Fork 558
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
Add profile for npm #3866
Add profile for npm #3866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, some nitpicks to go trough. Furthermore, can we add
disable-exec.inc
(with exception for${HOME}
)whitelist-runuser-common.inc
whitelist-var-common.inc
?
* Remove redundant blacklisting of Wayland. * Remove unnecessary noblacklist lines for nodejs. * Replace absolute paths to .inc files with filenames. * Remove unneeded dbus whitelisting. Co-authored-by: rusty-snake <41237666+rusty-snake@users.noreply.github.com>
To keep consistent with other profiles, remove the blank line after the header comment. Co-authored-by: rusty-snake <41237666+rusty-snake@users.noreply.github.com>
So that our addition of npm paths to disable-programs.inc dose not break IDEs, we need to unblacklist these same paths in allow-common-devel.inc.
Include disable-exec.inc, but allowing ${HOME}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitelisting in $HOME is likely too much, rest looks fine.
whitelist-common breaks npm, and since we don't know where the user's npm projects will be, leave the whitelist-common include in a comment with a note about how to enable it for their setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, added a few nitpicks.
I have been trying to create a profile for npm (npx & yarn) for a while now. I find it rather hard to have it cover several personal npm use-cases. Would it be possible to show a fully working example here? This is not a critique, sandboxing npm is a very welcome profile. I'm just wondering if it's not going to create a lot of confusion for users when things start to break due to the vast amount of varied implementations out there.
Example of a project that builds fine with a firejailed npm:
- https://github.com/mozilla/web-ext.git
Examples of projects that throw all kinds of obstacles in the firejail sandboxing of npm:
- https://github.com/bitwarden/browser.git
- https://github.com/minbrowser/min.git
- https://github.com/openstyles/stylus.git
@@ -0,0 +1,60 @@ | |||
# Firejail profile for npm | |||
# Description: The Node.js Package Manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to add quiet here to avoid making npm's already pretty verbose output even noisier.
noblacklist ${HOME}/.npm | ||
noblacklist ${HOME}/.npmrc | ||
|
||
noblacklist ${PATH}/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only allow these from disable-shell.inc? IMHO it would make more sense to drop these individual paths and remove 'include disable-shell.inc' below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If just /bin/sh
is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my use of this profile, /bin/sh
was insufficient, as some packages seemed to use /bin/bash
. The other shells do not seem to be used anywhere near as commonly for build scripts, if ever.
include disable-exec.inc | ||
include disable-passwdmgr.inc | ||
include disable-programs.inc | ||
include disable-shell.inc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See earlier comment.
It's not in firecfg.config. So it just breaks if you add it and then you know enough to adapt your workflows. IMHO it's good to provide a strict template which can be relaxed from users as they need. |
Co-authored-by: rusty-snake <41237666+rusty-snake@users.noreply.github.com>
@rusty-snake Agreed.
@aidalgol That's a fair point. In the mean time I re-did some intensive testing of this profile and it looks fine. Had something in my globals.local that kept breaking npm so scratch my earlier examples. For the record, I could add For using it as a whitelist profile though I needed to add our 'regular' mkdir/mkfile/whitelist foo routine. Without those my ${HOME}/.npmrc was not getting respected. So here's a suggestion to incorporate into this npm.profile:
|
I can't provide a working example that would be easy for anyone to try out, but I have used my original profile (before the suggested changes were made) for gatsby projects, a web-extension project, and some miscellaneous frontend stuff. |
To make it consistent with the other include profiles. See etc/templates/profile.template. Relates to netblue30#3866 netblue30#5881.
No description provided.