Skip to content

Commit

Permalink
SECURITY: escape display names
Browse files Browse the repository at this point in the history
Ensure we escape the display names before passing it to the regexp used to update
quotes whenever a user change their display name.
  • Loading branch information
ZogStriP authored and lis2 committed Nov 9, 2023
1 parent 332f562 commit 2ec2510
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 13 deletions.
17 changes: 12 additions & 5 deletions lib/quote_rewriter.rb
Expand Up @@ -6,17 +6,20 @@ def initialize(user_id)
end

def rewrite_raw_username(raw, old_username, new_username)
escaped_old_username = Regexp.escape(old_username)
pattern =
Regexp.union(
/(?<pre>\[quote\s*=\s*["'']?.*username:)#{old_username}(?<post>\,?[^\]]*\])/i,
/(?<pre>\[quote\s*=\s*["'']?)#{old_username}(?<post>\,?[^\]]*\])/i,
/(?<pre>\[quote\s*=\s*["'']?.*username:)#{escaped_old_username}(?<post>\,?[^\]]*\])/i,
/(?<pre>\[quote\s*=\s*["'']?)#{escaped_old_username}(?<post>\,?[^\]]*\])/i,
)

raw.gsub(pattern, "\\k<pre>#{new_username}\\k<post>")
end

def rewrite_cooked_username(cooked, old_username, new_username, avatar_img)
pattern = /(?<=\s)#{PrettyText::Helpers.format_username(old_username)}(?=:)/i
formatted_old_username = PrettyText::Helpers.format_username(old_username)
escaped_old_username = Regexp.escape(formatted_old_username)
pattern = /(?<=\s)#{escaped_old_username}(?=:)/i

cooked
.css("aside.quote")
Expand All @@ -42,13 +45,17 @@ def rewrite_cooked_username(cooked, old_username, new_username, avatar_img)
end

def rewrite_raw_display_name(raw, old_display_name, new_display_name)
pattern = /(?<pre>\[quote\s*=\s*["'']?)#{old_display_name}(?<post>\,[^\]]*username[^\]]*\])/i
escaped_old_display_name = Regexp.escape(old_display_name)
pattern =
/(?<pre>\[quote\s*=\s*["'']?)#{escaped_old_display_name}(?<post>\,[^\]]*username[^\]]*\])/i

raw.gsub(pattern, "\\k<pre>#{new_display_name}\\k<post>")
end

def rewrite_cooked_display_name(cooked, old_display_name, new_display_name)
pattern = /(?<=\s)#{PrettyText::Helpers.format_username(old_display_name)}(?=:)/i
formatted_old_display_name = PrettyText::Helpers.format_username(old_display_name)
escaped_old_display_name = Regexp.escape(formatted_old_display_name)
pattern = /(?<=\s)#{escaped_old_display_name}(?=:)/i

cooked
.css("aside.quote")
Expand Down
20 changes: 12 additions & 8 deletions spec/jobs/change_display_name_spec.rb
Expand Up @@ -3,33 +3,37 @@
RSpec.describe Jobs::ChangeDisplayName do
before { stub_image_size }

let(:user) { Fabricate(:user, username: "codinghorror", name: "Jeff") }
let(:username) { "codinghorror" }
let(:old_display_name) { "|| Jeff ||" }
let(:new_display_name) { "|| Mr. Atwood ||" }

let(:user) { Fabricate(:user, username: username, name: old_display_name) }
let(:topic) { Fabricate(:topic, user: user) }
let!(:post) { create_post(post_attributes.merge(topic_id: topic.id)) }

let!(:quoted_post) { create_post(user: user, topic: topic, post_number: 1, raw: "quoted post") }
let(:avatar_url) { user.avatar_template_url.gsub("{size}", "48") }

let(:post_attributes) { { raw: <<~RAW } }
[quote="Jeff, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
[quote="#{old_display_name}, post:1, topic:#{quoted_post.topic.id}, username:#{username}"]
quoted post
[/quote]
RAW

let(:revised_post_attributes) { { raw: <<~RAW } }
[quote="Jeff, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
[quote="#{old_display_name}, post:1, topic:#{quoted_post.topic.id}, username:#{username}"]
quoted post
[/quote]
Forgot something.
RAW

let(:args) { { user_id: user.id, old_name: "Jeff", new_name: "Mr. Atwood" } }
let(:args) { { user_id: user.id, old_name: old_display_name, new_name: new_display_name } }

describe "#execute" do
context "when the renamed user has been quoted" do
it "rewrites the raw quote display name" do
expect { described_class.new.execute(args) }.to change { post.reload.raw }.to(<<~RAW.strip)
[quote="Mr. Atwood, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
[quote="#{new_display_name}, post:1, topic:#{quoted_post.topic.id}, username:#{username}"]
quoted post
[/quote]
RAW
Expand All @@ -38,10 +42,10 @@
it "rewrites the cooked quote display name" do
expect { described_class.new.execute(args) }.to change { post.reload.cooked }.to(
match_html(<<~HTML.strip),
<aside class="quote no-group" data-username="codinghorror" data-post="1" data-topic="#{quoted_post.topic.id}">
<aside class="quote no-group" data-username="#{username}" data-post="1" data-topic="#{quoted_post.topic.id}">
<div class="title">
<div class="quote-controls"></div>
<img loading="lazy" alt="" width="24" height="24" src="#{avatar_url}" class="avatar"> Mr. Atwood:</div>
<img loading="lazy" alt="" width="24" height="24" src="#{avatar_url}" class="avatar"> #{new_display_name}:</div>
<blockquote>
<p>quoted post</p>
</blockquote>
Expand All @@ -58,7 +62,7 @@
expect { described_class.new.execute(args) }.to change {
post.reload.revisions[0].modifications["raw"][0]
}.to(<<~RAW.strip)
[quote="Mr. Atwood, post:1, topic:#{quoted_post.topic.id}, username:codinghorror"]
[quote="#{new_display_name}, post:1, topic:#{quoted_post.topic.id}, username:#{username}"]
quoted post
[/quote]
RAW
Expand Down

0 comments on commit 2ec2510

Please sign in to comment.