Opened 3 years ago
Closed 3 years ago
#29394 closed defect (fixed)
Drop the python2 patch sys_path_security-issue_16202
Reported by: | mkoeppe | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.1 |
Component: | packages: optional | Keywords: | |
Cc: | vbraun, jdemeyer, embray, slelievre, gh-timokau, fbissey, arojas, nthiery, isuruf, pcpa, thansen, infinity0, snark, cschwan, jhpalmieri | Merged in: | |
Authors: | Matthias Koeppe, John Palmieri | Reviewers: | François Bissey, Dima Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | cbdce1c (Commits, GitHub, GitLab) | Commit: | cbdce1cef6350482d9ec65c878188e0f6530bc22 |
Dependencies: | #25358 | Stopgaps: |
Change History (32)
comment:1 Changed 3 years ago by
Cc: | jhpalmieri added |
---|
comment:2 Changed 3 years ago by
Branch: | → u/mkoeppe/drop_the_python2_patch_sys_path_security_issue_16202 |
---|
comment:3 Changed 3 years ago by
Authors: | → Matthias Koeppe |
---|---|
Commit: | → 1a578e4e520b1becde150ff7ca1ae61bf85296d9 |
Status: | new → needs_review |
comment:4 Changed 3 years ago by
Reviewers: | → François Bissey |
---|---|
Status: | needs_review → positive_review |
Good riddance.
comment:5 Changed 3 years ago by
Status: | positive_review → needs_work |
---|
comment:6 Changed 3 years ago by
Branch: | u/mkoeppe/drop_the_python2_patch_sys_path_security_issue_16202 → u/jhpalmieri/drop_the_python2_patch_sys_path_security_issue_16202 |
---|
comment:7 Changed 3 years ago by
Commit: | 1a578e4e520b1becde150ff7ca1ae61bf85296d9 → fd4e4086d60f3b7212bcbee03e0f360ecd8d3a05 |
---|---|
Status: | needs_work → needs_review |
Doctest failures with a Python 2 build, so let's get rid of the offending doctests.
New commits:
fd4e408 | trac 29394: get rid of corresponding doctest
|
comment:8 Changed 3 years ago by
Status: | needs_review → positive_review |
---|
Hum, I thought it was going to be handled in a different ticket, but yes that test should go, of course. And that's the only place I know where the issue would crop up.
comment:9 follow-up: 10 Changed 3 years ago by
Do we need to delete the function test_safe_directory
in doctest/control.py
?
comment:10 Changed 3 years ago by
Replying to jhpalmieri:
Do we need to delete the function
test_safe_directory
indoctest/control.py
?
I wouldn't think it is needed. After all it works well with python without change. Let me double check.
comment:11 Changed 3 years ago by
Status: | positive_review → needs_work |
---|
To partially answer my own question: that function gives another doctest failure with a Python 2 build. Its docstring also says "This is only relevant with Python 2, because Sage's Python 2 is patched to give a warning when the current directory is unsafe, but Python 3 is not." So I think we should remove the function entirely.
comment:12 Changed 3 years ago by
Oh right, I am forgetting we are still doing py2 builds here. Yes it should go.
comment:13 Changed 3 years ago by
I'll provide a branch which removes it. If you're happy with that, once I run tests successfully with Python 2, I can set to positive review on your behalf.
comment:15 Changed 3 years ago by
Commit: | fd4e4086d60f3b7212bcbee03e0f360ecd8d3a05 → c1f6dcded4ce720d67c0be89b44d40a76bc6eef3 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
c1f6dcd | trac 29395: remove test_safe_directory
|
comment:16 follow-up: 17 Changed 3 years ago by
comment:17 follow-up: 21 Changed 3 years ago by
Replying to fbissey:
Replying to mkoeppe:
#25358 has a better fix for test_safe_directory
If you want to try to revive that, you can. It can be done on #25358, with this ticket as a prerequisite, since one of the main obstacles there was the patched Python. Once we remove this patch to Python, the current incarnation of test_safe_directory
is no longer relevant, as far as I can tell.
So, that's why I thought that stuff was dealt with in another ticket. I am not completely losing my marbles.
That ticket had completely stalled, though.
comment:18 Changed 3 years ago by
Thanks John, we have to make a decision on what we do with #25358 (and how fast) before we proceed with the current changes. Plain removal is attractive to me. Less code.
comment:19 Changed 3 years ago by
I think test_safe_directory is a useful security mechanism. Its current implementation depends on the patch that we are now dropping (and hence on Python2)
comment:20 Changed 3 years ago by
Dependencies: | → #25358 |
---|
comment:21 Changed 3 years ago by
comment:22 Changed 3 years ago by
Branch: | u/jhpalmieri/drop_the_python2_patch_sys_path_security_issue_16202 → u/mkoeppe/drop_the_python2_patch_sys_path_security_issue_16202 |
---|
comment:23 Changed 3 years ago by
Authors: | Matthias Koeppe → Matthias Koeppe, John Palmieri |
---|---|
Commit: | c1f6dcded4ce720d67c0be89b44d40a76bc6eef3 → cbdce1cef6350482d9ec65c878188e0f6530bc22 |
Status: | needs_work → needs_review |
comment:24 Changed 3 years ago by
comment:25 Changed 3 years ago by
If we want, we can of course keep *both* tests in #25358. Keeping py2 as "secure" as it was; and adding security to py3.
comment:26 follow-up: 27 Changed 3 years ago by
We can't keep the secure py2 test if we're getting rid of the patch, or am I misunderstanding you?
comment:27 Changed 3 years ago by
Replying to jhpalmieri:
We can't keep the secure py2 test if we're getting rid of the patch, or am I misunderstanding you?
Right, it would be purely decorative or for the hypothetical situation that someone decides to run sagelib with a patched python2.
comment:28 Changed 3 years ago by
Testing this branch at https://github.com/mkoeppe/sage/actions/runs/61077298 (on top of other tickets)
comment:29 Changed 3 years ago by
This passes tests for me on OS X with builds based on Python 2 and Python 3.
comment:30 Changed 3 years ago by
Reviewers: | François Bissey → François Bissey, Dima Pasechnik |
---|---|
Status: | needs_review → positive_review |
lgtm
comment:32 Changed 3 years ago by
Branch: | u/mkoeppe/drop_the_python2_patch_sys_path_security_issue_16202 → cbdce1cef6350482d9ec65c878188e0f6530bc22 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
New commits:
Remove python2 patch sys_path_security-issue_16202