Skip to content

Fix parsing of the --bind option for abstract UNIX sockets #248

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.

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

Merged
merged 11 commits into from
Nov 18, 2019
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
29 changes: 17 additions & 12 deletions cheroot/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
from importlib import import_module
import os
import sys
import contextlib
import inspect

import six

Expand All @@ -49,6 +49,7 @@ def __init__(self, address, port):
Args:
address (str): Host name or IP address
port (int): TCP port number

"""
self.bind_addr = address, port

Expand All @@ -64,9 +65,9 @@ def __init__(self, path):
class AbstractSocket(BindLocation):
"""AbstractSocket."""

def __init__(self, addr):
def __init__(self, abstract_socket):
"""Initialize."""
self.bind_addr = '\0{}'.format(self.abstract_socket)
self.bind_addr = '\0{}'.format(abstract_socket)


class Application:
Expand All @@ -77,12 +78,12 @@ def resolve(cls, full_path):
"""Read WSGI app/Gateway path string and import application module."""
mod_path, _, app_path = full_path.partition(':')
app = getattr(import_module(mod_path), app_path or 'application')

with contextlib.suppress(TypeError):
if issubclass(app, server.Gateway):
return GatewayYo(app)

return cls(app)
# verify that `app` is a class before the issubclass verification,
# otherwise a TypeError will be rised.
if inspect.isclass(app) and issubclass(app, server.Gateway):
Copy link
Member

Choose a reason for hiding this comment

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

EAFP is more natural to Python. If you worry about the Py2 compat, I could cherry-pick this https://github.com/cherrypy/cheroot/pull/243/files#diff-544070cbdc8d4e10b3faf72ba309abc6R18-R31 shim to master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the original approach was a plain try/except TypeError and ignore with a comment on why are we ignoring that exception, but I end up preferring the LBFL in this case. But it seem that the suppress context manager is used a lot in the other PR. If that's going to be included, then yeah I could use that here and be done by leaving this fragment as it was.

Copy link
Member

Choose a reason for hiding this comment

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

It's in master already.

return GatewayYo(app)
else:
return cls(app)

def __init__(self, wsgi_app):
"""Initialize."""
Expand Down Expand Up @@ -128,6 +129,13 @@ def server(self, parsed_args):

def parse_wsgi_bind_location(bind_addr_string):
"""Convert bind address string to a BindLocation."""
# if the string begins with an @ symbol, use an abstract socket,
# this is the first condition to verify, otherwise the urlparse
# validation would detect //@<value> as a valid url with a hostname
# with value: "<value>" and port: None
if bind_addr_string.startswith('@'):
return AbstractSocket(bind_addr_string[1:])

# try and match for an IP/hostname and port
match = six.moves.urllib.parse.urlparse('//{}'.format(bind_addr_string))
try:
Expand All @@ -139,9 +147,6 @@ def parse_wsgi_bind_location(bind_addr_string):
pass

# else, assume a UNIX socket path
# if the string begins with an @ symbol, use an abstract socket
if bind_addr_string.startswith('@'):
return AbstractSocket(bind_addr_string[1:])
return UnixSocket(path=bind_addr_string)


Expand Down
81 changes: 81 additions & 0 deletions cheroot/test/test_cli.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
"""Tests to verify the command line interface."""
# -*- coding: utf-8 -*-
# vim: set fileencoding=utf-8 :
import sys

import six
import pytest

from cheroot.cli import (
Application,
parse_wsgi_bind_addr,
)


@pytest.mark.parametrize(
'raw_bind_addr, expected_bind_addr', (
('192.168.1.1:80', ('192.168.1.1', 80)),
('[::1]:8000', ('::1', 8000)),
Copy link
Member

@webknjaz webknjaz Nov 15, 2019

Choose a reason for hiding this comment

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

This reminds me that we should probably also test catch-all addrs like

Suggested change
('[::1]:8000', ('::1', 8000)),
('[::1]:8000', ('::1', 8000)),
('[::]:8000', ('::', 8000)),
('12345', ('::', 12345)),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... I was just avoiding doing the extra work of thinking of future possible uses cases and all the possible values that urllib may support.

),
)
def test_parse_wsgi_bind_addr_for_tcpip(raw_bind_addr, expected_bind_addr):
"""Check the parsing of the --bind option for TCP/IP addresses."""
assert parse_wsgi_bind_addr(raw_bind_addr) == expected_bind_addr
Copy link
Member

Choose a reason for hiding this comment

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

I'd merge these all similar tests into parametrize of the first one. Just make sure to use pytest.param(..., ..., id=) (http://doc.pytest.org/en/latest/example/parametrize.html#different-options-for-test-ids / https://docs.pytest.org/en/latest/reference.html#pytest-param)



def test_parse_wsgi_bind_addr_for_unix_socket():
"""Check the parsing of the --bind option for UNIX Sockets."""
assert parse_wsgi_bind_addr('/tmp/cheroot.sock') == '/tmp/cheroot.sock'


def test_parse_wsgi_bind_addr_for_abstract_unix_socket():
"""Check the parsing of the --bind option for Abstract UNIX Sockets."""
assert parse_wsgi_bind_addr('@cheroot') == '\0cheroot'


class WSGIAppMock:
Copy link
Member

@webknjaz webknjaz Nov 15, 2019

Choose a reason for hiding this comment

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

This is how you make a fixture with pytest:

Suggested change
class WSGIAppMock:
@pytest.fixture
def wsgi_app(monkeypatch):
"""Return a WSGI app stub."""
class WSGIAppMock:
"""Mock of a wsgi module."""
def application(self):
"""Empty application method.
Default method to be called when no specific callable
is defined in the wsgi application identifier.
It has an empty body because we are expecting to verify that
the same method is return no the actual execution of it.
"""
def main(self):
"""Empty custom method (callable) inside the mocked WSGI app.
It has an empty body because we are expecting to verify that
the same method is return no the actual execution of it.
"""
mocked_app = WSGIAppMock()
if six.PY2:
# python2 requires the previous namespaces to be part of sys.modules
οΏΌ # (e.g. for 'a.b.c' we need to insert 'a', 'a.b' and 'a.b.c')
οΏΌ # otherwise it fails, we're setting the same instance on each level,
οΏΌ # we don't really care about those, just the last one.
full_path, *pkg_parts, _last_part = pkg_name.split('.')
οΏΌ for p in pkg_parts:
οΏΌ full_path = '.'.join((full_path, p))
οΏΌ monkeypatch.setitem(sys.modules, full_path, mocked_app)
οΏΌ monkeypatch.setitem(sys.modules, pkg_name, mocked_app)
return mocked_app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW that unpacking syntax doesn't work on Python 2.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, I guess I'm too addicted to py3 :)

"""Mock of a wsgi module."""

def application(self):
"""Empty application method.

