Clean up Lookbook-related alerts (#2846)

Co-authored-by: Cameron Dutro <[email protected]>

Author: joelhawksley

.changeset/heavy-radios-confess.md

@@ -0,0 +1,5 @@
+---
+"@primer/view-components": patch
+---
+
+Clean up Lookbook-related security alerts.

app/components/primer/alpha/toggle_switch.rb

@@ -56,10 +56,6 @@ def initialize(src: nil, csrf_token: nil, checked: false, enabled: true, size: S
         }
 
         @system_arguments[:src] = @src if @src
-
-        return unless @src && @csrf_token
-
-        @system_arguments[:csrf] = @csrf_token
       end
 
       def on?
@@ -73,6 +69,22 @@ def enabled?
       def disabled?
         !enabled?
       end
+
+      private
+
+      def before_render
+        @csrf_token ||= view_context.form_authenticity_token(
+          form_options: {
+            method: :post,
+            action: @src
+          }
+        )
+
+        @system_arguments[:data] = merge_data(
+          @system_arguments,
+          { data: { csrf: @csrf_token } }
+        )
+      end
     end
   end
 end

app/components/primer/alpha/toggle_switch.ts

@@ -19,7 +19,7 @@ class ToggleSwitchElement extends HTMLElement {
 
   get csrf(): string | null {
     const csrfElement = this.querySelector('[data-csrf]')
-    return this.getAttribute('csrf') || (csrfElement instanceof HTMLInputElement && csrfElement.value) || null
+    return this.getAttribute('data-csrf') || (csrfElement instanceof HTMLInputElement && csrfElement.value) || null
   }
 
   get csrfField(): string {

app/lib/primer/attributes_helper.rb

@@ -54,7 +54,7 @@ def merge_aria(*hashes)
     # It's designed to be used to normalize and merge data information from system_arguments
     # hashes. Consider using this pattern in component initializers:
     #
-    # @system_arguments[:data] = merge_aria(
+    # @system_arguments[:data] = merge_data(
     #   @system_arguments,
     #   { data: { foo: "bar" } }
     # )

demo/app/controllers/application_controller.rb

@@ -2,5 +2,5 @@
 
 # :nodoc:
 class ApplicationController < ActionController::Base
-  protect_from_forgery
+  protect_from_forgery with: :exception
 end

demo/app/controllers/auto_check_controller.rb

@@ -3,8 +3,6 @@
 # For auto-check previews
 # :nocov:
 class AutoCheckController < ApplicationController
-  skip_before_action :verify_authenticity_token
-
   def error
     render partial: "auto_check/error_message",
       locals: { input_value: params[:value] },

demo/app/controllers/toggle_switch_controller.rb

@@ -7,39 +7,37 @@ class << self
     attr_accessor :last_request
   end
 
-  skip_before_action :verify_authenticity_token
+  rescue_from ActionController::InvalidAuthenticityToken, with: :handle_invalid_authenticity_token
 
   before_action :reject_non_ajax_request
-  before_action :verify_artificial_authenticity_token
 
   def create
     # lol this is so not threadsafe
     self.class.last_request = request
 
     sleep 1 unless Rails.env.test?
 
+    if params[:fail] == "true"
+      render status: :internal_server_error, plain: "Something went wrong."
+      return
+    end
+
     head :accepted
   end
 
   private
 
+  def handle_invalid_authenticity_token
+    render status: :unauthorized, plain: "Bad CSRF token."
+  end
+
   # this mimics dotcom behavior
   def reject_non_ajax_request
     return if request.headers["HTTP_REQUESTED_WITH"] == "XMLHttpRequest"
 
     head :unprocessable_entity
   end
 
-  def verify_artificial_authenticity_token
-    # don't check token if not provided
-    return unless form_params[:authenticity_token]
-
-    # if provided, check token
-    return if form_params[:authenticity_token] == "let_me_in"
-
-    render status: :unauthorized, plain: "Bad CSRF token"
-  end
-
   def form_params
     params.permit(:value, :authenticity_token)
   end

demo/app/views/lookbook/panels/_assets.html.erb

@@ -34,7 +34,7 @@
           <h4 class="asset-title font-mono"><%= asset[:path].relative_path_from(Primer::ViewComponents.root) %></h4>
         </header>
         <div class="asset-contents">
-          <%= code language: asset[:language], line_numbers: true do %><%== File.read(asset[:path]) %><% end %>
+          <%= code language: asset[:language], line_numbers: true do %><%= File.read(asset[:path]) %><% end %>
         </div>
       </div>
     <% end %>

demo/config/environments/test.rb

@@ -31,7 +31,7 @@
   config.action_dispatch.show_exceptions = false
 
   # Disable request forgery protection in test environment.
-  config.action_controller.allow_forgery_protection = false
+  config.action_controller.allow_forgery_protection = true
 
   # Print deprecation notices to the stderr.
   config.active_support.deprecation = :stderr
@@ -48,9 +48,7 @@
   config.autoload_paths << Rails.root.join("..", "test", "forms")
   config.view_component.preview_paths << Rails.root.join("..", "test", "previews")
 
-  # rubocop:disable Style/IfUnlessModifier
   if ENV.fetch("VC_COMPAT_PATCH_ENABLED", "false") == "true"
     config.view_component.capture_compatibility_patch_enabled = true
   end
-  # rubocop:enable Style/IfUnlessModifier
 end

lib/primer/forms/toggle_switch.html.erb

@@ -10,13 +10,5 @@
 
     <div><%= render(Caption.new(input: @input)) %></div>
   </span>
-  <%
-    csrf = @input.csrf || @view_context.form_authenticity_token(
-      form_options: {
-        method: :post,
-        action: @input.src
-      }
-    )
-  %>
-  <%= render(Primer::Alpha::ToggleSwitch.new(src: @input.src, csrf: csrf, **@input.input_arguments)) %>
+  <%= render(Primer::Alpha::ToggleSwitch.new(src: @input.src, csrf_token: @input.csrf, **@input.input_arguments)) %>
 <% end %>

previews/primer/alpha/toggle_switch_preview.rb

@@ -52,7 +52,7 @@ def with_no_src
       end
 
       def with_csrf_token
-        render(Primer::Alpha::ToggleSwitch.new(src: UrlHelpers.toggle_switch_index_path, csrf_token: "let_me_in"))
+        render(Primer::Alpha::ToggleSwitch.new(src: UrlHelpers.toggle_switch_index_path))
       end
 
       def with_bad_csrf_token

previews/primer/forms_preview/example_toggle_switch_form.html.erb

@@ -1,3 +1,3 @@
-<%= render(ExampleToggleSwitchForm.new(csrf: "let_me_in", label: "Good example", src: toggle_switch_index_path, id: "success-toggle")) %>
+<%= render(ExampleToggleSwitchForm.new(label: "Good example", src: toggle_switch_index_path, id: "success-toggle")) %>
 <hr>
-<%= render(ExampleToggleSwitchForm.new(csrf: "a_bad_value", label: "Bad example", src: toggle_switch_index_path, id: "error-toggle")) %>
+<%= render(ExampleToggleSwitchForm.new(label: "Bad example", src: toggle_switch_index_path(fail: true), id: "error-toggle")) %>

test/components/alpha/toggle_switch_test.rb

@@ -66,7 +66,7 @@ def test_small
       def test_csrf_token
         render_inline(Primer::Alpha::ToggleSwitch.new(src: "/foo", csrf_token: "abc123"))
 
-        assert_selector("[csrf]")
+        assert_selector("[data-csrf]")
       end
     end
   end

test/lib/primer/forms/integration_forms_test.rb

@@ -47,19 +47,12 @@ def test_toggle_switch_form_errors
       wait_for_toggle_switch_spinner
 
       assert_selector("#error-toggle [data-target='toggle-switch.errorIcon']")
-      assert_selector(".FormControl-inlineValidation", text: "Bad CSRF token")
+      assert_selector(".FormControl-inlineValidation", text: "Something went wrong.")
 
-      page.evaluate_script(<<~JAVASCRIPT)
-        document
-          .querySelector('toggle-switch#error-toggle')
-          .setAttribute('csrf', 'let_me_in');
-      JAVASCRIPT
-
-      find("#error-toggle").click
+      find("#success-toggle").click
       wait_for_toggle_switch_spinner
 
-      refute_selector("#error-toggle [data-target='toggle-switch.errorIcon']")
-      refute_selector("#error-toggle", text: "Bad CSRF token")
+      refute_selector("#success-toggle [data-target='toggle-switch.errorIcon']")
     end
 
     def test_action_menu_form_input

test/lib/primer/forms/toggle_switch_form_test.rb

@@ -5,14 +5,19 @@
 class Primer::Forms::ToggleSwitchFormTest < Minitest::Test
   include Primer::ComponentTestHelpers
 
-  def test_it_renders_with_a_name
-    bogus_csrf = "let me in"
-    render_inline(ExampleToggleSwitchForm.new(csrf: bogus_csrf, src: "/toggle_switch"))
+  def test_it_renders
+    render_inline(ExampleToggleSwitchForm.new(src: "/toggle_switch"))
 
-    assert_selector "toggle-switch[src='/toggle_switch'][csrf='#{bogus_csrf}']"
+    assert_selector "toggle-switch[src='/toggle_switch']"
     assert_selector "em", text: "favorite"
   end
 
+  def test_accepts_custom_csrf_token
+    bogus_csrf = "let me in"
+    render_inline(ExampleToggleSwitchForm.new(csrf: bogus_csrf, src: "/toggle_switch"))
+    assert_selector "toggle-switch[data-csrf='#{bogus_csrf}']"
+  end
+
   def test_can_render_without_subclass
     render_inline(
       Primer::Forms::ToggleSwitchForm.new(