Fix virtualenv extraction from activate for newer virtualenvs#35
Fix virtualenv extraction from activate for newer virtualenvs#35
Conversation
| result = subprocess.run(["bash", "-c", f"source {activate_path} && echo $VIRTUAL_ENV"], capture_output=True, text=True) | ||
| return result.stdout.strip() |
There was a problem hiding this comment.
this introduces several arbitrary code execution vulnerabilities:
activate_pathis not properly quoted- the activate script itself is arbitrary code and may not be a virtualenv activate script
There was a problem hiding this comment.
Is parsing then still the best option even though its brittle?
There was a problem hiding this comment.
that is the only approach I would take personally -- you'll still have problems with substitution either way
There was a problem hiding this comment.
In #36, I'm doing a str.replace for the virtualenv path. Because the bash script now has 3 lines to update, the existing regex doesn't cover all the cases. Though, maybe I should just create 2 more regexes to be more explicit about the replacement.
| activate_path | ||
| ) | ||
|
|
||
| result = subprocess.run(["bash", "-c", f"source {activate_path} && echo $VIRTUAL_ENV"], capture_output=True, text=True) |
There was a problem hiding this comment.
You might want to consider using shlex.quote on the activate_path. I think this will fail for paths with spaces or other special characters in them.
|
As the tests show, sourcing the script doesn't work in the general case. The changes in virtualenv give |
|
Closing in favor on #36 |
5 participants
Problem
pypa/virtualenv#2996 (
virtualenv==20.36.0) changes how theVIRTUAL_ENVvariable is defined in the bash activate script. Our tool explicitly reads and parses the activate script for the value ofVIRTUAL_ENV=, which no longer works with the change and breaks withFixes #34.
Solution
The parsing is very brittle against changes to the script. Instead, we explicitly source the script and extract the value of the exported env variable.
Validation
We added a new test
test_get_orig_path. During development, we tested against both older and newer virtualenv. Our change is compatible with both so we don't need to explicitly test different virtualenvs.Note
HLFH in #34 provided their own patch in their fork for this change in HLFH@4971532, but it seems to be targeting a different form of the activation script.