Add project configuration trust verification (trust-project-config) - #6365
Add project configuration trust verification (trust-project-config)#6365swissspidy wants to merge 26 commits into
trust-project-config)#6365Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWP-CLI now tracks project and global ChangesProject Config Trust
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ProjectConfig
participant LoadExecCommand
participant LoadRequiredCommand
participant TrustCheck
participant GlobalConfig
ProjectConfig->>LoadExecCommand: provide project exec directives
ProjectConfig->>LoadRequiredCommand: provide project require directives
LoadExecCommand->>TrustCheck: validate project exec trust
LoadRequiredCommand->>TrustCheck: validate project require trust
TrustCheck->>GlobalConfig: persist approved project path
TrustCheck-->>LoadExecCommand: allow or reject exec evaluation
TrustCheck-->>LoadRequiredCommand: allow or reject required files
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Fixed 3 file(s) based on 5 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
…ve PHPCS/PHPStan warnings
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…oding, and expanded acceptance tests
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… and transactional locking for config updates
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…er is uninitialized
… code-executing directives
- Defer the trust check until after runtime config parsing so that an explicit `--trust-project-config` flag is honored. - Derive the reported directive label (exec/require/generic) from the gated directives instead of always using the generic label. - Remove an orphaned docblock in utils.php. - Do not truncate the trust store when json_encode() fails. - Escape the config path in all user-facing messages and include it in the non-interactive error. - Treat an empty WP_CLI_TRUST_PROJECT_CONFIG as unset. - Surface the "allow always" confirmation when no logger is set up yet.
- Move the gated directive collection out of the bare config block so the indentation matches the coding standard. - Predeclare the system/global snapshots so PHPStan can see them. - Harden load_trusted_configs() against a malformed trust store. - Add trust-project-config to the wp-cli.yml JSON schema.
The project-config trust gate only flagged aliases whose *name* was new (`array_diff_key` against the system/global aliases). A project wp-cli.yml could therefore redefine an existing, commonly-named alias (e.g. `@prod`) to add `ssh-args: -oProxyCommand=...`, and the injected SSH option would execute locally when the operator invoked that alias — without ever tripping the trust prompt. Compare ssh-args per alias instead: gate any project alias whose ssh-args is new or differs from the same-named alias in the trusted system/global config. An existing alias with unchanged ssh-args stays trusted, so this adds no new prompts for already-trusted configurations. Adds a regression scenario covering redefinition of a global alias. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LV1bNtxNCZ3QXujJHYfhZv
swissspidy
left a comment
There was a problem hiding this comment.
Reviewed the trust mechanism end-to-end from a security-audit angle. Overall this is a solid, well-structured design: snapshotting global_trust_config from system/global config before the project merge correctly prevents a project config from self-authorizing (nicely covered by the dedicated scenario); the store is content-hashed and 0600; the interactive prompt control-char-escapes the attacker-controlled path and directives; and non-interactive contexts default to deny. 👍
I pushed one commit fixing a gap I found (squash/adjust/drop as you like):
Alias ssh-args gating missed redefinition of an existing alias. The gate used array_diff_key($project_aliases, $global_aliases), which only flags aliases whose name is new. But add_alias() overwrites an existing entry, so a project wp-cli.yml could redefine a commonly-named existing alias (@prod, @staging, …) to add ssh-args: -oProxyCommand=… and slip past the gate — then execute locally when the operator ran wp @prod …. Existing new-alias tests still pass. The commit switches to a per-alias comparison (gate when ssh-args is new or differs from the same-named system/global alias) and adds a regression scenario.
Three things I did not change, for your call:
- Trust hash covers only
wp-cli.yml, not the files itrequire:s. Once a config is trusted, editing arequired PHP file without touchingwp-cli.ymlruns the new code without re-prompting — same model as direnv (.envrcvs. sourced files). Likely acceptable; worth a doc note. - Redefining an existing alias's connection target (not
ssh-args) isn't gated. A project config can repoint@prod'sssh:host;validate_ssh_bits()blocks option-injection but not a plain hostname swap, sowp @prod …could silently connect to an attacker's host (using the operator's key). Not code-exec and lower severity, but it's the same "redefine a trusted alias" class as thessh-argsgap — you may want to gate any project redefinition of an existing alias. - BC note for the changelog: in non-interactive contexts (CI), existing users with
require:/exec:in a projectwp-cli.ymlnow hit a hard error until they setWP_CLI_TRUST_PROJECT_CONFIG=trueor allowlist the path. That's the intended secure-by-default behavior — just worth calling out prominently.
Generated by Claude Code
Summary
Adds a project configuration trust mechanism (
--trust-project-config/WP_CLI_TRUST_PROJECT_CONFIG) so that a project-levelwp-cli.ymlcannot execute arbitrary code just because someonecds into the directory and runswp.What is gated
Trust is verified for the directives that a project
wp-cli.ymlintroduces on top of the system/global configuration:require— loads a PHP fileexec— runs PHP codeenv— sets WP-CLI environment variables such asWP_CLI_PACKAGES_DIR, excluding an allowlist of inert keys (WP_ENV,WP_DEBUG,WP_DEBUG_LOG,WP_DEBUG_DISPLAY)ssh-args, includingssh-argson an alias, which can smuggle aProxyCommandBenign keys (
path,url,user, …) are not gated. A project config that introduces none of the above is untouched.How trust is granted
Checked in order, in
Utils\check_project_config_trust():--trust-project-configon the command line (true,false, or a path/list of paths).trust-project-configin the system or global config file. This value is snapshotted before the project config is merged, so a project cannot grant itself trust by settingtrust-project-config: truein its ownwp-cli.yml.~/.wp-cli/trusted-configs.json, which maps a canonical config path to the SHA-256 hash of its contents. A previously trusted file that has since been modified is re-prompted rather than silently trusted.WP_CLI_TRUST_PROJECT_CONFIGenvironment variable, same accepted values as the flag.[y/n/a]prompt on a TTY.awrites the path and content hash to the trust store.Outside a TTY, an untrusted project config is a hard error rather than a prompt — including under
--yes, which deliberately does not grant trust.Implementation notes
Runner::init_config()rather than in the individual bootstrap steps, so it coversenv/ssh-args/alias directives that are consumed beforeLoadExecCommandandLoadRequiredCommandrun. It happens after runtime arguments are parsed so that--trust-project-configis honored.WP_CLI::warning()andWP_CLI::error()fall back to STDERR when they are called before the logger is initialized, which is the case for the trust check.Tests
Unit tests in
tests/UtilsTest.phpand acceptance scenarios infeatures/config.feature.Note: this needs wp-cli/wp-cli-tests#348 to be released before CI can go green — Behat fixtures create project
wp-cli.ymlfiles withrequiredirectives all the time, and a non-interactive test run cannot answer the trust prompt.