Merge pull request #4117 from open-formulieren/issue/4015

[#4015] Fix possible traversal attack in service fetch

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(

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

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="")