Skip to content

[mime] Harden BoundMultipartStream error routing, headers, and epilogue - #2629

Merged
kevmoo merged 6 commits into
mainfrom
fix-mime
Oct 30, 2026
Merged

[mime] Harden BoundMultipartStream error routing, headers, and epilogue#2629
kevmoo merged 6 commits into
mainfrom
fix-mime

Conversation

@kevmoo

@kevmoo kevmoo commented Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Fix three BoundMultipartStream failure modes on malformed or trailing multipart input:

  • Route synchronous _parse() MimeMultipartExceptions and non-UTF-8 header FormatExceptions through _controller.addError instead of throwing uncaught or hanging paused subscriptions.
  • Validate _isTokenChar on the first byte of header field names.
  • Reset _buffer and resume _subscription when exiting _parse() in _doneCode so trailing epilogue bytes after --boundary-- do not hang the stream.

@kevmoo
[mime] Route multipart parse and header UTF-8 errors to stream
78ec86b 
Catch `MimeMultipartException` thrown synchronously inside
`BoundMultipartStream._parse()` during `onData`, transition to
`_failCode`, forward the exception to `_controller.addError`, cancel
the upstream subscription, and close the output stream so downstream
consumers receive the error instead of hanging or leaking uncaught zone
exceptions.

Also wrap `FormatException` from `utf8.decode` when decoding multipart
header fields and values into `MimeMultipartException`.
@kevmoo
kevmoo requested a review from lrhn as a code owner October 2, 2026 16:21
@github-actions

github-actions Bot commented Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Package publishing

Package Version Status Publish tag (post-merge)
package:mime ⭐ 2.1.1-wip WIP (no publish necessary)
  • 19 already published.
  • 23 WIP (no publish necessary).

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@github-actions

github-actions Bot commented Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
mime None 2.1.0 2.1.1-wip 2.1.1-wip ✔️

This check can be disabled by tagging the PR with skip-breaking-check.

Coverage ⚠️
File Coverage
pkgs/mime/lib/src/bound_multipart_stream.dart 💚 93 % ⬆️ 5 %
pkgs/mime/lib/src/char_code.dart 💔 Not covered

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check (for example, for non-user-visible changes when the package version is already at -wip).

License Headers ✔️ 
// Copyright (c) 2026, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/html/test/differential_test.dart
pkgs/html/test/dom_parsing_test.dart
pkgs/html/test/mxss_test.dart
pkgs/html/test/noah_ark_clause_limit_test.dart

This check can be disabled by tagging the PR with skip-license-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbol Leaking sources

This check can be disabled by tagging the PR with skip-leaking-check.

Unused Dependencies ✔️
Package Status
mime ✔️ All dependencies utilized correctly.

For details on how to fix these, see dependency_validator.

This check can be disabled by tagging the PR with skip-unused-dependencies-check.

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Oct 2, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the mime package to forward mid-stream multipart parse errors and invalid UTF-8 header bytes to the output stream as a MimeMultipartException instead of throwing uncaught synchronous errors or raw FormatExceptions. It implements error handling in BoundMultipartStream during parsing and UTF-8 decoding, updates stream cleanup logic, and adds comprehensive tests covering these scenarios. There are no review comments, and I have no feedback to provide.

@kevmoo
[mime] Validate first header field byte, fix epilogue hang, and remov…
5d561c2 
…e dead code

* Validate the first byte of multipart header field names with `_isTokenChar`
  in `_headerStartCode` and `_headerValueFoldOrEndCode` so invalid initial
  characters (such as control bytes, spaces, or `:` on empty field names)
  throw `MimeMultipartException` instead of bypassing validation.
* Always reset `_buffer` and `_index` and resume `_subscription` at the end
  of `BoundMultipartStream._parse()`, preventing a stream hang (or buffer
  assertion failure on subsequent chunks) when trailing epilogue bytes follow
  the closing `--boundary--\r\n` delimiter.
