Make WordPress Core

Opened 5 weeks ago

Last modified 10 days ago

#65821 accepted defect (bug)

Connectors: a masked API key posted back to /wp/v2/settings overwrites the stored key

Reported by: bejignesh Owned by: gziolo
Priority: normal Milestone: 7.2
Component: AI Version: 7.0
Severity: normal Keywords: has-patch has-unit-tests commit
Cc: Focuses: rest-api

Description

_wp_connectors_rest_settings_dispatch() masks connector API keys in /wp/v2/settings responses, but those settings are registered with sanitize_text_field as their sanitize callback, so nothing recognises the mask on the way back in. A client that reads settings and posts them back, the ordinary read-modify-write pattern, writes the mask over the stored key.

Reproduced on trunk (7.1-beta4-62899-src) with Akismet installed, since core registers its wordpress_api_key as a connector setting with show_in_rest:

  1. Store a key: wp option update wordpress_api_key 'ak-live-9f3b2c8d1e4a7601'
  2. GET /wp/v2/settings returns wordpress_api_key: "••••••••••••••••7601"
  3. POST that value back, 200 OK
  4. The option now holds ••••••••••••••••7601

Step 4 is with only core acting on the option. With Akismet's own rest_post_dispatch validation left in place the option ends up empty instead, because Akismet verifies the stored value, which is now the mask, and clears it. Removing that one filter is what leaves the mask in the option, so both outcomes come from the same write. Either way the key is gone, and because the response still shows a masked looking value, nothing on screen says so.

For ai_provider connectors the value that reaches validation is the mask rather than the user's key, and the same dispatch filter discards it unless validation returns exactly true:

