3.0.0 en multisitio

Hola Fernando,

Enhorabuena por la 3.0.0. Me he leído el diff completo y el changelog entero, y es un salto de versión que se lo ha ganado. El salto del segundo factor usando un subsitio como puerta era serio, y la manera de cerrarlo, la unión de lo que pide cada sitio en lugar de leer solo el principal, es la correcta: leer solo el principal habría apagado el 2FA en todas las redes que lo tenían configurado por subsitio, que hasta ahora era la única forma de configurarlo.

Y gracias por la mención en el changelog. 🙂

Van dos cosas de multisitio. La primera creo que la querrás mirar antes de que alguien actualice una red grande.

1. La autoprotección es por sitio, y los ficheros son de la instalación

Es el mismo razonamiento que te llevó al registro único de red en la 2.11.3, aplicado al código nuevo.

El estado vive en una opción del sitio:

// class-self-integrity.php:834
$state = get_option( self::STATE_OPTION, array() );

Los checksums de wordpress.org, en un transient del sitio (:754). El throttle del watchdog, en otro (:1769). Y vigilante_daily_maintenance es un evento de cron por sitio, que llama a las dos entradas:

// vigilante.php:819
if ( $this->self_integrity ) {
    if ( $this->self_integrity->is_enabled() ) {
        $this->self_integrity->detect_version_change();
        $this->self_integrity->run_watchdog();

El chequeo de ficheros que hay al final de run_watchdog() queda fuera del bloque de owns_shared_files(), así que corre en todos los sitios, no solo en el que manda.

En una red de N sitios eso son, cada día y para unos ficheros que son uno solo: N recorridos completos del árbol con hash y normalización de los ficheros de texto (2,9 MB, 63 ficheros), hasta N peticiones HTTP a wordpress.org, y N copias de vigilante_self_integrity_state. Y maybe_run_watchdog() cuelga de admin_init, así que ese wp_remote_get() con timeout de 5 segundos puede caer dentro de una carga del escritorio.

El argumento ya lo tienes escrito, en el comentario de maybe_send_self_alert(): “the plugin files are the same for every site and every network of an installation, so one email”. Si vale para el correo, vale para el chequeo.

Pero lo que más me preocupa no es el coste, es lo que se ve. Al actualizar en una red, solo el sitio donde corre el updater recibe el contexto ‘upgrader’. Los demás llegan por detect_version_change(), con contexto ‘version_change’, y ahí la rama que se toma depende de si wordpress.org confirma el manifiesto nuevo. En las primeras horas después de publicar no lo confirma, porque los checksums todavía no están generados, y tu propio código cachea ese 404 una hora. Entonces cada uno de esos sitios levanta manifest_unverified:

"MANIFEST.sha256 changed after a Vigilant version change but could not be verified
 against WordPress.org. If you did not just update Vigilant, treat this as possible tampering."

O sea, una actualización normal de una red deja ese aviso en todos los subsitios menos uno. Y display_state() no lo tapa: se queda con el chequeo más reciente de los dos, que es el del subsitio, porque corrió después.

Yo lo llevaría a lo mismo que hiciste con la línea base: estado en opción de red, y el evento del updater anclado también en una opción de red (versión, huella del manifiesto y momento), de modo que un sitio hermano que se encuentre el cambio de versión pueda leer que lo escribió el actualizador de WordPress y no tenga que deducirlo de wordpress.org. Con eso desaparecen de golpe el aviso falso, las N peticiones y los N recorridos, y de paso floor_version() en class-self-repair.php deja de depender de desde qué sitio se pulse el botón de reparar.

2. two_factor_demands_for() no memoiza, y se pregunta en cada pantalla del escritorio

Esta es barata de arreglar y se nota en cualquier red con muchos sitios.

En class-settings.php:1038, para un superadministrador:

if ( is_super_admin( $user->ID ) ) {
    $blog_ids = array_merge( $blog_ids, get_sites( array( 'fields' => 'ids', 'number' => 200 ) ) );
}

y luego, por cada uno de esos IDs, un get_blog_option() y un WP_User::for_site(), que carga las capacidades de la cuenta en ese blog.

Quien la llama son dos ganchos del escritorio, no solo el login:

// class-two-factor-totp.php
add_action( 'admin_notices', array( $this, 'show_grace_period_notice' ) );
add_action( 'admin_init', array( $this, 'force_totp_setup_redirect' ) );

Los dos pasan por handles_second_factor() y acaban en two_factor_demands_for(). En una red de 200 sitios son hasta 400 lecturas de opción y 400 de meta de capacidades por cada página de wp-admin que abra un superadministrador, y en el mismo request se repite entera la segunda vez porque no hay ninguna caché: busqué un static en la clase y no hay.

El comentario del método ya avisa de la parte difícil, que la pregunta no es solo de login (“the dashboard hooks of the TOTP class ask it on every admin screen of every site of a network, at least once per hook”), así que supongo que fue quedarse sin manos. Con una caché estática por petición indexada por ID de usuario se queda en una pasada:

private static function two_factor_demands_for( $user ) {
    if ( empty( $user->ID ) ) {
        return array();
    }

    static $cache = array();

    if ( isset( $cache[ $user->ID ] ) ) {
        return $cache[ $user->ID ];
    }

    // ... el cuerpo actual, guardando en $cache[ $user->ID ] antes de devolver.

Dentro de una misma petición los ajustes de los otros sitios no van a cambiar, así que no se pierde nada.

3. Tres cosas menores (para seguir ayudando)

• get_wporg_sha256_checksums() construye la URL con el slug fijo ‘vigilante’ (:762), mientras que Vigilante_Self_Repair::folder_is_the_distributed_one() sí contempla que la carpeta se haya renombrado y por eso no ofrece el botón. Es coherente, pero entonces en una instalación con la carpeta renombrada el chequeo sigue preguntando por un slug que puede no ser el suyo. Quizá merezca que el mensaje de “carpeta renombrada” cubra también el chequeo, y no solo la reparación.

• disabled_by() lista todos los callbacks enganchados a vigilante_self_integrity_enabled, devuelvan true o false. Hoy no se nota porque solo la llamas cuando el chequeo está apagado, pero si algún día la llamas desde otro sitio, un plugin que enganche el filtro para devolver true explícitamente saldría como el que lo apagó.

• Y una de expectativas más que de código: Loco Translate escribe por defecto los .po y .mo dentro de la carpeta del plugin, así que en instalaciones que lo usen van a aparecer como self_extra. La severidad está bien, es warning y no crítico, pero una línea en SECURITY.md diciendo que un fichero de traducción ahí dentro es eso y no un ataque ahorraría algún susto.

Nada de esto es urgente salvo lo del punto 1, y ahí lo urgente no es el coste sino el aviso de posible manipulación que van a ver los subsitios de cualquier red que actualice en las primeras horas.

Por mi parte, y por si te sirve para saber que el acoplamiento sigue sano: los cuatro métodos de los que depende mi Vigilante Network Sync están intactos en la 3.0.0, y gracias por haber dejado escrito en get_user_data_keys() que alguien de fuera la lee. La 3.0.0 sí me obligó a un cambio, pero era fallo mío: trusted_proxies entró en esa lista en la 2.11.9 y mi red de seguridad hizo lo correcto, preservarla por sitio, mientras trusted_proxy_header sí se copiaba. Los subsitios se quedaban con la cabecera puesta y la lista vacía, y como desde la 2.11.9 la cabecera solo se acepta desde un par reconocido, detrás de un proxy público veían a todos los visitantes con la dirección del proxy. Las dos claves describen el mismo hosting, así que ahora viajan juntas. Lo digo por si alguien te llega con un multisitio donde el bloqueo de login se come a todo el mundo a la vez: puede ser eso, y se arregla actualizando mi plugin.

Un saludo,
Albert