✅ Add comprehensive tests for extract owner options#2596
✅ Add comprehensive tests for extract owner options#2596
Conversation
📝 WalkthroughWalkthroughAdds Unix-only test utilities and a new ownership-focused extract test module, plus a minor test skip refactor and a module export gated to Unix targets. All changes are test-only; no public API behavior altered. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ChanTsune, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the test coverage for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive suite of tests for the owner-related options of the extract command. The new tests are well-structured, cover a wide range of scenarios including various flags and their interactions, and handle environment-specific conditions like root privileges gracefully. The changes are a great addition to ensure the correctness of file ownership handling. I have a few suggestions to improve maintainability and strengthen some assertions.
| assert_ne!( | ||
| meta.uid(), | ||
| 1000, | ||
| "extracted file should not have original uid after --uname override" | ||
| ); |
There was a problem hiding this comment.
The assertion assert_ne!(meta.uid(), 1000, ...) is correct but could be stronger. Since the test is skipped if the 'nobody' user doesn't exist, you can assert that the extracted file's UID is exactly the UID of 'nobody' when the test runs.
let nobody_user = nix::unistd::User::from_name("nobody")
.expect("user 'nobody' lookup failed")
.expect("'nobody' user not found, but extraction succeeded");
assert_eq!(
meta.uid(),
nobody_user.uid.as_raw(),
"extracted file should be owned by 'nobody'"
);| assert_ne!( | ||
| meta.gid(), | ||
| 1000, | ||
| "extracted file should not have original gid after --gname override" | ||
| ); |
There was a problem hiding this comment.
Similar to the uname override test, this assertion can be strengthened. Since the test skips if 'nogroup' doesn't exist, you can assert that the file's GID matches the GID of 'nogroup' for a more precise check.
let nogroup_group = nix::unistd::Group::from_name("nogroup")
.expect("group 'nogroup' lookup failed")
.expect("'nogroup' group not found, but extraction succeeded");
assert_eq!(
meta.gid(),
nogroup_group.gid.as_raw(),
"extracted file should have GID of 'nogroup'"
);![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let current_uid = nix::unistd::Uid::effective().as_raw(); | ||
| let current_gid = nix::unistd::Gid::effective().as_raw(); | ||
|
|
||
| assert_eq!( |
Check failure
Code scanning / CodeQL
Cleartext logging of sensitive information High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, to fix cleartext logging issues, avoid including sensitive data directly in log or assertion messages; instead, use generic messages or, if absolutely needed, sanitize/obfuscate or encrypt the data before logging. Here, the only use of archive_uid and archive_gid in a logging/formatting context is within the assert_eq! failure messages. The numeric values are not structurally required for the test—they only serve to make error messages more descriptive.
The best fix that preserves behavior is to keep the assertions but change their custom failure strings so they no longer format archive_uid and archive_gid. We can keep the descriptive text while dropping the {} placeholders and the corresponding arguments, e.g. change "..., not archive's uid {}" to "..., not archive's uid". No new imports or helper functions are needed; we only adjust the assert_eq! invocations in cli/tests/cli/extract/option_owner.rs at lines 106–111 and 112–117.
| @@ -106,14 +106,12 @@ | ||
| assert_eq!( | ||
| meta.uid(), | ||
| current_uid, | ||
| "extracted file should be owned by current user, not archive's uid {}", | ||
| archive_uid | ||
| "extracted file should be owned by current user, not archive's uid", | ||
| ); | ||
| assert_eq!( | ||
| meta.gid(), | ||
| current_gid, | ||
| "extracted file should be owned by current group, not archive's gid {}", | ||
| archive_gid | ||
| "extracted file should be owned by current group, not archive's gid", | ||
| ); | ||
| } | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cli/tests/cli/extract/option_owner.rs:
- Around line 529-591: The test extract_with_uid_overrides_uname currently skips
if user lookup fails, which masks the bug; change the CLI invocation to pass a
definitely-nonexistent username (e.g., "user_does_not_exist_12345") for the
--uname argument and remove the conditional skip block that checks result for
"not found"/"No such" so the test requires the command to succeed even when the
uname doesn't exist; keep the rest (override_uid, skip_if_not_root!,
result.unwrap(), and the final assert that meta.uid() == override_uid) so the
--uid precedence is actually enforced.
🧹 Nitpick comments (4)
cli/tests/cli/utils/unix.rs (1)
1-11: Consider simplifying macro exposure to avoid#[macro_export]+ re-export ambiguity.If the intent is “use it as
crate::utils::unix::skip_if_not_root!()”, double-check that Line 2 (#[macro_export]) isn’t unintentionally forcing the “real” path to becrate::skip_if_not_root!()(making Line 11 a workaround). If that’s the case, I’d prefer standardizing on the crate-root macro path and dropping the module re-export.Possible direction (standardize on crate-root macro)
#[macro_export] macro_rules! skip_if_not_root { () => { if !nix::unistd::Uid::effective().is_root() { eprintln!("Skipping test: requires root privileges"); return; } }; } -pub use skip_if_not_root;cli/tests/cli/extract/option_owner.rs (3)
22-57: Helper is fine; consider narrowing uid/gid types (and reducing duplication later).
OwnerEntryDef+create_archive_with_owneris straightforward. Minor: you could consideru32for uid/gid fields (matchingMetadataExt) and cast only at the archive boundary, to reduce mixed-width comparisons.
286-409: Avoid brittle “skip by error string”; prefer preflight user/group existence checks.Line 328-334 and Line 390-396 skip based on message substrings (“not found”, “No such”), which is prone to churn across platforms/locales/error wrappers. Prefer checking whether the target user/group exists up front (then
return;) and let real extraction failures fail the test.
120-176: Cross-platform/CI expectations: root-only coverage and “nobody/nogroup” assumptions.
- Line 129-131 and Line 601-614 hard-code
65534+"nobody"/"nogroup"; this is common on Linux but not universal across all unix variants.- Many tests are root-gated (skip), so ensure CI has at least one job that actually runs these as root; otherwise you’ll only validate the non-root paths.
Also applies to: 593-664
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cli/tests/cli/extract/option_chroot.rscli/tests/cli/extract/option_owner.rscli/tests/cli/utils.rscli/tests/cli/utils/unix.rs
🔇 Additional comments (2)
cli/tests/cli/utils.rs (1)
4-5: Unix-gatedutils::unixmodule export looks correct.Line 4-5 keeps unix-only test helpers from breaking non-unix builds; the provided callsites are also unix-gated.
cli/tests/cli/extract/option_chroot.rs (1)
2-2: Nice refactor: root-only test now skips cleanly (verify macro scoping).Line 12 is a solid improvement for non-root CI runs. Please just ensure Line 2’s macro import path works reliably with
#[macro_export]semantics on your MSRV andnix0.30.1.Also applies to: 12-12
| /// Precondition: An archive contains files with uid/gid and uname/gname. | ||
| /// Action: Extract the archive with both `--uid` and `--uname` specified. | ||
| /// Expectation: The `--uid` option takes precedence over `--uname`. | ||
| #[test] | ||
| #[cfg(unix)] | ||
| fn extract_with_uid_overrides_uname() { | ||
| setup(); | ||
| skip_if_not_root!(); | ||
|
|
||
| let override_uid = 65534; | ||
|
|
||
| fs::create_dir_all("extract_uid_overrides_uname").unwrap(); | ||
| create_archive_with_owner( | ||
| "extract_uid_overrides_uname/archive.pna", | ||
| &[OwnerEntryDef { | ||
| path: "test.txt", | ||
| content: b"test content", | ||
| uid: 1000, | ||
| uname: "originaluser", | ||
| gid: 1000, | ||
| gname: "originalgroup", | ||
| permission: 0o644, | ||
| }], | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // Extract with both --uid and --uname (--uid should take precedence) | ||
| let result = cli::Cli::try_parse_from([ | ||
| "pna", | ||
| "--quiet", | ||
| "x", | ||
| "extract_uid_overrides_uname/archive.pna", | ||
| "--overwrite", | ||
| "--out-dir", | ||
| "extract_uid_overrides_uname/out/", | ||
| "--keep-permission", | ||
| "--same-owner", | ||
| "--uid", | ||
| &override_uid.to_string(), | ||
| "--uname", | ||
| "nobody", | ||
| ]) | ||
| .unwrap() | ||
| .execute(); | ||
|
|
||
| // Skip if user lookup fails | ||
| if let Err(e) = &result { | ||
| let err_str = format!("{:#}", e); | ||
| if err_str.contains("not found") || err_str.contains("No such") { | ||
| eprintln!("Skipping test: user lookup failed"); | ||
| return; | ||
| } | ||
| } | ||
| result.unwrap(); | ||
|
|
||
| let meta = fs::metadata("extract_uid_overrides_uname/out/test.txt").unwrap(); | ||
|
|
||
| assert_eq!( | ||
| meta.uid(), | ||
| override_uid, | ||
| "--uid should take precedence over --uname" | ||
| ); | ||
| } |
There was a problem hiding this comment.
--uid precedence test currently masks the bug it claims to catch.
Line 574-581 skips on user-lookup failure, but if --uid truly takes precedence over --uname, the command shouldn’t need the username to exist at all. This skip can hide a real regression.
I’d change --uname to a definitely-nonexistent name and assert the command succeeds (then assert uid == override).
Proposed fix
- let result = cli::Cli::try_parse_from([
+ let result = cli::Cli::try_parse_from([
"pna",
"--quiet",
"x",
"extract_uid_overrides_uname/archive.pna",
"--overwrite",
"--out-dir",
"extract_uid_overrides_uname/out/",
"--keep-permission",
"--same-owner",
"--uid",
&override_uid.to_string(),
"--uname",
- "nobody",
+ "this-user-should-not-exist-12345",
])
.unwrap()
.execute();
- // Skip if user lookup fails
- if let Err(e) = &result {
- let err_str = format!("{:#}", e);
- if err_str.contains("not found") || err_str.contains("No such") {
- eprintln!("Skipping test: user lookup failed");
- return;
- }
- }
- result.unwrap();
+ // If this fails, it likely means `--uname` is still being resolved despite `--uid`.
+ result.unwrap();🤖 Prompt for AI Agents
In @cli/tests/cli/extract/option_owner.rs around lines 529 - 591, The test
extract_with_uid_overrides_uname currently skips if user lookup fails, which
masks the bug; change the CLI invocation to pass a definitely-nonexistent
username (e.g., "user_does_not_exist_12345") for the --uname argument and remove
the conditional skip block that checks result for "not found"/"No such" so the
test requires the command to succeed even when the uname doesn't exist; keep the
rest (override_uid, skip_if_not_root!, result.unwrap(), and the final assert
that meta.uid() == override_uid) so the --uid precedence is actually enforced.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.