if ( true !== _wp_connectors_is_ai_api_key_valid( $value, $connector_id ) ) {
	update_option( $setting_name, '' );

The application_password method added in 7.1 already guards against this. wp_connectors_sanitize_application_password_credentials() keeps the stored password when the submitted one matches the mask, and tests/phpunit/tests/connectors/wpConnectorsSanitizeApplicationPasswordCredentials.php covers the round trip. The api_key method never got the same treatment.

Patch adds wp_connectors_sanitize_api_key(), which returns the stored key when the submitted value is exactly that key's mask, and registers it as the sanitize callback for API key settings. Everything else still goes through sanitize_text_field(), so a new key still replaces the old one and an empty string still clears it.

Separate from #65551 and #65554. Both of those concern the AI validation step, neither stops the mask reaching the database, and neither applies to connectors that are not AI providers.

Change History (8)

This ticket was mentioned in PR #12865 on WordPress/wordpress-develop by @bejignesh.


5 weeks ago
#1

  • Keywords has-patch has-unit-tests added

Connector API keys are masked in /wp/v2/settings responses but sanitized with sanitize_text_field(), so posting a settings response back unchanged stores the mask in place of the key. For ai_provider connectors the value that reaches validation is then the mask rather than the user's key, and it is discarded unless validation returns exactly true.

Adds wp_connectors_sanitize_api_key(), which keeps the stored key when the submitted value is exactly that key's mask, and uses it as the sanitize callback for API key settings. This mirrors wp_connectors_sanitize_application_password_credentials(), which already does this for the application_password method.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Drafting the fix and the test file. I reproduced the data loss on a running site and in PHPUnit, confirmed the new assertions fail without the source change, and ran the test groups and linters. I have reviewed the change and take responsibility for it.

#2 @gziolo
11 days ago

  • Milestone Awaiting Review7.2
  • Owner set to gziolo
  • Status newreviewing

@gziolo commented on PR #12865:


11 days ago
#3

Thanks for this fix, and for the clear description. Storing the mask in place of the key is a real bug and the sanitize callback is the right place to solve it.

## Action required

A resubmitted mask is still validated against the provider, and can wipe the key

With this change the sanitizer keeps the stored key, so WP_REST_Settings_Controller::update_item() returns the real key in the response. _wp_connectors_rest_settings_dispatch() then sees $is_update && $request->has_param( $setting_name ) with a non-empty value and calls _wp_connectors_is_ai_api_key_valid() for ai_provider connectors. We should skip validation when the submitted value is the mask of the stored key, since there is nothing new to validate:

if ( $is_update
        && $request->has_param( $setting_name )
        && is_string( $value ) && '' !== $value
        && _wp_connectors_mask_api_key( $value ) !== $request->get_param( $setting_name )
        && 'ai_provider' === $connector_data['type']
) {

A test in wpConnectorsRestSettingsDispatch.php would cover it well. The file already has a mock provider. Set self::set_mock_provider_configured( false ), POST the mask of the stored key, and assert the option is unchanged.

## Nice to have

One masking helper for both auth methods

The password mask is currently written out as a literal in two places, and the key mask lives in _wp_connectors_mask_api_key(). It would be good to have a single private helper that produces the mask for any credential value: the full fixed-length mask by default, or a mask that keeps the last few characters visible when asked. _wp_connectors_mask_api_key() can then call that helper with a visible suffix of 4, and the dispatch function and wp_connectors_sanitize_application_password_credentials() can call it for passwords. That way the two auth methods always use the same masking rules.

State the exact-match rule

Treating a value as a placeholder only when it exactly matches the mask of the stored key is a good rule. It has no false positives and every other input behaves as before. It would help to say this in the docblock ("all other values are sanitized as text, as before") and to add a test that a value starting with a bullet but not matching the mask is stored as a new key, for both API keys and application passwords.

@bejignesh commented on PR #12865:


10 days ago
#4

@gziolo Confirmed, and it does wipe the key. I registered an ai_provider connector whose provider is not in the AI client registry, so _wp_connectors_is_ai_api_key_valid() returns null, then posted the masked response back over real HTTP as an admin:

  • trunk: sk-live-...1234 becomes empty
  • this PR as you reviewed it: still empty, so the round trip was only fixed for non-AI connectors
  • with the skip: the stored key survives

Added your condition, and a test in wpConnectorsRestSettingsDispatch.php next to the #65554 ones. Removing the condition fails that test and nothing else.

Also in this round:

  • Rebased onto trunk. #65554 landed after this branch was cut, so the validation block now carries the has_param() guard.
  • @since is 7.2.0 now that trunk has opened.
  • The docblock states the exact match rule, and wpConnectorsSanitizeApplicationPasswordCredentials.php gets the near miss test. The API key side already had test_mask_of_a_different_key_does_not_preserve_stored_api_key.

On the shared masking helper I would rather open a separate ticket than fold it in here. It changes _wp_connectors_mask_api_key() and the application password path, both already shipped, and this one is a data loss fix you have reviewed. Happy to file it and link it back if you agree.

--group connectors 121 pass. --group restapi and --group option are clean apart from failures that are already on trunk. phpcs and phpstan clean on the changed files.

#5 @dilip2615
10 days ago

## Test Report

Trac: https://core.trac.wordpress.org/ticket/65821

### Environment

  • OS: Windows 11
  • Local wordpress-develop Docker env (npm run env:start)
  • PHPUnit via npm run test:php

### What I tested

  1. Applied this PR’s connector changes on top of current trunk.
  2. Ran the full connectors PHPUnit group.
  3. Ran the sanitize API key and REST settings dispatch test classes.

### Results

  • npm run test:php -- --group connectors -> OK (121 tests, 295 assertions)
  • Tests_Connectors_WpConnectorsSanitizeApiKey -> OK (7 tests, 10 assertions)
  • Tests_Connectors_WpConnectorsRestSettingsDispatch -> OK (5 tests, 11 assertions)

The covered behavior matches the ticket: a resubmitted masked API key keeps the stored value, and the AI-provider validation skip for an exact mask resubmit is covered.

No blocking issues found. Looks good from a testing perspective.

@dilip2615 commented on PR #12865:


10 days ago
#6

## Test Report

Trac: https://core.trac.wordpress.org/ticket/65821

### Environment

  • OS: Windows 11
  • Local wordpress-develop Docker env (npm run env:start)
  • PHPUnit via npm run test:php

### What I tested

  1. Applied this PR’s connector changes on top of current trunk.
  2. Ran the full connectors PHPUnit group.
  3. Ran the sanitize API key and REST settings dispatch test classes.

### Results

  • npm run test:php -- --group connectors -> OK (121 tests, 295 assertions)
  • Tests_Connectors_WpConnectorsSanitizeApiKey -> OK (7 tests, 10 assertions)
  • Tests_Connectors_WpConnectorsRestSettingsDispatch -> OK (5 tests, 11 assertions)

The covered behavior matches the ticket: a resubmitted masked API key keeps the stored value, and the AI-provider validation skip for an exact mask resubmit is covered.

No blocking issues found. Looks good from a testing perspective.

#7 @iconpublication
10 days ago

Test Report

Trac: https://core.trac.wordpress.org/ticket/65821(https://core.trac.wordpress.org/ticket/65821)

Environment

  • OS: macOS
  • Browser: Safari
  • WordPress Playground (via PR #12865 preview link)
  • Code Snippets plugin (pre_http_request filter applied to mock remote API validation)

What I tested

  • Added a pre_http_request hook to return a mock 200 OK response for outbound API validation.
  • Saved a test connector API key in the settings panel.
  • Verified the key was stored and displayed as a masked value (••••••••).
  • Clicked "Save Changes" a second time without modifying the masked value.
  • Confirmed the stored value remained intact via /wp/v2/settings.

Results

  • Resubmitting the exact masked key preserved the original stored value.
  • The API key was not wiped out or overwritten by literal mask characters.
  • No validation errors occurred upon resubmission.
  • The covered behavior matches the ticket requirements. No issues found.

#8 @gziolo
10 days ago

  • Keywords commit added
  • Status reviewingaccepted
Note: See TracTickets for help on using tickets.