Add support for multiple IP Addresses found in X-Forwarded-For request header

Created on 3 January 2019, over 5 years ago
Updated 19 May 2023, over 1 year ago

Problem/Motivation

When using VPN to access a site in combination with CloudFlare DNS service, CloudFlare (https://support.cloudflare.com/hc/en-us/articles/200170986-How-does-Clou...)

Second, if there was an "X-Forwarded-For" header present in the request sent to Cloudflare, Cloudflare appends the IP address of the HTTP proxy to its value, as the last in the list.

"X-Forwarded-For: A.B.C.D[,X.X.X.X,Y.Y.Y.Y,]"

The restrict_ip use of $this->currentUserIp = $requestStack->getCurrentRequest()->getClientIp(); returns the requests originating IP Address, not the VPN IP Address. If attempting to whitelist the VPN IP Address, users accessing from home or other remote locations a restricted b/c restrict_ip, specifically Symfony\Component\HttpFoundation\Request::getClientIp(), returns a single originating IP Address.

Proposed resolution

Add support for multiple IP Addresses found in X-Forwarded-For request header.

Do we want to this to be an additional configurable feature or default behavior.

  • Update currentUserIp to currentUserIps by using Symfony\Component\HttpFoundation\Request::getClientIps()
  • Update comparison logic everyone b/c currentUserIps is an array

Remaining tasks

TBD

User interface changes

TBD

API changes

TBD

Data model changes

TBD

Release notes snippet

TBD

✨ Feature request
Status

Needs work

Version

4.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States jasonawant New Orleans, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Status changed to Needs review over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States byrond

    This should likely not be merged without making this behavior optional (and adding a warning about enabling it). From the getClientIps() method documentation:

         * In the returned array the most trusted IP address is first, and the
         * least trusted one last. The "real" client IP address is the last one,
         * but this is also the least trusted one. Trusted proxies are stripped.
         *
         * Use this method carefully; you should use getClientIp() instead.
    

    https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/HttpFo...

    We considered using this patch for a client using Akamai but aren't comfortable with the security risk associated with trusting all addresses in X-Forwarded-For.

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States byrond

    Marking this as "needs work" based on my concern above. An more secure approach is to add trusted proxy addresses to settings.php using $settings['reverse_proxy_addresses']. When these are set, Drupal will strip those addresses from X-Forwarded-For, leaving the client's real IP as the only one in the header. Drupal will trust that IP as long as the request was received from a trusted proxy. This does mean that list must be maintained as the CDN changes those addresses.

    The Restrict by IP module allows you to configure the header that contains the client's real IP. This is often configured on the CDN (sometimes called "True-Client-IP") and can be trusted by Drupal when that is the case. We are working on a patch for this module to add the same functionality and will post it in a separate issue.

Production build 0.71.5 2024