Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Comments

bpo-41928: Add support for Unicode Path Extra Field in ZipFile#23736

Closed
agiudiceandrea wants to merge 6 commits intopython:mainfrom
agiudiceandrea:fix-issue-41928
Closed

bpo-41928: Add support for Unicode Path Extra Field in ZipFile#23736
agiudiceandrea wants to merge 6 commits intopython:mainfrom
agiudiceandrea:fix-issue-41928

Conversation

@agiudiceandrea
Copy link

@agiudiceandreaagiudiceandrea commentedDec 10, 2020
edited
Loading

Add support for Unicode Path Extra Field (0x7075) following4.6.9 APPNOTE.TXT - .ZIP File Format Specification.

https://bugs.python.org/issue41928

Copy link

@ghostghost left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

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

@github-actionsgithub-actionsbot added the staleStale PR or inactive for long period of time. labelJan 22, 2021
@agiudiceandrea
Copy link
Author

When this PR will be reviewed and merged?

@github-actionsgithub-actionsbot removed the staleStale PR or inactive for long period of time. labelJan 23, 2021
@orsenthil
Copy link
Member

@agiudiceandrea - Is there a way to add tests to this PR? It will be easier for a core-dev to review and merge this.

Copy link
Contributor

@danifusdanifus left a comment

Choose a reason for hiding this comment

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

Hi, this is looking good. I've added some comments about validating the data in the extra fields. It would be good to have some tests (particularly ensuring that malicious file names get caught).

@agiudiceandrea
Copy link
Author

@danifus thank you very much for reviewing this PR!
I'll try to address your suggestions.

@agiudiceandrea
Copy link
Author

@danifus I've added both your suggested improvements.

@danifus
Copy link
Contributor

Thanks for adding those changes!

It would be good to add some tests for this new functionality. Are you able to create a minimal test file using an external program such as 7z and then embed the bytes into a test inLib/tests/test_zipfile.py?

Here's a script that you can use to turn an arbitrary file into text that you can paste into the test:

with open('minimal_example.zip', 'rb') as fid:    data = fid.read()    out = ["data = ("]    line_len = 70    line_start = 0    for i, char in enumerate(data):        str_len = len(str(char))        if len(str(data[line_start:i])) > line_len:            out.append("    " + str(data[line_start:i]))            line_start = i    if data[line_start:]:        out.append("    " + str(data[line_start:]))    out.append(")")    print("\n".join(out))

There are a number of tests in test_zipfile.py that do something similar for various functionality

@yeojin-dev
Copy link
Contributor

@agiudiceandrea Can I finish this issue? I ask you a question first because you're almost done.

arhadthedev and corona10 reacted with thumbs up emoji

@ArcticLampyrid
Copy link

Is there any progress?

@ambv
Copy link
Contributor

Closing and reopening to trigger CLA check.

@ambvambv closed thisAug 11, 2023
@ambvambv reopened thisAug 11, 2023
@ambvambv closed thisAug 11, 2023
@ambvambv reopened thisAug 11, 2023
@agiudiceandrea
Copy link
Author

Hi@yeojin-dev, I apologise for replying so late. I've missed the notification of comments from this PR.
Feel free (you and or any other dev) to finish this PR add and or modifying what is needed in order to have it merged.
Unfortunately I cannot remember the details about this PR and I cannot finish it.

@ArcticLampyrid
Copy link

Seems that the task has been finished in#102566.
Maybe we should close this PR.

agiudiceandrea reacted with thumbs up emoji

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

Reviewers

@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka

2 more reviewers

@danifusdanifusdanifus left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@agiudiceandrea@orsenthil@danifus@yeojin-dev@ArcticLampyrid@ambv@the-knights-who-say-ni@ezio-melotti@bedevere-bot

[8]ページ先頭

©2009-2026 Movatter.jp