Better connector approval matching

[dkotter]

What?

See #467 (comment)

Add a few more paths that we always allow through for the Connector Approval experiment (to avoid blocking authentication requests). And change our approval matching to look through the entire stack and find the deepest match instead of the first match.

Why?

Right now the Connector Approval matching is a little too aggressive. First, in some situations it will block the WP AI Client from doing authentication requests without first approving the AI plugin. And second, it some situations it will block the individual AI Provider plugins from being used without first being approved.

Ideally any requests that originate from WP Core (like authentication of credentials) or requests that filter through a Provider plugin (but don't originate there) are allowed through without approvals.

How?

• Add more allowed paths to our list: /includes/Logging/, /includes/Settings/ and /includes/helpers.php. This allows things like the Logging experiment and credential authentication to work without first needing the AI plugin to be approved
• Change our resolve functionality to look through the entire stack and find the deepest match which hopefully gets us closer to the originating code

Use of AI Tools

AI assistance: Yes
Tool(s): Cursor
Model(s): GPT-5.5
Used for: Review of the current code, coming up with a plan and implementing that plan. Iteration and testing done by me

Testing Instructions

1. On the current release, turn on the Connector Approval and Request Logging experiments
2. Ensure nothing is approved under Tools > Connector Approvals
3. Remove credentials from any Connector
4. Try adding credentials back. You should get an error message when saving and you should see a request to allow the AI plugin on the Connector Approvals page
5. Approve the AI plugin and turn on a feature like Title Generation
6. Try to generate a title. You should get an error that the AI Provider isn't approved
7. Go to the Connector Approvals screen and you should see a request there to approve the AI Provider plugin
8. Check out this branch
9. Ensure nothing is approved under Tools > Connector Approvals
10. Remove credentials from any Connector
11. Try adding credentials back and save. It should work now and you shouldn't see any request to allow the AI plugin
12. Try generating a title and it should be blocked and you should now be prompted to approve the AI plugin
13. Approve the AI plugin and try generating titles again
14. It should work now without having to approve the AI Provider plugin

Changelog Entry

Fixed - If the Connector Approval experiment is turned on, ensure we don't over aggressively block functionality in the AI plugin that isn't actually making requests, like request logging.
Fixed - Better matching of the originating code when the Connector Approval experiment is on.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: dkotter <dkotter@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: jeffpaul <jeffpaul@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[codecov]

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 73.43%. Comparing base (246680c) to head (76c41d9).

Files with missing lines	Patch %	Lines
includes/Connector_Approval/Caller_Identifier.php	96.66%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #595      +/-   ##
=============================================
+ Coverage      73.18%   73.43%   +0.24%     
- Complexity      1731     1733       +2     
=============================================
  Files             85       85              
  Lines           7473     7483      +10     
=============================================
+ Hits            5469     5495      +26     
+ Misses          2004     1988      -16     

Flag	Coverage Δ
unit	73.43% <96.66%> (+0.24%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jeffpaul]

Pinging @t-hamano for review

[t-hamano]

Thanks for the PR.

I would like to state my concerns, but I don't fully understand the Content Approval experiment, so please let me know if I'm mistaken.

The skip_substrings are intended to define paths that should be skipped. However, the should_skip() method checks file paths for partial matches. This means that if a plugin developer sends a request from a file like plugins/my-plugin/includes/Settings/Options.php, it will be unintentionally skipped. Consequently, it will be determined as having no caller, and the request will be implicitly allowed. Is my understanding correct?

If that is the case, I think we might need to reconsider the original skip logic. For example, how about the following code?

diff --git a/includes/Connector_Approval/Caller_Identifier.php b/includes/Connector_Approval/Caller_Identifier.php
index 454d762..b139fd8 100644
--- a/includes/Connector_Approval/Caller_Identifier.php
+++ b/includes/Connector_Approval/Caller_Identifier.php
@@ -70,13 +70,13 @@ final class Caller_Identifier {
 	private array $cache = array();
 
 	/**
-	 * Substrings that indicate a stack frame is infrastructure rather than a request origin.
+	 * Path prefixes that indicate a stack frame is infrastructure rather than a request origin.
 	 *
 	 * @since 1.0.0
 	 *
 	 * @var list<string>
 	 */
-	private array $skip_substrings;
+	private array $skip_prefixes;
 
 	/**
 	 * Constructor.
@@ -84,24 +84,27 @@ final class Caller_Identifier {
 	 * @since 1.0.0
 	 */
 	public function __construct() {
-		$this->skip_substrings = array(
-			'/wp-includes/option.php',
-			'/wp-includes/class-wp-hook.php',
-			'/wp-includes/plugin.php',
-			'/wp-includes/connectors.php',
-			'/wp-includes/class-wp-connector-registry.php',
-			'/wp-includes/http.php',
-			'/wp-includes/class-wp-http.php',
-			'/wp-includes/class-http.php',
-			'/wp-includes/class-wp-http-requests-hooks.php',
-			'/wp-includes/Requests/',
-			'/wp-includes/class-requests.php',
-			'/wp-includes/ai-client/',
-			'/wp-includes/php-ai-client/',
-			DIRECTORY_SEPARATOR . 'includes' . DIRECTORY_SEPARATOR . 'Connector_Approval' . DIRECTORY_SEPARATOR,
-			DIRECTORY_SEPARATOR . 'includes' . DIRECTORY_SEPARATOR . 'Logging' . DIRECTORY_SEPARATOR,
-			DIRECTORY_SEPARATOR . 'includes' . DIRECTORY_SEPARATOR . 'Settings' . DIRECTORY_SEPARATOR,
-			DIRECTORY_SEPARATOR . 'includes' . DIRECTORY_SEPARATOR . 'helpers.php',
+		$core   = wp_normalize_path( ABSPATH . WPINC ) . '/';
+		$plugin = wp_normalize_path( WPAI_PLUGIN_DIR ) . 'includes/';
+
+		$this->skip_prefixes = array(
+			$core . 'option.php',
+			$core . 'class-wp-hook.php',
+			$core . 'plugin.php',
+			$core . 'connectors.php',
+			$core . 'class-wp-connector-registry.php',
+			$core . 'http.php',
+			$core . 'class-wp-http.php',
+			$core . 'class-http.php',
+			$core . 'class-wp-http-requests-hooks.php',
+			$core . 'Requests/',
+			$core . 'class-requests.php',
+			$core . 'ai-client/',
+			$core . 'php-ai-client/',
+			$plugin . 'Connector_Approval/',
+			$plugin . 'Logging/',
+			$plugin . 'Settings/',
+			$plugin . 'helpers.php',
 		);
 	}
 
@@ -189,8 +192,8 @@ final class Caller_Identifier {
 	 */
 	private function should_skip( string $file ): bool {
 		$normalized = wp_normalize_path( $file );
-		foreach ( $this->skip_substrings as $needle ) {
-			if ( str_contains( $normalized, wp_normalize_path( $needle ) ) ) {
+		foreach ( $this->skip_prefixes as $prefix ) {
+			if ( str_starts_with( $normalized, wp_normalize_path( $prefix ) ) ) {
 				return true;
 			}
 		}
diff --git a/tests/Integration/Includes/Connector_Approval/Http_GuardTest.php b/tests/Integration/Includes/Connector_Approval/Http_GuardTest.php
index 9f4ac1e..5bd1fcc 100644
--- a/tests/Integration/Includes/Connector_Approval/Http_GuardTest.php
+++ b/tests/Integration/Includes/Connector_Approval/Http_GuardTest.php
@@ -169,7 +169,7 @@ class Http_GuardTest extends WP_UnitTestCase {
 	 * @since 1.0.0
 	 */
 	private function force_unidentifiable_caller(): void {
-		$property = new ReflectionProperty( Caller_Identifier::class, 'skip_substrings' );
+		$property = new ReflectionProperty( Caller_Identifier::class, 'skip_prefixes' );
 		$property->setAccessible( true );
 
 		$current   = (array) $property->getValue( $this->identifier );

[dkotter]

@t-hamano Thanks for the feedback, that's super helpful! And yeah, your suggestion is definitely a better matching approach. Pushed that change in 68cd4be and this seems to still work as expected for me.

[t-hamano]

LGTM 👍