From 5d36b7a981d81fc31adc978c428b74eea1737fa1 Mon Sep 17 00:00:00 2001 From: vakaris_zilius Date: Fri, 12 Aug 2022 14:54:28 +0000 Subject: [PATCH 1/3] Island: Remove trailing slashes before registering a URL Strict slashes seems to not handle a case when URL is defined with a trailing slash, but request is sent without one. Removing trailing slashes before registering a URL will solve the burden of remembering to register URLS without slashes --- monkey/monkey_island/cc/app.py | 2 +- monkey/tests/unit_tests/monkey_island/cc/test_app.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index c522c3082..0391026bb 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -128,7 +128,7 @@ class FlaskDIWrapper: raise ValueError(f"Resource {resource.__name__} has no defined URLs") self._reserve_urls(resource.urls) - + resource.urls = map(lambda url: url.rstrip("/"), resource.urls) dependencies = self._container.resolve_dependencies(resource) self._api.add_resource(resource, *resource.urls, resource_class_args=dependencies) diff --git a/monkey/tests/unit_tests/monkey_island/cc/test_app.py b/monkey/tests/unit_tests/monkey_island/cc/test_app.py index 225f87ff8..27971e0b6 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/test_app.py +++ b/monkey/tests/unit_tests/monkey_island/cc/test_app.py @@ -81,3 +81,11 @@ def test_url_check_slash_stripping__path_separation(resource_manager): # Following shouldn't raise and exception resource_manager.add_resource(resource3) resource_manager.add_resource(resource4) + + +def test_trailing_slash_removal(resource_manager): + bogus_endpoint = "/beef/face" + resource3 = get_mock_resource("res3", [f"{bogus_endpoint}/"]) + resource_manager.add_resource(resource3) + registered_rules = resource_manager._api.app.url_map._rules + assert any([rule.rule == bogus_endpoint for rule in registered_rules]) From 19df4d97556bc559ea3cbed09a4cce9ec0aedac9 Mon Sep 17 00:00:00 2001 From: Kekoa Kaaikala Date: Mon, 15 Aug 2022 17:34:16 +0000 Subject: [PATCH 2/3] Island: Enforce "no trailing slash" rule for URLs --- monkey/monkey_island/cc/app.py | 9 ++++++++- .../tests/unit_tests/monkey_island/cc/test_app.py | 13 ++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/monkey/monkey_island/cc/app.py b/monkey/monkey_island/cc/app.py index 0391026bb..d27fd24bf 100644 --- a/monkey/monkey_island/cc/app.py +++ b/monkey/monkey_island/cc/app.py @@ -128,7 +128,14 @@ class FlaskDIWrapper: raise ValueError(f"Resource {resource.__name__} has no defined URLs") self._reserve_urls(resource.urls) - resource.urls = map(lambda url: url.rstrip("/"), resource.urls) + + # enforce our rule that URLs should not contain a trailing slash + for url in resource.urls: + if url.endswith("/"): + raise ValueError( + f"Resource {resource.__name__} has an invalid URL: A URL " + "should not have a trailing slash." + ) dependencies = self._container.resolve_dependencies(resource) self._api.add_resource(resource, *resource.urls, resource_class_args=dependencies) diff --git a/monkey/tests/unit_tests/monkey_island/cc/test_app.py b/monkey/tests/unit_tests/monkey_island/cc/test_app.py index 27971e0b6..40e87a32b 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/test_app.py +++ b/monkey/tests/unit_tests/monkey_island/cc/test_app.py @@ -75,7 +75,7 @@ def test_url_check_slash_stripping__trailing_slash(resource_manager): def test_url_check_slash_stripping__path_separation(resource_manager): - resource3 = get_mock_resource("res3", ["/beef/face/"]) + resource3 = get_mock_resource("res3", ["/beef/face"]) resource4 = get_mock_resource("res4", ["/beefface"]) # Following shouldn't raise and exception @@ -83,9 +83,8 @@ def test_url_check_slash_stripping__path_separation(resource_manager): resource_manager.add_resource(resource4) -def test_trailing_slash_removal(resource_manager): - bogus_endpoint = "/beef/face" - resource3 = get_mock_resource("res3", [f"{bogus_endpoint}/"]) - resource_manager.add_resource(resource3) - registered_rules = resource_manager._api.app.url_map._rules - assert any([rule.rule == bogus_endpoint for rule in registered_rules]) +def test_trailing_slash_enforcement(resource_manager): + bad_endpoint = "/beef/face/" + with pytest.raises(ValueError): + resource3 = get_mock_resource("res3", [f"{bad_endpoint}"]) + resource_manager.add_resource(resource3) From a67a4418c91e846740f5fd24198b9995aa516ab7 Mon Sep 17 00:00:00 2001 From: Kekoa Kaaikala Date: Mon, 15 Aug 2022 17:40:01 +0000 Subject: [PATCH 3/3] Island: Remove PropagationCredentials URL trailing slash --- .../monkey_island/cc/resources/propagation_credentials.py | 2 +- .../cc/resources/test_propagation_credentials.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/monkey/monkey_island/cc/resources/propagation_credentials.py b/monkey/monkey_island/cc/resources/propagation_credentials.py index a017d2625..1130d50bd 100644 --- a/monkey/monkey_island/cc/resources/propagation_credentials.py +++ b/monkey/monkey_island/cc/resources/propagation_credentials.py @@ -11,7 +11,7 @@ _stolen_collection = "stolen-credentials" class PropagationCredentials(AbstractResource): - urls = ["/api/propagation-credentials/", "/api/propagation-credentials/"] + urls = ["/api/propagation-credentials", "/api/propagation-credentials/"] def __init__(self, credentials_repository: ICredentialsRepository): self._credentials_repository = credentials_repository diff --git a/monkey/tests/unit_tests/monkey_island/cc/resources/test_propagation_credentials.py b/monkey/tests/unit_tests/monkey_island/cc/resources/test_propagation_credentials.py index 5991abec5..0de5c2c91 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/resources/test_propagation_credentials.py +++ b/monkey/tests/unit_tests/monkey_island/cc/resources/test_propagation_credentials.py @@ -22,8 +22,8 @@ from monkey_island.cc.resources.propagation_credentials import ( ) ALL_CREDENTIALS_URL = PropagationCredentials.urls[0] -CONFIGURED_CREDENTIALS_URL = urljoin(ALL_CREDENTIALS_URL, _configured_collection) -STOLEN_CREDENTIALS_URL = urljoin(ALL_CREDENTIALS_URL, _stolen_collection) +CONFIGURED_CREDENTIALS_URL = urljoin(ALL_CREDENTIALS_URL + "/", _configured_collection) +STOLEN_CREDENTIALS_URL = urljoin(ALL_CREDENTIALS_URL + "/", _stolen_collection) @pytest.fixture @@ -105,7 +105,7 @@ def test_all_propagation_credentials_endpoint__put_not_allowed(flask_client): assert resp.status_code == HTTPStatus.METHOD_NOT_ALLOWED -NON_EXISTENT_COLLECTION_URL = urljoin(ALL_CREDENTIALS_URL, "bogus-credentials") +NON_EXISTENT_COLLECTION_URL = urljoin(ALL_CREDENTIALS_URL + "/", "bogus-credentials") def test_propagation_credentials_endpoint__get_not_found(flask_client):