* Remove unreachable duplicate `_multipartController` cleanup in
  `_boundaryEndCode` and `_lastBoundaryEndCode` (already closed and nulled in
  `_contentCode`).
@kevmoo kevmoo changed the title [mime] Route multipart parse and header UTF-8 errors to stream [mime] Harden BoundMultipartStream error routing, headers, and epilogue Oct 3, 2026
davidmorgan
davidmorgan reviewed Oct 23, 2026
View reviewed changes

@davidmorgan davidmorgan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. analyze and format are clean. The suite passes on head (+102) and main (+87). On main, the new epilogue tests time out, so they cover the fix.

On main and head, I ran probes over valid input, byte-at-a-time chunking, epilogue in the same chunk, the next chunk and byte-wise, truncation, invalid header bytes, non-UTF-8 headers, LF-only line endings and a source error mid-part. Every uncaught zone error and hang on main is gone, and valid input parses the same. One behaviour change is worth a look; see the comment on the onDone handler.

Observation, no action needed: header parsing is stricter. A first header line starting with a space, or with a separator such as @, used to give a header named a or @; now it's an error. That matches how later bytes of the name were already checked, and the CHANGELOG says so.

Pre-existing, not from this PR: _isTokenChar accepts 127 (DEL), which RFC 7230 excludes from tokens, so a header name containing DEL is still accepted.

Comment thread pkgs/mime/lib/src/bound_multipart_stream.dart Resolved
Comment thread pkgs/mime/lib/src/bound_multipart_stream.dart Resolved
Comment thread pkgs/mime/test/mime_multipart_transformer_test.dart Resolved
Comment thread pkgs/mime/test/mime_multipart_transformer_test.dart Resolved
@davidmorgan

davidmorgan commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

This is well outside my areas of expertise, would it make sense to get a review from someone familiar with the package too e.g. @lrhn?

@kevmoo

kevmoo commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

Addressed all inline comments. PTAL!

@kevmoo
kevmoo requested a review from davidmorgan October 23, 2026 17:14
kevmoo added 2 commits October 23, 2026 19:02
@kevmoo
[mime] Propagate source errors to active part stream and decode heade…
9f06441 
…r names as ASCII

* Forward source stream `onError` events to both the outer multipart stream and
  any active part stream, marking `_state = _failCode` before `onDone`.
* Decode validated multipart header field names via `ascii.decode` outside the
  header value `utf8.decode` `try` block, use a `const` `MimeMultipartException`
  for truncated endings, and clear `_multipartController` after closing.
* Add unit tests covering mid-part EOF truncation and mid-part source stream
  error propagation on active part streams.
@kevmoo
[mime] Consolidate stream failure cleanup in _fail and simplify tests
7e11b50 
- Extract `BoundMultipartStream._fail` shared by `onData` (`on MimeMultipartException`), `onDone` (`_state != _doneCode && _state != _failCode`), and `onError` so source errors also close and null `_multipartController`, cancel `_subscription`, and close `_controller` without double-closing in `onDone`.
- Remove redundant `.toLowerCase()` on `headerField` (bytes in `_headerField` are already lowercased on insertion).
- Replace manual `Completer`/`Future.wait` plumbing in the error-forwarding tests with `emitsInOrder` + `emitsDone`, including an unclosed source error test case.
davidmorgan
davidmorgan approved these changes Oct 26, 2026
View reviewed changes

@davidmorgan davidmorgan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM--but same comment as before--please consider whether somebody who knows HTTP multipart things should take a look too :)

@kevmoo

kevmoo commented Oct 27, 2026

Copy link
Copy Markdown
Member Author

er whether somebody who knows HTTP multipart things should take a look too :)

I don't know if anyone on the team fits in there...

@davidmorgan

davidmorgan commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

Up to you :) I already tagged @lrhn whose opinion usually worth the wait ;)

lrhn
lrhn reviewed Oct 28, 2026
View reviewed changes

