Skip to content

fix: Fails to extract file which might or might not be malformed (#376)#426

Merged
Pr0methean merged 10 commits intomasterfrom
Q-DEV-issue-376-1757641314
Jan 18, 2026
Merged

fix: Fails to extract file which might or might not be malformed (#376)#426
Pr0methean merged 10 commits intomasterfrom
Q-DEV-issue-376-1757641314

Conversation

@amazon-q-developer
Copy link
Contributor

@amazon-q-developer amazon-q-developer bot commented Sep 12, 2025

Pull request for Fails to extract file which might or might not be malformed

@amazon-q-developer
Copy link
Contributor Author

amazon-q-developer bot commented Sep 12, 2025

Resolves #376

@amazon-q-developer
Copy link
Contributor Author

amazon-q-developer bot commented Sep 12, 2025

To provide feedback, I recommend leaving inline comments for best results. Navigate to the Files changed tab and leave comments on the proposed code changes. Choose Start review for each comment, and then choose Request changes, and I'll propose revised changes. If creating PR-level comments, include as much detail as possible in your feedback (for example, file name, class name, line number).

@amazon-q-developer amazon-q-developer bot mentioned this pull request Sep 12, 2025
Closed
@beeb
Copy link

beeb commented Sep 12, 2025 •
edited
Loading

The tests are failing in CI with --no-default-features --features deflate-zopfli

running 5 tests
test test_extract_zip_with_windows_absolute_paths ... FAILED
test test_individual_file_access_with_absolute_paths ... FAILED
test test_extract_zip_with_absolute_paths ... FAILED
test test_extract_soldeer_like_zip ... FAILED
test test_security_still_prevents_directory_traversal ... FAILED

failures:

---- test_extract_zip_with_windows_absolute_paths stdout ----

thread 'test_extract_zip_with_windows_absolute_paths' panicked at tests/invalid_path.rs:80:38:
called `Result::unwrap()` on an `Err` value: UnsupportedArchive("Compression method not supported")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test_individual_file_access_with_absolute_paths stdout ----
File name: /_/
Enclosed name: None

thread 'test_individual_file_access_with_absolute_paths' panicked at tests/invalid_path.rs:126:9:
assertion failed: enclosed_name.is_some()

---- test_extract_zip_with_absolute_paths stdout ----

thread 'test_extract_zip_with_absolute_paths' panicked at tests/invalid_path.rs:61:38:
called `Result::unwrap()` on an `Err` value: UnsupportedArchive("Compression method not supported")

---- test_extract_soldeer_like_zip stdout ----

thread 'test_extract_soldeer_like_zip' panicked at tests/invalid_path.rs:98:38:
called `Result::unwrap()` on an `Err` value: UnsupportedArchive("Compression method not supported")

---- test_security_still_prevents_directory_traversal stdout ----

thread 'test_security_still_prevents_directory_traversal' panicked at tests/invalid_path.rs:152:40:
called `Result::unwrap()` on an `Err` value: UnsupportedArchive("Compression method not supported")


failures:
    test_extract_soldeer_like_zip
    test_extract_zip_with_absolute_paths
    test_extract_zip_with_windows_absolute_paths
    test_individual_file_access_with_absolute_paths
    test_security_still_prevents_directory_traversal

test result: FAILED. 0 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.13s

@Pr0methean Pr0methean changed the title Fails to extract file which might or might not be malformed fix: Fails to extract file which might or might not be malformed (#376) Sep 12, 2025
@Pr0methean
Copy link
Member

Pr0methean commented Sep 16, 2025

Update: the cause of most of these failures is that zopfli only writes deflate-compressed files and doesn't read them, so flate2 must be enabled as well.

@Pr0methean Pr0methean force-pushed the Q-DEV-issue-376-1757641314 branch from 41ff300 to 62353ca Compare September 16, 2025 20:49
@Pr0methean
Copy link
Member

Pr0methean commented Sep 16, 2025 •
edited
Loading

Update: one new test is still failing on Unix because a Windows drive letter is being interpreted as a normal directory on Unix. Apparently Rust's Path implementation always assumes the path comes from whatever platform it's being built for, so it looks like we'll need to use the typed-path crate to handle these properly when a ZIP file has been sent between platforms.

@Pr0methean Pr0methean added help wanted Extra attention is needed Please fix failing tests Tests are failing with this change; please fix them. labels Jan 16, 2026
@Pr0methean Pr0methean removed help wanted Extra attention is needed Please fix failing tests Tests are failing with this change; please fix them. labels Jan 17, 2026
@Pr0methean Pr0methean added this to the 7.2.0 milestone Jan 17, 2026
@Pr0methean Pr0methean enabled auto-merge January 17, 2026 01:45
@Pr0methean Pr0methean added this pull request to the merge queue Jan 17, 2026
@Pr0methean Pr0methean removed this pull request from the merge queue due to a manual request Jan 17, 2026
@Pr0methean Pr0methean enabled auto-merge January 17, 2026 18:52
@Pr0methean Pr0methean added this pull request to the merge queue Jan 18, 2026
@Pr0methean Pr0methean removed this pull request from the merge queue due to the queue being cleared Jan 18, 2026
@Pr0methean Pr0methean added this pull request to the merge queue Jan 18, 2026
Merged via the queue into master with commit d5cf4b5 Jan 18, 2026
117 checks passed
@Pr0methean Pr0methean deleted the Q-DEV-issue-376-1757641314 branch January 18, 2026 12:32
@Pr0methean Pr0methean mentioned this pull request Jan 18, 2026
Merged
@Its-Just-Nans
Copy link
Member

Its-Just-Nans commented Jan 19, 2026

I think this MR introduced the file test_fix.rs

https://github.com/zip-rs/zip2/blob/master/test_fix.rs

I think we should delete it

@Pr0methean
Copy link
Member

Pr0methean commented Jan 19, 2026

That file was added by Amazon Q Developer to the first version of this PR back in September: 18b8948

I'll take another look in the morning, but I suspect that Q added this test for a good reason, given how lazy it was in general at that time, and that if we deleted it without merging it into the existing tests then we'd get a coverage regression.

@Its-Just-Nans
Copy link
Member

Its-Just-Nans commented Jan 19, 2026

I suspect that Q added this test for a good reason

The problem is that it's not even a test :( but just a main() func

  • no #[test]
  • not in the correct place/folder

I'll take another look in the morning

Thanks

@Pr0methean
Copy link
Member

Pr0methean commented Jan 19, 2026

I suspect that Q added this test for a good reason

The problem is that it's not even a test :( but just a main() func

  • no #[test]
  • not in the correct place/folder

I'll take another look in the morning

Thanks

Fair enough; I'll convert it into a test if no existing test uses absolute paths (and I don't remember there being any that do).

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

7.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@beeb @Pr0methean @Its-Just-Nans