Fix truncated body in exception view

[timomeh]

ActionPack's debug_exceptions will calculate the Content-Length of the rendered body. See implementation here.

After injecting the console into the body, there will be a mismatch between the Content-Length Header and the actual response body. So the browser will truncate the output.

As a solution the Content-Length Header will be deleted, as the application server will calculate the Content-Length again, if it isn't present already.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @gsamokovarov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[gsamokovarov]

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

[timomeh]

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

[gsamokovarov]

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.

[timomeh]

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. 😅

[timomeh]

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.

[gsamokovarov]

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.

[timomeh]

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)

[gsamokovarov]

@timomeh, thanks for the change! I'll merge it as is and write a test in an upcoming commit. I have some time today and will like to release 3.6.2 with this fix soonish. ✌️

[timomeh]

@gsamokovarov Cool! I wrote a test yesterday using rack-test, but (who would have thought) it overrides the Content-Length Header, too. I'm looking forward to see how you tested it :)