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

Add a warning when run as root (e.g., sudo pip) #9394

Merged
merged 5 commits into from
Mar 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/6409.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a warning, discouraging the usage of pip as root, outside a virtual environment.
4 changes: 2 additions & 2 deletions src/pip/_internal/cli/base_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ def _main(self, args):
"The directory '%s' or its parent directory is not owned "
"or is not writable by the current user. The cache "
"has been disabled. Check the permissions and owner of "
"that directory. If executing pip with sudo, you may want "
"sudo's -H flag.",
"that directory. If executing pip with sudo, you should "
"use sudo's -H flag.",
options.cache_dir,
)
options.cache_dir = None
Expand Down
31 changes: 31 additions & 0 deletions src/pip/_internal/cli/req_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import logging
import os
import sys
from functools import partial
from optparse import Values
from typing import Any, List, Optional, Tuple
Expand Down Expand Up @@ -38,6 +39,7 @@
TempDirectoryTypeRegistry,
tempdir_kinds,
)
from pip._internal.utils.virtualenv import running_under_virtualenv

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -152,6 +154,35 @@ def handle_pip_version_check(self, options):
]


def warn_if_run_as_root():
# type: () -> None
"""Output a warning for sudo users on Unix.

In a virtual environment, sudo pip still writes to virtualenv.
On Windows, users may run pip as Administrator without issues.
This warning only applies to Unix root users outside of virtualenv.
"""
if running_under_virtualenv():
return
if not hasattr(os, "getuid"):
return
# On Windows, there are no "system managed" Python packages. Installing as
# Administrator via pip is the correct way of updating system environments.
#
# We choose sys.platform over utils.compat.WINDOWS here to enable Mypy platform
# checks: https://mypy.readthedocs.io/en/stable/common_issues.html
if sys.platform == "win32" or sys.platform == "cygwin":
winsonluk marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we doing this for cygwin? I'd have imagined that cygwin counts as a "system package manager" for the purposes of a cygwin build of Python. But having said that, I know very little about cygwin and I'm happy to assune it's OK unless cygwin users complain that they don't like it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s been a looong while since I last used Cygwin, but IIRC it does not have a notion of superusers, so it makes sense to skip the checks in it. But it’s probably a good idea to not over-anticipate things and wait for complaints instead, since this is just a line of message anyway.

return
if sys.platform == "darwin" or sys.platform == "linux":
if os.getuid() != 0:
winsonluk marked this conversation as resolved.
Show resolved Hide resolved
return
logger.warning(
"Running pip as root will break packages and permissions. "
"You should install packages reliably by using venv: "
"https://pip.pypa.io/warnings/venv"
)


def with_cleanup(func):
# type: (Any) -> Any
"""Decorator for common logic related to managing temporary
Expand Down
7 changes: 6 additions & 1 deletion src/pip/_internal/commands/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@
from pip._internal.cache import WheelCache
from pip._internal.cli import cmdoptions
from pip._internal.cli.cmdoptions import make_target_python
from pip._internal.cli.req_command import RequirementCommand, with_cleanup
from pip._internal.cli.req_command import (
RequirementCommand,
warn_if_run_as_root,
with_cleanup,
)
from pip._internal.cli.status_codes import ERROR, SUCCESS
from pip._internal.exceptions import CommandError, InstallationError
from pip._internal.locations import get_scheme
Expand Down Expand Up @@ -443,6 +447,7 @@ def run(self, options, args):
options.target_dir, target_temp_dir, options.upgrade
)

warn_if_run_as_root()
return SUCCESS

def _handle_target_dir(self, target_dir, target_temp_dir, upgrade):
Expand Down
3 changes: 2 additions & 1 deletion src/pip/_internal/commands/uninstall.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from pip._vendor.packaging.utils import canonicalize_name

from pip._internal.cli.base_command import Command
from pip._internal.cli.req_command import SessionCommandMixin
from pip._internal.cli.req_command import SessionCommandMixin, warn_if_run_as_root
from pip._internal.cli.status_codes import SUCCESS
from pip._internal.exceptions import InstallationError
from pip._internal.req import parse_requirements
Expand Down Expand Up @@ -88,4 +88,5 @@ def run(self, options, args):
if uninstall_pathset:
uninstall_pathset.commit()

warn_if_run_as_root()
return SUCCESS