From a7a15122db9d3aa16d8a02193d3f6b1b0aebf801 Mon Sep 17 00:00:00 2001 From: Enej Bajgoric Date: Tue, 22 Sep 2026 13:05:40 -0700 Subject: [PATCH] Feature Flags: show the a8c screen on every proxied request, gate saving on a support session check --- .../fix-wpcom-feature-flags-menu-visibility | 4 + .../class-wpcom-feature-flags.php | 198 +++++++++++------- .../Wpcom_Feature_Flags_Simple_Gate_Test.php | 7 + .../Wpcom_Feature_Flags_Test.php | 139 +++++++++--- 4 files changed, 240 insertions(+), 108 deletions(-) create mode 100644 projects/packages/jetpack-mu-wpcom/changelog/fix-wpcom-feature-flags-menu-visibility diff --git a/projects/packages/jetpack-mu-wpcom/changelog/fix-wpcom-feature-flags-menu-visibility b/projects/packages/jetpack-mu-wpcom/changelog/fix-wpcom-feature-flags-menu-visibility new file mode 100644 index 000000000000..695768707a7b --- /dev/null +++ b/projects/packages/jetpack-mu-wpcom/changelog/fix-wpcom-feature-flags-menu-visibility @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Feature Flags: Show the Automattician screen on every proxied Atomic request, and offer a support session check before saving. diff --git a/projects/packages/jetpack-mu-wpcom/src/features/wpcom-feature-flags/class-wpcom-feature-flags.php b/projects/packages/jetpack-mu-wpcom/src/features/wpcom-feature-flags/class-wpcom-feature-flags.php index 7bad15b5773e..baa9f298b56e 100644 --- a/projects/packages/jetpack-mu-wpcom/src/features/wpcom-feature-flags/class-wpcom-feature-flags.php +++ b/projects/packages/jetpack-mu-wpcom/src/features/wpcom-feature-flags/class-wpcom-feature-flags.php @@ -20,7 +20,9 @@ * Two properties are load-bearing: * * - The *screen* is Automattician-only and fails closed. Without the wpcom - * platform primitives that identify an Automattician, nobody sees it. + * platform primitives that identify an Automattician, nobody sees it. On + * Atomic, saving additionally needs a positive "not a support session" + * verdict; the screen offers a way to get one. * - The *overrides* are site-wide and are NOT re-gated on the Automattician * check. That is the point: an override has to change what the site actually * does, logged-out visitors included, or it cannot be used to test a flag @@ -80,6 +82,16 @@ class Wpcom_Feature_Flags { */ const CAPABILITY = 'manage_options'; + /** + * Save blocked: no support session verdict is stored for this browser. + */ + const SAVE_BLOCKED_UNVERIFIED = 'unverified'; + + /** + * Save blocked: this request is probably a support session. + */ + const SAVE_BLOCKED_SUPPORT_SESSION = 'support-session'; + /** * Cache of one blog's sanitized override map; valid only for $overrides_blog_id. * @@ -109,25 +121,16 @@ public static function init() { } /** - * Whether the current visitor is an Automattician. - * - * Mirrors the platform split already used by do_not_track_a11ns() in - * wpcom-wpadmin-page-view.php: on Simple the platform's own - * is_automattician() is authoritative, and on Atomic the a8c proxy is what - * identifies us. Both branches fail closed when their primitive is missing. - * - * A support session reaches an Atomic site through the same proxy, but it is - * a Happiness Engineer acting on the site owner's behalf rather than an - * Automattician testing unreleased work, so it is excluded. That exclusion - * fails closed too — see is_support_session() for why it cannot just ask - * wpcomsh and believe the answer. - * - * AT_PROXIED_REQUEST is read through Constants rather than defined() — which - * is what the neighbouring code uses — so the Atomic branch is reachable from - * tests at all. That is safe here because Constants::set_constant() is only - * callable by code already executing in this process, which by then can do - * anything this gate protects, and because manage_options is required on top. - * Do not lean on this gate alone for anything stronger. + * Whether the current visitor is an Automattician, which is what shows the screen. + * + * Mirrors do_not_track_a11ns() in wpcom-wpadmin-page-view.php: on Simple the + * platform's is_automattician() is authoritative, on Atomic the a8c proxy is. + * Both fail closed when their primitive is missing. This matches the + * "PROXIED V2" banner, so the menu entry shows whenever that banner does; + * writes are held to the stricter current_user_can_save(). + * + * AT_PROXIED_REQUEST is read through Constants so tests can reach the Atomic + * branch; manage_options is required on top. * * @return bool Whether the current visitor is an Automattician. */ @@ -136,22 +139,83 @@ public static function is_a11n() { return function_exists( 'is_automattician' ) && (bool) is_automattician(); } - if ( ! Constants::is_true( 'AT_PROXIED_REQUEST' ) ) { - return false; - } - - return ! self::is_support_session(); + return Constants::is_true( 'AT_PROXIED_REQUEST' ); } /** - * Whether the current user may read and change this site's flag overrides. + * Whether the current user may see this site's flag overrides. * - * @return bool Whether the current user may manage flag overrides. + * @return bool Whether the current user may view the screen. */ public static function current_user_can_manage() { return self::is_a11n() && current_user_can( self::CAPABILITY ); } + /** + * Whether the current user may change this site's flag overrides. + * + * @return bool Whether the current user may save overrides. + */ + public static function current_user_can_save() { + return self::current_user_can_manage() && '' === self::get_save_block_reason(); + } + + /** + * Why saving is blocked for this request, beyond the view gate. + * + * @return string '' when nothing blocks it, SAVE_BLOCKED_UNVERIFIED when + * no support session verdict is available, or + * SAVE_BLOCKED_SUPPORT_SESSION during a support session. + */ + public static function get_save_block_reason() { + if ( ( new Host() )->is_wpcom_simple() ) { + return ''; + } + + // The detector ships in wpcomsh; without it a support session cannot be ruled out. + if ( ! class_exists( 'WPCOMSH_Support_Session_Detect' ) ) { + return self::SAVE_BLOCKED_UNVERIFIED; + } + + // wpcomsh reads a missing cookie as "not a support session", which is too weak for a write gate. + if ( ! WPCOMSH_Support_Session_Detect::has_detection_result() ) { + return self::SAVE_BLOCKED_UNVERIFIED; + } + + return WPCOMSH_Support_Session_Detect::is_probably_support_session() ? self::SAVE_BLOCKED_SUPPORT_SESSION : ''; + } + + /** + * URL that runs wpcomsh's support session detection and lands back on the screen. + * + * The detector only accepts a /wp-login.php return path, and wp-login.php + * forwards an already logged-in user straight to redirect_to. + * + * @return string The URL, or '' when detection is unavailable on this request. + */ + public static function get_verification_url() { + if ( ! class_exists( 'WPCOMSH_Support_Session_Detect' ) || WPCOMSH_Support_Session_Detect::has_detection_result() ) { + return ''; + } + + $return_to = add_query_arg( + array( + WPCOMSH_Support_Session_Detect::QUERY_PARAM_TO_SHORT_CIRCUIT => '', + 'redirect_to' => rawurlencode( admin_url( 'tools.php?page=' . self::PAGE_SLUG ) ), + ), + WPCOMSH_Support_Session_Detect::LOGIN_PATH + ); + + return add_query_arg( + array( + 'redirect' => rawurlencode( $return_to ), + 'nonce' => wp_create_nonce( WPCOMSH_Support_Session_Detect::NONCE_ACTION ), + ), + // Relative, so the cookie is set on the host serving wp-admin. + WPCOMSH_Support_Session_Detect::DETECTION_URI + ); + } + /** * Return this site's flag overrides. * @@ -351,8 +415,8 @@ public static function maybe_handle_submission() { /** * Apply a submitted override form. * - * Re-checks the Automattician gate, the capability, and the nonce: the - * screen's absence from the menu is not authorization on its own. + * Re-checks the save gate, the capability, and the nonce: the screen's + * absence from the menu is not authorization on its own. * * The submitted map replaces the stored one wholesale, so two Automatticians * saving the same site concurrently is last-write-wins. Every form carries @@ -364,7 +428,7 @@ public static function maybe_handle_submission() { * @return bool Whether the overrides were saved. */ public static function handle_save( array $request ) { - if ( ! self::current_user_can_manage() ) { + if ( ! self::current_user_can_save() ) { return false; } @@ -401,7 +465,7 @@ public static function render_admin_page() { ? sanitize_key( wp_unslash( $_GET['flags-notice'] ) ) : ''; - self::print_screen( self::get_rows(), self::get_overrides(), $notice ); + self::print_screen( self::get_rows(), self::get_overrides(), $notice, self::get_save_block_reason() ); } /** @@ -410,9 +474,10 @@ public static function render_admin_page() { * @param array $rows Flags to list, keyed by flag name. * @param array $overrides The overrides currently in force. * @param string $notice 'saved', 'rejected', or '' for a plain load. + * @param string $blocked A get_save_block_reason() value. * @return void */ - private static function print_screen( array $rows, array $overrides, $notice = '' ) { + private static function print_screen( array $rows, array $overrides, $notice = '', $blocked = '' ) { $states = array( 'default' => 'Default', 'on' => 'Force on', @@ -439,6 +504,32 @@ private static function print_screen( array $rows, array $overrides, $notice = ' + +
+

+ Saving is disabled during a support session. This screen is for + Automatticians testing their own work, not for changing a customer’s site. +

+
+ + +
+

+ Saving is disabled until this browser is checked for a support session. + The check stores its answer in a cookie, which is missing here — it expires, and + is only set when you log in through WordPress.com. +

+

+ + Verify this session + It takes a few seconds and brings you back here. + + Log out, then log back in with WordPress.com, and return to this screen. + +

+
+ +

Automatticians only — internal Automattic tooling. This screen is @@ -549,53 +640,12 @@ private static function print_screen( array $rows, array $overrides, $notice = ' - + 'disabled' ) ); ?>

assertFalse( Wpcom_Feature_Flags::is_a11n() ); } + + /** + * Simple has no support session cookie; is_automattician() alone decides. + */ + public function test_nothing_blocks_saving_on_simple() { + $this->assertSame( '', Wpcom_Feature_Flags::get_save_block_reason() ); + } } diff --git a/projects/packages/jetpack-mu-wpcom/tests/php/features/wpcom-feature-flags/Wpcom_Feature_Flags_Test.php b/projects/packages/jetpack-mu-wpcom/tests/php/features/wpcom-feature-flags/Wpcom_Feature_Flags_Test.php index b8ef43ca90da..ece453df7a26 100644 --- a/projects/packages/jetpack-mu-wpcom/tests/php/features/wpcom-feature-flags/Wpcom_Feature_Flags_Test.php +++ b/projects/packages/jetpack-mu-wpcom/tests/php/features/wpcom-feature-flags/Wpcom_Feature_Flags_Test.php @@ -92,12 +92,9 @@ public function tear_down() { } /** - * Make the request look like a proxied Automattician request on Atomic. + * Make the request look like a verified proxied Automattician request on Atomic. * - * The detection cookie has to say "not a support session" out loud. The gate - * fails closed on a missing verdict, so the proxy constant alone is not enough - * — which is what test_gate_rejects_atomic_request_without_a_detection_result - * pins. + * The cookie is what unlocks saving; the proxy constant alone only shows the screen. */ private function simulate_proxied_atomic_request() { Constants::set_constant( 'AT_PROXIED_REQUEST', true ); @@ -161,46 +158,41 @@ public function test_gate_passes_for_proxied_atomic_request() { } /** - * A support session also arrives through the proxy, but it is driven by a - * Happiness Engineer acting for the site owner rather than by an - * Automattician testing unreleased work. It must not open the panel. + * The screen shows whenever the "PROXIED V2" banner does, even when the + * SameSite=Strict detection cookie was dropped on a cross-site navigation. */ - public function test_gate_rejects_atomic_support_session() { - $this->simulate_proxied_atomic_request(); - $_COOKIE[ \WPCOMSH_Support_Session_Detect::COOKIE_NAME ] = 'true'; + public function test_gate_passes_for_proxied_request_without_a_detection_result() { + Constants::set_constant( 'AT_PROXIED_REQUEST', true ); - $this->assertFalse( Wpcom_Feature_Flags::is_a11n() ); + $this->assertTrue( Wpcom_Feature_Flags::is_a11n() ); } /** - * The proxy alone is not enough: without a stored detection result there is - * nothing ruling out a support session, so the gate must stay shut. - * - * The wpcomsh detector reports a missing cookie as "not a support session", - * which is the wrong default to build an authorization gate on: the cookie - * expires on wpcom's schedule, is SameSite=Strict so a cross-site navigation - * omits it, and can simply be deleted. Trusting that answer let the support - * session this gate exists to exclude walk straight through it. + * Without a stored verdict nothing rules out a support session, so saving stays shut. */ - public function test_gate_rejects_atomic_request_without_a_detection_result() { + public function test_save_gate_rejects_atomic_request_without_a_detection_result() { + $this->login_as_admin(); Constants::set_constant( 'AT_PROXIED_REQUEST', true ); - $this->assertArrayNotHasKey( - \WPCOMSH_Support_Session_Detect::COOKIE_NAME, - $_COOKIE, - 'Guard: this test is meaningless if a detection result is present.' - ); - $this->assertFalse( Wpcom_Feature_Flags::is_a11n() ); + $this->assertTrue( Wpcom_Feature_Flags::current_user_can_manage() ); + $this->assertFalse( Wpcom_Feature_Flags::current_user_can_save() ); + $this->assertSame( Wpcom_Feature_Flags::SAVE_BLOCKED_UNVERIFIED, Wpcom_Feature_Flags::get_save_block_reason() ); } - /** - * A positive "not a support session" verdict is what opens the Atomic gate. - */ - public function test_gate_passes_for_proxied_request_with_a_negative_detection_result() { - Constants::set_constant( 'AT_PROXIED_REQUEST', true ); - $_COOKIE[ \WPCOMSH_Support_Session_Detect::COOKIE_NAME ] = 'false'; + public function test_save_gate_rejects_atomic_support_session() { + $this->login_as_admin(); + $this->simulate_proxied_atomic_request(); + $_COOKIE[ \WPCOMSH_Support_Session_Detect::COOKIE_NAME ] = 'true'; - $this->assertTrue( Wpcom_Feature_Flags::is_a11n() ); + $this->assertFalse( Wpcom_Feature_Flags::current_user_can_save() ); + $this->assertSame( Wpcom_Feature_Flags::SAVE_BLOCKED_SUPPORT_SESSION, Wpcom_Feature_Flags::get_save_block_reason() ); + } + + public function test_save_gate_passes_for_proxied_request_with_a_negative_detection_result() { + $this->login_as_admin(); + $this->simulate_proxied_atomic_request(); + + $this->assertTrue( Wpcom_Feature_Flags::current_user_can_save() ); } /** @@ -902,6 +894,85 @@ public function test_admin_page_refuses_a_non_automattician() { Wpcom_Feature_Flags::render_admin_page(); } + public function test_save_handler_rejects_an_unverified_session() { + $this->login_as_admin(); + Constants::set_constant( 'AT_PROXIED_REQUEST', true ); + + $saved = Wpcom_Feature_Flags::handle_save( + array( + '_wpnonce' => wp_create_nonce( Wpcom_Feature_Flags::NONCE_ACTION ), + 'flag_state' => array( 'my-feature' => 'on' ), + ) + ); + + $this->assertFalse( $saved ); + $this->assertSame( array(), Wpcom_Feature_Flags::get_overrides() ); + } + + public function test_admin_page_registered_for_an_unverified_session() { + $this->login_as_admin(); + Constants::set_constant( 'AT_PROXIED_REQUEST', true ); + + $this->assertNotFalse( Wpcom_Feature_Flags::register_admin_page() ); + } + + public function test_unverified_screen_offers_verification_and_disables_saving() { + $this->login_as_admin(); + Constants::set_constant( 'AT_PROXIED_REQUEST', true ); + + $output = $this->render_admin_page(); + + $this->assertStringContainsString( 'Verify this session', $output ); + $this->assertStringContainsString( 'href="' . esc_url( Wpcom_Feature_Flags::get_verification_url() ) . '"', $output ); + $this->assertMatchesRegularExpression( '/]*type="submit"[^>]*disabled/', $output ); + } + + public function test_verified_screen_leaves_saving_enabled() { + $this->become_automattician_admin(); + + $output = $this->render_admin_page(); + + $this->assertStringNotContainsString( 'Verify this session', $output ); + $this->assertDoesNotMatchRegularExpression( '/]*type="submit"[^>]*disabled/', $output ); + } + + public function test_support_session_screen_explains_and_disables_saving() { + $this->become_automattician_admin(); + $_COOKIE[ \WPCOMSH_Support_Session_Detect::COOKIE_NAME ] = 'true'; + + $output = $this->render_admin_page(); + + $this->assertStringContainsString( 'disabled during a support session', $output ); + $this->assertStringNotContainsString( 'Verify this session', $output ); + $this->assertMatchesRegularExpression( '/]*type="submit"[^>]*disabled/', $output ); + } + + /** + * The detector only honours a /wp-login.php return path, so the round trip + * has to go through it and name the screen as redirect_to. + */ + public function test_verification_url_round_trips_through_the_detector_to_the_screen() { + $this->login_as_admin(); + Constants::set_constant( 'AT_PROXIED_REQUEST', true ); + + $url = Wpcom_Feature_Flags::get_verification_url(); + wp_parse_str( (string) wp_parse_url( $url, PHP_URL_QUERY ), $query ); + + $this->assertSame( \WPCOMSH_Support_Session_Detect::DETECTION_URI, wp_parse_url( $url, PHP_URL_PATH ) ); + $this->assertSame( 1, wp_verify_nonce( $query['nonce'], \WPCOMSH_Support_Session_Detect::NONCE_ACTION ) ); + $this->assertSame( \WPCOMSH_Support_Session_Detect::LOGIN_PATH, wp_parse_url( $query['redirect'], PHP_URL_PATH ) ); + + wp_parse_str( (string) wp_parse_url( $query['redirect'], PHP_URL_QUERY ), $login_query ); + $this->assertArrayHasKey( \WPCOMSH_Support_Session_Detect::QUERY_PARAM_TO_SHORT_CIRCUIT, $login_query ); + $this->assertSame( admin_url( 'tools.php?page=' . Wpcom_Feature_Flags::PAGE_SLUG ), $login_query['redirect_to'] ); + } + + public function test_no_verification_url_once_a_verdict_is_stored() { + $this->become_automattician_admin(); + + $this->assertSame( '', Wpcom_Feature_Flags::get_verification_url() ); + } + /** * Return a wp_die handler that throws instead of exiting, so wp_die() can be * asserted on.