From d1933cc0a8791efbf7f75c3abf941ccffa2eabe0 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 14 Feb 2023 19:21:54 +0100 Subject: [PATCH 1/5] APIv2: remove double `\` This made the regex to not match properly. --- readthedocs/api/v2/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 0e14572c372..7acb36187b6 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -179,7 +179,7 @@ def get_command(self, obj): regex = r"^\$READTHEDOCS_VIRTUALENV_PATH/bin/" command = re.sub(regex, "", command, count=1) - regex = r"^\$CONDA_ENVS_PATH/\\$CONDA_DEFAULT_ENV/bin/" + regex = r"^\$CONDA_ENVS_PATH/\$CONDA_DEFAULT_ENV/bin/" command = re.sub(regex, "", command, count=1) return command From 6b42ea3cceba05ea797b54d17bb745a141b0ea5a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 15 Feb 2023 09:26:11 +0100 Subject: [PATCH 2/5] Test: add test for build command sanitizer --- readthedocs/api/v2/serializers.py | 31 ++++-------------------- readthedocs/api/v2/utils.py | 32 +++++++++++++++++++++++++ readthedocs/rtd_tests/tests/test_api.py | 29 ++++++++++++++++++---- 3 files changed, 60 insertions(+), 32 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 7acb36187b6..0e820900483 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -1,11 +1,10 @@ """Defines serializers for each of our models.""" -import re from allauth.socialaccount.models import SocialAccount -from django.conf import settings from rest_framework import serializers +from readthedocs.api.v2.utils import sanitize_build_command from readthedocs.builds.models import Build, BuildCommandResult, Version from readthedocs.oauth.models import RemoteOrganization, RemoteRepository from readthedocs.projects.models import Domain, Project @@ -157,31 +156,9 @@ class BuildCommandReadOnlySerializer(BuildCommandSerializer): command = serializers.SerializerMethodField() def get_command(self, obj): - project_slug = obj.build.version.project.slug - version_slug = obj.build.version.slug - docroot = settings.DOCROOT.rstrip("/") # remove trailing '/' - - # Remove Docker hash from DOCROOT when running it locally - # DOCROOT contains the Docker container hash (e.g. b7703d1b5854). - # We have to remove it from the DOCROOT it self since it changes each time - # we spin up a new Docker instance locally. - container_hash = "/" - if settings.RTD_DOCKER_COMPOSE: - docroot = re.sub("/[0-9a-z]+/?$", "", settings.DOCROOT, count=1) - container_hash = "/([0-9a-z]+/)?" - - command = obj.command - regex = f"{docroot}{container_hash}{project_slug}/envs/{version_slug}(/bin/)?" - command = re.sub(regex, "", command, count=1) - - # Remove explicit variable names we use to run commands, - # since users don't care about these. - regex = r"^\$READTHEDOCS_VIRTUALENV_PATH/bin/" - command = re.sub(regex, "", command, count=1) - - regex = r"^\$CONDA_ENVS_PATH/\$CONDA_DEFAULT_ENV/bin/" - command = re.sub(regex, "", command, count=1) - return command + return sanitize_build_command( + obj.command, obj.build.project.slug, obj.build.version.slug + ) class BuildSerializer(serializers.ModelSerializer): diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index d7b3cd209c3..e2ecb6daed8 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -1,8 +1,10 @@ """Utility functions that are used by both views and celery tasks.""" import itertools +import re import structlog +from django.conf import settings from rest_framework.pagination import PageNumberPagination from readthedocs.builds.constants import ( @@ -260,6 +262,36 @@ def run_automation_rules(project, added_versions, deleted_active_versions): rule.run(version) +def sanitize_build_command(command, project_slug, version_slug): + """ + Sanitize the build command to be shown to users. + + It removes internal variables and long paths to make them nicer. + """ + docroot = settings.DOCROOT.rstrip("/") # remove trailing '/' + + # Remove Docker hash from DOCROOT when running it locally + # DOCROOT contains the Docker container hash (e.g. b7703d1b5854). + # We have to remove it from the DOCROOT it self since it changes each time + # we spin up a new Docker instance locally. + container_hash = "/" + if settings.RTD_DOCKER_COMPOSE: + docroot = re.sub("/[0-9a-z]+/?$", "", settings.DOCROOT, count=1) + container_hash = "/([0-9a-z]+/)?" + + regex = f"{docroot}{container_hash}{project_slug}/envs/{version_slug}(/bin/)?" + command = re.sub(regex, "", command, count=1) + + # Remove explicit variable names we use to run commands, + # since users don't care about these. + regex = r"^\$READTHEDOCS_VIRTUALENV_PATH/bin/" + command = re.sub(regex, "", command, count=1) + + regex = r"^\$CONDA_ENVS_PATH/\$CONDA_DEFAULT_ENV/bin/" + command = re.sub(regex, "", command, count=1) + return command + + class RemoteOrganizationPagination(PageNumberPagination): page_size = 25 diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 33d28050048..29974c006f8 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -484,8 +484,20 @@ def test_make_build_commands(self): '/api/v2/command/', { "build": build["id"], - "command": "echo test", - "description": "foo", + "command": "$CONDA_ENVS_PATH/$CONDA_DEFAULT_ENV/bin/python -m sphinx", + "description": "Conda and Sphinx command", + "exit_code": 0, + "start_time": start_time, + "end_time": end_time, + }, + format="json", + ) + resp = client.post( + "/api/v2/command/", + { + "build": build["id"], + "command": "$READTHEDOCS_VIRTUALENV_PATH/bin/python -m sphinx", + "description": "Python and Sphinx command", "exit_code": 0, "start_time": start_time, "end_time": end_time, @@ -496,10 +508,12 @@ def test_make_build_commands(self): resp = client.get('/api/v2/build/%s/' % build['id']) self.assertEqual(resp.status_code, 200) build = resp.data - self.assertEqual(len(build["commands"]), 1) - self.assertEqual(build["commands"][0]["command"], "echo test") + self.assertEqual(len(build["commands"]), 2) + self.assertEqual(build["commands"][0]["command"], "python -m sphinx") self.assertEqual(build["commands"][0]["run_time"], 5) - self.assertEqual(build["commands"][0]["description"], "foo") + self.assertEqual( + build["commands"][0]["description"], "Conda and Sphinx command" + ) self.assertEqual(build["commands"][0]["exit_code"], 0) self.assertEqual( dateutil.parser.parse(build["commands"][0]["start_time"]), start_time @@ -508,6 +522,11 @@ def test_make_build_commands(self): dateutil.parser.parse(build["commands"][0]["end_time"]), end_time ) + self.assertEqual(build["commands"][1]["command"], "python -m sphinx") + self.assertEqual( + build["commands"][1]["description"], "Python and Sphinx command" + ) + def test_get_raw_log_success(self): project = Project.objects.get(pk=1) version = project.versions.first() From 59612562df6d078081c40a462b9f207674acab8b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 15 Feb 2023 09:26:37 +0100 Subject: [PATCH 3/5] APIv2: sanitize command when it comes from cold storage --- readthedocs/api/v2/views/model_views.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 1faf7dc2066..77e316465ab 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -13,6 +13,7 @@ from rest_framework.renderers import BaseRenderer, JSONRenderer from rest_framework.response import Response +from readthedocs.api.v2.utils import sanitize_build_command from readthedocs.builds.constants import INTERNAL from readthedocs.builds.models import Build, BuildCommandResult, Version from readthedocs.oauth.models import RemoteOrganization, RemoteRepository @@ -282,6 +283,14 @@ def retrieve(self, *args, **kwargs): try: json_resp = build_commands_storage.open(storage_path).read() data['commands'] = json.loads(json_resp) + + # Sanitize commands in the same way than when returning them using the serializer + for buildcommand in data["commands"]: + buildcommand["command"] = sanitize_build_command( + buildcommand["command"], + instance.project.slug, + instance.version.slug, + ) except Exception: log.exception( 'Failed to read build data from storage.', From e51f4c532944e23650f14c0148aec98f3f85c445 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 15 Feb 2023 16:23:27 +0100 Subject: [PATCH 4/5] Feedback: rename function --- readthedocs/api/v2/serializers.py | 4 ++-- readthedocs/api/v2/utils.py | 2 +- readthedocs/api/v2/views/model_views.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 0e820900483..4a5c03e758a 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -4,7 +4,7 @@ from allauth.socialaccount.models import SocialAccount from rest_framework import serializers -from readthedocs.api.v2.utils import sanitize_build_command +from readthedocs.api.v2.utils import normalize_build_command from readthedocs.builds.models import Build, BuildCommandResult, Version from readthedocs.oauth.models import RemoteOrganization, RemoteRepository from readthedocs.projects.models import Domain, Project @@ -156,7 +156,7 @@ class BuildCommandReadOnlySerializer(BuildCommandSerializer): command = serializers.SerializerMethodField() def get_command(self, obj): - return sanitize_build_command( + return normalize_build_command( obj.command, obj.build.project.slug, obj.build.version.slug ) diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index e2ecb6daed8..f6dcfb2a90d 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -262,7 +262,7 @@ def run_automation_rules(project, added_versions, deleted_active_versions): rule.run(version) -def sanitize_build_command(command, project_slug, version_slug): +def normalize_build_command(command, project_slug, version_slug): """ Sanitize the build command to be shown to users. diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index 77e316465ab..adb187145c9 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -13,7 +13,7 @@ from rest_framework.renderers import BaseRenderer, JSONRenderer from rest_framework.response import Response -from readthedocs.api.v2.utils import sanitize_build_command +from readthedocs.api.v2.utils import normalize_build_command from readthedocs.builds.constants import INTERNAL from readthedocs.builds.models import Build, BuildCommandResult, Version from readthedocs.oauth.models import RemoteOrganization, RemoteRepository @@ -286,7 +286,7 @@ def retrieve(self, *args, **kwargs): # Sanitize commands in the same way than when returning them using the serializer for buildcommand in data["commands"]: - buildcommand["command"] = sanitize_build_command( + buildcommand["command"] = normalize_build_command( buildcommand["command"], instance.project.slug, instance.version.slug, From 1ea36ab7c84e2ae4f6a3a936db8737a8602203a1 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 15 Feb 2023 16:24:24 +0100 Subject: [PATCH 5/5] Lint --- readthedocs/api/v2/views/model_views.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/readthedocs/api/v2/views/model_views.py b/readthedocs/api/v2/views/model_views.py index adb187145c9..8c88f81feb4 100644 --- a/readthedocs/api/v2/views/model_views.py +++ b/readthedocs/api/v2/views/model_views.py @@ -284,7 +284,8 @@ def retrieve(self, *args, **kwargs): json_resp = build_commands_storage.open(storage_path).read() data['commands'] = json.loads(json_resp) - # Sanitize commands in the same way than when returning them using the serializer + # Normalize commands in the same way than when returning + # them using the serializer for buildcommand in data["commands"]: buildcommand["command"] = normalize_build_command( buildcommand["command"],