Skip to content

Commit

Permalink
SECURITY: correctly escape event name (#280)
Browse files Browse the repository at this point in the history
  • Loading branch information
oblakeerickson committed Jun 13, 2022
1 parent 3dd9514 commit 2719b9e
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 6 deletions.
4 changes: 3 additions & 1 deletion assets/javascripts/discourse/widgets/discourse-post-event.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { createWidget } from "discourse/widgets/widget";
import { routeAction } from "discourse/helpers/route-action";
import { buildParams, replaceRaw } from "../../lib/raw-event-helper";
import bootbox from "bootbox";
import { escapeExpression } from "discourse/lib/utilities";

export default createWidget("discourse-post-event", {
tagName: "div.discourse-post-event-widget",
Expand Down Expand Up @@ -160,7 +161,7 @@ export default createWidget("discourse-post-event", {
startsAtMonth: moment(eventModel.starts_at).format("MMM"),
startsAtDay: moment(eventModel.starts_at).format("D"),
eventName: emojiUnescape(
eventModel.name ||
escapeExpression(eventModel.name) ||
this._cleanTopicTitle(
eventModel.post.topic.title,
eventModel.starts_at
Expand Down Expand Up @@ -257,6 +258,7 @@ export default createWidget("discourse-post-event", {
`,

_cleanTopicTitle(topicTitle, startsAt) {
topicTitle = escapeExpression(topicTitle);
const cleaned = cleanTitle(topicTitle, startsAt);
if (cleaned) {
return topicTitle.replace(cleaned, "");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import I18n from "I18n";
import { createWidgetFrom } from "discourse/widgets/widget";
import { DefaultNotificationItem } from "discourse/widgets/default-notification-item";
import { formatUsername } from "discourse/lib/utilities";
import { escapeExpression, formatUsername } from "discourse/lib/utilities";
import { iconNode } from "discourse-common/lib/icon-library";

createWidgetFrom(
Expand All @@ -16,7 +16,9 @@ createWidgetFrom(
const username = `<span>${formatUsername(data.display_username)}</span>`;
let description;
if (data.topic_title) {
description = `<span data-topic-id="${this.attrs.topic_id}">${data.topic_title}</span>`;
description = `<span data-topic-id="${
this.attrs.topic_id
}">${escapeExpression(data.topic_title)}</span>`;
} else {
description = this.description(data);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import I18n from "I18n";
import { createWidgetFrom } from "discourse/widgets/widget";
import { DefaultNotificationItem } from "discourse/widgets/default-notification-item";
import { formatUsername } from "discourse/lib/utilities";
import { escapeExpression, formatUsername } from "discourse/lib/utilities";
import { iconNode } from "discourse-common/lib/icon-library";

createWidgetFrom(DefaultNotificationItem, "event-reminder-notification-item", {
Expand All @@ -14,7 +14,9 @@ createWidgetFrom(DefaultNotificationItem, "event-reminder-notification-item", {

let description;
if (data.topic_title) {
description = `<span data-topic-id="${this.attrs.topic_id}">${data.topic_title}</span>`;
description = `<span data-topic-id="${
this.attrs.topic_id
}">${escapeExpression(data.topic_title)}</span>`;
} else {
description = this.description(data);
}
Expand Down
65 changes: 65 additions & 0 deletions db/post_migrate/20220613073844_unescape_event_name.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# frozen_string_literal: true

class UnescapeEventName < ActiveRecord::Migration[7.0]
disable_ddl_transaction!

def up
# event notifications
start = 1
limit = DB.query_single("SELECT MAX(id) FROM notifications WHERE notification_type IN (27, 28)").first.to_i

notifications_query = <<~SQL
SELECT id, data
FROM notifications
WHERE
id >= :start AND
notification_type IN (27, 28) AND
data::json ->> 'topic_title' LIKE '%&%'
ORDER BY id ASC
LIMIT 1000
SQL
while true
if start > limit
break
end
max_seen = -1
DB.query(notifications_query, start: start).each do |record|
id = record.id
if id > max_seen
max_seen = id
end
data = JSON.parse(record.data)
unescaped = CGI.unescapeHTML(data["topic_title"])
next if unescaped == data["topic_title"]
data["topic_title"] = unescaped
DB.exec(<<~SQL, data: data.to_json, id: id)
UPDATE notifications SET data = :data WHERE id = :id
SQL
end
start += 1000
if max_seen > start
start = max_seen + 1
end
end

# event names
events_query = <<~SQL
SELECT id, name
FROM discourse_post_event_events
WHERE name LIKE '%&%'
ORDER BY id ASC
SQL

DB.query(events_query).each do |event|
unescaped_name = CGI.unescapeHTML(event.name)
next if unescaped_name == event.name
DB.exec(<<~SQL, unescaped_name: unescaped_name, id: event.id)
UPDATE discourse_post_event_events SET name = :unescaped_name WHERE id = :id
SQL
end
end

def down
raise ActiveRecord::IrreversibleMigration
end
end
6 changes: 5 additions & 1 deletion lib/discourse_post_event/event_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ def self.extract_events(post)

if value && valid_options.include?(name)
event ||= {}
event[name.sub('data-', '').to_sym] = CGI.escapeHTML(value)
event[name.sub('data-', '').to_sym] = if name == "data-name"
value
else
CGI.escapeHTML(value)
end
end

valid_custom_fields.each do |valid_custom_field|
Expand Down
5 changes: 5 additions & 0 deletions spec/lib/discourse_post_event/event_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ def build_post(user, raw)
expect(events).to eq([])
end

it 'doesn’t escape event name' do
events = subject.extract_events(build_post(user, '[event start="foo" name="bar <script> baz"]\n[/event]'))
expect(events[0][:name]).to eq("bar <script> baz")
end

context 'with custom fields' do
before do
SiteSetting.discourse_post_event_allowed_custom_fields = 'foo-bar|bar'
Expand Down

0 comments on commit 2719b9e

Please sign in to comment.