Page MenuHomePhabricator

Support X-Forwarded-For in Access Log
Closed, SpitePublic

Description

Our Phabricator instance is behind a load balancer making the client IP mostly useless. Phabricator should show the contents of the X-Forwarded-For header for the %r log output (or a new format string should be created)

Event Timeline

Doing this blindly would let clients spoof remote addresses on installs that are not behind load balancers. The current recommendation is to replace REMOTE_ADDR in preamble.php if you trust the connecting device. Here's what we do in the Phacility cluster (although we're in SiteConfig here, not preamble):

// If this request is coming from within the cluster, we'll trust HTTP
// headers added by the load balancer. We do not trust these headers for
// requests which do not originate from within the cluster, because they
// are client-controlled if the request is being received directly.
if (PhacilityServices::isClusterRequest()) {
  // Since we terminate SSL at the ELB, requests are normally HTTP by the
  // time they reach the server, even if they were originally HTTPS. The
  // ELB sets this header to indicate that the original request was HTTPS.
  if (idx($_SERVER, 'HTTP_X_FORWARDED_PROTO') == 'https') {
    $_SERVER['HTTPS'] = true;
  }

  $forwarded_for = idx($_SERVER, 'HTTP_X_FORWARDED_FOR');
  if ($forwarded_for) {
    // This may be a list of IPs, like "1.2.3.4, 4.5.6.7", if the
    // request the load balancer received also had this header. In
    // particular, this happens routinely with requests received through
    // the CloudFront CDN.

    // We only care about or trust the last IP in the list: the others are
    // controlled by the client.
    $forwarded_for = explode(',', $forwarded_for);
    $forwarded_for = last($forwarded_for);
    $forwarded_for = trim($forwarded_for);
    $_SERVER['REMOTE_ADDR'] = $forwarded_for;
  }

A minimal preamble might be:

phabricator/support/preamble.php
<?php

$_SERVER['REMOTE_ADDR'] = $_SERVER['HTTP_X_FORWARDED_FOR'];

But you may need more logic than that if a connection may possibly be forwarded multiple times, the LB handles terminating SSL, etc.

eadler claimed this task.

In our instance the LB handles SSL but we already account for that in preamble.php

Closed since its better offer handled elsewhere.