Skip to content

Check for root earlier #5987

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

Merged
merged 6 commits into from
May 6, 2025
Merged

Check for root earlier #5987

merged 6 commits into from
May 6, 2025

Conversation

schlessera
Copy link
Member

The check for running WP-CLI as a root user inadvertently is running pretty late and allows for mechanisms like the --exec flag to be executed already before the root user is being checked for.

This PR moves the logic for this check around so that it is included as its own step in the bootstrapping steps and happens at the earliest occasion, before other flags are being processed.

@schlessera schlessera added this to the 2.12.0 milestone Sep 19, 2024
@schlessera schlessera requested a review from a team as a code owner September 19, 2024 01:32
@schlessera
Copy link
Member Author

/cc @johnbillion

@schlessera schlessera force-pushed the fix/check-for-root-earlier branch from dafa9b4 to 8083d35 Compare September 19, 2024 01:35
@johnbillion
Copy link
Contributor

Is there a way to add tests for this that mock the return value of posix_geteuid()?

@schlessera
Copy link
Member Author

We could add a helper that wraps the posix_geteuid() function (and does the compatibility check, returning something like -1 if not available).

Then the tests could check the order/timing of the root block.

@phanduynam
Copy link

Does no one have a solution to this problem?

@phanduynam
Copy link

@swissspidy I see this issue has been resolved but no one has closed it yet

@swissspidy
Copy link
Member

@phanduynam I don't know what you mean 🤔 This is a pull request, not an issue. And it's still open, and not merged. So it's not "resolved".

@phanduynam
Copy link

@swissspidy I don't quite understand, all the tests are passed, why not close?

Copy link

codecov bot commented Apr 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
php/WP_CLI/Bootstrap/CheckRoot.php 0.00% 30 Missing ⚠️
php/WP_CLI/Bootstrap/ConfigureRunner.php 0.00% 3 Missing ⚠️
php/bootstrap.php 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@schlessera schlessera mentioned this pull request Apr 29, 2025
47 tasks
@mrsdizzie
Copy link
Member

mrsdizzie commented Apr 30, 2025

If the only thing this is waiting on is a test, I believe this would cover everything mentioned above:

  Feature: Bootstrap WP-CLI

  Scenario: Test early root detection

    Given an empty directory
    And a include.php file:
    """
    <?php
      namespace WP_CLI\Bootstrap;

      // To override posix_geteuid in our namespace
      function posix_geteuid() {
        return 0;
      }
    ?>
    """

    And I try `WP_CLI_EARLY_REQUIRE=include.php wp cli version --debug`

    Then STDERR should contain:
    """
    WP_CLI\Bootstrap\CheckRoot
    """

    And STDERR should not contain:
    """
    WP_CLI\Bootstrap\IncludeRequestsAutoloader
    """

    And STDERR should contain:
    """
    YIKES!
    """

Happy to add to the PR if that seems OK

@schlessera
Copy link
Member Author

@mrsdizzie Yes, that looks like what we need. If you add it here, I'll merge.

@schlessera
Copy link
Member Author

I'll put this test in to move forward with the release. Thanks for the prep work on this, @mrsdizzie !

@schlessera
Copy link
Member Author

Confirmed that the test fails without and succeeds with the fix.

@schlessera
Copy link
Member Author

Ignoring code coverage here to get it merged.

@schlessera schlessera merged commit 7e8b548 into main May 6, 2025
51 of 52 checks passed
@schlessera schlessera deleted the fix/check-for-root-earlier branch May 6, 2025 14:24
@TimothyBJacobs
Copy link
Contributor

This seems to break passing --allow-root=1. Previously, it was a simple truthy check, now it's a strict true check.

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

Successfully merging this pull request may close these issues.

6 participants