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

[3.10] gh-123270: Replaced SanitizedNames with a more surgical fix. (GH-123354) #123426

Merged
merged 6 commits into from
Sep 4, 2024

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Aug 28, 2024

Applies changes from zipp 3.20.1 and jaraco/zippGH-124 (cherry picked from commit 2231286) (cherry picked from commit 17b77bb)

…rgical fix. (pythonGH-123354)

Applies changes from zipp 3.20.1 and jaraco/zippGH-124
(cherry picked from commit 2231286)
(cherry picked from commit 17b77bb)

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
@jaraco
Copy link
Member Author

jaraco commented Sep 2, 2024

The tested assertions are failing in the backport on Windows:

======================================================================
FAIL: test_backslash_not_separator (test.test_zipfile.TestPath)
In a zip file, backslashes are not separators.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\lib\test\test_zipfile.py", line 3329, in test_backslash_not_separator
    assert first.name == 'foo\\bar'
AssertionError

======================================================================
FAIL: test_unsupported_names (test.test_zipfile.TestPath)
Path segments with special characters are readable.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\lib\test\test_zipfile.py", line 3315, in test_unsupported_names
    assert next(contents).name == 'V: NMS.flac'
AssertionError

----------------------------------------------------------------------

@jaraco
Copy link
Member Author

jaraco commented Sep 2, 2024

Since these same tests pass on zipp on Windows+Python 3.10, it must be the case that something else has changed in zipp that wasn't backported to Python 3.10. But what? I applied the latest code from zipp to CPython, and the diff doesn't reveal anything that would explain the failing tests.

@jaraco
Copy link
Member Author

jaraco commented Sep 2, 2024

Adding some additional information to the troubleshooting, it's slightly more clear what's happening:

======================================================================
FAIL: test_backslash_not_separator (test.test_zipfile.TestPath)
In a zip file, backslashes are not separators.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\lib\test\test_zipfile.py", line 3330, in test_backslash_not_separator
    assert first.name == 'foo\\bar', first.name
AssertionError: bar
======================================================================
FAIL: test_unsupported_names (test.test_zipfile.TestPath)
Path segments with special characters are readable.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\lib\test\test_zipfile.py", line 3316, in test_unsupported_names
    assert item.name == 'V: NMS.flac', item.name
AssertionError:  NMS.flac
----------------------------------------------------------------------

@jaraco
Copy link
Member Author

jaraco commented Sep 2, 2024

Aha! That diff is relevant in how it handles .name on Windows (pathlib.Path vs. pathlib.PurePosixPath) as applied in jaraco/zipp@9d6e639. So these tests can't be used in the backport. That's fine, as these tests are there to guarantee expectation in main and aren't pertinent to the security issue.

@jaraco
Copy link
Member Author

jaraco commented Sep 2, 2024

@pablogsal In addition to approving/merging this security fix, I'd like your opinion.

In troubleshooting why the two new tests were failing, I learned that on Python 3.10 only, zipfile.Path.name is incorrect for names containing characters specific to the platform's file system (i.e. : and \ on Windows). Probably nobody is going to care about it, but it's conceivable it could lead to a security issue. Trivially, files that are present in the zipfile will appear to be missing on Windows, and potentially someone could craft a zipfile that could cause more serious harm, all because pathlib.Path is used to derive the .name of the path, producing invalid and unexpected results.

I can think of a few options:

  • track the issue as a separate security issue and patch it separately.
  • patch the issue as part of this fix, making the behavior consistent with Python 3.9 and 3.11.
  • (default) disregard the concern for now, leaving out the tests that caught the concern.

How would you like me to proceed?

@pablogsal
Copy link
Member

  • track the issue as a separate security issue and patch it separately.

I would prefer to do that. The reason is that it's unclear if that would affect anything else and I would like more eyes on this because 3.10 it's long into security-only so I don't want to risk this. I personally would prefer to keep it consistent with 3.9 and 3.11 if it's related to the security fix but I want to see what others think

@ambv @Yhg1s what are your thoughts?

@ambv
Copy link
Contributor

ambv commented Sep 4, 2024

Agree this should be patched separately. @jaraco, the security releases will go out on Friday. Would it be possible to get a fix before then?

@jaraco
Copy link
Member Author

jaraco commented Sep 4, 2024

No problem. I'll get it done today.

@jaraco
Copy link
Member Author

jaraco commented Sep 4, 2024

I'll revise this PR to be acceptable as-is with the expectation that the additional issue will be addressed to re-enable the tests.

@jaraco
Copy link
Member Author

jaraco commented Sep 4, 2024

This patch now retains the tests but marks the two new ones as skipped on Windows, allowing the test suite to pass. A follow up GH-123694 promises to fix the emergent issue and restore the tests.

@ambv ambv merged commit 0aa1ee2 into python:3.10 Sep 4, 2024
14 of 15 checks passed
@jaraco jaraco deleted the backport-17b77bb-3.10 branch September 4, 2024 21:08
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.

3 participants