From ae6792830de22bfb15e7d7154469707cb0604b21 Mon Sep 17 00:00:00 2001
From: Christian Lawson-Perfect <christianperfect@gmail.com>
Date: Tue, 27 Feb 2024 15:46:37 +0000
Subject: [PATCH] editable packages: only allow reading or editing files inside
 the package's directory

This commit adds validation on views which read, write or delete files
belonging to extensions and themes.
---
 editor/forms.py                               | 16 ++++++--
 .../templates/editable_package/edit_base.html |  6 +--
 .../editable_package/edit_source.html         | 20 +++++++---
 editor/views/editable_package.py              | 39 ++++++++++++++++---
 editor/views/theme.py                         |  3 ++
 5 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/editor/forms.py b/editor/forms.py
index 5dcb4df2..196d8be7 100644
--- a/editor/forms.py
+++ b/editor/forms.py
@@ -278,8 +278,18 @@ def clean_zipfile(self):
             raise forms.ValidationError('Uploaded file is not a zip file')
         return value
 
+class PackageFileFormMixin:
+    def clean(self):
+        cleaned_data = super().clean()
+        package = self.instance
+        filename = cleaned_data.get('filename')
+        package_path = Path(package.extracted_path).resolve()
+        path = (package_path / filename).resolve()
+        if not path.is_relative_to(package_path):
+            raise forms.ValidationError("This file is not in the package's directory.")
+        return cleaned_data
 
-class EditPackageForm(forms.ModelForm):
+class EditPackageForm(PackageFileFormMixin, forms.ModelForm):
     
     """Form to edit a file in a package."""
 
@@ -299,7 +309,7 @@ def save(self, commit=True):
                 f.write(self.cleaned_data.get('source'))
         return package
 
-class EditPackageReplaceFileForm(forms.ModelForm):
+class EditPackageReplaceFileForm(PackageFileFormMixin, forms.ModelForm):
     
     """Form to replace a file in a package."""
 
@@ -330,7 +340,7 @@ class ReplaceExtensionFileForm(EditPackageReplaceFileForm):
     class Meta(EditPackageReplaceFileForm.Meta):
         model = Extension
 
-class PackageDeleteFileForm(forms.ModelForm):
+class PackageDeleteFileForm(PackageFileFormMixin, forms.ModelForm):
     filename = forms.CharField(widget=forms.HiddenInput)
 
     class Meta:
diff --git a/editor/templates/editable_package/edit_base.html b/editor/templates/editable_package/edit_base.html
index ef5744a0..a1c37687 100644
--- a/editor/templates/editable_package/edit_base.html
+++ b/editor/templates/editable_package/edit_base.html
@@ -35,7 +35,7 @@ <h1 class="name-header">
             {% endif %}
         </ul>
         {% if object.editable %}
-        <div class="panel-heading">Files {{filename}}</div>
+        <div class="panel-heading">Files</div>
         <ul class="list-group">
             {% if parent_directory != current_directory %}
             <div class="list-group-item parent-directories">
@@ -49,8 +49,8 @@ <h1 class="name-header">
                 </nav>
             </div>
             {% endif %}
-            {% for fname, is_dir in filenames %}
-            <a class="list-group-item monospace{% if fname == filename %} active{% endif %}{% if is_dir %} dir{% endif %}" href="{% package_url 'edit_source' object.pk %}?filename={{fname}}">{{fname}}</a>
+            {% for absfname, relfname, is_dir in filenames %}
+            <a class="list-group-item monospace{% if absfname == filename %} active{% endif %}{% if is_dir %} dir{% endif %}" href="{% package_url 'edit_source' object.pk %}?filename={{absfname}}">{{relfname}}</a>
             {% endfor %}
 
             {% if editable %}
diff --git a/editor/templates/editable_package/edit_source.html b/editor/templates/editable_package/edit_source.html
index 1d659e6a..1a13fd8a 100644
--- a/editor/templates/editable_package/edit_source.html
+++ b/editor/templates/editable_package/edit_source.html
@@ -57,22 +57,32 @@
         {% endif %}
     </div>
 
+    <form action="{% package_url 'edit_source' form.instance.pk %}" method="POST" id="save-file">
     {% if is_image %}
         {% if exists %}
             <a href="{{file_url}}" download="{{filename_without_directories}}"><img class="image-file thumbnail" src="{{file_url}}"></a>
         {% endif %}
     {% else %}
         {% if not is_binary %}
-            <form action="{% package_url 'edit_source' form.instance.pk %}" method="POST" id="save-file">
-                {% csrf_token %}
-                {% for field in form %}
-                    {{field}}
+            {% if form.errors %}
+                {% for error_group in form.errors.values %}
+                    {% for error in error_group %}
+                        <div class="alert alert-danger">
+                            <p>{{error}}</p>
+                        </div>
+                    {% endfor %}
                 {% endfor %}
