diff --git a/.github/workflows/docker-images.yml b/.github/workflows/docker-images.yml index 25e8241e5..41f564d23 100644 --- a/.github/workflows/docker-images.yml +++ b/.github/workflows/docker-images.yml @@ -36,5 +36,5 @@ jobs: run: docker compose --file docker-compose.build.yml build - name: Publish images - if: contains( github.ref_name, 'master' ) + if: github.ref == 'refs/heads/master' run: docker buildx bake --file docker-compose.build.yml --push --set '*.platform=linux/amd64,linux/arm64' diff --git a/abilities/class-ability-create-exclusion-rule.php b/abilities/class-ability-create-exclusion-rule.php index a8d429e09..2a5350e63 100644 --- a/abilities/class-ability-create-exclusion-rule.php +++ b/abilities/class-ability-create-exclusion-rule.php @@ -32,6 +32,19 @@ public function get_name() { return 'stream/create-exclusion-rule'; } + /** + * Exclusion rules suppress future audit records. On a network-activated + * install the rule is written to the network option and therefore applies + * across every site, so require a network capability in that case. + * + * @param array $input Input that will be passed to execute(). + * @return bool + */ + public function permission_callback( $input = array() ) { + unset( $input ); + return $this->can_write_settings(); + } + /** * {@inheritDoc} */ diff --git a/abilities/class-ability-get-alerts.php b/abilities/class-ability-get-alerts.php index 24f2d694e..9045ce075 100644 --- a/abilities/class-ability-get-alerts.php +++ b/abilities/class-ability-get-alerts.php @@ -124,13 +124,13 @@ public function execute( $input = null ) { continue; } - // Alert::$alert_meta defaults to array(); an empty PHP array() also - // JSON-encodes as a list ([]), which violates the declared object - // output schema. Normalize empty/non-array values to a real object - // so wp_json_encode() emits {} when there is no meta. - $alert_meta = is_array( $alert->alert_meta ) && ! empty( $alert->alert_meta ) - ? $alert->alert_meta - : new \stdClass(); + // This ability only requires view_stream, which is a lower bar than + // the settings capability that gates alert configuration in the + // admin UI. Strip destination credentials (Slack webhook, IFTTT + // maker key) before they cross that boundary. Also normalizes + // empty/non-array meta to stdClass so wp_json_encode() emits {} and + // satisfies the declared object output schema. + $alert_meta = $this->redact_alert_meta( $alert->alert_meta ); $out[] = array( 'id' => (int) $alert->ID, diff --git a/abilities/class-ability-update-settings.php b/abilities/class-ability-update-settings.php index 5ef486db5..96d0d4dd0 100644 --- a/abilities/class-ability-update-settings.php +++ b/abilities/class-ability-update-settings.php @@ -19,6 +19,18 @@ public function get_name() { return 'stream/update-settings'; } + /** + * Writes land on the network option when Stream is network activated, so + * require a network capability in that case. + * + * @param array $input Input that will be passed to execute(). + * @return bool + */ + public function permission_callback( $input = array() ) { + unset( $input ); + return $this->can_write_settings(); + } + /** * {@inheritDoc} */ diff --git a/classes/class-ability.php b/classes/class-ability.php index 9029be4c8..cd13d1c36 100644 --- a/classes/class-ability.php +++ b/classes/class-ability.php @@ -96,6 +96,105 @@ public function permission_callback( $input = array() ) { return current_user_can( WP_STREAM_SETTINGS_CAPABILITY ); } + /** + * Permission check for abilities that persist Stream settings. + * + * On network-activated multisite, Settings::update_all_setting_values() + * writes the authoritative network option (wp_stream_network), so the write + * affects every site on the network. WP_STREAM_SETTINGS_CAPABILITY defaults + * to 'manage_options', which every single-site administrator holds -- on its + * own it is not sufficient authority for a network-wide change. Require a + * network capability whenever the write is going to be network-scoped. + * + * The network capability is required in addition to the settings + * capability, not instead of it: on multisite a super admin passes every + * capability check by definition, so returning the network check alone + * would silently ignore a deployment that narrowed + * WP_STREAM_SETTINGS_CAPABILITY. + * + * @return bool + */ + protected function can_write_settings() { + $can_write_settings = current_user_can( WP_STREAM_SETTINGS_CAPABILITY ); + + // Mirrors the branch in Settings::update_all_setting_values(): when the + // write lands on the network option it additionally requires network + // authority. This is an extra requirement, never a substitute -- the + // settings capability is a site-overridable constant, so a deployment + // that restricts or revokes it must keep being honoured for super + // admins too. + if ( is_multisite() && $this->plugin->is_network_activated() ) { + return $can_write_settings && current_user_can( 'manage_network_options' ); + } + + return $can_write_settings; + } + + /** + * Alert meta keys holding reusable credentials rather than configuration. + * + * Alert destinations are configured by users with the Stream settings + * capability, but alerts are readable through get-alerts by anyone with + * `view_stream`. These values are bearer credentials -- a Slack incoming + * webhook URL is sufficient on its own to post into the target channel, and + * an IFTTT Maker key is sufficient to fire that account's applets -- so they + * must not cross that privilege boundary. + * + * @const array + */ + const SECRET_ALERT_META_KEYS = array( + 'webhook', + 'maker_key', + ); + + /** + * Replace credential values in an alert_meta array with a boolean marker. + * + * Callers still need to know whether a destination is configured, so each + * secret key is replaced by `{key}_configured` rather than dropped. The + * value itself never leaves the site. + * + * Returns stdClass for empty input so wp_json_encode() emits `{}` and + * satisfies the `alert_meta: object` output schema (an empty PHP array + * encodes as `[]`). + * + * @param mixed $alert_meta Raw alert meta, usually an array. + * @return array|\stdClass + */ + protected function redact_alert_meta( $alert_meta ) { + if ( ! is_array( $alert_meta ) || empty( $alert_meta ) ) { + return new \stdClass(); + } + + /** + * Filters the alert_meta keys treated as credentials and withheld from + * ability output. + * + * Third-party alert types registered via `wp_stream_alert_types` may + * store their own destination secrets under names Stream cannot know + * about; add them here so they are redacted too. + * + * @param array $keys Meta keys to redact. + * @param array $alert_meta The alert meta being redacted. + */ + $secret_keys = (array) apply_filters( + 'wp_stream_secret_alert_meta_keys', + self::SECRET_ALERT_META_KEYS, + $alert_meta + ); + + foreach ( $secret_keys as $key ) { + if ( ! array_key_exists( $key, $alert_meta ) ) { + continue; + } + + $alert_meta[ $key . '_configured' ] = ! empty( $alert_meta[ $key ] ); + unset( $alert_meta[ $key ] ); + } + + return empty( $alert_meta ) ? new \stdClass() : $alert_meta; + } + /** * Annotation flags for the ability (readonly, destructive, idempotent). * diff --git a/classes/class-connector.php b/classes/class-connector.php index 171f32f62..e7966e4ee 100644 --- a/classes/class-connector.php +++ b/classes/class-connector.php @@ -186,6 +186,141 @@ public function log( $message, $args, $object_id, $context, $action, $user_id = return call_user_func_array( array( wp_stream_get_instance()->log, 'log' ), compact( 'connector', 'message', 'args', 'object_id', 'context', 'action', 'user_id' ) ); } + /** + * Substrings that mark a setting name as holding a credential. + * + * Deliberately matched as substrings so unknown third-party settings are + * covered by default: connectors log arbitrary option arrays from other + * plugins, and an allowlist cannot anticipate every field a payment gateway + * or integration might add. Over-redacting a harmless field only costs a + * little detail in the audit log; under-redacting persists a live credential + * in a table that lower-privileged Stream viewers can read. + * + * @const array + */ + const SECRET_KEY_PATTERNS = array( + 'pass', + 'secret', + 'private_key', + 'apikey', + 'token', + 'webhook', + 'license', + 'salt', + 'credential', + 'oauth', + ); + + /** + * Setting names, or suffixes of them, that hold a credential but do not + * contain any of the substrings above. + * + * Matched against the end of the name so option prefixes used by individual + * plugins (rg_gforms_key, woocommerce_..._key) are covered without treating + * every name that merely contains "key" as sensitive. + * + * @const array + */ + const SECRET_KEY_SUFFIXES = array( + '_key', + ); + + /** + * Names that end in a secret-looking suffix but are not credentials. + * + * Public halves of key pairs are meant to be published, and redacting them + * removes useful audit detail for no benefit. Matched as substrings so the + * various plugin prefixes are covered. + * + * Only consulted after SECRET_KEY_PATTERNS, so a name containing an + * explicit secret marker is never exempted by appearing "public" too. + * + * @const array + */ + const PUBLIC_KEY_PATTERNS = array( + 'public_key', + 'publishable_key', + 'site_key', + ); + + /** + * Placeholder stored in place of a redacted value. + * + * A distinct marker rather than an empty string, so a reader of the audit + * trail can tell "this credential changed, value withheld" apart from "this + * field was cleared" -- an empty string would conflate the two. + * + * @const string + */ + const REDACTED_PLACEHOLDER = '[redacted]'; + + /** + * Whether a setting/field name looks like it holds a credential. + * + * @param string $key Setting or field name. + * @return bool + */ + public function is_secret_key( $key ) { + if ( ! is_string( $key ) || '' === $key ) { + return false; + } + + $needle = strtolower( $key ); + + // An explicit secret marker always wins. The public-name exemption + // below is only there to stop the broad "_key" suffix rule from + // swallowing published key halves, so it must not be able to rescue a + // name that also says "secret", "private_key" or "webhook" -- that + // would invert the over-redact-rather-than-under-redact preference. + foreach ( self::SECRET_KEY_PATTERNS as $pattern ) { + if ( false !== strpos( $needle, $pattern ) ) { + return true; + } + } + + foreach ( self::PUBLIC_KEY_PATTERNS as $pattern ) { + if ( false !== strpos( $needle, $pattern ) ) { + return false; + } + } + + foreach ( self::SECRET_KEY_SUFFIXES as $suffix ) { + if ( substr( $needle, -strlen( $suffix ) ) === $suffix ) { + return true; + } + } + + return false; + } + + /** + * Redact credential values before they are persisted as record metadata. + * + * Accepts either a scalar (redacted when $key itself looks secret) or an + * array of settings (each secret-looking member redacted, recursively). + * Values are replaced rather than removed so the audit trail still shows + * that the field changed, without retaining the credential. + * + * @param mixed $value Value about to be logged. + * @param string $key Setting or field name the value belongs to. + * @return mixed + */ + public function redact_secret_values( $value, $key = '' ) { + if ( is_array( $value ) ) { + foreach ( $value as $child_key => $child_value ) { + $value[ $child_key ] = $this->redact_secret_values( $child_value, (string) $child_key ); + } + + return $value; + } + + if ( $this->is_secret_key( $key ) && ! empty( $value ) ) { + return self::REDACTED_PLACEHOLDER; + } + + return $value; + } + /** * Save log data till shutdown, so other callbacks would be able to override * diff --git a/classes/class-network.php b/classes/class-network.php index c1133ed33..402868989 100644 --- a/classes/class-network.php +++ b/classes/class-network.php @@ -92,6 +92,8 @@ public function ajax_network_admin() { DOING_AJAX && 0 === stripos( $http_referer, network_admin_url() ) + && + $this->can_view_network_records() ) { define( 'WP_NETWORK_ADMIN', true ); return WP_NETWORK_ADMIN; @@ -100,6 +102,34 @@ public function ajax_network_admin() { return false; } + /** + * Whether the current user is allowed to read records across the whole + * network (and to be treated as being in the Network Admin). + * + * The Referer prefix checked in ajax_network_admin() is caller-controlled, + * so it can only ever be a UI hint about where a request came from -- never + * a source of authority. Network-wide record access additionally requires a + * real network capability, otherwise a site-level Stream viewer could spoof + * the header to lift the per-blog restriction applied in + * network_query_args() or to have their actions logged against blog_id 0. + * + * @return bool + */ + public function can_view_network_records() { + if ( ! is_multisite() ) { + return false; + } + + // WP-CLI runs with shell-level access and usually with no logged-in + // user, so capability checks would fail for a legitimate operator and + // break `wp stream query --blog_id=N`. It sits outside this boundary. + if ( defined( 'WP_CLI' ) && WP_CLI ) { + return true; + } + + return current_user_can( 'manage_network_options' ); + } + /** * Builds a stdClass object used when displaying actions done in network administration * @@ -501,6 +531,19 @@ public function blog_id_logged( $blog_id ) { */ public function network_query_args( $args ) { $args['site_id'] = is_numeric( $args['site_id'] ) ? $args['site_id'] : get_current_site()->id; + + // Only users with a network capability may choose which blog to read + // from. For everyone else the requested blog_id is ignored entirely and + // forced to the current blog: a numeric type check is not an + // authorization check, and the Stream tables are shared across the + // whole network, so honouring an arbitrary ?blog_id= would let a + // site-level viewer read another site's activity. + if ( ! $this->can_view_network_records() ) { + $args['blog_id'] = get_current_blog_id(); + + return $args; + } + $args['blog_id'] = is_numeric( $args['blog_id'] ) ? $args['blog_id'] : ( is_network_admin() ? null : get_current_blog_id() ); return $args; diff --git a/connectors/class-connector-edd.php b/connectors/class-connector-edd.php index 953060525..acb7e360b 100644 --- a/connectors/class-connector-edd.php +++ b/connectors/class-connector-edd.php @@ -394,14 +394,16 @@ public function check_edd_settings( $old_value, $new_value ) { continue; } + // EDD settings include payment gateway API keys and secrets, so + // redact credential-looking fields before they are persisted. $this->log( /* translators: %s: a setting title (e.g. "Language") */ __( '"%s" setting updated', 'stream' ), array( 'option_title' => $field['name'], 'option' => $option, - 'old_value' => isset( $old_value[ $option ] ) ? $old_value[ $option ] : null, - 'value' => isset( $new_value[ $option ] ) ? $new_value[ $option ] : null, + 'old_value' => isset( $old_value[ $option ] ) ? $this->redact_secret_values( $old_value[ $option ], $option ) : null, + 'value' => isset( $new_value[ $option ] ) ? $this->redact_secret_values( $new_value[ $option ], $option ) : null, 'tab' => $tab, ), null, diff --git a/connectors/class-connector-gravityforms.php b/connectors/class-connector-gravityforms.php index 034bbcda0..3e3cc717e 100644 --- a/connectors/class-connector-gravityforms.php +++ b/connectors/class-connector-gravityforms.php @@ -507,6 +507,12 @@ public function check( $option, $old_value, $new_value ) { $option_title = $data['label']; $context = isset( $data['context'] ) ? $data['context'] : 'settings'; + // Tracked options include credentials such as + // rg_gforms_captcha_private_key; record that they changed without + // retaining the value. + $old_value = $this->redact_secret_values( $old_value, $option ); + $new_value = $this->redact_secret_values( $new_value, $option ); + $this->log( /* translators: %s: a setting title (e.g. "Language") */ __( '"%s" setting updated', 'stream' ), @@ -529,6 +535,12 @@ public function check_rg_gforms_key( $old_value, $new_value ) { $is_update = ( $new_value && strlen( $new_value ) ); $option = 'rg_gforms_key'; + // The license key is a reusable vendor credential granting account and + // paid-download access, so only the fact of the change is recorded. The + // message already conveys whether it was set or removed. + $old_value = $this->redact_secret_values( $old_value, $option ); + $new_value = $this->redact_secret_values( $new_value, $option ); + $this->log( sprintf( /* translators: %s: a status (e.g. "updated") */ diff --git a/connectors/class-connector-jetpack.php b/connectors/class-connector-jetpack.php index d11492d76..2f1af579d 100644 --- a/connectors/class-connector-jetpack.php +++ b/connectors/class-connector-jetpack.php @@ -622,8 +622,8 @@ public function check_jetpack_options( $old_value, $new_value ) { $settings['meta'] += array( 'option' => $option, - 'old_value' => $old_value, - 'value' => $new_value, + 'old_value' => $this->redact_secret_values( $old_value, $option ), + 'value' => $this->redact_secret_values( $new_value, $option ), ); $this->log( diff --git a/connectors/class-connector-settings.php b/connectors/class-connector-settings.php index a5b7543ea..cd9250695 100644 --- a/connectors/class-connector-settings.php +++ b/connectors/class-connector-settings.php @@ -739,6 +739,10 @@ public function callback_updated_option( $option, $old_value, $value ) { $changed_options = array(); + // sanitize_value() only flattens complex types to strings, so on its own + // it happily persists credential settings such as mailserver_pass in + // record metadata. Redaction is applied per field, keyed on the setting + // name, before the value reaches the log. if ( $this->is_option_group( $value ) ) { foreach ( $this->get_changed_keys( $old_value, $value ) as $field_key ) { if ( ! $this->is_key_ignored( $option, $field_key ) ) { @@ -748,8 +752,8 @@ public function callback_updated_option( $option, $old_value, $value ) { 'option' => $option, 'option_key' => $field_key, 'context' => ( false !== $key_context ? $key_context : $context ), - 'old_value' => isset( $old_value[ $field_key ] ) ? $this->sanitize_value( $old_value[ $field_key ] ) : null, - 'value' => isset( $value[ $field_key ] ) ? $this->sanitize_value( $value[ $field_key ] ) : null, + 'old_value' => isset( $old_value[ $field_key ] ) ? $this->redact_secret_values( $this->sanitize_value( $old_value[ $field_key ] ), $field_key ) : null, + 'value' => isset( $value[ $field_key ] ) ? $this->redact_secret_values( $this->sanitize_value( $value[ $field_key ] ), $field_key ) : null, ); } } @@ -758,8 +762,8 @@ public function callback_updated_option( $option, $old_value, $value ) { 'label' => $this->get_field_label( $option ), 'option' => $option, 'context' => $context, - 'old_value' => $this->sanitize_value( $old_value ), - 'value' => $this->sanitize_value( $value ), + 'old_value' => $this->redact_secret_values( $this->sanitize_value( $old_value ), $option ), + 'value' => $this->redact_secret_values( $this->sanitize_value( $value ), $option ), ); } diff --git a/connectors/class-connector-woocommerce.php b/connectors/class-connector-woocommerce.php index b38e477fe..c44352a8e 100644 --- a/connectors/class-connector-woocommerce.php +++ b/connectors/class-connector-woocommerce.php @@ -723,6 +723,12 @@ public function callback_updated_option( $option_key, $old_value, $value ) { continue; } + // Payment gateway options are whole settings arrays belonging to + // third-party gateways, and they routinely co-locate operational + // settings with live API secrets and webhook signing keys. Redact + // credential-looking members before serializing, otherwise the + // secret is persisted in stream_meta and readable by every Stream + // viewer and record-detail API consumer. $this->log( /* translators: %1$s: a setting name, %2$s: a setting type (e.g. "Direct Deposit", "Payment Method") */ __( '"%1$s" %2$s updated', 'stream' ), @@ -733,8 +739,8 @@ public function callback_updated_option( $option_key, $old_value, $value ) { 'tab' => $this->settings[ $option ]['tab'], 'section' => $this->settings[ $option ]['section'], 'option' => $option, - 'old_value' => maybe_serialize( $old_value ), - 'value' => maybe_serialize( $value ), + 'old_value' => maybe_serialize( $this->redact_secret_values( $old_value, $option ) ), + 'value' => maybe_serialize( $this->redact_secret_values( $value, $option ) ), ), null, $this->settings[ $option ]['tab'], diff --git a/tests/phpunit/abilities/test-class-ability-get-alerts.php b/tests/phpunit/abilities/test-class-ability-get-alerts.php index b020a524a..7ccb1d794 100644 --- a/tests/phpunit/abilities/test-class-ability-get-alerts.php +++ b/tests/phpunit/abilities/test-class-ability-get-alerts.php @@ -126,4 +126,170 @@ public function test_alert_meta_is_normalized_to_object_when_missing() { // Schema validates as well — exercises the live contract. $this->assert_matches_schema( $result, $this->ability->get_output_schema() ); } + + /** + * Alert destinations are configured behind the Stream settings capability + * but listed behind view_stream, so credentials in alert_meta would cross a + * privilege boundary. A Slack incoming webhook URL is a bearer credential: + * possession alone is enough to post into the channel. + */ + public function test_slack_webhook_is_redacted() { + wp_set_current_user( $this->admin_user_id ); + + // Deliberately not shaped like a real Slack webhook URL: the redaction + // keys off the alert_meta key name, not the value, and a realistic + // looking URL trips secret scanning on push. + $webhook = 'https://example.test/redaction-fixture/not-a-real-webhook'; + + $post_id = wp_insert_post( + array( + 'post_type' => Alerts::POST_TYPE, + 'post_status' => 'wp_stream_enabled', + 'post_title' => 'Slack alert', + ) + ); + update_post_meta( $post_id, 'alert_type', 'slack' ); + update_post_meta( + $post_id, + 'alert_meta', + array( + 'webhook' => $webhook, + 'channel' => '#general', + 'trigger_action' => 'any', + ) + ); + + $row = $this->find_alert_row( $this->ability->execute( array( 'status' => 'any' ) ), $post_id ); + + $this->assertArrayNotHasKey( 'webhook', (array) $row['alert_meta'], 'The webhook URL must not be returned.' ); + $this->assertTrue( + ( (array) $row['alert_meta'] )['webhook_configured'], + 'Callers still need to know a webhook is configured.' + ); + + // Non-secret configuration must survive untouched. + $this->assertSame( '#general', ( (array) $row['alert_meta'] )['channel'] ); + + $this->assertStringNotContainsString( + $webhook, + (string) wp_json_encode( $row ), + 'The webhook URL must not appear anywhere in the serialized response.' + ); + } + + /** + * Same boundary for the IFTTT Maker key, which is reusable across every + * applet on the owning account. + */ + public function test_ifttt_maker_key_is_redacted() { + wp_set_current_user( $this->admin_user_id ); + + $maker_key = 'dxxxxxxxxxxxxxxxxxxxxxx'; + + $post_id = wp_insert_post( + array( + 'post_type' => Alerts::POST_TYPE, + 'post_status' => 'wp_stream_enabled', + 'post_title' => 'IFTTT alert', + ) + ); + update_post_meta( $post_id, 'alert_type', 'ifttt' ); + update_post_meta( + $post_id, + 'alert_meta', + array( + 'maker_key' => $maker_key, + 'event_name' => 'stream_event', + ) + ); + + $row = $this->find_alert_row( $this->ability->execute( array( 'status' => 'any' ) ), $post_id ); + + $this->assertArrayNotHasKey( 'maker_key', (array) $row['alert_meta'] ); + $this->assertTrue( ( (array) $row['alert_meta'] )['maker_key_configured'] ); + $this->assertSame( 'stream_event', ( (array) $row['alert_meta'] )['event_name'] ); + $this->assertStringNotContainsString( $maker_key, (string) wp_json_encode( $row ) ); + } + + /** + * An unconfigured secret reports false rather than being reported as + * present, so the marker is meaningful either way. + */ + public function test_unconfigured_secret_reports_false() { + wp_set_current_user( $this->admin_user_id ); + + $post_id = wp_insert_post( + array( + 'post_type' => Alerts::POST_TYPE, + 'post_status' => 'wp_stream_enabled', + 'post_title' => 'Slack alert without webhook', + ) + ); + update_post_meta( $post_id, 'alert_type', 'slack' ); + update_post_meta( $post_id, 'alert_meta', array( 'webhook' => '' ) ); + + $row = $this->find_alert_row( $this->ability->execute( array( 'status' => 'any' ) ), $post_id ); + + $this->assertFalse( ( (array) $row['alert_meta'] )['webhook_configured'] ); + } + + /** + * Third-party alert types can store destination secrets under names Stream + * does not know, so the redaction list is filterable. + */ + public function test_secret_alert_meta_keys_are_filterable() { + wp_set_current_user( $this->admin_user_id ); + + $post_id = wp_insert_post( + array( + 'post_type' => Alerts::POST_TYPE, + 'post_status' => 'wp_stream_enabled', + 'post_title' => 'Third-party alert', + ) + ); + update_post_meta( $post_id, 'alert_type', 'custom' ); + update_post_meta( + $post_id, + 'alert_meta', + array( + 'custom_api_secret' => 'super-secret-value', + 'endpoint' => 'https://example.com/notify', + ) + ); + + $add_key = static function ( $keys ) { + $keys[] = 'custom_api_secret'; + return $keys; + }; + add_filter( 'wp_stream_secret_alert_meta_keys', $add_key ); + + try { + $row = $this->find_alert_row( $this->ability->execute( array( 'status' => 'any' ) ), $post_id ); + $meta = (array) $row['alert_meta']; + + $this->assertArrayNotHasKey( 'custom_api_secret', $meta ); + $this->assertTrue( $meta['custom_api_secret_configured'] ); + $this->assertSame( 'https://example.com/notify', $meta['endpoint'] ); + $this->assertStringNotContainsString( 'super-secret-value', (string) wp_json_encode( $row ) ); + } finally { + remove_filter( 'wp_stream_secret_alert_meta_keys', $add_key ); + } + } + + /** + * Locate a single alert row in the ability output by post ID. + * + * @param array $result Ability output. + * @param int $post_id Alert post ID. + * @return array + */ + private function find_alert_row( $result, $post_id ) { + foreach ( $result as $entry ) { + if ( $entry['id'] === $post_id ) { + return $entry; + } + } + + $this->fail( 'Seeded alert missing from get-alerts output.' ); + } } diff --git a/tests/phpunit/abilities/test-class-ability-update-settings.php b/tests/phpunit/abilities/test-class-ability-update-settings.php index a0024c37b..814dbecbe 100644 --- a/tests/phpunit/abilities/test-class-ability-update-settings.php +++ b/tests/phpunit/abilities/test-class-ability-update-settings.php @@ -48,6 +48,87 @@ public function test_permissions() { $this->assertTrue( $this->ability->permission_callback() ); } + /** + * The update_all_setting_values() helper writes the network option when + * Stream is network activated, so the write hits every site. A plain site + * administrator holds manage_options but has no network authority and must + * be refused. + * + * @group ms-required + */ + public function test_site_admin_cannot_write_network_settings() { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'Requires multisite.' ); + } + + wp_set_current_user( $this->admin_user_id ); + $this->assertFalse( + is_super_admin( $this->admin_user_id ), + 'Guard: this test is only meaningful for a non-super-admin.' + ); + + add_filter( 'wp_stream_is_network_activated', '__return_true' ); + + try { + $this->assertFalse( + $this->ability->permission_callback(), + 'A site administrator must not be able to write network-wide Stream settings.' + ); + } finally { + remove_filter( 'wp_stream_is_network_activated', '__return_true' ); + } + } + + /** + * The network capability is an additional requirement, not a replacement + * for the Stream settings capability. On multisite a super admin passes + * every current_user_can() check by definition, so a permission callback + * that only consulted manage_network_options would silently ignore a + * deployment that locked Stream settings down. + * + * WP_User::has_cap() returns early for super admins, before the + * user_has_cap filter runs, so the only way a deployment can restrict them + * is 'do_not_allow' via map_meta_cap -- which is exactly the escape hatch + * that early return honours. That is what is simulated here. + * + * @group ms-required + */ + public function test_super_admin_denied_when_settings_capability_revoked() { + if ( ! is_multisite() ) { + $this->markTestSkipped( 'Requires multisite.' ); + } + + wp_set_current_user( $this->admin_user_id ); + grant_super_admin( $this->admin_user_id ); + + add_filter( 'wp_stream_is_network_activated', '__return_true' ); + + // Baseline: an unrestricted super admin is allowed. + $this->assertTrue( + $this->ability->permission_callback(), + 'Guard: a super admin should be allowed before the capability is revoked.' + ); + + $deny_settings_cap = static function ( $caps, $cap ) { + if ( WP_STREAM_SETTINGS_CAPABILITY === $cap ) { + return array( 'do_not_allow' ); + } + return $caps; + }; + add_filter( 'map_meta_cap', $deny_settings_cap, 10, 2 ); + + try { + $this->assertFalse( + $this->ability->permission_callback(), + 'Revoking the Stream settings capability must deny the write even for a super admin.' + ); + } finally { + remove_filter( 'map_meta_cap', $deny_settings_cap, 10 ); + remove_filter( 'wp_stream_is_network_activated', '__return_true' ); + revoke_super_admin( $this->admin_user_id ); + } + } + public function test_partial_update_preserves_other_keys() { wp_set_current_user( $this->admin_user_id ); diff --git a/tests/phpunit/connectors/test-class-connector-gravityforms.php b/tests/phpunit/connectors/test-class-connector-gravityforms.php new file mode 100644 index 000000000..798d97918 --- /dev/null +++ b/tests/phpunit/connectors/test-class-connector-gravityforms.php @@ -0,0 +1,143 @@ +plugin->connectors->unload_connectors(); + + $this->mock = $this->getMockBuilder( Connector_GravityForms::class ) + ->setMethods( array( 'log' ) ) + ->getMock(); + + // Populates $options, which check() consults. Safe without the plugin + // present: register() only builds that array. + $this->mock->register(); + } + + /** + * The reCAPTCHA private key is a tracked option, so check() would otherwise + * persist both the previous and replacement secret in record metadata, + * where the record-detail ability exposes it to any view_stream caller. + */ + public function test_captcha_private_key_is_redacted() { + $logged = array(); + $this->mock->expects( $this->once() ) + ->method( 'log' ) + ->willReturnCallback( + function ( $message, $args ) use ( &$logged ) { + $logged = $args; + return true; + } + ); + + $this->mock->check( 'rg_gforms_captcha_private_key', 'old-private-key', 'new-private-key' ); + + $this->assertSame( + 'rg_gforms_captcha_private_key', + $logged['option'], + 'The setting change must still be recorded.' + ); + $this->assertSame( Connector::REDACTED_PLACEHOLDER, $logged['old_value'] ); + $this->assertSame( Connector::REDACTED_PLACEHOLDER, $logged['new_value'] ); + + $serialized = maybe_serialize( $logged ); + $this->assertStringNotContainsString( 'old-private-key', $serialized ); + $this->assertStringNotContainsString( 'new-private-key', $serialized ); + } + + /** + * The matching public key is not a credential and must stay readable, so + * the audit log remains useful. + */ + public function test_captcha_public_key_is_not_redacted() { + $logged = array(); + $this->mock->expects( $this->once() ) + ->method( 'log' ) + ->willReturnCallback( + function ( $message, $args ) use ( &$logged ) { + $logged = $args; + return true; + } + ); + + $this->mock->check( 'rg_gforms_captcha_public_key', 'old-public', 'new-public' ); + + $this->assertSame( 'old-public', $logged['old_value'] ); + $this->assertSame( 'new-public', $logged['new_value'] ); + } + + /** + * The license key is a reusable vendor credential. Only the fact of the + * change is recorded; the message still distinguishes set from removed. + */ + public function test_license_key_is_redacted_on_update() { + $message = ''; + $logged = array(); + $this->mock->expects( $this->once() ) + ->method( 'log' ) + ->willReturnCallback( + function ( $msg, $args ) use ( &$logged, &$message ) { + $message = $msg; + $logged = $args; + return true; + } + ); + + $this->mock->check_rg_gforms_key( 'old-license', 'new-license' ); + + $this->assertSame( 'rg_gforms_key', $logged['option'] ); + $this->assertSame( Connector::REDACTED_PLACEHOLDER, $logged['old_value'] ); + $this->assertSame( Connector::REDACTED_PLACEHOLDER, $logged['new_value'] ); + + // The update/delete distinction is derived before redaction, so it must + // still report an update here. + $this->assertStringContainsString( 'updated', $message ); + + $serialized = maybe_serialize( $logged ); + $this->assertStringNotContainsString( 'old-license', $serialized ); + $this->assertStringNotContainsString( 'new-license', $serialized ); + } + + /** + * Clearing the license key must still be reported as a deletion, proving + * redaction did not disturb the status derivation. + */ + public function test_license_key_deletion_still_reported() { + $message = ''; + $this->mock->expects( $this->once() ) + ->method( 'log' ) + ->willReturnCallback( + function ( $msg ) use ( &$message ) { + $message = $msg; + return true; + } + ); + + $this->mock->check_rg_gforms_key( 'old-license', '' ); + + $this->assertStringContainsString( 'deleted', $message ); + } +} diff --git a/tests/phpunit/connectors/test-class-connector-settings.php b/tests/phpunit/connectors/test-class-connector-settings.php index 4c1baf350..e1ebb2d76 100644 --- a/tests/phpunit/connectors/test-class-connector-settings.php +++ b/tests/phpunit/connectors/test-class-connector-settings.php @@ -156,4 +156,107 @@ public function test_callback_updated_option() { $this->assertGreaterThan( 0, did_action( $this->action_prefix . 'callback_update_option' ) ); } } + + /** + * The mailserver_pass core setting is tracked, so both its previous and new + * value would otherwise be persisted verbatim in record metadata -- + * sanitize_value() only casts to string. The change must still be recorded, + * but without the mailbox password. + */ + public function test_mailserver_pass_is_redacted() { + $add_method = is_multisite() ? 'add_site_option' : 'add_option'; + $update_method = is_multisite() ? 'update_site_option' : 'update_option'; + + // Core seeds this option as an empty string, so add_option() would be a + // no-op; write the prior value before the logging expectation is set so + // both sides of the observed change are non-empty. + call_user_func( $update_method, 'mailserver_pass', 'old-secret-password' ); + + $this->register_writing_options(); + + $logged = array(); + $this->mock->expects( $this->atLeastOnce() ) + ->method( 'log' ) + ->willReturnCallback( + function ( $message, $args ) use ( &$logged ) { + $logged = $args; + return true; + } + ); + + call_user_func( $update_method, 'mailserver_pass', 'new-secret-password' ); + + $this->assertSame( + 'mailserver_pass', + $logged['option'], + 'The setting change must still be recorded.' + ); + $this->assertSame( Connector::REDACTED_PLACEHOLDER, $logged['old_value'], 'The previous password must not be retained.' ); + $this->assertSame( Connector::REDACTED_PLACEHOLDER, $logged['value'], 'The new password must not be retained.' ); + + $serialized = maybe_serialize( $logged ); + $this->assertStringNotContainsString( 'old-secret-password', $serialized ); + $this->assertStringNotContainsString( 'new-secret-password', $serialized ); + } + + /** + * Redaction is keyed on the setting name, so ordinary settings are logged + * with their values intact. + */ + public function test_non_secret_setting_is_not_redacted() { + $add_method = is_multisite() ? 'add_site_option' : 'add_option'; + $update_method = is_multisite() ? 'update_site_option' : 'update_option'; + + $this->register_writing_options(); + + // This option is seeded by the WP test fixture, so read the existing + // value rather than assuming add_option() sets it. + call_user_func( $add_method, 'mailserver_login', 'old-login' ); + $previous = is_multisite() ? get_site_option( 'mailserver_login' ) : get_option( 'mailserver_login' ); + + $logged = array(); + $this->mock->expects( $this->once() ) + ->method( 'log' ) + ->willReturnCallback( + function ( $message, $args ) use ( &$logged ) { + $logged = $args; + return true; + } + ); + + call_user_func( $update_method, 'mailserver_login', 'new-login' ); + + $this->assertSame( $previous, $logged['old_value'] ); + $this->assertSame( 'new-login', $logged['value'] ); + $this->assertNotSame( '', $logged['value'], 'A non-secret setting must keep its value.' ); + } + + /** + * Connector_Settings::callback_update_option() only forwards to the logging + * path when the request looks like a real settings save -- either WP-CLI or + * inside customize_save. Mirrors the setup already used by + * test_callback_updated_option(). + * + * Also registers the mail server options under the "writing" group the way + * wp-admin/options.php does, so callback_updated_option() resolves a context + * for them. + * + * @return void + */ + private function register_writing_options() { + global $whitelist_options; + + if ( ! is_array( $whitelist_options ) ) { + $whitelist_options = array(); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited + } + + $whitelist_options['writing'] = array_merge( // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited + isset( $whitelist_options['writing'] ) ? (array) $whitelist_options['writing'] : array(), + array( 'mailserver_pass', 'mailserver_login' ) + ); + + // Simulate being on a settings save request. + require_once ABSPATH . WPINC . '/class-wp-customize-manager.php'; + do_action( 'customize_save', new \WP_Customize_Manager( array() ) ); + } } diff --git a/tests/phpunit/test-class-connector.php b/tests/phpunit/test-class-connector.php index 55305f6b8..d3a11505d 100644 --- a/tests/phpunit/test-class-connector.php +++ b/tests/phpunit/test-class-connector.php @@ -271,4 +271,144 @@ public function test_escape_percentages() { $escaped_value ); } + + /** + * Credential-looking setting names are recognised regardless of case or + * surrounding words, since connectors log arbitrary third-party option + * names that an allowlist could not anticipate. + * + * @return void + */ + public function test_is_secret_key() { + $secret = array( + 'mailserver_pass', + 'password', + 'rg_gforms_captcha_private_key', + 'stripe_secret_key', + 'API_KEY', + 'publishable_token', + 'webhook', + 'client_secret', + 'rg_gforms_key', + ); + foreach ( $secret as $key ) { + $this->assertTrue( + $this->connector->is_secret_key( $key ), + "$key should be treated as a secret." + ); + } + + $safe = array( + 'blogname', + 'general_records_ttl', + 'title', + 'enabled', + 'email_recipient', + 'default_category', + // Words merely ending in "key" must not be caught; only the "_key" + // suffix used by real credential option names. + 'monkey', + 'turkey', + // Public halves of key pairs are meant to be published. + 'rg_gforms_captcha_public_key', + 'stripe_publishable_key', + 'recaptcha_site_key', + '', + ); + foreach ( $safe as $key ) { + $this->assertFalse( + $this->connector->is_secret_key( $key ), + "$key should not be treated as a secret." + ); + } + } + + /** + * The public-name exemption exists only to stop the broad "_key" suffix + * rule from catching published key halves. It must not rescue a name that + * also carries an explicit secret marker, or the exemption would become a + * bypass. + * + * @return void + */ + public function test_explicit_secret_marker_beats_public_name() { + $this->assertTrue( $this->connector->is_secret_key( 'secret_site_key' ) ); + $this->assertTrue( $this->connector->is_secret_key( 'webhook_public_key' ) ); + $this->assertTrue( $this->connector->is_secret_key( 'private_key_public_key' ) ); + + // Still exempt when nothing marks them as secret. + $this->assertFalse( $this->connector->is_secret_key( 'recaptcha_site_key' ) ); + $this->assertFalse( $this->connector->is_secret_key( 'rg_gforms_captcha_public_key' ) ); + } + + /** + * A scalar is redacted based on its own key, which is the shape used by the + * settings and Gravity Forms connectors. + * + * @return void + */ + public function test_redact_secret_values_scalar() { + $this->assertSame( + Connector::REDACTED_PLACEHOLDER, + $this->connector->redact_secret_values( 'hunter2', 'mailserver_pass' ) + ); + $this->assertSame( + 'My Site', + $this->connector->redact_secret_values( 'My Site', 'blogname' ) + ); + } + + /** + * An unset credential stays an empty string rather than gaining the + * placeholder, so a reader can distinguish "was never set" from "set but + * withheld" -- the latter carries the marker. + * + * @return void + */ + public function test_redact_secret_values_leaves_empty_values() { + $this->assertSame( '', $this->connector->redact_secret_values( '', 'password' ) ); + $this->assertNotSame( + Connector::REDACTED_PLACEHOLDER, + $this->connector->redact_secret_values( '', 'password' ), + 'An empty credential must not look like a withheld one.' + ); + } + + /** + * Whole settings arrays (payment gateways, integration configs) are + * redacted member-wise and recursively, keeping non-secret settings intact + * so the record still describes what changed. + * + * @return void + */ + public function test_redact_secret_values_array() { + $gateway = array( + 'enabled' => 'yes', + 'title' => 'Credit Card', + 'secret_key' => 'sk_live_deadbeefdeadbeef', + 'publishable_key' => 'pk_live_public', + 'nested' => array( + 'webhook' => 'https://example.com/hook/abcdef', + 'description' => 'Pay by card', + ), + ); + + $redacted = $this->connector->redact_secret_values( $gateway, 'woocommerce_stripe_settings' ); + + $this->assertSame( 'yes', $redacted['enabled'] ); + $this->assertSame( 'Credit Card', $redacted['title'] ); + $this->assertSame( Connector::REDACTED_PLACEHOLDER, $redacted['secret_key'], 'A live API secret must not be persisted.' ); + $this->assertSame( Connector::REDACTED_PLACEHOLDER, $redacted['nested']['webhook'], 'Nested credentials must be redacted too.' ); + $this->assertSame( 'Pay by card', $redacted['nested']['description'] ); + + // The publishable half of a key pair is designed to be public, so + // redacting it would remove audit detail for no benefit. + $this->assertSame( 'pk_live_public', $redacted['publishable_key'] ); + + $this->assertStringNotContainsString( + 'sk_live_deadbeefdeadbeef', + maybe_serialize( $redacted ), + 'The secret must not survive serialization into record metadata.' + ); + } } diff --git a/tests/phpunit/test-class-network.php b/tests/phpunit/test-class-network.php new file mode 100644 index 000000000..9c7fe8d38 --- /dev/null +++ b/tests/phpunit/test-class-network.php @@ -0,0 +1,198 @@ +markTestSkipped( 'Requires multisite.' ); + } + + $this->network = new Network( $this->plugin ); + } + + /** + * A site administrator holds manage_options but has no network authority, + * so a caller-supplied blog_id must be discarded in favour of the current + * blog. A numeric type check is not an authorization check: honouring an + * arbitrary ?blog_id= would expose another site's records from the shared + * Stream table. + * + * @group ms-required + */ + public function test_site_admin_cannot_choose_blog_id() { + $user_id = self::factory()->user->create( array( 'role' => 'administrator' ) ); + wp_set_current_user( $user_id ); + + $other_blog_id = (int) get_current_blog_id() + 100; + + $args = $this->network->network_query_args( + array( + 'site_id' => null, + 'blog_id' => $other_blog_id, + ) + ); + + $this->assertSame( + get_current_blog_id(), + $args['blog_id'], + 'A site administrator must be pinned to the current blog.' + ); + $this->assertNotSame( + $other_blog_id, + $args['blog_id'], + 'The requested blog_id must not survive for a non-network user.' + ); + } + + /** + * The same restriction applies to a Stream viewer with no settings + * capability at all. + * + * @group ms-required + */ + public function test_subscriber_cannot_choose_blog_id() { + $user_id = self::factory()->user->create( array( 'role' => 'subscriber' ) ); + wp_set_current_user( $user_id ); + + $args = $this->network->network_query_args( + array( + 'site_id' => null, + 'blog_id' => (int) get_current_blog_id() + 100, + ) + ); + + $this->assertSame( get_current_blog_id(), $args['blog_id'] ); + } + + /** + * A super admin legitimately administers the whole network, so an explicit + * blog_id must still be honoured for them -- the fix must not break + * cross-site filtering in the Network Admin. + * + * @group ms-required + */ + public function test_super_admin_may_choose_blog_id() { + $user_id = self::factory()->user->create( array( 'role' => 'administrator' ) ); + wp_set_current_user( $user_id ); + grant_super_admin( $user_id ); + + $other_blog_id = (int) get_current_blog_id() + 100; + + try { + $args = $this->network->network_query_args( + array( + 'site_id' => null, + 'blog_id' => $other_blog_id, + ) + ); + + $this->assertSame( + $other_blog_id, + $args['blog_id'], + 'A super admin must retain cross-site record filtering.' + ); + } finally { + revoke_super_admin( $user_id ); + } + } + + /** + * The site_id argument keeps its existing default behaviour for every caller. + * + * @group ms-required + */ + public function test_site_id_defaults_to_current_network() { + wp_set_current_user( self::factory()->user->create( array( 'role' => 'administrator' ) ) ); + + $args = $this->network->network_query_args( + array( + 'site_id' => null, + 'blog_id' => null, + ) + ); + + $this->assertSame( (int) get_current_site()->id, (int) $args['site_id'] ); + } + + /** + * HTTP_REFERER is caller-controlled, so it can only ever be a hint about + * where a request originated -- never a grant of authority. Without a + * network capability a spoofed Referer must not be treated as network + * admin, otherwise a site-level viewer could lift the per-blog query + * restriction and have their actions logged against blog_id 0. + * + * @group ms-required + */ + public function test_spoofed_referer_does_not_grant_network_context() { + $user_id = self::factory()->user->create( array( 'role' => 'administrator' ) ); + wp_set_current_user( $user_id ); + + $this->assertFalse( + $this->network->can_view_network_records(), + 'A site administrator must not be treated as a network-wide reader.' + ); + } + + /** + * Counterpart to the above: a genuine super admin is recognised. + * + * @group ms-required + */ + public function test_super_admin_may_view_network_records() { + $user_id = self::factory()->user->create( array( 'role' => 'administrator' ) ); + wp_set_current_user( $user_id ); + grant_super_admin( $user_id ); + + try { + $this->assertTrue( $this->network->can_view_network_records() ); + } finally { + revoke_super_admin( $user_id ); + } + } + + /** + * The blog_id_logged() filter zeroes the recorded blog when the request is + * in network admin context. Since that context can no longer be set from a + * Referer alone, a site user's actions stay attributed to their own blog. + * + * @group ms-required + */ + public function test_blog_id_logged_keeps_site_attribution_for_site_user() { + wp_set_current_user( self::factory()->user->create( array( 'role' => 'administrator' ) ) ); + + $blog_id = get_current_blog_id(); + + $this->assertSame( + $blog_id, + $this->network->blog_id_logged( $blog_id ), + 'A site user must not have their activity recorded as network activity.' + ); + } +}