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

Jump to bottom

browser_monitoring throws exception on non string HTTP header Content-Type #2462

Closed
miry opened this issue Feb 26, 2024 · 7 comments · Fixed by #2465
Closed

browser_monitoring throws exception on non string HTTP header Content-Type #2462

miry opened this issue Feb 26, 2024 · 7 comments · Fixed by #2465
Assignees
@fallwith
Labels
3 Story Point Estimate bug community To tag external issues and PRs submitted by the community

Comments

@miry
Copy link

miry commented Feb 26, 2024 •
edited
Loading

Description

Found two problems in case the request HTTP header ContentType is Symbol,
then newrelic just produces exception:

NoMethodError "undefined method `include?' for an instance of Symbol

Expected Behavior

Newrelic should catch all exceptions with invalid headers and does not throw exception.

Troubleshooting or NR Diag results

Provide any other relevant log data.
TIP: Scrub logs and diagnostic information for sensitive information

The line:

newrelic-ruby-agent/lib/new_relic/rack/browser_monitoring.rb

Line 103 in 414ddad

headers[CONTENT_TYPE] && headers[CONTENT_TYPE].include?(TEXT_HTML) # rubocop:disable Style/SafeNavigation

Backtrace 

/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/rack/browser_monitoring.rb:103:in `html?'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/rack/browser_monitoring.rb:65:in `should_instrument?'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/rack/browser_monitoring.rb:42:in `traced_call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/warden-jwt_auth-0.8.0/lib/warden/jwt_auth/middleware/token_dispatcher.rb:20:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/warden-jwt_auth-0.8.0/lib/warden/jwt_auth/middleware/revocation_manager.rb:21:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/rack-2.2.8.1/lib/rack/builder.rb:244:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/warden-jwt_auth-0.8.0/lib/warden/jwt_auth/middleware.rb:22:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/warden-1.2.9/lib/warden/manager.rb:36:in `block in call'
/app/vendor/bundle/ruby/3.3.0/gems/warden-1.2.9/lib/warden/manager.rb:34:in `catch'
/app/vendor/bundle/ruby/3.3.0/gems/warden-1.2.9/lib/warden/manager.rb:34:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/rack-2.2.8.1/lib/rack/tempfile_reaper.rb:15:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/rack-2.2.8.1/lib/rack/etag.rb:27:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/rack-2.2.8.1/lib/rack/conditional_get.rb:27:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/rack-2.2.8.1/lib/rack/head.rb:12:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/actionpack-7.0.8.1/lib/action_dispatch/http/permissions_policy.rb:38:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
<truncated 43 additional frames>
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/actionpack-7.0.8.1/lib/action_dispatch/middleware/executor.rb:14:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/actionpack-7.0.8.1/lib/action_dispatch/middleware/static.rb:23:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/rack-2.2.8.1/lib/rack/sendfile.rb:110:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/actionpack-7.0.8.1/lib/action_dispatch/middleware/ssl.rb:77:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/actionpack-7.0.8.1/lib/action_dispatch/middleware/host_authorization.rb:131:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/rack-cors-2.0.1/lib/rack/cors.rb:102:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/railties-7.0.8.1/lib/rails/engine.rb:530:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/instrumentation/middleware_tracing.rb:99:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/puma-6.4.2/lib/puma/configuration.rb:272:in `call'
/app/vendor/bundle/ruby/3.3.0/gems/puma-6.4.2/lib/puma/request.rb:100:in `block in handle_request'
/app/vendor/bundle/ruby/3.3.0/gems/puma-6.4.2/lib/puma/thread_pool.rb:378:in `with_force_shutdown'
/app/vendor/bundle/ruby/3.3.0/gems/puma-6.4.2/lib/puma/request.rb:99:in `handle_request'
/app/vendor/bundle/ruby/3.3.0/gems/puma-6.4.2/lib/puma/server.rb:464:in `process_client'
/app/vendor/bundle/ruby/3.3.0/gems/puma-6.4.2/lib/puma/server.rb:245:in `block in run'
/app/vendor/bundle/ruby/3.3.0/gems/puma-6.4.2/lib/puma/thread_pool.rb:155:in `block in spawn_thread'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/tracer.rb:434:in `block (2 levels) in thread_block_with_current_transaction'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/tracer.rb:357:in `capture_segment_error'
/app/vendor/bundle/ruby/3.3.0/gems/newrelic_rpm-9.7.1/lib/new_relic/agent/tracer.rb:433:in `block in thread_block_with_current_transaction'

Steps to Reproduce

Set in test header['Content-Type'] = :"text/html"

Your Environment

Rails 7.0
Ruby 3.3
@miry miry added the bug label Feb 26, 2024
@workato-integration
Copy link

https://new-relic.atlassian.net/browse/NR-232640

