Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix wsgi environment building #573

Merged
merged 3 commits into from
Oct 21, 2015
Merged

Fix wsgi environment building #573

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d904071
Fix wsgi environment building
Oct 17, 2015
8991a8f
Add missing assert in wsgi test
Oct 20, 2015
eb54e6c
Fix SERVER_PORT in wsgi headers
Oct 20, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 11 additions & 17 deletions aiohttp/wsgi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from urllib.parse import urlsplit

import aiohttp
from aiohttp import server, helpers, hdrs
from aiohttp import server, hdrs

__all__ = ('WSGIServerHttpProtocol',)

Expand Down Expand Up @@ -63,16 +63,11 @@ def create_wsgi_environ(self, message, payload):
'SERVER_PROTOCOL': 'HTTP/%s.%s' % message.version
}

# authors should be aware that REMOTE_HOST and REMOTE_ADDR
# may not qualify the remote addr:
# http://www.ietf.org/rfc/rfc3875
forward = self.transport.get_extra_info('addr', '127.0.0.1')
script_name = self.SCRIPT_NAME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the comment worth to be saved?
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Added this comment on lines 86-90

server = forward

for hdr_name, hdr_value in message.headers.items():
if hdr_name == 'HOST':
server = hdr_value
if hdr_name == 'AUTHORIZATION':
continue
elif hdr_name == 'SCRIPT_NAME':
script_name = hdr_value
elif hdr_name == 'CONTENT-TYPE':
Expand All @@ -88,17 +83,16 @@ def create_wsgi_environ(self, message, payload):

environ[key] = hdr_value

remote = helpers.parse_remote_addr(forward)
remote = self.transport.get_extra_info('peername')
environ['REMOTE_ADDR'] = remote[0]
environ['REMOTE_PORT'] = remote[1]

if isinstance(server, str):
server = server.split(':')
if len(server) == 1:
server.append('80' if url_scheme == 'http' else '443')

environ['SERVER_NAME'] = server[0]
environ['SERVER_PORT'] = str(server[1])
host = message.headers.get("HOST", None)
if host:
host = host.split(":")
else:
host = self.transport.get_extra_info('sockname')
environ['SERVER_NAME'] = host[0]
environ['SERVER_PORT'] = str(host[1]) if len(host) > 1 else '80'

path_info = uri_parts.path
if script_name:
Expand Down
57 changes: 24 additions & 33 deletions tests/test_wsgi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ def setUp(self):
self.writer = unittest.mock.Mock()
self.writer.drain.return_value = ()
self.transport = unittest.mock.Mock()
self.transport.get_extra_info.return_value = '127.0.0.1'
self.transport.get_extra_info.return_value = ('1.2.3.4', 8080)

self.headers = multidict.MultiDict()
self.headers = multidict.MultiDict({"HOST": "python.org"})
self.message = protocol.RawRequestMessage(
'GET', '/path', (1, 0), self.headers, True, 'deflate')
self.payload = aiohttp.FlowControlDataQueue(self.reader)
Expand Down Expand Up @@ -63,8 +63,7 @@ def test_environ(self):

def test_environ_headers(self):
self.headers.extend(
(('HOST', 'python.org'),
('SCRIPT_NAME', 'script'),
(('SCRIPT_NAME', 'script'),
('CONTENT-TYPE', 'text/plain'),
('CONTENT-LENGTH', '209'),
('X_TEST', '123'),
Expand All @@ -75,10 +74,9 @@ def test_environ_headers(self):
self.assertEqual(environ['HTTP_X_TEST'], '123,456')
self.assertEqual(environ['SCRIPT_NAME'], 'script')
self.assertEqual(environ['SERVER_NAME'], 'python.org')
self.assertEqual(environ['SERVER_PORT'], '443')
self.assertEqual(environ['SERVER_PORT'], '80')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow why the port has changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Port has changed because old version was trying to determine port number by url scheme. (see line 71 is_ssl=True)

So in old version it was '80' if url_scheme == 'http' else '443' (see old version aiohttp/wsgi.py:95-98)

But there is possible situations:

http://example.com:8080/
https://example.com:4443/

Old version will give wrong result in both cases.

Ill add more tests to cover it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. I just realized that client may connect to port 123 but say

GET / HTTP/1.1
Host: example.com:456

According to CGI 1.1 in this case SERVER_PORT should be 123.

Ill fix it right now.


def test_environ_host_header(self):
self.headers.add('HOST', 'python.org')
environ = self._make_one()

self.assertEqual(environ['HTTP_HOST'], 'python.org')
Expand All @@ -87,41 +85,16 @@ def test_environ_host_header(self):
self.assertEqual(environ['SERVER_PROTOCOL'], 'HTTP/1.0')

def test_environ_host_port_header(self):
headers = multidict.MultiDict({'HOST': 'python.org:443'})
self.message = protocol.RawRequestMessage(
'GET', '/path', (1, 1), self.headers, True, 'deflate')
self.headers.add('HOST', 'python.org:443')
'GET', '/path', (1, 1), headers, True, 'deflate')
environ = self._make_one()

self.assertEqual(environ['HTTP_HOST'], 'python.org:443')
self.assertEqual(environ['SERVER_NAME'], 'python.org')
self.assertEqual(environ['SERVER_PORT'], '443')
self.assertEqual(environ['SERVER_PROTOCOL'], 'HTTP/1.1')

def test_environ_forward(self):
self.transport.get_extra_info.return_value = 'localhost,127.0.0.1'
environ = self._make_one()

self.assertEqual(environ['REMOTE_ADDR'], '127.0.0.1')
self.assertEqual(environ['REMOTE_PORT'], '80')

self.transport.get_extra_info.return_value = 'localhost,127.0.0.1:443'
environ = self._make_one()

self.assertEqual(environ['REMOTE_ADDR'], '127.0.0.1')
self.assertEqual(environ['REMOTE_PORT'], '443')

self.transport.get_extra_info.return_value = ('127.0.0.1', 443)
environ = self._make_one()

self.assertEqual(environ['REMOTE_ADDR'], '127.0.0.1')
self.assertEqual(environ['REMOTE_PORT'], '443')

self.transport.get_extra_info.return_value = '[::1]'
environ = self._make_one()

self.assertEqual(environ['REMOTE_ADDR'], '::1')
self.assertEqual(environ['REMOTE_PORT'], '80')

def test_wsgi_response(self):
srv = self._make_srv()
resp = srv.create_wsgi_response(self.message)
Expand Down Expand Up @@ -280,3 +253,21 @@ def test_dont_unquote_environ_path_info(self):
'GET', path, (1, 0), self.headers, True, 'deflate')
environ = self._make_one()
self.assertEqual(environ['PATH_INFO'], path)

def test_not_add_authorization(self):
self.headers.extend({'AUTHORIZATION': 'spam',
'X-CUSTOM-HEADER': 'eggs'})
self.message = protocol.RawRequestMessage(
'GET', '/', (1, 1), self.headers, True, 'deflate')
environ = self._make_one()
self.assertEqual('eggs', environ['HTTP_X_CUSTOM_HEADER'])
self.assertFalse('AUTHORIZATION' in environ)

def test_http_1_0_no_host(self):
headers = multidict.MultiDict({})
self.message = protocol.RawRequestMessage(
'GET', '/', (1, 0), headers, True, 'deflate')
environ = self._make_one()
self.assertEqual('1.2.3.4', environ['SERVER_NAME'])
self.assertEqual('8080', environ['SERVER_PORT'])
self.transport.get_extra_info.assert_called_with('sockname')