http: remove xftrust options

This has long been considered a mistake and not documented for very
long.

I considered removing X-Forwarded-Proto and X-Forwarded-SSL
handling, too, so rack.url_scheme is always "http", but that might
lead to compatibility issues in rare apps if Rack::Request#scheme is
not used.

6 files changed, 20 insertions, 169 deletions

diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
index 6293033..ecdadb0 100644
--- a/ext/unicorn_http/unicorn_http.rl
+++ b/ext/unicorn_http/unicorn_http.rl
@@ -29,12 +29,6 @@ void init_unicorn_httpdate(void);
 /* all of these flags need to be set for keepalive to be supported */
 #define UH_FL_KEEPALIVE (UH_FL_KAVERSION | UH_FL_REQEOF | UH_FL_HASHEADER)
 
-/*
- * whether or not to trust X-Forwarded-Proto and X-Forwarded-SSL when
- * setting rack.url_scheme
- */
-static VALUE trust_x_forward = Qtrue;
-
 static unsigned long keepalive_requests = 100; /* same as nginx */
 
 /*
@@ -58,31 +52,6 @@ static VALUE set_ka_req(VALUE self, VALUE val)
   return ka_req(self);
 }
 
-/*
- * Sets whether or not the parser will trust X-Forwarded-Proto and
- * X-Forwarded-SSL headers and set "rack.url_scheme" to "https" accordingly.
- * Rainbows!/Zbatery installations facing untrusted clients directly
- * should set this to +false+
- */
-static VALUE set_xftrust(VALUE self, VALUE val)
-{
-  if (Qtrue == val || Qfalse == val)
-    trust_x_forward = val;
-  else
-    rb_raise(rb_eTypeError, "must be true or false");
-
-  return val;
-}
-
-/*
- * returns whether or not the parser will trust X-Forwarded-Proto and
- * X-Forwarded-SSL headers and set "rack.url_scheme" to "https" accordingly
- */
-static VALUE xftrust(VALUE self)
-{
-  return trust_x_forward;
-}
-
 static size_t MAX_HEADER_LEN = 1024 * (80 + 32); /* same as Mongrel */
 
 /* this is only intended for use with Rainbows! */