@github-actions github-actions bot added the community To tag external issues and PRs submitted by the community label Feb 26, 2024
@miry miry changed the title browser_monitoring expects string for HTTP header and raises error browser_monitoring throws exception on non string HTTP header Content-Type Feb 26, 2024
@kford-newrelic kford-newrelic moved this from Triage to In Sprint in Ruby Engineering Board Feb 26, 2024
@kford-newrelic kford-newrelic added the estimate Issue needing estimation label Feb 26, 2024
@fallwith
Copy link
Contributor

Hi @miry,

Thanks for letting us know about this issue. It looks like you have a symbol (:'text-html') as a header hash value when we were expecting a string ('text-html'). We attempt to invoke String#include? which won't work on an instance of Symbol.

Would you please let us know which part of your app / stack is responsible for setting that ':text-html' value?

We currently expect a) that all header hashes we're working with have been built by Rack and b) that they adhere to the Rack spec that requires header hash keys and values to be strings. The latest version of Rack still provides Rack::Headers#invert, which sees the keys become values and the values become keys, and it seems to assume strings too.

We are curious if there is something in Rails v7 or Rack v3 that is using symbols that we weren't previously aware of, or if your application is using something other than Rack or using some custom middleware to touch the headers.

If you happen to know how the symbol is being set as a hash value, that information would help us in coming up with a fix. Please let us know your best understanding of how the value is getting set when you get a chance. Thanks!

@fallwith fallwith self-assigned this Feb 26, 2024
@miry
Copy link
Author

miry commented Feb 26, 2024 •
edited
Loading

I have the same expectations; headers are supposed to consist only of strings.

The challenge for me is that I've been unable to reproduce the problem on my development environment, but it does occur in staging and production (and don't have access to staging and production). I would try to simulate the problem though.
As you can see the backtrace (in the issue's description),
here are 3 suspects: newrelic_rpm itself, warden, rails and friends.

However, resolving the header issue is not the primary focus here
(I assume the primitive fix will be something: headers[CONTENT_TYPE].is_a?(String) && ...).
The true aim is to explore alternative methods of handling exceptions without interrupting the existing flow.

@fallwith
Copy link
Contributor

Thanks, @miry.

As part of resolving this one we'll be sure to harden the various touchpoints defined in browser_monitoring.rb that could interrupt customer application flow. Any internal errors experienced by that class should simply result in the browser agent <script> tag not being inserted in the outbound HTML. Any errors should not bring the app down with it. Totally agree.

@miry
Copy link
Author

miry commented Feb 27, 2024 •
edited
Loading

A bit more of the context: Ruby runs with +YJIT mode.
The requests has "Content-Type": "application/json".
It was also visible on NewRelic exceptions page.

The application runs behind Cloudflare on Heroku.
Not all endpoints were affected,
we identified only on single endpoint, that also strange.

I tried to reproduce localy, but did not help.

UPDATE:

If I understand correctly the Middleware calls, after the next call,
it means the headers (means response header) was changed on Next middlewares and Application level.
So I would assume there is a some sort helper render "{}", type: :json

newrelic-ruby-agent/lib/new_relic/rack/browser_monitoring.rb

Line 38 in 414ddad

result = @app.call(env)

In this case we use https://github.com/ruby-grape/grape.

I would expect clients would make such mistakes :(

UPDATE:

Now I found the reason of such sily mistake. As I expected in the Grape code someone wrote:

    desc "Return a list of comments for the specific product"
    get do
        # ...
        content_type :json
        # ...
    end

MY SUMMARY:

  1. Application could return a not valid response headers, rails/puma do not complain about it :(.
  2. The exception is general and does not help with identification of the root cause (i did mistake, i really thought it was request headers).

fallwith added a commit that referenced this issue Feb 27, 2024
@fallwith
browser monitoring injection logic hardening
799d12b 
These changes introduce hardening to the logic that determines whether
or not to inject the browser agent script tag.

In particular, the following 2 issues have been addressed:

1. If either `#traced_call` or `#should_instrument?` encounter any
   exceptions, these exceptions should be handled and logged without
   impacting the observed web application.
2. Allow a response headers hash to use symbols for values.

Closes #2462
@fallwith fallwith mentioned this issue Feb 27, 2024
Merged
@fallwith
Copy link
Contributor

Thank you, @miry. I have submitted #2465 to address these issues. The other repo maintainers will take a look sometime in the near future.

@kford-newrelic kford-newrelic added 3 Story Point Estimate and removed estimate Issue needing estimation labels Feb 27, 2024
@github-project-automation github-project-automation bot moved this from In Sprint to Code Complete/Done in Ruby Engineering Board Feb 29, 2024
@fallwith
Copy link
Contributor

Hi @miry. The PR has been merged and the next version of the agent that gets released with accept symbol based values and better handle exceptions. Thanks for bringing this one to our attention!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fallwith fallwith

Labels
3 Story Point Estimate bug community To tag external issues and PRs submitted by the community
Projects
Ruby Engineering Board
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

browser monitoring injection logic hardening newrelic/newrelic-ruby-agent
3 participants
@miry @fallwith @kford-newrelic