fix: prevent mcp-oauth's duplicate server registration instead of tolerating it
Proactively unhook wp-media/mcp-oauth's own mcp_adapter_init callback before creating our server, rather than letting it collide and rely on WP_DEBUG=true being harmless. The collision previously surfaced as a fatal-looking _doing_it_wrong notice under WP-CLI (breaking every wp command while the plugin is active) and could leak debug notices into HTTP responses on customer sites that run WP_DEBUG. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7
This commit is contained in:
@@ -9,14 +9,19 @@ use LIT\MCP\Integrations\IntegrationRegistry;
|
||||
use WP\MCP\Core\McpAdapter;
|
||||
use WP\MCP\Infrastructure\ErrorHandling\ErrorLogMcpErrorHandler;
|
||||
use WPMedia\MCP\OAuth\Transport\OAuthHttpTransport;
|
||||
use WPMedia\MCP\OAuth\Transport\ServerRegistrar as OAuthServerRegistrar;
|
||||
|
||||
/**
|
||||
* Registers the OAuth-protected MCP server with our own tool list.
|
||||
*
|
||||
* wp-media/mcp-oauth hardcodes server id/route (token audience) and only three
|
||||
* meta-tools. We register the same server id first (priority 5 < mcp-oauth's 10);
|
||||
* mcp-oauth's later attempt fails with "duplicate server id" (logged via
|
||||
* _doing_it_wrong in debug mode only) – see spec §4.2.
|
||||
* meta-tools, and registers its own server unconditionally on the same
|
||||
* `mcp_adapter_init` action (priority 10). We register the same server id
|
||||
* first (priority 5) and, right before creating it, unhook mcp-oauth's own
|
||||
* `ServerRegistrar::register_server` callback so it never runs – see
|
||||
* `unhookOAuthDefaultServer()`. This avoids the duplicate-server-id WP_Error
|
||||
* (and the `_doing_it_wrong()` notice it used to trigger) entirely instead of
|
||||
* merely tolerating it – see spec §4.2.
|
||||
*/
|
||||
final class ServerRegistrar
|
||||
{
|
||||
@@ -79,6 +84,8 @@ final class ServerRegistrar
|
||||
return;
|
||||
}
|
||||
|
||||
$this->unhookOAuthDefaultServer();
|
||||
|
||||
$result = McpAdapter::instance()->create_server(
|
||||
$args['id'],
|
||||
$args['namespace'],
|
||||
@@ -97,4 +104,36 @@ final class ServerRegistrar
|
||||
error_log($message); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Removes wp-media/mcp-oauth's own `mcp_adapter_init` callback so it never attempts to
|
||||
* register its (now redundant) copy of the `mcp-oauth-server` server.
|
||||
*
|
||||
* Reads the raw `WP_Hook` for `mcp_adapter_init` from `$GLOBALS['wp_filter']` – there is no
|
||||
* public API to look up a hooked callback by the class of its bound object, only by the exact
|
||||
* callable, which we don't have here. Removing a same-action, later-priority callback while an
|
||||
* earlier-priority one for the same action is running is explicitly supported by
|
||||
* `WP_Hook::remove_filter()` (it calls `resort_active_iterations()` for exactly this case).
|
||||
* No-op if the hook has not fired yet, has no callbacks, or the mcp-oauth package is absent.
|
||||
*/
|
||||
public function unhookOAuthDefaultServer(): void
|
||||
{
|
||||
if (! class_exists(OAuthServerRegistrar::class)) {
|
||||
return;
|
||||
}
|
||||
|
||||
$hook = $GLOBALS['wp_filter']['mcp_adapter_init'] ?? null;
|
||||
if (! is_object($hook) || ! isset($hook->callbacks) || ! is_array($hook->callbacks)) {
|
||||
return;
|
||||
}
|
||||
|
||||
foreach ($hook->callbacks as $priority => $callbacks) {
|
||||
foreach ($callbacks as $callback) {
|
||||
$function = $callback['function'] ?? null;
|
||||
if (is_array($function) && ($function[0] ?? null) instanceof OAuthServerRegistrar) {
|
||||
remove_action('mcp_adapter_init', $function, $priority);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -12,9 +12,16 @@ use LIT\MCP\Integrations\IntegrationRegistry;
|
||||
use LIT\MCP\Server\ServerRegistrar;
|
||||
use LIT\MCP\Tests\Unit\TestCase;
|
||||
use Mockery;
|
||||
use WPMedia\MCP\OAuth\Transport\ServerRegistrar as OAuthServerRegistrar;
|
||||
|
||||
final class ServerRegistrarTest extends TestCase
|
||||
{
|
||||
protected function tearDown(): void
|
||||
{
|
||||
unset($GLOBALS['wp_filter']);
|
||||
parent::tearDown();
|
||||
}
|
||||
|
||||
private function registry(): IntegrationRegistry
|
||||
{
|
||||
$direct = Mockery::mock(Ability::class);
|
||||
@@ -77,4 +84,37 @@ final class ServerRegistrarTest extends TestCase
|
||||
|
||||
self::assertFalse($oauthChecked, 'createServer must bail out before touching the adapter');
|
||||
}
|
||||
|
||||
public function testUnhookOAuthDefaultServerRemovesOnlyTheMcpOauthCallback(): void
|
||||
{
|
||||
$oauthRegistrar = Mockery::mock(OAuthServerRegistrar::class);
|
||||
$oauthCallback = [$oauthRegistrar, 'register_server'];
|
||||
$unrelatedCallback = static function (): void {
|
||||
};
|
||||
|
||||
$hook = new class {
|
||||
/** @var array<int, array<string, array{function: mixed, accepted_args: int}>> */
|
||||
public array $callbacks = [];
|
||||
};
|
||||
$hook->callbacks = [
|
||||
5 => ['unrelated-id' => ['function' => $unrelatedCallback, 'accepted_args' => 0]],
|
||||
10 => ['oauth-id' => ['function' => $oauthCallback, 'accepted_args' => 0]],
|
||||
];
|
||||
$GLOBALS['wp_filter']['mcp_adapter_init'] = $hook;
|
||||
|
||||
Functions\expect('remove_action')
|
||||
->once()
|
||||
->with('mcp_adapter_init', $oauthCallback, 10);
|
||||
|
||||
(new ServerRegistrar($this->registry(), $this->instructions()))->unhookOAuthDefaultServer();
|
||||
}
|
||||
|
||||
public function testUnhookOAuthDefaultServerIsNoOpWhenHookIsMissing(): void
|
||||
{
|
||||
unset($GLOBALS['wp_filter']['mcp_adapter_init']);
|
||||
|
||||
Functions\expect('remove_action')->never();
|
||||
|
||||
(new ServerRegistrar($this->registry(), $this->instructions()))->unhookOAuthDefaultServer();
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user