diff --git a/backend/services/attachment_parser.py b/backend/services/attachment_parser.py index 7359d6b2f..de087087b 100644 --- a/backend/services/attachment_parser.py +++ b/backend/services/attachment_parser.py @@ -5,9 +5,8 @@ from dataclasses import dataclass from pathlib import Path from typing import Any -from urllib.parse import unquote -from .text_safety import strip_html_markup +from .text_safety import contains_html_markup, strip_html_markup _GENERIC_CONTENT_TYPES = { "", @@ -17,7 +16,6 @@ } MAX_ATTACHMENT_PARSE_SOURCE_CHARS = 1_000_000 MAX_ATTACHMENT_PARSE_SOURCE_BYTES = 20 * 1024 * 1024 -MAX_ATTACHMENT_FILENAME_DECODE_ROUNDS = 3 @dataclass(frozen=True) @@ -119,6 +117,15 @@ class AttachmentParserDescriptor: or descriptor.parse_status in _DEFERRED_PARSE_STATUSES for extension in descriptor.extensions } +_BIDI_FILENAME_CONTROL_CODEPOINTS = frozenset( + { + 0x061C, + 0x200E, + 0x200F, + *range(0x202A, 0x202F), + *range(0x2066, 0x206A), + } +) @dataclass(frozen=True) @@ -148,9 +155,10 @@ def parse_email_attachment( ) -> AttachmentParseResult: """Classify and normalize one attachment without running heavy parsers.""" safe_filename = _safe_filename(filename) + parser_filename = _parser_authority_filename(filename) normalized_content_type = _normalize_content_type(content_type) parse_content_type = _parse_content_type_for( - safe_filename, + parser_filename, normalized_content_type, ) @@ -266,26 +274,62 @@ def _parser_key_for(parse_content_type: str, parse_status: str) -> str: return "unsupported_binary" +def _has_unsafe_filename_control(filename: str) -> bool: + """Return whether a MIME filename contains unsafe control semantics.""" + return any( + (codepoint := ord(character)) < 0x20 + or 0x7F <= codepoint <= 0x9F + or codepoint in _BIDI_FILENAME_CONTROL_CODEPOINTS + for character in filename + ) + + def _safe_filename(filename: str | None) -> str: - """Return a basename-only attachment display filename.""" - display_filename = filename or "attachment" - for _ in range(MAX_ATTACHMENT_FILENAME_DECODE_ROUNDS): - decoded_filename = unquote(display_filename) - if decoded_filename == display_filename: - break - display_filename = decoded_filename - # Entity-encoded percent escapes (for example ``%2e``) only become - # literal ``%`` sequences during markup decoding, so the residual-encoding - # guard must run after ``strip_html_markup`` to stay fail-closed. - display_filename = strip_html_markup(_sanitize_nul(display_filename)) - if unquote(display_filename) != display_filename: + """Return a basename-only filename projection safe for display and storage. + + Display sanitization is intentionally separate from parser selection. Known + markup is stripped so active HTML cannot reach UI-facing attachment fields, + while raw angle-bracket labels and control-bearing names fail closed. + Percent and character-reference text that is not markup remains literal. + """ + raw_filename = filename or "attachment" + if _has_unsafe_filename_control(raw_filename): + return "attachment" + if contains_html_markup(raw_filename): + display_filename = strip_html_markup(raw_filename) + elif "<" in raw_filename or ">" in raw_filename: return "attachment" + else: + display_filename = raw_filename display_filename = Path(display_filename.replace("\\", "/")).name.strip() if display_filename in {"", ".", ".."}: return "attachment" return display_filename +def _parser_authority_filename(filename: str | None) -> str: + """Return the literal basename eligible to select a parser by extension. + + MIME filename identity is neither HTML nor URL source. Parser authority must + therefore use the pre-display representation: semantic decoding, markup + stripping, control deletion, or whitespace trimming must never manufacture + a recognized suffix for a generic MIME type. + """ + raw_filename = filename or "attachment" + if _has_unsafe_filename_control(raw_filename): + return "attachment" + if ( + "<" in raw_filename + or ">" in raw_filename + or contains_html_markup(raw_filename) + ): + return "attachment" + authority_filename = Path(raw_filename.replace("\\", "/")).name + if authority_filename in {"", ".", ".."}: + return "attachment" + return authority_filename + + def _coerce_deferred_payload_bytes(raw_content: Any) -> bytes: """Return the exact byte payload retained for deferred recognition.""" if isinstance(raw_content, bytes): diff --git a/backend/tests/test_attachment_filename_identity.py b/backend/tests/test_attachment_filename_identity.py new file mode 100644 index 000000000..6803c79ae --- /dev/null +++ b/backend/tests/test_attachment_filename_identity.py @@ -0,0 +1,201 @@ +"""Regression contracts for MIME attachment filename identity.""" + +from services.attachment_parser import _safe_filename, parse_email_attachment +from services.email_parser import parse_eml_bytes + + +def test_html_entity_dot_does_not_change_generic_mime_parser() -> None: + """HTML entity syntax is literal MIME filename text, not an extension codec.""" + result = parse_email_attachment( + filename="quarterly.json", + content_type="application/octet-stream", + raw_content=b'{"project":"Launch"}', + ) + + assert result.filename == "quarterly.json" + assert result.parse_content_type == "application/octet-stream" + assert result.parser_key == "unsupported_binary" + assert result.parse_status == "unsupported_content_type" + + +def test_html_display_sanitization_cannot_smuggle_generic_mime_extension() -> None: + """Safe display projection must not become parser-selection authority.""" + result = parse_email_attachment( + filename="quarterly.json", + content_type="application/octet-stream", + raw_content=b'{"project":"Launch"}', + ) + + assert result.filename == "quarterly.json" + assert result.parse_content_type == "application/octet-stream" + assert result.parser_key == "unsupported_binary" + assert result.parse_status == "unsupported_content_type" + + +def test_unknown_angle_bracket_filename_cannot_select_generic_parser() -> None: + """Unknown tag-shaped filename text must not become extension authority.""" + result = parse_email_attachment( + filename=".json", + content_type="application/octet-stream", + raw_content=b'{"project":"Launch"}', + ) + + assert result.filename == "attachment" + assert result.parse_content_type == "application/octet-stream" + assert result.parser_key == "unsupported_binary" + assert result.parse_status == "unsupported_content_type" + + +def test_nul_filename_cannot_create_generic_mime_extension_authority() -> None: + """A production EML NUL must not disappear into a parser-recognized suffix.""" + parsed = parse_eml_bytes( + b"Message-ID: \r\n" + b"From: sender@test.com\r\n" + b"To: recipient@test.com\r\n" + b"Subject: NUL filename\r\n" + b"Date: Mon, 27 Apr 2026 10:00:00 +0000\r\n" + b'Content-Type: multipart/mixed; boundary="mixed-boundary"\r\n' + b"\r\n" + b"--mixed-boundary\r\n" + b"Content-Type: text/plain; charset=utf-8\r\n" + b"\r\n" + b"See attached.\r\n" + b"--mixed-boundary\r\n" + b"Content-Type: application/octet-stream\r\n" + b'Content-Disposition: attachment; filename="quarterly.json\x00"\r\n' + b"\r\n" + b'{"project":"Launch"}\r\n' + b"--mixed-boundary--\r\n" + ) + + attachment = parsed["attachments"][0] + assert attachment["filename"] == "attachment" + assert attachment["parse_content_type"] == "application/octet-stream" + assert attachment["parser_key"] == "unsupported_binary" + assert attachment["parse_status"] == "unsupported_content_type" + + +def test_rfc2231_control_character_filename_fails_closed() -> None: + """RFC 2231 decoding must not turn control-bearing names into parser authority.""" + parsed = parse_eml_bytes( + b"Message-ID: \r\n" + b"From: sender@test.com\r\n" + b"To: recipient@test.com\r\n" + b"Subject: Control filename\r\n" + b"Date: Mon, 27 Apr 2026 10:00:00 +0000\r\n" + b'Content-Type: multipart/mixed; boundary="mixed-boundary"\r\n' + b"\r\n" + b"--mixed-boundary\r\n" + b"Content-Type: text/plain; charset=utf-8\r\n" + b"\r\n" + b"See attached.\r\n" + b"--mixed-boundary\r\n" + b"Content-Type: application/octet-stream\r\n" + b"Content-Disposition: attachment; " + b"filename*=utf-8''quarterly%0A.json\r\n" + b"\r\n" + b'{"project":"Launch"}\r\n' + b"--mixed-boundary--\r\n" + ) + + attachment = parsed["attachments"][0] + assert attachment["filename"] == "attachment" + assert attachment["parse_content_type"] == "application/octet-stream" + assert attachment["parser_key"] == "unsupported_binary" + assert attachment["parse_status"] == "unsupported_content_type" + + +def test_filename_controls_fail_closed_before_display_or_parser_selection() -> None: + """C0/C1 controls are not valid display or parser-authority characters.""" + for control in ("\t", "\x1b", "\x7f", "\x85"): + filename = f"quarterly{control}.json" + result = parse_email_attachment( + filename=filename, + content_type="application/octet-stream", + raw_content=b'{"project":"Launch"}', + ) + + assert _safe_filename(filename) == "attachment" + assert result.filename == "attachment" + assert result.parse_content_type == "application/octet-stream" + assert result.parser_key == "unsupported_binary" + assert result.parse_status == "unsupported_content_type" + + +def test_rfc2231_bidi_control_filename_fails_closed() -> None: + """RFC 2231 decoding must not retain invisible display-order authority.""" + parsed = parse_eml_bytes( + b"Message-ID: \r\n" + b"From: sender@test.com\r\n" + b"To: recipient@test.com\r\n" + b"Subject: Bidi filename\r\n" + b"Date: Mon, 27 Apr 2026 10:00:00 +0000\r\n" + b'Content-Type: multipart/mixed; boundary="mixed-boundary"\r\n' + b"\r\n" + b"--mixed-boundary\r\n" + b"Content-Type: text/plain; charset=utf-8\r\n" + b"\r\n" + b"See attached.\r\n" + b"--mixed-boundary\r\n" + b"Content-Type: application/octet-stream\r\n" + b"Content-Disposition: attachment; " + b"filename*=utf-8''quarterly%E2%80%AEfdp.json\r\n" + b"\r\n" + b'{"project":"Launch"}\r\n' + b"--mixed-boundary--\r\n" + ) + + attachment = parsed["attachments"][0] + assert attachment["filename"] == "attachment" + assert attachment["parse_content_type"] == "application/octet-stream" + assert attachment["parser_key"] == "unsupported_binary" + assert attachment["parse_status"] == "unsupported_content_type" + + +def test_bidi_controls_fail_closed_without_rejecting_bidi_scripts() -> None: + """Unicode Bidi_Control characters fail closed; ordinary RTL text remains valid.""" + bidi_controls = ( + "\u061c", + "\u200e", + "\u200f", + "\u202a", + "\u202b", + "\u202c", + "\u202d", + "\u202e", + "\u2066", + "\u2067", + "\u2068", + "\u2069", + ) + for control in bidi_controls: + filename = f"quarterly{control}.json" + result = parse_email_attachment( + filename=filename, + content_type="application/octet-stream", + raw_content=b'{"project":"Launch"}', + ) + + assert _safe_filename(filename) == "attachment" + assert result.filename == "attachment" + assert result.parse_content_type == "application/octet-stream" + assert result.parser_key == "unsupported_binary" + assert result.parse_status == "unsupported_content_type" + + rtl_filename = "تقرير-ربع-سنوي.json" + rtl_result = parse_email_attachment( + filename=rtl_filename, + content_type="application/octet-stream", + raw_content=b'{"project":"Launch"}', + ) + assert _safe_filename(rtl_filename) == rtl_filename + assert rtl_result.filename == rtl_filename + assert rtl_result.parser_key == "json" + assert rtl_result.parse_status == "parsed" + + +def test_benign_ampersand_filename_remains_literal() -> None: + """Ordinary filename punctuation remains unchanged.""" + assert _safe_filename("quarterly report & notes.pdf") == ( + "quarterly report & notes.pdf" + ) \ No newline at end of file diff --git a/backend/tests/test_attachment_parser.py b/backend/tests/test_attachment_parser.py index 4eeb27228..f6da18971 100644 --- a/backend/tests/test_attachment_parser.py +++ b/backend/tests/test_attachment_parser.py @@ -258,22 +258,41 @@ def test_deferred_pdf_decoder_rejects_non_pdf_and_oversized_payloads(monkeypatch decode_deferred_attachment_payload(oversized) -def test_safe_filename_handles_windows_path_traversal(): +def test_literal_percent_escape_does_not_change_attachment_parser(): + result = parse_email_attachment( + filename="quarterly%2Ejson", + content_type="application/octet-stream", + raw_content=b'{"project":"Launch"}', + ) + + assert result.filename == "quarterly%2Ejson" + assert result.parse_content_type == "application/octet-stream" + assert result.parser_key == "unsupported_binary" + assert result.parse_status == "unsupported_content_type" + + +def test_safe_filename_strips_literal_path_segments_without_percent_decoding(): assert _safe_filename("..\\..\\upload.txt") == "upload.txt" assert _safe_filename("C:\\mail\\report.pdf") == "report.pdf" - assert _safe_filename("%5c%2e%2e%5csecret.txt") == "secret.txt" - assert _safe_filename("%252e%252e%252fsecret.txt") == "secret.txt" - assert _safe_filename("%252525252e%252525252e%252525252fsecret.txt") == "attachment" + assert _safe_filename("%5c%2e%2e%5csecret.txt") == "%5c%2e%2e%5csecret.txt" + assert _safe_filename("%252e%252e%252fsecret.txt") == ( + "%252e%252e%252fsecret.txt" + ) + assert _safe_filename("%252525252e%252525252e%252525252fsecret.txt") == ( + "%252525252e%252525252e%252525252fsecret.txt" + ) -def test_safe_filename_fails_closed_after_entity_decoding(): - """Entity-encoded percent escapes must trip the residual guard post-decode.""" - assert _safe_filename("%2e%2e%2fsecret.txt") == "attachment" +def test_safe_filename_preserves_entity_encoded_percent_text(): + """MIME filename character references remain literal filename text.""" + assert _safe_filename("%2e%2e%2fsecret.txt") == ( + "%2e%2e%2fsecret.txt" + ) -def test_safe_filename_plain_percent_encoded_traversal_still_decodes_to_basename(): - """Single percent-encoded traversal still decodes in-round to its basename.""" - assert _safe_filename("%2e%2e%2fsecret.txt") == "secret.txt" +def test_safe_filename_preserves_plain_percent_encoded_text(): + """MIME filenames are not URL paths and must not be percent-decoded again.""" + assert _safe_filename("%2e%2e%2fsecret.txt") == "%2e%2e%2fsecret.txt" def test_safe_filename_benign_name_survives_unchanged(): diff --git a/docs/doctoring/mime-attachment-filename-identity.md b/docs/doctoring/mime-attachment-filename-identity.md new file mode 100644 index 000000000..db9f3c1c2 --- /dev/null +++ b/docs/doctoring/mime-attachment-filename-identity.md @@ -0,0 +1,54 @@ +# MIME attachment filename identity and parser authority + +**Status:** open-PR evidence only; not yet shipped on protected `develop`. + +## Boundary under repair + +Naruon receives attachment filenames from Python's MIME parser and uses them for two different purposes: a human-facing display/storage filename and, only when the declared media type is generic, a filename-extension hint for parser selection. Those purposes must not share a representation-transform pipeline. + +The display/storage projection may remove known active markup and literal path segments. The parser-authority projection must not be derived from that sanitized display value, because percent decoding, entity decoding, markup stripping, control deletion, or whitespace normalization can manufacture a recognized suffix that the sender did not present to the parser boundary. + +The current PR therefore keeps the two projections separate. Generic-MIME extension fallback uses only the literal parser-authority projection. C0, DEL, C1, and Unicode `Bidi_Control` characters fail closed to `attachment` for both display and parser selection; percent/entity text remains literal unless the MIME parser itself has already decoded a standards-defined MIME parameter encoding. Ordinary Arabic/Hebrew and other right-to-left script text remains allowed: the restriction is on explicit/implicit directional formatting controls, not on bidirectional scripts. + +## Standards traceability + +RFC 2183 defines the MIME `Content-Disposition` filename as a sender-suggested value, not trusted local authority. Section 2.3 requires a receiving MUA to check and possibly change the suggested filename so it conforms to local conventions and does not present a security problem, and says apparent directory path information should not be respected. Naruon therefore treats the value as a terminal component and rejects representations that are unsafe for display or parser authority. + +RFC 2231 extends MIME parameter values with character-set/language information and percent-encoded octets. That decoding belongs to the MIME parameter layer. With Python `policy.default`, `Message.get_filename()` already performs the RFC 2231 decoding before Naruon's attachment parser receives the filename. Naruon must not apply a second URL-style percent decode to that already-decoded identity. + +A production-ingress probe also shows why control characters need an explicit fail-closed rule. The RFC 2231 parameter `filename*=utf-8''quarterly%0A.json` is exposed by `Message.get_filename()` as `quarterly\n.json`. Without a control guard, `Path(...).suffix` still returns `.json`, allowing a generic `application/octet-stream` part to select the JSON parser while the display/storage value contains a line control. The regression contract fixes that exact MIME ingress rather than testing an artificial helper-only string. + +Unicode Standard Annex #9 defines the `Bidi_Control` set used to influence display ordering: ALM, LRM, RLM, LRE, RLE, LRO, RLO, PDF, LRI, RLI, FSI, and PDI. UAX #9 explicitly distinguishes logical order from visual order and notes security concerns around bidirectional text; directional overrides are to be avoided where possible because of those concerns. A MIME attachment filename is both identity-bearing input and user-facing text, so retaining invisible directional controls permits a sender to make the visible filename diverge from its logical code-point order without changing the stored string. The RFC 2231 production regression `filename*=utf-8''quarterly%E2%80%AEfdp.json` proves that U+202E RIGHT-TO-LEFT OVERRIDE reaches Naruon's filename boundary after standards-defined MIME decoding. Naruon therefore rejects the Unicode `Bidi_Control` property at this trust boundary while preserving ordinary RTL-script filenames that contain no directional formatting control. + +## TDD and implementation traceability + +- Protected base: `develop@042b0c70531b229af3acbd0421a2f23098d848b3`. +- Existing representation-separation causal head: `fea71c7fc2b49d7d47b0c91862786bccddd29d07`. +- Control-character RED: `288136ef8b1a6ffd4c1d910fcd9654d25671f9d1` adds RFC 2231 newline ingress plus C0/C1 unit cases. On the predecessor implementation, control-bearing values remain display filenames and a trailing `.json` retains generic-MIME parser authority. +- Control-character causal fix: `92ec6151d11d9b125880a0405dab9ef59bc9293a` adds `_has_unsafe_filename_control()` and applies it before either display projection or parser-authority projection. +- Bidi-control RED: `51f45492309b40799d7a026c52688a6a999a043e` adds a real RFC 2231 U+202E ingress regression plus all twelve Unicode `Bidi_Control` code-point cases. The predecessor helper rejects only C0/DEL/C1, so it retains the controls and generic `.json` parser authority. +- Bidi-control causal fix: `ddc11593ac5af1528c9068f3275be728e28f3dc8` adds the UAX #9 `Bidi_Control` code-point set to the same fail-closed boundary. The regression also proves that an Arabic filename without formatting controls remains valid and continues to select the JSON parser under generic MIME. +- Product code: `backend/services/attachment_parser.py`. +- Production-ingress regressions: `backend/tests/test_attachment_filename_identity.py::test_rfc2231_control_character_filename_fails_closed` and `::test_rfc2231_bidi_control_filename_fails_closed`. +- Edge regressions: `backend/tests/test_attachment_filename_identity.py::test_filename_controls_fail_closed_before_display_or_parser_selection` and `::test_bidi_controls_fail_closed_without_rejecting_bidi_scripts`. + +No CardDAV/DAV/HTTP URL canonicalization, declared non-generic MIME type, attachment payload parser, PDF byte validation, or ordinary body-text sanitization is changed by this decision. + +## Verification required before merge + +The unchanged exact PR head must run the focused filename/parser suite and the normal Python 3.14 suite, then pass all repository-required security, dependency, coverage, image, SBOM/provenance, and review gates. The focused command is: + +```bash +cd backend +python -m pytest tests/test_attachment_filename_identity.py tests/test_attachment_parser.py -q +``` + +Queued, absent, startup-failed, predecessor-head, or author-only evidence is non-passing. The current organization Actions queue/startup failure is tracked through the canonical `.github` owner path rather than by weakening Naruon's required checks. + +## References (APA 7th) + +Freed, N., & Moore, K. (1997). *MIME parameter value and encoded word extensions: Character sets, languages, and continuations* (RFC 2231). RFC Editor. https://doi.org/10.17487/RFC2231 + +The Unicode Consortium. (2025). *Unicode Bidirectional Algorithm* (Unicode Standard Annex #9, Version 17.0.0, Revision 51). https://www.unicode.org/reports/tr9/ + +Troost, R., Dorner, S., & Moore, K. (1997). *Communicating presentation information in Internet messages: The Content-Disposition header field* (RFC 2183). RFC Editor. https://doi.org/10.17487/RFC2183