fix(headers): respect quoted strings when splitting header parameters

[athul-22]

Fixes #195. Supersedes #140's sibling PR #194, which fixed the Content-Type case alone with a local helper; that call site now uses the shared one instead.

Three structured header values were parsed by splitting on the separator and stripping quote characters, with no tracking of quoted strings. A quoted parameter value was not opaque, so a separator inside it ended the parameter and the text after it became a parameter or directive of its own.

Content-Disposition — a quoted filename could smuggle filename*

Content-Disposition: attachment; filename="a;filename*=UTF-8''evil.exe"

RFC 6266 makes the whole quoted string the filename value; there is no filename* here at all. Moli split on the inner ;, found a filename*, preferred it over the plain name, and saved the download as evil.exe.

Echoing a user-supplied filename into this header is common for file hosting, attachments and exports, and quoting the name — the usual advice — does not help, because the smuggled parameter travels inside the quotes. The name the site intended is discarded and the attacker picks the extension.

The same function already used a real RFC 6266 parser for the plain filename, so its detection scan and its extraction disagreed with each other.

Content-Type — a quoted parameter could displace the charset

value	before	after
text/html; boundary="; charset=gbk"	gbk	(none)
text/html; name="a\"; charset=gbk"; charset=utf-8	gbk	utf-8
text/html; charset="utf\-8"	utf\-8 → dropped → windows-1252	utf-8

This decides the document's transport encoding, which outranks the meta prescan and the fallback, so the third row is mojibake for any non-ASCII content.

Cache-Control — a quoted field list could leak directives

RFC 9111 lets private and no-cache carry a quoted field list containing commas. max-age=600, community="x, no-store, y" had the argument's own text read as directives, so a cacheable response was not stored. This one fails safe — the failure mode is a lost cache hit, not caching something it should not — but it is the same defect and is fixed alongside.

Approach

moli-header-field already exists for exactly this and already backs Content-Disposition parsing in moli-multipart; these three call sites predate it and hand-rolled the split. This adds split_outside_quoted_strings and unquote_parameter_value there and moves all three onto them.

Parsing the plain filename directly also removed the last use of the content_disposition crate, which splits the same way and would have reintroduced the truncation, so that dependency is dropped.

Notes for review

• Byte indices land only on ASCII bytes; a UTF-8 continuation byte can never equal ", \ or a separator, so scanning stays on character boundaries. A multibyte case is covered by a test.
• Existing tolerances are preserved deliberately rather than swapped for a strict parser. For Content-Type that means whitespace around =, apostrophe delimiters, an unterminated quoted string, uppercase parameter names and a trailing ;. I compared eighteen header forms before and after; only the three defects above change.
• split_outside_quoted_strings is pinned against plain split for inputs containing no quotes, so the common path is provably unchanged.

Verification

Per AGENTS.md, on aarch64-darwin:

• cargo fmt --all — clean
• cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
• moli-header-field 13 passed, moli-encoding 56 passed, moli-http-cache 49 passed, moli-protocol downloads suite 29 passed

cargo nextest run --no-fail-fast was still running locally when this was opened; CI covers it here.

[ldm0]

Thanks for consolidating the quoted-parameter handling. The intended quoted-separator fixes look good, including the Content-Disposition filename-smuggling case, but the shared parsing logic currently introduces three additional regressions:

[ldm0]

This toggles the quote state for every " in the field, even when the quote is not starting a parameter value after =. As a result, the Content-Type caller no longer recovers from an invalid parameter before a real charset.

For example:

assert_eq!(
    charset_from_content_type(r#"text/html;";charset=gbk"#).as_deref(),
    Some("gbk"),
);

This passes on main and is an explicit WPT MIME case. With this helper, the quote leaves inside_quotes set, so the following semicolon is ignored and the function returns None.

Could the semicolon-separated callers use parameter-aware parsing so quoted-string mode starts only when a parameter value actually begins with "? Please add this case as a regression test.

[athul-22]

Confirmed and fixed in f9d24a4. You are right that the field-wide toggle was wrong: text/html;";charset=gbk returned None here and gbk on main.

Quoted-string mode now begins only at the first non-whitespace character after =, which is the only place a parameter value can start, so a " anywhere else stays ordinary data and the separators behind it keep working. Added your case as header_charset_recovers_after_a_stray_quote, plus a_quote_outside_a_parameter_value_is_ordinary_data at the splitter level and whitespace_between_equals_and_a_quoted_value_still_opens_it to pin the boundary = "; x" form that must still open a quoted value.

[ldm0]

When \ is the final character, characters.next() returns None and this branch silently drops it. This can turn a malformed value into a valid one. For example, Cache-Control: max-age="31536000\ becomes 31536000, so this PR accepts a one-year freshness lifetime; on main, 31536000\ does not parse.

The Fetch quoted-string algorithm preserves a trailing backslash at EOF. Could we keep it here and add a regression test?

Suggested change

	if let Some(escaped) = characters.next() {
	match characters.next() {
	Some(escaped) => unquoted.push(escaped),
	None => unquoted.push('\\'),
	}

[athul-22]

Fixed in f9d24a4, taking your suggestion. Dropping the trailing backslash turned a malformed value into a well-formed one, which is the wrong direction for a cache lifetime.

Kept as a regression test in two places: a_trailing_backslash_is_kept_rather_than_dropped on the helper, and a_trailing_backslash_does_not_manufacture_a_freshness_lifetime in moli-http-cache, which asserts max-age="31536000\ yields no expiry.

[ldm0]

unquote_parameter_value has already removed the syntactic delimiters. Any " left in its result can be data produced by an escaped quote, so trimming double quotes here can create a valid encoding label from an invalid value.

For example:

let label =
    charset_from_content_type(r#"text/html; charset="utf-8\"""#).unwrap();

assert_eq!(label, "utf-8\"");
assert!(encoding_for_label(&label).is_none());

The quoted value is utf-8", but this line trims the final data quote and returns utf-8.

[athul-22]

Agreed, fixed in f9d24a4. The delimiters are gone by that point, so trimming was operating on data and could manufacture a valid label from an invalid one.

The apostrophe tolerance that trim was preserving only ever applied to unquoted values, so it is now applied only to those: a value starting with " goes through unquote_parameter_value untouched, anything else keeps the old trim_matches. Your example is pinned as header_charset_keeps_an_escaped_quote_as_data, asserting the label is utf-8" and that encoding_for_label rejects it.

[ldm0]

LGTM