Allow reading member names with bogus encodings in zipfile

[yaseppochi]

BPO	28080
Nosy	@gpshead, @ned-deily, @yaseppochi, @methane, @serhiy-storchaka
PRs	bpo-28080:: Add support for the fallback encoding in ZIP files #32007
Files	encoded-member-names: Mercurial queue style patch for Lib/zipfile.py, Doc/Lib/zipfile.rst, Lib/test/test_zipfile.py encoded-member-names-v2 encoded-member-names-v3

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2022-03-24.00:33:09.557>
created_at = <Date 2016-09-11.18:23:14.387>
labels = ['type-feature', 'library', '3.11']
title = 'Allow reading member names with bogus encodings in zipfile'
updated_at = <Date 2022-03-24.00:33:09.555>
user = 'https://github.com/yaseppochi'

bugs.python.org fields:

activity = <Date 2022-03-24.00:33:09.555>
actor = 'gregory.p.smith'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2022-03-24.00:33:09.557>
closer = 'gregory.p.smith'
components = ['Library (Lib)']
creation = <Date 2016-09-11.18:23:14.387>
creator = 'sjt'
dependencies = []
files = ['44564', '44571', '44593']
hgrepos = []
issue_num = 28080
keywords = ['patch', 'needs review']
message_count = 19.0
messages = ['275828', '275834', '275837', '275842', '275844', '275847', '275856', '275891', '275996', '276025', '276051', '276054', '284029', '284139', '415605', '415746', '415756', '415876', '415917']
nosy_count = 5.0
nosy_names = ['gregory.p.smith', 'ned.deily', 'sjt', 'methane', 'serhiy.storchaka']
pr_nums = ['32007']
priority = 'normal'
resolution = 'fixed'
stage = 'commit review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue28080'
versions = ['Python 3.11']

[serhiy-storchaka]

Could you please provide more information sjt?

[yaseppochi]

Suggested NEWS/whatsnew entry:

Add a new *memberNameEncoding* argument to the ZipFile constructor, allowing
:mod:`zipfile` to read filenames in non-conforming encodings from the
zipfile as Unicode. This implementation assumes all member names have the same encoding.

Motivation:

There are applications in Japan that create zipfiles with directories containing filenames encoded in Shift JIS. There may be such software in other countries as well. As this is a violation of the Zip format definition, this library implements only an option to read such files.

Done:

(1) Add a memberNameEncoding argument to the main() function, which may be set from the command line with "--membernameencoding={codec}". This command line option may be used with -e or -l, but not -c or -t. There is no point to it in the latter, since the member names are not printed.
(2) Add a memberNameEncoding argument to the ZipFile constructor. This is the only way to set it, so this is global to the ZipFile.
(3) Add this attribute to repr.
(4) Add a check that the mode is read in main() and in the ZipFile constructor, and if not invoke USAGE and exit or raise RuntimeError.
(5) When retrieving member names in constructing ZipInfo instances, check if memberNameEncoding is set, and if so use it, unless the UTF-8 bit is set. In that case, obey the UTF-8 bit, as the specified encoding is surely user error.
(6) Add a CODEC_USAGE message.
(7) Update the docs (docstrings, library reference, NEWS).
(8) Add tests:
(a) List a zipfile's SJIS-encoded directory.
(b) List a UTF-8-encoded directory and an ISO-8859-1-encoded directory as Shift-JIS.
(c) Check that USAGE is invoked on attempts to write a zipfile in main().
(d) Check that an appropriate error is raised on attempts to write in other functions.
Many other tests are run as well.
ALL TESTS PASS.
(9) Docs build without error.

To do (?):

(10) NEWS/whatsnew
(11) Check relevant code paths are all covered by tests.
(12) Review docs for clarity and organization.

Not done:

I don't think these are appropriate/needed at this time, but listed in case somebody thinks otherwise.

(13) Add a subtype of RuntimeError (see 7d)?
(14) Issue warning if both membernameencoding and utf-8 bit are set (see 4)?
(15) Support InfoZip encoding extension mentioned in APPNOTE.TXT - .ZIP File Format Specification, v6.3.4.
(16) Support per-member encodings (I think the zipfile standard permits, but not sure).

