Merge pull request #2137 from basecamp/flavorjones/data-action-fix

Image lightbox uses Stimulus target callbacks instead of `data-action`

app/assets/stylesheets/lightbox.css

@@ -67,7 +67,7 @@
   }
 
   .lightbox__caption {
-    color: var(--color-ink-inverted);
+    color: var(--color-white);
 
     &:empty {
       display: none;

app/javascript/controllers/lightbox_controller.js

@@ -3,9 +3,22 @@ import { Controller } from "@hotwired/stimulus"
 export default class extends Controller {
   static targets = [ "caption", "image", "dialog", "zoomedImage" ]
 
-  open(event) {
+  imageTargetConnected(element) {
+    element.addEventListener("click", this.#handleImageClick)
+  }
+
+  imageTargetDisconnected(element) {
+    element.removeEventListener("click", this.#handleImageClick)
+  }
+
+  #handleImageClick = (event) => {
+    event.preventDefault()
+    this.#open(event.currentTarget)
+  }
+
+  #open(link) {
     this.dialogTarget.showModal()
-    this.#set(event.target.closest("a"))
+    this.#set(link)
   }
 
   // Wait for the transition to finish before resetting the image

app/views/active_storage/blobs/web/_representation.html.erb

@@ -15,7 +15,7 @@
     <source src="<%= rails_blob_url(blob) %>" type="<%= blob.content_type %>">
   </audio>
 <% elsif blob.variable? %>
-  <%= link_to url_for(blob.variant(variant)), data: { action: "lightbox#open:prevent", lightbox_target: "image" } do %>
+  <%= link_to url_for(blob.variant(variant)), data: { lightbox_target: "image", lightbox_caption_value: blob.filename.to_s } do %>
     <%= image_tag url_for(blob.variant(variant)), width: width, height: height %>
   <% end %>
 <% elsif blob.previewable? %>

app/views/cards/display/perma/_background.html.erb

@@ -1,5 +1,5 @@
 <%= render "cards/display/common/background", card: card do %>
-  <%= link_to card.image.presence, class: "btn fill-transparent flex-item-justify-end", data: { controller: "tooltip", action: "lightbox#open:prevent", lightbox_target: "image" } do %>
+  <%= link_to card.image.presence, class: "btn fill-transparent flex-item-justify-end", data: { controller: "tooltip", lightbox_target: "image" } do %>
     <%= icon_tag "picture-zoom" %>
     <span class="for-screen-reader">Zoom background image</span>
   <% end %>

config/initializers/sanitization.rb

@@ -1,6 +1,6 @@
 Rails.application.config.after_initialize do
   Rails::HTML5::SafeListSanitizer.allowed_tags.merge(%w[ s table tr td th thead tbody details summary video source ])
-  Rails::HTML5::SafeListSanitizer.allowed_attributes.merge(%w[ data-turbo-frame controls type width data-action data-lightbox-target data-lightbox-url-value ])
+  Rails::HTML5::SafeListSanitizer.allowed_attributes.merge(%w[ data-turbo-frame data-lightbox-target data-lightbox-caption-value controls type width ])
 
   # ugh, see https://github.com/rails/rails/issues/54478 which I need to fix upstream --mike
   ActionText::ContentHelper.allowed_tags = Rails::HTML5::SafeListSanitizer.allowed_tags.to_a + [ ActionText::Attachment.tag_name, "figure", "figcaption" ] + ActionText::ContentHelper.allowed_tags.to_a

test/helpers/action_text_rendering_test.rb

@@ -0,0 +1,15 @@
+require "test_helper"
+
+class ActionTextRenderingTest < ActionView::TestCase
+  test "data-action attributes in user content are stripped" do
+    malicious_html = <<~HTML
+      <p>Click here: <a href="#" data-action="dangerous#action">malicious link</a></p>
+    HTML
+
+    content = ActionText::Content.new(malicious_html)
+    rendered = content.to_s
+
+    assert_no_match(/data-action/, rendered)
+    assert_match(/<a href="#">malicious link<\/a>/, rendered)
+  end
+end

test/system/smoke_test.rb

@@ -54,6 +54,14 @@ class SmokeTest < ApplicationSystemTestCase
       assert_selector "a img[src*='/rails/active_storage']"
       assert_selector "figcaption span.attachment__name", text: "moon.jpg"
     end
+
+    # Click the image to open the lightbox
+    find("action-text-attachment figure.attachment a:has(img)").click
+
+    assert_selector "dialog.lightbox[open]"
+    within("dialog.lightbox") do
+      assert_selector "img.lightbox__image[src*='/rails/active_storage']"
+    end
   end
 
   test "dismissing notifications" do