Skip to content

Commit

Permalink
SECURITY: ensure url is properly normalized and quote escaped
Browse files Browse the repository at this point in the history
  • Loading branch information
SamSaffron committed Dec 19, 2016
1 parent afad3e0 commit bf72af9
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 7 deletions.
5 changes: 3 additions & 2 deletions lib/onebox/engine/audio_onebox.rb
Expand Up @@ -3,14 +3,15 @@ module Engine
class AudioOnebox
include Engine

matches_regexp /^(https?:)?\/\/.*\.(mp3|ogg|wav|m4a)(\?.*)?$/i
matches_regexp(/^(https?:)?\/\/.*\.(mp3|ogg|wav|m4a)(\?.*)?$/i)

def always_https?
WhitelistedGenericOnebox.host_matches(uri, WhitelistedGenericOnebox.https_hosts)
end

def to_html
"<audio controls><source src='#{@url}'><a href='#{@url}'>#{@url}</a></audio>"
url = ::Onebox::Helpers.normalize_url_for_output(@url)
"<audio controls><source src='#{url}'><a href='#{url}'>#{url}</a></audio>"
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/onebox/engine/image_onebox.rb
Expand Up @@ -3,7 +3,7 @@ module Engine
class ImageOnebox
include Engine

matches_regexp /^(https?:)?\/\/.+\.(png|jpg|jpeg|gif|bmp|tif|tiff)(\?.*)?$/i
matches_regexp(/^(https?:)?\/\/.+\.(png|jpg|jpeg|gif|bmp|tif|tiff)(\?.*)?$/i)

def always_https?
WhitelistedGenericOnebox.host_matches(uri, WhitelistedGenericOnebox.https_hosts)
Expand All @@ -15,7 +15,7 @@ def to_html
@url.gsub!("https://www.dropbox.com","https://dl.dropboxusercontent.com")
end

escaped = url.gsub(/'/, "%27")
escaped = Onebox::Helpers.normalize_url_for_output(url)
"<a href='#{escaped}' target='_blank'><img src='#{escaped}'></a>"
end
end
Expand Down
5 changes: 3 additions & 2 deletions lib/onebox/engine/video_onebox.rb
Expand Up @@ -3,14 +3,15 @@ module Engine
class VideoOnebox
include Engine

matches_regexp /^(https?:)?\/\/.*\.(mov|mp4|webm|ogv)(\?.*)?$/i
matches_regexp(/^(https?:)?\/\/.*\.(mov|mp4|webm|ogv)(\?.*)?$/i)

def always_https?
WhitelistedGenericOnebox.host_matches(uri, WhitelistedGenericOnebox.https_hosts)
end

def to_html
"<video width='100%' height='100%' controls><source src='#{@url}'><a href='#{@url}'>#{@url}</a></video>"
url = ::Onebox::Helpers.normalize_url_for_output(@url)
"<video width='100%' height='100%' controls><source src='#{url}'><a href='#{url}'>#{url}</a></video>"
end
end
end
Expand Down
9 changes: 9 additions & 0 deletions lib/onebox/helpers.rb
Expand Up @@ -71,5 +71,14 @@ def self.truncate(string, length = 50)
def self.title_attr(meta)
(meta && !blank?(meta[:title])) ? "title='#{CGI.escapeHTML(meta[:title])}'" : ""
end

def self.normalize_url_for_output(url)
url = url.dup
# expect properly encoded url, remove any unsafe chars
url.gsub!(/[^a-zA-Z0-9%\-`._~:\/?#\[\]@!$&'\(\)*+,;=]/, "")
url.gsub!("'", "&quot;")
url
end

end
end
4 changes: 4 additions & 0 deletions spec/lib/onebox/engine/audio_onebox_spec.rb
Expand Up @@ -28,4 +28,8 @@
it "includes a fallback direct link to the audio" do
expect(Onebox.preview('http://kolber.github.io/audiojs/demos/mp3/juicy.mp3').to_s).to match(/<a.*mp3/)
end

it "correctly escapes single quotes" do
expect(Onebox.preview("http://test.com/test'ing.mp3").to_s).not_to match(/test'ing/)
end
end
2 changes: 1 addition & 1 deletion spec/lib/onebox/engine/image_onebox_spec.rb
Expand Up @@ -38,6 +38,6 @@
end

it "doesn't inline single quotes" do
expect(Onebox.preview("http://host/path/to/image'withquote.png").to_s).to match(/image%27withquote/)
expect(Onebox.preview("http://host/path/to/Image'withquote.png").to_s).to match(/Image&quot;withquote/)
end
end

0 comments on commit bf72af9

Please sign in to comment.