diff --git a/src/openforms/submissions/tests/form_logic/test_fetching_form_variable_values_from_services.py b/src/openforms/submissions/tests/form_logic/test_fetching_form_variable_values_from_services.py index 16e4963203..a0f4bb9b61 100644 --- a/src/openforms/submissions/tests/form_logic/test_fetching_form_variable_values_from_services.py +++ b/src/openforms/submissions/tests/form_logic/test_fetching_form_variable_values_from_services.py @@ -3,6 +3,7 @@ from unittest import skip from urllib.parse import unquote +from django.core.exceptions import SuspiciousOperation from django.test import SimpleTestCase, tag import requests_mock @@ -112,7 +113,8 @@ def test_substitutions_all_happen_at_once(self): @example("./../.") @example("foo/ +.") def test_it_can_construct_simple_path_parameters_from_any_input(self, field_value): - assume(field_value != ".") # request_mock eats the single dot :pacman: + # request_mock eats the single dot :pacman:, and ".." is disallowed: + assume(field_value not in {".", ".."}) # https://swagger.io/docs/specification/describing-parameters/#path-parameters context = {"late": {"seconds": field_value}} @@ -150,6 +152,20 @@ def test_it_can_construct_simple_path_parameters_from_any_input(self, field_valu # and a weak assertion self.assertNotIn(str(field_value), request.headers.values()) + @tag("gh-4015") + def test_raises_suspicious_operation_on_double_dot_input(self): + context = {"late": {"seconds": ".."}} + + var = FormVariableFactory.build( + service_fetch_configuration=ServiceFetchConfigurationFactory.build( + service=self.service, + path="delay/{{late.seconds}}", # this is not defined as number in the OAS + ) + ) + + with self.assertRaises(SuspiciousOperation): + perform_service_fetch(var, context) + # TODO @tag("gh-2745") @given( diff --git a/src/openforms/variables/models.py b/src/openforms/variables/models.py index d2de3fec30..76fa667968 100644 --- a/src/openforms/variables/models.py +++ b/src/openforms/variables/models.py @@ -1,5 +1,3 @@ -from urllib.parse import quote - from django.core.exceptions import ValidationError from django.db import models from django.utils.translation import gettext_lazy as _ @@ -13,6 +11,7 @@ HeaderValidator, QueryParameterValidator, validate_mapping_expression, + validate_path_context_values, validate_request_body, ) @@ -149,7 +148,7 @@ def request_arguments(self, context: DataMapping) -> dict: HeaderValidator()(headers) escaped_for_path = recursive_apply( - context, lambda v: quote(str(v), safe=""), transform_leaf=True + context, validate_path_context_values, transform_leaf=True ) query_params = { @@ -165,7 +164,7 @@ def request_arguments(self, context: DataMapping) -> dict: for param, values in (self.query_params or {}).items() } - # Interface of :meth:`requests.Sesssion.request` + # Interface of :meth:`requests.Session.request` request_args = { "method": self.method, # client class automatically adds this to the API root diff --git a/src/openforms/variables/validators.py b/src/openforms/variables/validators.py index 9b5359efcc..59da5cd4fe 100644 --- a/src/openforms/variables/validators.py +++ b/src/openforms/variables/validators.py @@ -1,7 +1,8 @@ import re from typing import TYPE_CHECKING, Mapping +from urllib.parse import quote -from django.core.exceptions import ValidationError +from django.core.exceptions import SuspiciousOperation, ValidationError from django.utils.deconstruct import deconstructible from django.utils.translation import gettext_lazy as _ @@ -257,3 +258,15 @@ def validate_mapping_expression(configuration: "ServiceFetchConfiguration") -> N def has_whitespace_padding(s: str) -> bool: return not re.match(r"^([^ \t].*[^ \t]|[^ \t])$", s) + + +def validate_path_context_values(v: object) -> str: + if v == "..": + # GH-4015: requests is applying path traversal on "..", i.e. https://web.com/path1/../path2 + # will request to https://web.com/path2. "../" and similar are fine as '/' is escaped by `quote`. + # see https://github.com/urllib3/urllib3/issues/1781 + # see https://mazinahmed.net/blog/testing-for-path-traversal-with-python/ + raise SuspiciousOperation( + "Usage of '..' in service fetch can lead to path traversal attacks" + ) + return quote(str(v), safe="")