Update nodejs-common: enable npx, clarify #4172
Conversation
| ignore noexec ${HOME} | ||
|
|
||
| # Required to run `npx` |
There was a problem hiding this comment.
Not sure I understand the reasoning here. We already have noblacklist ${HOME}/.npm in npm.profile. Is npx not working without repeating it here?
There was a problem hiding this comment.
I don't think npm.profile is ever read. cat $(which npx) shows it's a script starting with #!/usr/bin/env node.
There was a problem hiding this comment.
I don't think npm.profile is ever read. cat $(which npx) shows it's a script starting with #!/usr/bin/env node.
Indeed it is. In fact, both /usr/bin/npm and /usr/bin/npx are symlinks to scripts with the node shebang (at least that's what I see on Arch Linux). Wouldn't that imply we need a npx.profile too, as well as a node.profile? Just asking, I'm not at all familiar with the Node.js stack.
There was a problem hiding this comment.
Regardless of the above remark, if we add noblacklist ${HOME}/.npm here, we should drop it from the npm.profile. Can you take care of that please? It's the only thing that needs to be done to merge this.
There was a problem hiding this comment.
npx being a node script, means firejail will read node.profile, not npm.profile.
Wouldn't that imply we need a npx.profile too, as well as a node.profile
Correct. Or, we could remove npm.profile, and only create a node.profile which would be used for node, npm and npx. Though @rusty-snake pointed out in #4085 that there's only nodejs-common.profile, so I suppose there might be a good reason for that.
if we add noblacklist ${HOME}/.npm here, we should drop it from the npm.profile. Can you take care of that please?
That would be moot if we get rid of npm.profile. @aidalgol ?
There was a problem hiding this comment.
That would be moot if we get rid of npm.profile. @aidalgol ?
It is not moot for a PR that adds it, which this does right?. Please let's not move ahead of ourselves and stick to the PR itself.
There was a problem hiding this comment.
That would be moot if we get rid of
npm.profile. @aidalgol ?
Sorry, I am not familiar enough with firejail's general policies and strategies to comment.
There was a problem hiding this comment.
Removed the private-tmp comment...
Did you mean the private-etc comment?
Is npx not working with the noblacklist ${HOME}/.npm from npm.profile?
Looking at npm.profile again, would it make more sense to move most of that into nodejs-common.profile (lines 10 to 26)?
Sorry, I forgot to remove the
Maybe; could we table this for later? |
No problem. It makes sense now.
Sure. I just wanted to add a few general remarks to the way we handle the Node.js stack in general (which IMO is still a bit of a mess). Apologies for digressing. The PR itself is LGTM. |
| # May need to add `passwd` to `private-etc` below to enable debugging with some IDEs | ||
| private-etc alternatives,ca-certificates,crypto-policies,host.conf,hostname,hosts,ld.so.cache,ld.so.conf,ld.so.conf.d,ld.so.preload,locale,locale.alias,locale.conf,localtime,login.defs,mime.types,nsswitch.conf,pki,protocols,resolv.conf,rpc,services,ssl,xdg | ||
| # May need to be commented out in order to enable debugging with some IDEs | ||
| private-tmp | ||
| # Pass-through passwd because it's required to run `npx` | ||
| private-etc alternatives,ca-certificates,crypto-policies,host.conf,hostname,hosts,ld.so.cache,ld.so.conf,ld.so.conf.d,ld.so.preload,locale,locale.alias,locale.conf,localtime,login.defs,mime.types,nsswitch.conf,passwd,pki,protocols,resolv.conf,rpc,services,ssl,xdg |
There was a problem hiding this comment.
- Why only adding a reason for passwd? What about login.defs, xdg, ...? IMHO we should just omit the "Pass-through ...".
- I don't understand what's the issue with
private-tmpis.
There was a problem hiding this comment.
I just have no idea why login.defs and the other ones are necessary. Whoever added them should've added a reason :)
private-tmp was necessary in webstorm.profile to be able to debug Node script. Back then, I added it by mistake to nodejs-common.profile as well, but it has no effect there when Node is being debugged by WebStorm.
There was a problem hiding this comment.
I just have no idea why login.defs and the other ones are necessary. Whoever added them should've added a reason :)
Our code is full of stuff like that. Look at other profiles that have login.defs and xdg for background. Let's stick to the PR at hand please. This should have been a one-line thing. Again, I apologize for opening the door on the more general topic of how we handle the Node.js stack, but let's move on towards a merge.
There was a problem hiding this comment.
I just have no idea why login.defs and the other ones are necessary. Whoever added them should've added a reason :)
Raises the general question if we want to include reasons in profiles. There are a few edge-cases where something looks complete random and a comment makes sense, however passwd is included in 22,86% of our private-etc lines. And there are a lot thing/librarys/progams/... which have trouble w/o it. Do we want profile look like the below?
click me
# Firejail profile for apostrophe
# Description: Distraction free Markdown editor for GNU/Linux made with GTK+
# This file is overwritten after every install/update
# Persistent local customizations
include apostrophe.local
# Persistent global definitions
include globals.local
# apostrophe is an editor and viewer for markdown document, allowing ${DOCUMENT} ...
noblacklist ${DOCUMENTS}
# allow ${PICTURES} because apostrophe can include pictures in documents
noblacklist ${PICTURES}
# Lua is required by pandoc
# Allow lua (blacklisted by disable-interpreters.inc)
include allow-lua.inc
# written in python
# Allow python (blacklisted by disable-interpreters.inc)
include allow-python3.inc
# needs no access here
include disable-common.inc
# isn't an IDE
include disable-devel.inc
# see above
include disable-exec.inc
# no other interpreters are used
include disable-interpreters.inc
# isn't a PWMGR
include disable-passwdmgr.inc
# unlikely that makrdownfiles are save there, disbale for security
include disable-programs.inc
# can be used w/o a shell
include disable-shell.inc
# disable the remains XDG dirs
include disable-xdg.inc
# gresource are stored there
whitelist /usr/share/apostrophe
# preview is broken without it
whitelist /usr/share/pandoc-*
# enable wruc for better sanboxing
include whitelist-runuser-common.inc
# wusc is used
include whitelist-usr-share-common.inc
# restrict access to /var
include whitelist-var-common.inc
# make the sandbox stronger
apparmor
# unpriv program
caps.drop all
# apostrophe can't play sound
machine-id
# disable network access for better security and privacy
net none
# no 3d rendering
no3d
# editing files on a ro device make no sense
nodvd
# no supplementary groups are requeired
nogroups
# no suid involved
nonewprivs
# needs no root
noroot
# apostrophe can't play sound
nosound
# apostrophe can't play tv
notv
# no login functionality at all
nou2f
# Disable spying
novideo
protocol unix
seccomp
shell none
# log blacklist violations to detect problems
tracelog
disable-mnt
# apostrophe is written in python and uses pandoc for converting markdown documents
private-bin apostrophe,pandoc,python3*
private-cache
private-dev
private-etc alternatives,dconf,fonts,gtk-3.0,ld.so.cache,ld.so.conf,ld.so.conf.d,ld.so.preload,pango,X11
private-tmp
dbus-user filter
dbus-user.own org.gnome.gitlab.somas.Apostrophe
# settings are imutable w/o it
dbus-user.talk ca.desrt.dconf
dbus-system none
I know this example is a bit extreme.
I added it … to nodejs-common.profile …, but it has no effect there when Node is being debugged by WebStorm.
and if it is not started by WebStorm ...
I would prefer to keep it. And if you say it shouldn't enable by default because of foobar we can still comment it.
Anyway if @glitsj16 is fine with is PR we can merge, I've even less is the npm stack.
There was a problem hiding this comment.
I just have no idea why login.defs and the other ones are necessary. Whoever added them should've added a reason :)
Raises the general question if we want to include reasons in profiles.
I honestly have no idea why I needed login.defs for npm to work (with private-etc), and I would not have the foggiest idea where to start trying to find out.
There was a problem hiding this comment.
I honestly have no idea why I needed login.defs for npm to work (with private-etc), and I would not have the foggiest idea where to start trying to find out.
That's quite understandable. If your uid is 1000 (the default on most OS'es) things will work without the need for login.defs. BUT we cannot assume that each and every Firejail user runs with uid=1000. That's why it is important to check and test our profiles, regardless of the user's uid. We don't nitpick for fun ;)
There was a problem hiding this comment.
If your uid is 1000 (the default on most OS'es) things will work without the need for login.defs. BUT we cannot assume that each and every Firejail user runs with uid=1000. That's why it is important to check and test our profiles, regardless of the user's uid. We don't nitpick for fun ;)
I actually was running as UID 1000, and I still needed login.defs for npm to work. ¯\_(ツ)_/¯
|
@tredondo I went ahead and merged a node.js stack refactoring. Hopefully that also fixes your webstorm issue. It would be great if you could |
Settings to enable npx (bundled with node).
Changed the order a bit to match the profile.template sections.
Removed the private-tmp comment because if an IDE debugs Node, then Node inherits the sandbox from the IDE. I wrote that mistaken PR early on when I didn't understand this aspect of firejail well.