From 2c32d9b00e025f0c936e0ade23b543d63ed605e9 Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Tue, 7 Apr 2026 16:39:21 -0300 Subject: [PATCH 1/9] feat(integrations): add oauth and hidden field types for integration settings --- .../integrations/class-integration.php | 3 ++ .../views/integrations/settings-field.js | 30 ++++++++++++++++++- 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/includes/reader-activation/integrations/class-integration.php b/includes/reader-activation/integrations/class-integration.php index c667727afa..f6cac1a2a6 100644 --- a/includes/reader-activation/integrations/class-integration.php +++ b/includes/reader-activation/integrations/class-integration.php @@ -910,6 +910,9 @@ private function get_settings_field_by_key( $key ) { protected function sanitize_settings_field_value( $field, $value ) { $type = $field['type'] ?? 'text'; switch ( $type ) { + case 'hidden': + case 'oauth': + return $value; // Read-only or managed programmatically. case 'checkbox': return (bool) $value; case 'number': diff --git a/src/wizards/audience/views/integrations/settings-field.js b/src/wizards/audience/views/integrations/settings-field.js index 07c94d61fb..a6b65e3dfa 100644 --- a/src/wizards/audience/views/integrations/settings-field.js +++ b/src/wizards/audience/views/integrations/settings-field.js @@ -7,7 +7,7 @@ import { CheckboxControl, ExternalLink, TextareaControl } from '@wordpress/compo /** * Internal dependencies. */ -import { Grid, SelectControl, TextControl } from '../../../../../packages/components/src'; +import { Button, Grid, SelectControl, TextControl } from '../../../../../packages/components/src'; /** * Render a single settings field. @@ -32,6 +32,34 @@ export const SettingsField = ( { field, value, onChange } ) => { ); switch ( type ) { + case 'hidden': + return null; + case 'oauth': { + const isConnected = !! value; + const oauthUrl = field.oauth_url || ''; + return ( +
+

+ { label } +

+ { description &&

{ description }

} + { isConnected ? ( + <> +

{ value }

+ { field.disconnect_url && ( + + ) } + + ) : ( + + ) } +
+ ); + } case 'metadata': { const selectedFields = Array.isArray( value ) ? value : []; const normalizedOptions = ( options || [] ).map( option => From 33bb7483e010f83821b4f5ad5d5186d56229359c Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Thu, 14 May 2026 16:59:23 -0300 Subject: [PATCH 2/9] fix(reader-activation): add error handling to rehydrateItem --- src/reader-activation/store.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/reader-activation/store.js b/src/reader-activation/store.js index 6c355d7cb7..11dc14661b 100644 --- a/src/reader-activation/store.js +++ b/src/reader-activation/store.js @@ -39,11 +39,16 @@ const mergeStrategies = new Map(); */ function rehydrateItem( key, serverValue ) { const merge = mergeStrategies.get( key ); - if ( merge ) { - const clientValue = _get( key ); - _set( key, merge( serverValue, clientValue ) ); - } else { - _set( key, serverValue ); + try { + if ( merge ) { + const clientValue = _get( key ); + _set( key, merge( serverValue, clientValue ) ); + } else { + _set( key, serverValue ); + } + } catch ( err ) { + // eslint-disable-next-line no-console + console.warn( `Unable to rehydrated ${ key }`, err ); } } From 99e7f5a5053409e234149c9e4f3e25911c45db97 Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Tue, 19 May 2026 19:13:18 -0300 Subject: [PATCH 3/9] fix(audience-integrations): improve oauth field rendering Address review feedback on the new oauth settings field: - Surface help_url alongside description by reusing the shared help fragment, matching other field types. - Avoid passing an empty href to the Connect button when no oauth_url is configured, so the disabled button no longer navigates anywhere if clicked. --- src/wizards/audience/views/integrations/settings-field.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wizards/audience/views/integrations/settings-field.js b/src/wizards/audience/views/integrations/settings-field.js index a6b65e3dfa..aa9fc6142b 100644 --- a/src/wizards/audience/views/integrations/settings-field.js +++ b/src/wizards/audience/views/integrations/settings-field.js @@ -42,7 +42,7 @@ export const SettingsField = ( { field, value, onChange } ) => {

{ label }

- { description &&

{ description }

} + { ( description || helpUrl ) &&

{ help }

} { isConnected ? ( <>

{ value }

@@ -53,7 +53,7 @@ export const SettingsField = ( { field, value, onChange } ) => { ) } ) : ( - ) } From aa82282c1167d7515eff76214d9bd48d0ebbb5d9 Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Tue, 19 May 2026 20:48:09 -0300 Subject: [PATCH 4/9] fix(reader-activation): fall back to server value when rehydrate merge throws Address review feedback on the rehydrateItem error handler: - Fix the warning message typo ("rehydrated" -> "rehydrate"). - Overwrite the key with the server value when a merge strategy throws, matching the no-merge branch so a single bad merge strategy can't leave the store with the key untouched. - Add a test that registers a throwing merge strategy and asserts the store falls back to the server value without throwing. --- src/reader-activation/store.js | 5 ++++- src/reader-activation/store.test.js | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/reader-activation/store.js b/src/reader-activation/store.js index 11dc14661b..29a47b9cad 100644 --- a/src/reader-activation/store.js +++ b/src/reader-activation/store.js @@ -48,7 +48,10 @@ function rehydrateItem( key, serverValue ) { } } catch ( err ) { // eslint-disable-next-line no-console - console.warn( `Unable to rehydrated ${ key }`, err ); + console.warn( `Unable to rehydrate ${ key }`, err ); + // Fall back to overwriting with the server value so a failing merge + // strategy can't leave the store in an inconsistent/missing state. + _set( key, serverValue ); } } diff --git a/src/reader-activation/store.test.js b/src/reader-activation/store.test.js index ebc016b8bd..51186af678 100644 --- a/src/reader-activation/store.test.js +++ b/src/reader-activation/store.test.js @@ -99,6 +99,24 @@ describe( 'Store', () => { store.rehydrate(); expect( store.get( 'foo' ) ).toEqual( 'bar' ); } ); + it( 'should fall back to the server value when a merge strategy throws', () => { + window.newspack_reader_data = { + items: { + flaky: '"server"', + }, + }; + const store = Store(); + store.register( 'flaky', { + merge: () => { + throw new Error( 'boom' ); + }, + } ); + const warn = jest.spyOn( console, 'warn' ).mockImplementation( () => {} ); + expect( () => store.rehydrate() ).not.toThrow(); + expect( store.get( 'flaky' ) ).toEqual( 'server' ); + expect( warn ).toHaveBeenCalledWith( expect.stringContaining( 'Unable to rehydrate flaky' ), expect.any( Error ) ); + warn.mockRestore(); + } ); describe( 'getAll', () => { beforeEach( () => { window.newspack_reader_data = {}; From dcf1ac9b03c92e07848341bcec41d71dbe09aad3 Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Tue, 19 May 2026 20:48:43 -0300 Subject: [PATCH 5/9] fix(integrations): sanitize oauth/hidden settings field values Address review feedback on sanitize_settings_field_value(): - Reject non-scalar payloads for the new oauth and hidden field types (return field default or empty string) so REST settings can't store arrays/objects where a scalar string is expected. - Sanitize scalar strings through sanitize_text_field, matching how text and password fields are handled. - Add PHPUnit coverage for the new types via a public test wrapper on Sample_Integration, covering scalar sanitization and non-scalar rejection with and without an explicit default. --- .../integrations/class-integration.php | 8 +- .../integrations/class-sample-integration.php | 11 +++ .../integrations/class-test-integrations.php | 90 +++++++++++++++++++ 3 files changed, 108 insertions(+), 1 deletion(-) diff --git a/includes/reader-activation/integrations/class-integration.php b/includes/reader-activation/integrations/class-integration.php index f6cac1a2a6..0b68cb5a85 100644 --- a/includes/reader-activation/integrations/class-integration.php +++ b/includes/reader-activation/integrations/class-integration.php @@ -912,7 +912,13 @@ protected function sanitize_settings_field_value( $field, $value ) { switch ( $type ) { case 'hidden': case 'oauth': - return $value; // Read-only or managed programmatically. + // Read-only or managed programmatically, but values still arrive via + // the REST settings endpoint — coerce to a sanitized scalar string and + // reject non-scalar payloads to keep options storage predictable. + if ( ! is_scalar( $value ) ) { + return $field['default'] ?? ''; + } + return \sanitize_text_field( (string) $value ); case 'checkbox': return (bool) $value; case 'number': diff --git a/tests/unit-tests/integrations/class-sample-integration.php b/tests/unit-tests/integrations/class-sample-integration.php index dcb9040d15..62e5aa301a 100644 --- a/tests/unit-tests/integrations/class-sample-integration.php +++ b/tests/unit-tests/integrations/class-sample-integration.php @@ -83,6 +83,17 @@ public function test_register_handler( $action_name, $method ) { $this->register_handler( $action_name, $method ); } + /** + * Sanitize a settings field value (public wrapper for testing). + * + * @param array $field The field declaration. + * @param mixed $value The value to sanitize. + * @return mixed + */ + public function test_sanitize_settings_field_value( $field, $value ) { + return $this->sanitize_settings_field_value( $field, $value ); + } + /** * Sample handler method for data events. * diff --git a/tests/unit-tests/integrations/class-test-integrations.php b/tests/unit-tests/integrations/class-test-integrations.php index 023c96e329..71b0ff28cc 100644 --- a/tests/unit-tests/integrations/class-test-integrations.php +++ b/tests/unit-tests/integrations/class-test-integrations.php @@ -1199,4 +1199,94 @@ public function test_my_account_endpoints_option_tracks_changes() { Integrations::register_my_account_endpoints(); $this->assertSame( [ 'two-page' ], get_option( Integrations::MY_ACCOUNT_ENDPOINTS_OPTION ) ); } + + /** + * OAuth settings field value: scalar strings are sanitized through sanitize_text_field. + */ + public function test_sanitize_settings_field_value_oauth_scalar() { + $integration = new Sample_Integration( 'test-id', 'Test Integration' ); + $field = [ + 'key' => 'token', + 'type' => 'oauth', + ]; + + $this->assertSame( + 'abc123', + $integration->test_sanitize_settings_field_value( $field, 'abc123' ) + ); + // Tags stripped by sanitize_text_field. + $this->assertSame( + 'token', + $integration->test_sanitize_settings_field_value( $field, 'token' ) + ); + // Non-string scalars are coerced to a string then sanitized. + $this->assertSame( + '42', + $integration->test_sanitize_settings_field_value( $field, 42 ) + ); + } + + /** + * OAuth settings field value: non-scalar payloads are rejected and the default is returned. + */ + public function test_sanitize_settings_field_value_oauth_non_scalar_returns_default() { + $integration = new Sample_Integration( 'test-id', 'Test Integration' ); + + // No default declared: falls back to empty string. + $this->assertSame( + '', + $integration->test_sanitize_settings_field_value( + [ + 'key' => 'token', + 'type' => 'oauth', + ], + [ 'unexpected' => 'array' ] + ) + ); + // Explicit default is honored. + $this->assertSame( + 'fallback', + $integration->test_sanitize_settings_field_value( + [ + 'key' => 'token', + 'type' => 'oauth', + 'default' => 'fallback', + ], + (object) [ 'unexpected' => 'object' ] + ) + ); + } + + /** + * Hidden settings field value: scalar strings are sanitized, non-scalars return the default. + */ + public function test_sanitize_settings_field_value_hidden() { + $integration = new Sample_Integration( 'test-id', 'Test Integration' ); + $field = [ + 'key' => 'secret', + 'type' => 'hidden', + ]; + + $this->assertSame( + 'opaque-id', + $integration->test_sanitize_settings_field_value( $field, 'opaque-id' ) + ); + // Non-scalar rejected. + $this->assertSame( + '', + $integration->test_sanitize_settings_field_value( $field, [ 'nope' ] ) + ); + // Explicit default honored on non-scalar payload. + $this->assertSame( + 'kept', + $integration->test_sanitize_settings_field_value( + [ + 'key' => 'secret', + 'type' => 'hidden', + 'default' => 'kept', + ], + [ 'nope' ] + ) + ); + } } From 45021bef60df9e6976b869039e6a203c2c53e73d Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Wed, 20 May 2026 15:00:27 -0300 Subject: [PATCH 6/9] fix(integrations): make oauth/hidden field types read-only on write path --- .../integrations/class-integration.php | 14 +-- .../integrations/class-test-integrations.php | 94 +++++++++++-------- 2 files changed, 61 insertions(+), 47 deletions(-) diff --git a/includes/reader-activation/integrations/class-integration.php b/includes/reader-activation/integrations/class-integration.php index 0b68cb5a85..479144a25d 100644 --- a/includes/reader-activation/integrations/class-integration.php +++ b/includes/reader-activation/integrations/class-integration.php @@ -912,13 +912,13 @@ protected function sanitize_settings_field_value( $field, $value ) { switch ( $type ) { case 'hidden': case 'oauth': - // Read-only or managed programmatically, but values still arrive via - // the REST settings endpoint — coerce to a sanitized scalar string and - // reject non-scalar payloads to keep options storage predictable. - if ( ! is_scalar( $value ) ) { - return $field['default'] ?? ''; - } - return \sanitize_text_field( (string) $value ); + // Read-only on the write path: these types are managed programmatically + // (e.g., server-side OAuth callbacks writing directly via update_option), + // so ignore inbound values from the settings REST endpoint and return + // the currently stored value, falling back to the declared default. + $option_name = self::SETTINGS_OPTION_PREFIX . $this->id . '_' . $field['key']; + $stored = \get_option( $option_name, null ); + return null !== $stored ? $stored : ( $field['default'] ?? '' ); case 'checkbox': return (bool) $value; case 'number': diff --git a/tests/unit-tests/integrations/class-test-integrations.php b/tests/unit-tests/integrations/class-test-integrations.php index 71b0ff28cc..7f0bc0145d 100644 --- a/tests/unit-tests/integrations/class-test-integrations.php +++ b/tests/unit-tests/integrations/class-test-integrations.php @@ -1201,82 +1201,85 @@ public function test_my_account_endpoints_option_tracks_changes() { } /** - * OAuth settings field value: scalar strings are sanitized through sanitize_text_field. + * OAuth settings field value is read-only on the write path: with no stored + * value, inbound scalars are ignored and the declared default is returned. */ - public function test_sanitize_settings_field_value_oauth_scalar() { + public function test_sanitize_settings_field_value_oauth_read_only_no_stored_value() { $integration = new Sample_Integration( 'test-id', 'Test Integration' ); $field = [ 'key' => 'token', 'type' => 'oauth', ]; + // No default declared: falls back to empty string regardless of inbound. $this->assertSame( - 'abc123', + '', $integration->test_sanitize_settings_field_value( $field, 'abc123' ) ); - // Tags stripped by sanitize_text_field. + // Explicit default is honored when no stored value exists. $this->assertSame( - 'token', - $integration->test_sanitize_settings_field_value( $field, 'token' ) + 'fallback', + $integration->test_sanitize_settings_field_value( + [ + 'key' => 'token', + 'type' => 'oauth', + 'default' => 'fallback', + ], + 'inbound-token' + ) ); - // Non-string scalars are coerced to a string then sanitized. + // Non-scalar inbound payloads are also ignored. $this->assertSame( - '42', - $integration->test_sanitize_settings_field_value( $field, 42 ) + '', + $integration->test_sanitize_settings_field_value( + $field, + [ 'unexpected' => 'array' ] + ) ); } /** - * OAuth settings field value: non-scalar payloads are rejected and the default is returned. + * OAuth settings field value is read-only on the write path: when a value is + * already stored (e.g., written by a server-side OAuth callback), the + * sanitizer returns the stored value and ignores any inbound payload. */ - public function test_sanitize_settings_field_value_oauth_non_scalar_returns_default() { + public function test_sanitize_settings_field_value_oauth_read_only_with_stored_value() { $integration = new Sample_Integration( 'test-id', 'Test Integration' ); + $field = [ + 'key' => 'token', + 'type' => 'oauth', + ]; + \update_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_token', 'server-managed-token' ); - // No default declared: falls back to empty string. + // Scalar inbound is ignored. $this->assertSame( - '', - $integration->test_sanitize_settings_field_value( - [ - 'key' => 'token', - 'type' => 'oauth', - ], - [ 'unexpected' => 'array' ] - ) + 'server-managed-token', + $integration->test_sanitize_settings_field_value( $field, 'attempted-override' ) ); - // Explicit default is honored. + // Non-scalar inbound is ignored. $this->assertSame( - 'fallback', - $integration->test_sanitize_settings_field_value( - [ - 'key' => 'token', - 'type' => 'oauth', - 'default' => 'fallback', - ], - (object) [ 'unexpected' => 'object' ] - ) + 'server-managed-token', + $integration->test_sanitize_settings_field_value( $field, [ 'unexpected' => 'array' ] ) ); } /** - * Hidden settings field value: scalar strings are sanitized, non-scalars return the default. + * Hidden settings field value is read-only on the write path: same behavior + * as oauth — inbound payloads are ignored, stored values are preserved. */ - public function test_sanitize_settings_field_value_hidden() { + public function test_sanitize_settings_field_value_hidden_read_only() { $integration = new Sample_Integration( 'test-id', 'Test Integration' ); $field = [ 'key' => 'secret', 'type' => 'hidden', ]; - $this->assertSame( - 'opaque-id', - $integration->test_sanitize_settings_field_value( $field, 'opaque-id' ) - ); - // Non-scalar rejected. + // No stored value, no default: empty string. $this->assertSame( '', - $integration->test_sanitize_settings_field_value( $field, [ 'nope' ] ) + $integration->test_sanitize_settings_field_value( $field, 'opaque-id' ) ); - // Explicit default honored on non-scalar payload. + // No stored value, explicit default: default returned. $this->assertSame( 'kept', $integration->test_sanitize_settings_field_value( @@ -1285,8 +1288,19 @@ public function test_sanitize_settings_field_value_hidden() { 'type' => 'hidden', 'default' => 'kept', ], - [ 'nope' ] + 'attempted-override' ) ); + + // With a stored value, inbound writes (scalar or non-scalar) are ignored. + \update_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_secret', 'server-managed-secret' ); + $this->assertSame( + 'server-managed-secret', + $integration->test_sanitize_settings_field_value( $field, 'attempted-override' ) + ); + $this->assertSame( + 'server-managed-secret', + $integration->test_sanitize_settings_field_value( $field, [ 'nope' ] ) + ); } } From f4d237cc003600f16fa7d3199fac204168fb928f Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Wed, 20 May 2026 15:12:54 -0300 Subject: [PATCH 7/9] Revert "fix(integrations): make oauth/hidden field types read-only on write path" This reverts commit 45021bef60df9e6976b869039e6a203c2c53e73d. --- .../integrations/class-integration.php | 14 +-- .../integrations/class-test-integrations.php | 94 ++++++++----------- 2 files changed, 47 insertions(+), 61 deletions(-) diff --git a/includes/reader-activation/integrations/class-integration.php b/includes/reader-activation/integrations/class-integration.php index 479144a25d..0b68cb5a85 100644 --- a/includes/reader-activation/integrations/class-integration.php +++ b/includes/reader-activation/integrations/class-integration.php @@ -912,13 +912,13 @@ protected function sanitize_settings_field_value( $field, $value ) { switch ( $type ) { case 'hidden': case 'oauth': - // Read-only on the write path: these types are managed programmatically - // (e.g., server-side OAuth callbacks writing directly via update_option), - // so ignore inbound values from the settings REST endpoint and return - // the currently stored value, falling back to the declared default. - $option_name = self::SETTINGS_OPTION_PREFIX . $this->id . '_' . $field['key']; - $stored = \get_option( $option_name, null ); - return null !== $stored ? $stored : ( $field['default'] ?? '' ); + // Read-only or managed programmatically, but values still arrive via + // the REST settings endpoint — coerce to a sanitized scalar string and + // reject non-scalar payloads to keep options storage predictable. + if ( ! is_scalar( $value ) ) { + return $field['default'] ?? ''; + } + return \sanitize_text_field( (string) $value ); case 'checkbox': return (bool) $value; case 'number': diff --git a/tests/unit-tests/integrations/class-test-integrations.php b/tests/unit-tests/integrations/class-test-integrations.php index 7f0bc0145d..71b0ff28cc 100644 --- a/tests/unit-tests/integrations/class-test-integrations.php +++ b/tests/unit-tests/integrations/class-test-integrations.php @@ -1201,85 +1201,82 @@ public function test_my_account_endpoints_option_tracks_changes() { } /** - * OAuth settings field value is read-only on the write path: with no stored - * value, inbound scalars are ignored and the declared default is returned. + * OAuth settings field value: scalar strings are sanitized through sanitize_text_field. */ - public function test_sanitize_settings_field_value_oauth_read_only_no_stored_value() { + public function test_sanitize_settings_field_value_oauth_scalar() { $integration = new Sample_Integration( 'test-id', 'Test Integration' ); $field = [ 'key' => 'token', 'type' => 'oauth', ]; - // No default declared: falls back to empty string regardless of inbound. $this->assertSame( - '', + 'abc123', $integration->test_sanitize_settings_field_value( $field, 'abc123' ) ); - // Explicit default is honored when no stored value exists. + // Tags stripped by sanitize_text_field. $this->assertSame( - 'fallback', - $integration->test_sanitize_settings_field_value( - [ - 'key' => 'token', - 'type' => 'oauth', - 'default' => 'fallback', - ], - 'inbound-token' - ) + 'token', + $integration->test_sanitize_settings_field_value( $field, 'token' ) ); - // Non-scalar inbound payloads are also ignored. + // Non-string scalars are coerced to a string then sanitized. $this->assertSame( - '', - $integration->test_sanitize_settings_field_value( - $field, - [ 'unexpected' => 'array' ] - ) + '42', + $integration->test_sanitize_settings_field_value( $field, 42 ) ); } /** - * OAuth settings field value is read-only on the write path: when a value is - * already stored (e.g., written by a server-side OAuth callback), the - * sanitizer returns the stored value and ignores any inbound payload. + * OAuth settings field value: non-scalar payloads are rejected and the default is returned. */ - public function test_sanitize_settings_field_value_oauth_read_only_with_stored_value() { + public function test_sanitize_settings_field_value_oauth_non_scalar_returns_default() { $integration = new Sample_Integration( 'test-id', 'Test Integration' ); - $field = [ - 'key' => 'token', - 'type' => 'oauth', - ]; - \update_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_token', 'server-managed-token' ); - // Scalar inbound is ignored. + // No default declared: falls back to empty string. $this->assertSame( - 'server-managed-token', - $integration->test_sanitize_settings_field_value( $field, 'attempted-override' ) + '', + $integration->test_sanitize_settings_field_value( + [ + 'key' => 'token', + 'type' => 'oauth', + ], + [ 'unexpected' => 'array' ] + ) ); - // Non-scalar inbound is ignored. + // Explicit default is honored. $this->assertSame( - 'server-managed-token', - $integration->test_sanitize_settings_field_value( $field, [ 'unexpected' => 'array' ] ) + 'fallback', + $integration->test_sanitize_settings_field_value( + [ + 'key' => 'token', + 'type' => 'oauth', + 'default' => 'fallback', + ], + (object) [ 'unexpected' => 'object' ] + ) ); } /** - * Hidden settings field value is read-only on the write path: same behavior - * as oauth — inbound payloads are ignored, stored values are preserved. + * Hidden settings field value: scalar strings are sanitized, non-scalars return the default. */ - public function test_sanitize_settings_field_value_hidden_read_only() { + public function test_sanitize_settings_field_value_hidden() { $integration = new Sample_Integration( 'test-id', 'Test Integration' ); $field = [ 'key' => 'secret', 'type' => 'hidden', ]; - // No stored value, no default: empty string. $this->assertSame( - '', + 'opaque-id', $integration->test_sanitize_settings_field_value( $field, 'opaque-id' ) ); - // No stored value, explicit default: default returned. + // Non-scalar rejected. + $this->assertSame( + '', + $integration->test_sanitize_settings_field_value( $field, [ 'nope' ] ) + ); + // Explicit default honored on non-scalar payload. $this->assertSame( 'kept', $integration->test_sanitize_settings_field_value( @@ -1288,19 +1285,8 @@ public function test_sanitize_settings_field_value_hidden_read_only() { 'type' => 'hidden', 'default' => 'kept', ], - 'attempted-override' + [ 'nope' ] ) ); - - // With a stored value, inbound writes (scalar or non-scalar) are ignored. - \update_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_secret', 'server-managed-secret' ); - $this->assertSame( - 'server-managed-secret', - $integration->test_sanitize_settings_field_value( $field, 'attempted-override' ) - ); - $this->assertSame( - 'server-managed-secret', - $integration->test_sanitize_settings_field_value( $field, [ 'nope' ] ) - ); } } From f12fe78d021543d100561e91f9ddf413af474f9f Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Wed, 20 May 2026 15:19:47 -0300 Subject: [PATCH 8/9] fix(integrations): block REST writes to oauth/hidden settings fields --- .../reader-activation/class-integrations.php | 11 ++- .../integrations/class-integration.php | 33 ++++++- .../integrations/class-sample-integration.php | 14 ++- .../integrations/class-test-integrations.php | 92 +++++++++++++++++++ 4 files changed, 143 insertions(+), 7 deletions(-) diff --git a/includes/reader-activation/class-integrations.php b/includes/reader-activation/class-integrations.php index 17ffe4a9d4..8846f95566 100644 --- a/includes/reader-activation/class-integrations.php +++ b/includes/reader-activation/class-integrations.php @@ -437,7 +437,13 @@ public static function get_all_integration_settings() { } /** - * Update settings for a specific integration. + * Update settings for a specific integration from an admin REST request. + * + * Skips fields whose type is managed server-side (see + * Integration::MANAGED_FIELD_TYPES — e.g., 'oauth', 'hidden') so admin + * clients can't overwrite tokens or other programmatically-managed values + * by POSTing them in the settings payload. Server-side writers continue + * to use Integration::update_settings_field_value() directly. * * @param string $integration_id The integration ID. * @param array $settings Key-value pairs of settings to update. @@ -449,6 +455,9 @@ public static function update_integration_settings( $integration_id, $settings ) return null; } foreach ( $settings as $key => $value ) { + if ( $integration->is_managed_settings_field( $key ) ) { + continue; + } $integration->update_settings_field_value( $key, $value ); } return true; diff --git a/includes/reader-activation/integrations/class-integration.php b/includes/reader-activation/integrations/class-integration.php index 0b68cb5a85..9d62fef02e 100644 --- a/includes/reader-activation/integrations/class-integration.php +++ b/includes/reader-activation/integrations/class-integration.php @@ -773,6 +773,29 @@ public function get_settings_fields() { ); } + /** + * Settings field types that are managed by server-side code (e.g., OAuth + * callbacks) and must not be writable from the admin settings REST endpoint. + * + * @var string[] + */ + const MANAGED_FIELD_TYPES = [ 'oauth', 'hidden' ]; + + /** + * Whether a settings field is managed by server-side code and therefore + * read-only on the REST settings save path. + * + * @param string $key The field key. + * @return bool True if the field is a managed type, false otherwise. + */ + public function is_managed_settings_field( $key ) { + $field = $this->get_settings_field_by_key( $key ); + if ( ! $field ) { + return false; + } + return in_array( $field['type'] ?? 'text', self::MANAGED_FIELD_TYPES, true ); + } + /** * Get the value of a settings field. * @@ -912,9 +935,13 @@ protected function sanitize_settings_field_value( $field, $value ) { switch ( $type ) { case 'hidden': case 'oauth': - // Read-only or managed programmatically, but values still arrive via - // the REST settings endpoint — coerce to a sanitized scalar string and - // reject non-scalar payloads to keep options storage predictable. + // Server-managed types. Admin POSTs to the settings REST endpoint + // are filtered out upstream in Integrations::update_integration_settings(), + // so values land here only via trusted PHP writers (e.g., an OAuth + // callback). Assumes a single-line, tag-free scalar — sanitize_text_field + // will strip tags and collapse whitespace, which is fine for opaque + // tokens but would silently corrupt a multiline secret. Non-scalar + // payloads fall back to the declared default. if ( ! is_scalar( $value ) ) { return $field['default'] ?? ''; } diff --git a/tests/unit-tests/integrations/class-sample-integration.php b/tests/unit-tests/integrations/class-sample-integration.php index 62e5aa301a..b9b7a4e4d9 100644 --- a/tests/unit-tests/integrations/class-sample-integration.php +++ b/tests/unit-tests/integrations/class-sample-integration.php @@ -32,12 +32,19 @@ class Sample_Integration extends Integration { */ public $my_account_render_calls = []; + /** + * Settings fields to return from register_settings_fields(). Tests that + * need declared fields set this before constructing the integration. + * + * @var array + */ + public static $declared_settings_fields = []; + /** * Register settings fields (test implementation). */ public function register_settings_fields() { - // No settings fields for this test implementation. - return []; + return self::$declared_settings_fields; } /** @@ -113,7 +120,8 @@ public function handle_test_event( $timestamp, $data, $client_id ) { * Reset captured state between tests. */ public static function reset() { - self::$handler_args = null; + self::$handler_args = null; + self::$declared_settings_fields = []; } /** diff --git a/tests/unit-tests/integrations/class-test-integrations.php b/tests/unit-tests/integrations/class-test-integrations.php index 71b0ff28cc..41619c63e4 100644 --- a/tests/unit-tests/integrations/class-test-integrations.php +++ b/tests/unit-tests/integrations/class-test-integrations.php @@ -1289,4 +1289,96 @@ public function test_sanitize_settings_field_value_hidden() { ) ); } + + /** + * The REST settings save entry point (update_integration_settings) drops + * oauth/hidden keys so admin clients can't overwrite server-managed values + * such as OAuth tokens. Other field types pass through. + */ + public function test_update_integration_settings_skips_managed_field_types() { + Sample_Integration::$declared_settings_fields = [ + [ + 'key' => 'api_label', + 'type' => 'text', + 'default' => '', + ], + [ + 'key' => 'access_token', + 'type' => 'hidden', + 'default' => '', + ], + [ + 'key' => 'connection', + 'type' => 'oauth', + 'default' => '', + ], + ]; + $integration = new Sample_Integration( 'test-id', 'Test Integration' ); + Integrations::register( $integration ); + + // Pre-seed the managed fields as if a server-side OAuth callback had + // written them. The REST endpoint must not overwrite these. + \update_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_access_token', 'server-managed-token' ); + \update_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_connection', 'connected' ); + + $result = Integrations::update_integration_settings( + 'test-id', + [ + 'api_label' => 'My Label', + 'access_token' => 'attempted-override', + 'connection' => 'attempted-override', + ] + ); + + $this->assertTrue( $result ); + + // The non-managed field was written through. + $this->assertSame( + 'My Label', + \get_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_api_label' ) + ); + + // The managed fields kept their server-managed values. + $this->assertSame( + 'server-managed-token', + \get_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_access_token' ) + ); + $this->assertSame( + 'connected', + \get_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_connection' ) + ); + } + + /** + * Server-side writers (e.g., an OAuth callback) calling + * update_settings_field_value() directly bypass the REST-path filter and + * can still write oauth/hidden values. + */ + public function test_update_settings_field_value_writes_managed_field_types_directly() { + Sample_Integration::$declared_settings_fields = [ + [ + 'key' => 'access_token', + 'type' => 'hidden', + 'default' => '', + ], + [ + 'key' => 'connection', + 'type' => 'oauth', + 'default' => '', + ], + ]; + $integration = new Sample_Integration( 'test-id', 'Test Integration' ); + + $integration->update_settings_field_value( 'access_token', 'fresh-token' ); + $integration->update_settings_field_value( 'connection', 'connected' ); + + $this->assertSame( + 'fresh-token', + \get_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_access_token' ) + ); + $this->assertSame( + 'connected', + \get_option( Integration::SETTINGS_OPTION_PREFIX . 'test-id_connection' ) + ); + } } From 32ac489631517a999e5ce34bbc0d894c4eb9f058 Mon Sep 17 00:00:00 2001 From: Miguel Peixe Date: Fri, 22 May 2026 12:38:03 -0300 Subject: [PATCH 9/9] perf(integrations): disable autoload for integration settings --- .../integrations/class-integration.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/includes/reader-activation/integrations/class-integration.php b/includes/reader-activation/integrations/class-integration.php index 9d62fef02e..9941c74c45 100644 --- a/includes/reader-activation/integrations/class-integration.php +++ b/includes/reader-activation/integrations/class-integration.php @@ -603,7 +603,7 @@ public function update_enabled_incoming_fields( $keys ) { $fields_to_store[ $key ] = $raw_data; } - return \update_option( self::INCOMING_FIELDS_OPTION_PREFIX . $this->id, $fields_to_store ); + return \update_option( self::INCOMING_FIELDS_OPTION_PREFIX . $this->id, $fields_to_store, false ); } /** @@ -615,7 +615,7 @@ public function update_enabled_incoming_fields( $keys ) { public function update_enabled_outgoing_fields( $fields ) { // Only allow fields that are in the metadata keys map. $fields = array_intersect( Sync\Metadata::get_default_fields(), $fields ); - return \update_option( self::OUTGOING_FIELDS_OPTION_PREFIX . $this->id, array_values( $fields ) ); + return \update_option( self::OUTGOING_FIELDS_OPTION_PREFIX . $this->id, array_values( $fields ), false ); } /** @@ -698,7 +698,7 @@ public function get_metadata_prefix() { $legacy_value = \get_option( Sync\Metadata::PREFIX_OPTION, null ); if ( null !== $legacy_value && ! empty( $legacy_value ) ) { // update option directly to avoid infinite loop. - \update_option( self::METADATA_PREFIX_OPTION_PREFIX . $this->id, $legacy_value ); + \update_option( self::METADATA_PREFIX_OPTION_PREFIX . $this->id, $legacy_value, false ); return $legacy_value; } return 'NP_'; @@ -758,7 +758,7 @@ public function update_metadata_prefix( $prefix ) { if ( empty( $prefix ) ) { $prefix = 'NP_'; } - return \update_option( self::METADATA_PREFIX_OPTION_PREFIX . $this->id, \sanitize_text_field( $prefix ) ); + return \update_option( self::METADATA_PREFIX_OPTION_PREFIX . $this->id, \sanitize_text_field( $prefix ), false ); } /** @@ -835,7 +835,7 @@ function( $field ) { $legacy_value = \get_option( self::$legacy_option_map[ $key ], null ); if ( null !== $legacy_value ) { // update option directly to avoid infinite loop. - \update_option( $option_name, $legacy_value ); + \update_option( $option_name, $legacy_value, false ); return $legacy_value; } } @@ -868,7 +868,7 @@ public function update_settings_field_value( $key, $value ) { } $option_name = self::SETTINGS_OPTION_PREFIX . $this->id . '_' . $key; - return \update_option( $option_name, $sanitized ); + return \update_option( $option_name, $sanitized, false ); } /**