Rails CSP nonce auto inject

[baldarn]

Overview

The idea is to allow apps with auto_instrument: true and CSP with nonce enforced to still have newrelic working

Submitter Checklist:

• Include a link to the related GitHub issue, if applicable
  newrelic & sidekiq 7.2.1 agent injection not working? #2427
• Include a security review link, if applicable

Testing

The agent includes a suite of unit and functional tests which should be used to
verify your changes don't break existing functionality. These tests will run with
GitHub Actions when a pull request is made. More details on running the tests locally can be found
here for our unit tests,
and here for our functional tests.
For most contributions it is strongly recommended to add additional tests which
exercise your changes.

Reviewer Checklist

• Perform code review
• Add performance label
• Perform appropriate level of performance testing
• Confirm all checks passed
• Add version label prior to acceptance
• Update Docs https://docs.newrelic.com/docs/apm/agents/ruby-agent/features/new-relic-browser-ruby-agent/

[CLAassistant]

All committers have signed the CLA.

[baldarn]

I'v submitted this to have your opinion for this feature. the code is not ready, that's just something like a monkey patch I did in my code to have this working without disabling the auto_instrument: true.

I think, with some more work and tests we could have this working for rails and other frameworks as well!

[kaylareopelle]

Hi @baldarn! Thanks for sharing your idea with us! ✨

I'll bring this up with the other maintainers and get back to you soon!

[kaylareopelle]

Hi @baldarn, the maintainers agree and we'd like to move forward with your idea! Thank you for your contribution! 🚀

As you mentioned earlier, we'll need tests, etc. before including this in a release, but that's something we could handle if you want to stop here. What next steps would you like to take?

[baldarn]

Hi @baldarn, the maintainers agree and we'd like to move forward with your idea! Thank you for your contribution! 🚀

As you mentioned earlier, we'll need tests, etc. before including this in a release, but that's something we could handle if you want to stop here. What next steps would you like to take?

Great news! I'll try to do something. I have 2 options in my mind, but I first have to try to do some code ;)

[baldarn]

@kaylareopelle I did something but I'm struggling to add the tests. maybe you can point me the right direction or just commit something that tests the code?

It's also not super clear how the configuration works, I added something in
lib/new_relic/agent/configuration/default_source.rb
but I really don't know what I'm doing XD

[kaylareopelle]

Thanks, @baldarn! This looks great! Your instincts were spot on with adding a configuration to default_source. That will provide users with a config option and also generate documentation for it!

We can take care of the tests, thanks for updating the environments!

[kaylareopelle]

I have a few suggestions before approval. Thanks again for you contribution!

[baldarn]

After the PR is merged, this doc should also be updated ;)

https://docs.newrelic.com/docs/apm/agents/ruby-agent/features/new-relic-browser-ruby-agent/

(to avoid forgetting, I added a check list in the PR description ;) )

[fallwith]

Hi @baldarn! With dd5cc7b I have gone ahead and added some simple unit tests to verify the nonce/no-nonce logic you established.

I think these unit tests are sufficient for now. As such, would you please git rm all of the initializers/[content_security_policy.rb files?

As for the changes to newrelic.yml, yes, please delete those and the lines will be added back later via autogeneration. Feel free to ever revert the changes or simply add another commit that removes the lines. Either way is fine.

I think after you do those 2 things (delete the Rails initializer files, remove the content from newrelic.yml), we'll be ready to merge.

[baldarn]

@fallwith done
I also used the ActionDispatch::ContentSecurityPolicy::Request::NONCE to get the env string to get the nonce ;)

taking over for Kayla

[fallwith]

Thanks, @baldarn. Everything looks great. We can ignore the "unit_tests" test failures as those exist in dev right now and are not related to browser monitoring. As long as the "rails" tests pass we'll be ok to merge.

Regarding your suggested documentation update... the text for that page is in a different open source repo here. We can't apply changes there until the changes here have made their way into a new agent release. We will make a note of it for our next release, but if you don't see that page get changed after we next release a new agent version, please feel free to reach out back here or on the docs repo with a new GitHub issue to remind us.

[fallwith]

Thanks again for these changes, @baldarn! They will go out in the next version of the Ruby agent.

[kaylareopelle]

Hi @baldarn! I wanted to let you know this change is part of newrelic_rpm version 9.10.0, which was released yesterday.

I've also updated the browser docs to mention this feature. If there's anything you'd like to see changed about this doc, on the righthand pane, there's an option to edit the doc. This will take you to the New Relic docs-website repository to make changes to the file.

Thanks again for your contribution!

[baldarn]

Hi @kaylareopelle , this was a pleasure xD

In the doc is written that this default is default true, but I'm not sure if this is correct

I don't understand well the config file XD

[kaylareopelle]

@baldarn, thank you for this catch! 😅

You're absolutely right, the default is false. I've updated our changelog and have a PR open to the docs website to make the changes there too.

[sgessa]

We just updated to 9.10 and we didn't change the newrelic.yml file.
This is what we get:

irb> NewRelic::Agent.config[:'browser_monitoring.content_security_policy_nonce']
=> true
irb> NewRelic::Agent.config[:'browser_monitoring.auto_instrument']
=> true

I am confused now 😓
Seems enabled by default.

[kaylareopelle]

We just updated to 9.10...

@sgessa, you're totally right. Thanks for chiming in here. 🙇

Our config can get a little tricky, especially when there's dynamic values at play.

What happened was I looked at the documentation_default in our default_source.rb file, which includes all our configurations. However, that wasn't the genuine default, because it's set dynamically based on the value of rum.enabled, which is a private config, set to true by default. 😅

We're reverting the changelog updates made this morning and fortunately caught the docs PR before it was merged.

This has made for an exciting Friday!

Let us know if you run into any issues with the new feature.