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

[qingqing-xiaowen]

Hi @athul-22 — thank you again for the careful work in #196. The original filename-smuggling and charset cases were already valuable, and the way you followed up on review—only opening quoted-string mode after =, preserving a trailing backslash, and keeping escaped quotes as data—helped us understand the parser boundaries much more clearly.

If you’re open to it, we’d really like to hear more about the workflow behind those cases: what kind of page, download/export, or agent task led you to them, where Moli helped, and whether any header or encoding behavior still gets in the way. We’re also interested in what you tried before or alongside Moli and what you would most like us to improve next.

We can continue entirely here on GitHub in writing. If email or another online channel is easier, that’s fine too. There’s nothing to prepare and no meeting is required; voice or video is completely optional.

We’ll make sure anything you share is brought back to the team as product input. If there’s relevant progress we can share later, we’ll do our best to update you here. Even a short reply would help, and no worries at all if now isn’t a good time.