Skip to content

Commit bc27626

Browse files
garysmurrayclaude
andcommitted
fix: address Codex review — PHPCS suppression + legacy-header scoping + README auth
Three fixes from the third Codex review: P1: PHPCS suppression on the new auth query The new permissions-aware lookup in `authenticate_mcp_request` only suppressed `WordPress.DB.DirectDatabaseQuery`. Every comparable query in the codebase (RestApiKey's three lookup helpers, the analytics controller, the customer-value test fixture) suppresses `WordPress.DB.PreparedSQL.InterpolatedNotPrepared` alongside it, since `{$wpdb->prefix}` interpolation triggers that sniff. Local PHPCS happened to pass without it but the convention exists for a reason — CI may be stricter or the rule may light up under future upstream tightening. Match the convention. P2: legacy X-MCP-API-Key bundles re-scoped The previous commit dropped X-MCP-API-Key reading from `extract_request_credential` "for consistency" with the auth callback. Wrong move: the credential extractor's job is defensive recognition, not authentication, and pre-migration `.mcpb` bundles distributed against earlier plugin versions still send the header to /wp-json/woocommerce/mcp (the deprecated WC core MCP endpoint, which still authenticates the same WC API key when its feature flag is on). Without the extractor reading the header, `enforce_setup_key_route_scope` treats those legacy requests as unrelated and lets them through — silently keeping the credential usable on a route the merchant didn't intend. Restore the X-MCP-API-Key branch in the extractor (defense-only, not a supported auth path on the new endpoint) and pin the deny behaviour with a regression test against /wp-json/woocommerce/mcp. P3: README documents the new auth shape The architecture paragraph still claimed "authenticates via an X-MCP-API-Key header" after the Basic-auth migration. The setup snippets, MCPB manifest, and `authenticate_mcp_request` all use `WP_API_USERNAME` / `WP_API_PASSWORD` Basic auth now. Native HTTP clients following the old paragraph would send the wrong shape and get rejected. Updated to describe the actual contract (ck:cs Basic auth from a WC key with read or read_write scope). 297 PHPUnit tests pass (was 296). PHPCS clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 0aa4fc8 commit bc27626

4 files changed

Lines changed: 61 additions & 8 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ WooCommerce core already exposes basic product and order CRUD via MCP. This proj
182182
| **WooCommerce core MCP** | HTTP transport, auth, product/order CRUD tools | WooCommerce core team |
183183
| **Hey Woo plugin** | Analytics skills, knowledge resources, prompts, readiness scoring | This project |
184184

185-
Everything ships through the single endpoint at `/wp-json/hey-woo/mcp`. There is no separate MCP server process to run — the plugin registers its abilities and stands up its own MCP server on `mcp_adapter_init` using the WordPress MCP adapter (vendored inside WooCommerce). The server bundles tools, resources, and prompts directly, and authenticates via an `X-MCP-API-Key` header backed by a standard WooCommerce REST API key.
185+
Everything ships through the single endpoint at `/wp-json/hey-woo/mcp`. There is no separate MCP server process to run — the plugin registers its abilities and stands up its own MCP server on `mcp_adapter_init` using the WordPress MCP adapter (vendored inside WooCommerce). The server bundles tools, resources, and prompts directly, and authenticates via standard HTTP Basic auth — `ck_xxx` as the username, `cs_xxx` as the password, sourced from a WooCommerce REST API key with `read` or `read_write` scope.
186186

187187
---
188188

includes/class-plugin.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -514,7 +514,7 @@ public function authenticate_mcp_request( $request ) {
514514
}
515515

516516
global $wpdb;
517-
// phpcs:ignore WordPress.DB.DirectDatabaseQuery -- direct lookup against woocommerce_api_keys; no caching surface for auth.
517+
// phpcs:ignore WordPress.DB.DirectDatabaseQuery,WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- direct lookup against woocommerce_api_keys; table prefix is trusted; no caching surface for auth.
518518
$row = $wpdb->get_row(
519519
$wpdb->prepare(
520520
"SELECT key_id, user_id, consumer_secret, permissions FROM {$wpdb->prefix}woocommerce_api_keys WHERE consumer_key = %s",

includes/setup/class-setup-page.php

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -522,9 +522,20 @@ private static function route_is_allowed_for_setup_key( $route ) {
522522

523523
/**
524524
* Extract the credential from the current REST request, if any.
525-
* Mirrors the surfaces that WC's REST auth recognises: HTTP Basic
526-
* auth (split form via PHP_AUTH_USER/PW or raw HTTP_AUTHORIZATION
527-
* header) and `consumer_key` / `consumer_secret` query params.
525+
* Recognises the surfaces WC's REST auth reads (Basic auth split
526+
* form via PHP_AUTH_USER/PW; raw HTTP_AUTHORIZATION; query-string
527+
* `consumer_key` / `consumer_secret`) plus the legacy
528+
* `X-MCP-API-Key` header.
529+
*
530+
* The legacy header is no longer accepted by our MCP auth callback,
531+
* but pre-migration `.mcpb` bundles distributed against earlier
532+
* plugin versions still send it — typically targeting the
533+
* deprecated WC core MCP endpoint at `/wp-json/woocommerce/mcp`.
534+
* Reading it here means `enforce_setup_key_route_scope()` can
535+
* still recognise our credential on those legacy requests and
536+
* deny them on every route except `/wp-json/hey-woo/mcp`. Drop
537+
* the header here and a leaked legacy bundle silently keeps
538+
* authenticating against the WC core endpoint.
528539
*
529540
* Returns '' if no credential is present (the request will be
530541
* unauthenticated or rely on cookies/nonces instead, neither of
@@ -536,7 +547,16 @@ private static function extract_request_credential() {
536547
// phpcs:disable WordPress.Security.NonceVerification.Recommended -- read-only inspection of an in-flight REST request.
537548
// phpcs:disable WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- credentials are byte-compared, not interpolated; sanitisation would corrupt them.
538549

539-
// 1a. PHP_AUTH_USER + PHP_AUTH_PW — the split form Apache/mod_php
550+
// 1. Legacy X-MCP-API-Key header — kept for defense-in-depth so
551+
// pre-migration bundles still hit the route-scope deny path on
552+
// non-allowed routes. Not a supported auth path for the new
553+
// /wp-json/hey-woo/mcp endpoint (Plugin::authenticate_mcp_request
554+
// only reads Basic auth).
555+
if ( ! empty( $_SERVER['HTTP_X_MCP_API_KEY'] ) ) {
556+
return wp_unslash( (string) $_SERVER['HTTP_X_MCP_API_KEY'] );
557+
}
558+
559+
// 2a. PHP_AUTH_USER + PHP_AUTH_PW — the split form Apache/mod_php
540560
// (and many fastcgi setups) expose Basic auth as. WC's auth reads
541561
// these directly, so the route-scope filter must too — otherwise
542562
// a request bearing our setup credential against a non-MCP route
@@ -546,7 +566,7 @@ private static function extract_request_credential() {
546566
return wp_unslash( (string) $_SERVER['PHP_AUTH_USER'] ) . ':' . wp_unslash( (string) $_SERVER['PHP_AUTH_PW'] );
547567
}
548568

549-
// 1b. HTTP Basic auth (raw header form) — username:password = ck_xxx:cs_xxx.
569+
// 2b. HTTP Basic auth (raw header form) — username:password = ck_xxx:cs_xxx.
550570
$auth = isset( $_SERVER['HTTP_AUTHORIZATION'] ) ? wp_unslash( (string) $_SERVER['HTTP_AUTHORIZATION'] ) : '';
551571
if ( 0 === stripos( $auth, 'Basic ' ) ) {
552572
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode -- decoding HTTP Basic auth header per RFC 7617; strict mode rejects invalid input.
@@ -556,7 +576,7 @@ private static function extract_request_credential() {
556576
}
557577
}
558578

559-
// 2. Query string consumer_key + consumer_secret (WC legacy auth).
579+
// 3. Query string consumer_key + consumer_secret (WC legacy auth).
560580
if ( ! empty( $_GET['consumer_key'] ) && ! empty( $_GET['consumer_secret'] ) ) {
561581
return wp_unslash( (string) $_GET['consumer_key'] ) . ':' . wp_unslash( (string) $_GET['consumer_secret'] );
562582
}

tests/integration/test-rest-api-key.php

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,39 @@ public function test_setup_key_is_allowed_on_mcp_route_with_php_auth_basic() {
620620
}
621621
}
622622

623+
/**
624+
* Legacy `.mcpb` bundles distributed before the wordpress/mcp
625+
* migration ship the credential as `X-MCP-API-Key` and target the
626+
* deprecated WC core MCP endpoint at /wp-json/woocommerce/mcp. The
627+
* auth callback for the new endpoint no longer accepts that header,
628+
* but the route-scope filter must still recognise it — otherwise a
629+
* pre-migration bundle whose credential matches our stored one
630+
* silently bypasses the scope guard against the WC core endpoint
631+
* (which still authenticates the same WC API key when its feature
632+
* flag is on). Pin the deny path so a regression here doesn't
633+
* re-open that bypass.
634+
*/
635+
public function test_legacy_x_mcp_api_key_header_is_scope_denied_on_non_mcp_routes() {
636+
$helper = new RestApiKey();
637+
$state = $helper->get_or_create();
638+
639+
$original_server = $_SERVER;
640+
try {
641+
// phpcs:disable WordPress.Security.NonceVerification.Recommended -- test fixture mutates $_SERVER directly.
642+
unset( $_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW'], $_SERVER['HTTP_AUTHORIZATION'] );
643+
$_SERVER['HTTP_X_MCP_API_KEY'] = $state['credential'];
644+
$_SERVER['REQUEST_URI'] = '/wp-json/woocommerce/mcp';
645+
$result = SetupPage::enforce_setup_key_route_scope( null );
646+
647+
$this->assertInstanceOf( \WP_Error::class, $result, 'Legacy X-MCP-API-Key against the deprecated WC MCP route must be denied.' );
648+
$this->assertSame( 'hey_woo_route_restricted', $result->get_error_code() );
649+
$this->assertSame( 403, $result->get_error_data()['status'] ?? 0 );
650+
} finally {
651+
$_SERVER = $original_server;
652+
// phpcs:enable WordPress.Security.NonceVerification.Recommended
653+
}
654+
}
655+
623656
/**
624657
* The route-scope filter is a no-op when the request isn't using
625658
* our credential at all. Anything else would inadvertently

0 commit comments

Comments
 (0)