NotificationsYou must be signed in to change notification settings
Fork33.3k
Star69.8k

gh-121999: Change default tarfile filter to 'data'#122002

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

encukou merged 9 commits intopython:mainfromWilliamRoyNelson:gh-121999-tarfile-filter

Jul 26, 2024

Merged

gh-121999: Change default tarfile filter to 'data'#122002

encukou merged 9 commits intopython:mainfromWilliamRoyNelson:gh-121999-tarfile-filter

Jul 26, 2024

+76 −76

Conversation

Copy link

Contributor

WilliamRoyNelson commentedJul 19, 2024•
edited by picnixz
Loading

Issue:Fully implement PEP 706: Change default filter for TarFile.extract() and extractall() to 'data' #121999

📚 Documentation preview 📚:https://cpython-previews--122002.org.readthedocs.build/

WilliamRoyNelson requested a review fromethanfurman as acode owner

July 19, 2024 04:14

Copy link

ghost commentedJul 19, 2024•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.

Copy link

bedevere-appbot commentedJul 19, 2024

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

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

bedevere-appbot mentioned this pull request

Jul 19, 2024

Fully implement PEP 706: Change default filter for TarFile.extract() and extractall() to 'data'#121999

Closed

7 tasks

bedevere-appbot added the awaiting review label

Jul 19, 2024

WilliamRoyNelson force-pushed thegh-121999-tarfile-filter branch from29c1861 to0529960Compare

July 19, 2024 04:20

pythongh-121999: Change default tarfile filter to 'data'

c74d0dc

WilliamRoyNelson force-pushed thegh-121999-tarfile-filter branch from0529960 toc74d0dcCompare

July 19, 2024 04:24

Update shutil docs and tests

12f183a

tomasr8 reviewed

Jul 19, 2024

View reviewed changes

Doc/library/tarfile.rst OutdatedShow resolvedHide resolved

Lib/test/test_tarfile.pyShow resolvedHide resolved

Copy link

Member

ZeroIntensity commentedJul 19, 2024

CC@encukou, as this is PEP 706.

WilliamRoyNelsonand others added2 commits

July 19, 2024 08:37

Update Doc/library/tarfile.rst

2d67e8e

Co-authored-by: Tomas R <tomas.roun8@gmail.com>

Add test for default filter of tarfile.extractall

15216b9

sodle force-pushed thegh-121999-tarfile-filter branch fromd3c2acc to15216b9Compare

July 20, 2024 09:36

picnixz reviewed

Jul 21, 2024

View reviewed changes

Doc/library/shutil.rst OutdatedShow resolvedHide resolved

Doc/library/tarfile.rst OutdatedShow resolvedHide resolved

Lib/test/test_tarfile.py OutdatedShow resolvedHide resolved

Misc/NEWS.d/next/Core_and_Builtins/2024-07-18-21-19-04.gh-issue-121999.8IBbTK.rst OutdatedShow resolvedHide resolved

sodleand others added3 commits

July 21, 2024 12:06

Update tarfile tests to use context managers

4571846

Improved phrasing for documentation of changes

136bc7e

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Remove unnecessarily wordy phrasing in warning about tarfiles.

57c60e7

WilliamRoyNelson force-pushed thegh-121999-tarfile-filter branch from1e3001d to57c60e7Compare

July 21, 2024 23:02

Copy link

Member

picnixz commentedJul 22, 2024

I'm a bit confused but... arae there two people working on this PR simultaneously@WilliamRoyNelson and@sodle?

picnixz reviewed

Jul 22, 2024

View reviewed changes

Copy link

Member

picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some minor comments.

Lib/test/test_tarfile.py OutdatedShow resolvedHide resolved

Doc/library/tarfile.rst

		..versionchanged::3.14
		Set the default extraction filter to:func:`data <data_filter>`,
		which disallows dangerous features such as links to absolute paths
		or paths outside of the destination. Previously, the filter strategy

Copy link

Member

picnixzJul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sorry about this one but theoutside of now feels weird to me :')

@AA-Turner As a native speaker (you're the only one I know...), should it be "outside the destination", or "outside of"? (or something else entirely?)

Copy link

ContributorAuthor

WilliamRoyNelsonJul 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think "outside of" is conventional in North America, less conventional in the UK.
https://learningenglish.voanews.com/a/should-we-think-outside-or-outside-of-the-box-/6434530.html

I think I was reading from PEP 706 when I wrote that update.

Refuse to extract links (hard or soft) which end up linking to a path outside of the destination. (On systems that don’t support links, tarfile will, in most cases, fall back to creating regular files. This proposal doesn’t change that behaviour.)

Copy link

Member

encukou commentedJul 22, 2024

I'll review later this week.

revert style changes for unrelated tests

619dc28

sodle force-pushed thegh-121999-tarfile-filter branch fromc3638d4 to619dc28Compare

July 22, 2024 14:46

Copy link

Contributor

sodle commentedJul 22, 2024

I'm a bit confused but... arae there two people working on this PR simultaneously@WilliamRoyNelson and@sodle?

Yeah. Bill is a friend of mine and enlisted my help with writing the tests.

Reword documentation

ae161ba

Copy link

Member

encukou commentedJul 25, 2024•
edited
Loading

For the documentation, communicating via GitHub review comments wouldn't be effective, so I took the liberty of pushing a commit to this PR directly. I hope you don't mind.

The main themes are:

Passingfilter='data' is still recommended, if there's any chance the code will run on Python 3.13 or below. When 3.14 is released, that's nearly all the code :)
All the security notes should lead to:ref:tarfile-extraction-filter(to explain things) and/or:ref:tarfile-further-verification (to explain that'data' still isn't unconditionally “safe”). Hopefully we can do that without annoying people who read those already :)

For shutil: zipfile also has some safeties, though they haven't been reviewed in a while. IMO we can claim for both formats that the defaults “prevent the most dangerous of such security issues”.

Does this look good to you?