Merge commit from fork

save_report_to_s3() put the sender-controlled report ID into the object
key and org_name/org_email/report_id into user-defined metadata verbatim.
botocore rejects non-ASCII metadata client-side (ParamValidationError) and
a bare CR/LF with "Invalid header value", which the CLI counts as an S3
output error, so one report held back its whole mailbox batch. "/" and
".." in the report ID added key segments, and keys had no sender identity.

- Percent-encode the report ID into one key segment ("/", the AWS
  "characters to avoid", "%", control and non-ASCII characters) and
  shorten it so the key stays within S3's 1,024-byte limit.
- Percent-encode metadata values (reversible with unquote) and cap each at
  256 characters, under S3's 2 KB metadata limit.
- Append the first 16 hex characters of a SHA-256 over the report type and
  its identifying, report-sourced fields (never enrichment), so reporters
  that reuse one report ID no longer overwrite their own reports and a
  retry still overwrites the same object.
- Fix failure reports, which raised KeyError on report_metadata (absent
  from FailureReport in types.py) since S3 output was added in 7.0.0.

Sources: https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html,
https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingMetadata.html,
botocore handlers.validate_ascii_metadata.

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

CHANGELOG.md

@@ -2,10 +2,18 @@
 
 ## Unreleased
 
+### Security
+
+- **S3 object keys and metadata are now sanitized, and each key ends in a hash of the fields that identify the report** ([GHSA-7wqr-f6g9-cm29](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-7wqr-f6g9-cm29), affects 11.1.0 and earlier). `S3Client.save_report_to_s3()` put the sender-controlled report ID into the object key as-is, and sent `org_name` and the other metadata values as-is. A report ID such as `../../x` added path segments to the key, an ID could carry `\`, `%`, and the other characters AWS lists as "characters to avoid", or control characters, which AWS says "might require special handling", and a long ID could push the key past the 1,024-byte limit ([AWS, "Naming Amazon S3 objects"](https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html)). botocore rejects non-ASCII metadata with `ParamValidationError`, so a report from an organization with a non-ASCII name was never saved, and its batch was held back and retried. A bare CR or LF in a metadata value made botocore raise `HTTPClientError: Invalid header value` before sending, which held the batch back the same way, while CR or LF followed by a space or tab (an obs-fold) was sent in the `x-amz-meta-*` header; the 2 KB metadata limit ([AWS, "Working with object metadata"](https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingMetadata.html)) was not enforced. The key also identified a report only by its type, begin day, and report ID, so anyone who could send reports to a monitored mailbox could replace another reporter's object for that day by reusing its report ID. The report ID is now percent-encoded into a single key segment and shortened so the whole key fits in 1,024 bytes. The encoded characters are `/`, every AWS "character to avoid" (`` \ { } ^ [ ] " < > ~ # | ` `` and `%`), control characters, and non-ASCII characters, so `%` appears in a key only as the escape character. The punctuation real report IDs use, such as `:`, `@`, `.`, and `_`, is kept. Each metadata value is percent-encoded (`%`, control characters, and non-ASCII characters) so `urllib.parse.unquote()` reverses it, and is cut to 256 characters, which keeps the total under 2 KB. Each key now ends in `_<hash>.json`, where the hash is the first 16 hex characters of a SHA-256 hash of the report type and the fields that identify the report (for aggregate reports: organization name, report ID, policy domain, begin date, and end date; for SMTP TLS reports: organization name, report ID, begin date, end date, and each policy domain). Enrichment such as reverse DNS is not hashed, so a retried report still overwrites its own object. This narrows the overwrite but does not close it: these fields are not secret, and a forged report whose reporter name, report ID, policy domain, and dates all match a real one still replaces it. Turning on S3 Versioning on the bucket keeps both. Compatibility: every report's key changes. Objects saved by earlier versions keep their old keys and are not rewritten, and a batch that is retried across the upgrade is saved again under its new key. Reported by helixnebulatech.
+
 ### Changes
 
 - **mailsuite 2.3.3 or later is now required.** mailsuite 2.3.3 ([GHSA-9jx3-p8wm-g8vw](https://github.com/seanthegeek/mailsuite/security/advisories/GHSA-9jx3-p8wm-g8vw)) makes the Maildir connection return a message's stored bytes from `fetch_message()` and rename the file in `move_message()`, instead of re-serializing it. A report email that cannot be re-serialized (the [GHSA-qxwv-2vwj-v828](https://github.com/domainaware/parsedmarc/security/advisories/GHSA-qxwv-2vwj-v828) messages) is therefore now moved to the `Invalid` folder on Maildir too, instead of staying in the inbox and being logged on every run.
 
+### Bug fixes
+
+- **Failure reports are now saved to S3.** `save_report_to_s3()` read `report["report_metadata"]` for every report type except SMTP TLS, but failure reports have no `report_metadata` (see `FailureReport` in `parsedmarc/types.py`), so every failure report raised `KeyError` since S3 output was added in 7.0.0. The error was counted as an S3 output error, so the batch holding the report was held back. A failure report is now saved under `<path>/failure/year=YYYY/month=MM/day=DD/<reported domain>_<hash>.json`, dated by its UTC arrival date, with `reported_domain` and `arrival_date` metadata. Its hash covers the report type, the arrival date, reported domain, original mail from, original recipient, original envelope ID, source IP address, and the message sample.
+
 ## 11.1.0
 
 ### Security

docs/source/usage.md

@@ -742,6 +742,51 @@ The full set of configuration options is:
   - `endpoint_url` - str: The endpoint URL (Optional)
   - `access_key_id` - str: The access key id (Optional)
   - `secret_access_key` - str: The secret access key (Optional)
+
+  Each report is saved as one JSON object, with this key:
+  `<path>/<report type>/year=YYYY/month=MM/day=DD/<report ID>_<hash>.json`.
+  The date is the report's begin date. Failure reports have no report ID
+  or begin date, so they use the reported domain and the arrival date
+  instead. Aggregate report dates are in the host's local time (SMTP TLS
+  dates are as the report gives them, and failure arrival dates are in
+  UTC), so for aggregate reports the key's day and the hash depend on the
+  host's time zone.
+
+  The report ID comes from the sender, so it is percent-encoded before it
+  is used in the key. `/`, the characters AWS lists as "characters to
+  avoid" (`` \ { } ^ [ ] " < > ~ # | ` `` and `%`), control characters,
+  and non-ASCII characters are encoded (for example, `/` becomes `%2F`),
+  so the ID is always a single key segment, and `%` appears in it only as
+  the escape character. If the key would be longer than the 1,024-byte S3
+  limit, the report ID is shortened.
+
+  The hash is the first 16 hex characters of a SHA-256 hash of the report
+  type and the fields that identify the report:
+
+  - Aggregate: the organization name, report ID, policy domain, begin
+    date, and end date
+  - SMTP TLS: the organization name, report ID, begin date, end date, and
+    the domain of each policy
+  - Failure: the arrival date, reported domain, original mail from,
+    original recipient, original envelope ID, source IP address, and the
+    message sample
+
+  Only fields from the report itself are hashed, not the IP enrichment
+  (reverse DNS, country, and so on), so a report that is saved again
+  overwrites its own object. Two reports get the same key only when all of
+  these fields match, so a report that reuses another report's ID, but
+  differs in any of these fields, does not replace it. These fields are
+  not secret, though: a forged report whose reporter name, report ID,
+  policy domain, and dates all match a real one still replaces it. Turn
+  on S3 Versioning on the bucket to keep both.
+
+  Each object also gets metadata from the report: `org_name`, `org_email`,
+  `report_id`, `begin_date`, and `end_date` for aggregate reports;
+  `org_name`, `report_id`, `begin_date`, and `end_date` for SMTP TLS
+  reports; and `reported_domain` and `arrival_date` for failure reports.
+  AWS recommends US-ASCII metadata, and botocore rejects anything else, so
+  in each value `%`, control characters, and non-ASCII characters are
+  percent-encoded, and the value is cut to at most 256 characters.
 - `syslog`
   - `server` - str: The Syslog server name or IP address
   - `port` - int: The port to use (Default: `514`)
@@ -1022,8 +1067,9 @@ move unsavable messages to `Unsaved` immediately instead.
 Retrying a batch means re-sending it. Output destinations that
 deduplicate — Elasticsearch, OpenSearch, and PostgreSQL, which recognize
 an already-saved report — are unaffected, and S3 is idempotent because
-each report is written to an object key built from its type, date, and
-report ID, so a retry overwrites the same object. Kafka,
+each report is written to an object key built from its type, date, report
+ID, and a hash of the fields that identify the report, so a retry
+overwrites the same object. Kafka,
 Splunk HEC, syslog, GELF, webhooks, Azure Log Analytics, and the
 `--output` JSON/CSV files all append unconditionally, so each retry adds
 another copy of every report in the batch. That is why the default retry

parsedmarc/s3.py

@@ -2,14 +2,93 @@
 
 from __future__ import annotations
 
+import hashlib
 import json
 from typing import Any
+from urllib.parse import quote
 
 import boto3
 
 from parsedmarc.log import logger
 from parsedmarc.utils import human_timestamp_to_datetime
 
+# Printable ASCII (0x20-0x7E) except "%", which is always encoded so that
+# urllib.parse.unquote() reverses the encoding exactly. AWS allows Unicode
+# metadata values but recommends US-ASCII for REST, and botocore rejects a
+# non-ASCII metadata key or value with ParamValidationError. A bare CR or LF
+# in a value makes botocore raise "Invalid header value" before sending,
+# while CR or LF followed by a space or tab (an obs-fold) is sent as-is.
+_METADATA_SAFE = "".join(chr(c) for c in range(0x20, 0x7F) if chr(c) != "%")
+# The same, minus "/" (the key delimiter) and every printable character in
+# the AWS object key "characters to avoid" list, so a report ID always stays
+# one key segment. After encoding, "%" appears in a key segment only as the
+# escape character. The "might require special handling" characters that
+# real report IDs use (such as ":" and "@") are kept.
+# https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html
+_KEY_AVOID = '/\\{}^[]"<>~#|`'
+_KEY_SEGMENT_SAFE = "".join(c for c in _METADATA_SAFE if c not in _KEY_AVOID)
+# User-defined metadata is limited to 2 KB, counted as the UTF-8 bytes of
+# every key and value. At most seven metadata keys exist; 7 * 256 bytes plus
+# the names (with their "x-amz-meta-" prefixes) is 1,940 bytes.
+# https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingMetadata.html
+_MAX_METADATA_VALUE_LENGTH = 256
+# An object key is at most 1,024 bytes of UTF-8, prefix included.
+_MAX_KEY_LENGTH = 1024
+# Hex characters of the SHA-256 digest appended to each key (64 bits).
+_KEY_HASH_LENGTH = 16
+
+
+def _percent_encode(value: str, safe: str, max_length: int) -> str:
+    """Percent-encode ``value`` as UTF-8, keeping only ``safe`` characters
+    as-is, and cut the result to at most ``max_length`` characters.
+
+    The result is ASCII, so its length in characters is its length in
+    bytes. It is cut between whole input characters, so neither a ``%XX``
+    triplet nor a multibyte UTF-8 sequence is ever split. A lone surrogate,
+    which has no UTF-8 encoding, becomes ``?``. A ``max_length`` of zero or
+    less returns an empty string.
+    """
+    pieces: list[str] = []
+    length = 0
+    # Every input character encodes to at least one output character, so
+    # no more than max_length input characters can fit.
+    for char in value[: max(max_length, 0)]:
+        piece = quote(char, safe=safe, errors="replace")
+        if "~" not in safe:
+            # quote() never encodes "~" (RFC 3986 unreserved), whatever
+            # the safe set says.
+            piece = piece.replace("~", "%7E")
+        if length + len(piece) > max_length:
+            break
+        pieces.append(piece)
+        length += len(piece)
+    return "".join(pieces)
+
+
+def _metadata_value(value: object) -> str:
+    """Return ``value`` as an S3 metadata value: printable ASCII other
+    than ``%`` is kept, everything else is percent-encoded, and the result
+    is at most 256 characters long."""
+    return _percent_encode(str(value), _METADATA_SAFE, _MAX_METADATA_VALUE_LENGTH)
+
+
+def _report_hash(report_type: str, fields: list[object]) -> str:
+    """Return the first 16 hex characters of the SHA-256 digest of the
+    report type and the fields that identify the report.
+
+    Only fields taken from the report itself are hashed, never enrichment
+    (reverse DNS, GeoIP, ...), so the same report retried later is saved
+    to the same key. Changing this recipe moves every object.
+    """
+    canonical = json.dumps(
+        [report_type, *fields],
+        sort_keys=True,
+        separators=(",", ":"),
+        default=str,
+    )
+    digest = hashlib.sha256(canonical.encode("utf-8")).hexdigest()
+    return digest[:_KEY_HASH_LENGTH]
+
 
 class S3Client(object):
     """A client for interacting with Amazon S3"""
@@ -41,6 +120,8 @@ class S3Client(object):
             "report_id",
             "begin_date",
             "end_date",
+            "reported_domain",
+            "arrival_date",
         ]
 
         # https://github.com/boto/boto3/blob/1.24.7/boto3/session.py#L312
