From 6cf8cbf786c33888845918d1c98dffd9db537cf8 Mon Sep 17 00:00:00 2001 From: Asish Kumar Date: Sat, 7 Nov 2026 14:56:55 +0530 Subject: [PATCH] fix(import): stop hardcoding the Docker image's paths Import uploads were always written to /usr/src/app/temp, presets were read from /usr/src/app/import_presets and the caches lived in /var/tmp, so WYGIWYH only worked from the Docker image's layout. Running it as a systemd service with a strict sandbox, or on an immutable distro like NixOS, needs these folders to follow the app or be configurable. - Presets are read from IMPORT_PRESETS_ROOT, the import_presets folder next to the app. - Uploads wait in IMPORT_TEMP_ROOT, the temp folder next to the app by default, and movable with the IMPORT_TEMP_ROOT env var. The web views, the API and the import service all use it. - The file-based caches live under CACHE_ROOT, /var/tmp by default, and movable with the CACHE_ROOT env var. In the Docker image the app sits in /usr/src/app, so every default resolves to the same folder as before. The import service checked that a file was inside the temp folder with a plain string prefix, which also accepted sibling folders such as /usr/src/app/temp-other. It now compares paths. CI no longer needs to create /usr/src/app before running the tests. Fixes #472 --- .github/workflows/tests.yml | 6 - .gitignore | 1 + README.md | 2 + app/WYGIWYH/settings.py | 9 +- app/apps/api/views/imports.py | 3 +- app/apps/import_app/services/presets.py | 6 +- app/apps/import_app/services/v1.py | 12 +- app/apps/import_app/tests/test_paths.py | 151 +++++++++++++++++++ app/apps/import_app/tests/test_qif_import.py | 10 +- app/apps/import_app/views.py | 3 +- 10 files changed, 180 insertions(+), 23 deletions(-) create mode 100644 app/apps/import_app/tests/test_paths.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 2a704fa5..038881a1 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -60,12 +60,6 @@ jobs: - name: Install dependencies run: uv sync --frozen - # Import code reads/writes the Docker image's paths directly - - name: Create container paths used by the app - run: | - sudo mkdir -p /usr/src/app/temp /usr/src/app/import_presets - sudo chown -R "$(id -u):$(id -g)" /usr/src/app - - name: Check for missing migrations working-directory: app run: uv run python manage.py makemigrations --check --dry-run diff --git a/.gitignore b/.gitignore index 93ba54a4..7c4160ef 100644 --- a/.gitignore +++ b/.gitignore @@ -168,3 +168,4 @@ postgres_data/ # Private local uploads app/attachments/ +app/temp/ diff --git a/README.md b/README.md index 10bde2d3..f757b874 100644 --- a/README.md +++ b/README.md @@ -149,6 +149,8 @@ To create the first user, open the container's console using Unraid's UI, by cli | DJANGO_VITE_DEV_MODE | true\|false | false | Enables Vite dev server mode for frontend development. When true, assets are served from Vite's dev server instead of the build manifest. For development only! | | DJANGO_VITE_DEV_SERVER_PORT | int | 5173 | The port where Vite's dev server is running. Only used when DJANGO_VITE_DEV_MODE is true. For development only! | | DJANGO_VITE_DEV_SERVER_HOST | string | localhost | The host where Vite's dev server is running. Only used when DJANGO_VITE_DEV_MODE is true. For development only! | +| IMPORT_TEMP_ROOT | string | temp, in the app folder | Where uploaded import files wait to be processed. Must be writable and shared between the web and task processes. | +| CACHE_ROOT | string | /var/tmp | The writable folder the file-based caches are stored in. | ## OIDC Configuration diff --git a/app/WYGIWYH/settings.py b/app/WYGIWYH/settings.py index f7756b73..44f2e9e5 100644 --- a/app/WYGIWYH/settings.py +++ b/app/WYGIWYH/settings.py @@ -313,6 +313,9 @@ LOCALE_PATHS = [BASE_DIR / "locale"] STATIC_URL = "static/" STATIC_ROOT = BASE_DIR / "static_files" ATTACHMENT_MEDIA_ROOT = BASE_DIR / "attachments" +IMPORT_PRESETS_ROOT = BASE_DIR / "import_presets" +# Web and task workers must see the same folder: uploads wait here to be processed +IMPORT_TEMP_ROOT = Path(os.getenv("IMPORT_TEMP_ROOT") or BASE_DIR / "temp").resolve() STATICFILES_DIRS = [ ROOT_DIR / "frontend" / "build", @@ -324,15 +327,17 @@ STATICFILES_FINDERS = [ "django.contrib.staticfiles.finders.AppDirectoriesFinder", ] +CACHE_ROOT = Path(os.getenv("CACHE_ROOT") or "/var/tmp").resolve() + CACHES = { "default": { "BACKEND": "django.core.cache.backends.filebased.FileBasedCache", - "LOCATION": "/var/tmp/django_cache", + "LOCATION": CACHE_ROOT / "django_cache", }, # Kept apart from "default": cachalot fills it and FileBasedCache culls random entries past MAX_ENTRIES "update_check": { "BACKEND": "django.core.cache.backends.filebased.FileBasedCache", - "LOCATION": "/var/tmp/django_cache_update_check", + "LOCATION": CACHE_ROOT / "django_cache_update_check", }, } diff --git a/app/apps/api/views/imports.py b/app/apps/api/views/imports.py index f8e6edb7..9abb945d 100644 --- a/app/apps/api/views/imports.py +++ b/app/apps/api/views/imports.py @@ -1,3 +1,4 @@ +from django.conf import settings from django.core.files.storage import FileSystemStorage from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import OpenApiParameter, extend_schema, extend_schema_view, inline_serializer @@ -130,7 +131,7 @@ class ImportViewSet(viewsets.ViewSet): uploaded_file = serializer.validated_data["file"] # Save file to temp location - fs = FileSystemStorage(location="/usr/src/app/temp") + fs = FileSystemStorage(location=settings.IMPORT_TEMP_ROOT) filename = fs.save(uploaded_file.name, uploaded_file) file_path = fs.path(filename) diff --git a/app/apps/import_app/services/presets.py b/app/apps/import_app/services/presets.py index 824a2468..e4cef311 100644 --- a/app/apps/import_app/services/presets.py +++ b/app/apps/import_app/services/presets.py @@ -1,17 +1,17 @@ import json from pathlib import Path +from django.conf import settings + from apps.import_app.models import ImportProfile class PresetService: - PRESET_PATH = "/usr/src/app/import_presets" - @classmethod def get_all_presets(cls): presets = [] - for folder in Path(cls.PRESET_PATH).iterdir(): + for folder in settings.IMPORT_PRESETS_ROOT.iterdir(): if folder.is_dir(): manifest_path = folder / "manifest.json" config_path = folder / "config.yml" diff --git a/app/apps/import_app/services/v1.py b/app/apps/import_app/services/v1.py index bed860ac..5aad0b9e 100644 --- a/app/apps/import_app/services/v1.py +++ b/app/apps/import_app/services/v1.py @@ -7,12 +7,14 @@ import zipfile from django.db import transaction from datetime import datetime, date from decimal import Decimal, InvalidOperation +from pathlib import Path from typing import Dict, Any, Literal, Union import openpyxl import xlrd import yaml from cachalot.api import cachalot_disabled +from django.conf import settings from django.core.exceptions import FieldDoesNotExist from django.utils import timezone from openpyxl.utils.exceptions import InvalidFileException @@ -39,8 +41,6 @@ logger = logging.getLogger(__name__) class ImportService: - TEMP_DIR = "/usr/src/app/temp" - def __init__(self, import_run: ImportRun): self.import_run: ImportRun = import_run self.profile: ImportProfile = import_run.profile @@ -54,7 +54,7 @@ class ImportService: self.mapping: Dict[str, version_1.ColumnMapping] = self.config.mapping # Ensure temp directory exists - os.makedirs(self.TEMP_DIR, exist_ok=True) + os.makedirs(settings.IMPORT_TEMP_ROOT, exist_ok=True) def _load_config(self) -> version_1.ImportProfileSchema: yaml_data = yaml.safe_load(self.profile.yaml_config) @@ -1132,8 +1132,10 @@ class ImportService: Returns the absolute path. """ abs_path = os.path.abspath(file_path) - if not abs_path.startswith(self.TEMP_DIR): - raise ValueError(f"Invalid file path. File must be in {self.TEMP_DIR}") + if not Path(abs_path).is_relative_to(settings.IMPORT_TEMP_ROOT): + raise ValueError( + f"Invalid file path. File must be in {settings.IMPORT_TEMP_ROOT}" + ) return abs_path def process_file(self, file_path: str): diff --git a/app/apps/import_app/tests/test_paths.py b/app/apps/import_app/tests/test_paths.py new file mode 100644 index 00000000..33c6d32d --- /dev/null +++ b/app/apps/import_app/tests/test_paths.py @@ -0,0 +1,151 @@ +"""Tests for the folders import code reads from and writes to.""" + +import json +import os +import runpy +import shutil +import tempfile +from pathlib import Path +from unittest.mock import patch + +from django.conf import settings +from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import SimpleTestCase, override_settings +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient + +from apps.import_app.models import ImportRun +from apps.import_app.services import PresetService +from apps.import_app.services.v1 import ImportService +from apps.import_app.tests.test_view_permissions import ( + HTMX, + ImportObjectPermissionTestCase, +) + + +def load_settings(**env): + """Evaluates the settings module from scratch under the given env vars.""" + with patch.dict(os.environ, env): + return runpy.run_path(str(Path(settings.BASE_DIR) / "WYGIWYH" / "settings.py")) + + +class PathSettingsTests(SimpleTestCase): + def test_defaults_match_the_docker_image_layout(self): + loaded = load_settings(IMPORT_TEMP_ROOT="", CACHE_ROOT="") + + base_dir = loaded["BASE_DIR"] + self.assertEqual(loaded["IMPORT_TEMP_ROOT"], base_dir / "temp") + self.assertEqual(loaded["IMPORT_PRESETS_ROOT"], base_dir / "import_presets") + self.assertEqual( + loaded["CACHES"]["default"]["LOCATION"], + Path("/var/tmp/django_cache").resolve(), + ) + self.assertEqual( + loaded["CACHES"]["update_check"]["LOCATION"], + Path("/var/tmp/django_cache_update_check").resolve(), + ) + + def test_env_vars_move_temp_and_cache_folders(self): + with tempfile.TemporaryDirectory() as root: + root = Path(root).resolve() + loaded = load_settings( + IMPORT_TEMP_ROOT=str(root / "uploads"), CACHE_ROOT=str(root / "cache") + ) + + self.assertEqual(loaded["IMPORT_TEMP_ROOT"], root / "uploads") + self.assertEqual( + loaded["CACHES"]["default"]["LOCATION"], root / "cache" / "django_cache" + ) + self.assertEqual( + loaded["CACHES"]["update_check"]["LOCATION"], + root / "cache" / "django_cache_update_check", + ) + + +class PresetServiceTests(SimpleTestCase): + def test_lists_the_presets_shipped_with_the_app(self): + folder_names = { + preset["folder_name"] for preset in PresetService.get_all_presets() + } + + self.assertIn("qif_standard", folder_names) + + def test_reads_presets_from_configured_folder(self): + with tempfile.TemporaryDirectory() as root: + preset = Path(root) / "my_bank" + preset.mkdir() + (preset / "manifest.json").write_text(json.dumps({"name": "My Bank"})) + (preset / "config.yml").write_text("settings: {}") + + with override_settings(IMPORT_PRESETS_ROOT=Path(root)): + presets = PresetService.get_all_presets() + + self.assertEqual([p["name"] for p in presets], ["My Bank"]) + + +class ImportTempRootTests(ImportObjectPermissionTestCase): + def setUp(self): + super().setUp() + self.temp_root = Path(tempfile.mkdtemp()).resolve() + self.addCleanup(shutil.rmtree, self.temp_root, ignore_errors=True) + temp_root_override = override_settings(IMPORT_TEMP_ROOT=self.temp_root) + temp_root_override.enable() + self.addCleanup(temp_root_override.disable) + + @patch("apps.import_app.views.process_import.defer") + def test_web_upload_is_stored_in_temp_root(self, mock_defer): + self.login(self.owner) + response = self.client.post( + reverse("import_run_add", kwargs={"profile_id": self.private_profile.id}), + {"file": SimpleUploadedFile("web.csv", b"date,amount\n")}, + **HTMX, + ) + + self.assertEqual(response.status_code, 204) + self.assertEqual( + mock_defer.call_args.kwargs["file_path"], str(self.temp_root / "web.csv") + ) + self.assertTrue((self.temp_root / "web.csv").is_file()) + + @patch("apps.api.views.imports.process_import.defer") + def test_api_upload_is_stored_in_temp_root(self, mock_defer): + client = APIClient() + client.force_authenticate(user=self.owner) + response = client.post( + "/api/import/import/", + { + "profile_id": self.private_profile.id, + "file": SimpleUploadedFile("api.csv", b"date,amount\n"), + }, + format="multipart", + ) + + self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED) + self.assertEqual( + mock_defer.call_args.kwargs["file_path"], str(self.temp_root / "api.csv") + ) + self.assertTrue((self.temp_root / "api.csv").is_file()) + + def _service(self): + run = ImportRun.objects.create( + profile=self.private_profile, file_name="file.csv", owner=self.owner + ) + return ImportService(run) + + def test_service_accepts_files_inside_temp_root(self): + path = str(self.temp_root / "file.csv") + + self.assertEqual(self._service()._validate_file_path(path), path) + + def test_service_rejects_files_outside_temp_root(self): + service = self._service() + + for path in ( + "/etc/passwd", + self.temp_root / ".." / "file.csv", + # Shares the temp root as a string prefix, but is a different folder + f"{self.temp_root}-other/file.csv", + ): + with self.subTest(path=path), self.assertRaises(ValueError): + service._validate_file_path(str(path)) diff --git a/app/apps/import_app/tests/test_qif_import.py b/app/apps/import_app/tests/test_qif_import.py index a79662b3..d2b7e9db 100644 --- a/app/apps/import_app/tests/test_qif_import.py +++ b/app/apps/import_app/tests/test_qif_import.py @@ -1,7 +1,7 @@ from decimal import Decimal import os import shutil -from django.test import TestCase +from django.test import TestCase, override_settings from django.contrib.auth import get_user_model from apps.accounts.models import Account, AccountGroup from apps.currencies.models import Currency @@ -15,10 +15,11 @@ from apps.transactions.models import ( class QIFImportTests(TestCase): def setUp(self): - # Patch TEMP_DIR for testing - self.original_temp_dir = ImportService.TEMP_DIR + # Point the import temp folder somewhere disposable for testing self.test_dir = os.path.abspath("temp_test_import") - ImportService.TEMP_DIR = self.test_dir + temp_root_override = override_settings(IMPORT_TEMP_ROOT=self.test_dir) + temp_root_override.enable() + self.addCleanup(temp_root_override.disable) os.makedirs(self.test_dir, exist_ok=True) # Create user and set context @@ -41,7 +42,6 @@ class QIFImportTests(TestCase): def tearDown(self): delete_current_user() - ImportService.TEMP_DIR = self.original_temp_dir if os.path.exists(self.test_dir): shutil.rmtree(self.test_dir) diff --git a/app/apps/import_app/views.py b/app/apps/import_app/views.py index adef7679..45dc61ab 100644 --- a/app/apps/import_app/views.py +++ b/app/apps/import_app/views.py @@ -1,5 +1,6 @@ import shutil +from django.conf import settings from django.contrib import messages from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied @@ -204,7 +205,7 @@ def import_run_add(request, profile_id): if form.is_valid(): uploaded_file = request.FILES["file"] - fs = FileSystemStorage(location="/usr/src/app/temp") + fs = FileSystemStorage(location=settings.IMPORT_TEMP_ROOT) filename = fs.save(uploaded_file.name, uploaded_file) file_path = fs.path(filename)