search

Mange / roadie-rails

Making HTML emails comfortable for the Rails rockstars
MIT License
367 stars 66 forks source link

Don't directly modify Sprockets::BundleAsset#source #32

Closed bmulholland closed 9 years ago

bmulholland commented 9 years ago

Passing asset.to_s into Stylesheet causes Sprockets to serve that asset with a Content-Length greater than the size of the response, which hangs connections until they timeout with bytes remaining. This happened in our tests, and caused Chrome/Cucumber to hang for 30s on many steps, which increased our test run time by 10 minutes.

In Sprocket asset.to_s directly returns a reference to the string of the asset's source (https://github.com/sstephenson/sprockets/blob/2.x/lib/sprockets/asset.rb#L112). That string is then modified inside of Stylesheet when it's parsed (https://github.com/Mange/roadie/blob/master/lib/roadie/stylesheet.rb#L67), removing whitespace, comments, etc. That directly modifies Sprockets' asset in memory, which is then cached for future requests.

Sprockets 2.x (and likely in 3.x) stores the length of the asset on asset init (https://github.com/sstephenson/sprockets/blob/2.x/lib/sprockets/bundled_asset.rb#L29), and that length is cached while the server is running.

Combining those two together, we get a situation where Sprockets has a cached BundledAsset that has a length that includes whitespace/comments/etc, but a source that does not. The stored length is what's used to set the Content-Length header when serving the asset (https://github.com/sstephenson/sprockets/blob/2.x/lib/sprockets/server.rb#L193). At the same time, the response to the client is set as the source string -- which has been modified in-memory by roadie. This causes the mismatch between Content-Length and the response.

Is this the sort of thing you'd like to have a test case for? If so, any guidance on a good way to go about it? Reproducing it exercises rather a lot of the stack.

Mange commented 9 years ago

Wow! Thank you for bringing this to my attention.

I want to fix this inside roadie's Stylesheet to keep it encapsulated. It was never my intention to mutate any parameter sent to Roadie (I avoid it like the plague). I did not realize that CSS Parser does this, so I choose to see this as a "I'm using a third-party API wrong".

I'll fix this myself on master and reference this PR and you. All credit will go to you, but it's easier for me to fix this myself rather than to tell you exactly how I want it done. Is that reasonable?

Is this the sort of thing you'd like to have a test case for? If so, any guidance on a good way to go about it? Reproducing it exercises rather a lot of the stack.

It's pretty unfeasable at this level, but I think I can narrow it down to another level. I'll get back to you.

As a general question, however: Yeah, no tests for this amount of stack exercise. I'll be complicated and slow the tests down a lot.

Do you have some way to exercise your stack so I can test this bug fix?

Mange commented 9 years ago

If it's possible for you to do so, please try master roadie in your app and see if this still works like it should. (It ought to; the change is trivial)

I'll release this after hearing back from you, or in a few hours if not.

Again, thank you for your contribution!

bmulholland commented 9 years ago

I can confirm that master roadie fixes the issue. Thanks for your response, the fix, and the credit!

Mange commented 9 years ago

Version 3.0.3 of roadie was just released. bundle update roadie to get this bug fix.

Thank you for your excellent report, research, and patience @bmulholland! :heart: