From 31bb1bcc23822950e5de580a76e3182c4d96d3ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A1szl=C3=B3=20K=C3=A1rolyi?= Date: Thu, 10 May 2018 17:22:27 +0200 Subject: [PATCH 1/4] Fixing non-bytes headers, adding proxyhost+proxyport custom headers The headers on my environment aren't bytes, rather str-s, and so getting the host and port from those will result None being passed as a result. Also, since X-Forwarded-For is not to be trusted, and custom nginx configurations can pass a `X-Real-IP` header, add two extra command line parameters to be able to parse custom passed remote IP headers. --- daphne/cli.py | 60 ++++++++++++++++++++++++++++++++++++++++++--- daphne/utils.py | 13 +++++++--- tests/test_cli.py | 59 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_utils.py | 14 +++++++++-- 4 files changed, 137 insertions(+), 9 deletions(-) diff --git a/daphne/cli.py b/daphne/cli.py index 7361914..f972a41 100755 --- a/daphne/cli.py +++ b/daphne/cli.py @@ -2,6 +2,9 @@ import argparse import logging import sys +from argparse import Namespace, ArgumentError +from typing import Union + from .access import AccessLogGenerator from .endpoints import build_endpoint_description_strings from .server import Server @@ -11,7 +14,7 @@ logger = logging.getLogger(__name__) DEFAULT_HOST = "127.0.0.1" DEFAULT_PORT = 8000 - +str_or_none = Union[None, str] class CommandLineInterface(object): """ @@ -132,6 +135,25 @@ class CommandLineInterface(object): default=False, action="store_true", ) + self._arg_proxy_host = self.parser.add_argument( + '--proxy-headers-host', + dest='proxy_headers_host', + help='Specify which header will be used for getting the host ' + 'part. Can be omitted, requires --proxy-headers to be specified ' + 'when passed. \'X-Real-IP\' (when passed by your webserver) is a ' + 'good candidate for this.', + default=False, + action='store', + ) + self._arg_proxy_port = self.parser.add_argument( + '--proxy-headers-port', + dest='proxy_headers_port', + help='Specify which header will be used for getting the port ' + 'part. Can be omitted, requires --proxy-headers to be specified ' + 'when passed.', + default=False, + action='store', + ) self.parser.add_argument( "application", help="The application to dispatch to as path.to.module:instance.path", @@ -146,6 +168,38 @@ class CommandLineInterface(object): """ cls().run(sys.argv[1:]) + def _check_proxy_headers_passed(self, argument: str, args: Namespace): + """Raise if the `--proxy-headers` weren't specified.""" + if args.proxy_headers: + return + raise ArgumentError( + argument=argument, + message='--proxy-headers has to be passed for this parameter.') + + def _get_forwarded_host(self, args: Namespace) -> str_or_none: + """ + Return the default host header from which the remote hostname/ip + will be extracted. + """ + if args.proxy_headers_host: + self._check_proxy_headers_passed( + argument=self._arg_proxy_host, args=args) + return args.proxy_headers_host + if args.proxy_headers: + return 'X-Forwarded-For' + + def _get_forwarded_port(self, args: Namespace) -> str_or_none: + """ + Return the default host header from which the remote hostname/ip + will be extracted. + """ + if args.proxy_headers_port: + self._check_proxy_headers_passed( + argument=self._arg_proxy_port, args=args) + return args.proxy_headers_port + if args.proxy_headers: + return 'X-Forwarded-Port' + def run(self, args): """ Pass in raw argument list and it will decode them @@ -212,7 +266,7 @@ class CommandLineInterface(object): ws_protocols=args.ws_protocols, root_path=args.root_path, verbosity=args.verbosity, - proxy_forwarded_address_header="X-Forwarded-For" if args.proxy_headers else None, - proxy_forwarded_port_header="X-Forwarded-Port" if args.proxy_headers else None, + proxy_forwarded_address_header=self._get_forwarded_host(args=args), + proxy_forwarded_port_header=self._get_forwarded_port(args=args), ) self.server.run() diff --git a/daphne/utils.py b/daphne/utils.py index b6f0fbb..1c5b3db 100644 --- a/daphne/utils.py +++ b/daphne/utils.py @@ -15,11 +15,11 @@ def import_by_path(path): return target -def header_value(headers, header_name): +def header_value(headers, header_name) -> str: value = headers[header_name] if isinstance(value, list): value = value[0] - return value.decode("utf-8") + return value.decode("utf-8") if type(value) is bytes else value def parse_x_forwarded_for(headers, @@ -43,9 +43,14 @@ def parse_x_forwarded_for(headers, headers = dict(headers.getAllRawHeaders()) # Lowercase all header names in the dict - headers = {name.lower(): values for name, values in headers.items()} + new_headers = dict() + for name, values in headers.items(): + name = name.lower() + name = name if type(name) is bytes else name.encode('utf-8') + new_headers[name] = values + headers = new_headers - address_header_name = address_header_name.lower().encode("utf-8") + address_header_name = address_header_name.lower().encode('utf-8') result = original if address_header_name in headers: address_value = header_value(headers, address_header_name) diff --git a/tests/test_cli.py b/tests/test_cli.py index 9e90ab0..61e468d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -2,6 +2,7 @@ import logging from unittest import TestCase +from argparse import ArgumentError from daphne.cli import CommandLineInterface from daphne.endpoints import build_endpoint_description_strings as build @@ -235,3 +236,61 @@ class TestCLIInterface(TestCase): ], }, ) + + def test_default_proxyheaders(self): + """ + Passing `--proxy-headers` without a parameter will use the + `X-Forwarded-For` header. + """ + self.assertCLI( + ['--proxy-headers'], + { + 'proxy_forwarded_address_header': 'X-Forwarded-For', + }, + ) + + def test_custom_proxyhost(self): + """ + Passing `--proxy-headers-host` will set the used host header to + the passed one, and `--proxy-headers` is mandatory. + """ + self.assertCLI( + ['--proxy-headers', '--proxy-headers-host', 'blah'], + { + 'proxy_forwarded_address_header': 'blah', + }, + ) + with self.assertRaises(expected_exception=ArgumentError) as exc: + self.assertCLI( + ['--proxy-headers-host', 'blah'], + { + 'proxy_forwarded_address_header': 'blah', + }, + ) + self.assertEqual(exc.exception.argument_name, '--proxy-headers-host') + self.assertEqual( + exc.exception.message, + '--proxy-headers has to be passed for this parameter.') + + def test_custom_proxyport(self): + """ + Passing `--proxy-headers-port` will set the used port header to + the passed one, and `--proxy-headers` is mandatory. + """ + self.assertCLI( + ['--proxy-headers', '--proxy-headers-port', 'blah2'], + { + 'proxy_forwarded_port_header': 'blah2', + }, + ) + with self.assertRaises(expected_exception=ArgumentError) as exc: + self.assertCLI( + ['--proxy-headers-port', 'blah2'], + { + 'proxy_forwarded_address_header': 'blah2', + }, + ) + self.assertEqual(exc.exception.argument_name, '--proxy-headers-port') + self.assertEqual( + exc.exception.message, + '--proxy-headers has to be passed for this parameter.') diff --git a/tests/test_utils.py b/tests/test_utils.py index 786b8c9..18cdd92 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -30,7 +30,7 @@ class TestXForwardedForHttpParsing(TestCase): ["10.1.2.3", 0] ) - def test_v6_address(self): + def test_v6_address_1(self): headers = Headers({ b"X-Forwarded-For": [b"1043::a321:0001, 10.0.5.6"], }) @@ -84,7 +84,17 @@ class TestXForwardedForWsParsing(TestCase): ["10.1.2.3", 0] ) - def test_v6_address(self): + def test_non_bytes_header(self): + """The passed headers can be non-bytes too.""" + headers = { + "X-Forwarded-For": "10.1.2.3", + } + self.assertEqual( + parse_x_forwarded_for(headers), + ["10.1.2.3", 0] + ) + + def test_v6_address_2(self): headers = { b"X-Forwarded-For": [b"1043::a321:0001, 10.0.5.6"], } From 4adcf9080e31d0badce10ece65800e51ddb0169f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A1szl=C3=B3=20K=C3=A1rolyi?= Date: Thu, 10 May 2018 17:32:01 +0200 Subject: [PATCH 2/4] Fixing for quote-nazism --- daphne/cli.py | 32 ++++++++++++++++---------------- daphne/utils.py | 4 ++-- tests/test_cli.py | 28 ++++++++++++++-------------- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/daphne/cli.py b/daphne/cli.py index f972a41..31cfe2d 100755 --- a/daphne/cli.py +++ b/daphne/cli.py @@ -136,23 +136,23 @@ class CommandLineInterface(object): action="store_true", ) self._arg_proxy_host = self.parser.add_argument( - '--proxy-headers-host', - dest='proxy_headers_host', - help='Specify which header will be used for getting the host ' - 'part. Can be omitted, requires --proxy-headers to be specified ' - 'when passed. \'X-Real-IP\' (when passed by your webserver) is a ' - 'good candidate for this.', + "--proxy-headers-host", + dest="proxy_headers_host", + help="Specify which header will be used for getting the host " + "part. Can be omitted, requires --proxy-headers to be specified " + "when passed. \"X-Real-IP\" (when passed by your webserver) is a " + "good candidate for this.", default=False, - action='store', + action="store", ) self._arg_proxy_port = self.parser.add_argument( - '--proxy-headers-port', - dest='proxy_headers_port', - help='Specify which header will be used for getting the port ' - 'part. Can be omitted, requires --proxy-headers to be specified ' - 'when passed.', + "--proxy-headers-port", + dest="proxy_headers_port", + help="Specify which header will be used for getting the port " + "part. Can be omitted, requires --proxy-headers to be specified " + "when passed.", default=False, - action='store', + action="store", ) self.parser.add_argument( "application", @@ -174,7 +174,7 @@ class CommandLineInterface(object): return raise ArgumentError( argument=argument, - message='--proxy-headers has to be passed for this parameter.') + message="--proxy-headers has to be passed for this parameter.") def _get_forwarded_host(self, args: Namespace) -> str_or_none: """ @@ -186,7 +186,7 @@ class CommandLineInterface(object): argument=self._arg_proxy_host, args=args) return args.proxy_headers_host if args.proxy_headers: - return 'X-Forwarded-For' + return "X-Forwarded-For" def _get_forwarded_port(self, args: Namespace) -> str_or_none: """ @@ -198,7 +198,7 @@ class CommandLineInterface(object): argument=self._arg_proxy_port, args=args) return args.proxy_headers_port if args.proxy_headers: - return 'X-Forwarded-Port' + return "X-Forwarded-Port" def run(self, args): """ diff --git a/daphne/utils.py b/daphne/utils.py index 1c5b3db..00a25c5 100644 --- a/daphne/utils.py +++ b/daphne/utils.py @@ -46,11 +46,11 @@ def parse_x_forwarded_for(headers, new_headers = dict() for name, values in headers.items(): name = name.lower() - name = name if type(name) is bytes else name.encode('utf-8') + name = name if type(name) is bytes else name.encode("utf-8") new_headers[name] = values headers = new_headers - address_header_name = address_header_name.lower().encode('utf-8') + address_header_name = address_header_name.lower().encode("utf-8") result = original if address_header_name in headers: address_value = header_value(headers, address_header_name) diff --git a/tests/test_cli.py b/tests/test_cli.py index 61e468d..f946a82 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -243,9 +243,9 @@ class TestCLIInterface(TestCase): `X-Forwarded-For` header. """ self.assertCLI( - ['--proxy-headers'], + ["--proxy-headers"], { - 'proxy_forwarded_address_header': 'X-Forwarded-For', + "proxy_forwarded_address_header": "X-Forwarded-For", }, ) @@ -255,22 +255,22 @@ class TestCLIInterface(TestCase): the passed one, and `--proxy-headers` is mandatory. """ self.assertCLI( - ['--proxy-headers', '--proxy-headers-host', 'blah'], + ["--proxy-headers", "--proxy-headers-host", "blah"], { - 'proxy_forwarded_address_header': 'blah', + "proxy_forwarded_address_header": "blah", }, ) with self.assertRaises(expected_exception=ArgumentError) as exc: self.assertCLI( - ['--proxy-headers-host', 'blah'], + ["--proxy-headers-host", "blah"], { - 'proxy_forwarded_address_header': 'blah', + "proxy_forwarded_address_header": "blah", }, ) - self.assertEqual(exc.exception.argument_name, '--proxy-headers-host') + self.assertEqual(exc.exception.argument_name, "--proxy-headers-host") self.assertEqual( exc.exception.message, - '--proxy-headers has to be passed for this parameter.') + "--proxy-headers has to be passed for this parameter.") def test_custom_proxyport(self): """ @@ -278,19 +278,19 @@ class TestCLIInterface(TestCase): the passed one, and `--proxy-headers` is mandatory. """ self.assertCLI( - ['--proxy-headers', '--proxy-headers-port', 'blah2'], + ["--proxy-headers", "--proxy-headers-port", "blah2"], { - 'proxy_forwarded_port_header': 'blah2', + "proxy_forwarded_port_header": "blah2", }, ) with self.assertRaises(expected_exception=ArgumentError) as exc: self.assertCLI( - ['--proxy-headers-port', 'blah2'], + ["--proxy-headers-port", "blah2"], { - 'proxy_forwarded_address_header': 'blah2', + "proxy_forwarded_address_header": "blah2", }, ) - self.assertEqual(exc.exception.argument_name, '--proxy-headers-port') + self.assertEqual(exc.exception.argument_name, "--proxy-headers-port") self.assertEqual( exc.exception.message, - '--proxy-headers has to be passed for this parameter.') + "--proxy-headers has to be passed for this parameter.") From 9806d2128080b8882803bba2846e7d143bc1b559 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A1szl=C3=B3=20K=C3=A1rolyi?= Date: Thu, 10 May 2018 17:34:13 +0200 Subject: [PATCH 3/4] Isort + flake8 fix --- daphne/cli.py | 4 ++-- tests/test_cli.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/daphne/cli.py b/daphne/cli.py index 31cfe2d..675878e 100755 --- a/daphne/cli.py +++ b/daphne/cli.py @@ -1,8 +1,7 @@ import argparse import logging import sys - -from argparse import Namespace, ArgumentError +from argparse import ArgumentError, Namespace from typing import Union from .access import AccessLogGenerator @@ -16,6 +15,7 @@ DEFAULT_HOST = "127.0.0.1" DEFAULT_PORT = 8000 str_or_none = Union[None, str] + class CommandLineInterface(object): """ Acts as the main CLI entry point for running the server. diff --git a/tests/test_cli.py b/tests/test_cli.py index f946a82..fa9881c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,8 +1,8 @@ # coding: utf8 import logging -from unittest import TestCase from argparse import ArgumentError +from unittest import TestCase from daphne.cli import CommandLineInterface from daphne.endpoints import build_endpoint_description_strings as build From a7ccae70258cdf94deadad3544bfb22d084da417 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A1szl=C3=B3=20K=C3=A1rolyi?= Date: Wed, 23 May 2018 12:37:27 +0200 Subject: [PATCH 4/4] Adjusting for PR as requested --- daphne/cli.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/daphne/cli.py b/daphne/cli.py index 675878e..497b503 100755 --- a/daphne/cli.py +++ b/daphne/cli.py @@ -2,7 +2,6 @@ import argparse import logging import sys from argparse import ArgumentError, Namespace -from typing import Union from .access import AccessLogGenerator from .endpoints import build_endpoint_description_strings @@ -13,7 +12,6 @@ logger = logging.getLogger(__name__) DEFAULT_HOST = "127.0.0.1" DEFAULT_PORT = 8000 -str_or_none = Union[None, str] class CommandLineInterface(object): @@ -135,7 +133,7 @@ class CommandLineInterface(object): default=False, action="store_true", ) - self._arg_proxy_host = self.parser.add_argument( + self.arg_proxy_host = self.parser.add_argument( "--proxy-headers-host", dest="proxy_headers_host", help="Specify which header will be used for getting the host " @@ -145,7 +143,7 @@ class CommandLineInterface(object): default=False, action="store", ) - self._arg_proxy_port = self.parser.add_argument( + self.arg_proxy_port = self.parser.add_argument( "--proxy-headers-port", dest="proxy_headers_port", help="Specify which header will be used for getting the port " @@ -176,26 +174,26 @@ class CommandLineInterface(object): argument=argument, message="--proxy-headers has to be passed for this parameter.") - def _get_forwarded_host(self, args: Namespace) -> str_or_none: + def _get_forwarded_host(self, args: Namespace): """ Return the default host header from which the remote hostname/ip will be extracted. """ if args.proxy_headers_host: self._check_proxy_headers_passed( - argument=self._arg_proxy_host, args=args) + argument=self.arg_proxy_host, args=args) return args.proxy_headers_host if args.proxy_headers: return "X-Forwarded-For" - def _get_forwarded_port(self, args: Namespace) -> str_or_none: + def _get_forwarded_port(self, args: Namespace): """ Return the default host header from which the remote hostname/ip will be extracted. """ if args.proxy_headers_port: self._check_proxy_headers_passed( - argument=self._arg_proxy_port, args=args) + argument=self.arg_proxy_port, args=args) return args.proxy_headers_port if args.proxy_headers: return "X-Forwarded-Port"