refs #53 handle invalid content-type headers #55
Conversation
* added a test to simulate the issue * unfortunately, not entirely sure how to catch this exception and handle it yet ...
|
You could probably sanitize the begin
request.content_type
rescue Mime::Type::InvalidMimeType
request.env["MALFORMED_HTTP_CONTENT_TYPE"], request.env["HTTP_CONTENT_TYPET"] = request.env["HTTP_CONTENT_TYPE"], "*/*"
endAnd you can probably add it to either before or after where the rambulance/lib/rambulance/exceptions_app.rb Lines 70 to 74 in 75e4d4a |
|
Thank @yuki24 ! I tried something similar. The problem I see is that Update: see 53ea75e although I'm a bit confused by the travis results... tests are passing for me locally Update2: Ah, I think I see how tests are run now, so hopefully can fix the response codes for each rails version |
Co-authored-by: Yuki Nishijima <yk.nishijima@gmail.com>
lib/rambulance/exceptions_app.rb
Outdated
| @@ -59,12 +59,16 @@ def process_action(*) | |||
| request.GET | |||
| rescue ActionController::BadRequest | |||
| request.env["MALFORMED_QUERY_STRING"], request.env["QUERY_STRING"] = request.env["QUERY_STRING"], "" | |||
| rescue Mime::Type::InvalidMimeType | |||
| request.env["MALFORMED_CONTENT_TYPE"], request.env["CONTENT_TYPE"] = request.env["CONTENT_TYPE"], "text/plain" | |||
There was a problem hiding this comment.
Each one of the elements should be sanitized independently to cover a case where both the query string and the content type header are malformed. You could probably do this at the beginning of this method:
def process_action(*)
begin
request.content_mime_type
rescue Mime::Type::InvalidMimeType
request.env["MALFORMED_CONTENT_TYPE"], request.env["CONTENT_TYPE"] = request.env["CONTENT_TYPE"], "*/*"
end
...I picked up HTTP_CONTENT_TYPET in my previous comment, which was wrong, and you seem to have figured it out correctly, so the code above should work now.
There was a problem hiding this comment.
cool. this seems to work! I'll update it. Thanks @yuki24
lib/rambulance/exceptions_app.rb
Outdated
| end | ||
|
|
||
| begin | ||
| request.POST | ||
| rescue ActionController::BadRequest | ||
| request.env["MALFORMED_BODY"], request.env["rack.input"] = request.env["rack.input"], StringIO.new | ||
| rescue Mime::Type::InvalidMimeType | ||
| request.env["MALFORMED_CONTENT_TYPE"], request.env["CONTENT_TYPE"] = request.env["CONTENT_TYPE"], "text/plain" |
There was a problem hiding this comment.
Let's remove this in favor of https://github.com/yuki24/rambulance/pull/55/files#r431494203.
|
|
||
| assert_equal 406, response.status | ||
| assert_equal "The requested content type is not acceptable.\n", response.body | ||
| end |
There was a problem hiding this comment.
test/requests/error_json_test.rb
Outdated
| @@ -67,12 +93,12 @@ def without_layouts | |||
| `mv error.html.erb test/fake_app/app/views/layouts/` | |||
| end | |||
|
|
|||
| def get(path) | |||
| def get(path, headers: {}) | |||
There was a problem hiding this comment.
I think this is a good change, but it would be great if you could send a separate pull request for that.
def get(path, params: {}, headers: {})
...
endThere was a problem hiding this comment.
That's a good idea. Although I do need it in order to have more flexibility testing with GET since you overwrote it. I'll create a separate PR, and then see if I can rebase this PR off the other one.
There was a problem hiding this comment.
see #56 (merged onto this PR)
gingerlime
changed the title
|
The test failure seems to be caused by the extra test which I think we can remove. I'll just go ahead nad merge since this pull request looks good as it is. |
|
That's great. Thanks so much @yuki24 and for your patience helping me work through this. |
handle it yet ... so this test currently fails