Default method to be called when no specific callable
is defined in the wsgi application identifier.

It has an empty body because we are expecting to verify that
the same method is return no the actual execution of it.
"""

def main(self):
"""Empty custom method (callable) inside the mocked WSGI app.

It has an empty body because we are expecting to verify that
the same method is return no the actual execution of it.
"""


@pytest.mark.parametrize(
'wsgi_app_spec, pkg_name, app_method, mocked_app', (
('mypkg.wsgi', 'mypkg.wsgi', 'application', WSGIAppMock()),
Copy link
Member

Choose a reason for hiding this comment

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

WSGIAppMock() is evaluated during the test module import time. Better use a fixture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mock instance is directly related to the other params. I did liked the idea of passing it along the others params to express the direct relationship.

For example a future param ('a.b:foo', 'a.b', 'foo') will break the test, because the mock doesn't define a foo method. This is not obvious via the implicit mock injection of pytest.

But I understand if you have a strong opinion on going over the pytest mock injection route.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, you'd have to deal with this on the fixture level. This is a pytest way.

('mypkg.wsgi:application', 'mypkg.wsgi', 'application', WSGIAppMock()),
('mypkg.wsgi:main', 'mypkg.wsgi', 'main', WSGIAppMock()),
),
)
def test_Aplication_resolve(
monkeypatch,
wsgi_app_spec, pkg_name, app_method, mocked_app,
Copy link
Member

Choose a reason for hiding this comment

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

To use the fixture from https://github.com/cherrypy/cheroot/pull/248/files#r347003745:

Suggested change
wsgi_app_spec, pkg_name, app_method, mocked_app,
wsgi_app_spec, pkg_name, app_method, wsgi_app,

):
"""Check the wsgi application name conversion."""
if six.PY2:
Copy link
Member

Choose a reason for hiding this comment

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

This whole monkeypatching can be also moved to the fixture so that only testing remains in the test function: https://github.com/cherrypy/cheroot/pull/248/files#r347003745.

# python2 requires the previous namespaces to be part of sys.modules
# (e.g. for 'a.b.c' we need to insert 'a', 'a.b' and 'a.b.c')
# otherwise it fails, we're setting the same instance on each level,
# we don't really care about those, just the last one.
full_path = None
for p in pkg_name.split('.'):
full_path = p if full_path is None else '.'.join((full_path, p))
monkeypatch.setitem(sys.modules, full_path, mocked_app)
else:
monkeypatch.setitem(sys.modules, pkg_name, mocked_app)
expected_app = getattr(mocked_app, app_method)
assert Application.resolve(wsgi_app_spec).wsgi_app == expected_app
4 changes: 2 additions & 2 deletions cheroot/test/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,8 @@ def test_content_length_required(test_client):
def test_large_request(test_client_with_defaults):
"""Test GET query with maliciously large Content-Length."""
# If the server's max_request_body_size is not set (i.e. is set to 0)
# then this will result in an `OverflowError: Python int too large to convert to C ssize_t`
# in the server.
# then this will result in an `OverflowError: Python int too large to
# convert to C `ssize_t` in the server.
Copy link
Member

Choose a reason for hiding this comment

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

Wow, I probably missed this in the previous PR. I'll commit it to master directly because it's completely unrelated to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

This one's in master already.

# We expect that this should instead return that the request is too
# large.
c = test_client_with_defaults.get_connection()
Expand Down