-
-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
gh-57684: Add -P cmdline option and PYTHONSAFEPATH env var #31542
Conversation
https://docs.python.org/3/using/cmdline.html#cmdoption-c should be updated. |
Thanks for this! This is a long-awaited and very useful change. I was going to suggest fixes to some grammar errors and phrasing issues in the docs that were added, as well as an issue with the description in the
and it is otherwise ready for review? I'd be happy to help with that too, but it looks like a pretty small change, just updating it to reflect that an empty string rather than the working dir is added to |
FTR I consider Don't add |
Yep, I noticed that as well above. I can't claim either of your expertise, but I'm not a beginner either and have actively been following the progress of and looking forward to this feature, and yet I had to carefully re-read the linked issue to be sure that it was actually doing what I thought it was. As both a developer and a documentarian, it can be easy for me to write documentation (or PEPs, etc) that makes perfect sense to me but is confusing to others without deep implicit knowledge of the code—I've seen both sides of that a lot lately, as well as the value of having someone else look over it. |
Awesome! Is there an associated environment variable? |
It would appear there isn't, as of yet. |
I rebased my PR and marked it as ready for review ;-) |
Can we add an associated environment variable? |
Since different people asked for an environment variable, I changed my mind and I added
|
Doc/using/cmdline.rst
Outdated
Don't add :data:`sys.path[0] <sys.path>`: don't prepend the current working | ||
directory (``python -m module``), an empty string (``python -c code``) or | ||
the first command line argument (``python script.py``) to :data:`sys.path`. |
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.
Don't add :data:`sys.path[0] <sys.path>`: don't prepend the current working | |
directory (``python -m module``), an empty string (``python -c code``) or | |
the first command line argument (``python script.py``) to :data:`sys.path`. | |
Do not prepend the initial working directory (``python -m module``), the | |
current working directory (``python -c code``), or the directory of the | |
executed script (``python script.py``) to :data:`sys.path`. |
I think saying "prepend" suffices instead of talking about adding sys.path[0]
.
With the -m
option, sys.path[0]
is the full path of the initial working directory. With the -c
option and the REPL, the current working directory is prepended to sys.path
. It's an implementation detail that it's represented in this case as an empty string. It's not exactly an obscure implementation detail, since most people know that normpath('')
-> '.'
, but I think it's better to be explicit about the behavior.
Doc/using/cmdline.rst
Outdated
@@ -583,6 +599,14 @@ conflict. | |||
within a Python program as the variable :data:`sys.path`. | |||
|
|||
|
|||
.. envvar:: PYTHONDONTADDPATH0 |
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.
You're emphasizing sys.path[0]
again. There's always a sys.path[0]
. How about PYTHONUSESAFEPATH
?
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.
How does the doc call the value that is prepended? Main script base directory? Could be PYTHONDONTADDMAINDIR
or PYTHONNOMAINDIR
.
(I wondered about including IMPORT
or PATH
but then you get something clunky or you add prepositions and get too long)
Proposed names so far (on python-dev and this PR), I add spaces for readability:
|
Proposed long option names (in the issue):
|
Perl 5.26 no longer includes the current working directly by default in
Perl also has a "taint mode" enabled by the Perl |
I updated my PR: it now uses the
|
I love PYTHONSAFEPATH! |
I'm not a fan of the "safe path" wording. Things can be "safe" or "unsafe" in many ways and the word doesn't give a clear indication of what the precise behavior is. Would something like "skip_cwd_in_path" work? |
It wouldn’t, because as detailed before it’s not always the working dir that’s added. |
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.
I'm not a fan of the "safe path" wording. Things can be "safe" or "unsafe" in many ways and the word doesn't give a clear indication of what the precise behavior is.
Same reservations here. What about changing the sense of the name, to PYTHONUNSAFEPATH
/sys.flags.unsafe_path
with a default of True
? Not a strong enough feeling to be blocking, though :)
@gpshead: Would you mind to review the updated PR? |
@hroncok: Thanks, I addressed your review on the doc. |
That's the bit that makes me squirm, though :). It makes things less potentially unsafe, but I can well imagine someone coming along in 5 years with some new contrivance that shows that no, in fact, the path is not completely safe with Maybe make it I fully recognize that this is just bikeshedding, though. Don't hold this up on quibbles about future maybes :) |
Maybe we should consider adding For example, Otherwise, |
For example, |
These tools are exceptions; they could add their parent directory to sys.path explicitly. |
I think that should be left for the future. Lets see if anyone actually wants it first. If we do go the route of aiming for a default flip in the future it might make sense. |
I'd be very much in favor of such a change 🙂 |
PYTHONSAFEPATH=1 is not safe at all :-) It does exactly one thing: not preprend a path to sys.path[0] which is "potentially" unsafe. It's just that it's really hard to select a short environment name which either summarizes well what the variable does, or explains in length what it does. For example, the |
Flipping the default can now be done by setting The problem is that right now, if IMO command line options must have the priority over environment variables. Python does not provide command line options to disable the effect of all Python environment variables. For example, The Python UTF-8 Mode is way more likely to cause compatibility issues and so We don't have to add |
I merged my PR, thanks @gpshead for the review. I'm not fully comfortable of merging this PR in a rush (before the Python 3.11 feature freeze), but IMO the scope of the feature is limited and well defined. It's different than other options discussed previously, especially the idea of changing the default behavior to "safe" (not prepend a path to sys.path). The idea of putting the feature is beta1 is to give users the ability to play with it before Python 3.11 final release, to get feedback, adjust issues, and maybe consider reverting the change if it causes too many issues. |
As written previously, you can now simply set PYTHONSAFEPATH=1 env var system wide, and Python 3.11 sys.path will be "safer" by default ;-) On Linux, just put it in |
Follow-up change: 329afe7 "Fix tests failing with the PYTHONSAFEPATH=1 env var". |
I created #92361 to add a new |
to sys.path.
the -P command line option.
present.
https://bugs.python.org/issue13475