Raise InvalidPartEncodingError on invalid multipart#2416
Conversation
eff3a87 to
5290997
Compare
5290997 to
e06b2f7
Compare
jeremyevans
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
You make a good point. I have moved the implementation into Would you prefer test/spec_query_parser.rb to test all ascii incompatible encodings (e.g. |
jeremyevans
left a comment
There was a problem hiding this comment.
This looks good to merge, other than the encoding issue you identified and one other issue.
b710de7 to
0fd5e53
Compare
0fd5e53 to
b2c1bd1
Compare
|
@jeremyevans Just a reminder that I consider this PR ready for review now. |
|
I pushed a fix for the TruffleRuby exception by changing The error from https://github.com/rack/rack/actions/runs/20459763308/job/59924014476 seems not related to my changes: Update: A fix for the last problem is proposed in #2417. |
274c09c to
6cc4984
Compare
|
Rebased and force-pushed, and CI is green. FYI |
|
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. |
6cc4984 to
dd2e781
Compare
The query parser now raises Rack::QueryParser::IncompatibleEncodingError if we try to parse params that are not ASCII compatible.
dd2e781 to
228b867
Compare
Multipart parser now raises
Rack::Multipart::InvalidPartEncodingErrorif 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.