Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AO3-6719 Leave src URLs instead of emptiness for images stripped in AO3-6564 #4799

Merged
merged 20 commits into from
Apr 28, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/helpers/comments_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ def get_commenter_pseud_or_name(comment)
end
end

def image_safety_mode_cache_key(comment)
"image-safety-mode" if comment.use_image_safety_mode?
end

####
## Mar 4 2009 Enigel: the below shouldn't happen anymore, please test
####
Expand Down
6 changes: 5 additions & 1 deletion app/models/comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,11 @@ def mark_unhidden!
end

def sanitized_content
sanitize_field(self, :comment_content, strip_images: ultimate_parent.is_a?(AdminPost))
sanitize_field(self, :comment_content, image_safety_mode: use_image_safety_mode?)
end

def use_image_safety_mode?
parent_type.in?(ArchiveConfig.PARENTS_WITH_IMAGE_SAFETY_MODE)
end
include Responder
end
2 changes: 1 addition & 1 deletion app/models/feedback_reporters/abuse_reporter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,6 @@ def subject
def ticket_description
return "No comment submitted." if description.blank?

strip_images(description.html_safe)
strip_images(description.html_safe, keep_src: true)
end
end
2 changes: 1 addition & 1 deletion app/models/feedback_reporters/support_reporter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,6 @@ def subject
def ticket_description
return "No description submitted." if description.blank?

strip_images(description.html_safe)
strip_images(description.html_safe, keep_src: true)
end
end
2 changes: 1 addition & 1 deletion app/views/admin/banners/_banner.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
<% # don't forget to update layouts/banner! %>
<div class="<%= admin_banner.banner_type %> announcement group">
<blockquote class="userstuff">
<%= raw sanitize_field(admin_banner, :content, strip_images: true) %>
<%= raw sanitize_field(admin_banner, :content, image_safety_mode: true) %>
</blockquote>
</div>
2 changes: 1 addition & 1 deletion app/views/bookmarks/_bookmark_blurb_short.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
<% if bookmark.bookmarker_notes.present? %>
<h6 class="landmark heading"><%= ts("Bookmark Notes:") %></h6>
<blockquote class="userstuff summary">
<%= raw sanitize_field(bookmark, :bookmarker_notes, strip_images: true) %>
<%= raw sanitize_field(bookmark, :bookmarker_notes, image_safety_mode: true) %>
</blockquote>
<% end %>
<!--actions-->
Expand Down
2 changes: 1 addition & 1 deletion app/views/bookmarks/_bookmark_user_module.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
<% if bookmark.bookmarker_notes.present? %>
<h6 class="landmark heading"><%= ts('Bookmarker\'s Notes') %></h6>
<blockquote class="userstuff notes">
<%= raw sanitize_field(bookmark, :bookmarker_notes, strip_images: true) %>
<%= raw sanitize_field(bookmark, :bookmarker_notes, image_safety_mode: true) %>
</blockquote>
<% end %>
<% # end of information added by the bookmark owner %>
Expand Down
4 changes: 2 additions & 2 deletions app/views/comments/_single_comment.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
<%= time_in_zone(single_comment.created_at) %>
</span>
</h4>
<% cache([single_comment, single_comment.comment_owner, "body"], expires_in: 1.week) do %>
<% cache([single_comment, single_comment.comment_owner, "body", image_safety_mode_cache_key(single_comment)], expires_in: 1.week) do %>
<div class="icon">
<% if !single_comment.pseud.nil? %>
<% if single_comment.by_anonymous_creator? %>
Expand All @@ -56,7 +56,7 @@
<p class="notice"><%= ts("This comment has been hidden by an admin.") %></p>
<% end %>
<blockquote class="userstuff">
<%= raw sanitize_field(single_comment, :comment_content, strip_images: single_comment.ultimate_parent.is_a?(AdminPost)) %>
<%= raw sanitize_field(single_comment, :comment_content, image_safety_mode: single_comment.use_image_safety_mode?) %>
</blockquote>
<% end %>
<% if single_comment.edited_at.present? %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/inbox/_inbox_comment_contents.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,5 @@
</div>

<blockquote class="userstuff">
<%= raw sanitize_field(feedback_comment, :comment_content, strip_images: feedback_comment.ultimate_parent.is_a?(AdminPost)) %>
<%= raw sanitize_field(feedback_comment, :comment_content, image_safety_mode: feedback_comment.use_image_safety_mode?) %>
</blockquote>
2 changes: 1 addition & 1 deletion app/views/layouts/_banner.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<% unless session[:hide_banner] || current_user&.preference&.banner_seen %>
<div class="<%= @admin_banner.banner_type %> announcement group" id="admin-banner">
<blockquote class="userstuff">
<%= raw sanitize_field(@admin_banner, :content, strip_images: true) %>
<%= raw sanitize_field(@admin_banner, :content, image_safety_mode: true) %>
</blockquote>
<% if current_user.nil? %>
<p class="submit">
Expand Down
2 changes: 1 addition & 1 deletion app/views/pseuds/_pseud_module.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@
</div>
<% if pseud.description.present? %>
<blockquote class="userstuff">
<%= raw sanitize_field(pseud, :description, strip_images: true) %>
<%= raw sanitize_field(pseud, :description, image_safety_mode: true) %>
</blockquote>
<% end %>
2 changes: 1 addition & 1 deletion app/views/share/_embed_meta_bookmark.erb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
<% end %>
<% if bookmark.bookmarker_notes.present? %>
<%= ts("Bookmarker's Notes: ").html_safe %>
<%= raw sanitize_field(bookmark, :bookmarker_notes, strip_images: true) %>
<%= raw sanitize_field(bookmark, :bookmarker_notes, image_safety_mode: true) %>
<% end %>
<% end %>
2 changes: 1 addition & 1 deletion app/views/user_mailer/abuse_report.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
</p>

<p><%= style_work_metadata_label(t(".copy.comment")) %></p>
<p><%= style_quote(raw(strip_images(@comment))) %></p>
<p><%= style_quote(raw(strip_images(@comment, keep_src: true))) %></p>

<p><%= t(".thank_you") %></p>

Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/abuse_report.text.erb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
<%= to_plain_text(raw @summary) %>

<%= work_metadata_label(t(".copy.comment")) %>
<%= to_plain_text(raw @comment) %>
<%= to_plain_text(raw(strip_images(@comment, keep_src: true))) %>

<%= text_divider %>

Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/feedback.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
form:
</p>

<%= style_quote("<b>#{raw(strip_images(@summary))}</b> #{raw(strip_images(@comment))}") %>
<%= style_quote("<b>#{raw(strip_images(@summary))}</b> #{raw(strip_images(@comment, keep_src: true))}") %>

<p>
If you have additional questions or information, do not hesitate to send in
Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/feedback.text.erb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ information you submitted through the Technical Support and Feedback form:
<%= text_divider %>

* <%= to_plain_text(raw @summary) %> *
<%= to_plain_text(raw @comment) %>
<%= to_plain_text(raw(strip_images(@comment, keep_src: true))) %>

<%= text_divider %>

Expand Down
6 changes: 6 additions & 0 deletions config/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,12 @@ NONZERO_INTEGER_PARAMETERS:
page: 1
per_page: 25

# Enable image safety mode when sanitizing comments on the following parents for
# display. This is the parent_type of the comment, not the ultimate_parent,
# so if you want to add extra sanitization to work comments, use Chapter here.
# Options: AdminPost, Chapter, Tag.
PARENTS_WITH_IMAGE_SAFETY_MODE: []

# These fields are encrypted and should not have characters escaped or be treated as HTML
FIELDS_WITHOUT_SANITIZATION: ["password", "password_check", "password_confirmation"]

Expand Down
11 changes: 0 additions & 11 deletions features/comments_and_kudos/comments_adminposts.feature
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,3 @@ Feature: Commenting on admin posts
When I follow "Edit Post"
Then I should see "No one can comment"
# TODO: Test that the other options aren't available/selected in a non-brittle way

Scenario: Admin post comment does not display images
Given I have posted an admin post
And I am logged in as "regular"
And I go to the admin-posts page
And I follow "Default Admin Post"
When I fill in "Comment" with "Hi!<img src='http://example.com/icon.svg'>"
And I press "Comment"
Then I should see "Comment created!"
And I should not see the image "src" text "http://example.com/icon.svg"
And I should see "Hi!"
62 changes: 62 additions & 0 deletions features/comments_and_kudos/image_safety_mode.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
Feature: Image safety mode
In order to protect users
As a site owner
I'd like to control which comments can include images

Scenario Outline: Images are embedded in comments when image safety mode is off.
Given the setup for testing image safety mode on <commentable>
And image safety mode is disabled for comments
When I view <commentable> with comments
Then I should see the image "src" text "https://example.com/image.jpg"
And I should not see "OMG! https://example.com/image.jpg"
Comment on lines +9 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test added to add_comment.feature in #4729 can be removed, because this tests the same.

When I go to the homepage
Then I should see the image "src" text "https://example.com/image.jpg"
And I should not see "OMG! https://example.com/image.jpg"
When I go to my inbox page
Then I should see the image "src" text "https://example.com/image.jpg"
When image safety mode is enabled for comments on a "<parent_type>"
And I view <commentable> with comments
Then I should not see the image "src" text "https://example.com/image.jpg"
But I should see "OMG! https://example.com/image.jpg"
When I go to the homepage
Then I should not see the image "src" text "https://example.com/image.jpg"
But I should see "OMG! https://example.com/image.jpg"
When I go to my inbox page
Then I should not see the image "src" text "https://example.com/image.jpg"
But I should see "OMG! https://example.com/image.jpg"

Examples:
| commentable | parent_type |
| the admin post "Change Log" | AdminPost |
| the work "My Opus" | Chapter |
| the tag "No Fandom" | Tag |

Scenario Outline: Embedded images in comments are replaced with their URLs when image safety mode is enabled.
Given the setup for testing image safety mode on <commentable>
And image safety mode is enabled for comments on a "<parent_type>"
When I view <commentable> with comments
Then I should not see the image "src" text "https://example.com/image.jpg"
But I should see "OMG! https://example.com/image.jpg"
When I go to the homepage
Then I should not see the image "src" text "https://example.com/image.jpg"
But I should see "OMG! https://example.com/image.jpg"
When I go to my inbox page
Then I should not see the image "src" text "https://example.com/image.jpg"
But I should see "OMG! https://example.com/image.jpg"
When image safety mode is disabled for comments
And I view <commentable> with comments
Then I should see the image "src" text "https://example.com/image.jpg"
And I should not see "OMG! https://example.com/image.jpg"
When I go to the homepage
Then I should see the image "src" text "https://example.com/image.jpg"
And I should not see "OMG! https://example.com/image.jpg"
When I go to my inbox page
Then I should see the image "src" text "https://example.com/image.jpg"
And I should not see "OMG! https://example.com/image.jpg"

Examples:
| parent_type | commentable |
| AdminPost | the admin post "Change Log" |
| Chapter | the work "My Opus" |
| Tag | the tag "No Fandom" |

9 changes: 0 additions & 9 deletions features/comments_and_kudos/inbox.feature
Original file line number Diff line number Diff line change
Expand Up @@ -159,12 +159,3 @@ Feature: Get messages in the inbox
And I go to the homepage
Then I should see "sewwiththeflo on Cat Thor's Bizarre Adventure"
And I should see "Thank you! Please go to bed."

Scenario: Reply to a comment on an admin post that contains an image
Given I have posted an admin post
And a comment "My comment" by "sewwiththeflo" on the admin post "Default Admin Post"
And a reply "My reply <img src='foo.jpg' />" by "unbeatablesg" on the admin post "Default Admin Post"
When I am logged in as "sewwiththeflo"
And I go to the homepage
Then I should see "My reply"
And I should not see "<img src='foo.jpg' />"
10 changes: 7 additions & 3 deletions features/other_a/abuse_report.feature
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,13 @@ Feature: Filing an abuse report
Given I am logged in as "otheruser"
And basic languages
When I follow "Policy Questions & Abuse Reports"
And I fill in "Description of the content you are reporting (required)" with "This is wrong"
And I fill in "Brief summary of Terms of Service violation (required)" with '<img src="foo.jpg" />Hi'
And I fill in "Brief summary of Terms of Service violation (required)" with '<img src="foo.jpg" />Gross'
And I fill in "Description of the content you are reporting (required)" with "This is wrong <img src='bar.jpeg' />"
And I fill in "Link to the page you are reporting (required)" with "http://www.archiveofourown.org/works"
And I press "Submit"
Then 1 email should be delivered
And the email should not contain "<img src="foo.jpg" />"
# The sanitizer adds the domain in front of relative image URLs as of AO3-6571
And the email should not contain "<img src="http://www.example.org/foo.jpg" />"
And the email should not contain "<img src="http://www.example.org/bar.jpeg" />"
But the email should contain "Gross"
And the email should contain "This is wrong http://www.example.org/bar.jpeg"
4 changes: 3 additions & 1 deletion features/other_b/support.feature
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,6 @@ Feature: Filing a support request
And I fill in "Your question or problem" with '<img src="foo.jpg" />Hi'
And I press "Send"
Then 1 email should be delivered
And the email should not contain "<img src="foo.jpg" />"
# The sanitizer adds the domain in front of relative image URLs as of AO3-6571
And the email should not contain "<img src="http://www.example.org/foo.jpg" />"
But the email should contain "http://www.example.org/foo.jpgHi"
27 changes: 27 additions & 0 deletions features/step_definitions/comment_steps.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,33 @@
comment_content: text)
end

Given "image safety mode is enabled for comments on a {string}" do |parent_type|
allow(ArchiveConfig).to receive(:PARENTS_WITH_IMAGE_SAFETY_MODE).and_return(parent_type)
end

Given "image safety mode is disabled for comments" do
allow(ArchiveConfig).to receive(:PARENTS_WITH_IMAGE_SAFETY_MODE).and_return([])
end

Given "the setup for testing image safety mode on the admin post {string}" do |title|
step %{the admin post "#{title}"}
step %{a comment "plain text" by "commentrecip" on the admin post "#{title}"}
step %{a reply 'OMG! <img src="https://example.com/image.jpg">' by "commenter" on the admin post "#{title}"}
step %{I am logged in as "commentrecip"}
end

Given "the setup for testing image safety mode on the tag {string}" do |name|
step %{the tag wrangler "commentrecip" with password "password" is wrangler of "No Fandom"}
step %{a comment 'OMG! <img src="https://example.com/image.jpg">' by "commenter" on the tag "#{name}"}
step %{I am logged in as "commentrecip"}
end

Given "the setup for testing image safety mode on the work {string}" do |title|
step %{the work "#{title}" by "commentrecip"}
step %{a comment 'OMG! <img src="https://example.com/image.jpg">' by "commenter" on the work "#{title}"}
step %{I am logged in as "commentrecip"}
end

# THEN

Then /^the comment's posted date should be nowish$/ do
Expand Down
16 changes: 11 additions & 5 deletions lib/html_cleaner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
module HtmlCleaner
# If we aren't sure that this field hasn't been sanitized since the last sanitizer version,
# we sanitize it before we allow it to pass through (and save it if possible).
def sanitize_field(object, fieldname, strip_images: false)
# For certain highly visible fields, we might want to be cautious about
# images. Use image_safety_mode: true to prevent images from embedding as-is.
def sanitize_field(object, fieldname, image_safety_mode: false)
return "" if object.send(fieldname).nil?

sanitizer_version = object.try("#{fieldname}_sanitizer_version")
Expand All @@ -15,7 +17,7 @@ def sanitize_field(object, fieldname, strip_images: false)
sanitize_value(fieldname, object.send(fieldname))
end

sanitized_field = strip_images(sanitized_field) if strip_images
sanitized_field = strip_images(sanitized_field, keep_src: true) if image_safety_mode
sanitized_field
end

Expand Down Expand Up @@ -144,9 +146,13 @@ def add_paragraphs_to_text(text)
# These assume they are running on well-formed XHTML, which we can do
# because they will only be used on already-cleaned fields.

# strip img tags
def strip_images(value)
value.gsub(/<img .*?>/, '')
# strip img tags, optionally leaving the src URL exposed
def strip_images(value, keep_src: false)
value.gsub(/(<img .*?>)/) do |img_tag|
match_data = img_tag.match(/src="([^"]+)"/) if keep_src
src = match_data[1] if match_data
src || ""
end
end

def strip_html_breaks_simple(value)
Expand Down
Loading
Loading