xray-rails icon indicating copy to clipboard operation
xray-rails copied to clipboard

Automatically add a nonce to script tag if a csp-nonce meta tag is available to get it from

Open TylerRick opened this issue 6 years ago • 8 comments

Resolves #100

TylerRick avatar Sep 10 '19 23:09 TylerRick

Thanks for the PR! I left some initial comments. I won't be able to approve or merge this PR until we get Travis CI working.

@brentd or @TylerRick do you know why we aren't seeing CI checks on this PR?

mattbrictson avatar Sep 11 '19 16:09 mattbrictson

I manually restarted the Travis CI build and it seems to be working now 🤔

mattbrictson avatar Sep 11 '19 22:09 mattbrictson

@TylerRick This is failing CI because CI is testing against Rails < 6. I think the appropriate next steps are:

  1. Open a separate PR to add Rails 6 to the CI matrix
  2. Update this PR to only run CSP-related test code for Rails >= 6.0.0

Can you help with either or both of those?

mattbrictson avatar Sep 12 '19 03:09 mattbrictson

@TylerRick This is failing CI because CI is testing against Rails < 6. I think the appropriate next steps are:

  1. Open a separate PR to add Rails 6 to the CI matrix
  2. Update this PR to only run CSP-related test code for Rails >= 6.0.0

Good catch. I've updated it to only run CSP-related test code for Rails >= 5.2.

I also added Rails 6 (and 5.2) to the testing matrix. It's a really trivial change. I don't think we need a separate PR for that unless you're likely to not merge this one right away... Also had to fix another issue related to sprockets 4... Everything is in pretty self-contained commits if we need to cherry pick anything.

Now just trying to fix the test failures... hopefully they'll pass this time around :crossed_fingers:

  3) Xray Bar includes the controller and action
     Failure/Error: find('#xray-bar').should have_text('ApplicationController#root')
     
     Capybara::ElementNotFound:
       Unable to find css "#xray-bar"
     # ./spec/xray/xray_bar_spec.rb:7:in `block (2 levels) in <top (required)>'

TylerRick avatar Feb 24 '21 09:02 TylerRick

Okay, finally got tests passing. Merge please?

Wasted too much time getting it to work with old Rubies/Rails, when I think the correct path should be to just remove support for them going forward (#109)...

Also, now that I realize how easy it is to just manually include this in my app:

<%= javascript_include_tag 'xray', nonce: true if Rails.env.development? %>

I regret even wasting my time getting it to automatically look for and add a nonce to the auto-injected xray.js script. I think we should even consider just removing the auto-injecting feature (#110).

But in any case, even if that ends up getting removed in the future, this PR still has some changes that will be valuable going forward, such as updates to support Sprockets 4, and a simple end-to-end test to make sure the JS actually works (and there weren't any run-time JS errors)...

TylerRick avatar Feb 24 '21 19:02 TylerRick

Hi @mattbrictson,

I love this gem and wie just upgraded to sprockets 4. https://github.com/brentd/xray-rails/issues/113 If this MR had been merged it would have saved us a lot of time and trouble.

Do you plan to continue merging MR and releaseing new versions for this Gem? I don't think there is any similar gem like this, so it would be sad to see it fade away.

If I can help in any way please let me know.

rocket-turtle avatar Oct 15 '21 05:10 rocket-turtle