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-121999: Change default tarfile filter to 'data' by WilliamRoyNelson · Pull Request #122002 · python/cpython

Conversation

@WilliamRoyNelson

@WilliamRoyNelson WilliamRoyNelson commented Jul 19, 2024 •
edited by picnixz
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
  • 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/

Sorry, something went wrong.

Uh oh!

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

All reactions
@WilliamRoyNelson
WilliamRoyNelson requested a review from ethanfurman as a code owner July 19, 2024 04:14
@ghost

ghost commented Jul 19, 2024 •
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 commented Jul 19, 2024

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.

WilliamRoyNelson reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot 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 done
@bedevere-app bedevere-app Bot added the awaiting review label Jul 19, 2024
@WilliamRoyNelson
WilliamRoyNelson force-pushed the gh-121999-tarfile-filter branch from 29c1861 to 0529960 Compare July 19, 2024 04:20
@WilliamRoyNelson
pythongh-121999: Change default tarfile filter to 'data'
c74d0dc
@WilliamRoyNelson
WilliamRoyNelson force-pushed the gh-121999-tarfile-filter branch from 0529960 to c74d0dc Compare July 19, 2024 04:24
@WilliamRoyNelson
Update shutil docs and tests
12f183a
tomasr8
tomasr8 reviewed Jul 19, 2024
View reviewed changes
Comment thread Doc/library/tarfile.rst Resolved

Uh oh!

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

Comment thread Lib/test/test_tarfile.py Resolved

Uh oh!

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

@ZeroIntensity

ZeroIntensity commented Jul 19, 2024

Copy link
Copy Markdown
Member

CC @encukou, as this is PEP 706.

All reactions

Sorry, something went wrong.

Uh oh!

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

WilliamRoyNelson and others added 2 commits July 19, 2024 08:37
@WilliamRoyNelson @tomasr8
Update Doc/library/tarfile.rst
2d67e8e
Co-authored-by: Tomas R <[email protected]>
@sodle
Add test for default filter of tarfile.extractall
15216b9
@sodle
sodle force-pushed the gh-121999-tarfile-filter branch from d3c2acc to 15216b9 Compare July 20, 2024 09:36
picnixz
picnixz reviewed Jul 21, 2024
View reviewed changes
Comment thread Doc/library/shutil.rst Resolved

Uh oh!

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

Comment thread Doc/library/tarfile.rst Resolved

Uh oh!

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

Comment thread Doc/library/tarfile.rst Resolved

Uh oh!

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

Comment thread Doc/library/tarfile.rst Resolved

Uh oh!

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

Comment thread Lib/test/test_tarfile.py Resolved

Uh oh!

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

Comment thread Lib/test/test_tarfile.py Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Core_and_Builtins/2024-07-18-21-19-04.gh-issue-121999.8IBbTK.rst Resolved

Uh oh!

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

sodle and others added 3 commits July 21, 2024 12:06
@sodle
Update tarfile tests to use context managers
4571846
@WilliamRoyNelson @picnixz
Improved phrasing for documentation of changes
136bc7e
Co-authored-by: Bénédikt Tran <[email protected]>
@WilliamRoyNelson
Remove unnecessarily wordy phrasing in warning about tarfiles.
57c60e7
@WilliamRoyNelson
WilliamRoyNelson force-pushed the gh-121999-tarfile-filter branch from 1e3001d to 57c60e7 Compare July 21, 2024 23:02
@picnixz

picnixz commented Jul 22, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz
picnixz reviewed Jul 22, 2024
View reviewed changes

@picnixz picnixz left a comment

Copy link
Copy Markdown
Member

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.

Some minor comments.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/test/test_tarfile.py Resolved

Uh oh!

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

Comment thread 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

@picnixz picnixz Jul 22, 2024

Copy link
Copy Markdown
Member

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 about this one but the outside 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?)

Sorry, something went wrong.

Uh oh!

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

All reactions

@WilliamRoyNelson WilliamRoyNelson Jul 22, 2024

Copy link
Copy Markdown
Contributor Author

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 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.)

Sorry, something went wrong.

Uh oh!

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

AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@encukou

encukou commented Jul 22, 2024

Copy link
Copy Markdown
Member

I'll review later this week.

All reactions

Sorry, something went wrong.

Uh oh!

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

@sodle
revert style changes for unrelated tests
619dc28
@sodle
sodle force-pushed the gh-121999-tarfile-filter branch from c3638d4 to 619dc28 Compare July 22, 2024 14:46
@sodle

sodle commented Jul 22, 2024

Copy link
Copy Markdown
Contributor

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.

picnixz reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@encukou
Reword documentation
ae161ba
@encukou

encukou commented Jul 25, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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:

  • Passing filter='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?

sodle and WilliamRoyNelson reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@encukou
encukou merged commit dcafb36 into python:main Jul 26, 2024
@bedevere-app bedevere-app Bot removed the awaiting review label Jul 26, 2024
@encukou

encukou commented Jul 26, 2024

Copy link
Copy Markdown
Member

Thank you for the update!

WilliamRoyNelson reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

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

@hadrizi hadrizi mentioned this pull request Oct 9, 2024
Documentation for tarfile has ambigouous references for releases #104638
Closed
@hugovk hugovk mentioned this pull request Oct 9, 2024
gh-104638: make TarFile.extraction_filter future release references more clear #109908
Closed
@encukou encukou mentioned this pull request May 8, 2025
Update tarfile.extractall and friends to use filter='data' by default #118875
Closed
@WilliamRoyNelson WilliamRoyNelson mentioned this pull request May 27, 2025
CVE-2021-35958 vulnerability in the latest v2.17 tensorflow/tensorflow#72282
Closed
This was referenced Aug 15, 2026
release: orchestrate retained alpha candidates azimuth-sh/azimuth#3
Merged
feat(dashboard): 指标采集器——逃逸双窗/台账/签署/误放行(W5-C4 .github#227,ADR-0073) Cloudbird-Software/.github#253
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasr8 tomasr8 tomasr8 left review comments
@picnixz picnixz picnixz left review comments
@ethanfurman ethanfurman Awaiting requested review from ethanfurman ethanfurman is a code owner
+1 more reviewer
@sodle sodle sodle left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

6 participants

@WilliamRoyNelson @ZeroIntensity @picnixz @encukou @sodle @tomasr8
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.