Skip to content

feat(cargo-php)!: escalate privilege and to copy extension and edit ini file #482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dilawar
Copy link

@dilawar dilawar commented Jul 3, 2025

Fixes: #481
BREAKING CHANGE: installing extensions as root user will now fail unless --bypass-root-check is provided

@dilawar dilawar marked this pull request as draft July 3, 2025 06:50
@coveralls
Copy link

coveralls commented Jul 3, 2025

Pull Request Test Coverage Report for Build 16323306954

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 48 (0.0%) changed or added relevant lines in 1 file are covered.
  • 193 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.6%) to 22.222%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/cli/src/lib.rs 0 48 0.0%
Files with Coverage Reduction New Missed Lines %
crates/cli/src/lib.rs 2 0.0%
src/builders/sapi.rs 3 96.67%
src/zend/mod.rs 4 0.0%
build.rs 5 0.0%
src/args.rs 7 81.61%
src/embed/mod.rs 9 66.67%
unix_build.rs 11 0.0%
src/flags.rs 13 27.38%
src/zend/globals.rs 51 15.26%
crates/macros/src/function.rs 88 0.0%
Totals Coverage Status
Change from base Build 16035783050: 0.6%
Covered Lines: 870
Relevant Lines: 3915

💛 - Coveralls

Copy link
Collaborator

@Xenira Xenira left a comment

Choose a reason for hiding this comment

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

That looks a lot like what I was thinking about. Thanks!

Maybe we can check if we are running with root without needing the dependency, but haven't looked into that.

Comment on lines 610 to 631
// write to a temp file
let tempf = std::env::temp_dir().join("__tmp_cargo_php");
std::fs::write(&tempf, content)?;

// Now move. `rename` will overwrite existing file.
if std::fs::rename(&tempf, filepath).is_err() {
#[cfg(unix)]
{
// if not successful, try with sudo on unix.
let _ = std::process::Command::new("sudo")
.arg("mv")
.arg(&tempf)
.arg(filepath)
.status()?;
}

#[cfg(not(unix))]
anyhow::bail!("failed to write to {filepath:?}");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically this could override changes made between the time the tmp file is created and its copied.

But I think that is fine for now.

Copy link
Author

Choose a reason for hiding this comment

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

I tried adding a write lock, but then I didn't know how to do it properly on Windows without pulling-in windows-rs crate.

There is https://crates.io/crates/tempfile that may be a cross-platform solution.

@dilawar
Copy link
Author

dilawar commented Jul 4, 2025

Maybe we can check if we are running with root without needing the dependency, but haven't looked into that.

I'll borrow the function from elevate/sudo crate that check if we are running as root. Knowing the uid/gui should be sufficient here.

@dilawar dilawar marked this pull request as ready for review July 4, 2025 05:02
@Xenira Xenira changed the title escalate privilege and try again to copy extension and edit ini file feat!(cargo-php)!: escalate privilege and to copy extension and edit ini file Jul 9, 2025
@Xenira Xenira changed the title feat!(cargo-php)!: escalate privilege and to copy extension and edit ini file feat(cargo-php)!: escalate privilege and to copy extension and edit ini file Jul 9, 2025
@dilawar dilawar requested a review from Xenira July 16, 2025 15:28
@Xenira
Copy link
Collaborator

Xenira commented Jul 17, 2025

@dilawar Thank you! Could you update the guide documentation to include the new behaviour?

The git commit history could benefit from a cleanup as well. See the CONTRIBUTING.md.

@dilawar
Copy link
Author

dilawar commented Jul 17, 2025

I'll definitely give it a shot, but I am terrible at writing! I'll rebase/cleanup git history.

@Xenira
Copy link
Collaborator

Xenira commented Jul 17, 2025

I'll definitely give it a shot, but I am terrible at writing! I'll rebase/cleanup git history.

I can do the documentation :)

@kakserpom
Copy link
Contributor

nix::unistd looks like a better fit.

dilawar and others added 3 commits July 20, 2025 08:07
And escalate sudo by default in both commands.
since libc is already in cargo.lock

Update crates/cli/src/lib.rs

Co-authored-by: Xenira <[email protected]>

Update crates/cli/src/lib.rs

comment -> doc

Co-authored-by: Xenira <[email protected]>

Update crates/cli/src/lib.rs

Co-authored-by: Xenira <[email protected]>

Update crates/cli/src/lib.rs

Co-authored-by: Xenira <[email protected]>

Update crates/cli/src/lib.rs

comment -> doc

Co-authored-by: Xenira <[email protected]>

Update crates/cli/src/lib.rs

remove commented out line

Co-authored-by: Xenira <[email protected]>

Update crates/cli/src/lib.rs

Co-authored-by: Xenira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider escalating priviledge to sudo automatically when copying .so file or editing ini file
4 participants