Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix truncated body in exception view #255

Merged
merged 3 commits into from
Apr 29, 2018
Merged

Fix truncated body in exception view #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
470eb24
Fix truncated body in exception view
timomeh Apr 26, 2018
cde6f27
Fix typo
timomeh Apr 26, 2018
bfa2a9c
Add test if Content-Length is correct
timomeh Apr 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/web_console/middleware.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ def call(env)
headers["X-Web-Console-Session-Id"] = session.id
headers["X-Web-Console-Mount-Point"] = mount_point

# Remove any previously set Content-Length header because we modify
# the body. Otherwise the response will be truncated.
# Someone will calculate it again, at least the application server.
headers.delete("Content-Length")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test for this, so we don't regress again?

Copy link
Author

Choose a reason for hiding this comment

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

I would be glad to do that, but unfortunately I don't have any clue how to test it. 😕

I tried adding a wrong Content-Length to the headers defined in helper_test.rb or middleware_test.rb to reproduce the bug in the first place, but even without this fix, the response in the test always contains the correct Content-Length. Maybe somewhere in the test-stack the Content-Length gets re-set to the correct value, where other application servers don't override the Content-Length if it's already set (e.g. puma)? My rails-knowledge isn't really that good...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can explicitly set the Content-Length after the injection on line 42 ourselves? Then we can test that the content length is extended after the middleware run? That way, we won't depend on someone else to set the proper content length and therefore test our own behavior.

Copy link
Author

Choose a reason for hiding this comment

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

The problem is that I cannot reproduce the bug (without my fix) in a test...

If I set headers["Content-Length"] = 7 (just an arbitrary obviously wrong value) in the middleware instead of deleting it, the response in the test will always have the correct Content-Length, not the previously defined value of 7. It seems like something in the test library is setting Content-Length to the correct value all the time, which is not the same behavior as puma.

So if I add a test to check if Content-Length is set to the correct value, it will be green even without my fix... which kinda defeats the purpose of writing a test.

Personally, I don't think a library or application has to care about setting Content-Length, since this is usually the task of a server. That's why I would prefer deleting the header. I would go so far to say that the behavior of Action Pack is wrong, it shouldn't set the Content-Length in the first place.

So to summarize: If we delete Content-Length or re-calculate it in the middleware, I have no idea how to write a reasonable middleware test. 😅

Copy link
Author

Choose a reason for hiding this comment

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

That being said, I only tried to test it using an Integration Test. I don't know how to properly test the middleware with another kind of test.

Copy link
Collaborator

@gsamokovarov gsamokovarov Apr 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe you can try to write a unit test (ActiveSupport::TestCase) and execute the middleware manually with rack-test. You can also move this behavior to the WebConsole::Injector and add another unit test to it, instead to the middleware. I'd be okay with the injector, caring about acting on the content length, even if we have to give it more input/output responsibilities.

Copy link
Author

Choose a reason for hiding this comment

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

I just added a test without seeing your comment. 😅 But I added the middleware_test which always will be green because of the previously mentioned IntegrationTest.

Executing it manually sounds like a good plan. I'll add that (later this day, hopefully)


template = Template.new(env, session)
body = Injector.new(body).inject(template.render("index"))
end
Expand Down
16 changes: 12 additions & 4 deletions test/web_console/middleware_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class MiddlewareTest < ActionDispatch::IntegrationTest
class Application
def initialize(options = {})
@response_content_type = options.fetch(:response_content_type, Mime[:html])
@response_content_length = options.fetch(:response_content_length, nil)
end

def call(env)
Expand All @@ -33,10 +34,9 @@ def status
end

def headers
if @response_content_type
{ "Content-Type" => "#{@response_content_type}; charset=utf-8" }
else
{}
Hash.new.tap do |header_hash|
header_hash['Content-Type'] = "#{@response_content_type}; charset=utf-8" unless @response_content_type.nil?
header_hash['Content-Length'] = @response_content_length unless @response_content_length.nil?
end
end
end
Expand Down Expand Up @@ -84,6 +84,14 @@ def headers
assert_select "#console"
end

test "sets correct Content-Length header" do
Thread.current[:__web_console_binding] = binding
@app = Middleware.new(Application.new(response_content_length: 7))

get "/", params: nil
assert_equal(response.body.size, response.headers['Content-Length'].to_i)
end

test "it closes original body if rendering console" do
Thread.current[:__web_console_binding] = binding
inner_app = Application.new(response_content_type: Mime[:html])
Expand Down