diff --git a/functions/submit_order/handler.py b/functions/submit_order/handler.py index c2c3515..e48d871 100644 --- a/functions/submit_order/handler.py +++ b/functions/submit_order/handler.py @@ -1,6 +1,7 @@ import json import logging import os +import re import sys import time import urllib.error @@ -34,6 +35,7 @@ if not logger.handlers: EASTERN = ZoneInfo("America/New_York") CACHE_TTL_SECONDS = 300 # 5-minute TTL for cached config values +PRESIGNED_URL_TTL_SECONDS = 300 # summary-PDF presigned URL lifetime ALLOWED_DOMAINS = {"seahavenind.com", "seahaven.com"} @@ -48,6 +50,7 @@ _settings_ts = 0.0 _google_client_id = None _google_client_id_ts = 0.0 _lambda = boto3.client("lambda") +_s3 = boto3.client("s3") def _get_api_key() -> str: @@ -262,18 +265,28 @@ def handle_admin_summary_pdf(event): qs = event.get("queryStringParameters") or {} week = qs.get("week", "") - if not week: - return response(400, {"error": "week query param is required"}) + if not re.match(r"^\d{4}-W\d{2}$", week): + return response(400, {"error": "week query param must be YYYY-WNN"}) summary = get_summary(week) - pdf_key = (summary or {}).get("weekly_summary_pdf_s3_key") - if not pdf_key: + if summary is None: + return response(404, {"error": "No summary PDF: week not found or not closed"}) + pdf_key = summary.get("weekly_summary_pdf_s3_key", "") + # The key comes from a DynamoDB record; only presign keys matching the + # shape aggregate_orders writes, so a tampered record can't expose other + # report files (e.g. payroll CSVs). + if not re.match( + r"^reports/\d{4}-W\d{2}/weekly-summary-\d{4}-W\d{2}\.pdf$", pdf_key + ): + if pdf_key: + logger.error("Unexpected summary PDF key shape for %s: %s", week, pdf_key) + return response(500, {"error": "Internal error"}) return response(404, {"error": "No summary PDF available for this week"}) - url = boto3.client("s3").generate_presigned_url( + url = _s3.generate_presigned_url( "get_object", Params={"Bucket": os.environ["REPORTS_BUCKET"], "Key": pdf_key}, - ExpiresIn=300, + ExpiresIn=PRESIGNED_URL_TTL_SECONDS, ) logger.info("Admin %s requested summary PDF for %s", user["email"], week) return response(200, {"week": week, "url": url}) diff --git a/template.yaml b/template.yaml index a66df50..aa07aec 100644 --- a/template.yaml +++ b/template.yaml @@ -271,11 +271,12 @@ Resources: - Effect: Allow Action: ssm:GetParameter Resource: !Sub 'arn:aws:ssm:${AWS::Region}:${AWS::AccountId}:parameter/meal-order-manager/*' - # Read-only access to weekly report PDFs for the admin - # summary-pdf presigned-URL endpoint. + # Read-only access to weekly summary PDFs (only — not the + # payroll/order CSVs) for the admin summary-pdf presigned-URL + # endpoint. - Effect: Allow Action: s3:GetObject - Resource: !Sub '${ReportsBucket.Arn}/reports/*' + Resource: !Sub '${ReportsBucket.Arn}/reports/*/weekly-summary-*.pdf' Events: SubmitOrder: Type: HttpApi diff --git a/tests/test_submit_order.py b/tests/test_submit_order.py index de1e976..47a9037 100644 --- a/tests/test_submit_order.py +++ b/tests/test_submit_order.py @@ -1600,14 +1600,14 @@ def _pdf_event(week=None): @patch.dict(os.environ, {"REPORTS_BUCKET": "test-reports-bucket"}) -@patch("submit_order_handler.boto3.client") +@patch("submit_order_handler._s3") @patch("submit_order_handler.get_summary") @patch( "submit_order_handler._verify_admin", return_value=({"email": "adam@seahavenind.com"}, None), ) def test_admin_summary_pdf_returns_presigned_url( - mock_verify, mock_get_summary, mock_boto_client + mock_verify, mock_get_summary, mock_s3 ): """Closed week with a stamped PDF key returns a presigned URL.""" from submit_order_handler import lambda_handler @@ -1615,9 +1615,7 @@ def test_admin_summary_pdf_returns_presigned_url( mock_get_summary.return_value = { "weekly_summary_pdf_s3_key": "reports/2026-W22/weekly-summary-2026-W22.pdf" } - mock_s3 = MagicMock() mock_s3.generate_presigned_url.return_value = "https://signed.example/pdf" - mock_boto_client.return_value = mock_s3 status, body = _parse_response(lambda_handler(_pdf_event("2026-W22"), None)) @@ -1662,6 +1660,39 @@ def test_admin_summary_pdf_400_without_week(mock_verify): assert status == 400, f"Expected 400, got {status}: {body}" +@patch( + "submit_order_handler._verify_admin", + return_value=({"email": "adam@seahavenind.com"}, None), +) +def test_admin_summary_pdf_400_malformed_week(mock_verify): + """Week not matching YYYY-WNN returns 400 before any lookup.""" + from submit_order_handler import lambda_handler + + status, body = _parse_response(lambda_handler(_pdf_event("../../etc"), None)) + + assert status == 400, f"Expected 400, got {status}: {body}" + + +@patch("submit_order_handler._s3") +@patch("submit_order_handler.get_summary") +@patch( + "submit_order_handler._verify_admin", + return_value=({"email": "adam@seahavenind.com"}, None), +) +def test_admin_summary_pdf_rejects_tampered_key(mock_verify, mock_get_summary, mock_s3): + """A PDF key outside the expected shape is never presigned (500, no URL).""" + from submit_order_handler import lambda_handler + + mock_get_summary.return_value = { + "weekly_summary_pdf_s3_key": "reports/2026-W22/payroll-deductions.csv" + } + + status, body = _parse_response(lambda_handler(_pdf_event("2026-W22"), None)) + + assert status == 500, f"Expected 500 for tampered key, got {status}: {body}" + mock_s3.generate_presigned_url.assert_not_called() + + @patch( "submit_order_handler._verify_admin", return_value=(None, submit_order_handler.response(401, {"error": "Missing token"})),