-            </form>
+            {% endif %}
+
+            {% csrf_token %}
+            {% for field in form %}
+                {{field}}
+            {% endfor %}
         {% else %}
             <p>This is a binary file.</p>
         {% endif %}
     {% endif %}
+    </form>
 {% endif %}
 {% endblock package_edit_content %}
 
diff --git a/editor/views/editable_package.py b/editor/views/editable_package.py
index cbf53802..6d1ba9fb 100644
--- a/editor/views/editable_package.py
+++ b/editor/views/editable_package.py
@@ -9,6 +9,7 @@
 from django import http
 from django.contrib import messages
 from django.contrib.contenttypes.models import ContentType
+from django.core.exceptions import PermissionDenied
 from django.views import generic
 from django.urls import reverse
 from django.utils.timezone import make_aware
@@ -33,15 +34,25 @@ def get_context_data(self,**kwargs):
 
         package = self.get_object()
 
-        context['filenames'] = [(x, (self.get_object().extracted_path / x).is_dir()) for x in self.get_package_filenames()]
+        package_path = Path(package.extracted_path).resolve()
+
+        current_dir = (package_path / self.get_current_directory()).resolve()
+
+        # For a given path, return a triple: (path relative to package root, path relative to current directory, is a directory?)
+        def path_info(path):
+            abspath = (package_path / path).resolve()
+            return (abspath.relative_to(package_path), abspath.relative_to(current_dir), abspath.is_dir())
+
+        context['filenames'] = [path_info(x) for x in self.get_package_filenames()]
         context['editable'] = self.package.can_be_edited_by(self.request.user)
         context['upload_file_form'] = self.upload_file_form_class(instance=self.get_object())
 
         filename = self.get_filename()
+
         if filename is not None:
             path = context['path'] = self.get_path()
             current_directory = context['current_directory'] = self.get_current_directory().relative_to(package.extracted_path)
-            context['filename'] = path.relative_to(package.extracted_path)
+            context['filename'] = (package_path / path).resolve().relative_to(package_path)
             context['parent_directory'] = current_directory.parent
 
             context['exists'] = path.exists()
@@ -72,10 +83,14 @@ def get_filename(self):
 
     def get_path(self):
         package = self.get_object()
-        return Path(package.extracted_path) / self.get_filename()
+        package_path = Path(package.extracted_path)
+        path = package_path / self.get_filename()
+        if not path.is_relative_to(package_path):
+            raise PermissionDenied("This file is not in the package's directory.")
+        return path
 
     def get_current_directory(self):
-        path = self.get_path()
+        path = self.get_path().resolve()
         if path.is_dir():
             return path
         else:
@@ -93,12 +108,15 @@ def __init__(self,*args,**kwargs):
         super().__init__(*args,**kwargs)
 
     def dispatch(self,request,*args,**kwargs):
-        package = self.get_object()
+        package = self.package = self.get_object()
         if not package.editable:
             return forbidden_response(self.request,"This package is not editable.")
         return super().dispatch(request,*args,**kwargs)
 
     def load_source(self,path):
+        if not path.resolve().is_relative_to(self.package.extracted_path):
+            raise PermissionDenied(f"This file is not in the package's directory.")
+
         if path.is_dir():
             return None
         try:
@@ -135,8 +153,14 @@ def get_context_data(self, **kwargs):
         path = context['path'] = self.get_path()
 
         filenames = context['filenames']
+
+        package_path = Path(package.extracted_path)
+
+        current_dir = (package_path / self.get_current_directory()).resolve()
+
         if not context['exists']:
-            filenames.append((self.get_current_directory() / filename, False))
+            filenames.append((Path(filename), (package_path / filename).resolve().relative_to(current_dir), False))
+
         filenames.sort()
 
         context['is_binary'] = self.is_binary
@@ -194,6 +218,9 @@ def get_success_url(self):
 
 class AccessView(ShowPackageFilesMixin, AuthorRequiredMixin, generic.UpdateView):
     template_name = 'editable_package/access.html'
+
+    def get_filename(self):
+        return None
     
     def get_form_kwargs(self, *args, **kwargs):
         kwargs = super().get_form_kwargs(*args,**kwargs)
diff --git a/editor/views/theme.py b/editor/views/theme.py
index de7f5aea..9491a021 100644
--- a/editor/views/theme.py
+++ b/editor/views/theme.py
@@ -23,6 +23,9 @@ def get_filename(self):
         return self.place_filename(filename)
 
     def place_filename(self, filename):
+        if filename is None:
+            return None
+
         extension_dirs = {
             '.md': '',
             '.txt': '',