E530 multipart: allow MIME head to span read boundary by alpaca-tc · Pull Request #2392 · rack/rack · GitHub
[go: up one dir, main page]

Skip to content

multipart: allow MIME head to span read boundary#2392

Merged
ioquatix merged 2 commits intorack:mainfrom
alpaca-tc:fix-multipart-header-parser
Nov 2, 2025
Merged

multipart: allow MIME head to span read boundary#2392
ioquatix merged 2 commits intorack:mainfrom
alpaca-tc:fix-multipart-header-parser

Conversation

@alpaca-tc
Copy link
Contributor
@alpaca-tc alpaca-tc commented Oct 14, 2025

Previously, the header size check compared MIME_HEADER_BYTESIZE_LIMIT against @sbuf.string.bytesize, which
included previously read data. When the scan buffer held several megabytes of processed data plus a unprocessed
header fragment, this could incorrectly raise “multipart mime part header too large.”

Fix: compare the limit against @sbuf.rest.bytesize instead. This measures only the unread portion of the
buffer—the region that may still contain the MIME header—ensuring the check applies to header data alone
without being affected by prior reads.

Copy link
Contributor
@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

Thank you for the patch, I agree we should merge this and backport it.

@alpaca-tc alpaca-tc force-pushed the fix-multipart-header-parser branch from 5cbdfc5 to e72bebb Compare October 15, 2025 04:11
@alpaca-tc
Copy link
Contributor Author
alpaca-tc commented Oct 15, 2025

I’m fixing code — I missed a few things.

@alpaca-tc alpaca-tc force-pushed the fix-multipart-header-parser branch 2 times, most recently from da2963b to 1e30d0f Compare October 15, 2025 08:46
@alpaca-tc
Copy link
Contributor Author
alpaca-tc commented Oct 15, 2025

The body is truncated only when the body cannot be fully read:

@sbuf.pos += delta
@sbuf.string = @sbuf.rest

When multiple multipart parts are attached, the buffer may be refilled several times to read each part’s header.
I hadn’t accounted for the fact that @sbuf.string could keep growing with each additional read.
Therefore, the comparison now uses @sbuf.rest.bytesize instead of @sbuf.string.bytesize, so that only the unread portion of the buffer is measured.

Please see the linked diff for details.
https://github.com/rack/rack/compare/5cbdfc5..1e30d0ff88f583b0c7226b1a762ffd46976b008c

@alpaca-tc
Copy link
Contributor Author

@jeremyevans I’ve fixed the issue I noticed and force-pushed the changes 🙏

@jeremyevans jeremyevans requested a review from ioquatix October 15, 2025 15:31
@naitoh
Copy link
naitoh commented Oct 29, 2025

@ioquatix
What do you think of this PR?
If possible, I'd appreciate it if you could review it and merge it.

@alpaca-tc
Copy link
Contributor Author

I saw that changelog were being pointed out in other PR reviews, so I added one as well.

@ioquatix ioquatix self-assigned this Oct 31, 2025
@ioquatix
Copy link
Member

I will try to get this backported and released this weekend.

alpaca-tc and others added 2 commits November 3, 2025 00:46
Previously, the header size check compared `MIME_HEADER_BYTESIZE_LIMIT` against `@sbuf.string.bytesize`, which
included previously read data. When the scan buffer held several megabytes of processed data plus a unprocessed
header fragment, this could incorrectly raise “multipart mime part header too large.”

Fix: compare the limit against `@sbuf.rest.bytesize` instead. This measures only the unread portion of the
buffer—the region that may still contain the MIME header—ensuring the check applies to header data alone
without being affected by prior reads.

Co-authored-by: Shinichi Maeshima <netwillnet@gmail.com>
Co-authored-by: krororo <krororo.07@gmail.com>
@ioquatix ioquatix force-pushed the fix-multipart-header-parser branch from e2b471f to 89fe65f Compare November 2, 2025 11:46
@ioquatix ioquatix merged commit 36877ed into rack:main Nov 2, 2025
17 checks passed
ioquatix pushed a commit that referenced this pull request Nov 2, 2025
Previously, the header size check compared `MIME_HEADER_BYTESIZE_LIMIT` against `@sbuf.string.bytesize`, which
included previously read data. When the scan buffer held several megabytes of processed data plus a unprocessed
header fragment, this could incorrectly raise “multipart mime part header too large.”

