Skip to content

Strip and re-author forwarding headers in Falcon::Middleware::Proxy#351

Open
benni-rogge wants to merge 1 commit into
socketry:mainfrom
benni-rogge:strip-forwarding-headers
Open

Strip and re-author forwarding headers in Falcon::Middleware::Proxy#351
benni-rogge wants to merge 1 commit into
socketry:mainfrom
benni-rogge:strip-forwarding-headers

Conversation

@benni-rogge
Copy link
Copy Markdown

@benni-rogge benni-rogge commented Jun 3, 2026

Summary

As the trust boundary, Falcon must not trust client-supplied forwarding headers. Previously Falcon::Middleware::Proxy#prepare_request appended to whatever Forwarded / X-Forwarded-* headers arrived on the inbound request, so a client could pre-seed those headers and have the spoofed values reach the upstream application (CWE-290 / CWE-348 — authentication/decision based on a spoofable, untrusted value).

This change makes prepare_request:

  1. Strip every inbound forwarding header (forwarded, x-forwarded-for, x-forwarded-proto, x-forwarded-host, x-forwarded-port) so none can be spoofed.
  2. Re-author them from connection-level facts: the modern RFC 7239 Forwarded header and the legacy X-Forwarded-For / X-Forwarded-Proto headers.

Why keep the legacy X-Forwarded-* headers?

X-Forwarded-For is still consumed widely downstream, so dropping it would break real consumers:

  • Rack's Rack::Request#forwarded_for (used by Rails' ActionDispatch::RemoteIp) defaults to forwarded_priority = [:forwarded, :x_forwarded] — it prefers RFC 7239 Forwarded but falls back to X-Forwarded-For.
  • Older Rack (< 3, which predates Forwarded support) and a lot of application code read X-Forwarded-For directly.

So we emit both header families. This is documented inline on the FORWARDING_HEADERS constant.

RFC 7239 correctness

Forwarded node identifiers for IPv6 are now correctly bracketed and quoted (for="[::1]"), via the new forwarded_node helper. The legacy X-Forwarded-For continues to carry the bare address (::1), per its conventions.

Tests

test/falcon/middleware/proxy.rb — 7 passing examples covering:

  • inbound spoofed headers (forwarded, x-forwarded-for, x-forwarded-proto, x-forwarded-host, x-forwarded-port) are stripped and replaced with Falcon's own;
  • both modern and legacy headers are authored from the connection;
  • IPv6 formatting (bracketed+quoted in Forwarded, bare in X-Forwarded-For);
  • the remote_address-unavailable branch emits neither for= nor X-Forwarded-For, but still authors proto and via.

CI note

The remaining Test Coverage / validate failure is inherited from main, not introduced by this PR:

  • main (e7007f6): 45 files checked; 755/882 lines executed; 85.6% covered.
  • this PR (6d54a53): 45 files checked; 761/888 lines executed; 85.7% covered.

A separate baseline CI fix is open as #352. Its exact commit passes the fork workflow, and a temporary fork branch combining #352 + this PR also passes Test Coverage / validate with this PR's 85.7% coverage.

🤖 Generated with Claude Code

As the trust boundary, Falcon must not trust client-supplied forwarding
headers. `prepare_request` now discards every inbound `Forwarded` /
`X-Forwarded-*` header and authors its own from connection-level facts,
preventing a client from spoofing the forwarded address or scheme
(CWE-290 / CWE-348).

We emit both the modern RFC 7239 `Forwarded` header (with correctly
bracketed and quoted IPv6) and the legacy `X-Forwarded-For` /
`X-Forwarded-Proto` headers. The legacy headers are retained because
they are still consumed downstream -- notably Rack's
`Rack::Request#forwarded_for` (used by Rails' `ActionDispatch::RemoteIp`)
falls back to `X-Forwarded-For`, and older Rack (< 3) and direct readers
depend on it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Benni Rogge <benni.rogge@shopify.com>
@benni-rogge benni-rogge force-pushed the strip-forwarding-headers branch from b77bae7 to 6d54a53 Compare June 3, 2026 22:02
@benni-rogge benni-rogge marked this pull request as ready for review June 3, 2026 22:04
@benni-rogge benni-rogge force-pushed the strip-forwarding-headers branch 3 times, most recently from d98bae2 to 6d54a53 Compare June 4, 2026 00:05
@benni-rogge
Copy link
Copy Markdown
Author

benni-rogge commented Jun 4, 2026

I investigated the remaining Test Coverage / validate failure. It doesn't appear to be introduced by this PR:

  • Current main coverage run (e7007f6): 45 files checked; 755/882 lines executed; 85.6% covered. (run)
  • This PR's coverage run (6d54a53): 45 files checked; 761/888 lines executed; 85.7% covered. (run)

covered:validate requires 100% by default, so the coverage validation job is already failing on main. This PR slightly increases executed coverage and the new proxy lines are covered; all non-coverage checks are green.

@benni-rogge
Copy link
Copy Markdown
Author

benni-rogge commented Jun 4, 2026

Opened #352 as a separate baseline CI fix so this PR can stay focused on forwarding headers: #352

Validation evidence:

The upstream #352 PR workflows are still waiting for maintainer approval because it is a fork PR.

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.

1 participant