Skip to content

Raise InvalidPartEncodingError on invalid multipart#2416

Merged
ioquatix merged 2 commits into
rack:mainfrom
bquorning:detect-invalid-encoding-in-multipart
Jan 10, 2026
Merged

Raise InvalidPartEncodingError on invalid multipart#2416
ioquatix merged 2 commits into
rack:mainfrom
bquorning:detect-invalid-encoding-in-multipart

Conversation

@bquorning

@bquorning bquorning commented Dec 13, 2025

Copy link
Copy Markdown
Contributor

Multipart parser now raises Rack::Multipart::InvalidPartEncodingError if one part uses a non-ASCII compatible, non-dummy encoding.

The added test case is very similar to one specifying how ISO-2022-JP encoding is handled, so I took the liberty of rewording that a bit.

Fixes #2414.

@bquorning bquorning force-pushed the detect-invalid-encoding-in-multipart branch from eff3a87 to 5290997 Compare December 13, 2025 13:47
@bquorning bquorning marked this pull request as ready for review December 13, 2025 13:47
@bquorning bquorning force-pushed the detect-invalid-encoding-in-multipart branch from 5290997 to e06b2f7 Compare December 13, 2025 13:50

@jeremyevans jeremyevans 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.

Thinking about this more, I think the correct place to raise this exception is not in the multipart parser, but in the query parser's _normalize_params. If you are using the multipart parser without the query parser, and this doesn't currently raise an exception, I don't think we should start raising an exception. The issue here is the query parser raises a generic exception and not a specific exception, so it would be best for the query parser to detect the incompatible encoding and raise the specific exception.

content-disposition: form-data; name="utf-16le"
content-type: text/plain; charset=utf-16le

Alice

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.

You should make this valid UTF-16LE encoded data. Same for other cases in this PR where you are using UTF-16LE. Currently, with this being invalid UTF-16LE data, it's ambiguous whether the related specs are for ascii-incompatible encodings in general, or invalid data with the encoding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point.

I’ve removed this file again, and moved the unit test to spec_query_parser.rb where the data is encoded using force_encoding.

Comment thread test/spec_multipart.rb Outdated
@bquorning

bquorning commented Dec 14, 2025

Copy link
Copy Markdown
Contributor Author

Thinking about this more, I think the correct place to raise this exception is not in the multipart parser, but in the query parser's _normalize_params. If you are using the multipart parser without the query parser, and this doesn't currently raise an exception, I don't think we should start raising an exception. The issue here is the query parser raises a generic exception and not a specific exception, so it would be best for the query parser to detect the incompatible encoding and raise the specific exception.

You make a good point. I have moved the implementation into Rack::QueryParser#_normalize_params and changed the tests accordingly. I’ve pushed the change as a separate commit so the two options are easier to compare, but intent to squash them into one before it’s ready to merge. The commits are squashed now.

Would you prefer test/spec_query_parser.rb to test all ascii incompatible encodings (e.g. Encoding.list.reject(&:ascii_compatible?).each), or is it fine just testing one of them?

Comment thread test/spec_method_override.rb Outdated

@jeremyevans jeremyevans 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.

This looks good to merge, other than the encoding issue you identified and one other issue.

Comment thread lib/rack/query_parser.rb Outdated
@bquorning bquorning force-pushed the detect-invalid-encoding-in-multipart branch from b710de7 to 0fd5e53 Compare December 14, 2025 22:40
@bquorning bquorning force-pushed the detect-invalid-encoding-in-multipart branch from 0fd5e53 to b2c1bd1 Compare December 23, 2025 11:40
@bquorning

Copy link
Copy Markdown
Contributor Author

@jeremyevans Just a reminder that I consider this PR ready for review now.

@jeremyevans jeremyevans requested a review from ioquatix January 9, 2026 15:15
@bquorning

bquorning commented Jan 9, 2026

Copy link
Copy Markdown
Contributor Author

I pushed a fix for the TruffleRuby exception

ArgumentError: UTF-16 string byte length (5) is not a multiple of 2
    test/spec_query_parser.rb:48:in 'String#force_encoding'

by changing "Alice" to "Alice?"

The error from https://github.com/rack/rack/actions/runs/20459763308/job/59924014476 seems not related to my changes:

Because every version of releaser depends on Ruby >= 3.3.0
  and Gemfile depends on releaser >= 0,
  Ruby >= 3.3.0 is required.
So, because current Ruby version is = 3.2.9,
  version solving has failed.

Update: A fix for the last problem is proposed in #2417.

@bquorning bquorning force-pushed the detect-invalid-encoding-in-multipart branch 2 times, most recently from 274c09c to 6cc4984 Compare January 9, 2026 19:02
@bquorning

Copy link
Copy Markdown
Contributor Author

Rebased and force-pushed, and CI is green.

FYI bundle exec rake is failing for Ruby 2.5 on CI but the job is still reported as passing – both on this branch, and on main. Here’s a workflow run from main from 2 months ago: https://github.com/rack/rack/actions/runs/19048129429/job/54401393373#step:4:33

@jeremyevans

Copy link
Copy Markdown
Contributor

Ruby 2.4/2.5 failures being ignored is expected. That support is considered best-effort. I generally check once a month and fix issues. Looks like I need to check both Ruby 2.4/2.5. I've been checking only 2.4, assuming if that 2.5 was broken, 2.4 would be as well, but that appears to be an invalid assumption.

Comment thread lib/rack/query_parser.rb Outdated
@ioquatix ioquatix force-pushed the detect-invalid-encoding-in-multipart branch from 6cc4984 to dd2e781 Compare January 10, 2026 04:12
The query parser now raises Rack::QueryParser::IncompatibleEncodingError
if we try to parse params that are not ASCII compatible.
@bquorning bquorning force-pushed the detect-invalid-encoding-in-multipart branch from dd2e781 to 228b867 Compare January 10, 2026 07:51
@bquorning bquorning changed the title Raise InvalidPartEncoding on invalid multipart Raise InvalidPartEncodingError on invalid multipart Jan 10, 2026
Comment thread CHANGELOG.md Outdated
@ioquatix ioquatix enabled auto-merge (squash) January 10, 2026 23:44
@ioquatix ioquatix disabled auto-merge January 10, 2026 23:44
@ioquatix ioquatix merged commit 603b799 into rack:main Jan 10, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Encoding::CompatibilityError in Rack::MethodOverride

4 participants