Skip to content
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

context=admin executes admin_init before anything on init is executed potentially causing fatal errors #6010

Open
2 tasks done
kkmuffme opened this issue Oct 22, 2024 · 5 comments · May be fixed by #6011
Open
2 tasks done
Assignees

Comments

@kkmuffme
Copy link

kkmuffme commented Oct 22, 2024

Bug Report

Describe the current, buggy behavior
Describe how other contributors can replicate this bug
Describe what you expect as the correct outcome

in theme functions.php add:

function init_cb() {
    var_dump( 'this is init' );
}
add_action( 'init', 'init_cb' );

function admin_init_cb() {
    var_dump( 'this is admin init' );
}
add_action( 'admin_init', 'admin_init_cb' );

Then run:

wp --user=1 --context=admin eval ""

You will get:

this is admin init
this is init

However, this is the opposite order of what it would normally be in an wp-admin request.

Let us know what environment you are running this on

WP 6.6
WP CLI 2.11.0

Provide a possible solution

  1. The issue is https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Context/Admin.php#L45 is run on init hook with earliest priority (PHP_INT_MIN)
    To keep it consistent with WP core, it needs to be loaded on wp_loaded with PHP_INT_MAX (last hook in settings.php)

  2. And the user auth https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Context/Admin.php#L44 is wrong too. Normally the user is set up already on plugins_loaded - therefore that needs to be called on plugins_loaded with PHP_INT_MIN

  3. Also there's a bug here https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Context/Admin.php#L63 - when a --user is provided, it should use that user and not arbitrarily fall back to ID 1 since this will make current_user_can checks fail and therefore the context=admin is pointless, since it's an unauthenticated request anyway
    EDIT: looks like the correct code is there https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Runner.php#L1655 but the user then gets overwritten by the wrong user in https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Context/Admin.php#L63 (since that init hook was added later it runs after) - I guess a simple check whether the user is set already in https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Context/Admin.php#L63 would be sufficient before overwriting it with id 1

@swissspidy
Copy link
Member

Normally the user is set up already on plugins_loaded

That's not normal. It's supposed to happen right before init (https://github.com/WordPress/wordpress-develop/blob/771ef176d770dce40491d39007262d308d08585d/src/wp-settings.php#L690-L691) or right after setup_theme if in the admin (https://github.com/WordPress/wordpress-develop/blob/771ef176d770dce40491d39007262d308d08585d/src/wp-settings.php#L640-L641)

@kkmuffme
Copy link
Author

kkmuffme commented Oct 22, 2024

plugins_loaded is the earliest hook where you can check current_user_can() reliably since any plugin (and WP core) filters on 'determine_current_user' are loaded at that point

@kkmuffme
Copy link
Author

kkmuffme commented Oct 22, 2024

Also there's a bug here https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Context/Admin.php#L63 - when a --user is provided, it should use that user and not arbitrarily fall back to ID 1 since this will make current_user_can checks fail and therefore the context=admin is pointless, since it's an unauthenticated request anyway

EDIT: looks like the correct code is there https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Runner.php#L1655 but the user then gets overwritten by the wrong user in https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Context/Admin.php#L63 (since that init hook was added later it runs after) - I guess a simple check whether the user is set already in https://github.com/wp-cli/wp-cli/blob/main/php/WP_CLI/Context/Admin.php#L63 would be sufficient before overwriting it with id 1

@EloB
Copy link

EloB commented Nov 26, 2024

This seems to be completely broken at the moment. Here is a working reproduction script.

# Create docker compose file
cat <<EOT > docker-compose.yml
services:
  wordpress:
    image: wordpress:latest
    container_name: wordpress
    environment:
      WORDPRESS_DB_NAME: example_db
      WORDPRESS_DB_USER: example_user
      WORDPRESS_DB_PASSWORD: example_pass
      WORDPRESS_DB_HOST: db:3306
    ports:
      - "8000:80"
    volumes:
      - wordpress_data:/var/www/html
    depends_on:
      - db

  db:
    image: mysql:latest
    container_name: wordpress_db
    environment:
      MYSQL_DATABASE: example_db
      MYSQL_USER: example_user
      MYSQL_PASSWORD: example_pass
      MYSQL_ROOT_PASSWORD: example_root_pass
    volumes:
      - db_data:/var/lib/mysql

volumes:
  wordpress_data:
  db_data:
EOT

# Initialize containers
docker compose up -d

# Install dependencies and WP CLI
docker compose exec -T wordpress bash <<EOF
apt-get update
apt-get install -y less mariadb-client
curl -O https://raw.githubusercontent.com/wp-cli/builds/gh-pages/phar/wp-cli.phar
php wp-cli.phar --info  # This will verify WP-CLI is installed correctly
chmod +x wp-cli.phar
mv wp-cli.phar /usr/local/bin/wp
EOF

# Install wordpress
docker compose exec -T -u www-data wordpress bash <<EOT
until wp db check; do
  sleep 1
done
wp core install --url=http://localhost:8000 --title="Test" --admin_user=admin --admin_password=password [email protected]
EOT

# Testing that eval works (outputs 50 like expected)
docker compose exec -T -u www-data wordpress wp eval "$(cat <<'EOT'
$value = 13;
echo ($value+37) . "\n";
EOT
)"

# Testing eval with context (nothing happens)
docker compose exec -T -u www-data wordpress wp --user=1 --context=admin eval "$(cat <<'EOT'
echo 'Test with: --user=1 --context=admin';

function init_cb() {
    var_dump( 'this is init' );
}
add_action( 'init', 'init_cb' );

function admin_init_cb() {
    var_dump( 'this is admin init' );
}
add_action( 'admin_init', 'admin_init_cb' );

var_dump(function_exists('add_action'));
EOT
)"

docker compose exec -T -u www-data wordpress wp --context=admin eval "$(cat <<'EOT'
echo 'Test with: --context=admin';

function init_cb() {
    var_dump( 'this is init' );
}
add_action( 'init', 'init_cb' );

function admin_init_cb() {
    var_dump( 'this is admin init' );
}
add_action( 'admin_init', 'admin_init_cb' );

var_dump(function_exists('add_action'));
EOT
)"

docker compose exec -T -u www-data wordpress wp eval "$(cat <<'EOT'
echo 'Test with nothing';

function init_cb() {
    var_dump( 'this is init' );
}
add_action( 'init', 'init_cb' );

function admin_init_cb() {
    var_dump( 'this is admin init' );
}
add_action( 'admin_init', 'admin_init_cb' );

var_dump(function_exists('add_action'));
EOT
)"

@EloB
Copy link

EloB commented Nov 26, 2024

Looking at the test again I see that admin_init shouldn't run... So maybe it works as intended.

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 a pull request may close this issue.

4 participants