[yaseppochi]

I should have a contributor agreement form on file.

Ned Deily suggested that I try to get this patch in before the 12 noon deadline Sept. 12, so here it is.

I believe the patch is "safe" in the sense that its functionality needs to be explicitly enabled, and it should be very difficult to persuade it to inadvertantly write to any file. No existing execution paths should be changed at all.

[serhiy-storchaka]

This issue is a duplicate of bpo-10614. But proposed patch looks more ready.

[ned-deily]

Stephen, thanks for submitting the patch. Unless another core developer has time to review and commit this prior to the feature code off tomorrow, this will probably need to wait for 3.7. Also, at the moment, your tracker user record does not indicate that there is a contributor agreement on file for you. You could followup on that with Ewa at the PSF or just submit a new form.

[yaseppochi]

Re: wait for 3.7 if reviewers are busy, understood. N.B. Contributor agreement is now on file (I received the PDF from python.org already).

Re: existing patches:
My patch is very similar in the basic approach to Sergey Dorofeev's patch in bpo-10614. Main differences:
(1) Sergey's patch treats the "encoding" parameter as a first class citizen with a default to cp437, whereas mine treats it as a special case defaulting to None, with utf-8 and cp437 getting special treatment as the standard encodings. Subtle point, but I like it this way.
(2) My patch includes support for the argument in the __main__ script.
(3) Sergey's patch misses one execution path in the current code so needs update before application.

The Japanese patches by umedoblock are very Japanese-centric, and worse, they try to guess the encoding by the crude method of seeing what decodes successfully. They are not acceptable IMO.

Aaargh. Just noticed the Japanese in test_zipfile.py. Will change it to use \u escapes soon.

[serhiy-storchaka]

Added comments on Rietveld. Maybe I'll commit rewritten patch tomorrow before 12:00 UTC (oh, already today!).

[yaseppochi]

Can't reply on Rietveld? Lost 2 hours work!

Patch updated (encoded-member-names-v2), most changes accepted. Not happy about name change or default to cp437, I want this API to be hard to use and not be part of the normal process (utf-8 or cp437). Considering errors= argument, but that must default to 'strict' -- the problem this patch solves is zip utilities extracting to files with unreadable names, surrogateescape is more of the same. Two incomplete tests (assertRaisesRegex and capture main() stderr) still in progress, must do dayjob now.

[serhiy-storchaka]

Sorry, I can't push the patch in a haste. In needs more design discussion, comparing with other implementations (I found only that ZipFile in Java can take the charset argument, but "charset" is common name for text encoding in Java), wider discussion. The problem exists for years, and there are workarounds.

[yaseppochi]

If you have a workaround that's available to nonprogrammers, I'd like to hear about it. I have found none, that's why I went to the trouble to put together a patch even though I knew that the odds of actually getting it in to Python 3.6 was very low -- my patch (or Sergey Dorofeev's, but that needs work to be applicable to trunk) does everything I've ever needed, so I suppose it would do for all the use cases so far posted (except umedoblock's encoding-guessing approach, but that can be handled by many 3rd-party encoding-guessing codecs, I think, and IMO should be).

[serhiy-storchaka]

Python is programming language, I don't understand what you mean saying "available to nonprogrammers". As a programmer you can recode ZipInfo name before outputting or what you want to do with it:

    filename = filename.encode('cp437').decode(encoding)

In command line you can can use iconv or other converters:

python3 -m zipfile -l myzip.zip | iconv -t cp437 | iconv -f "$encoding"

This is not very handy, but works in most cases.

[yaseppochi]

Cleaned up a few loose ends while it's all fresh in mind. Will ping python-dev in 4-6 weeks for review for 3.7.

Thanks to Serhiy for review. The current version of the patch is much improved over the initial submission due to his efforts.

[serhiy-storchaka]

A ZipFile can be read when open in not read-only mode. Thus the encoding argument should be accepted when mode != 'r'.

It would be weird to read file names and write new entries with different encodings. Thus the encoding argument should affect output encoding too.