@@ -63,6 +144,7 @@ class S3Client(object):
         self.save_report_to_s3(report, "smtp_tls")
 
     def save_report_to_s3(self, report: dict[str, Any], report_type: str):
+        metadata_source: dict[str, Any]
         if report_type == "smtp_tls":
             # SMTP TLS reports (RFC 8460) are flat — they have no
             # `report_metadata` sub-object — and parse_smtp_tls_report_json
@@ -70,32 +152,72 @@ class S3Client(object):
             # (per SMTPTLSReport's TypedDict).
             report_date = human_timestamp_to_datetime(report["begin_date"])
             report_id = report["report_id"]
+            hash_fields: list[object] = [
+                report.get("organization_name"),
+                report.get("report_id"),
+                report.get("begin_date"),
+                report.get("end_date"),
+                sorted(
+                    str(policy.get("policy_domain"))
+                    for policy in report.get("policies") or []
+                ),
+            ]
             metadata_source = {
                 "org_name": report.get("organization_name"),
                 "report_id": report.get("report_id"),
                 "begin_date": str(report.get("begin_date")),
                 "end_date": str(report.get("end_date")),
             }
-        else:
+        elif report_type == "failure":
+            # Failure reports (RFC 9991) have no report_metadata and no
+            # report ID (see FailureReport in types.py): the arrival date
+            # and the reported domain stand in for the begin date and the
+            # report ID. arrival_date_utc is a UTC wall-clock string.
             report_date = human_timestamp_to_datetime(
-                report["report_metadata"]["begin_date"]
+                report["arrival_date_utc"], assume_utc=True
             )
