-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
[3.11] gh-123270: Replaced SanitizedNames with a more surgical fix. (GH-123354) #123425
Conversation
…fix. (pythonGH-123354) Applies changes from zipp 3.20.1 and jaraco/zippGH-124 (cherry picked from commit 2231286) Co-authored-by: Jason R. Coombs <[email protected]>
|
||
Multiple separators are treated like a single. | ||
|
||
>>> list(_ancestry('//b//d///f//')) |
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.
Sorry if this should be obvious, but is this doctest run as part of the test suite? If not, wouldn't that mean removing //two-slash.txt
from the test_malformed_paths
test means the security fix for the infinite loop doesn't have test coverage in 3.8..3.11?
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.
Good question. Thanks for asking. I thought about this too.
The doctests are not run as part of the test suite. Perhaps that's something worth pursuing, but not something I've yet invested time or energy on. Since I'm the primary or sole maintainer on these projects that are preferably developed upstream in the third-party repos, I'm comfortable relying on the doctests, but you're absolutely right that it's a risk. There are other tests too, like "test_complexity" that exist only in the third-party repo (due to the dependency on other third-party packages to perform the tests).
I had thought that the //two-slash.txt
was still part of the unit test, but I see now, looking at the diff, that it is indeed missing. Looking at the same test in zipp, the lines are still present, so I need to assess where those lines went missing.
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.
As your comment implies, the lines are still there for Python 3.12, so the lines must have been lost when porting the test to 3.11. I don't believe that's intentional nor necessary.
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.
Great catch! Indeed, those lines were omitted by accident. I don't recall why; perhaps I failed to save the full file or pass -a
to git or something. Regardless, adding those lines back in restores the coverage for the security issue (0478c46).
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'll cherry-pick that commit to the other PRs as well.
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.
As your comment implies, the lines are still there for Python 3.12.
Yes. They are present in the PRs for main
, 3.13
and 3.12
, but gone in 3.11
(this PR), 3.10
, 3.9
, and 3.8
.
Applies changes from zipp 3.20.1 and jaraco/zippGH-124 (cherry picked from commit 2231286)