Security fixes (#130)

* SYS-1621: Enforce correct authorization of delete_file view
* SYS-1612: Block media file downloads from outside the staff UI
* Fix file sorting display for pdf/text/xml files
* Update packages for security alerts
* Bump version to 1.1.7 and add release notes

charts/prod-ohstaff-values.yaml

@@ -6,7 +6,7 @@ replicaCount: 1
 
 image:
   repository: uclalibrary/oral-history-staff-ui
-  tag: v1.1.6
+  tag: v1.1.7
   pullPolicy: Always
 
 nameOverride: ""

oh_staff_ui/templates/oh_staff_ui/release_notes.html

@@ -4,6 +4,15 @@
 <h3>Release Notes</h3>
 <hr/>
 
+<h4>1.1.7</h4>
+<p><i>May 28, 2024</i></p>
+<ul>
+   <li>Fixed bug allowing staff with lower permissions to delete files if internal URLs were known.</li>
+   <li>Fixed bug allowing download of master files by anonymous users if URLs were known.</li>
+   <li>Updated gunicorn, pillow, and requests packages to patch security issues.</li>
+   <li>Fixed issue where PDF/XML submaster files displayed before master files in "File Information" table.</li>
+</ul>
+
 <h4>1.1.6</h4>
 <p><i>May 22, 2024</i></p>
 <ul>

oh_staff_ui/tests.py

@@ -1,3 +1,4 @@
+from http import HTTPStatus
 from shutil import rmtree
 from lxml import etree
 from pathlib import Path
@@ -7,8 +8,8 @@
 from django.core.management.base import CommandError
 from django.db import IntegrityError
 from django.http import HttpRequest
-from django.test import SimpleTestCase, TestCase, override_settings, Client
-from django.contrib.auth.models import User
+from django.test import SimpleTestCase, TestCase, override_settings
+from django.contrib.auth.models import User, Group
 from eulxml.xmlmap import load_xmlobject_from_string, mods
 from oh_staff_ui.classes.GeneralFileHandler import GeneralFileHandler
 from oh_staff_ui.classes.ImageFileHandler import ImageFileHandler
@@ -87,9 +88,6 @@ def setUpTestData(cls):
         # Get mock request with generic user info for command-line processing.
         cls.mock_request = HttpRequest()
         cls.mock_request.user = User.objects.get(username=cls.user.username)
-        # Add a Django client for testing web requests for media files.
-        cls.client = Client()
-        cls.client.force_login(user=cls.user)
 
     def get_full_path(self, relative_path: str) -> Path:
         # MediaFile.file.name contains a path relative to MEDIA_ROOT;
@@ -704,15 +702,28 @@ def test_file_url_static_thumbnail(self):
         )
 
     @override_settings(RUN_ENV="prod", DEBUG=False)
-    def test_master_file_can_be_served_prod(self):
-        # Confirm master files are web-accessible in production.
+    def test_file_is_served_prod(self):
+        # Confirm media files are web-accessible in production mode,
+        # where DEBUG=False.
         master = self.create_master_general_file()
         handler = GeneralFileHandler(master)
         handler.process_files()
         url = master.media_file.file_url
+        self.client.force_login(self.user)
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
 
+    def test_media_file_is_not_served_without_login(self):
+        # Confirm media files are not web-accessible to anonymous users.
+        master = self.create_master_general_file()
+        handler = GeneralFileHandler(master)
+        handler.process_files()
+        url = master.media_file.file_url
+        response = self.client.get(url)
+        # Login is required, so request is redirected
+        expected_url = f"/accounts/login/?next=/media/{master.media_file.file.name}"
+        self.assertRedirects(response, expected_url=expected_url)
+
     def test_file_size_file_exists(self):
         # Uses real file samples/sample.xml
         file = self.create_master_general_file()
@@ -1725,7 +1736,7 @@ def setUpTestData(cls):
         cls.mock_request = HttpRequest()
         cls.mock_request.user = User.objects.get(username=cls.user.username)
 
-    def create_master_and_derivatives(self, user, mock_request):
+    def create_master_and_derivatives(self, user, mock_request) -> ProjectItem:
         item = ProjectItem.objects.create(
             ark="fake/abcdef",
             created_by=user,
@@ -1745,20 +1756,10 @@ def create_master_and_derivatives(self, user, mock_request):
         handler = ImageFileHandler(master_image_file)
         handler.process_files()
 
-        # get created date from derivative images, so we can check if they are updated
-        thumbnail_create_date = MediaFile.objects.get(
-            item=item, file_type__file_code="image_thumbnail"
-        ).create_date
-        submaster_create_date = MediaFile.objects.get(
-            item=item, file_type__file_code="image_submaster"
-        ).create_date
-
-        return item, thumbnail_create_date, submaster_create_date
+        return item
 
     def test_delete_file_and_children(self):
-        self.item, self.thumbnail_created_date, self.submaster_created_date = (
-            self.create_master_and_derivatives(self.user, self.mock_request)
-        )
+        self.item = self.create_master_and_derivatives(self.user, self.mock_request)
 
         # check that the thumbnail and submaster images are created
         self.assertTrue(MediaFile.objects.filter(item=self.item).count() == 3)
@@ -1787,32 +1788,15 @@ def test_delete_file_and_children(self):
         self.assertFalse(submaster_path.is_file())
 
     def test_delete_file_and_children_no_children(self):
-        self.item = ProjectItem.objects.create(
-            ark="fake/abcdef",
-            created_by=self.user,
-            last_modified_by=self.user,
-            title="Fake title",
-            type=ItemType.objects.get(type="Audio"),
-        )
-        file_type = MediaFileType.objects.get(file_code="image_master")
-        master_image_file = OralHistoryFile(
-            self.item.id,
-            "samples/sample_marbles.tif",
-            file_type,
-            "master",
-            self.mock_request,
-        )
-
-        handler = ImageFileHandler(master_image_file)
-        handler.process_files()
+        self.item = self.create_master_and_derivatives(self.user, self.mock_request)
 
         # check that the thumbnail and submaster images are created
         self.assertTrue(MediaFile.objects.filter(item=self.item).count() == 3)
 
         submaster_mediafile = MediaFile.objects.get(
             item=self.item, file_type__file_code="image_submaster"
         )
-        # delete files
+        # delete submaster only
         delete_file_and_children(submaster_mediafile, self.user)
 
         # check that master and thumbnail files are still there (in DB and on disk)
@@ -1828,6 +1812,33 @@ def test_delete_file_and_children_no_children(self):
         )
         self.assertFalse(submaster_path.is_file())
 
+    def test_unauthorized_user_cannot_delete_files_via_view(self):
+        self.item = self.create_master_and_derivatives(self.user, self.mock_request)
+        file_id = MediaFile.objects.last().id
+        url = f"/delete_file/{file_id}"
+        # Default user created in setUpTestData() does not have the necessary permission.
+        self.client.force_login(self.user)
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN)
+
+    def test_authorized_user_can_delete_files_via_view(self):
+        self.item = self.create_master_and_derivatives(self.user, self.mock_request)
+        file_id = MediaFile.objects.last().id
+        url = f"/delete_file/{file_id}"
+        # Default user created in setUpTestData() does not have the necessary permission,
+        # so create new one and the necessary group.
+        auth_user = User.objects.create_user("auth_tester")
+        authorized_group, created = Group.objects.get_or_create(
+            name="Oral History Staff"
+        )
+        auth_user.groups.add(authorized_group)
+        self.client.force_login(auth_user)
+        # Successful deletion redirects to the upload_file view of the associated
+        # project item.
+        expected_url = f"/upload_file/{self.item.id}"
+        response = self.client.get(url)
+        self.assertRedirects(response, expected_url=expected_url)
+
     def tearDown(self) -> None:
         media_files = MediaFile.objects.filter(item=self.item)
         # delete files on disk first