-            report_id = report["report_metadata"]["report_id"]
-            metadata_source = report["report_metadata"]
-        path_template = "{0}/{1}/year={2}/month={3:02d}/day={4:02d}/{5}.json"
-        object_path = path_template.format(
-            self.bucket_path,
-            report_type,
-            report_date.year,
-            report_date.month,
-            report_date.day,
-            report_id,
+            report_id = report["reported_domain"]
+            source = report.get("source") or {}
+            hash_fields = [
+                report.get("arrival_date_utc"),
+                report.get("reported_domain"),
+                report.get("original_mail_from"),
+                report.get("original_rcpt_to"),
+                report.get("original_envelope_id"),
+                source.get("ip_address"),
+                report.get("sample"),
+            ]
+            metadata_source = {
+                "reported_domain": report.get("reported_domain"),
+                "arrival_date": report.get("arrival_date_utc"),
+            }
+        else:
+            report_metadata = report["report_metadata"]
+            report_date = human_timestamp_to_datetime(report_metadata["begin_date"])
+            report_id = report_metadata["report_id"]
+            hash_fields = [
+                report_metadata.get("org_name"),
+                report_metadata.get("report_id"),
+                (report.get("policy_published") or {}).get("domain"),
+                report_metadata.get("begin_date"),
+                report_metadata.get("end_date"),
+            ]
+            metadata_source = report_metadata
+        prefix = (
+            f"{self.bucket_path}/{report_type}/year={report_date.year}/"
+            f"month={report_date.month:02d}/day={report_date.day:02d}/"
         )
+        suffix = f"_{_report_hash(report_type, hash_fields)}.json"
+        # The report ID is percent-encoded into a single key segment and
+        # shortened so the whole key fits in S3's 1,024-byte limit.
+        budget = _MAX_KEY_LENGTH - len(prefix.encode("utf-8")) - len(suffix)
+        segment = _percent_encode(str(report_id), _KEY_SEGMENT_SAFE, budget)
+        object_path = f"{prefix}{segment}{suffix}"
         logger.debug(
             f"Saving {report_type} report to s3://{self.bucket_name}/{object_path}"
         )
         object_metadata = {
-            k: v
+            k: _metadata_value(v)
             for k, v in metadata_source.items()
             if k in self.metadata_keys and v is not None
         }

tests/test_s3.py

@@ -1,11 +1,23 @@
 """Tests for parsedmarc.s3"""
 
+import copy
+import hashlib
 import json
+import os
+import re
 import unittest
 from unittest.mock import MagicMock, patch
+from urllib.parse import unquote
 
+from botocore.exceptions import ParamValidationError
+from botocore.stub import ANY, Stubber
+
+import parsedmarc
 from parsedmarc.s3 import S3Client
 
+_SAMPLES_DIR = os.path.join(os.path.dirname(os.path.dirname(__file__)), "samples")
+_HASH_SUFFIX = r"_[0-9a-f]{16}\.json"
+
 
 def _sample_aggregate_report():
     """Minimal aggregate report shape used by S3Client.save_*_to_s3."""
@@ -79,44 +91,59 @@ class TestS3ClientInit(unittest.TestCase):
         self.assertEqual(client.bucket, "bucket-handle")
 
 
-class TestS3ClientSavePathsAndMetadata(unittest.TestCase):
-    """The S3 key is built from the report's begin_date and report_id.
-    Wrong format = unfindable reports; wrong metadata filtering = secret
-    leakage onto the S3 object."""
+def _client_with_mock_bucket(bucket_path="dmarc"):
+    with patch("parsedmarc.s3.boto3.resource"):
+        client = S3Client(
+            bucket_name="b",
+            bucket_path=bucket_path,
+            region_name="us-east-1",
+            endpoint_url="https://s3.example.com",
+            access_key_id="k",
+            secret_access_key="s",
+        )
+    client.bucket = MagicMock()
+    return client
 
-    def _client_with_mock_bucket(self):
-        with patch("parsedmarc.s3.boto3.resource"):
-            client = S3Client(
-                bucket_name="b",
-                bucket_path="dmarc",
-                region_name="us-east-1",
-                endpoint_url="https://s3.example.com",
-                access_key_id="k",
-                secret_access_key="s",
-            )
-        client.bucket = MagicMock()
-        return client
+
+def _saved_key(client):
+    return client.bucket.put_object.call_args.kwargs["Key"]
+
+
+def _saved_metadata(client):
+    return client.bucket.put_object.call_args.kwargs["Metadata"]
+
+
+def _aggregate_key(report, bucket_path="dmarc"):
+    client = _client_with_mock_bucket(bucket_path)
+    client.save_aggregate_report_to_s3(report)
+    return _saved_key(client)
+
+
+def _aggregate_key_segment(report_id):
+    """The last key segment for an aggregate report with ``report_id``."""
+    report = _sample_aggregate_report()
+    report["report_metadata"]["report_id"] = report_id
+    return _aggregate_key(report).rsplit("/", 1)[1]
+
+
+class TestS3ClientSavePathsAndMetadata(unittest.TestCase):
+    """The S3 key is built from the report's begin_date, report_id, and a
+    hash of the fields that identify the report. Wrong format = unfindable
+    reports; wrong metadata filtering = secret leakage onto the S3 object."""
 
     def test_aggregate_dispatches_with_aggregate_in_key_path(self):
         """save_aggregate_report_to_s3 puts the object under
-        <bucket_path>/aggregate/year=YYYY/month=MM/day=DD/<report_id>.json."""
-        client = self._client_with_mock_bucket()
+        <bucket_path>/aggregate/year=YYYY/month=MM/day=DD/<report_id>_<hash>.json."""
+        client = _client_with_mock_bucket()
         client.save_aggregate_report_to_s3(_sample_aggregate_report())
         client.bucket.put_object.assert_called_once()
-        call = client.bucket.put_object.call_args
-        self.assertEqual(
-            call.kwargs["Key"],
-            "dmarc/aggregate/year=2024/month=01/day=15/agg-123.json",
+        self.assertRegex(
+            _saved_key(client),
+            r"\Admarc/aggregate/year=2024/month=01/day=15/agg-123"
+            + _HASH_SUFFIX
+            + r"\Z",
         )
 
-    def test_failure_dispatches_with_failure_in_key_path(self):
-        client = self._client_with_mock_bucket()
-        report = _sample_aggregate_report()
-        report["report_metadata"]["report_id"] = "fail-789"
-        client.save_failure_report_to_s3(report)
-        key = client.bucket.put_object.call_args.kwargs["Key"]
-        self.assertEqual(key, "dmarc/failure/year=2024/month=01/day=15/fail-789.json")
-
     def test_smtp_tls_uses_report_begin_date(self):
         """SMTP TLS reports are flat — no report_metadata — and
         begin_date is the ISO string produced by parse_smtp_tls_report_json.
@@ -127,25 +154,33 @@ class TestS3ClientSavePathsAndMetadata(unittest.TestCase):
         report_metadata sub-object, which crashed with KeyError on every
         SMTP TLS save. The CLI swallowed the error and only logged it,
         so the bug was invisible in production."""
-        client = self._client_with_mock_bucket()
+        client = _client_with_mock_bucket()
         client.save_smtp_tls_report_to_s3(_sample_smtp_tls_report())
-        key = client.bucket.put_object.call_args.kwargs["Key"]
-        self.assertEqual(key, "dmarc/smtp_tls/year=2024/month=02/day=03/tls-456.json")
+        self.assertRegex(
+            _saved_key(client),
+            r"\Admarc/smtp_tls/year=2024/month=02/day=03/tls-456"
+            + _HASH_SUFFIX
+            + r"\Z",
+        )
 
     def test_smtp_tls_metadata_comes_from_flat_report_fields(self):
         """SMTP TLS object metadata is built from the flat report
         instead of report_metadata. organization_name is renamed to
         org_name (the S3 metadata key) for consistency with DMARC."""
-        client = self._client_with_mock_bucket()
+        client = _client_with_mock_bucket()
         client.save_smtp_tls_report_to_s3(_sample_smtp_tls_report())
-        meta = client.bucket.put_object.call_args.kwargs["Metadata"]
-        self.assertEqual(meta["org_name"], "example.com")
-        self.assertEqual(meta["report_id"], "tls-456")
-        self.assertEqual(meta["begin_date"], "2024-02-03T00:00:00Z")
-        self.assertEqual(meta["end_date"], "2024-02-04T00:00:00Z")
+        self.assertEqual(
+            _saved_metadata(client),
+            {
+                "org_name": "example.com",
+                "report_id": "tls-456",
+                "begin_date": "2024-02-03T00:00:00Z",
+                "end_date": "2024-02-04T00:00:00Z",
+            },
+        )
 
     def test_object_body_is_json_serialized_report(self):
-        client = self._client_with_mock_bucket()
+        client = _client_with_mock_bucket()
         report = _sample_aggregate_report()
         client.save_aggregate_report_to_s3(report)
         body = client.bucket.put_object.call_args.kwargs["Body"]
@@ -157,12 +192,12 @@ class TestS3ClientSavePathsAndMetadata(unittest.TestCase):
         """report_metadata fields outside `metadata_keys` must not be
         attached to the S3 object — they could leak large or sensitive
         payloads (errors lists, internal IDs) into object metadata."""
-        client = self._client_with_mock_bucket()
+        client = _client_with_mock_bucket()
         report = _sample_aggregate_report()
         report["report_metadata"]["errors"] = ["a", "b"]
         report["report_metadata"]["internal_diag"] = "secret"
         client.save_aggregate_report_to_s3(report)
-        meta = client.bucket.put_object.call_args.kwargs["Metadata"]
+        meta = _saved_metadata(client)
         self.assertEqual(
             set(meta.keys()),
             {"org_name", "org_email", "report_id", "begin_date", "end_date"},
@@ -171,6 +206,385 @@ class TestS3ClientSavePathsAndMetadata(unittest.TestCase):
         self.assertNotIn("internal_diag", meta)
 
 
+class TestS3ClientKeyHash(unittest.TestCase):
+    """Every key ends in ``_<hash>.json``, where the hash covers the fields
+    that identify the report, so two different reports that share a report
+    ID (by accident, or because a sender reuses another reporter's ID) are
+    saved to different objects, while a retry of the same report
+    overwrites its own object."""
+
+    def test_normal_report_id_prefix_and_id_unchanged(self):
+        """Report IDs real reporters use keep their punctuation, so
+        existing key prefixes and IDs stay readable."""
+        for report_id in (
+            "agg-123",
+            "example.com:1538463741",
+            "x@mail.ru",
+            "2024-01-09T00:00:00Z_example.com",
+        ):
+            with self.subTest(report_id=report_id):
+                report = _sample_aggregate_report()
+                report["report_metadata"]["report_id"] = report_id
+                key = _aggregate_key(report)
+                self.assertTrue(
+                    key.startswith("dmarc/aggregate/year=2024/month=01/day=15/")
+                )
+                self.assertRegex(
+                    key.rsplit("/", 1)[1],
+                    r"\A" + re.escape(report_id) + _HASH_SUFFIX + r"\Z",
+                )
+
+    def test_key_hash_pinned_for_fixed_report(self):
+        """The hash recipe is pinned. Changing it moves every object to a
+        new key, so a batch retried across the upgrade is saved twice;
+        that is a compatibility event that needs a changelog entry, not a
+        silent refactor.
+
+        The golden value is the first 16 hex characters of
+        sha256('["aggregate","example.com","agg-123","example.com",
+        "2024-01-15 00:00:00","2024-01-16 00:00:00"]'): report type, then
+        org_name, report_id, policy_published.domain, begin_date, end_date,
+        as compact JSON. It is recomputed independently below."""
+        canonical = json.dumps(
+            [
+                "aggregate",
+                "example.com",
+                "agg-123",
+                "example.com",
+                "2024-01-15 00:00:00",
+                "2024-01-16 00:00:00",
+            ],
+            separators=(",", ":"),
+        )
+        self.assertEqual(
+            hashlib.sha256(canonical.encode("utf-8")).hexdigest()[:16],
+            "f80ea35e1b3f6ad4",
+        )
+        self.assertEqual(
+            _aggregate_key(_sample_aggregate_report()),
+            "dmarc/aggregate/year=2024/month=01/day=15/agg-123_f80ea35e1b3f6ad4.json",
+        )
+
+    def test_key_hash_ignores_enrichment(self):
+        """A report retried later may get different reverse DNS or GeoIP
+        results; only fields from the report itself are hashed, so the
+        retry still overwrites the same object."""
+        first = _sample_aggregate_report()
+        first["records"] = [
+            {"source": {"ip_address": "192.0.2.1", "reverse_dns": None}}
+        ]
+        second = copy.deepcopy(first)
+        second["records"][0]["source"]["reverse_dns"] = "mail.example.net"
+        second["records"][0]["source"]["country"] = "US"
+        self.assertEqual(_aggregate_key(first), _aggregate_key(second))
+
+    def test_key_hash_separates_reports_sharing_report_id(self):
+        """Reports with the same org_name and report_id but a different
+        policy domain, begin_date (same day), end_date, or org_name are
+        different reports and must not overwrite each other."""
+        base_key = _aggregate_key(_sample_aggregate_report())
+        variants = {
+            "domain": ("policy_published", "domain", "other.example"),
+            "begin_date": ("report_metadata", "begin_date", "2024-01-15 06:00:00"),
+            "end_date": ("report_metadata", "end_date", "2024-01-17 00:00:00"),
+            "org_name": ("report_metadata", "org_name", "attacker.example"),
+        }
+        for name, (section, field, value) in variants.items():
+            with self.subTest(field=name):
+                report = _sample_aggregate_report()
+                report[section][field] = value
+                key = _aggregate_key(report)
+                # Same directory and report ID; only the hash differs.
+                self.assertEqual(key.rsplit("_", 1)[0], base_key.rsplit("_", 1)[0])
+                self.assertNotEqual(key, base_key)
+
+    def test_smtp_tls_hash_includes_policy_domains(self):
+        """Two SMTP TLS reports sharing organization, report ID, and dates
+        but covering different policy domains get different keys; the
+        policy order inside one report does not matter."""
+
+        def key_for(*domains):
+            client = _client_with_mock_bucket()
+            report = _sample_smtp_tls_report()
+            report["policies"] = [{"policy_domain": d} for d in domains]
+            client.save_smtp_tls_report_to_s3(report)
+            return _saved_key(client)
+
+        self.assertNotEqual(key_for("a.example"), key_for("b.example"))
+        self.assertEqual(
+            key_for("a.example", "b.example"), key_for("b.example", "a.example")
+        )
+
+
+class TestS3ClientKeySanitization(unittest.TestCase):
+    """The report ID is sender-controlled. It is percent-encoded into one
+    key segment so it cannot add path segments, carry characters AWS says
+    to avoid, or push the key past S3's 1,024-byte limit
+    (https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html)."""
+
+    def test_traversal_report_id_stays_one_segment(self):
+        report = _sample_aggregate_report()
+        report["report_metadata"]["report_id"] = "../../etc/passwd"
+        key = _aggregate_key(report)
+        normal_key = _aggregate_key(_sample_aggregate_report())
+        self.assertEqual(key.count("/"), normal_key.count("/"))
+        self.assertTrue(key.startswith("dmarc/aggregate/year=2024/month=01/day=15/"))
+        self.assertRegex(
+            key.rsplit("/", 1)[1], r"\A\.\.%2F\.\.%2Fetc%2Fpasswd" + _HASH_SUFFIX
+        )
+
+    def test_report_id_backslash_percent_control_nonascii_encoded(self):
+        """ "/", every printable character in the AWS "characters to avoid"
+        list, control characters, and non-ASCII characters are encoded.
+        quote() never encodes "~" on its own, so it is checked too."""
+        cases = {
+            "a/b": "a%2Fb",
+            "a{b}": "a%7Bb%7D",
+            "a^b": "a%5Eb",
+            "a[b]": "a%5Bb%5D",
+            'a"b': "a%22b",
+            "a<b>": "a%3Cb%3E",
+            "a~b": "a%7Eb",
+            "a#b": "a%23b",
+            "a|b": "a%7Cb",
+            "a`b": "a%60b",
+            "a\\b": "a%5Cb",
+            "a%b": "a%25b",
+            "a\r\nb": "a%0D%0Ab",
+            "a\x00b": "a%00b",
+            "a\x7fb": "a%7Fb",
+            "café": "caf%C3%A9",
+        }
+        for report_id, encoded in cases.items():
+            with self.subTest(report_id=report_id):
+                segment = _aggregate_key_segment(report_id)
+                self.assertRegex(
+                    segment, r"\A" + re.escape(encoded) + _HASH_SUFFIX + r"\Z"
+                )
+                # The encoding is reversible.
+                self.assertEqual(unquote(segment.rsplit("_", 1)[0]), report_id)
+
+    def test_empty_dot_dotdot_report_id(self):
+        """The hash suffix keeps the last segment from ever being empty or
+        period-only, which AWS says to avoid."""
+        for report_id, expected in (("", ""), (".", "."), ("..", "..")):
+            with self.subTest(report_id=report_id):
+                self.assertRegex(
+                    _aggregate_key_segment(report_id),
+                    r"\A" + re.escape(expected) + _HASH_SUFFIX + r"\Z",
+                )
+
+    def test_oversized_report_id_key_within_1024_bytes(self):
+        """A 5,000-character multibyte report ID under a long bucket_path
+        is shortened between whole characters so the key fits in 1,024
+        bytes, the hash suffix is intact, and no %XX triplet or UTF-8
+        sequence is split."""
+        report = _sample_aggregate_report()
+        report_id = "€" * 5000  # 3 UTF-8 bytes -> 9 encoded characters
+        report["report_metadata"]["report_id"] = report_id
+        key = _aggregate_key(report, bucket_path="p" * 300)
+        self.assertLessEqual(len(key.encode("utf-8")), 1024)
+        # Only a partial character's worth of room may be left unused.
+        self.assertGreater(len(key.encode("utf-8")), 1024 - 9)
+        segment = key.rsplit("/", 1)[1]
+        self.assertRegex(segment, r"\A(%E2%82%AC)+" + _HASH_SUFFIX + r"\Z")
+        decoded = unquote(segment.rsplit("_", 1)[0], errors="strict")
+        self.assertTrue(report_id.startswith(decoded))
+
+    def test_oversized_tilde_report_id_counts_encoded_length(self):
+        """ "~" grows to "%7E" after quote() returns, and the shortening
+        counts the encoded length, so the key still fits in 1,024 bytes
+        and ends on a whole triplet."""
+        report = _sample_aggregate_report()
+        report["report_metadata"]["report_id"] = "~" * 2000
+        key = _aggregate_key(report)
+        self.assertLessEqual(len(key.encode("utf-8")), 1024)
+        self.assertGreater(len(key.encode("utf-8")), 1024 - 3)
+        self.assertRegex(key.rsplit("/", 1)[1], r"\A(%7E)+" + _HASH_SUFFIX + r"\Z")
+
+    def test_bucket_path_longer_than_key_limit_leaves_id_empty(self):
+        """If bucket_path alone uses up the 1,024 bytes, the report ID is
+        dropped rather than sliced with a negative length; the hash still
+        identifies the report and S3 reports the over-long key."""
+        key = _aggregate_key(_sample_aggregate_report(), bucket_path="p" * 1100)
+        self.assertRegex(key, r"/year=2024/month=01/day=15/" + _HASH_SUFFIX + r"\Z")
+
+
+class TestS3ClientMetadataSanitization(unittest.TestCase):
+    """S3 user-defined metadata must be ASCII (botocore rejects anything
+    else before sending) and is limited to 2 KB in total, counted as the
+    UTF-8 bytes of every key and value
+    (https://docs.aws.amazon.com/AmazonS3/latest/userguide/UsingMetadata.html).
+    Values are percent-encoded so they round-trip through unquote()."""
+
+    def _stubbed_client(self):
+        """A real S3Client whose boto3 client is stubbed, so botocore's
+        own parameter handlers, including the ASCII metadata check, run."""
+        client = S3Client(
+            bucket_name="b",
+            bucket_path="dmarc",
+            region_name="us-east-1",
+            endpoint_url="https://s3.example.com",
+            access_key_id="k",
+            secret_access_key="s",
+        )
+        self.addCleanup(client.close)
+        meta = client.s3.meta
+        assert meta is not None
+        stubber = Stubber(meta.client)
+        stubber.activate()
+        self.addCleanup(stubber.deactivate)
+        return client, stubber
+
+    def test_metadata_non_ascii_org_name_accepted_by_botocore(self):
+        client, stubber = self._stubbed_client()
+        report = _sample_aggregate_report()
+        report["report_metadata"]["org_name"] = "Ünïcode Org"
+        stubber.add_response(
+            "put_object",
+            {},
+            expected_params={
+                "Bucket": "b",
+                "Key": ANY,
+                "Body": ANY,
+                "Metadata": {
+                    "org_name": "%C3%9Cn%C3%AFcode Org",
+                    "org_email": "dmarc@example.com",
+                    "report_id": "agg-123",
+                    "begin_date": "2024-01-15 00:00:00",
+                    "end_date": "2024-01-16 00:00:00",
+                },
+            },
+        )
+        client.save_aggregate_report_to_s3(report)
+        stubber.assert_no_pending_responses()
+        self.assertEqual(unquote("%C3%9Cn%C3%AFcode Org"), "Ünïcode Org")
+
+        # The SMTP TLS path goes through the same encoding.
+        tls_report = _sample_smtp_tls_report()
+        tls_report["organization_name"] = "Ünïcode Org"
+        stubber.add_response(
+            "put_object",
+            {},
+            expected_params={
+                "Bucket": "b",
+                "Key": ANY,
+                "Body": ANY,
+                "Metadata": {
+                    "org_name": "%C3%9Cn%C3%AFcode Org",
+                    "report_id": "tls-456",
+                    "begin_date": "2024-02-03T00:00:00Z",
+                    "end_date": "2024-02-04T00:00:00Z",
+                },
+            },
+        )
+        client.save_smtp_tls_report_to_s3(tls_report)
+        stubber.assert_no_pending_responses()
+
+        # The validator is live under the Stubber: the raw value that the
+        # unfixed code sent is rejected before any request is made.
+        stubber.add_response("put_object", {})
+        with self.assertRaises(ParamValidationError):
+            client.bucket.put_object(
+                Body="{}", Key="k.json", Metadata={"org_name": "Ünïcode Org"}
+            )
+
+    def test_metadata_crlf_and_percent_round_trip(self):
+        client = _client_with_mock_bucket()
+        report = _sample_aggregate_report()
+        org_name = "Org\r\nX-Injected: 1 100%"
+        report["report_metadata"]["org_name"] = org_name
+        client.save_aggregate_report_to_s3(report)
+        value = _saved_metadata(client)["org_name"]
+        self.assertEqual(value, "Org%0D%0AX-Injected: 1 100%25")
+        self.assertEqual(unquote(value), org_name)
+
+    def test_metadata_values_capped_total_under_2kb(self):
+        """Each value is at most 256 characters, cut between whole
+        characters, so the metadata sent stays under 2 KB, counting the
+        x-amz-meta- header names."""
+        client = _client_with_mock_bucket()
+        report = _sample_aggregate_report()
+        # begin_date is parsed for the key, so it stays a real date.
+        for field in ("org_name", "org_email", "report_id"):
+            report["report_metadata"][field] = "x" * 10_000
+        # 255 ASCII characters, then one that encodes to 6: it must be
+        # dropped whole, not cut to "%C3" or "%C3%9".
+        report["report_metadata"]["end_date"] = "a" * 255 + "Ü"
+        client.save_aggregate_report_to_s3(report)
+        meta = _saved_metadata(client)
+        self.assertEqual(meta["end_date"], "a" * 255)
+        for value in meta.values():
+            self.assertLessEqual(len(value), 256)
+        total = sum(
+            len(f"x-amz-meta-{k}".encode("utf-8")) + len(v.encode("utf-8"))
+            for k, v in meta.items()
+        )
+        self.assertLess(total, 2048)
+
+    def test_smtp_tls_lone_surrogate_does_not_raise(self):
+        """A lone surrogate has no UTF-8 encoding; it becomes "?" instead
+        of raising UnicodeEncodeError and failing the batch."""
+        client = _client_with_mock_bucket()
+        report = _sample_smtp_tls_report()
+        report["organization_name"] = "Org\ud800"
+        report["report_id"] = "id\udfff"
+        client.save_smtp_tls_report_to_s3(report)
+        self.assertEqual(_saved_metadata(client)["org_name"], "Org?")
+        self.assertEqual(_saved_metadata(client)["report_id"], "id?")
+        self.assertRegex(_saved_key(client), r"/id\?" + _HASH_SUFFIX + r"\Z")
+
+
+class TestS3ClientFailureReports(unittest.TestCase):
+    """Failure reports (FailureReport in parsedmarc/types.py) have no
+    report_metadata and no report ID. Saving one used to raise KeyError
+    on report_metadata, so no failure report was ever saved to S3."""
+
+    @classmethod
+    def setUpClass(cls):
+        parsed = parsedmarc.parse_report_file(
+            os.path.join(_SAMPLES_DIR, "failure", "dmarc_ruf_report_linkedin.eml"),
+            offline=True,
+        )
+        assert parsed["report_type"] == "failure"
+        cls.report = parsed["report"]
+
+    def _save(self, report):
+        client = _client_with_mock_bucket()
+        client.save_failure_report_to_s3(report)
+        return client
+
+    def test_failure_report_key_from_arrival_date_and_reported_domain(self):
+        client = self._save(copy.deepcopy(self.report))
+        key = _saved_key(client)
+        self.assertRegex(
+            key,
+            r"\Admarc/failure/year=2019/month=04/day=30/example\.com"
+            + _HASH_SUFFIX
+            + r"\Z",
+        )
+        self.assertEqual(
+            _saved_metadata(client),
+            {
+                "reported_domain": "example.com",
+                "arrival_date": "2019-04-30 02:09:00",
+            },
+        )
+        body = json.loads(client.bucket.put_object.call_args.kwargs["Body"])
+        self.assertEqual(body["reported_domain"], "example.com")
+
+        # A different sample is a different report.
+        other = copy.deepcopy(self.report)
+        other["sample"] = other["sample"] + "\n"
+        self.assertNotEqual(_saved_key(self._save(other)), key)
+
+        # Different enrichment of the same report is the same report.
+        enriched = copy.deepcopy(self.report)
+        enriched["source"]["reverse_dns"] = "mail.example.net"
+        enriched["source"]["country"] = "US"
+        self.assertEqual(_saved_key(self._save(enriched)), key)
+
+
 class TestS3ClientClose(unittest.TestCase):
     """close() must release the underlying boto3 client; a slow leak
     here matters for long-running watch-mode processes."""