Security fix for image rotations. Add proper permissions, tests and use a form to avoid cross domain forgery attacks

sas/forms.py

@@ -90,3 +90,10 @@ class PictureModerationRequestForm(forms.ModelForm):
         self.instance.author = self.user
         self.instance.picture = self.picture
         return super().save(commit)
+
+
+class PictureRotationForm(forms.Form):
+    picture = forms.ModelChoiceField(Picture.objects.all(), required=True)
+    direction = forms.ChoiceField(
+        choices=[("LEFT", _("Left")), ("RIGHT", _("Right"))], required=True
+    )

sas/models.py

@@ -139,20 +139,20 @@ class Picture(SasFile):
         self.compressed.name = new_extension_name
 
     def rotate(self, degree):
-        for attr in ["file", "compressed", "thumbnail"]:
-            name = self.__getattribute__(attr).name
-            with open(settings.MEDIA_ROOT / name, "r+b") as file:
-                if file:
-                    im = Image.open(BytesIO(file.read()))
-                    file.seek(0)
-                    im = im.rotate(degree, expand=True)
-                    im.save(
-                        fp=file,
-                        format=self.mime_type.split("/")[-1].upper(),
-                        quality=90,
-                        optimize=True,
-                        progressive=True,
-                    )
+        im = Image.open(BytesIO(self.file.read()))
+        self.file.seek(0)
+        with open(self.file.path, "r+b") as f:
+            im = im.rotate(degree, expand=True)
+            im.save(
+                fp=f,
+                format=self.mime_type.split("/")[-1].upper(),
+                quality=90,
+                optimize=True,
+                progressive=True,
+            )
+        self.file.seek(0)
+        self.generate_thumbnails(overwrite=True)
+        self.save()
 
     def get_next(self):
         if self.is_moderated:

sas/static/sas/css/picture.scss

@@ -191,7 +191,9 @@
     }
 
     >form {
-      input, .ts-wrapper {
+
+      input,
+      .ts-wrapper {
         min-width: 100%;
         max-width: 100%;
         margin: 5px;
@@ -212,22 +214,27 @@
     flex-direction: column;
   }
 
-  .infos, .tools {
+  .infos,
+  .tools {
     flex: 1;
     display: flex;
     flex-direction: column;
     gap: .5em;
+
     @media (min-width: 700px) {
       max-width: 350px;
     }
   }
-  .infos > div, .tools > div > div {
+
+  .infos>div,
+  .tools>div>div {
     display: flex;
     flex-direction: column;
     gap: .35em;
   }
 
-  .tools > div, >.infos >div>div {
+  .tools>div,
+  >.infos>div>div {
     display: flex;
     flex-direction: row;
     justify-content: space-between;
@@ -237,7 +244,15 @@
     flex: 1;
 
     >div>div {
-      >a.btn {
+
+      >form {
+        margin: 0;
+        padding: 0;
+        display: flex;
+      }
+
+      >a.btn,
+      >form>button {
         background-color: $primary-neutral-light-color;
         display: flex;
         justify-content: center;

sas/templates/sas/picture.jinja

@@ -122,7 +122,8 @@
                   {% trans %}Ask for removal{% endtrans %}
                 </a>
               </div>
-              <div class="buttons">
+              <div class="buttons"
+              >
                 <a
                   class="btn btn-no-text"
                   :href="currentPicture.edit_url"
@@ -130,8 +131,22 @@
                 >
                   <i class="fa-regular fa-pen-to-square edit-action"></i>
                 </a>
-                <a class="btn btn-no-text" href="?rotate_left"><i class="fa-solid fa-rotate-left"></i></a>
-                <a class="btn btn-no-text" href="?rotate_right"><i class="fa-solid fa-rotate-right"></i></a>
+                <form method="post" action="{{ url("sas:picture_rotate") }}"
+                      x-show="{{ user.has_perm("sas.change_sasfile")|tojson}}"
+                >
+                  {% csrf_token %}
+                  <input type="hidden" name="picture" :value="currentPicture.id">
+                  <input type="hidden" name="direction" value="LEFT">
+                  <button><i class="fa-solid fa-rotate-left"></i></button>
+                </form>
+                <form method="post" action="{{ url("sas:picture_rotate") }}"
+                      x-show="{{ user.has_perm("sas.change_sasfile")|tojson}}"
+                >
+                  {% csrf_token %}
+                  <input type="hidden" name="picture" :value="currentPicture.id">
+                  <input type="hidden" name="direction" value="RIGHT">
+                  <button><i class="fa-solid fa-rotate-right"></i></button>
+                </form>
               </div>
             </div>
           </div>

sas/tests/test_views.py

@@ -12,16 +12,19 @@
 # OR WITHIN THE LOCAL FILE "LICENSE"
 #
 #
+from io import BytesIO
 from typing import Callable
 
 import pytest
 from bs4 import BeautifulSoup
 from django.conf import settings
 from django.core.cache import cache
+from django.core.files.base import ContentFile
 from django.test import Client, TestCase
 from django.urls import reverse
 from django.utils.timezone import localdate
 from model_bakery import baker
+from PIL import Image
 from pytest_django.asserts import assertHTMLEqual, assertInHTML, assertRedirects
 
 from core.baker_recipes import old_subscriber_user, subscriber_user
@@ -297,6 +300,93 @@ class TestAlbumEdit:
         assert localdate(album.date) == localdate()
 
 
+@pytest.mark.django_db
+class TestPictureRotation:
+    @pytest.fixture
+    def picture(self) -> Picture:
+        # Creating a fake image from scratch is painful
+        # One of the base image in the test set is good enough
+        return Picture.objects.get(name="sli.jpg")
+
+    def load_image(self, file: ContentFile) -> Image.Image:
+        file.seek(0)
+        im = Image.open(BytesIO(file.read()))
+        file.seek(0)
+        return im
+
+    @pytest.mark.parametrize(
+        "user",
+        [
+            None,
+            lambda: baker.make(User),
+            subscriber_user.make,
+            old_subscriber_user.make,
+        ],
+    )
+    def test_permission_denied(
+        self,
+        client: Client,
+        picture: Picture,
+        user: Callable[[], User] | None,
+    ):
+        if user:
+            client.force_login(user())
+
+        payload = {
+            "picture": picture.pk,
+            "direction": "LEFT",
+        }
+        url = reverse("sas:picture_rotate")
+        response = client.post(url, payload)
+        if user:
+            assert response.status_code == 403
+        else:
+            assertRedirects(
+                response,
+                reverse(
+                    "core:login",
+                    query={
+                        "next": url,
+                    },
+                ),
+            )
+
+    @pytest.mark.parametrize(
+        "user",
+        [
+            lambda: baker.make(User, is_superuser=True),
+            lambda: baker.make(
+                User, groups=[Group.objects.get(pk=settings.SITH_GROUP_SAS_ADMIN_ID)]
+            ),
+        ],
+    )
+    def test_rotation(
+        self,
+        client: Client,
+        picture: Picture,
+        user: Callable[[], User],
+    ):
+        client.force_login(user())
+
+        payload = {
+            "picture": picture.pk,
+            "direction": "LEFT",
+        }
+        response = client.post(reverse("sas:picture_rotate"), payload)
+        assertRedirects(
+            response, reverse("sas:picture", kwargs={"picture_id": picture.pk})
+        )
+
+        payload = {
+            "picture": picture.pk,
+            "direction": "RIGHT",
+        }
+        response = client.post(reverse("sas:picture_rotate"), payload)
+        assertRedirects(
+            response, reverse("sas:picture", kwargs={"picture_id": picture.pk})
+        )
+
+
 class TestSasModeration(TestCase):
     @classmethod
     def setUpTestData(cls):

sas/urls.py

@@ -22,6 +22,7 @@ from sas.views import (
     ModerationView,
     PictureAskRemovalView,
     PictureEditView,
+    PictureRotateView,
     PictureView,
     SASMainView,
     UserPicturesView,
@@ -52,6 +53,7 @@ urlpatterns = [
         send_compressed,
         name="download_compressed",
     ),
+    path("picture/rotate", PictureRotateView.as_view(), name="picture_rotate"),
     path("picture/<int:picture_id>/download/thumb/", send_thumb, name="download_thumb"),
     path(
         "user/<int:user_id>/pictures/", UserPicturesView.as_view(), name="user_pictures"

sas/views.py

@@ -15,12 +15,14 @@
 from typing import Any
 
 from django.conf import settings
+from django.contrib import messages
 from django.contrib.auth.mixins import PermissionRequiredMixin
 from django.db.models import Count, OuterRef, Subquery
 from django.http import Http404, HttpResponseRedirect
 from django.shortcuts import get_object_or_404, redirect
 from django.urls import reverse
 from django.utils.safestring import SafeString
+from django.utils.translation import gettext_lazy as _
 from django.views.generic import CreateView, DetailView, TemplateView
 from django.views.generic.edit import FormView, UpdateView
 
@@ -35,6 +37,7 @@ from sas.forms import (
     AlbumEditForm,
     PictureEditForm,
     PictureModerationRequestForm,
+    PictureRotationForm,
     PictureUploadForm,
 )
 from sas.models import Album, PeoplePictureRelation, Picture
@@ -96,20 +99,39 @@ class PictureView(CanViewMixin, DetailView):
     pk_url_kwarg = "picture_id"
     template_name = "sas/picture.jinja"
 
-    def get(self, request, *args, **kwargs):
-        self.object = self.get_object()
-        if "rotate_right" in request.GET:
-            self.object.rotate(270)
-        if "rotate_left" in request.GET:
-            self.object.rotate(90)
-        return super().get(request, *args, **kwargs)
-
     def get_context_data(self, **kwargs):
         return super().get_context_data(**kwargs) | {
             "album": Album.objects.get(children=self.object)
         }
 
 
+class PictureRotateView(PermissionRequiredMixin, FormView):
+    form_class = PictureRotationForm
+    template_name = "core/edit.jinja"
+    permission_required = "sas.moderate_sasfile"
+
+    def form_valid(self, form: PictureRotationForm):
+        angles = {"RIGHT": 270, "LEFT": 90}
+        cleaned = form.clean()
+        cleaned["picture"].rotate(angles[cleaned["direction"]])
+        self._success_url = reverse(
+            "sas:picture",
+            kwargs={
+                "picture_id": cleaned["picture"].pk,
+            },
+        )
+        messages.warning(
+            self.request,
+            _(
+                "Newly rotated image might not be immediately displayed due to your web browser's cache"
+            ),
+        )
+        return super().form_valid(form)
+
+    def get_success_url(self):
+        return self._success_url
+
+
 def send_album(request, album_id):
     return send_file(request, album_id, Album)