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

os.userInfo().shell is '' if no shell in /etc/passwd #15684

Closed
gibfahn opened this issue Sep 29, 2017 · 4 comments
Closed

os.userInfo().shell is '' if no shell in /etc/passwd #15684

gibfahn opened this issue Sep 29, 2017 · 4 comments
Labels
os Issues and PRs related to the os subsystem.

Comments

@gibfahn
Copy link
Member

gibfahn commented Sep 29, 2017

  • Version: master
  • Platform: Linux (Ubuntu 14.04)
  • Subsystem: os

Continuation of libuv/libuv#1570

Basically this line fails:

assert.ok(pwd.shell.includes(path.sep));

because os.userInfo().shell is ''. This is because os.userInfo() calls uv__getpwuid_r() which calls getpwuid_r(3), which reads the relevant line from /etc/passwd:

gib:x:1234:3210:Gibson Fahnestock:/home/gib:

However the user shell on this machine is not defined in /etc/passwd, so we get a default.

The question is whether we should just update the test (remove this check, so don't actually check that os.userInfo().shell actually returns anything, or whether os.userInfo().shell should handle this case.

I guess it comes down to whether this function's purpose is just "read the shell from /etc/passwd", or "tell you which shell the user is using". If it's the former we remove the test, if it's the latter we should check either the default shell, or process.env.SHELL.

The default shell is either just defaulting to /bin/sh, or it's coming from /etc/adduser.conf:

# The DSHELL variable specifies the default login shell on your system
DSHELL=/bin/bash
@gibfahn gibfahn added the os Issues and PRs related to the os subsystem. label Sep 29, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Sep 29, 2017

A few comments. First, the os.userInfo() docs state:

on POSIX platforms, this is typically a subset of the password file

So, I don't think it would be unreasonable to require that the information be in the password file.

remove this check, so don't actually check that os.userInfo().shell actually returns anything

The test could still check that a string is returned. It could even go further and perform the current check if the string has a length > 0.

if it's the latter we should check either the default shell, or process.env.SHELL

I'm not saying that we should go this route, but we already do something like this with the user's home directory. os.userInfo() returns the home directory from the password file, while os.homedir() takes a few environment variables into account.

@shellberg
Copy link

This is potentially an Ubuntu/Debian behaviour.

At least, the typical Ubuntu user entry, based on (default) adduser creation, results in an empty shell specified in the /etc/passwd file (no shell specifically defined). Ubuntu's man page specifies that the agent setting the SHELL environment variable is login; where empty, the value will default to /bin/sh.

So, it seems it is possible to have an interactive shell without the export of SHELL on Ubuntu systems.

@silverwind
Copy link
Contributor

typical Ubuntu user entry, based on (default) adduser creation, results in an empty shell specified in the /etc/passwd file

Unrelated, but that sounds like a bug in adduser to me.

@gibfahn
Copy link
Member Author

gibfahn commented Oct 18, 2017

I'm not saying that we should go this route, but we already do something like this with the user's home directory. os.userInfo() returns the home directory from the password file, while os.homedir() takes a few environment variables into account.

That makes sense, so if people need something higher level to get the user's default shell we can just add an os.shell().

gibfahn added a commit to gibfahn/node that referenced this issue Oct 22, 2017
The shell in /etc/passwd can be blank, in which case the user is given
the default shell. Handle this by only checking the shell contains a
path separator if the string isn't empty.

PR-URL: nodejs#16287
Fixes: nodejs#15684
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Oct 23, 2017
The shell in /etc/passwd can be blank, in which case the user is given
the default shell. Handle this by only checking the shell contains a
path separator if the string isn't empty.

PR-URL: #16287
Fixes: #15684
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
The shell in /etc/passwd can be blank, in which case the user is given
the default shell. Handle this by only checking the shell contains a
path separator if the string isn't empty.

PR-URL: nodejs/node#16287
Fixes: nodejs/node#15684
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Nov 16, 2017
The shell in /etc/passwd can be blank, in which case the user is given
the default shell. Handle this by only checking the shell contains a
path separator if the string isn't empty.

PR-URL: #16287
Fixes: #15684
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Nov 21, 2017
The shell in /etc/passwd can be blank, in which case the user is given
the default shell. Handle this by only checking the shell contains a
path separator if the string isn't empty.

PR-URL: #16287
Fixes: #15684
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue Nov 28, 2017
The shell in /etc/passwd can be blank, in which case the user is given
the default shell. Handle this by only checking the shell contains a
path separator if the string isn't empty.

PR-URL: #16287
Fixes: #15684
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
The shell in /etc/passwd can be blank, in which case the user is given
the default shell. Handle this by only checking the shell contains a
path separator if the string isn't empty.

PR-URL: nodejs/node#16287
Fixes: nodejs/node#15684
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os Issues and PRs related to the os subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants