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

fish_is_root_user only checks $USER but not actual uid #8866

Closed
septatrix opened this issue Apr 8, 2022 · 11 comments
Closed

fish_is_root_user only checks $USER but not actual uid #8866

septatrix opened this issue Apr 8, 2022 · 11 comments

Comments

@septatrix
Copy link
Contributor

fish, version 3.4.1

The list for which the red root prompt is chosen is hardcoded and can lead to false positives and negatives.
Special purpose distros and containers sometimes use an uncommon root username which is not in the current list.
On the other side there might be users coming from windows which are used to having an "Administrator" account set up manually which will not have a uid of 0.

While this function is only used for prompts currently it can still be confusing.
Instead it should simply check if test (id -u) = 0 holds true.

@faho
Copy link
Member

faho commented Apr 8, 2022

Duplicate of #8583

@faho faho marked this as a duplicate of #8583 Apr 8, 2022
@faho faho closed this as completed Apr 8, 2022
@faho faho added the duplicate label Apr 8, 2022
@faho
Copy link
Member

faho commented Apr 8, 2022

On the other side there might be users coming from windows which are used to having an "Administrator" account set up manually which will not have a uid of 0.

If you have specialty needs you can override the function. That's why it's a function.

The bigger issue is $USER not matching up with the uid is why we should fix $USER.

@septatrix
Copy link
Contributor Author

No this is not a duplicate of the linked issue. Even if USER is correct it might not be in the list of root users root toor Administrator or a user might not be root yet still have a name from that list.

@faho
Copy link
Member

faho commented Apr 8, 2022

Again: If you have special needs you can modify the function.

Having a root user not in that list isn't something you typically encounter. Having an admin user with a UID that isn't 0 isn't something you typically encounter.

This is a simple, managable default that handles the majority of use cases. If you need more you can customize it.

@septatrix
Copy link
Contributor Author

Having a root user not in that list isn't something you typically encounter. Having an admin user with a UID that isn't 0 isn't something you typically encounter.

The former is indeed rare though the latter is impossible because every root user has a uid of 0 by definition(!).

Again: If you have special needs you can modify the function.
This is a simple, managable default that handles the majority of use cases. If you need more you can customize it.

That I know, though I think all users would benefit from a more precise function. And checking the uid would test for the very definition of being root.

@anordal
Copy link
Contributor

anordal commented Apr 15, 2022

septatrix has a point, though: He is proposing a logical simplification that is more correct. AFAICS, the drawback is only that id is not a builtin.

As for why it is more correct:
It depends on what you want to know, of course, but if you want to know whether you have the power of root, it's not the name that matters, but precisely the effective user id (as in geteuid()). To my surprise, that is indeed what id -u claims to print, according to its manpage.

@faho
Copy link
Member

faho commented Apr 15, 2022

The case of being root should simply be fixed by fixing the value of $USER - #8583.

That's because that is the thing you would print in your prompt if you printed the user. The prior value of $USER doesn't really matter in most cases, it's basically all the name related to the effective uid.

And if you have any other specialty needs, like you have a system where an admin user doesn't have a uid of 0 or a name that's not "root" or something similarly well-known, you can redefine the function for your specialty needs.

Which is why I'm calling this a duplicate of 8583.

@septatrix
Copy link
Contributor Author

Using the $USER is simply a Jacky workaround which works 98% of the time and what you proposed in 8583 is simply a workaround atop another workaround to bump that number to 99%. It still does not fix the underlying problem: The name of the user has no underlying causal connection to what is means to have root privileges.
Yes, there is a strong correlation in most distros but why can we not simply use the correct definition, please?

faho added a commit that referenced this issue Apr 15, 2022
@faho faho added this to the fish 3.5.0 milestone Apr 15, 2022
@faho
Copy link
Member

faho commented Apr 15, 2022

I have now added a bash-inspired $EUID variable that is used inside fish_is_root_user, which I believe should be enough.

I still plan on fixing $USER because I do not see a point in a $USER that doesn't reflect the permissions you actually have.

@septatrix
Copy link
Contributor Author

Thank you (though I think a read-only shell var might have been better—again trying to not touch the environment where possible and subprocesses could not trust and therefore use it anyhow).

Also the contains check should be removed to prevent false positives.

@faho
Copy link
Member

faho commented Apr 15, 2022

a read-only shell var might have been better

That breaks scripts that already use that name.

Also the contains check should be removed to prevent false positives.

The contains check remains so this doesn't break on older fishes. False positives are quite unlikely given these are names like "root" and we already update $USER if uid is 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants