diff --git a/app/controllers/concerns/authentication/session_lookup.rb b/app/controllers/concerns/authentication/session_lookup.rb index b180f21..f9c0181 100644 --- a/app/controllers/concerns/authentication/session_lookup.rb +++ b/app/controllers/concerns/authentication/session_lookup.rb @@ -1,7 +1,7 @@ module Authentication::SessionLookup def find_session_by_cookie if token = cookies.signed[:session_token] - Session.find_by(token: token) + Session.eager_load(:user).find_by(token: token) end end end diff --git a/app/controllers/concerns/cached_responses.rb b/app/controllers/concerns/cached_responses.rb index f25cf6a..bc45c88 100644 --- a/app/controllers/concerns/cached_responses.rb +++ b/app/controllers/concerns/cached_responses.rb @@ -47,8 +47,6 @@ module CachedResponses return yield unless encoding key = response_cache_key(encoding) - original_session = session.to_hash.deep_dup - return yield if key.bytesize > ResponseCache::MAX_KEY_BYTES entry = ResponseCache.instance.read(key, @response_cache_version) @@ -57,6 +55,7 @@ module CachedResponses ResponseCache.instance.synchronize_render(key, @response_cache_version) do entry = ResponseCache.instance.read(key, @response_cache_version) if !entry && ResponseCache.instance.version == @response_cache_version + original_session = session.to_hash.deep_dup yield rendered = true entry = cache_completed_response(key, original_session, encoding) diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb index 6f80646..5c63392 100644 --- a/app/controllers/messages_controller.rb +++ b/app/controllers/messages_controller.rb @@ -28,7 +28,9 @@ class MessagesController < ApplicationController set_room @message = @room.messages.create_with_attachment!(message_params) - @message.broadcast_create + # Both deliveries contain the same token-free, viewer-independent markup. + @message_html = render_to_string partial: "messages/message", formats: :html, locals: { message: @message } + @message.broadcast_create(html: @message_html) deliver_webhooks_to_bots rescue ActiveRecord::RecordNotFound render action: :room_not_found diff --git a/app/helpers/messages_helper.rb b/app/helpers/messages_helper.rb index 066e15e..6ef8590 100644 --- a/app/helpers/messages_helper.rb +++ b/app/helpers/messages_helper.rb @@ -63,8 +63,7 @@ module MessagesHelper when "sound" message_sound_presentation(message) else - auto_link h(ContentFilters::TextMessagePresentationFilters.apply(message.body.body)), - html: { target: "_blank" }, sanitize_options: { tags: AUTO_LINK_ALLOWED_TAGS, attributes: AUTO_LINK_ALLOWED_ATTRIBUTES } + text_message_presentation(message.body.body) end rescue Exception => e Sentry.capture_exception(e, extra: { message: message }) @@ -73,7 +72,34 @@ module MessagesHelper "" end + # These controls contain URLs and static markup, but no viewer or token state. + # Keep them across database commits; attachment controls still render afresh. + def cache_message_actions(message, &block) + return capture(&block) unless controller.perform_caching && !message.attachment? + + key = fragment_name_with_digest([ + "message-actions-v1", request.base_url, request.script_name, I18n.locale, message.id, message.room_id + ], nil) + FragmentCache.store.fetch(key) { capture(&block) } + end + private + def text_message_presentation(body) + render = -> do + auto_link h(ContentFilters::TextMessagePresentationFilters.apply(body)), + html: { target: "_blank" }, sanitize_options: { tags: AUTO_LINK_ALLOWED_TAGS, attributes: AUTO_LINK_ALLOWED_ATTRIBUTES } + end + + # Embedded attachments render database-backed metadata and signed URLs. + # Plain text HTML depends only on its content, even after a foreign edit. + html = body.to_html + if controller.perform_caching && !html.include?("messages#beforeStreamRender keydown.up@document->messages#editMyLastMessage" end diff --git a/app/models/message/broadcasts.rb b/app/models/message/broadcasts.rb index e3f8c00..7465ccd 100644 --- a/app/models/message/broadcasts.rb +++ b/app/models/message/broadcasts.rb @@ -1,6 +1,6 @@ module Message::Broadcasts - def broadcast_create - broadcast_append_to room, :messages, target: [ room, :messages ] + def broadcast_create(html: nil) + broadcast_append_to room, :messages, target: [ room, :messages ], **(html ? { html: html } : {}) broadcast_unread_room_later end diff --git a/app/views/messages/_actions.html.erb b/app/views/messages/_actions.html.erb index 150c7f5..ab6f2ec 100644 --- a/app/views/messages/_actions.html.erb +++ b/app/views/messages/_actions.html.erb @@ -1,5 +1,6 @@ <%# Be sure to check/update messages/_template.html.erb when changing this file %> +<%= cache_message_actions(message) do %>
<%= tag.details class: "position-relative", data: { controller: "popup", action: "keydown.esc->popup#close toggle->popup#toggle click@document->popup#closeOnClickOutside", popup_orientation_top_class: "popup-orientation-top" } do %> @@ -56,3 +57,4 @@
<% end %> +<% end %> diff --git a/app/views/messages/create.turbo_stream.erb b/app/views/messages/create.turbo_stream.erb index 4990631..5f93315 100644 --- a/app/views/messages/create.turbo_stream.erb +++ b/app/views/messages/create.turbo_stream.erb @@ -1 +1 @@ -<%= turbo_stream.append dom_id(@message.room, :messages), @message %> +<%= turbo_stream.append dom_id(@message.room, :messages), @message_html %> diff --git a/test/controllers/fragment_cache_test.rb b/test/controllers/fragment_cache_test.rb index 0e8501d..f74953a 100644 --- a/test/controllers/fragment_cache_test.rb +++ b/test/controllers/fragment_cache_test.rb @@ -63,6 +63,17 @@ class FragmentRenderingTest < ActionDispatch::IntegrationTest assert_includes response.body, "foreign conditional body" end + test "message controls survive creator edits while the author is rendered fresh" do + get room_messages_url(@room) + assert_response :success + foreign_write("UPDATE users SET name = ? WHERE id = ?", "Fresh creator with cached controls", @message.creator_id) + ActionView::Base.any_instance.expects(:form_with).never + get room_messages_url(@room) + assert_response :success + assert_includes response.body, "Fresh creator with cached controls" + assert_select "form[action=?]", message_boosts_path(@message) + end + test "a foreign commit after capture bypasses old native fragment lookups" do ResponseCache.instance.stubs(:budget).returns(0) get room_messages_url(@room) diff --git a/test/controllers/messages_controller_test.rb b/test/controllers/messages_controller_test.rb index d6cbfd0..09a319f 100644 --- a/test/controllers/messages_controller_test.rb +++ b/test/controllers/messages_controller_test.rb @@ -49,8 +49,12 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest end test "creating a message broadcasts the message to the room" do + ContentFilters::TextMessagePresentationFilters.expects(:apply).once.with { |body| body.to_plain_text == "New one" }.returns(ActionText::Content.new("New one")) post room_messages_url(@room, format: :turbo_stream), params: { message: { body: "New one", client_message_id: 999 } } + assert_response :success + assert_select "turbo-stream[action=append] .message__body", text: /New one/ + assert_rendered_turbo_stream_broadcast @room, :messages, action: "append", target: [ @room, :messages ] do assert_select ".message__body", text: /New one/ assert_copy_link_button room_at_message_url(@room, Message.last, host: "once.campfire.test") diff --git a/test/helpers/content_filters_test.rb b/test/helpers/content_filters_test.rb index 11df023..208c87a 100644 --- a/test/helpers/content_filters_test.rb +++ b/test/helpers/content_filters_test.rb @@ -172,7 +172,7 @@ class ContentFiltersTest < ActionView::TestCase filtered = ContentFilters::TextMessagePresentationFilters.apply(message.body.body).to_html - assert_equal body, filtered + assert_dom_equal body, filtered assert_match %r{.*.*}m, message_presentation(message) end diff --git a/test/helpers/messages_helper_test.rb b/test/helpers/messages_helper_test.rb index 78a97c9..5bf77ac 100644 --- a/test/helpers/messages_helper_test.rb +++ b/test/helpers/messages_helper_test.rb @@ -1,6 +1,18 @@ require "test_helper" class MessagesHelperTest < ActionView::TestCase + test "plain text presentation is reused by content rather than database epoch" do + view.controller.stubs(:perform_caching).returns(true) + FragmentCache.store.clear + message = Message.create! room: rooms(:pets), body: "A reusable safe body", creator: users(:jason) + first = view.message_presentation(message) + assert_includes first, "A reusable safe body" + ContentFilters::TextMessagePresentationFilters.expects(:apply).never + assert_equal first, view.message_presentation(message) + ensure + FragmentCache.store.clear + end + test "message_presentation neutralizes unsafe URI schemes in links" do message = Message.create! room: rooms(:pets), body: '
x
', client_message_id: "0015", creator: users(:jason)

Name

Jason