@lrhn lrhn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all in your code. Feel free to ignore. (I'll have a look at it later then.)

Comment thread pkgs/mime/test/mime_multipart_transformer_test.dart Resolved
Comment thread pkgs/mime/test/mime_multipart_transformer_test.dart Resolved
Comment thread pkgs/mime/test/mime_multipart_transformer_test.dart Resolved
Comment thread pkgs/mime/test/mime_multipart_transformer_test.dart Resolved
Comment thread pkgs/mime/test/mime_multipart_transformer_test.dart Resolved
Comment thread pkgs/mime/lib/src/bound_multipart_stream.dart Resolved
Comment thread pkgs/mime/lib/src/bound_multipart_stream.dart Resolved
Comment thread pkgs/mime/lib/src/bound_multipart_stream.dart Resolved
Comment thread pkgs/mime/lib/src/bound_multipart_stream.dart Resolved
Comment thread pkgs/mime/lib/src/bound_multipart_stream.dart Resolved
@kevmoo
[mime] Address review comments on BoundMultipartStream and tests
ba962bb 
- Await `_subscription.cancel()` before closing `_controller` in `_fail`, pattern-match `_multipartController`, and drop the unreachable `_state != _failCode` check in `onDone`.
- Bound `_isTokenChar` at `< 127` to exclude ASCII `0x7F` (`DEL`), express `_separators` with hex `char_code` constants, and include `FormatException.message` when header UTF-8 decoding fails.
- Use collection literals, deduplicated invalid-header inputs, inline syntax error comments, and `Future.delayed(Duration.zero)` in `mime_multipart_transformer_test.dart`.
@kevmoo

kevmoo commented Oct 28, 2026

Copy link
Copy Markdown
Member Author

Thanks for the thorough review! Addressed all 13 comments in ba962bb (awaited _subscription.cancel() before closing _controller, pattern-matched _multipartController, dropped _state != _failCode in onDone, bounded _isTokenChar at < 127 with hex char_code constants, included FormatException.message, and cleaned up/documented the tests). PTAL!

@kevmoo
kevmoo requested a review from lrhn October 28, 2026 18:23
@kevmoo
kevmoo merged commit 14ad5e5 into main Oct 30, 2026
21 checks passed
@kevmoo
kevmoo deleted the fix-mime branch October 30, 2026 17:49
copybara-service Bot pushed a commit to dart-lang/sdk that referenced this pull request Nov 3, 2026
@natebosch dart-scoped@luci-project-accounts.iam.gserviceaccount.com
[deps] rev core, ecosystem, http, i18n, shelf, test, tools
c48ebb8 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

core (https://github.com/dart-lang/core/compare/d7bd45c..24e09c4):
  24e09c4c  Sun Nov 1 21:12:19 2026 +0000  dependabot[bot]  Bump the github-actions group with 4 updates (dart-lang/core#1037)
  87ac1b4c  Fri Oct 30 16:02:43 2026 +0100  Michael Thomsen  [os_detect] Deprecate package:os_detect in favor of package:platform (dart-lang/core#1003)
  8debf335  Fri Oct 30 01:57:36 2026 +0530  Sahaja Pallapothula  Clarify Characters equality and Unicode normalization (dart-lang/core#1031)

ecosystem (https://github.com/dart-lang/ecosystem/compare/24b9ba8..f8faabf):
  f8faabf  Sun Nov 1 18:16:20 2026 +0000  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/ecosystem#481)
  27c2fc8  Sat Oct 31 12:36:53 2026 +0100  Moritz  Fix publish workflow pins, add tag_prefix to health, and update docs (dart-lang/ecosystem#480)
  05641a9  Sat Oct 31 09:56:42 2026 +0100  Moritz  Restore deprecated publish.yaml workflow and firehose CLI entrypoint (dart-lang/ecosystem#479)
  78abf72  Fri Oct 30 08:24:07 2026 -0700  Kevin Moore  fix(repo_manage): use pub_semver for changelog -wip transitions (dart-lang/ecosystem#475)
  0bfa654  Fri Oct 30 09:33:57 2026 +0100  Moritz  Move publish check to health workflow and use individual package publishing workflows from `setup-dart` (dart-lang/ecosystem#454)
  adf7979  Thu Oct 29 23:46:49 2026 -0700  Kevin Moore  test(firehose): use hasLength, containsPair, and isFalse (dart-lang/ecosystem#477)

http (https://github.com/dart-lang/http/compare/c76dfb3..8c0d66a):
  8c0d66a  Fri Oct 30 09:54:41 2026 -0700  Brian Quinlan  chore(cupertino_http): check ffigen generation (dart-lang/http#2044)
  c8737d1  Fri Oct 30 09:11:46 2026 +0100  Moritz  chore(http): run Firefox tests with -headless instead of xvfb-run (dart-lang/http#2033)
  05d0b18  Fri Oct 30 07:08:19 2026 +0100  Moritz  fix(http2): make StreamException a StreamTransportException and forward ALPN errors to onError (dart-lang/http#2009)

i18n (https://github.com/dart-lang/i18n/compare/4c7b4ad..7475ce6):
  7475ce64  Sat Oct 31 02:49:28 2026 +0100  Moritz  [intl4x] Fix NumberFormat.percent ignoring digits (dart-lang/i18n#1115)

shelf (https://github.com/dart-lang/shelf/compare/62d3e4e..9946de5):
  9946de5  Sun Nov 1 12:27:59 2026 +0000  dependabot[bot]  build(deps): bump the github-actions group with 3 updates (dart-lang/shelf#554)

test (https://github.com/dart-lang/test/compare/6d74720..2a9e017):
  2a9e0175  Sun Nov 1 03:47:24 2026 +0000  dependabot[bot]  Bump the github-actions group with 6 updates (dart-lang/test#2813)
  55cd5cd5  Fri Oct 30 13:43:47 2026 -0700  Nate Bosch  Migrate publishing workflows from firehose to setup-dart (dart-lang/test#2812)
  10dc78c0  Fri Oct 30 11:21:53 2026 -0700  Nate Bosch  [checks] Describe what the stream did in failures (dart-lang/test#2811)
  c11ad06d  Fri Oct 30 08:59:57 2026 -0700  Nate Bosch  [checks] Add CheckFailure.describeNested (dart-lang/test#2793)
  0137b43e  Fri Oct 30 05:40:43 2026 +0530  Shriraj Jadhav  [checks] Use the Iterable.last member for the last expectation (dart-lang/test#2807)
  50c9f454  Thu Oct 29 15:11:39 2026 -0700  Nate Bosch  [checks] Collapse clauses for described conditions (dart-lang/test#2794)

tools (https://github.com/dart-lang/tools/compare/3980dca..1579f8d):
  1579f8d69  Mon Nov 2 02:44:58 2026 +0530  Joshua  fix(coverage): reject package: and dart: URIs outside known roots in Resolver (dart-lang/tools#2764)
  f869fdca4  Sun Nov 1 10:26:34 2026 +0100  dart-ecosystem-bot[bot]  [boring] Update BoringSSL to ed9209cf (dart-lang/tools#2774)
  d10e2b87f  Sun Nov 1 10:26:11 2026 +0100  dart-ecosystem-bot[bot]  [boring] Pin the prebuilt binaries of boring-binaries-20261101-33e9e69722af (dart-lang/tools#2777)
  33e9e6972  Sun Nov 1 05:56:30 2026 +0000  dependabot[bot]  build(deps): bump the github-actions group with 4 updates (dart-lang/tools#2775)
  f84d0d0d2  Sun Nov 1 05:36:12 2026 +0000  dependabot[bot]  build(deps): bump ffigen from 22.0.0 to 23.0.0 in /pkgs/boring (dart-lang/tools#2776)
  14ad5e515  Fri Oct 30 10:48:58 2026 -0700  Kevin Moore  [mime] Harden BoundMultipartStream error routing, headers, and epilogue (dart-lang/tools#2629)
  7a780d48c  Fri Oct 30 12:57:03 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid rescanning failed plain-text runs in InlineParser (dart-lang/tools#2757)
  3671d33a9  Fri Oct 30 12:56:48 2026 +0100  Jonas Finnemann Jensen  [markdown] Index footnote definitions and labels on Document for O(1) lookup (dart-lang/tools#2760)
  6f38e9fda  Fri Oct 30 12:56:09 2026 +0100  Jonas Finnemann Jensen  [markdown] Bound the domain of extended URL autolinks to DNS limits (dart-lang/tools#2759)
  aa0c38544  Fri Oct 30 12:20:40 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid quadratic removeRange in _combineAdjacentText (dart-lang/tools#2758)
  f425baf46  Fri Oct 30 11:16:28 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid rescanning failed space runs in LineBreakSyntax (dart-lang/tools#2756)
  c7eafbec3  Fri Oct 30 11:09:45 2026 +0100  Jonas Finnemann Jensen  [markdown] Reject '<' in AutolinkSyntax (dart-lang/tools#2755)
  86ed0f902  Fri Oct 30 11:08:25 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid quadratic backtracking in headerPattern (dart-lang/tools#2754)
  8ea07517f  Fri Oct 30 07:45:42 2026 +0100  Moritz  [boring] Regenerate the x509-limbo expected failures in the refresh job (dart-lang/tools#2712)
  40235c365  Thu Oct 29 14:46:17 2026 +0100  Jonas Finnemann Jensen  [markdown] Make HtmlBlockSyntax.parseChildLines iterative (dart-lang/tools#2749)
  0ff03f643  Thu Oct 29 14:41:33 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid re-escaping nested image alt text in ImageSyntax.createNode (dart-lang/tools#2752)
  784654d01  Thu Oct 29 14:37:11 2026 +0100  Jonas Finnemann Jensen  [markdown] Add test cases covering parser and renderer edge cases (dart-lang/tools#2753)
  4f2718ad8  Thu Oct 29 14:36:52 2026 +0100  Jonas Finnemann Jensen  [markdown] Make Document._parseInlineContent iterative (dart-lang/tools#2750)
  cac1421d5  Thu Oct 29 14:17:06 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid quadratic duplicate heading ID probing in HtmlRenderer (dart-lang/tools#2748)
  66880bcd3  Thu Oct 29 14:11:27 2026 +0100  Jonas Finnemann Jensen  [markdown] Check footnote label and empty map before scanning footnote keys (dart-lang/tools#2751)
  7f8696523  Thu Oct 29 14:07:06 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid quadratic blank-line lookahead in CodeBlockSyntax (dart-lang/tools#2747)
  9da6bb291  Thu Oct 29 14:03:04 2026 +0100  Jonas Finnemann Jensen  [markdown] Avoid exponential and quadratic backtracking in tablePattern (dart-lang/tools#2746)

dartdoc is held back at db94437, before dart-lang/dartdoc#4312, because its
mock SDK version changes conflict in the google3 3-way merge with the hotfix
in cl/1005963642.

native is held back at e0ca7c6, before dart-lang/native#3742, because
deprecating `MacOSCodeConfig(targetVersion: ...)` causes a fatal
`deprecated_member_use` diagnostic in `pkg/dartdev/lib/src/native_assets.dart`.

web is held back at 4e60e61, before dart-lang/web#596, because it changes
`package:web` signatures (e.g. `innerHTML=`, `parseFromString`, `Worker()`)
from `JSAny` to `String` in ways that break callers in google3.

Change-Id: I53d2cbde741c45b8fd33bca42742549819dd332b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/572780
Reviewed-by: Brian Quinlan <[email protected]>
Commit-Queue: Nate Bosch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidmorgan davidmorgan davidmorgan approved these changes
@lrhn lrhn Awaiting requested review from lrhn lrhn is a code owner
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

package:mime

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kevmoo @davidmorgan @lrhn