From 7bb9530760e4f6a0ea17a3e772c5a0632ea3199b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 22 Jan 2024 10:43:34 -0500 Subject: [PATCH] Resolver: don't use str.format (#11044) We were using .format over a user controlled string, the project prefix. This can be used to traverse to some private variables in some cases (this isn't the case here), or to cause a DoS by using an expensive format operation (`{language:>999999999999}`). This isn't a security issue, since we don't expose the prefix to users yet. --- readthedocs/core/resolver.py | 20 +++++++------------- readthedocs/rtd_tests/tests/test_resolver.py | 7 +++++++ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index 1ab644f401e..47b389fd1ad 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -70,13 +70,13 @@ def base_resolve_path( """ Build a path using the given fields. - We first build a format string based on the given fields, - then we just call ``string.format()`` with the given values. - For example, if custom prefix is given, the path will be prefixed with it. In case of a subproject (project_relationship is given), the path will be prefixed with the subproject prefix (defaults to ``/projects//``). + + Then we add the filename, version_slug and language to the path + depending on the versioning scheme. """ path = "/" @@ -88,19 +88,13 @@ def base_resolve_path( path = unsafe_join_url_path(path, custom_prefix) if versioning_scheme == SINGLE_VERSION_WITHOUT_TRANSLATIONS: - path = unsafe_join_url_path(path, "{filename}") + path = unsafe_join_url_path(path, filename) elif versioning_scheme == MULTIPLE_VERSIONS_WITHOUT_TRANSLATIONS: - path = unsafe_join_url_path(path, "{version}/{filename}") + path = unsafe_join_url_path(path, f"{version_slug}/{filename}") else: - path = unsafe_join_url_path(path, "{language}/{version}/{filename}") + path = unsafe_join_url_path(path, f"{language}/{version_slug}/{filename}") - subproject_alias = project_relationship.alias if project_relationship else "" - return path.format( - filename=filename, - version=version_slug, - language=language, - subproject=subproject_alias, - ) + return path def resolve_path( self, diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index b7fa0d3fa58..be0405ecaaf 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -1078,6 +1078,13 @@ def test_custom_prefix_in_subproject_and_custom_prefix_in_superproject(self): url, "http://pip.readthedocs.io/s/sub/prefix/es/latest/api/index.html" ) + def test_format_injection(self): + self.pip.custom_prefix = "/prefix/{language}" + self.pip.save() + url = resolver.resolve(self.pip) + # THe {language} inside the prefix isn't evaluated. + self.assertEqual(url, "http://pip.readthedocs.io/prefix/{language}/en/latest/") + def test_get_project_domain(self): domain = resolver.get_domain(self.pip) self.assertEqual(domain, "http://pip.readthedocs.io")