From 344a390978010670085c4100e30808e3b317fb26 Mon Sep 17 00:00:00 2001 From: John Hooks Date: Tue, 4 Aug 2026 13:51:33 -0700 Subject: [PATCH 1/2] fix(http): stop trusting spoofable IP headers The activity log recorded whichever address a request claimed to come from. It read the forwarded-for headers without checking that the request had passed through a proxy, and took the left-most entry, which is the part of that list a client writes. Anyone could attribute their own actions to an address of their choosing, so the log could not be relied on when investigating one. Two other places answered the same question differently. The audit log read the connecting address through a helper of its own, and rate limiting had a third implementation. A limit, a log line, and an audit record could each name a different caller for the same request. They now share one answer, defaulting to the address the webserver saw, which a caller cannot forge. Sites behind a CDN still need the proxy's header to see past the edge, so an operator names it with WORKOS_CLIENT_IP_HEADER. That is a deliberate assertion that the origin accepts nothing but proxied traffic, because an origin reachable directly makes the header attacker-controlled again. The effect is that per-IP limits count real callers instead of a value the caller chooses, and the activity log becomes usable as evidence. --- docs/rate-limiting.md | 29 ++- src/WorkOS/ActivityLog/EventLogger.php | 28 +-- src/WorkOS/Auth/ChangeEmail/RestApi.php | 13 +- .../Auth/PasswordResetAdmin/RestApi.php | 13 +- src/WorkOS/Config.php | 34 +++ src/WorkOS/Controller.php | 4 +- src/WorkOS/Http/ClientIp.php | 133 ++++++++++ src/WorkOS/Http/Controller.php | 42 ++++ src/WorkOS/REST/Auth/BaseEndpoint.php | 11 + src/WorkOS/REST/Auth/Invitation.php | 4 +- src/WorkOS/REST/Auth/MagicCode.php | 4 +- src/WorkOS/REST/Auth/Mfa.php | 4 +- src/WorkOS/REST/Auth/OAuth.php | 2 +- src/WorkOS/REST/Auth/Password.php | 6 +- src/WorkOS/REST/Auth/Signup.php | 4 +- src/WorkOS/RateLimit/BaseRateLimiter.php | 31 --- src/WorkOS/RateLimit/RateLimiter.php | 16 -- src/WorkOS/RateLimit/TieredRateLimiter.php | 18 -- src/WorkOS/Sync/AuditLog.php | 19 +- tests/wpunit/AuthKitRestMagicSessionTest.php | 5 +- tests/wpunit/AuthKitRestMfaTest.php | 2 + tests/wpunit/AuthKitRestPasswordTest.php | 4 +- .../AuthKitRestSignupInvitationOAuthTest.php | 7 +- tests/wpunit/ChangeEmailRestApiTest.php | 2 + tests/wpunit/ClientIpTest.php | 232 ++++++++++++++++++ .../wpunit/PasswordResetAdminRestApiTest.php | 2 + tests/wpunit/RateLimiterTest.php | 52 ---- tests/wpunit/TieredRateLimiterTest.php | 15 -- 28 files changed, 552 insertions(+), 184 deletions(-) create mode 100644 src/WorkOS/Http/ClientIp.php create mode 100644 src/WorkOS/Http/Controller.php create mode 100644 tests/wpunit/ClientIpTest.php diff --git a/docs/rate-limiting.md b/docs/rate-limiting.md index 161cbc5..1fd5007 100644 --- a/docs/rate-limiting.md +++ b/docs/rate-limiting.md @@ -28,6 +28,8 @@ Everything lives in `src/WorkOS/RateLimit/`: | `TieredRateLimiter` | Stacks windows into a policy. Owns the policy constants. | | `Controller` | Binds the implementation and the wrapper. | +Client IP resolution lives outside this module, in `WorkOS\Http\ClientIp`, because the activity log and the audit log need the same answer. See [Who the caller is](#who-the-caller-is). + ```mermaid flowchart LR E[Endpoint] --> T[TieredRateLimiter] @@ -105,8 +107,8 @@ So that path claims the slot before the call and hands it back when the outcome ```php $reserved = $this->rate_limit( [ - [ 'password_otp_ip', $ip, TieredRateLimiter::SEND_IP ], - [ 'password_otp_recipient', $email, TieredRateLimiter::SEND_RECIPIENT ], + [ self::BUCKET_PASSWORD_OTP_IP, $ip, TieredRateLimiter::SEND_IP ], + [ self::BUCKET_PASSWORD_OTP_RECIPIENT, $email, TieredRateLimiter::SEND_RECIPIENT ], ] ); if ( is_wp_error( $reserved ) ) { @@ -116,8 +118,8 @@ if ( is_wp_error( $reserved ) ) { $workos_response = workos()->api()->authenticate_with_password( /* … */ ); if ( ! $this->triggered_verification_email( $workos_response ) ) { - $this->rate_limiter->refund( 'password_otp_ip', $ip, TieredRateLimiter::SEND_IP ); - $this->rate_limiter->refund( 'password_otp_recipient', $email, TieredRateLimiter::SEND_RECIPIENT ); + $this->rate_limiter->refund( self::BUCKET_PASSWORD_OTP_IP, $ip, TieredRateLimiter::SEND_IP ); + $this->rate_limiter->refund( self::BUCKET_PASSWORD_OTP_RECIPIENT, $email, TieredRateLimiter::SEND_RECIPIENT ); } ``` @@ -135,6 +137,22 @@ One consequence worth knowing: windows are aligned to the clock rather than star Tiers are charged shortest window first, and the first exhausted tier returns immediately. A burst stopped by the short tier therefore never inflates the longer ones. A refused attempt still counts against the tier that refused it, so hammering keeps that window pinned until it rolls. +### Who the caller is + +Every per-IP bucket is only as good as the address behind it, so resolution has one owner: `WorkOS\Http\ClientIp`, shared by rate limiting, the activity log, and the audit log. + +By default that is `REMOTE_ADDR` and nothing else. The webserver sets it and a caller cannot forge it. + +Behind a CDN, `REMOTE_ADDR` is the edge, so every visitor collapses into one bucket and the proxy's own header carries the real client. That header is authoritative only if the request actually arrived through the proxy: if the origin is reachable directly, the header is caller-supplied, and trusting it is worse than ignoring it, because anyone can mint a fresh bucket per request by changing a string. So it is never auto-detected. An operator names it, asserting that the origin only accepts proxied traffic: + +```php +define( 'WORKOS_CLIENT_IP_HEADER', 'CF-Connecting-IP' ); +``` + +A real environment variable of the same name works too. When the header holds a list, the **right-most public** entry wins: `X-Forwarded-For` appends on each hop, so entries to the right were written by infrastructure and everything to the left is whatever the client sent. Private and reserved entries are skipped while walking leftwards, and an entirely private list (a local proxy in development) falls back to the right-most valid entry. + +When nothing usable is found, resolution returns `0.0.0.0` so a bucket still has a stable subject. + ### Storage `ObjectCacheRateLimiter` is used when WordPress reports a persistent object cache. On a real backend `wp_cache_incr` is an atomic in-place increment, so two concurrent requests get distinct counts and a limit cannot be overspent. @@ -150,7 +168,7 @@ wp eval 'var_dump( wp_using_ext_object_cache() );' ## Adding a send path 1. Take the slot before sending, with `SEND_IP` and `SEND_RECIPIENT`. -2. Reuse the `otp_send` prefix unless the path has a reason not to share the allowance. +2. Reuse `BaseEndpoint::BUCKET_OTP_SEND_IP` / `_RECIPIENT` unless the path has a reason not to share the allowance. Bucket names are constants so the sharing survives a rename. 3. Charge it before any "should we actually send?" branch, so a `429` never depends on whether the account exists. 4. If you only learn afterwards that nothing went out, refund both buckets. @@ -187,5 +205,6 @@ That is what was removed. One policy, one place, applied to every path that mail - `src/WorkOS/RateLimit/` — the module. - `src/WorkOS/REST/Auth/BaseEndpoint.php` — the `rate_limit()` helper endpoints call. +- `src/WorkOS/Http/ClientIp.php` — caller identity, shared with the logs. - [`docs/change-email.md`](change-email.md) — the change-email flow's use of the send policy. - [`docs/password-reset.md`](password-reset.md) — the reset endpoints. diff --git a/src/WorkOS/ActivityLog/EventLogger.php b/src/WorkOS/ActivityLog/EventLogger.php index 92831ad..ddf2fc6 100644 --- a/src/WorkOS/ActivityLog/EventLogger.php +++ b/src/WorkOS/ActivityLog/EventLogger.php @@ -7,6 +7,8 @@ namespace WorkOS\ActivityLog; +use WorkOS\App; +use WorkOS\Http\ClientIp; use WorkOS\Vendor\StellarWP\SuperGlobals\SuperGlobals; defined( 'ABSPATH' ) || exit; @@ -190,30 +192,14 @@ public static function is_enabled(): bool { /** * Get the client IP address. * + * Resolved from the container so a site that trusts a proxy header + * gets the same caller here as its rate limits and audit log do. + * * @return string */ private static function get_ip_address(): string { - $headers = [ - 'HTTP_CF_CONNECTING_IP', - 'HTTP_X_FORWARDED_FOR', - 'HTTP_X_REAL_IP', - 'REMOTE_ADDR', - ]; - - foreach ( $headers as $header ) { - // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Dynamic key; sanitized below via FILTER_VALIDATE_IP. - if ( ! empty( $_SERVER[ $header ] ) ) { - $ip = sanitize_text_field( wp_unslash( $_SERVER[ $header ] ) ); - // X-Forwarded-For may contain multiple IPs; use the first. - if ( str_contains( $ip, ',' ) ) { - $ip = trim( explode( ',', $ip )[0] ); - } - if ( filter_var( $ip, FILTER_VALIDATE_IP ) ) { - return $ip; - } - } - } + $ip = App::container()->get( ClientIp::class )->get(); - return ''; + return ClientIp::UNKNOWN === $ip ? '' : $ip; } } diff --git a/src/WorkOS/Auth/ChangeEmail/RestApi.php b/src/WorkOS/Auth/ChangeEmail/RestApi.php index 5140c67..d2044c6 100644 --- a/src/WorkOS/Auth/ChangeEmail/RestApi.php +++ b/src/WorkOS/Auth/ChangeEmail/RestApi.php @@ -10,6 +10,7 @@ use WorkOS\ActivityLog\EventLogger; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\Email\AddressMask; +use WorkOS\Http\ClientIp; use WP_Error; use WP_REST_Request; use WP_REST_Response; @@ -60,6 +61,13 @@ class RestApi { */ private TieredRateLimiter $rate_limiter; + /** + * Client IP resolver. + * + * @var ClientIp + */ + private ClientIp $client_ip; + /** * Token factory. * @@ -104,6 +112,7 @@ class RestApi { * change-email flow). The same same-host policy is enforced. * * @param TieredRateLimiter $rate_limiter Rate limiter. + * @param ClientIp $client_ip Client IP resolver. * @param TokenFactory $tokens Token factory. * @param PendingChange $pending Pending-change storage. * @param ConflictResolver $conflicts Conflict resolver. @@ -112,6 +121,7 @@ class RestApi { */ public function __construct( TieredRateLimiter $rate_limiter, + ClientIp $client_ip, TokenFactory $tokens, PendingChange $pending, ConflictResolver $conflicts, @@ -119,6 +129,7 @@ public function __construct( AddressMask $masker ) { $this->rate_limiter = $rate_limiter; + $this->client_ip = $client_ip; $this->tokens = $tokens; $this->pending = $pending; $this->conflicts = $conflicts; @@ -278,7 +289,7 @@ public function initiate( WP_REST_Request $request ) { // account is what's common to them. $rate_ok = $this->rate_limiter->attempt( 'change_email_init_ip', - $this->rate_limiter->client_ip(), + $this->client_ip->get(), TieredRateLimiter::SEND_IP ); if ( is_wp_error( $rate_ok ) ) { diff --git a/src/WorkOS/Auth/PasswordResetAdmin/RestApi.php b/src/WorkOS/Auth/PasswordResetAdmin/RestApi.php index 3b15082..2c4c079 100644 --- a/src/WorkOS/Auth/PasswordResetAdmin/RestApi.php +++ b/src/WorkOS/Auth/PasswordResetAdmin/RestApi.php @@ -12,6 +12,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\Email\AddressMask; +use WorkOS\Http\ClientIp; use WP_Error; use WP_REST_Request; use WP_REST_Response; @@ -48,6 +49,13 @@ class RestApi { */ private TieredRateLimiter $rate_limiter; + /** + * Client IP resolver. + * + * @var ClientIp + */ + private ClientIp $client_ip; + /** * Redirect URL validator. * @@ -67,17 +75,20 @@ class RestApi { * * @param ProfileRepository $profiles Profile repository. * @param TieredRateLimiter $rate_limiter Rate limiter. + * @param ClientIp $client_ip Client IP resolver. * @param RedirectValidator $redirect_validator Redirect validator. * @param AddressMask $masker Email-address masker. */ public function __construct( ProfileRepository $profiles, TieredRateLimiter $rate_limiter, + ClientIp $client_ip, RedirectValidator $redirect_validator, AddressMask $masker ) { $this->profiles = $profiles; $this->rate_limiter = $rate_limiter; + $this->client_ip = $client_ip; $this->redirect_validator = $redirect_validator; $this->masker = $masker; } @@ -195,7 +206,7 @@ public function send_reset( WP_REST_Request $request ) { ); } - $ip = $this->rate_limiter->client_ip(); + $ip = $this->client_ip->get(); // No send policy here: this route needs `edit_user` on the target and // mails that account's own address, so the recipient is never // caller-chosen. diff --git a/src/WorkOS/Config.php b/src/WorkOS/Config.php index 0885e3f..b42e1a8 100644 --- a/src/WorkOS/Config.php +++ b/src/WorkOS/Config.php @@ -47,6 +47,17 @@ class Config { 'redirect_urls' => 'WORKOS_REDIRECT_URLS', ]; + /** + * Map of plain-string setting names to their PHP constant overrides. + * + * Read straight from the constant or environment rather than synced into + * an option row: these are deployment facts, not editorial settings, and + * nothing in the admin edits them. + */ + private const STRING_CONSTANT_MAP = [ + 'client_ip_header' => 'WORKOS_CLIENT_IP_HEADER', + ]; + /** * Get the active environment. * @@ -167,6 +178,29 @@ public static function is_overridden( string $setting ): bool { return $generic_const && defined( $generic_const ) && '' !== constant( $generic_const ); } + /** + * Read a plain-string deployment setting. + * + * Constant first, matching how every other override here resolves, then a + * real environment variable for containerised installs. + * + * @param string $setting Key from {@see self::STRING_CONSTANT_MAP}. + * @param string $fallback Value when unset. + * + * @return string + */ + public static function string_setting( string $setting, string $fallback ): string { + $name = self::STRING_CONSTANT_MAP[ $setting ] ?? ''; + + if ( '' === $name ) { + return $fallback; + } + + $value = defined( $name ) ? constant( $name ) : getenv( $name ); + + return is_string( $value ) && '' !== $value ? $value : $fallback; + } + /** * Mask a secret value for UI display. * diff --git a/src/WorkOS/Controller.php b/src/WorkOS/Controller.php index 6896f05..42f638b 100644 --- a/src/WorkOS/Controller.php +++ b/src/WorkOS/Controller.php @@ -19,6 +19,7 @@ use WorkOS\REST\Controller as RESTController; use WorkOS\Webhook\Controller as WebhookController; use WorkOS\Sync\Controller as SyncController; +use WorkOS\Http\Controller as HttpController; use WorkOS\Organization\Controller as OrganizationController; use WorkOS\RateLimit\Controller as RateLimitController; use WorkOS\CLI\Controller as CLIController; @@ -36,7 +37,8 @@ class Controller extends BaseController { * @return void */ protected function doRegister(): void { - // Ahead of the features that resolve a rate limiter at construction. + // Ahead of the features that resolve these at construction. + $this->container->register( HttpController::class ); $this->container->register( RateLimitController::class ); $this->container->register( AdminController::class ); diff --git a/src/WorkOS/Http/ClientIp.php b/src/WorkOS/Http/ClientIp.php new file mode 100644 index 0000000..0fb4101 --- /dev/null +++ b/src/WorkOS/Http/ClientIp.php @@ -0,0 +1,133 @@ +trusted_header = trim( $trusted_header ); + } + + /** + * Resolve the client IP for this request. + * + * @return string IPv4 or IPv6 address; {@see self::UNKNOWN} when nothing is usable. + */ + public function get(): string { + if ( '' !== $this->trusted_header ) { + $forwarded = $this->from_header( $this->trusted_header ); + + if ( '' !== $forwarded ) { + return $forwarded; + } + } + + return $this->remote_addr(); + } + + /** + * The address the webserver saw. Unforgeable, but possibly a proxy. + * + * @return string + */ + public function remote_addr(): string { + $remote = isset( $_SERVER['REMOTE_ADDR'] ) + ? filter_var( wp_unslash( $_SERVER['REMOTE_ADDR'] ), FILTER_VALIDATE_IP ) + : false; + + return is_string( $remote ) && '' !== $remote ? $remote : self::UNKNOWN; + } + + /** + * Pull a validated address out of a named request header. + * + * When the header carries a list, the *right-most public* entry wins. + * `X-Forwarded-For` appends on each hop, so everything to the right of + * the client was written by infrastructure and everything to the left is + * whatever the client chose to send; reading from the front hands the + * bucket key back to the attacker. Private and reserved entries are + * skipped on the way left so internal hops don't shadow the client, and + * if the whole list is private (a local proxy in development) the + * right-most valid entry is still used. + * + * Returns empty rather than a fallback so the caller can decide. + * + * @param string $header Header name, e.g. `CF-Connecting-IP`. + * + * @return string Empty when absent or not an address. + */ + private function from_header( string $header ): string { + $key = 'HTTP_' . strtoupper( str_replace( '-', '_', $header ) ); + + if ( empty( $_SERVER[ $key ] ) ) { + return ''; + } + + $value = sanitize_text_field( wp_unslash( $_SERVER[ $key ] ) ); + $entries = array_reverse( explode( ',', $value ) ); + $fallback = ''; + + foreach ( $entries as $entry ) { + $candidate = filter_var( trim( $entry ), FILTER_VALIDATE_IP ); + + if ( ! is_string( $candidate ) ) { + continue; + } + + if ( '' === $fallback ) { + $fallback = $candidate; + } + + if ( false !== filter_var( $candidate, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE ) ) { + return $candidate; + } + } + + return $fallback; + } +} diff --git a/src/WorkOS/Http/Controller.php b/src/WorkOS/Http/Controller.php new file mode 100644 index 0000000..6e2e254 --- /dev/null +++ b/src/WorkOS/Http/Controller.php @@ -0,0 +1,42 @@ +container->when( ClientIp::class ) + ->needs( '$trusted_header' ) + ->give( static fn(): string => Config::string_setting( 'client_ip_header', '' ) ); + + $this->container->singleton( ClientIp::class ); + } + + /** + * Nothing to unhook: this module only provides container bindings. + * + * @return void + */ + protected function doUnregister(): void { + } +} diff --git a/src/WorkOS/REST/Auth/BaseEndpoint.php b/src/WorkOS/REST/Auth/BaseEndpoint.php index b11029b..a32a651 100644 --- a/src/WorkOS/REST/Auth/BaseEndpoint.php +++ b/src/WorkOS/REST/Auth/BaseEndpoint.php @@ -12,6 +12,7 @@ use WorkOS\Auth\AuthKit\Profile; use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; +use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WP_Error; use WP_REST_Request; @@ -71,6 +72,13 @@ abstract class BaseEndpoint { */ protected TieredRateLimiter $rate_limiter; + /** + * Client IP resolver. + * + * @var ClientIp + */ + protected ClientIp $client_ip; + /** * Login completer. * @@ -85,6 +93,7 @@ abstract class BaseEndpoint { * @param Nonce $nonce Nonce helper. * @param Radar $radar Radar helper. * @param TieredRateLimiter $rate_limiter Rate limiter. + * @param ClientIp $client_ip Client IP resolver. * @param LoginCompleter $login_completer Login completer. */ public function __construct( @@ -92,12 +101,14 @@ public function __construct( Nonce $nonce, Radar $radar, TieredRateLimiter $rate_limiter, + ClientIp $client_ip, LoginCompleter $login_completer ) { $this->profiles = $profiles; $this->nonce = $nonce; $this->radar = $radar; $this->rate_limiter = $rate_limiter; + $this->client_ip = $client_ip; $this->login_completer = $login_completer; } diff --git a/src/WorkOS/REST/Auth/Invitation.php b/src/WorkOS/REST/Auth/Invitation.php index 152a8ca..460708c 100644 --- a/src/WorkOS/REST/Auth/Invitation.php +++ b/src/WorkOS/REST/Auth/Invitation.php @@ -74,7 +74,7 @@ public function lookup( WP_REST_Request $request ) { $rate_ok = $this->rate_limit( [ - [ 'invitation_lookup_ip', $this->rate_limiter->client_ip(), TieredRateLimiter::REQUEST_IP ], + [ 'invitation_lookup_ip', $this->client_ip->get(), TieredRateLimiter::REQUEST_IP ], ] ); if ( is_wp_error( $rate_ok ) ) { @@ -152,7 +152,7 @@ public function accept( WP_REST_Request $request ) { $rate_ok = $this->rate_limit( [ - [ 'invitation_accept_ip', $this->rate_limiter->client_ip(), TieredRateLimiter::REQUEST_IP ], + [ 'invitation_accept_ip', $this->client_ip->get(), TieredRateLimiter::REQUEST_IP ], ] ); if ( is_wp_error( $rate_ok ) ) { diff --git a/src/WorkOS/REST/Auth/MagicCode.php b/src/WorkOS/REST/Auth/MagicCode.php index f3f2984..d640f79 100644 --- a/src/WorkOS/REST/Auth/MagicCode.php +++ b/src/WorkOS/REST/Auth/MagicCode.php @@ -83,7 +83,7 @@ public function send( WP_REST_Request $request ) { ); } - $ip = $this->rate_limiter->client_ip(); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'magic_send_ip', $ip, TieredRateLimiter::REQUEST_IP ], @@ -165,7 +165,7 @@ public function verify( WP_REST_Request $request ) { ); } - $ip = $this->rate_limiter->client_ip(); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'magic_verify_ip', $ip, TieredRateLimiter::REQUEST_IP ], diff --git a/src/WorkOS/REST/Auth/Mfa.php b/src/WorkOS/REST/Auth/Mfa.php index 7226926..a629daf 100644 --- a/src/WorkOS/REST/Auth/Mfa.php +++ b/src/WorkOS/REST/Auth/Mfa.php @@ -152,7 +152,7 @@ public function challenge( WP_REST_Request $request ) { // single IP and collecting a fresh OTP on each one. $rate_ok = $this->rate_limit( [ - [ 'mfa_challenge_ip', $this->rate_limiter->client_ip(), TieredRateLimiter::REQUEST_IP ], + [ 'mfa_challenge_ip', $this->client_ip->get(), TieredRateLimiter::REQUEST_IP ], [ 'mfa_challenge_factor', $factor_id, TieredRateLimiter::REQUEST_SUBJECT ], ] ); @@ -208,7 +208,7 @@ public function verify( WP_REST_Request $request ) { $rate_ok = $this->rate_limit( [ - [ 'mfa_verify_ip', $this->rate_limiter->client_ip(), TieredRateLimiter::REQUEST_IP ], + [ 'mfa_verify_ip', $this->client_ip->get(), TieredRateLimiter::REQUEST_IP ], ] ); if ( is_wp_error( $rate_ok ) ) { diff --git a/src/WorkOS/REST/Auth/OAuth.php b/src/WorkOS/REST/Auth/OAuth.php index 5629d14..4b8b9eb 100644 --- a/src/WorkOS/REST/Auth/OAuth.php +++ b/src/WorkOS/REST/Auth/OAuth.php @@ -103,7 +103,7 @@ public function authorize_url( WP_REST_Request $request ) { $rate_ok = $this->rate_limit( [ - [ 'oauth_url_ip', $this->rate_limiter->client_ip(), self::RATE_LIMIT_URL_IP ], + [ 'oauth_url_ip', $this->client_ip->get(), self::RATE_LIMIT_URL_IP ], ] ); if ( is_wp_error( $rate_ok ) ) { diff --git a/src/WorkOS/REST/Auth/Password.php b/src/WorkOS/REST/Auth/Password.php index e1efddf..faf4c67 100644 --- a/src/WorkOS/REST/Auth/Password.php +++ b/src/WorkOS/REST/Auth/Password.php @@ -120,7 +120,7 @@ public function authenticate( WP_REST_Request $request ) { ); } - $ip = $this->rate_limiter->client_ip(); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'password_ip', $ip, TieredRateLimiter::REQUEST_IP ], @@ -248,7 +248,7 @@ public function reset_start( WP_REST_Request $request ) { ); } - $ip = $this->rate_limiter->client_ip(); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'pw_reset_ip', $ip, TieredRateLimiter::REQUEST_IP ], @@ -338,7 +338,7 @@ public function reset_confirm( WP_REST_Request $request ) { ); } - $ip = $this->rate_limiter->client_ip(); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'pw_reset_confirm_ip', $ip, TieredRateLimiter::REQUEST_IP ], diff --git a/src/WorkOS/REST/Auth/Signup.php b/src/WorkOS/REST/Auth/Signup.php index c4f985e..ceb3244 100644 --- a/src/WorkOS/REST/Auth/Signup.php +++ b/src/WorkOS/REST/Auth/Signup.php @@ -108,7 +108,7 @@ public function create( WP_REST_Request $request ) { ); } - $ip = $this->rate_limiter->client_ip(); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'signup_ip', $ip, self::RATE_LIMIT_CREATE_IP ], @@ -185,7 +185,7 @@ public function verify( WP_REST_Request $request ) { ); } - $ip = $this->rate_limiter->client_ip(); + $ip = $this->client_ip->get(); $rate_ok = $this->rate_limit( [ [ 'signup_verify_ip', $ip, self::RATE_LIMIT_VERIFY_IP ], diff --git a/src/WorkOS/RateLimit/BaseRateLimiter.php b/src/WorkOS/RateLimit/BaseRateLimiter.php index 72c81b0..b023f80 100644 --- a/src/WorkOS/RateLimit/BaseRateLimiter.php +++ b/src/WorkOS/RateLimit/BaseRateLimiter.php @@ -166,35 +166,4 @@ protected function key( string $bucket, string $subject, int $window, int $now ) return self::KEY_PREFIX . $safe_bucket . '_' . $window . '_' . intdiv( $now, $window ) . '_' . $hash; } - - /** - * Best-effort client IP extraction. - * - * Respects the standard WP proxy hint hierarchy without blindly trusting - * X-Forwarded-For (a caller can spoof the header otherwise). When no - * trusted IP can be determined, returns '0.0.0.0' so the caller still has - * *something* stable to rate-limit against. - * - * @return string IPv4 or IPv6 address string. - */ - public function client_ip(): string { - // REMOTE_ADDR is set by the webserver and cannot be spoofed by callers. - $remote = isset( $_SERVER['REMOTE_ADDR'] ) - ? filter_var( wp_unslash( $_SERVER['REMOTE_ADDR'] ), FILTER_VALIDATE_IP ) - : false; - - return is_string( $remote ) && '' !== $remote ? $remote : '0.0.0.0'; - } - - /** - * Normalize an email for consistent per-email bucketing. - * - * @param string $email Raw email string. - * - * @return string Lowercased, trimmed email; empty string if not an email. - */ - public function normalize_email( string $email ): string { - $email = strtolower( trim( $email ) ); - return is_email( $email ) ? $email : ''; - } } diff --git a/src/WorkOS/RateLimit/RateLimiter.php b/src/WorkOS/RateLimit/RateLimiter.php index 1994b4e..2969f16 100644 --- a/src/WorkOS/RateLimit/RateLimiter.php +++ b/src/WorkOS/RateLimit/RateLimiter.php @@ -62,20 +62,4 @@ public function refund( string $bucket, string $subject, int $window ): void; * @return void */ public function reset( string $bucket, string $subject, int $window ): void; - - /** - * Best-effort client IP for the current request. - * - * @return string IPv4 or IPv6 address string. - */ - public function client_ip(): string; - - /** - * Normalize an email for consistent per-email bucketing. - * - * @param string $email Raw email string. - * - * @return string Lowercased, trimmed email; empty string if not an email. - */ - public function normalize_email( string $email ): string; } diff --git a/src/WorkOS/RateLimit/TieredRateLimiter.php b/src/WorkOS/RateLimit/TieredRateLimiter.php index b16f20c..f0fd1e4 100644 --- a/src/WorkOS/RateLimit/TieredRateLimiter.php +++ b/src/WorkOS/RateLimit/TieredRateLimiter.php @@ -128,25 +128,7 @@ public function reset( string $bucket, string $subject, array $policy ): void { } } - /** - * Best-effort client IP for the current request. - * - * @return string - */ - public function client_ip(): string { - return $this->limiter->client_ip(); - } - /** - * Normalize an email for consistent per-email bucketing. - * - * @param string $email Raw email string. - * - * @return string - */ - public function normalize_email( string $email ): string { - return $this->limiter->normalize_email( $email ); - } /** * Order a policy shortest window first. diff --git a/src/WorkOS/Sync/AuditLog.php b/src/WorkOS/Sync/AuditLog.php index 8d08b57..cea9b64 100644 --- a/src/WorkOS/Sync/AuditLog.php +++ b/src/WorkOS/Sync/AuditLog.php @@ -7,7 +7,7 @@ namespace WorkOS\Sync; -use WorkOS\Vendor\StellarWP\SuperGlobals\SuperGlobals; +use WorkOS\Http\ClientIp; defined( 'ABSPATH' ) || exit; @@ -16,10 +16,21 @@ */ class AuditLog { + /** + * Client IP resolver. + * + * @var ClientIp + */ + private ClientIp $client_ip; + /** * Constructor — register event hooks. + * + * @param ClientIp $client_ip Client IP resolver. */ - public function __construct() { + public function __construct( ClientIp $client_ip ) { + $this->client_ip = $client_ip; + // Authentication events. add_action( 'wp_login', [ $this, 'log_login' ], 10, 2 ); add_action( 'wp_logout', [ $this, 'log_logout' ] ); @@ -318,8 +329,6 @@ private function send_event_async( string $org_id, array $event ): void { * @return string */ private function get_client_ip(): string { - $ip = SuperGlobals::get_server_var( 'REMOTE_ADDR' ) ?? '0.0.0.0'; - $validated_ip = filter_var( $ip, FILTER_VALIDATE_IP ); - return $validated_ip ? $validated_ip : '0.0.0.0'; + return $this->client_ip->get(); } } diff --git a/tests/wpunit/AuthKitRestMagicSessionTest.php b/tests/wpunit/AuthKitRestMagicSessionTest.php index 5dddd26..7a7a54b 100644 --- a/tests/wpunit/AuthKitRestMagicSessionTest.php +++ b/tests/wpunit/AuthKitRestMagicSessionTest.php @@ -16,6 +16,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\REST\Auth\MagicCode; use WorkOS\REST\Auth\Session; @@ -104,8 +105,8 @@ public function setUp(): void { $rate_limiter = new TieredRateLimiter( new DatabaseRateLimiter() ); $completer = new LoginCompleter(); - $magic = new MagicCode( $this->repository, $this->nonce, $radar, $rate_limiter, $completer ); - $session = new Session( $this->repository, $this->nonce, $radar, $rate_limiter, $completer ); + $magic = new MagicCode( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); + $session = new Session( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); add_action( 'rest_api_init', [ $magic, 'register_routes' ] ); add_action( 'rest_api_init', [ $session, 'register_routes' ] ); diff --git a/tests/wpunit/AuthKitRestMfaTest.php b/tests/wpunit/AuthKitRestMfaTest.php index 44f7c97..aa5c0fa 100644 --- a/tests/wpunit/AuthKitRestMfaTest.php +++ b/tests/wpunit/AuthKitRestMfaTest.php @@ -16,6 +16,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\REST\Auth\Mfa; @@ -93,6 +94,7 @@ public function setUp(): void { $this->nonce, new Radar(), new TieredRateLimiter( new DatabaseRateLimiter() ), + new ClientIp(), new LoginCompleter() ); add_action( 'rest_api_init', [ $mfa, 'register_routes' ] ); diff --git a/tests/wpunit/AuthKitRestPasswordTest.php b/tests/wpunit/AuthKitRestPasswordTest.php index 17d03ed..0c7b3c6 100644 --- a/tests/wpunit/AuthKitRestPasswordTest.php +++ b/tests/wpunit/AuthKitRestPasswordTest.php @@ -16,6 +16,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\REST\Auth\Password; @@ -121,6 +122,7 @@ public function setUp(): void { $this->nonce, new Radar(), new TieredRateLimiter( new DatabaseRateLimiter() ), + new ClientIp(), new LoginCompleter() ); @@ -812,7 +814,7 @@ public function test_fallback_email_confirmation_rate_limits_sends_per_ip(): voi $send_ok = [ 'response' => [ 'code' => 200, 'message' => 'OK' ], 'body' => '{}' ]; $limiter = new TieredRateLimiter( new DatabaseRateLimiter() ); - $ip = $limiter->client_ip(); + $ip = ( new ClientIp() )->get(); for ( $i = 0; $i < 10; $i++ ) { // The endpoint's own 10/min IP bucket would mask the hourly one; diff --git a/tests/wpunit/AuthKitRestSignupInvitationOAuthTest.php b/tests/wpunit/AuthKitRestSignupInvitationOAuthTest.php index 41ff260..ca427e8 100644 --- a/tests/wpunit/AuthKitRestSignupInvitationOAuthTest.php +++ b/tests/wpunit/AuthKitRestSignupInvitationOAuthTest.php @@ -16,6 +16,7 @@ use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\Auth\AuthKit\Radar; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\REST\Auth\Invitation; use WorkOS\REST\Auth\OAuth; @@ -110,9 +111,9 @@ public function setUp(): void { $rate_limiter = new TieredRateLimiter( new DatabaseRateLimiter() ); $completer = new LoginCompleter(); - $signup = new Signup( $this->repository, $this->nonce, $radar, $rate_limiter, $completer ); - $invitation = new Invitation( $this->repository, $this->nonce, $radar, $rate_limiter, $completer ); - $oauth = new OAuth( $this->repository, $this->nonce, $radar, $rate_limiter, $completer ); + $signup = new Signup( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); + $invitation = new Invitation( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); + $oauth = new OAuth( $this->repository, $this->nonce, $radar, $rate_limiter, new ClientIp(), $completer ); add_action( 'rest_api_init', [ $signup, 'register_routes' ] ); add_action( 'rest_api_init', [ $invitation, 'register_routes' ] ); diff --git a/tests/wpunit/ChangeEmailRestApiTest.php b/tests/wpunit/ChangeEmailRestApiTest.php index 7384393..87da5a7 100644 --- a/tests/wpunit/ChangeEmailRestApiTest.php +++ b/tests/wpunit/ChangeEmailRestApiTest.php @@ -9,6 +9,7 @@ use lucatume\WPBrowser\TestCase\WPTestCase; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\Auth\ChangeEmail\ConflictResolver; use WorkOS\Auth\ChangeEmail\Notifier; @@ -78,6 +79,7 @@ public function setUp(): void { $rest = new RestApi( new TieredRateLimiter( new DatabaseRateLimiter() ), + new ClientIp(), $tokens, $pending, new ConflictResolver(), diff --git a/tests/wpunit/ClientIpTest.php b/tests/wpunit/ClientIpTest.php new file mode 100644 index 0000000..e6fc4e4 --- /dev/null +++ b/tests/wpunit/ClientIpTest.php @@ -0,0 +1,232 @@ +server_backup = $_SERVER; + $this->client_ip = new ClientIp(); + } + + /** + * Tear down. + */ + public function tearDown(): void { + $_SERVER = $this->server_backup; + + parent::tearDown(); + } + + /** + * A resolver for an operator who has named the header their proxy + * overwrites. + * + * @param string $header Header name. + * + * @return ClientIp + */ + private function trusting( string $header ): ClientIp { + return new ClientIp( $header ); + } + + /** + * With nothing configured, REMOTE_ADDR is the answer. + */ + public function test_uses_remote_addr_by_default(): void { + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + + $this->assertSame( '203.0.113.4', $this->client_ip->get() ); + } + + /** + * A forwarded header is ignored unless an operator names it. This is + * the spoofing case: trusting it by default would hand the bucket key + * to the caller. + * + * @param string $header Header a caller might try. + */ + #[DataProvider( 'spoofable_header_provider' )] + public function test_ignores_untrusted_headers( string $header ): void { + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + $_SERVER[ $header ] = '198.51.100.99'; + + $this->assertSame( '203.0.113.4', $this->client_ip->get() ); + } + + /** + * Once named, the header wins, because the proxy overwrites it. + */ + public function test_uses_configured_header(): void { + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + $_SERVER['HTTP_CF_CONNECTING_IP'] = '198.51.100.99'; + + $this->assertSame( '198.51.100.99', $this->trusting( 'CF-Connecting-IP' )->get() ); + } + + /** + * In a list, the right-most public entry wins. Everything left of the + * infrastructure hops is whatever the caller chose to send. + */ + public function test_reads_right_most_public_entry_from_a_list(): void { + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + $_SERVER['HTTP_X_FORWARDED_FOR'] = '5.5.5.5, 198.51.100.99'; + + $this->assertSame( '198.51.100.99', $this->trusting( 'X-Forwarded-For' )->get() ); + } + + /** + * Private hops don't shadow the client: they're skipped while walking + * leftwards. + */ + public function test_skips_private_hops_in_a_list(): void { + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + $_SERVER['HTTP_X_FORWARDED_FOR'] = '5.5.5.5, 198.51.100.99, 10.0.0.1'; + + $this->assertSame( '198.51.100.99', $this->trusting( 'X-Forwarded-For' )->get() ); + } + + /** + * An all-private list still yields the right-most valid entry, which + * is the local-proxy case in development. + */ + public function test_falls_back_to_right_most_valid_when_all_private(): void { + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + $_SERVER['HTTP_X_FORWARDED_FOR'] = '10.0.0.5, 192.168.1.7'; + + $this->assertSame( '192.168.1.7', $this->trusting( 'X-Forwarded-For' )->get() ); + } + + /** + * A configured header that isn't present falls through rather than + * failing. + */ + public function test_falls_back_to_remote_addr_when_header_absent(): void { + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + + $this->assertSame( '203.0.113.4', $this->trusting( 'CF-Connecting-IP' )->get() ); + } + + /** + * Garbage in the trusted header doesn't win either. + */ + public function test_falls_back_when_header_holds_no_valid_address(): void { + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + $_SERVER['HTTP_CF_CONNECTING_IP'] = 'not-an-ip, also-not'; + + $this->assertSame( '203.0.113.4', $this->trusting( 'CF-Connecting-IP' )->get() ); + } + + /** + * IPv6 survives round-tripping. + */ + public function test_accepts_ipv6(): void { + $_SERVER['REMOTE_ADDR'] = '2001:db8::1'; + + $this->assertSame( '2001:db8::1', $this->client_ip->get() ); + } + + /** + * Missing or unusable REMOTE_ADDR resolves to the sentinel rather than + * an empty subject, so callers still have something stable to key on. + * + * @param mixed $value REMOTE_ADDR value, or null to unset it. + */ + #[DataProvider( 'unusable_remote_addr_provider' )] + public function test_unusable_remote_addr_returns_sentinel( $value ): void { + if ( null === $value ) { + unset( $_SERVER['REMOTE_ADDR'] ); + } else { + $_SERVER['REMOTE_ADDR'] = $value; + } + + $this->assertSame( ClientIp::UNKNOWN, $this->client_ip->get() ); + } + + /** + * The container hands out one configured resolver. + * + * With no header named, the bound instance must trust nothing but + * REMOTE_ADDR, which is the safe posture for a site that never + * configures this. + */ + public function test_container_resolves_a_configured_singleton(): void { + $resolved = \WorkOS\App::container()->get( ClientIp::class ); + + $this->assertInstanceOf( ClientIp::class, $resolved ); + $this->assertSame( $resolved, \WorkOS\App::container()->get( ClientIp::class ) ); + + $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; + $_SERVER['HTTP_X_FORWARDED_FOR'] = '198.51.100.99'; + + $this->assertSame( '203.0.113.4', $resolved->get() ); + } + + // ------------------------------------------------------------------------- + // Providers + // ------------------------------------------------------------------------- + + /** + * Headers a caller can set that must not be trusted by default. + * + * @return array + */ + public function spoofable_header_provider(): array { + return [ + 'X-Forwarded-For' => [ 'HTTP_X_FORWARDED_FOR' ], + 'CF-Connecting-IP' => [ 'HTTP_CF_CONNECTING_IP' ], + 'X-Real-IP' => [ 'HTTP_X_REAL_IP' ], + 'Client-IP' => [ 'HTTP_CLIENT_IP' ], + ]; + } + + /** + * REMOTE_ADDR values that yield no usable address. + * + * @return array + */ + public function unusable_remote_addr_provider(): array { + return [ + 'unset' => [ null ], + 'empty' => [ '' ], + 'garbage' => [ 'not-an-ip' ], + 'list' => [ '203.0.113.4, 198.51.100.99' ], + ]; + } +} diff --git a/tests/wpunit/PasswordResetAdminRestApiTest.php b/tests/wpunit/PasswordResetAdminRestApiTest.php index 497586b..c082e24 100644 --- a/tests/wpunit/PasswordResetAdminRestApiTest.php +++ b/tests/wpunit/PasswordResetAdminRestApiTest.php @@ -11,6 +11,7 @@ use WorkOS\Auth\AuthKit\Profile; use WorkOS\Auth\AuthKit\ProfileRepository; use WorkOS\RateLimit\DatabaseRateLimiter; +use WorkOS\Http\ClientIp; use WorkOS\RateLimit\TieredRateLimiter; use WorkOS\Auth\PasswordResetAdmin\RedirectValidator; use WorkOS\Auth\PasswordResetAdmin\RestApi; @@ -118,6 +119,7 @@ public function setUp(): void { $rest_api = new RestApi( $this->repository, new TieredRateLimiter( new DatabaseRateLimiter() ), + new ClientIp(), new RedirectValidator(), new AddressMask() ); diff --git a/tests/wpunit/RateLimiterTest.php b/tests/wpunit/RateLimiterTest.php index ee05d62..db17545 100644 --- a/tests/wpunit/RateLimiterTest.php +++ b/tests/wpunit/RateLimiterTest.php @@ -169,58 +169,6 @@ public function test_non_positive_limit_or_window_bypasses_check( string $class, } } - /** - * Email normalization lowercases and trims; non-emails return empty. - * - * @param string $class Limiter implementation. - */ - #[DataProvider( 'limiter_provider' )] - public function test_normalize_email( string $class ): void { - $limiter = $this->limiter( $class ); - - $this->assertSame( 'alice@example.com', $limiter->normalize_email( ' Alice@Example.COM ' ) ); - $this->assertSame( '', $limiter->normalize_email( 'not-an-email' ) ); - $this->assertSame( '', $limiter->normalize_email( '' ) ); - } - - /** - * client_ip falls back to 0.0.0.0 when REMOTE_ADDR is unset. - * - * @param string $class Limiter implementation. - */ - #[DataProvider( 'limiter_provider' )] - public function test_client_ip_returns_fallback_when_unset( string $class ): void { - $limiter = $this->limiter( $class ); - $previous = $_SERVER['REMOTE_ADDR'] ?? null; - unset( $_SERVER['REMOTE_ADDR'] ); - - $this->assertSame( '0.0.0.0', $limiter->client_ip() ); - - if ( null !== $previous ) { - $_SERVER['REMOTE_ADDR'] = $previous; - } - } - - /** - * client_ip uses REMOTE_ADDR when set to a valid IP. - * - * @param string $class Limiter implementation. - */ - #[DataProvider( 'limiter_provider' )] - public function test_client_ip_uses_remote_addr( string $class ): void { - $limiter = $this->limiter( $class ); - $previous = $_SERVER['REMOTE_ADDR'] ?? null; - $_SERVER['REMOTE_ADDR'] = '203.0.113.4'; - - $this->assertSame( '203.0.113.4', $limiter->client_ip() ); - - if ( null === $previous ) { - unset( $_SERVER['REMOTE_ADDR'] ); - } else { - $_SERVER['REMOTE_ADDR'] = $previous; - } - } - // ------------------------------------------------------------------------- // Providers // ------------------------------------------------------------------------- diff --git a/tests/wpunit/TieredRateLimiterTest.php b/tests/wpunit/TieredRateLimiterTest.php index ee8cf4f..cc6b59b 100644 --- a/tests/wpunit/TieredRateLimiterTest.php +++ b/tests/wpunit/TieredRateLimiterTest.php @@ -81,13 +81,6 @@ public function reset( string $bucket, string $subject, int $window ): void { unset( $this->counts[ $bucket . '|' . $subject . '|' . $window ] ); } - public function client_ip(): string { - return '203.0.113.7'; - } - - public function normalize_email( string $email ): string { - return strtolower( trim( $email ) ); - } }; $this->limiter = new TieredRateLimiter( $this->fake ); @@ -203,14 +196,6 @@ public function test_policy_with_no_usable_tiers_allows_everything(): void { $this->assertSame( [], $this->fake->windows_seen ); } - /** - * Request helpers pass straight through to the wrapped limiter. - */ - public function test_helpers_delegate(): void { - $this->assertSame( '203.0.113.7', $this->limiter->client_ip() ); - $this->assertSame( 'alice@example.com', $this->limiter->normalize_email( ' Alice@Example.COM ' ) ); - } - /** * The shipped policies, so a retune can't silently invert the tiers. * From d7e10a36a629b96c4bf0a43b55dc5df2479688a2 Mon Sep 17 00:00:00 2001 From: John Hooks Date: Tue, 4 Aug 2026 15:16:50 -0700 Subject: [PATCH 2/2] ci: run checks on stacked pull requests The pull_request trigger filtered on branches, which matches the base of a PR rather than the head. A branch opened against another branch instead of main therefore matched nothing and ran no checks at all: no PHPCS, no PHPStan, no tests, no JS build. Stacked work was the least verified when it needed review the most. Pushes still only build main directly; branches with a PR open are covered by the pull_request event. --- .github/workflows/test.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3d7fef4..ed19c99 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -3,8 +3,10 @@ name: Tests on: push: branches: [ main ] + # No base-branch filter: `pull_request.branches` matches the *base*, so + # limiting it to main skips every check on a stacked PR, which is when + # review needs them most. pull_request: - branches: [ main ] env: SLIC_BIN: ${{ github.workspace }}/slic/slic