oh_staff_ui/urls.py

@@ -1,5 +1,3 @@
-from django.conf import settings
-from django.views.static import serve
 from django.urls import path, re_path
 from . import views
 
@@ -20,8 +18,8 @@
     path("delete_file/<int:file_id>", views.delete_file, name="delete_file"),
     path("order_files/<int:item_id>", views.order_files, name="order_files"),
     path("browse/", views.browse, name="browse"),
-    # Allow access to download media files via built-in view django.views.static.serve()
-    re_path(r"^media/(?P<path>.*)$", serve, {"document_root": settings.MEDIA_ROOT}),
+    # Allow staff access to download media files
+    re_path(r"^media/(?P<path>.*)$", views.serve_media_file, name="serve_media_file"),
     # To follow OAI practice, a query parameter combination is requred of either:
     # verb=GetRecord and identifier={ark_value}
     # verb=ListRecords

oh_staff_ui/views.py

@@ -1,12 +1,15 @@
 import logging
 from threading import Thread
 from requests.exceptions import HTTPError
+from django.conf import settings
 from django.contrib import messages
+from django.core.exceptions import PermissionDenied
 from django.core.management.base import CommandError
 from django.shortcuts import redirect, render
 from django.contrib.auth.decorators import login_required
 from django.http.request import HttpRequest  # for code completion
 from django.http.response import HttpResponse  # for code completion
+from django.views.static import serve
 from oh_staff_ui.forms import (
     FileUploadForm,
     ProjectItemForm,
@@ -134,8 +137,10 @@ def show_log(request, line_count: int = 200) -> HttpResponse:
 @login_required
 def upload_file(request: HttpRequest, item_id: int) -> HttpResponse:
     item = ProjectItem.objects.get(pk=item_id)
+    # Sort for display: file_code works for images & audio, file(name)
+    # breaks the tie for pdf & text, since oh_masters -> oh_submasters.
     files = MediaFile.objects.filter(item=item).order_by(
-        "sequence", "file_type__file_code"
+        "sequence", "file_type__file_code", "file"
     )
     # add "children" attribute to each file to hold its derivatives
     # this is used in the template to display child files before deletion
@@ -195,8 +200,25 @@ def upload_file(request: HttpRequest, item_id: int) -> HttpResponse:
 def delete_file(request: HttpRequest, file_id: int) -> HttpResponse:
     media_file = MediaFile.objects.get(pk=file_id)
     item_id = media_file.item.pk
-    delete_file_and_children(media_file, request.user)
-    return redirect("upload_file", item_id=item_id)
+    # Ensure only authorized users can execute this.
+    if user_in_oh_staff_group(request.user):
+        delete_file_and_children(media_file, request.user)
+        return redirect("upload_file", item_id=item_id)
+    else:
+        logger.warning(
+            f"Unauthorized attempt to delete {media_file.file} ({file_id}) by {request.user}"
+        )
+        raise PermissionDenied
+
+
+@login_required
+def serve_media_file(request: HttpRequest, path: str) -> HttpResponse:
+    # Wrap django.views.static.serve view, which normally is in urls.py,
+    # so that login requirement is enforced.
+    # The path parameter comes from re_path in urls.py.
+    # django.views.static.serve is a view, so returns HttpResponse already;
+    # we just pass it on.
+    return serve(request, path=path, document_root=settings.MEDIA_ROOT)
 
 
 @login_required

requirements.txt

@@ -1,10 +1,10 @@
 Django == 4.2.11
 psycopg2 == 2.9.5
-gunicorn == 20.1.0
+gunicorn == 22.0.0
 whitenoise == 6.0.0
-requests == 2.31.0
-pillow == 10.2.0
+requests == 2.32.2
+pillow == 10.3.0
 ffmpeg-python == 0.2.0
 eulxml == 1.1.3
 ply == 3.11
-django-bootstrap5 == 23.1
+django-bootstrap5 == 23.1