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

feat: inherit PYTHONSAFEPATH env var from outer process #2076

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

rickeylev
Copy link
Contributor

@rickeylev rickeylev commented Jul 18, 2024

By default, PYTHONSAFEPATH is enabled to help prevent imports being found where they
shouldn't be. However, this behavior can't be disabled, which makes it harder to use
a py_binary when the non-safe path behavior is explicitly desired.

To fix, the bootstrap now respects the caller environment's PYTHONSAFEPATH environment variable, if set. This allows the callers to set PYTHONSAFEPATH= (empty string) to
override the default behavior that enables it.

Fixes #2060

@rickeylev rickeylev requested a review from aignas as a code owner July 18, 2024 18:39
@rickeylev rickeylev changed the title feat: allow overriding PYTHONSAFEPATH env var feat: inherit PYTHONSAFEPATH env var from outer process Jul 18, 2024
@rickeylev rickeylev enabled auto-merge July 19, 2024 15:44
@rickeylev rickeylev added this pull request to the merge queue Jul 19, 2024
Merged via the queue into bazelbuild:main with commit ecad092 Jul 19, 2024
4 checks passed
@allsey87
Copy link

I will check if this solves #2060, but I have a hunch that just setting PYTHONSAFEPATH as empty (which is required for the opt-out to work) will still be enough to trigger this behavior in Python.

@rickeylev
Copy link
Contributor Author

One of the tests does exactly that and checks the interpreter flag to verify that safe path is disabled, so it should work :). Feel free to re-open the issue with a repro if you find otherwise.

Actually, I'm going to re-open the issue for now. This only fixes it for --bootstrap_impl=script, not windows, and, come to think of it, certain types of zips might also still have the bug.

@rickeylev rickeylev deleted the feat.allow.env.override branch July 19, 2024 19:00
@rickeylev
Copy link
Contributor Author

Doh, this introduced a bug that disable safe path by default unless it was opted in to. That'll be fixed shortly in #2073.

github-merge-queue bot pushed a commit that referenced this pull request Jul 19, 2024
Previously, all the user import paths were put at the end of sys.path.
This was done so that
user import paths didn't hide stdlib modules. However, a side-effect is
that user import
paths came after the runtime's site-packages directory. This prevented
user imports from
overriding non-stdlib modules included in a runtime (e.g. pip).

To fix, we look for the runtime site-packages directory, then insert the
user import paths
before it. A test is used to ensure that the ordering is `[stdlib, user,
runtime site-packages]`

Also fixes a bug introduced by #2076: safe path was being disabled by
default

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

Successfully merging this pull request may close these issues.

Add an option to opt-out of PYTHONSAFEPATH
3 participants