Skip to content

Commit

Permalink
FIX: reword whitelist to allowlist (#430)
Browse files Browse the repository at this point in the history
The version was bumped to 2.0.0 as this change is not backward compatible
  • Loading branch information
lis2 committed Jul 14, 2020
1 parent 093ad85 commit c27aebc
Show file tree
Hide file tree
Showing 20 changed files with 46 additions and 43 deletions.
8 changes: 4 additions & 4 deletions README.md
Expand Up @@ -75,7 +75,7 @@ Adding Support for a new URL
----------------------------

1. Check if the site supports [oEmbed](http://oembed.com/) or [Open Graph](https://developers.facebook.com/docs/opengraph/).
If it does, you can probably get away with just whitelisting the URL in `Onebox::Engine::WhitelistedGenericOnebox` (see: [Whitelisted Generic Onebox caveats](#user-content-whitelisted-generic-onebox-caveats)).
If it does, you can probably get away with just allowing the URL in `Onebox::Engine::AllowlistedGenericOnebox` (see: [Allowlisted Generic Onebox caveats](#user-content-allowlisted-generic-onebox-caveats)).
If the site does not support open standards, you can create a new engine.

2. Create new onebox engine
Expand Down Expand Up @@ -163,12 +163,12 @@ Adding Support for a new URL
require_relative "engine/name_onebox"
```

Whitelisted Generic Onebox caveats
Allowlisted Generic Onebox caveats
----------------------------------

The Whitelisted Generic Onebox has some caveats for its use, beyond simply whitelisting the domain.
The Allowlisted Generic Onebox has some caveats for its use, beyond simply allowlisting the domain.

1. The domain must be whitelisted
1. The domain must be allowlisted
2. The URL you're oneboxing cannot be a root url (e.g. `http://example.com` won't work, but `http://example.com/page` will)
3. If the oneboxed URL responds with oEmbed and has a `rich` type: the `html` content must contain an `<iframe>`. Responses without an iframe will not be oneboxed.

Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine.rb
Expand Up @@ -141,7 +141,7 @@ def always_https?
require_relative "engine/wikipedia_onebox"
require_relative "engine/youtube_onebox"
require_relative "engine/youku_onebox"
require_relative "engine/whitelisted_generic_onebox"
require_relative "engine/allowlisted_generic_onebox"
require_relative "engine/pubmed_onebox"
require_relative "engine/soundcloud_onebox"
require_relative "engine/imgur_onebox"
Expand Down
Expand Up @@ -4,20 +4,20 @@

module Onebox
module Engine
class WhitelistedGenericOnebox
class AllowlistedGenericOnebox
include Engine
include StandardEmbed
include LayoutSupport

def self.whitelist=(list)
@whitelist = list
def self.allowed_domains=(list)
@allowed_domains = list
end

def self.whitelist
@whitelist ||= default_whitelist.dup
def self.allowed_domains
@allowed_domains ||= default_allowed_domains.dup
end

def self.default_whitelist
def self.default_allowed_domains
%w(
23hq.com
500px.com
Expand Down Expand Up @@ -176,13 +176,13 @@ def self.probable_wordpress(uri)
!!(uri.path =~ /\d{4}\/\d{2}\//)
end

def self.twitter_label_whitelist
def self.allowed_twitter_labels
['brand', 'price', 'usd', 'cad', 'reading time', 'likes']
end

def self.===(other)
other.kind_of?(URI) ?
host_matches(other, whitelist) || probable_wordpress(other) || probable_discourse(other) :
host_matches(other, allowed_domains) || probable_wordpress(other) || probable_discourse(other) :
super
end

Expand Down Expand Up @@ -233,11 +233,11 @@ def data
end

# Twitter labels
if !Onebox::Helpers.blank?(d[:label1]) && !Onebox::Helpers.blank?(d[:data1]) && !!WhitelistedGenericOnebox.twitter_label_whitelist.find { |l| d[:label1] =~ /#{l}/i }
if !Onebox::Helpers.blank?(d[:label1]) && !Onebox::Helpers.blank?(d[:data1]) && !!AllowlistedGenericOnebox.allowed_twitter_labels.find { |l| d[:label1] =~ /#{l}/i }
d[:label_1] = Onebox::Helpers.truncate(d[:label1])
d[:data_1] = Onebox::Helpers.truncate(d[:data1])
end
if !Onebox::Helpers.blank?(d[:label2]) && !Onebox::Helpers.blank?(d[:data2]) && !!WhitelistedGenericOnebox.twitter_label_whitelist.find { |l| d[:label2] =~ /#{l}/i }
if !Onebox::Helpers.blank?(d[:label2]) && !Onebox::Helpers.blank?(d[:data2]) && !!AllowlistedGenericOnebox.allowed_twitter_labels.find { |l| d[:label2] =~ /#{l}/i }
unless Onebox::Helpers.blank?(d[:label_1])
d[:label_2] = Onebox::Helpers.truncate(d[:label2])
d[:data_2] = Onebox::Helpers.truncate(d[:data2])
Expand All @@ -261,7 +261,7 @@ def data
def rewrite_https(html)
return unless html
uri = URI(@url)
if WhitelistedGenericOnebox.host_matches(uri, WhitelistedGenericOnebox.rewrites)
if AllowlistedGenericOnebox.host_matches(uri, AllowlistedGenericOnebox.rewrites)
html = html.gsub("http://", "https://")
end
html
Expand Down Expand Up @@ -309,7 +309,7 @@ def is_embedded?
data[:height] &&
(
data[:html]["iframe"] ||
WhitelistedGenericOnebox.html_providers.include?(data[:provider_name])
AllowlistedGenericOnebox.html_providers.include?(data[:provider_name])
)
end

Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine/audio_onebox.rb
Expand Up @@ -8,7 +8,7 @@ class AudioOnebox
matches_regexp(/^(https?:)?\/\/.*\.(mp3|ogg|opus|wav|m4a)(\?.*)?$/i)

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

def to_html
Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine/facebook_media_onebox.rb
Expand Up @@ -22,7 +22,7 @@ def to_html
</iframe>
HTML
else
html = Onebox::Engine::WhitelistedGenericOnebox.new(@url, @timeout).to_html
html = Onebox::Engine::AllowlistedGenericOnebox.new(@url, @timeout).to_html
return if Onebox::Helpers.blank?(html)
html
end
Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine/gfycat_onebox.rb
Expand Up @@ -10,7 +10,7 @@ class GfycatOnebox
always_https

def self.priority
# This engine should have priority over WhitelistedGenericOnebox.
# This engine should have priority over AllowlistedGenericOnebox.
1
end

Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine/image_onebox.rb
Expand Up @@ -8,7 +8,7 @@ class ImageOnebox
matches_regexp(/^(https?:)?\/\/.+\.(png|jpg|jpeg|gif|bmp|tif|tiff)(\?.*)?$/i)

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

def to_html
Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine/reddit_media_onebox.rb
Expand Up @@ -45,7 +45,7 @@ def to_html
</aside>
HTML
else
html = Onebox::Engine::WhitelistedGenericOnebox.new(@url, @timeout).to_html
html = Onebox::Engine::AllowlistedGenericOnebox.new(@url, @timeout).to_html
return if Onebox::Helpers.blank?(html)
html
end
Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine/standard_embed.rb
Expand Up @@ -32,7 +32,7 @@ def self.add_opengraph_provider(regexp)
add_oembed_provider(/nytimes\.com\//, 'https://www.nytimes.com/svc/oembed/json/')

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

def raw
Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine/video_onebox.rb
Expand Up @@ -8,7 +8,7 @@ class VideoOnebox
matches_regexp(/^(https?:)?\/\/.*\.(mov|mp4|webm|ogv)(\?.*)?$/i)

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

def to_html
Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/engine/youtube_onebox.rb
Expand Up @@ -45,7 +45,7 @@ def to_html
HTML
else
# for channel pages
html = Onebox::Engine::WhitelistedGenericOnebox.new(@url, @timeout).to_html
html = Onebox::Engine::AllowlistedGenericOnebox.new(@url, @timeout).to_html
return if Onebox::Helpers.blank?(html)
html.gsub!(/['"]\/\//, "https://")
html
Expand Down
5 changes: 4 additions & 1 deletion lib/onebox/matcher.rb
Expand Up @@ -16,7 +16,10 @@ def oneboxed
uri = URI(@url)
return unless uri.port.nil? || Onebox.options.allowed_ports.include?(uri.port)
return unless uri.scheme.nil? || Onebox.options.allowed_schemes.include?(uri.scheme)
ordered_engines.find { |engine| engine === uri }
ordered_engines
.select { |engine| engine === uri }
.sort_by { |engine| engine.to_s }
.last
rescue URI::InvalidURIError
nil
end
Expand Down
2 changes: 1 addition & 1 deletion lib/onebox/version.rb
@@ -1,5 +1,5 @@
# frozen_string_literal: true

module Onebox
VERSION = "1.9.30"
VERSION = "2.0.0"
end
2 changes: 1 addition & 1 deletion spec/fixtures/discourse_topic.response

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion spec/fixtures/discourse_topic_reply.response

Large diffs are not rendered by default.

Expand Up @@ -2,11 +2,11 @@

require "spec_helper"

describe Onebox::Engine::WhitelistedGenericOnebox do
describe Onebox::Engine::AllowlistedGenericOnebox do

describe ".===" do
before do
described_class.whitelist = %w(eviltrout.com discourse.org)
described_class.allowed_domains = %w(eviltrout.com discourse.org)
end

it "matches an entire domain" do
Expand All @@ -31,7 +31,7 @@
end

describe 'html_providers' do
class HTMLOnebox < Onebox::Engine::WhitelistedGenericOnebox
class HTMLOnebox < Onebox::Engine::AllowlistedGenericOnebox
def data
{
html: 'cool html',
Expand All @@ -42,30 +42,30 @@ def data
end

it "doesn't return the HTML when not in the `html_providers`" do
Onebox::Engine::WhitelistedGenericOnebox.html_providers = []
Onebox::Engine::AllowlistedGenericOnebox.html_providers = []
expect(HTMLOnebox.new("http://coolsite.com").to_html).to be_nil
end

it "returns the HMTL when in the `html_providers`" do
Onebox::Engine::WhitelistedGenericOnebox.html_providers = ['CoolSite']
Onebox::Engine::AllowlistedGenericOnebox.html_providers = ['CoolSite']
expect(HTMLOnebox.new("http://coolsite.com").to_html).to eq "cool html"
end
end

describe 'rewrites' do
class DummyOnebox < Onebox::Engine::WhitelistedGenericOnebox
class DummyOnebox < Onebox::Engine::AllowlistedGenericOnebox
def generic_html
"<iframe src='http://youtube.com/asdf'></iframe>"
end
end

it "doesn't rewrite URLs that arent in the list" do
Onebox::Engine::WhitelistedGenericOnebox.rewrites = []
Onebox::Engine::AllowlistedGenericOnebox.rewrites = []
expect(DummyOnebox.new("http://youtube.com").to_html).to eq "<iframe src='http://youtube.com/asdf'></iframe>"
end

it "rewrites URLs when whitelisted" do
Onebox::Engine::WhitelistedGenericOnebox.rewrites = %w(youtube.com)
it "rewrites URLs when allowlisted" do
Onebox::Engine::AllowlistedGenericOnebox.rewrites = %w(youtube.com)
expect(DummyOnebox.new("http://youtube.com").to_html).to eq "<iframe src='https://youtube.com/asdf'></iframe>"
end
end
Expand Down Expand Up @@ -154,7 +154,7 @@ def generic_html
let(:redirect_link) { 'http://www.dailymail.co.uk/news/article-479146/Brutality-justice-The-truth-tarred-feathered-drug-dealer.html' }

before do
described_class.whitelist = %w(dailymail.co.uk discourse.org)
described_class.allowed_domains = %w(dailymail.co.uk discourse.org)
FakeWeb.register_uri(
:get,
original_link,
Expand Down
2 changes: 1 addition & 1 deletion spec/lib/onebox/layout_spec.rb
Expand Up @@ -59,7 +59,7 @@

it "rewrites relative image path" do
record = { image: "/image.png", link: "https://discourse.org" }
klass = described_class.new("whitelistedgeneric", record)
klass = described_class.new("allowlistedgeneric", record)
expect(klass.view.record[:image]).to include("https://discourse.org")
end
end
Expand Down
4 changes: 2 additions & 2 deletions spec/lib/onebox/matcher_spec.rb
Expand Up @@ -52,7 +52,7 @@ def self.===(uri)
end
end

describe "with a whitelisted port/scheme" do
describe "with a allowlisted port/scheme" do
%w{http://example.com https://example.com http://example.com:80 //example.com}.each do |url|
it "finds an engine for '#{url}'" do
matcher = Onebox::Matcher.new(url)
Expand All @@ -62,7 +62,7 @@ def self.===(uri)
end
end

describe "without a whitelisted port/scheme" do
describe "without a allowlisted port/scheme" do
%w{http://example.com:21 ftp://example.com}.each do |url|
it "doesn't find an engine for '#{url}'" do
matcher = Onebox::Matcher.new(url)
Expand Down
2 changes: 1 addition & 1 deletion spec/lib/onebox_spec.rb
Expand Up @@ -24,7 +24,7 @@ def expect_templates_to_not_match(text)

describe 'has_matcher?' do
before do
Onebox::Engine::WhitelistedGenericOnebox.whitelist = %w(youtube.com)
Onebox::Engine::AllowlistedGenericOnebox.allowed_domains = %w(youtube.com)
end

it "has no matcher for a made up url" do
Expand Down
File renamed without changes.

0 comments on commit c27aebc

Please sign in to comment.