fix(http): stop trusting spoofable IP headers

.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

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.

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;
 	}
 }

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,13 +121,15 @@ class RestApi {
 	 */
 	public function __construct(
 		TieredRateLimiter $rate_limiter,
+		ClientIp $client_ip,
 		TokenFactory $tokens,
 		PendingChange $pending,
 		ConflictResolver $conflicts,
 		Notifier $notifier,
 		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 ) ) {

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.

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.
 	 *

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 );