escape attachment filename in Content-Disposition header#69435
Conversation
|
Confirmed the root cause and the fix. build_mime_message and SmtpHook._build_mime_message both built Content-Disposition via an f-string, so a filename containing a double quote (e.g. report.txt"; x-evil="1) closes the quoted value and injects an extra parameter. Switching to part.add_header("Content-Disposition", "attachment", filename=basename) hands escaping to the stdlib. Ordinary names come out byte-identical to the old output (attachment; filename="report.txt"), names with a quote get backslash-escaped inside the quotes with no injected param, and non-ASCII names are RFC 2231 encoded; get_filename() round-trips in every case. Both call sites are fixed and each has a regression test that fails against the old f-string (asserts get_filename() equals the raw name and x-evil is not in the parsed params). I also checked the imap provider hook, which showed up in a search for the same pattern, but it only parses received Content-Disposition headers, so there is no third site to fix. One nit: the description says RFC 2231-encoded, but for ASCII-with-quote inputs it is actually quoted-string backslash escaping (RFC 2231 only kicks in for non-ASCII) -- same security result, just worth correcting in the summary. Practical severity is limited since attachment filenames are usually operator/DAG-author controlled, so this is hardening more than a high-severity exploit. LGTM. |
|
Good catch on the RFC 2231 wording. The quoted-string backslash escaping only crosses over into 2231 encoding for non-ASCII names, so I reworded the description to spell out both cases (plain quoted-string for the ASCII-with-quote path, 2231 for non-ASCII). Same behavior in code, just clearer in the summary. Thanks for the review. |
build_mime_message in airflow.utils.email and SmtpHook._build_mime_message build the attachment Content-Disposition with an f-string, so a filename containing a double quote breaks out of the quoted value and injects extra Content-Disposition parameters, letting a crafted attachment name override the filename the recipient's mail client shows or saves. Switch both to Message.add_header with a filename parameter, which lets the stdlib escape the value: ordinary names serialize exactly as before, an ASCII name with a quote is backslash-escaped inside the quoted-string, and non-ASCII names are RFC 2231 encoded. get_filename() round-trips in every case.
Was generative AI tooling used to co-author this PR?