Fix: compare the limit against `@sbuf.rest.bytesize` instead. This measures only the unread portion of the
buffer—the region that may still contain the MIME header—ensuring the check applies to header data alone
without being affected by prior reads.

Co-authored-by: Shinichi Maeshima <netwillnet@gmail.com>
Co-authored-by: krororo <krororo.07@gmail.com>
ioquatix pushed a commit that referenced this pull request Nov 2, 2025
Previously, the header size check compared `MIME_HEADER_BYTESIZE_LIMIT` against `@sbuf.string.bytesize`, which
included previously read data. When the scan buffer held several megabytes of processed data plus a unprocessed
header fragment, this could incorrectly raise “multipart mime part header too large.”

Fix: compare the limit against `@sbuf.rest.bytesize` instead. This measures only the unread portion of the
buffer—the region that may still contain the MIME header—ensuring the check applies to header data alone
without being affected by prior reads.

Co-authored-by: Shinichi Maeshima <netwillnet@gmail.com>
Co-authored-by: krororo <krororo.07@gmail.com>
@ioquatix ioquatix added this to the v2.2.21 milestone Nov 2, 2025
ioquatix pushed a commit that referenced this pull request Nov 2, 2025
Previously, the header size check compared `MIME_HEADER_BYTESIZE_LIMIT` against `@sbuf.string.bytesize`, which
included previously read data. When the scan buffer held several megabytes of processed data plus a unprocessed
header fragment, this could incorrectly raise “multipart mime part header too large.”

Fix: compare the limit against `@sbuf.rest.bytesize` instead. This measures only the unread portion of the
buffer—the region that may still contain the MIME header—ensuring the check applies to header data alone
without being affected by prior reads.

Co-authored-by: Shinichi Maeshima <netwillnet@gmail.com>
Co-authored-by: krororo <krororo.07@gmail.com>
@ioquatix ioquatix removed this from the v2.2.21 milestone Nov 2, 2025
@ioquatix ioquatix added this to the v3.1.19 milestone Nov 2, 2025
ioquatix pushed a commit that referenced this pull request Nov 2, 2025
Previously, the header size check compared `MIME_HEADER_BYTESIZE_LIMIT` against `@sbuf.string.bytesize`, which
included previously read data. When the scan buffer held several megabytes of processed data plus a unprocessed
header fragment, this could incorrectly raise “multipart mime part header too large.”

Fix: compare the limit against `@sbuf.rest.bytesize` instead. This measures only the unread portion of the
buffer—the region that may still contain the MIME header—ensuring the check applies to header data alone
without being affected by prior reads.

Co-authored-by: Shinichi Maeshima <netwillnet@gmail.com>
Co-authored-by: krororo <krororo.07@gmail.com>
@ioquatix ioquatix modified the milestones: v3.1.19, 3.2.4 Nov 2, 2025
@ioquatix
Copy link
Member
ioquatix commented Nov 2, 2025

Okay, all 3 releases are done.

@alpaca-tc alpaca-tc deleted the fix-multipart-header-parser branch November 3, 2025 11:42
@zzak
Copy link
Contributor
zzak commented Nov 6, 2025

Does handle_fast_forward have the same issue because it's using @sbuf.string.bytesize?
https://github.com/rack/rack/blob/main/lib/rack/multipart/parser.rb#L328

@jeremyevans
Copy link
Contributor

Does handle_fast_forward have the same issue because it's using @sbuf.string.bytesize? https://github.com/rack/rack/blob/main/lib/rack/multipart/parser.rb#L328

No, because that is only done in the initial read, there shouldn't be any data already in the buffer when we start looking for the boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

0