From 3289be73830c4063beafcc5a039fd3d7bae3baa8 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Sat, 31 Oct 2026 09:32:58 -0700 Subject: [PATCH] Fix: Various small bugs found during typing updates (#14593) * Fix: match Apple Mail tag colors case-insensitively * Fix: reject OAuth callbacks with an unrecognized scope * Fix: refuse to refresh OAuth tokens for non-OAuth accounts or missing refresh tokens * Fix: parse datetime strings in the format_datetime filter and reject invalid ones * Fix: reject a PAPERLESS_URL without a hostname * Fix: report a database error in the system status instead of failing with a 500 * Fix: surface the database error when refreshing a document before moving its files * Fix: report no unapplied migrations when the applied migrations cannot be read * Fix: return the parsed value directly in format_datetime for typing * Chore: annotate the paperless_dirs fixture in the refresh failure test * Allow null for the latest_migration system status type --------- Co-authored-by: shamoon <4887959+shamoon@users.noreply.github.com> --- src-ui/src/app/data/system-status.ts | 2 +- src/documents/signals/handlers.py | 12 ++-- src/documents/templating/filters.py | 5 +- src/documents/tests/test_api_status.py | 47 +++++++++++++++ src/documents/tests/test_file_handling.py | 26 +++++++++ src/documents/tests/test_filters.py | 21 +++++++ src/documents/views.py | 13 ++++- src/paperless/settings/__init__.py | 7 ++- src/paperless/tests/settings/test_settings.py | 19 ++++++ src/paperless_mail/mail.py | 4 +- src/paperless_mail/oauth.py | 6 ++ src/paperless_mail/tests/test_mail.py | 27 +++++++++ src/paperless_mail/tests/test_mail_oauth.py | 58 +++++++++++++++++++ src/paperless_mail/views.py | 8 +++ 14 files changed, 241 insertions(+), 14 deletions(-) diff --git a/src-ui/src/app/data/system-status.ts b/src-ui/src/app/data/system-status.ts index 41cc738cc..f2a8f90d2 100644 --- a/src-ui/src/app/data/system-status.ts +++ b/src-ui/src/app/data/system-status.ts @@ -24,7 +24,7 @@ export interface SystemStatus { status: SystemStatusItemStatus error?: string migration_status: { - latest_migration: string + latest_migration: string | null unapplied_migrations: string[] } } diff --git a/src/documents/signals/handlers.py b/src/documents/signals/handlers.py index f6a318c0e..9c20e97f2 100644 --- a/src/documents/signals/handlers.py +++ b/src/documents/signals/handlers.py @@ -476,13 +476,13 @@ def update_filename_and_move_files( return with FileLock(settings.MEDIA_LOCK): - try: - # If this was waiting for the lock, the filename or archive_filename - # of this document may have been updated. This happens if multiple updates - # get queued from the UI for the same document - # So freshen up the data before doing anything - instance.refresh_from_db() + # If this was waiting for the lock, the filename or archive_filename + # of this document may have been updated. This happens if multiple updates + # get queued from the UI for the same document + # So freshen up the data before doing anything + instance.refresh_from_db() + try: old_filename = instance.filename old_source_path = instance.source_path move_original = False diff --git a/src/documents/templating/filters.py b/src/documents/templating/filters.py index 698746c76..ac065b34c 100644 --- a/src/documents/templating/filters.py +++ b/src/documents/templating/filters.py @@ -44,7 +44,10 @@ def localize_date(value: date | datetime | str, format: str, locale: str) -> str def format_datetime(value: str | datetime, format: str) -> str: if isinstance(value, str): - value = parse_date(value) + parsed = parse_datetime(value) or parse_date(value) + if parsed is None: + raise ValueError(f"Invalid date or datetime string: {value!r}") + return parsed.strftime(format=format) return value.strftime(format=format) diff --git a/src/documents/tests/test_api_status.py b/src/documents/tests/test_api_status.py index 6f200e54f..2e458fa80 100644 --- a/src/documents/tests/test_api_status.py +++ b/src/documents/tests/test_api_status.py @@ -539,3 +539,50 @@ class TestSystemStatus(APITestCase): self.assertEqual(response.data["tasks"]["summary"]["success_count"], 1) self.assertEqual(response.data["tasks"]["summary"]["failure_count"], 1) self.assertEqual(response.data["tasks"]["summary"]["pending_count"], 1) + + def test_system_status_reports_database_error(self) -> None: + """ + GIVEN: + - Reading the migration state fails + WHEN: + - The system status is requested + THEN: + - The response reports the database error rather than a server error + """ + self.client.force_login(self.user) + with mock.patch( + "documents.views.MigrationLoader", + side_effect=Exception("database is down"), + ): + response = self.client.get(self.ENDPOINT) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + database = response.data["database"] + self.assertEqual(database["status"], "ERROR") + self.assertIsNotNone(database["error"]) + self.assertEqual( + database["migration_status"], + {"latest_migration": None, "unapplied_migrations": []}, + ) + + def test_system_status_migration_query_error(self) -> None: + """ + GIVEN: + - The migration graph loads but reading the applied migrations fails + WHEN: + - The system status is requested + THEN: + - No migrations are reported as unapplied, since the state is unknown + """ + self.client.force_login(self.user) + with mock.patch("documents.views.MigrationRecorder") as recorder: + recorder.Migration.objects.all.side_effect = Exception("query failed") + response = self.client.get(self.ENDPOINT) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + database = response.data["database"] + self.assertEqual(database["status"], "ERROR") + self.assertEqual( + database["migration_status"], + {"latest_migration": None, "unapplied_migrations": []}, + ) diff --git a/src/documents/tests/test_file_handling.py b/src/documents/tests/test_file_handling.py index 46c54f98f..fd6d2c85c 100644 --- a/src/documents/tests/test_file_handling.py +++ b/src/documents/tests/test_file_handling.py @@ -31,6 +31,7 @@ from documents.serialisers import DocumentSerializer from documents.tasks import empty_trash from paperless_testing.assertions import FileSystemAssertsMixin from paperless_testing.dirs import DirectoriesMixin +from paperless_testing.dirs import PaperlessDirs from paperless_testing.factories import DocumentFactory from paperless_testing.factories import UserFactory @@ -2134,3 +2135,28 @@ class TestPathDateLocalization: with override_settings(FILENAME_FORMAT=filename_format): filename = generate_filename(document) assert filename == Path(expected_filename) + + +@pytest.mark.django_db +def test_refresh_failure_surfaces_database_error( + paperless_dirs: PaperlessDirs, +) -> None: + """ + GIVEN: + - A document with a stored filename + WHEN: + - Saving it triggers a filename update and refreshing it from the database fails + THEN: + - The database error is raised, not an unbound variable error from the rollback + """ + document = DocumentFactory(filename="some.pdf") + + with ( + mock.patch.object( + Document, + "refresh_from_db", + side_effect=DatabaseError("database is locked"), + ), + pytest.raises(DatabaseError, match="database is locked"), + ): + document.save() diff --git a/src/documents/tests/test_filters.py b/src/documents/tests/test_filters.py index a7ecbf1ce..7625b474e 100644 --- a/src/documents/tests/test_filters.py +++ b/src/documents/tests/test_filters.py @@ -4,6 +4,7 @@ from typing import Literal import pytest +from documents.templating.filters import format_datetime from documents.templating.filters import localize_date @@ -294,3 +295,23 @@ class TestDateLocalization: Tests `localize_date` with `date` string across different locales and formats. """ assert localize_date(value, format_style, locale_str) == expected_output + + +class TestFormatDatetime: + @pytest.mark.parametrize( + ("value", "expected"), + [ + pytest.param("2026-09-21", "2026-09-21 00:00", id="date"), + pytest.param( + "2026-09-21T13:45:00+00:00", + "2026-09-21 13:45", + id="datetime", + ), + ], + ) + def test_parses_iso_strings(self, value: str, expected: str) -> None: + assert format_datetime(value, "%Y-%m-%d %H:%M") == expected + + def test_rejects_unparseable_string(self) -> None: + with pytest.raises(ValueError, match="not a date"): + format_datetime("not a date", "%Y") diff --git a/src/documents/views.py b/src/documents/views.py index 51dab2ecd..aeed498cb 100644 --- a/src/documents/views.py +++ b/src/documents/views.py @@ -5268,7 +5268,9 @@ class CustomFieldViewSet(PermissionsAwareDocumentCountMixin, ModelViewSet[Custom "migration_status": inline_serializer( name="MigrationStatus", fields={ - "latest_migration": serializers.CharField(), + "latest_migration": serializers.CharField( + allow_null=True, + ), "unapplied_migrations": serializers.ListSerializer( child=serializers.CharField(), ), @@ -5343,6 +5345,8 @@ class SystemStatusView(PassUserMixin): db_conn = connections["default"] db_url = str(db_conn.settings_dict["NAME"]) db_error = None + all_migrations: list[str] = [] + applied_migrations: list[str] = [] try: db_conn.ensure_connection() @@ -5353,7 +5357,8 @@ class SystemStatusView(PassUserMixin): f"{m.app}.{m.name}" for m in MigrationRecorder.Migration.objects.all().order_by("id") ] - except Exception as e: # pragma: no cover + except Exception as e: + all_migrations = [] applied_migrations = [] db_status = "ERROR" logger.exception( @@ -5549,7 +5554,9 @@ class SystemStatusView(PassUserMixin): "status": db_status, "error": db_error, "migration_status": { - "latest_migration": applied_migrations[-1], + "latest_migration": ( + applied_migrations[-1] if applied_migrations else None + ), "unapplied_migrations": [ m for m in all_migrations if m not in applied_migrations ], diff --git a/src/paperless/settings/__init__.py b/src/paperless/settings/__init__.py index 4fe51bda1..b3e2b71ab 100644 --- a/src/paperless/settings/__init__.py +++ b/src/paperless/settings/__init__.py @@ -461,9 +461,14 @@ def _parse_paperless_url(): global CSRF_TRUSTED_ORIGINS, CORS_ALLOWED_ORIGINS, ALLOWED_HOSTS url = os.getenv("PAPERLESS_URL") if url: + hostname = urlparse(url).hostname + if not hostname: + raise ImproperlyConfigured( + f"PAPERLESS_URL {url!r} has no hostname, expected a URL like https://example.com", + ) CSRF_TRUSTED_ORIGINS.append(url) CORS_ALLOWED_ORIGINS.append(url) - ALLOWED_HOSTS.append(urlparse(url).hostname) + ALLOWED_HOSTS.append(hostname) return url diff --git a/src/paperless/tests/settings/test_settings.py b/src/paperless/tests/settings/test_settings.py index 701f203f1..be218a3d7 100644 --- a/src/paperless/tests/settings/test_settings.py +++ b/src/paperless/tests/settings/test_settings.py @@ -166,3 +166,22 @@ class TestPaperlessURLSettings(TestCase): self.assertIn(url, settings.CSRF_TRUSTED_ORIGINS) self.assertIn(url, settings.CORS_ALLOWED_ORIGINS) + + +@pytest.mark.parametrize("url", ["example.com", "https://", "/just/a/path"]) +def test_paperless_url_without_hostname_is_rejected( + monkeypatch: pytest.MonkeyPatch, + url: str, +) -> None: + """ + GIVEN: + - PAPERLESS_URL is set to a value with no hostname + WHEN: + - The URL is parsed + THEN: + - A configuration error is raised instead of adding None to ALLOWED_HOSTS + """ + monkeypatch.setenv("PAPERLESS_URL", url) + + with pytest.raises(ImproperlyConfigured, match="no hostname"): + _parse_paperless_url() diff --git a/src/paperless_mail/mail.py b/src/paperless_mail/mail.py index fb8568e8f..f5f3ffeef 100644 --- a/src/paperless_mail/mail.py +++ b/src/paperless_mail/mail.py @@ -169,9 +169,9 @@ class TagMailAction(BaseMailAction): # The custom tag should look like "apple:" if "apple:" in parameter.lower(): _, self.color = parameter.split(":") - self.color = self.color.strip() + self.color = self.color.strip().lower() - if self.color.lower() not in APPLE_MAIL_TAG_COLORS: + if self.color not in APPLE_MAIL_TAG_COLORS: raise MailError("Not a valid AppleMail tag color.") self.keyword = None diff --git a/src/paperless_mail/oauth.py b/src/paperless_mail/oauth.py index 08b5d9647..30c9b8933 100644 --- a/src/paperless_mail/oauth.py +++ b/src/paperless_mail/oauth.py @@ -89,6 +89,9 @@ class PaperlessMailOAuth2Manager: """ logger = logging.getLogger("paperless_mail") logger.debug(f"Attempting to refresh oauth token for account {account}") + if account.refresh_token is None: + logger.error(f"Account {account} has no refresh token to use") + return False try: result: OAuth2Token if account.account_type == MailAccount.MailAccountType.GMAIL_OAUTH: @@ -103,6 +106,9 @@ class PaperlessMailOAuth2Manager: refresh_token=account.refresh_token, ), ) + else: + logger.error(f"Account {account} does not use OAuth") + return False if "refresh_token" in result: # Outlook returns a new refresh token on refresh, Gmail does not account.refresh_token = result["refresh_token"] diff --git a/src/paperless_mail/tests/test_mail.py b/src/paperless_mail/tests/test_mail.py index 56c2da467..366fe0e64 100644 --- a/src/paperless_mail/tests/test_mail.py +++ b/src/paperless_mail/tests/test_mail.py @@ -23,6 +23,7 @@ from rest_framework.test import APITestCase from documents.models import Correspondent from documents.models import MatchingModel from paperless_mail import tasks +from paperless_mail.mail import APPLE_MAIL_TAG_COLORS from paperless_mail.mail import MailAccountHandler from paperless_mail.mail import MailError from paperless_mail.mail import TagMailAction @@ -1873,6 +1874,32 @@ class TestErrorCallback: assert processed_mails.get().owner == owner +class TestTagMailActionColor: + @pytest.mark.parametrize( + "parameter", + ["apple:green", "apple:Green", "APPLE: GREEN "], + ) + def test_color_case_is_ignored(self, parameter: str) -> None: + """ + GIVEN: + - An Apple Mail tag parameter in any letter case + WHEN: + - The action tags a message + THEN: + - The lowercase color's flag bits are set on the message + """ + action = TagMailAction(parameter, supports_gmail_labels=False) + mailbox = mock.Mock() + + action.post_consume(mailbox, "1", parameter) + + assert mailbox.flag.call_args_list[1] == mock.call( + "1", + APPLE_MAIL_TAG_COLORS["green"], + value=True, + ) + + class TestManagementCommand(TestCase): @mock.patch( "paperless_mail.management.commands.mail_fetcher.tasks.process_mail_accounts", diff --git a/src/paperless_mail/tests/test_mail_oauth.py b/src/paperless_mail/tests/test_mail_oauth.py index c3ba99ce9..8e0768a88 100644 --- a/src/paperless_mail/tests/test_mail_oauth.py +++ b/src/paperless_mail/tests/test_mail_oauth.py @@ -264,6 +264,28 @@ class TestMailOAuth( MailAccount.objects.filter(imap_server="outlook.office365.com").exists(), ) + def test_oauth_callback_view_unrecognized_scope(self) -> None: + """ + GIVEN: + - A valid OAuth state and a callback scope that is not Google's + WHEN: + - OAuth callback is called + THEN: + - 400 is returned instead of a server error and no account is created + """ + session = self.client.session + session["oauth_state"] = "test_state" + session.save() + + for scope in ("", "openid", "https://example.com/other"): + with self.subTest(scope=scope): + response = self.client.get( + "/api/oauth/callback/", + {"code": "test_code", "scope": scope, "state": "test_state"}, + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertFalse(MailAccount.objects.exists()) + @mock.patch("paperless_mail.mail.get_mailbox") @mock.patch( "httpx_oauth.oauth2.BaseOAuth2.refresh_token", @@ -366,3 +388,39 @@ class TestMailOAuth( f"Failed to refresh oauth token for account {mail_account}: test_error", cm.output[0], ) + + def test_refresh_account_oauth_token_non_oauth_account(self) -> None: + """ + GIVEN: + - A mail account which does not use OAuth + WHEN: + - A token refresh is requested + THEN: + - False is returned rather than an unbound variable error + """ + account = MailAccountFactory( + account_type=MailAccount.MailAccountType.IMAP, + refresh_token="token", + ) + + self.assertFalse( + PaperlessMailOAuth2Manager().refresh_account_oauth_token(account), + ) + + def test_refresh_account_oauth_token_without_refresh_token(self) -> None: + """ + GIVEN: + - An OAuth mail account with no refresh token stored + WHEN: + - A token refresh is requested + THEN: + - False is returned + """ + account = MailAccountFactory( + account_type=MailAccount.MailAccountType.GMAIL_OAUTH, + refresh_token=None, + ) + + self.assertFalse( + PaperlessMailOAuth2Manager().refresh_account_oauth_token(account), + ) diff --git a/src/paperless_mail/views.py b/src/paperless_mail/views.py index ce1f6a079..1da81cf66 100644 --- a/src/paperless_mail/views.py +++ b/src/paperless_mail/views.py @@ -319,6 +319,14 @@ class OauthCallbackView(GenericAPIView[Any]): result = oauth_manager.get_outlook_access_token(code) + else: + logger.error( + f"Invalid oauth callback request: unrecognized scope {scope!r}", + ) + return HttpResponseBadRequest( + "Invalid request, see logs for more detail", + ) + access_token = result["access_token"] refresh_token = result["refresh_token"] expires_in = result["expires_in"]