Skip to content
Permalink
Browse files Browse the repository at this point in the history
SECURITY: Prevent XSS in local oneboxes (#20009)
Co-authored-by: OsamaSayegh <asooomaasoooma90@gmail.com>
  • Loading branch information
nbianca and OsamaSayegh committed Jan 25, 2023
1 parent 15a2af1 commit 1a5a6f6
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 4 deletions.
@@ -0,0 +1,22 @@
# frozen_string_literal: true

class TriggerPostRebakeLocalOneboxXss < ActiveRecord::Migration[7.0]
def up
val =
DB.query_single(
"SELECT value FROM site_settings WHERE name = 'content_security_policy'",
).first

return if val == nil || val == "t"

DB.exec(<<~SQL)
UPDATE posts
SET baked_version = NULL
WHERE cooked LIKE '%<a href=%'
SQL
end

def down
raise ActiveRecord::IrreversibleMigration
end
end
19 changes: 15 additions & 4 deletions lib/oneboxer.rb
Expand Up @@ -343,7 +343,8 @@ def self.local_onebox(url, opts = {})
end
end

html = html.presence || "<a href='#{URI(url).to_s}'>#{URI(url).to_s}</a>"
normalized_url = ::Onebox::Helpers.normalize_url_for_output(URI(url).to_s)
html = html.presence || "<a href='#{normalized_url}'>#{normalized_url}</a>"
{ onebox: html, preview: html }
end

Expand All @@ -355,18 +356,28 @@ def self.local_upload_html(url)
""
end

normalized_url = ::Onebox::Helpers.normalize_url_for_output(url)
case File.extname(URI(url).path || "")
when VIDEO_REGEX
<<~HTML
<div class="onebox video-onebox">
<video #{additional_controls} width="100%" height="100%" controls="">
<source src='#{url}'>
<a href='#{url}'>#{url}</a>
<source src='#{normalized_url}'>
<a href='#{normalized_url}'>
#{normalized_url}
</a>
</video>
</div>
HTML
when AUDIO_REGEX
"<audio #{additional_controls} controls><source src='#{url}'><a href='#{url}'>#{url}</a></audio>"
<<~HTML
<audio #{additional_controls} controls>
<source src='#{normalized_url}'>
<a href='#{normalized_url}'>
#{normalized_url}
</a>
</audio>
HTML
end
end

Expand Down
60 changes: 60 additions & 0 deletions spec/lib/oneboxer_spec.rb
Expand Up @@ -198,6 +198,66 @@ def preview(url, user = nil, category = nil, topic = nil)
"<p><a href=\"#{Discourse.base_url}/new?%27class=black\">http://test.localhost/new?%27class=black</a></p>",
)
end

it "escapes URLs of local audio uploads" do
result =
described_class.onebox_raw(
"#{Discourse.base_url}/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#'<>",
)
expect(result[:onebox]).to eq(<<~HTML)
<audio controls>
<source src='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E'>
<a href='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E'>
http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E
</a>
</audio>
HTML
expect(result[:preview]).to eq(<<~HTML)
<audio controls>
<source src='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E'>
<a href='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E'>
http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.wav#&apos;%3C%3E
</a>
</audio>
HTML
end

it "escapes URLs of local video uploads" do
result =
described_class.onebox_raw(
"#{Discourse.base_url}/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#'<>",
)
expect(result[:onebox]).to eq(<<~HTML)
<div class="onebox video-onebox">
<video width="100%" height="100%" controls="">
<source src='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E'>
<a href='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E'>
http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E
</a>
</video>
</div>
HTML
expect(result[:preview]).to eq(<<~HTML)
<div class="onebox video-onebox">
<video width="100%" height="100%" controls="">
<source src='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E'>
<a href='http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E'>
http://test.localhost/uploads/default/original/1X/a1c31803be81b85ecafc4f77b1008eee9b3b82f4.mp4#&apos;%3C%3E
</a>
</video>
</div>
HTML
end

it "escapes URLs of generic local links" do
result = described_class.onebox_raw("#{Discourse.base_url}/g/somegroup#'onerror='")
expect(result[:onebox]).to eq(
"<a href='http://test.localhost/g/somegroup#&apos;onerror=&apos;'>http://test.localhost/g/somegroup#&apos;onerror=&apos;</a>",
)
expect(result[:preview]).to eq(
"<a href='http://test.localhost/g/somegroup#&apos;onerror=&apos;'>http://test.localhost/g/somegroup#&apos;onerror=&apos;</a>",
)
end
end

describe ".external_onebox" do
Expand Down

0 comments on commit 1a5a6f6

Please sign in to comment.