@@ -491,26 +460,29 @@ static void set_url_scheme(VALUE env, VALUE *server_port)
   VALUE scheme = rb_hash_aref(env, g_rack_url_scheme);
 
   if (NIL_P(scheme)) {
-    if (trust_x_forward == Qfalse) {
-      scheme = g_http;
+    /*
+     * would anybody be horribly opposed to removing the X-Forwarded-SSL
+     * and X-Forwarded-Proto handling from this parser?  We've had it
+     * forever and nobody has said anything against it, either.
+     * Anyways, please send comments to our public mailing list:
+     * unicorn-public@bogomips.org (no HTML mail, no subscription necessary)
+     */
+    scheme = rb_hash_aref(env, g_http_x_forwarded_ssl);
+    if (!NIL_P(scheme) && STR_CSTR_EQ(scheme, "on")) {
+      *server_port = g_port_443;
+      scheme = g_https;
     } else {
-      scheme = rb_hash_aref(env, g_http_x_forwarded_ssl);
-      if (!NIL_P(scheme) && STR_CSTR_EQ(scheme, "on")) {
-        *server_port = g_port_443;
-        scheme = g_https;
+      scheme = rb_hash_aref(env, g_http_x_forwarded_proto);
+      if (NIL_P(scheme)) {
+        scheme = g_http;
       } else {
-        scheme = rb_hash_aref(env, g_http_x_forwarded_proto);
-        if (NIL_P(scheme)) {
-          scheme = g_http;
+        long len = RSTRING_LEN(scheme);
+        if (len >= 5 && !memcmp(RSTRING_PTR(scheme), "https", 5)) {
+          if (len != 5)
+            scheme = g_https;
+          *server_port = g_port_443;
         } else {
-          long len = RSTRING_LEN(scheme);
-          if (len >= 5 && !memcmp(RSTRING_PTR(scheme), "https", 5)) {
-            if (len != 5)
-              scheme = g_https;
-            *server_port = g_port_443;
-          } else {
-            scheme = g_http;
-          }
+          scheme = g_http;
         }
       }
     }
@@ -1018,8 +990,6 @@ void Init_unicorn_http(void)
 
   rb_define_singleton_method(cHttpParser, "keepalive_requests", ka_req, 0);
   rb_define_singleton_method(cHttpParser, "keepalive_requests=", set_ka_req, 1);
-  rb_define_singleton_method(cHttpParser, "trust_x_forwarded=", set_xftrust, 1);
-  rb_define_singleton_method(cHttpParser, "trust_x_forwarded?", xftrust, 0);
   rb_define_singleton_method(cHttpParser, "max_header_len=", set_maxhdrlen, 1);
 
   init_common_fields();
diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
index 9406223..5962418 100644
--- a/lib/unicorn/configurator.rb
+++ b/lib/unicorn/configurator.rb
@@ -48,7 +48,6 @@ class Unicorn::Configurator
     :check_client_connection => false,
     :rewindable_input => true, # for Rack 2.x: (Rack::VERSION[0] <= 1),
     :client_body_buffer_size => Unicorn::Const::MAX_BODY,
-    :trust_x_forwarded => true,
   }
   #:startdoc:
 
@@ -558,18 +557,6 @@ class Unicorn::Configurator
     set[:user] = [ user, group ]
   end
 
-  # Sets whether or not the parser will trust X-Forwarded-Proto and
-  # X-Forwarded-SSL headers and set "rack.url_scheme" to "https" accordingly.
-  # Rainbows!/Zbatery installations facing untrusted clients directly
-  # should set this to +false+.  This is +true+ by default as Unicorn
-  # is designed to only sit behind trusted nginx proxies.
-  #
-  # This has never been publically documented and is subject to removal
-  # in future releases.
-  def trust_x_forwarded(bool) # :nodoc:
-    set_bool(:trust_x_forwarded, bool)
-  end
-
   # expands "unix:path/to/foo" to a socket relative to the current path
   # expands pathnames of sockets if relative to "~" or "~username"
   # expands "*:port and ":port" to "0.0.0.0:port"
diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index a0ca302..819a0a8 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -369,14 +369,6 @@ class Unicorn::HttpServer
     Unicorn::TeeInput.client_body_buffer_size = bytes
   end
 
-  def trust_x_forwarded
-    Unicorn::HttpParser.trust_x_forwarded?
-  end
-
-  def trust_x_forwarded=(bool)
-    Unicorn::HttpParser.trust_x_forwarded = bool
-  end
-
   def check_client_connection
     Unicorn::HttpRequest.check_client_connection
   end
diff --git a/t/t0016-trust-x-forwarded-false.sh b/t/t0016-trust-x-forwarded-false.sh
deleted file mode 100755
index 3163690..0000000
--- a/t/t0016-trust-x-forwarded-false.sh
+++ /dev/null
@@ -1,30 +0,0 @@
-#!/bin/sh
-. ./test-lib.sh
-t_plan 5 "trust_x_forwarded=false configuration test"
-
-t_begin "setup and start" && {
-        unicorn_setup
-        echo "trust_x_forwarded false" >> $unicorn_config
-        unicorn -D -c $unicorn_config env.ru
-        unicorn_wait_start
-}
-
-t_begin "spoofed request with X-Forwarded-Proto does not trigger" && {
-        curl -H 'X-Forwarded-Proto: https' http://$listen/ | \
-                grep -F '"rack.url_scheme"=>"http"'
-}
-
-t_begin "spoofed request with X-Forwarded-SSL does not trigger" && {
-        curl -H 'X-Forwarded-SSL: on' http://$listen/ | \
-                grep -F '"rack.url_scheme"=>"http"'
-}
-
-t_begin "killing succeeds" && {
-        kill $unicorn_pid
-}
-
-t_begin "check stderr has no errors" && {
-        check_stderr
-}
-
-t_done
diff --git a/t/t0017-trust-x-forwarded-true.sh b/t/t0017-trust-x-forwarded-true.sh
deleted file mode 100755
index 11103c5..0000000
--- a/t/t0017-trust-x-forwarded-true.sh
+++ /dev/null
@@ -1,30 +0,0 @@
-#!/bin/sh
-. ./test-lib.sh
-t_plan 5 "trust_x_forwarded=true configuration test"
-
-t_begin "setup and start" && {
-        unicorn_setup
-        echo "trust_x_forwarded true " >> $unicorn_config
-        unicorn -D -c $unicorn_config env.ru
-        unicorn_wait_start
-}
-
-t_begin "spoofed request with X-Forwarded-Proto sets 'https'" && {
-        curl -H 'X-Forwarded-Proto: https' http://$listen/ | \
-                grep -F '"rack.url_scheme"=>"https"'
-}
-
-t_begin "spoofed request with X-Forwarded-SSL sets 'https'" && {
-        curl -H 'X-Forwarded-SSL: on' http://$listen/ | \
-                grep -F '"rack.url_scheme"=>"https"'
-}
-
-t_begin "killing succeeds" && {
-        kill $unicorn_pid
-}
-
-t_begin "check stderr has no errors" && {
-        check_stderr
-}
-
-t_done
diff --git a/test/unit/test_http_parser_xftrust.rb b/test/unit/test_http_parser_xftrust.rb
deleted file mode 100644
index db8cfa9..0000000
--- a/test/unit/test_http_parser_xftrust.rb
+++ /dev/null
@@ -1,38 +0,0 @@
-# -*- encoding: binary -*-
-require 'test/test_helper'
-
-include Unicorn
-
-class HttpParserXFTrustTest < Test::Unit::TestCase
-  def setup
-    assert HttpParser.trust_x_forwarded?
-  end
-
-  def test_xf_trust_false_xfp
-    HttpParser.trust_x_forwarded = false
-    parser = HttpParser.new
-    parser.buf << "GET / HTTP/1.1\r\nHost: foo:\r\n" \
-                  "X-Forwarded-Proto: https\r\n\r\n"
-    env = parser.parse
-    assert_kind_of Hash, env
-    assert_equal 'foo', env['SERVER_NAME']
-    assert_equal '80', env['SERVER_PORT']
-    assert_equal 'http', env['rack.url_scheme']
-  end
-
-  def test_xf_trust_false_xfs
-    HttpParser.trust_x_forwarded = false
-    parser = HttpParser.new
-    parser.buf << "GET / HTTP/1.1\r\nHost: foo:\r\n" \
-                  "X-Forwarded-SSL: on\r\n\r\n"
-    env = parser.parse
-    assert_kind_of Hash, env
-    assert_equal 'foo', env['SERVER_NAME']
-    assert_equal '80', env['SERVER_PORT']
-    assert_equal 'http', env['rack.url_scheme']
-  end
-
-  def teardown
-    HttpParser.trust_x_forwarded = true
-  end
-end