diff --git a/website/models/artifact.py b/website/models/artifact.py index 90aa621b..708e9bc5 100644 --- a/website/models/artifact.py +++ b/website/models/artifact.py @@ -3,6 +3,7 @@ import os # for file handling import website.utils.fileutils as ml_fileutils # for custom file handling from sortedm2m.fields import SortedManyToManyField +from website.utils.upload_validators import validate_pdf_upload, validate_raw_file_upload # This retrieves a Python logging instance (or creates it) _logger = logging.getLogger(__name__) @@ -34,9 +35,9 @@ class Artifact(models.Model): location.help_text = "The geographic location of where this artifact was presented" # The artifacts themselves - pdf_file = models.FileField(upload_to=get_upload_dir, null=True, default=None, max_length=255) + pdf_file = models.FileField(upload_to=get_upload_dir, null=True, default=None, max_length=255, validators=[validate_pdf_upload]) pdf_file.help_text = "The rendered PDF of the artifact" - raw_file = models.FileField(upload_to=get_upload_dir, blank=True, null=True, default=None, max_length=255) + raw_file = models.FileField(upload_to=get_upload_dir, blank=True, null=True, default=None, max_length=255, validators=[validate_raw_file_upload]) raw_file.help_text = "The raw file (e.g., pptx, keynote) for the artifact. While not required, this is "\ "highly recommended as it creates a better archive of the work" thumbnail = models.ImageField(upload_to=get_upload_thumbnail_dir, editable=False, null=True, max_length=255) diff --git a/website/models/banner.py b/website/models/banner.py index ccda28d1..b25728f6 100644 --- a/website/models/banner.py +++ b/website/models/banner.py @@ -3,6 +3,7 @@ from django.db.models.signals import pre_delete, post_save, m2m_changed, post_delete from website.utils.fileutils import UniquePathAndRename +from website.utils.upload_validators import validate_image_upload, validate_video_upload from image_cropping import ImageRatioField from .project import Project @@ -17,13 +18,13 @@ class Banner(models.Model): landing_page = models.BooleanField(default=False) landing_page.help_text = 'Check this box if this banner should appear on the landing page.' - image = models.ImageField(blank=True, upload_to=UniquePathAndRename(UPLOAD_DIR, True), max_length=255) + image = models.ImageField(blank=True, upload_to=UniquePathAndRename(UPLOAD_DIR, True), max_length=255, validators=[validate_image_upload]) cropping = ImageRatioField('image', '1600x500', free_crop=False) image.help_text = 'After choosing an image, crop it right here using the cropper below — no need to save first.\ Please note that since we are using a responsive design with fixed height banners, your selected image may appear\ differently on various screens.' - video = models.FileField(upload_to=UniquePathAndRename(VIDEO_UPLOAD_DIR, True), blank=True, null=True) + video = models.FileField(upload_to=UniquePathAndRename(VIDEO_UPLOAD_DIR, True), blank=True, null=True, validators=[validate_video_upload]) video.help_text = "Add in a background video. Ideally, video should be 10MB or less. If both a video and image are specified, the video is prioritized. The image is fallback." alt_text = models.CharField(max_length=1024, blank=True, null=True) diff --git a/website/models/news.py b/website/models/news.py index d4845f04..b25a2e7e 100644 --- a/website/models/news.py +++ b/website/models/news.py @@ -4,6 +4,7 @@ from ckeditor_uploader.fields import RichTextUploadingField from website.utils.fileutils import UniquePathAndRename +from website.utils.upload_validators import validate_image_upload from image_cropping import ImageRatioField from django.utils.text import slugify @@ -44,7 +45,7 @@ def get_thumbnail_size_as_str(): content = RichTextUploadingField(config_name='default') # Following the scheme of above thumbnails in other models - image = models.ImageField(blank=True, upload_to=UniquePathAndRename("news", True), max_length=255) + image = models.ImageField(blank=True, upload_to=UniquePathAndRename("news", True), max_length=255, validators=[validate_image_upload]) image.help_text = 'After choosing an image, crop it right here using the cropper below — no need to save first.' # We use the django-image-cropping ImageRatioField https://github.com/jonasundderwolf/django-image-cropping diff --git a/website/models/person.py b/website/models/person.py index 449310f6..ee3fdc25 100644 --- a/website/models/person.py +++ b/website/models/person.py @@ -6,6 +6,7 @@ from website.models.project_role import ProjectRole from django.core.files import File import website.utils.fileutils as ml_fileutils +from website.utils.upload_validators import validate_image_upload from django.db.models.functions import Coalesce from django.conf import settings @@ -135,7 +136,7 @@ def get_thumbnail_size_as_str(): # Note: the ImageField requires the pillow library # We use the get_unique_path function because otherwise if two people use the same # filename (something generic like picture.jpg), one will overwrite the other. - image = models.ImageField(blank=True, upload_to=get_upload_to_for_person, max_length=255) + image = models.ImageField(blank=True, upload_to=get_upload_to_for_person, max_length=255, validators=[validate_image_upload]) image.help_text = 'After choosing an image, crop it right here using the cropper below — no need to save first.' # We use the django-image-cropping ImageRatioField https://github.com/jonasundderwolf/django-image-cropping @@ -145,7 +146,7 @@ def get_thumbnail_size_as_str(): cropping = ImageRatioField('image', get_thumbnail_size_as_str(), size_warning=True) # This is the hover image (aka easter egg) - easter_egg = models.ImageField(blank=True, null=True, upload_to=get_upload_to_for_person_easter_egg, max_length=255) + easter_egg = models.ImageField(blank=True, null=True, upload_to=get_upload_to_for_person_easter_egg, max_length=255, validators=[validate_image_upload]) easter_egg.help_text = mark_safe("You do not have to set this field. It defaults to a Star Wars\ Rebels LEGO character from here\ but you can use whatever you want. This image is shown on mouseover on the people.html page.") diff --git a/website/models/photo.py b/website/models/photo.py index d3455033..7d947346 100644 --- a/website/models/photo.py +++ b/website/models/photo.py @@ -1,6 +1,7 @@ from django.db import models from django.utils.safestring import mark_safe from image_cropping import ImageRatioField +from website.utils.upload_validators import validate_image_upload from .project import Project @@ -12,7 +13,7 @@ class Photo(models.Model): def get_cropping_size_as_str(): return f"{DEFAULT_CROPPING_SIZE[0]}x{DEFAULT_CROPPING_SIZE[1]}" - picture = models.ImageField(upload_to='projects/images/', max_length=255) + picture = models.ImageField(upload_to='projects/images/', max_length=255, validators=[validate_image_upload]) # TODO: force both caption and alt_text to be non-null and non-blank # This requires a migration so need to talk with Matt/Jason in IT about it. diff --git a/website/models/project.py b/website/models/project.py index 869bd577..abaefc33 100644 --- a/website/models/project.py +++ b/website/models/project.py @@ -4,6 +4,7 @@ from django.db.models.functions import Coalesce from image_cropping import ImageRatioField +from website.utils.upload_validators import validate_image_upload from datetime import date, datetime, timedelta from django.utils import timezone @@ -82,7 +83,7 @@ def get_thumbnail_size_as_str(): # pis = models.ManyToOneField(Person, blank=True, null=True) # TODO: consider switching gallery_image var name to thumbnail - gallery_image = models.ImageField(upload_to=IMAGE_DIR, blank=True, null=True, max_length=255) + gallery_image = models.ImageField(upload_to=IMAGE_DIR, blank=True, null=True, max_length=255, validators=[validate_image_upload]) gallery_image.help_text = "This is the image which will show up on the project gallery page.\ It is not displayed anywhere else. After choosing an image, crop it right here\ using the cropper below — no need to save first." diff --git a/website/models/sponsor.py b/website/models/sponsor.py index 7fb7e35a..ad3269a8 100644 --- a/website/models/sponsor.py +++ b/website/models/sponsor.py @@ -1,5 +1,6 @@ from django.db import models from image_cropping import ImageRatioField +from website.utils.upload_validators import validate_image_upload import os SPONSOR_THUMBNAIL_SIZE = (245, 245) @@ -18,7 +19,7 @@ def get_thumbnail_size_as_str(): short_name = models.CharField(max_length=255, null=True) short_name.help_text = "Short name for the sponsor (e.g., NSF)" - icon = models.ImageField(upload_to=ICON_DIR, blank=True, null=True, max_length=255) + icon = models.ImageField(upload_to=ICON_DIR, blank=True, null=True, max_length=255, validators=[validate_image_upload]) icon.help_text = "Icon for the sponsor (e.g., NSF logo)" alt_text = models.CharField(max_length=1024, blank=True, null=True) diff --git a/website/tests/test_upload_validators.py b/website/tests/test_upload_validators.py new file mode 100644 index 00000000..9b104bd2 --- /dev/null +++ b/website/tests/test_upload_validators.py @@ -0,0 +1,162 @@ +""" +Unit tests for website.utils.upload_validators (issue #6). + +Pure-logic tests: each validator is a function over an uploaded file, so these +use SimpleTestCase + SimpleUploadedFile with crafted bytes — no DB, runs in ms. + +Each category covers four cases: + * accept a file whose extension AND bytes are valid, + * reject a disallowed extension, + * reject a renamed payload (allowed extension, wrong/dangerous bytes), + * plus category-specific cases (HEIC guidance, .fig/.sketch for raw_file). +""" + +from django.core.exceptions import ValidationError +from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import SimpleTestCase + +from website.utils.upload_validators import ( + validate_image_upload, + validate_pdf_upload, + validate_raw_file_upload, + validate_video_upload, +) + + +# --- Sample file headers --------------------------------------------------- + +PNG = b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR" + b"\x00" * 16 +JPEG = b"\xff\xd8\xff\xe0\x00\x10JFIF\x00\x01" + b"\x00" * 16 +GIF = b"GIF89a\x01\x00\x01\x00\x80\x00\x00" + b"\x00" * 16 +WEBP = b"RIFF\x24\x00\x00\x00WEBPVP8 " + b"\x00" * 16 +PDF = b"%PDF-1.4\n%\xe2\xe3\xcf\xd3\n" + b"\x00" * 16 +MP4 = b"\x00\x00\x00\x18ftypmp42\x00\x00\x00\x00" + b"\x00" * 16 +WEBM = b"\x1aE\xdf\xa3\x01\x00\x00\x00" + b"\x00" * 16 +ZIP = b"PK\x03\x04\x14\x00\x00\x00\x08\x00" + b"\x00" * 16 # pptx/docx/key/sketch/zip +OLE = b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1" + b"\x00" * 16 # legacy ppt/doc +FIG = b"fig-kiwi\x0f\x00\x00\x00\x01\x02\x03" + b"\x00" * 16 # proprietary binary +HTML = b"\nhi" +SVG = b'' + + +def _upload(name, content): + return SimpleUploadedFile(name, content) + + +class _CommittedFile: + """ + Minimal stand-in for an already-stored FieldFile (``_committed = True``), + i.e. an existing file on an unchanged record. Validators should skip it. + """ + + def __init__(self, name): + self.name = name + self._committed = True + + def seek(self, *a): + raise AssertionError("a committed file should not be read by validators") + + def read(self, *a): + raise AssertionError("a committed file should not be read by validators") + + +class ImageValidatorTests(SimpleTestCase): + def test_accepts_valid_images(self): + for name, content in [ + ("a.png", PNG), ("a.jpg", JPEG), ("a.jpeg", JPEG), + ("a.gif", GIF), ("a.webp", WEBP), + ]: + with self.subTest(name=name): + validate_image_upload(_upload(name, content)) # no raise + + def test_rejects_disallowed_extension(self): + with self.assertRaises(ValidationError): + validate_image_upload(_upload("a.svg", SVG)) + with self.assertRaises(ValidationError): + validate_image_upload(_upload("a.html", HTML)) + + def test_rejects_renamed_payload(self): + # Allowed extension, but the bytes are HTML, not an image. + with self.assertRaises(ValidationError) as ctx: + validate_image_upload(_upload("evil.png", HTML)) + self.assertEqual(ctx.exception.code, "invalid_image_content") + + def test_heic_gets_guiding_message(self): + for name in ("photo.heic", "photo.HEIC", "photo.heif"): + with self.subTest(name=name): + with self.assertRaises(ValidationError) as ctx: + validate_image_upload(_upload(name, PNG)) + self.assertEqual(ctx.exception.code, "heic_not_supported") + + +class PdfValidatorTests(SimpleTestCase): + def test_accepts_valid_pdf(self): + validate_pdf_upload(_upload("paper.pdf", PDF)) + + def test_rejects_disallowed_extension(self): + with self.assertRaises(ValidationError): + validate_pdf_upload(_upload("paper.exe", PDF)) + + def test_rejects_renamed_payload(self): + with self.assertRaises(ValidationError) as ctx: + validate_pdf_upload(_upload("evil.pdf", HTML)) + self.assertEqual(ctx.exception.code, "invalid_pdf_content") + + +class VideoValidatorTests(SimpleTestCase): + def test_accepts_valid_videos(self): + validate_video_upload(_upload("clip.mp4", MP4)) + validate_video_upload(_upload("clip.mov", MP4)) + validate_video_upload(_upload("clip.webm", WEBM)) + + def test_rejects_disallowed_extension(self): + with self.assertRaises(ValidationError): + validate_video_upload(_upload("clip.avi", MP4)) + + def test_rejects_renamed_payload(self): + with self.assertRaises(ValidationError) as ctx: + validate_video_upload(_upload("evil.mp4", HTML)) + self.assertEqual(ctx.exception.code, "invalid_video_content") + + +class RawFileValidatorTests(SimpleTestCase): + def test_accepts_known_source_formats(self): + for name, content in [ + ("talk.pptx", ZIP), ("talk.key", ZIP), ("doc.docx", ZIP), + ("src.zip", ZIP), ("legacy.ppt", OLE), ("paper.pdf", PDF), + ]: + with self.subTest(name=name): + validate_raw_file_upload(_upload(name, content)) # no raise + + def test_accepts_proprietary_design_files(self): + # .fig / .sketch are accepted by extension; the denylist content check + # passes any non-web-executable bytes, so we don't need their signatures. + validate_raw_file_upload(_upload("poster.fig", FIG)) + validate_raw_file_upload(_upload("poster.sketch", ZIP)) + + def test_rejects_disallowed_extension(self): + with self.assertRaises(ValidationError): + validate_raw_file_upload(_upload("page.html", HTML)) + with self.assertRaises(ValidationError): + validate_raw_file_upload(_upload("image.svg", SVG)) + + def test_rejects_web_executable_content_via_rename(self): + # Allowed extension (.fig) but HTML bytes -> caught by the denylist. + with self.assertRaises(ValidationError) as ctx: + validate_raw_file_upload(_upload("evil.fig", HTML)) + self.assertEqual(ctx.exception.code, "invalid_raw_content") + + +class ExistingFileGateTests(SimpleTestCase): + """An already-stored file (unchanged record edit) is skipped, even if its + extension/content would fail today's rules. Re-validating it adds no + security and would break editing legacy records.""" + + def test_committed_files_are_not_validated(self): + # Each of these would fail if validated as a new upload; the gate + # short-circuits before the extension/content checks (and before any + # read of the file, which _CommittedFile asserts against). + validate_image_upload(_CommittedFile("legacy.bmp")) + validate_pdf_upload(_CommittedFile("legacy.txt")) + validate_video_upload(_CommittedFile("legacy.avi")) + validate_raw_file_upload(_CommittedFile("legacy.tex")) diff --git a/website/utils/upload_validators.py b/website/utils/upload_validators.py new file mode 100644 index 00000000..618d0d38 --- /dev/null +++ b/website/utils/upload_validators.py @@ -0,0 +1,228 @@ +""" +Upload file-type validation for user-supplied media (issue #6). + +All uploads enter through the Django admin (staff/superuser only), so this is +defense-in-depth: it stops a careless or compromised account from placing a +browser-executable file (e.g. ``.svg`` / ``.html``) on a public ``/media/`` +path, where the web server would serve it as active content (stored XSS). It +also rejects plainly wrong file types (e.g. a ``.pages`` headshot) before they +break the thumbnail pipeline. + +Two layers, no third-party dependency: + +1. **Extension allowlist** via Django's ``FileExtensionValidator``. +2. **Magic-byte content sniff** so a renamed payload (``evil.html`` saved as + ``evil.pdf``) is still caught. + +The content strategy differs by field, on purpose: + +* **images / PDFs / videos** use a *positive* check — the bytes must match a + known-good signature. These formats are few, stable, and well known. +* **raw_file** (talk/poster/pub source files) uses a *negative* check. It + accepts a broad, growing set of proprietary source formats (pptx, key, fig, + sketch, zip, ...) whose byte signatures we can't practically enumerate or + keep current, and only *rejects* bytes that look like browser-executable web + content. The security goal for raw_file was always "don't let it be served + as active web content," which a denylist expresses more honestly than a + positive allowlist (which would force us to chase a magic number for every + new design tool). + +Validators run during ``full_clean()``, which the admin's ModelForms call, so +they enforce exactly at the upload surface. They intentionally do *not* run on +a bare ``.save()`` (management commands, signals); that path is not the threat +model. + +These are plain module-level functions so Django serializes them into the +(per-environment, regenerated) migrations by dotted path, with no equality or +deconstruction surprises. + +Usage:: + + from website.utils.upload_validators import validate_image_upload + image = models.ImageField(..., validators=[validate_image_upload]) +""" + +import os + +from django.core.exceptions import ValidationError +from django.core.files.uploadedfile import UploadedFile +from django.core.validators import FileExtensionValidator + +# --- Extension allowlists -------------------------------------------------- + +IMAGE_EXTENSIONS = ["jpg", "jpeg", "png", "gif", "webp"] +PDF_EXTENSIONS = ["pdf"] +VIDEO_EXTENSIONS = ["mp4", "webm", "mov", "m4v"] +# raw_file is a grab-bag of source formats. pptx/docx/key/sketch are all ZIP +# containers underneath; ppt/doc are legacy OLE; fig is a proprietary Figma +# binary. We can't positively fingerprint them all, so the content check below +# is a denylist (see module docstring). +RAW_FILE_EXTENSIONS = [ + "pdf", "ppt", "pptx", "key", "doc", "docx", "zip", "fig", "sketch", +] + + +# --- New-upload gate ------------------------------------------------------- + + +def _is_new_upload(value): + """ + True only for a freshly uploaded file (not an already-stored one). + + Django re-runs field validators on every ``full_clean()``, including when an + admin saves an existing record without touching its file. We skip those: + a committed file is already stored and served, so re-validating it adds no + security, and enforcing today's rules on a legacy file (e.g. a ``raw_file`` + uploaded before these rules existed) would wrongly block the edit. + + A new upload assigned to a FileField is wrapped as a ``FieldFile`` with + ``_committed == False``; an unchanged stored file has ``_committed == True``. + Direct calls in tests pass an ``UploadedFile``. + """ + if isinstance(value, UploadedFile): + return True + return not getattr(value, "_committed", True) + + +# --- Header sniffing ------------------------------------------------------- + + +def _read_header(value, num_bytes=1024): + """ + Return the first ``num_bytes`` of an uploaded file as bytes. + + Seeks back to 0 afterward so the subsequent storage save (which reads from + the current position) writes the whole file. Returns ``b""`` if the file + can't be read, in which case the caller's content check is skipped and only + the extension allowlist applies. + """ + try: + value.seek(0) + header = value.read(num_bytes) or b"" + value.seek(0) + except (OSError, ValueError, AttributeError): + return b"" + return header if isinstance(header, bytes) else bytes(header) + + +def _looks_like_image(header): + """True if the header matches a PNG, JPEG, GIF, or WebP signature.""" + return ( + header.startswith(b"\x89PNG\r\n\x1a\n") # PNG + or header.startswith(b"\xff\xd8\xff") # JPEG + or header.startswith(b"GIF87a") # GIF + or header.startswith(b"GIF89a") # GIF + or (header[:4] == b"RIFF" and header[8:12] == b"WEBP") # WebP + ) + + +def _looks_like_pdf(header): + """True if the header contains the PDF signature (allowing a small BOM/WS offset).""" + return b"%PDF-" in header[:1024] + + +def _looks_like_video(header): + """True if the header matches an ISO-BMFF (mp4/mov/m4v) or Matroska/WebM signature.""" + return ( + header[4:8] == b"ftyp" # ISO base media: mp4, mov, m4v + or header.startswith(b"\x1aE\xdf\xa3") # EBML: webm / matroska + ) + + +def _looks_like_web_executable(header): + """ + True if the header looks like browser-executable web content + (HTML / SVG / XML), which must never be served from a public media path. + """ + stripped = header.lstrip().lower() + return ( + stripped.startswith(b"JPEG conversion on upload is tracked + separately.) + """ + if not _is_new_upload(value): + return + if _extension(value) in ("heic", "heif"): + raise ValidationError( + "HEIC/HEIF images aren't supported by most web browsers. Please " + "convert the image to JPEG or PNG before uploading.", + code="heic_not_supported", + ) + FileExtensionValidator(allowed_extensions=IMAGE_EXTENSIONS)(value) + header = _read_header(value) + if header and not _looks_like_image(header): + raise ValidationError( + "This file doesn't look like a valid image (expected PNG, JPEG, " + "GIF, or WebP). The file extension may not match its contents.", + code="invalid_image_content", + ) + + +def validate_pdf_upload(value): + """Validate an uploaded PDF: ``.pdf`` extension + ``%PDF-`` signature.""" + if not _is_new_upload(value): + return + FileExtensionValidator(allowed_extensions=PDF_EXTENSIONS)(value) + header = _read_header(value) + if header and not _looks_like_pdf(header): + raise ValidationError( + "This file doesn't look like a valid PDF. The file extension may " + "not match its contents.", + code="invalid_pdf_content", + ) + + +def validate_video_upload(value): + """Validate an uploaded video: allowlisted extension + MP4/WebM/MOV signature.""" + if not _is_new_upload(value): + return + FileExtensionValidator(allowed_extensions=VIDEO_EXTENSIONS)(value) + header = _read_header(value) + if header and not _looks_like_video(header): + raise ValidationError( + "This file doesn't look like a valid video (expected MP4, WebM, or " + "MOV). The file extension may not match its contents.", + code="invalid_video_content", + ) + + +def validate_raw_file_upload(value): + """ + Validate a raw source file: allowlisted extension + a denylist content + check that rejects browser-executable (HTML/SVG/XML) content. + + Unlike the image/pdf/video validators, this accepts any file whose bytes + are *not* web-executable, because raw_file holds a broad set of proprietary + source formats (pptx, key, fig, sketch, ...) we can't positively fingerprint. + """ + if not _is_new_upload(value): + return + FileExtensionValidator(allowed_extensions=RAW_FILE_EXTENSIONS)(value) + header = _read_header(value) + if _looks_like_web_executable(header): + raise ValidationError( + "This file appears to contain web/HTML content, which isn't allowed " + "as a raw source file.", + code="invalid_raw_content", + )