From 144a90fa73978ee915020324a55dcb15d7f00497 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 2 Mar 2026 14:01:26 +0000 Subject: [PATCH] fix(pdfa): address code review - validate pdfa_format, add S3 comment, add format test Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- app/tasks/convert_to_pdfa.py | 8 +++++++- tests/test_convert_to_pdfa.py | 5 +++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/tasks/convert_to_pdfa.py b/app/tasks/convert_to_pdfa.py index 2fd7452a..b94ae74a 100644 --- a/app/tasks/convert_to_pdfa.py +++ b/app/tasks/convert_to_pdfa.py @@ -59,6 +59,11 @@ def _convert_pdf_to_pdfa(input_path: str, output_path: str, pdfa_format: str = " Returns: True if conversion succeeded, False otherwise. """ + # Validate format to prevent argument injection via output-type + if pdfa_format not in ("1", "2", "3"): + logger.error(f"[convert_to_pdfa] Invalid pdfa_format: {pdfa_format}") + return False + ocrmypdf_bin = shutil.which("ocrmypdf") if not ocrmypdf_bin: logger.error("[convert_to_pdfa] ocrmypdf binary not found on PATH") @@ -186,7 +191,8 @@ def _compute_pdfa_folder_overrides() -> dict[str, str]: base = getattr(settings, folder_attr, "") or "" overrides[provider] = f"{base.rstrip('/')}/{subfolder}" if base else subfolder - # S3: append subfolder to prefix + # S3: append subfolder to prefix (trailing slash is required by S3 convention + # where "folder" paths are key prefixes, unlike path-based providers above) s3_prefix = getattr(settings, "s3_folder_prefix", "") or "" overrides["s3"] = f"{s3_prefix.rstrip('/')}/{subfolder}/" diff --git a/tests/test_convert_to_pdfa.py b/tests/test_convert_to_pdfa.py index 3a5e8880..12ee6243 100644 --- a/tests/test_convert_to_pdfa.py +++ b/tests/test_convert_to_pdfa.py @@ -73,6 +73,11 @@ class TestConvertPdfToPdfa: cmd = mock_run.call_args[0][0] assert f"pdfa-{fmt}" in cmd + def test_invalid_pdfa_format_rejected(self): + """Test that invalid PDF/A format values are rejected.""" + result = _convert_pdf_to_pdfa("/input.pdf", "/output.pdf", "invalid") + assert result is False + @patch("app.tasks.convert_to_pdfa.subprocess.run") @patch("app.tasks.convert_to_pdfa.shutil.which", return_value="/usr/bin/ocrmypdf") def test_conversion_failure_empty_stderr(self, mock_which, mock_run):