Fix for form reading bug. #60
Conversation
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 12 12
Lines 628 633 +5
=========================================
+ Hits 628 633 +5
Continue to review full report at Codecov.
|
|
@PreferLinux, care to take a look? |
There was a problem hiding this comment.
That's an interesting way of getting around the problem. Not without issues, but once it works it should keep working without problems.
Ideally there would be a way to stop the form being processed in the first place, but I don't think there is other than the relatively fragile workaround mentioned in the issue. Maybe it would be a good idea to ask the Asp.Net Core team about that (or even open an issue, because model binding should be checking route values before touching the form)?
|
|
||
| internal static StreamContent ToStreamContent(this IFormCollection collection) | ||
| { | ||
| return new StreamContent(new MemoryStream(HttpUtility.UrlDecodeToBytes(string.Join("&", collection.Select(f => $"{f.Key}={f.Value}"))))); |
There was a problem hiding this comment.
Is there a specific reason you haven't used a FormUrlEncodedContent or something? The form data is already in memory. I guess they might try interacting with the headers?
I don't think this handles multiple values per key correctly (e.g. HTML forms with <select multiple></select>). It'll encode them as name=item1,item2, which I don't think is valid (how does the recipient know it is two values instead of one containing a comma?). Browsers seem to do name=item1&name=item2 but I think there are other possible ways as well.
Are the names and values encoded? I'm guessing not, so that'd cause problems too (especially an "&").
This won't work correctly for Content-Type: multipart/form-data. Therefore it won't handle file uploads. Obviously using a FormUrlEncodedContent won't help here, because it is still the wrong format. From my research it'll require either manually creating a multipart message body which would require loading any files, or using a MultipartFormDataContent and adding everything to it...
There was a problem hiding this comment.
I didn't use FormUrlEncodedContent because the API felt cumbersome. I can switch back to it, though.
There was a problem hiding this comment.
Yeah, it isn't quite as good as I'd like now I look at it a bit closer...
I've had a go at fixing these issues here: PreferLinux@45da1f9
I used FormUrlEncodedContent and MultipartFormDataContent, and they do add the Content-Type headers. Adding that a second time (copied from the initial request) causes problems, so I made sure it didn't do that. The other thing is that both of them can change the content length slightly (the former because spaces can be encoded as either + (which it uses) or %20 (which the client I was testing with uses), the latter by giving a Content-Type for each part rather than only when strictly necessary), so I also don't copy across the original Content-Length header.
I haven't put in any tests, partly because I'm not sure how to test file uploads, and partly because JSONPlaceholder doesn't seem to parse Content-Type: multipart/form-data.
There was a problem hiding this comment.
Cool: very nice. Want me to cherry pick it into here, or do you want to do a PR, and I'll close this?
There was a problem hiding this comment.
Maybe we can just write a unit test for the multipart code?
There was a problem hiding this comment.
I did get a test largely written, but ran into the issue with JSONPlaceholder. Since then I've discovered https://httpbin.org which does support Content-Type: multipart/form-data and file upload, so I've finished off the test (including file upload).
Cool: very nice. Want me to cherry pick it into here, or do you want to do a PR, and I'll close this?
Either way is fine by me. Probably easiest for me to do a pull request though, especially as I've added a test, and also a very small fix that the test uncovered (the boundary can be quoted, and .Net quotes it, but the MultipartFormDataContent constructor doesn't accept a quoted boundary – even though it later checks if it is already quoted before adding the quotes).
There was a problem hiding this comment.
Nice: please do. Closing this one.
|
Thanks. The only problem with the other solution is that there is no way to know, in the proxy logic, whether or not a controller "accidentally" consumes the form body by taking method parameters. |
No description provided.