GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111791: delegating extraction to zipfile module's extractall() method by sepastian · Pull Request #111824 · python/cpython

Conversation

@sepastian

@sepastian sepastian commented Nov 7, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

shutil.unpack_archive: deletage extracting ZIP files to zipfile (#111791)

gh-111791: rely on `zipfile.extractall` when extraxting ZIP archives, 
           preventing false negatives and code duplication

As reported in #111791, if the path of a file inside a ZIP file contains "..", e.g. myfile..txt (probably misspelled), shutil.unpack_archive will silently skip extracting the file, because it wrongly assumes a relative path.

This is problematic for two reasons:

  1. The current implementation of shutil.unpack_archive wrongly identifies relative path components. Scanning for ".." does not tell whether a path contains relative components, or not; one must scan for "../" instead.
  2. Besides, files inside a ZIP archive described through relative paths should be extracted, see below.

Python's own zipfile module and the unzip are handling relative path components "../"and names containins ".." correctly. For reference, the man unzip page says:

For security reasons, unzip normally removes parent dir path components (../) from the names of extracted file. This safety feature (new for version 5.50) prevents unzip from accidentally writing files to sensitive areas outside the active extraction folder tree head.

Solution: delegate extracting ZIP archives to Python's own zipfile.extractall method.

Appendix

The following example shows, how extracting a ZIP archive containing paths containing relative components "../" and files with names containing ".." differs in shutil.unpack_archive, zipfile.extractall and the Linux tool unzip.

# Create some files and directories.
$ cd /tmp
$ mkdir -p a/b/c
$ touch a/a.txt a/a..txt a/b/b.txt a/b/b..txt a/b/c/c.txt a/b/c/c..txt
$ find a
a
a/b
a/b/b.txt
a/b/b..txt
a/b/c
a/b/c/c.txt
a/b/c/c..txt
a/a.txt
a/a..txt

# Create a ZIP file for testing.
$ cd /tmp/a/b
$ zip /tmp/test.zip ../a.txt /tmp/a/b/c/c.txt ../../a/b/b.txt

# Inspect the newly created ZIP file.
$ zipinfo /tmp/test.zip
zipinfo /tmp/test.zip 
Archive:  /tmp/test.zip
Zip file size: 482 bytes, number of entries: 3
-rw-rw-r--  3.0 unx        0 bx stor 23-Nov-07 16:03 ../a.txt
-rw-rw-r--  3.0 unx        0 bx stor 23-Nov-07 16:04 tmp/a/b/c/c.txt
-rw-rw-r--  3.0 unx        0 bx stor 23-Nov-07 16:03 ../../a/b/b.txt
3 files, 0 bytes uncompressed, 0 bytes compressed:  0.0%

# Prepare directories holding extraction results.
$ mkdir -p /tmp/extract/unzip /tmp/extract/zipfile /tmp/extract/shutil

# Extract using 'unzip' extracts all files 
# and warns about relative path components.
$ cd /tmp/extract/unzip
$ unzip /tmp/test.zip
Archive:  /tmp/test.zip
warning:  skipped "../" path component(s) in ../a.txt
 extracting: a.txt                   
 extracting: tmp/a/b/c/c.txt         
warning:  skipped "../" path component(s) in ../../a/b/b.txt
 extracting: a/b/b.txt               
$ find
.
./a
./a/b
./a/b/b.txt
./tmp
./tmp/a
./tmp/a/b
./tmp/a/b/c
./tmp/a/b/c/c.txt
./a.txt

# Extract using 'zipfile' (CLI).
$ cd /tmp/extract/zipfile
$ python3 -m zipfile -e /tmp/test.zip .
$ find
.
./a
./a/b
./a/b/b.txt
./tmp
./tmp/a
./tmp/a/b
./tmp/a/b/c
./tmp/a/b/c/c.txt
./a.txt

# Extract using 'shutil.unpack_archive'.
$ cd /tmp/extract/shutil
$ python3 -c 'import shutil; shutil.unpack_archive("/tmp/test.zip",".")' 
$ find
.
./tmp
./tmp/a
./tmp/a/b
./tmp/a/b/c
./tmp/a/b/c/c.txt

# As can be seen, and as can be confirmed using `diff`,
# zipfile and unzip produce the same result,
# while shutil silently skips paths containing "..",
# i.e. both names containing ".." and paths containing
# actual relative components.

# Diff.
$ cd /tmp/extract
$ diff -r zipfile/ unzip/

$ diff -r unzip/ shutil/
Only in unzip/: a
Only in unzip/: a..txt
Only in unzip/: a.txt
Only in unzip/: tmp

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sepastian
fixing python#111791: delegating extraction in shutil.unpack_archive …
c8a6b24
…to zipfile

shutil.unpack_archive fails, if file name contains '..'; zipfile handles everything correctly, i.e. in the same way than 'unzip'; let zipfile unpack archives, instead of reinventing the wheel here
@ghost

ghost commented Nov 7, 2023 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added the awaiting review label Nov 7, 2023
@bedevere-app

bedevere-app Bot commented Nov 7, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sepastian
Merge branch 'main' into shutil_unpack_archive_false_positives
7875a14
@bedevere-app

bedevere-app Bot commented Nov 7, 2023

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@blurb-it
📜🤖 Added by blurb_it.
d50ea17
@sepastian sepastian changed the title fixing #111791: delegating extraction to zipfile delegating extraction to zipfile, fixing #111791 Nov 7, 2023
@sepastian sepastian changed the title delegating extraction to zipfile, fixing #111791 gh-111791: delegating extraction to zipfile module's extractall() method Nov 7, 2023
@bedevere-app bedevere-app Bot mentioned this pull request Nov 7, 2023
shutil.unpack_archive skips false negatives containing '..' #111791
Closed
sepastian added 5 commits November 8, 2023 11:04
@sepastian
Update shutil.rst
9de81b1
Filenames containing two dots ".." do not necessarily indicate relative path components. Only "../" identify a relative path component.
@sepastian
Merge branch 'main' into shutil_unpack_archive_false_positives
4ece8c1
@sepastian
Merge branch 'main' into shutil_unpack_archive_false_positives
055e113
@sepastian
Merge branch 'main' into shutil_unpack_archive_false_positives
0f66cb3
@sepastian
Merge branch 'main' into shutil_unpack_archive_false_positives
7f789f1
@sepastian

sepastian commented Dec 13, 2023

Copy link
Copy Markdown
Contributor Author

This is a bug that needs to be fixed. Any progress on this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Dec 16, 2023
behavioral differences between shutil.unpack_archive and ZipFile.extractall #65106
Open
gh-65106: shutil._unpack_zipfile add warnings for skipped files #29910
Open
@serhiy-storchaka
serhiy-storchaka self-requested a review December 16, 2023 09:08
sepastian and others added 2 commits February 15, 2024 09:15
@sepastian
Merge branch 'main' into shutil_unpack_archive_false_positives
f090e3d
@AlexWaygood
Merge branch 'main' into shutil_unpack_archive_false_positives
0a18c01
@python-cla-bot

python-cla-bot Bot commented Apr 18, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coderabbitai coderabbitai Bot mentioned this pull request Apr 5, 2026
⚡ Bolt: Optimize database ingestion with executemany for batch processing vibhu1212/ufdr-analysis-tool#14
Merged
@github-actions

github-actions Bot commented Apr 14, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 14, 2026
@serhiy-storchaka

serhiy-storchaka commented May 18, 2026

Copy link
Copy Markdown
Member

Thank you for your contribution @sepastian and sorry for missing your PR. Similar solution was applied in #146591.

I have noticed that your PR fixes also a part of the docs which I missed. We will fix it too.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka closed this May 18, 2026
@sepastian

sepastian commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

I detected this problem and provided a fix for this 3 years ago. I asked you specifically, @serhiy-storchaka, about the status of this, that was two years ago. You never replied to me. Although you referenced my PR elsewhere. See #111824 (comment), for example.

And now you are merging a copy of my solution, which is only 2 months old, and make this an official contribution?

What's going on here?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented May 18, 2026

Copy link
Copy Markdown
Member

I admit my guilt.

That issue looked like an ordinary bug affecting not many people, until a security issue was reported few months ago, to which this was the simplest solution. I completely forgot about your PR! It seems my memory is failing me. I reviewed almost 2000 PRs in the last 3 years, have more than 100 PRs in my backlog, and add new PRs to it every day. Add to this that some issues need several days to work. It's inevitable that I'll fail many reviews. My apologies.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sepastian sepastian mentioned this pull request May 18, 2026
shutil.unpack_archive() on Windows writes outside extract_dir for ZIP entries with drive-prefixed names #146581
Closed
@sepastian

sepastian commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

I have noticed that your PR fixes also a part of the docs which I missed. We will fix it too.

What does that mean exactly? I take it for granted that you will apply my PR here, at least.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sepastian

sepastian commented May 18, 2026

Copy link
Copy Markdown
Contributor Author

I admit my guilt.

That issue looked like an ordinary bug affecting not many people, until a security issue was reported few months ago, to which this was the simplest solution. I completely forgot about your PR! It seems my memory is failing me. I reviewed almost 2000 PRs in the last 3 years, have more than 100 PRs in my backlog, and add new PRs to it every day. Add to this that some issues need several days to work. It's inevitable that I'll fail many reviews. My apologies.

Thank you for your time and all the work you put into Python - seriously! My PR is not even tiny compared to that. Still, I put in a lot of work. If my PR had been found not good enough for merging, I would totally understand. If I had received some feedback at the very least, also good. But I have been ignored for three years, even after asking several times. Just silence.

Today I have to learn that my idea and solution has been taken and merged under a different name. I understand what you are saying and, among all the things you are doing, this happened. Again, highly appreciate what you do for this project.

This is not about blaming someone, I'm just wondering how this can be fixed. Because the way it is now, I'm mad and disappointed. From what you wrote above, I get the impression you are even updating the docs with what I wrote, without mentioning me. This is not acceptable.

StanFromIreland reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented May 18, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Your PR was not not good enough. It just fell out of the attention span. Bad luck.

For now, there are 2000 open PRs and 69000 closed PRs. Almost 1000 new PRs created each month, 70% of them are closed in the first month, 90% are closed in the first year, but there are PRs waiting for several years.

I try to clean languishing PRs. Your PR somehow fell through my filters. This is a sad lesson for me.

I updating the docs in different way: #149994.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka mentioned this pull request May 18, 2026
gh-146581: Update docs for dangerous filenames in ZIP files #149994
Merged
@StanFromIreland

StanFromIreland commented May 18, 2026

Copy link
Copy Markdown
Member

Almost 1000 new PRs created each month, 70% of them are closed in the first month, 90% are closed in the first year, but there are PRs waiting for several years.

FYI, I did some calculations a few weeks ago (excluding backports):

pr_fate
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@sepastian @serhiy-storchaka @StanFromIreland @AlexWaygood
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.