Skip to content

fix(http): stop trusting spoofable IP headers - #40

Open
johnhooks wants to merge 2 commits into
refactor/rate-limiter-2from
feature/client-ip
Open

fix(http): stop trusting spoofable IP headers#40
johnhooks wants to merge 2 commits into
refactor/rate-limiter-2from
feature/client-ip

Conversation

@johnhooks

@johnhooks johnhooks commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Branch: feature/client-iprefactor/rate-limiter-2. Stacked on #39; review that one first.

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, so a rate limit, a log line, and an audit record could each name a different caller for the same request.

Changes

  • Client IP resolution has one owner, shared by rate limiting, the activity log, and the audit log. The default is the address the webserver saw, which a caller cannot forge.
  • A proxy header is used only when an operator names it through WORKOS_CLIENT_IP_HEADER (constant or environment variable). Behind a CDN that is the only way to see past the edge, and requiring it to be named is a deliberate assertion that the origin accepts nothing but proxied traffic. An origin reachable directly makes the header attacker-controlled again, which is why it is never auto-detected.
  • When the named header carries a list, the right-most public entry wins, since entries to the right were written by infrastructure. Private and reserved hops are skipped so an internal address can't shadow the client.
  • The rate limiter drops client_ip() and normalize_email(). Neither is about counting, and the second had no callers left.

Why it matters

Per-IP limits now count real callers rather than a value the caller picks: previously anyone could mint a fresh bucket per request by changing a header, which made every per-IP limit in the plugin advisory. The activity log becomes usable as evidence rather than a record of what a caller asserted.

Behavior changes to note

  • Sites already behind a CDN will see per-IP limits begin to bite per visitor rather than collapsing every visitor into the edge's address, but only once WORKOS_CLIENT_IP_HEADER is set. Until it is set, those sites keep counting the edge, which is the safe direction.
  • Activity log rows recorded before this change may carry caller-supplied addresses and should not be trusted retroactively.

Details in the "Who the caller is" section of docs/rate-limiting.md.

Not included: LoginBypass still reads the connecting address directly. It matches the new default so there is no defect, but behind a CDN an IP allowlist would be comparing against the edge, which is worth a follow-up.

@johnhooks
johnhooks marked this pull request as ready for review August 4, 2026 20:57
@johnhooks
johnhooks requested review from bordoni and redscar August 4, 2026 20:57
Comment thread src/WorkOS/ActivityLog/EventLogger.php Outdated
Comment thread src/WorkOS/Http/ClientIp.php Outdated
# limiting it to main skips every check on a stacked PR, which is when
# review needs them most.
pull_request:
branches: [ main ]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cuz I like a good stack

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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants