diff --git a/AGENTS.md b/AGENTS.md index dbb603e..e51c7df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,3 +47,20 @@ Project constraints (do not regress): and the server only applies the result via POST /api/files/J-x/optimize/. - Do not use imgdd; perceptual hashing is imagehash server-side. - Chat/messaging features are out of scope. + +Security hardening (do not weaken): +- e621 API keys are stored Fernet-encrypted with a key derived from + SECRET_KEY (apps/accounts/crypto.py); rotating SECRET_KEY invalidates them + (and all signed media URLs), so users must re-enter the key. +- API throttles live in REST_FRAMEWORK (env-overridable): anon 120/min, + user 600/min, login 5/min, register 20/hour, e621_proxy 60/hour. +- Only admins (superusers) may grant/revoke the staff role or delete + staff/admin accounts; staff manage regular/uploader accounts only. +- Storage, duplicates, delete, temp-clear, uploads and downloads require + upload rights (CanUpload), not merely authentication. +- Server-side fetching only happens for allowlisted e621 media hosts via + services.validate_remote_url / open_remote (every redirect hop is + re-validated); do not call requests.get on user-supplied URLs elsewhere. +- A repeatable audit harness (permission matrix, IDOR, guest visibility, + signed URLs, SSRF, throttles) was used to verify this; formalising it as + a test suite is still open in the roadmap. diff --git a/ROADMAP.md b/ROADMAP.md index 83b721b..be5611a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -170,3 +170,6 @@ Files now stage first and are resolved before entering the library. - Anything touching e621 endpoints follows the OpenAPI spec (https://e621.wiki/openapi.yaml) — see AGENTS.md for how to fetch and which response shapes to watch out for. +- Security review done (Aug 2026): permissions verified per role with a + repeatable harness, e621 keys encrypted at rest, rate limits added and the + download paths host-allowlisted. See AGENTS.md for the rules to keep. diff --git a/backend/.env.example b/backend/.env.example index 1d59693..f29b2da 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -2,6 +2,14 @@ SECRET_KEY=change-me-to-a-long-random-string DEBUG=True ALLOWED_HOSTS=localhost,127.0.0.1 +# Rate limits (per IP for anonymous, per account when signed in). Counted in +# the shared Redis cache; the defaults fit the shell's polling. +# THROTTLE_ANON=120/min +# THROTTLE_USER=600/min +# THROTTLE_LOGIN=5/min +# THROTTLE_REGISTER=20/hour +# THROTTLE_E621_PROXY=60/hour + # Cross-origin frontends (comma separated). Same-origin keeps working with an # empty list; add e.g. https://j621.example.com when the SPA is served from # another origin than this API. diff --git a/backend/apps/accounts/crypto.py b/backend/apps/accounts/crypto.py new file mode 100644 index 0000000..63a4f17 --- /dev/null +++ b/backend/apps/accounts/crypto.py @@ -0,0 +1,48 @@ +"""Symmetric encryption for secrets stored at rest, keyed by SECRET_KEY. + +Used for the per-user e621 API keys: a database dump (or backup) alone does +not expose them. Rotating SECRET_KEY makes existing values undecryptable, +exactly like the signed media URLs stop verifying — users then re-enter +their e621 key. +""" + +import base64 +import hashlib + +from cryptography.fernet import Fernet, InvalidToken +from django.conf import settings + +PREFIX = "enc:" + + +def _fernet() -> Fernet: + # Derive a stable Fernet key from the project secret. + digest = hashlib.sha256( + f"j621-secret-v1:{settings.SECRET_KEY}".encode() + ).digest() + return Fernet(base64.urlsafe_b64encode(digest)) + + +def encrypt_secret(value: str) -> str: + """Encrypt for storage; empty stays empty and already-encrypted passes through.""" + if not value: + return "" + if value.startswith(PREFIX): + return value + return PREFIX + _fernet().encrypt(value.encode()).decode() + + +def decrypt_secret(value: str) -> str: + """Decrypt a stored value; legacy plaintext passes through. + + Returns "" when the value cannot be decrypted (e.g. SECRET_KEY changed), + which reads as "not configured" and asks the user for the key again. + """ + if not value: + return "" + if not value.startswith(PREFIX): + return value + try: + return _fernet().decrypt(value[len(PREFIX) :].encode()).decode() + except InvalidToken: + return "" diff --git a/backend/apps/accounts/migrations/0006_encrypt_e621_api_keys.py b/backend/apps/accounts/migrations/0006_encrypt_e621_api_keys.py new file mode 100644 index 0000000..01fef3d --- /dev/null +++ b/backend/apps/accounts/migrations/0006_encrypt_e621_api_keys.py @@ -0,0 +1,34 @@ +from django.db import migrations, models + + +def encrypt_existing_keys(apps, schema_editor): + """Store existing e621 API keys encrypted (idempotent).""" + from apps.accounts.crypto import encrypt_secret + + User = apps.get_model("accounts", "User") + for user in User.objects.exclude(e621_api_key="").iterator(): + if user.e621_api_key.startswith("enc:"): + continue + user.e621_api_key = encrypt_secret(user.e621_api_key) + user.save(update_fields=["e621_api_key"]) + + +def noop(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + dependencies = [ + ("accounts", "0005_user_preferences"), + ] + + operations = [ + # Ciphertext is longer than the plaintext key, so widen the column + # before encrypting existing rows. + migrations.AlterField( + model_name="user", + name="e621_api_key", + field=models.CharField(blank=True, default="", max_length=400), + ), + migrations.RunPython(encrypt_existing_keys, noop), + ] diff --git a/backend/apps/accounts/models.py b/backend/apps/accounts/models.py index fb5ffd8..a55b024 100644 --- a/backend/apps/accounts/models.py +++ b/backend/apps/accounts/models.py @@ -23,14 +23,22 @@ class User(AbstractUser): related_name="+", ) e621_username = models.CharField(max_length=100, blank=True, default="") - e621_api_key = models.CharField(max_length=100, blank=True, default="") + # Stored encrypted (see apps/accounts/crypto.py): never the plaintext key. + e621_api_key = models.CharField(max_length=400, blank=True, default="") e621_base_url = models.CharField(max_length=200, default="https://e621.net") # Per-user browse preferences (landing page, default filters, grid size). preferences = models.JSONField(default=dict, blank=True) + @property + def e621_api_key_plain(self): + """The decrypted e621 key ("" when it cannot be decrypted).""" + from .crypto import decrypt_secret + + return decrypt_secret(self.e621_api_key) + @property def e621_configured(self): - return bool(self.e621_username and self.e621_api_key) + return bool(self.e621_username and self.e621_api_key_plain) @property def can_upload(self): diff --git a/backend/apps/accounts/urls.py b/backend/apps/accounts/urls.py index 7e45c27..154202e 100644 --- a/backend/apps/accounts/urls.py +++ b/backend/apps/accounts/urls.py @@ -1,9 +1,9 @@ from django.urls import path -from rest_framework.authtoken.views import obtain_auth_token from .views import ( AvatarView, E621CredentialsView, + LoginView, LogoutView, MeView, PreferencesView, @@ -12,7 +12,7 @@ from .views import ( urlpatterns = [ path("register/", RegisterView.as_view(), name="register"), - path("token/", obtain_auth_token, name="login"), + path("token/", LoginView.as_view(), name="login"), path("logout/", LogoutView.as_view(), name="logout"), path("me/", MeView.as_view(), name="me"), path("avatar/", AvatarView.as_view(), name="avatar"), diff --git a/backend/apps/accounts/views.py b/backend/apps/accounts/views.py index 16ac423..01f1c1c 100644 --- a/backend/apps/accounts/views.py +++ b/backend/apps/accounts/views.py @@ -2,14 +2,17 @@ import logging from rest_framework import mixins, status, viewsets from rest_framework.authtoken.models import Token +from rest_framework.authtoken.views import ObtainAuthToken from rest_framework.permissions import AllowAny, IsAuthenticated from rest_framework.response import Response +from rest_framework.throttling import ScopedRateThrottle from rest_framework.views import APIView from django.db.models import Count, Q from apps.core.permissions import IsAppStaff from apps.library.models import MediaItem +from .crypto import encrypt_secret from .models import User from .serializers import ( E621CredentialsSerializer, @@ -23,8 +26,17 @@ from .serializers import ( logger = logging.getLogger(__name__) +class LoginView(ObtainAuthToken): + """Token login, rate limited per IP to slow down credential stuffing.""" + + throttle_classes = [ScopedRateThrottle] + throttle_scope = "login" + + class RegisterView(APIView): permission_classes = [AllowAny] + throttle_classes = [ScopedRateThrottle] + throttle_scope = "register" def post(self, request): serializer = RegisterSerializer(data=request.data) @@ -64,7 +76,7 @@ class E621CredentialsView(APIView): def _payload(self, user): return { "username": user.e621_username, - "api_key": user.e621_api_key, + "api_key": user.e621_api_key_plain, "base_url": user.e621_base_url, "configured": user.e621_configured, } @@ -78,7 +90,7 @@ class E621CredentialsView(APIView): data = serializer.validated_data user = request.user user.e621_username = data["username"].strip() - user.e621_api_key = data["api_key"].strip() + user.e621_api_key = encrypt_secret(data["api_key"].strip()) user.e621_base_url = ( (data.get("base_url") or "https://e621.net").strip().rstrip("/") ) @@ -172,13 +184,28 @@ class UserViewSet( def update(self, request, *args, **kwargs): user = self.get_object() + actor = request.user serializer = UserUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) data = serializer.validated_data update_fields = [] if "role" in data: - user.role = data["role"] + new_role = data["role"] + # Same rule as account deletion: only admins may move accounts + # across the staff boundary (granting or revoking staff). + if ( + not actor.is_superuser + and new_role != user.role + and (user.is_app_staff or new_role == User.ROLE_STAFF) + ): + return Response( + { + "detail": "Only an admin can change staff roles.", + }, + status=status.HTTP_403_FORBIDDEN, + ) + user.role = new_role update_fields.append("role") if "avatar_j_id" in data: item, error = resolve_avatar_item(data.get("avatar_j_id")) diff --git a/backend/apps/library/e621.py b/backend/apps/library/e621.py index e76e0ac..98c4c50 100644 --- a/backend/apps/library/e621.py +++ b/backend/apps/library/e621.py @@ -59,7 +59,11 @@ def get(user, path, params=None, timeout=30, require_auth=True): response = requests.get( f"{base}{path}", params=params, - auth=(user.e621_username, user.e621_api_key) if configured else None, + auth=( + (user.e621_username, user.e621_api_key_plain) + if configured + else None + ), headers={"User-Agent": settings.USER_AGENT}, timeout=timeout, ) diff --git a/backend/apps/library/services.py b/backend/apps/library/services.py index 759fc99..9ca3374 100644 --- a/backend/apps/library/services.py +++ b/backend/apps/library/services.py @@ -255,6 +255,51 @@ class DownloadCancelled(Exception): """Raised when a streamed download is cancelled by the user.""" +class RemoteUrlError(ValueError): + """The URL is not an allowed e621 media URL.""" + + +def validate_remote_url(url): + """Only http(s) URLs on the known e621 media hosts may be fetched. + + Without this the download paths are an SSRF hole: any uploader could make + the server fetch internal addresses (127.0.0.1, LAN services, cloud + metadata) and read the response back through the library. + """ + from urllib.parse import urlparse + + parsed = urlparse(str(url or "").strip()) + if parsed.scheme not in {"http", "https"} or not parsed.hostname: + raise RemoteUrlError("Only http(s) URLs can be fetched.") + if parsed.hostname not in settings.E621_MEDIA_HOSTS: + raise RemoteUrlError("That host is not an allowed e621 media host.") + return url + + +def open_remote(url, *, max_redirects=3, **kwargs): + """GET an allowlisted URL, re-validating every redirect hop. + + Returns a streaming ``requests`` response. Redirects are followed + manually so a hop cannot jump to an internal host. + """ + import requests + from urllib.parse import urljoin + + current = url + for _ in range(max_redirects + 1): + validate_remote_url(current) + response = requests.get(current, allow_redirects=False, **kwargs) + if response.is_redirect or response.is_permanent_redirect: + location = response.headers.get("Location") + response.close() + if not location: + raise RemoteUrlError("The remote server redirected without a target.") + current = urljoin(current, location) + continue + return response + raise RemoteUrlError("Too many redirects from the remote server.") + + def download_file( url, destination, @@ -268,10 +313,8 @@ def download_file( worker: without it a hung socket would keep a job "downloading" forever and the cancel flag could never be observed. """ - import requests - headers = {"User-Agent": settings.USER_AGENT} - with requests.get( + with open_remote( url, headers=headers, stream=True, timeout=(10, read_timeout) ) as response: response.raise_for_status() diff --git a/backend/apps/library/tools.py b/backend/apps/library/tools.py index 8be258d..93861f9 100644 --- a/backend/apps/library/tools.py +++ b/backend/apps/library/tools.py @@ -8,7 +8,7 @@ from django.conf import settings from django.core.cache import cache from django.db.models import Count, Q from rest_framework import status -from rest_framework.permissions import AllowAny, IsAuthenticated +from rest_framework.permissions import AllowAny from rest_framework.response import Response from rest_framework.views import APIView @@ -120,7 +120,7 @@ def resolve_item(data): class ExactDuplicatesView(APIView): """Items whose content exists at more than one path.""" - permission_classes = [IsAuthenticated] + permission_classes = [CanUpload] def get(self, request): items = ( @@ -143,7 +143,7 @@ class ExactDuplicatesView(APIView): class VisualMatchesView(APIView): """Items visually similar to one library item.""" - permission_classes = [IsAuthenticated] + permission_classes = [CanUpload] def post(self, request): threshold = parse_threshold(request.data.get("threshold")) @@ -185,7 +185,7 @@ class VisualGroupsView(APIView): a personal library. Revisit with a bucketed index if libraries grow huge. """ - permission_classes = [IsAuthenticated] + permission_classes = [CanUpload] def post(self, request): threshold = parse_threshold(request.data.get("threshold")) @@ -450,7 +450,7 @@ def storage_info(): class StorageView(APIView): """Disk usage for the watched folder, media root and temp uploads.""" - permission_classes = [IsAuthenticated] + permission_classes = [CanUpload] def get(self, request): return Response(storage_info()) diff --git a/backend/apps/library/views.py b/backend/apps/library/views.py index 42b482c..6fc7fce 100644 --- a/backend/apps/library/views.py +++ b/backend/apps/library/views.py @@ -15,6 +15,7 @@ from rest_framework import mixins, status, viewsets from rest_framework.decorators import action from rest_framework.permissions import AllowAny, IsAuthenticatedOrReadOnly from rest_framework.response import Response +from rest_framework.throttling import ScopedRateThrottle from rest_framework.views import APIView from . import e621, matching, services @@ -428,9 +429,11 @@ class DownloadTaskViewSet( url = str(request.data.get("url") or "").strip() post_id = request.data.get("post_id") filename = str(request.data.get("filename") or "").strip() - if not url.startswith(("http://", "https://")): + try: + services.validate_remote_url(url) + except services.RemoteUrlError as exc: return Response( - {"detail": "A valid file URL is required."}, + {"detail": str(exc)}, status=status.HTTP_400_BAD_REQUEST, ) trimmed = services.trim_e621_post(request.data.get("post")) @@ -521,15 +524,15 @@ class ClientDownloadView(APIView): """Stream an e621 file straight to the browser (no library write).""" permission_classes = [AllowAny] + throttle_classes = [ScopedRateThrottle] + throttle_scope = "e621_proxy" def get(self, request): url = str(request.query_params.get("url") or "").strip() filename = str(request.query_params.get("filename") or "").strip() - parsed = urlparse(url) - if ( - parsed.scheme not in {"http", "https"} - or parsed.hostname not in settings.E621_MEDIA_HOSTS - ): + try: + services.validate_remote_url(url) + except services.RemoteUrlError: return Response( {"detail": "URL not allowed."}, status=status.HTTP_400_BAD_REQUEST, @@ -538,13 +541,17 @@ class ClientDownloadView(APIView): import requests try: - upstream = requests.get( + upstream = services.open_remote( url, headers={"User-Agent": settings.USER_AGENT}, stream=True, timeout=60, ) upstream.raise_for_status() + except services.RemoteUrlError as exc: + return Response( + {"detail": str(exc)}, status=status.HTTP_400_BAD_REQUEST + ) except requests.RequestException as exc: return Response( {"detail": f"Could not fetch the file: {exc}"}, diff --git a/backend/config/settings.py b/backend/config/settings.py index 5b55dd7..5c9521c 100644 --- a/backend/config/settings.py +++ b/backend/config/settings.py @@ -6,6 +6,7 @@ import os import subprocess from pathlib import Path +from django.core.exceptions import ImproperlyConfigured from dotenv import load_dotenv BASE_DIR = Path(__file__).resolve().parent.parent @@ -14,6 +15,12 @@ load_dotenv(BASE_DIR / ".env") SECRET_KEY = os.getenv("SECRET_KEY", "django-insecure-dev-only-change-me") DEBUG = os.getenv("DEBUG", "True").lower() == "true" +if not DEBUG and SECRET_KEY == "django-insecure-dev-only-change-me": + raise ImproperlyConfigured( + "SECRET_KEY is still the development default. Set a long random value " + "in backend/.env before running with DEBUG=False — it signs the media " + "URLs and encrypts stored e621 keys." + ) ALLOWED_HOSTS = [ host.strip() for host in os.getenv("ALLOWED_HOSTS", "localhost,127.0.0.1,0.0.0.0").split(",") @@ -234,6 +241,21 @@ REST_FRAMEWORK = { ], "DEFAULT_PAGINATION_CLASS": "config.pagination.StandardPagination", "PAGE_SIZE": 48, + # Per-IP/per-user rate limits (counted in the shared Redis cache). + "DEFAULT_THROTTLE_CLASSES": [ + "rest_framework.throttling.AnonRateThrottle", + "rest_framework.throttling.UserRateThrottle", + ], + "DEFAULT_THROTTLE_RATES": { + # Generous enough for the shell polling (status every 5s, stats every 2s). + "anon": os.getenv("THROTTLE_ANON", "120/min"), + "user": os.getenv("THROTTLE_USER", "600/min"), + # Credential stuffing / spam guards. + "login": os.getenv("THROTTLE_LOGIN", "5/min"), + "register": os.getenv("THROTTLE_REGISTER", "20/hour"), + # The guest e621 download proxy streams whole files. + "e621_proxy": os.getenv("THROTTLE_E621_PROXY", "60/hour"), + }, } DEFAULT_AUTO_FIELD = "django.db.models.BigAutoField" diff --git a/backend/requirements.txt b/backend/requirements.txt index 102799f..31dafb8 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -2,6 +2,7 @@ Django>=6.0,<6.2 djangorestframework>=3.17 django-filter>=25.1 django-cors-headers>=4.9 +cryptography>=46.0 Pillow>=11.0 python-dotenv>=1.0 requests>=2.32 diff --git a/frontend/src/features/users/UsersPage.tsx b/frontend/src/features/users/UsersPage.tsx index 2999b8a..790fc93 100644 --- a/frontend/src/features/users/UsersPage.tsx +++ b/frontend/src/features/users/UsersPage.tsx @@ -47,6 +47,18 @@ function UserRow({ profile }: { profile: AdminUser }) { // Mirrors the backend rules: nobody deletes themselves, and staff can only // delete accounts that are not staff/admin. + const targetIsStaff = Boolean( + profile.is_superuser || profile.is_staff || profile.role === "staff", + ); + const canChangeRole = Boolean( + actor && (actor.is_superuser || !targetIsStaff), + ); + // Staff cannot grant the staff role, so it is not even offered to them. + const roleOptions = + actor?.is_superuser || targetIsStaff + ? ROLE_OPTIONS + : ROLE_OPTIONS.filter((option) => option.value !== "staff"); + const canDelete = Boolean( actor && actor.id !== profile.id && @@ -108,10 +120,14 @@ function UserRow({ profile }: { profile: AdminUser }) { onChange={(event) => mutation.mutate({ role: event.target.value }) } - disabled={mutation.isPending} - title="Role" + disabled={mutation.isPending || !canChangeRole} + title={ + canChangeRole + ? "Role" + : "Only an admin can change staff roles" + } > - {ROLE_OPTIONS.map((option) => ( + {roleOptions.map((option) => (