Sidekiq: Truncated response body using Sidekiq dashboard w/New Relic

[pelargir]

Ruby version: 2.6.5p114
Sidekiq / Pro / Enterprise version(s): 6.0.4

We're using newrelic_rpm 6.8.0.360 to add JavaScript instrumentation to our Rails 5.2.4.1 app. After upgrading to Rack 2.1.2 the dashboard went blank. Viewing the page source, the HTML stops in the middle of the New Relic JS code near the top of the page (see screenshot).

Apparently, Rack::Response no longer overwrites the Content-Length header, which means an incorrectly configured Content-Length header is now exposed. See https://github.com/rack/rack/issues/1520#issuecomment-579838704

Is it possible Sidekiq is setting Content-Length prior to New Relic adding its instrumentation, resulting in an incomplete page load since the New Relic code takes up the entire Content-Length before the remaining body can be sent?

As soon as I remove New Relic from our app the dashboard starts working again.

[mperham]

Should NewRelic be instrumenting Sidekiq's pages at all? Maybe it should exclude /sidekiq/*.

See https://github.com/mperham/sidekiq/blob/3510de7df3c2eaacec9061184666aa853a0abb13/lib/sidekiq/web/application.rb#L317

Added here with no explanation:

https://github.com/mperham/sidekiq/commit/9ae228c521699eee43d7b2c371ecb86ca28468f9

I just tested removing those two lines (315 and 317) and the Sidekiq UI appears to work without them in both Rack 2.0.8 and 2.1.2. Unfortunately I have no idea whether this is a good idea or not.

[pelargir]

Yes, New Relic Browser instrumentation should probably be excluded from Sidekiq::Web by default. They give examples of how to do this in a Rails controller and a Sinatra app, but not a pure Rack app. I'm thinking a middleware might work.

[pelargir]

Still trying to turn off New Relic for Sidekiq::Web using this:
https://docs.newrelic.com/docs/agents/ruby-agent/api-guides/ignoring-specific-transactions#page-load-timing-rum

In the meantime, to get running I've created a monkey patch that deletes the Content-Type header from responses coming from Sidekiq::Web. Hope this helps someone:

require 'sidekiq/web'

module RemoveContentLengthHeader
  def call(env)
    resp = super
    resp[1].delete('Content-Length')
    resp
  end
end

Sidekiq::WebApplication.prepend(RemoveContentLengthHeader)

[mperham]

If it works without NewRelic, please open an issue with them too. Maybe Sidekiq::Web shouldn't be calculating Content-Length but I'd like someone to explain why I should change code that has worked fine for years now.

[pelargir]

I'll do that, however I suspect the only reason it's worked fine until now was due to older versions of Rack overwriting the Content-Length. See https://github.com/rack/rack/issues/1520#issuecomment-579830631

This did not happen with Rack versions prior to 2.1.2 (even with New Relic instrumentation installed).

[mperham]

You've convinced me. No sense in calculating something unnecessarily when we can let Rack do it correctly for us.

[ArturT]

@mperham When do you plan to release sidekiq 6.0.5?

[mperham]

I believe the workaround today is gem 'rack', '~> 2.0.8'. Because there is a workaround, I don't feel the need to ship a quick fix. Please correct me if there is a more urgent need for some reason.

[ArturT]

@mperham I assumed you will be releasing a new sidekiq soon because this PR was merged. That's fine if you have a bit longer release timeline. gem 'rack', '~> 2.0.8' works fine. Thanks.

[flanger001]

This helped us a lot too, thank you. @mperham any chance you can backport this into a 5.x version? I can't upgrade to 6 yet. Locking rack is fine, but I thought I'd ask.

[mperham]

I've committed a gemspec update to the 5-x branch which you can run. It has two commits in the last year so it should be very stable.

gem 'sidekiq', branch: '5-x'

[flanger001]

Thanks @mperham!

[ritaly]

@mperham, @pelargir looks like newrelic_rpm with version 6.9.0 is fixed and ready https://github.com/newrelic/rpm/issues/313
Sidekiq 5.2.8 locked rack to 2.0.8, could be now unlocked?

[mperham]

@ritaly Sidekiq 5.2.8 is locked to Rack 2.0.x which I think is reasonable. I don't want to support future Rack versions for a Sidekiq version which is in maintenance mode, given these issues with backwards compatibility. Sidekiq 6 will support Rack 2.1 and future versions.

[kulas115]

@mperham Given that a new security vulnerability was found in rack 2.0.9 the only way to fix is to update to rack 2.1.3 which lock in sidekiq 5.2.8 is currently preventing. I am aware that's already maintenance mode but I guess there will be a lot of people forced to use sidekiq 5.2.x (for example due to Redis 4 requirement) and without newer versions or rack a lot of builds will be blocked. Could you consider relaxing this constrain to include newer versions of rack as well?

https://groups.google.com/forum/#!topic/ruby-security-ann/T4ZIsfRf2eA

[mperham]

It only affects Rack::Directory, which Sidekiq does not use.

[drcapulet]

@mperham Although Sidekiq doesn't use it, other pieces in an application could. Even if they don't, we don't want to operate with an open vulnerability - our vulnerability management system doesn't let us do that so our current recommendation is downgrading Sidekiq to 5.2.7 so they can update Rack, but if possible we'd appreciate a 5.2.9 release that relaxed the Rack version constraint.

[mperham]

I'm not interested in rushing a release in order to appease a brainless tool. I will happily turn a quick release if there's a bug in something in wide use but this is a CVE for code no one uses.

Using Advanced Search I found a grand total of 2 ruby repos that actually use Rack::Directory, out of 10,000s on github.

[AlexDWu]

We solved this problem by using New Relic's manual instrumentation https://docs.newrelic.com/docs/agents/ruby-agent/features/new-relic-browser-ruby-agent#manual_instrumentation. We put that in a layout, put that layout where we needed it, and set this in newrelic.yml:

browser_monitoring:
    auto_instrument: false

We didn't want instrumentation for our Sidekiq pages anyways. Should still update our gems... one day.

NewRelic::Agent#browser_timing_header just returns a string that is empty or a script tag, you can probably use it in ways other than a Rails view layout https://rubydoc.info/github/newrelic/newrelic-ruby-agent/NewRelic%2FAgent:browser_timing_header

[morriswchris]

FWIW we recently saw this problem as well. Trying @pelargir's patch worked initially but rather than deploying code we used NEW_RELIC_RULES_IGNORE_URL_REGEXES to block our sidekiq route from the agent, which immediately brought the dashboards back.

[mperham]

The solution as I know it is bundle up newrelic_rpm. It has been fixed in a later version of the gem.