You have named the new ZipFile attribute metadataEncoding. Indeed, I missed this, and other developers missed this when ported to Python 3, but the specification says, that the UTF-8 bit affect not just the encoding of file names, but the encoding of comments. Thus a file comment must be a string, and be decoded with the same encoding as a file name. Currently it is of type bytes. I don't know what is the best way to resolve this issue without breaking backward compatibility. Perhaps add the text_comment property.

[yaseppochi]

Thanks for followup! I was just about to write you, now that 3.6 is out. Season's Greetings!

First, how do you propose to proceed with bpo-28115 ("use argparse for the ZipFile module")? If you expect to commit that first (I'm in no hurry for this patch, BTW, as long as it gets into 3.7 I'm happy), this issue should depend on it and use argparse too.

I don't see any good reason for allowing non-UTF-8 encoding to a file open for writing, and a good reason (the ZipFile standard) for not allowing it. Certainly the CLI should not allow it, any more than it does now. At least in my experiments InfoZip and the default zip utilities on Windows and Mac DTRT with UTF-8 zipfiles, so there is no absolute need for writing nonconforming zipfiles. If you want to block on a convert-to-UTF-8 option I can do that (but I don't need it myself). (Note to self: if writing to existing zipfile is extension of existing file, need to prevent mixed encodings. Also warn about conversion.)

I thought I checked that comments were decoded. Maybe that's only on the UTF-8 path? Or maybe I needed more coffee. (Hope so, that would be a messy problem if ASCII/Latin1 returns bytes and UTF-8 returns str!) I'll think about this. Yes, it's a backwards-compatibility issue so needs care. Would be weird if names are decoded but other metadata (comments) not, though. Surely someone would complain if they actually used comments? (I'm thinking maybe a compatibility break might be OK? With deprecation cycle?) I expect to check all execution paths accessing metadata and have a proposed patch by 12/31.

I think I'm still short some tests, will check and write them if needed.

[serhiy-storchaka]

I experimented with this a lot. There is a problem with the append mode. We can read in the append mode, therefore we need an encoding. But when we close a ZipFile after appending, non-ASCII file names will be encoded in UTF-8 in the central directory. Next time when we open the archive for reading with different encoding we will get an error because filenames in the central directory and in local headers are different. We need to write non-ASCII files back with the specified encoding to get a self-consistent data.

Finally I left this as it was initially. We can return to the problem with the append module later.

The differences between PR 32007 and your patches:

• The parameter was renamed to metadata_encoding to avoid confusion with existing parameter of ZipFile.open() encoding. In future I am going to use it also for comments. The attribute and the CLI option were renamed correspondingly.
• --metadata-encoding can also be used with the -t option.
• "surrogateescape" no longer used. If the encoding in not suitable, you will get an error. Use the default and decode filenames manually in such cases. We can change this in future.
• Updated documentation.
• Tests were significantly rewritten. Now they test the behavior with wrong metadata_encoding, mixed UTF-8 and legacy encodings, and reading after append.

I was going to make more changes, but left it for future.

[gpshead]

Your PR looks good to me.

I agree with not making it easy to _write_ zipfiles with non-standard encoding used for names.

There is a possibility that someone wants that ability when writing zip files (not yet clear) in https://bugs.python.org/issue40172. I don't like it, but if the need exists, that feature can be addressed on that issue via relevant options.

[serhiy-storchaka]

New changeset a25a985 by Serhiy Storchaka in branch 'main':
bpo-28080: Add support for the fallback encoding in ZIP files (GH-32007)
a25a985

[yaseppochi]

I'm not going to have time to look at the PR for a couple days.

I don't understand what the use case is for writing or appending with filenames in a non-UTF-8 encoding. At least in my experience, reading such files is rare, but I have never been asked to write one. The correspondents who send me zipfiles with the directory encoded in shift_jisx0213 are perfectly happy to read zipfiles with the directory encoded in UTF-8.

If that is true for other users, then unzipping the file to a temporary directory with the appropriate --metadata-encoding, adding the required paths there, and zipping a new archive seems perfectly workable. In that case making this feature read-only makes the most sense to me.

[gpshead]

Thanks Serhiy!