From fca166524bcf7f0919af07bf2cb23cd14c8c09f9 Mon Sep 17 00:00:00 2001 From: Chris Gavin Date: Fri, 12 May 2017 09:47:37 +0100 Subject: [PATCH 001/106] Fix the padding on the login and register buttons. --- app/assets/stylesheets/common/base/login.scss | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/assets/stylesheets/common/base/login.scss b/app/assets/stylesheets/common/base/login.scss index e625ab2294..4c03017800 100644 --- a/app/assets/stylesheets/common/base/login.scss +++ b/app/assets/stylesheets/common/base/login.scss @@ -103,7 +103,5 @@ $input-width: 220px; button#login-link, button#new-account-link { background: transparent; - padding-left: 0; - margin-left: 20px; color: dark-light-choose(scale-color($primary, $lightness: 35%), scale-color($secondary, $lightness: 65%)); } From 1dcd61fa341ba101018b7fea202a8d1e565ee082 Mon Sep 17 00:00:00 2001 From: Jared Reisinger Date: Fri, 12 May 2017 13:28:35 -0700 Subject: [PATCH 002/106] Add pagination to /admin/users/list API Prior to this, only the first 100 active/new/etc. users were available via the `/admin/users/list` API. This change adds support for a `page=#` querystring parameter so that *all* of the users can be retrieved. Requests for pages past the last user result in an empty-list response; requests for negative pages (or zero) just return the first page. Added tests to cover pagination. --- lib/admin_user_index_query.rb | 6 ++++- .../components/admin_user_index_query_spec.rb | 24 ++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/lib/admin_user_index_query.rb b/lib/admin_user_index_query.rb index 8279ad2017..ffd1dd404d 100644 --- a/lib/admin_user_index_query.rb +++ b/lib/admin_user_index_query.rb @@ -25,7 +25,11 @@ class AdminUserIndexQuery } def find_users(limit=100) - find_users_query.limit(limit) + page = params[:page].to_i - 1 + if page < 0 + page = 0 + end + find_users_query.limit(limit).offset(page * limit) end def count_users diff --git a/spec/components/admin_user_index_query_spec.rb b/spec/components/admin_user_index_query_spec.rb index e5c7195708..cb0bf16130 100644 --- a/spec/components/admin_user_index_query_spec.rb +++ b/spec/components/admin_user_index_query_spec.rb @@ -26,7 +26,7 @@ describe AdminUserIndexQuery do query = ::AdminUserIndexQuery.new({ order: "trust_level" }) expect(query.find_users_query.to_sql).to match("trust_level DESC") end - + it "allows custom ordering asc" do query = ::AdminUserIndexQuery.new({ order: "trust_level", ascending: true }) expect(query.find_users_query.to_sql).to match("trust_level ASC" ) @@ -43,6 +43,28 @@ describe AdminUserIndexQuery do end end + describe "pagination" do + it "defaults to the first page" do + query = ::AdminUserIndexQuery.new({}) + expect(query.find_users.to_sql).to match("OFFSET 0") + end + + it "offsets by 100 by default for page 2" do + query = ::AdminUserIndexQuery.new({ page: "2"}) + expect(query.find_users.to_sql).to match("OFFSET 100") + end + + it "offsets by limit for page 2" do + query = ::AdminUserIndexQuery.new({ page: "2"}) + expect(query.find_users(10).to_sql).to match("OFFSET 10") + end + + it "ignores negative pages" do + query = ::AdminUserIndexQuery.new({ page: "-2" }) + expect(query.find_users.to_sql).to match("OFFSET 0") + end + end + describe "no users with trust level" do TrustLevel.levels.each do |key, value| From a307ad65172ad21ba59c4b82377440dcf39469fd Mon Sep 17 00:00:00 2001 From: mcmcclur Date: Tue, 16 May 2017 11:17:05 -0400 Subject: [PATCH 003/106] Update crawler_detection.rb Add HTTrack to the list of detected crawlers so that Discourse will serve vanilla HTML per https://meta.discourse.org/t/a-basic-discourse-archival-tool/62614/25 --- lib/crawler_detection.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/crawler_detection.rb b/lib/crawler_detection.rb index 075375ebc9..a8892fdc76 100644 --- a/lib/crawler_detection.rb +++ b/lib/crawler_detection.rb @@ -4,6 +4,6 @@ module CrawlerDetection # added 'Swiftbot' based on https://meta.discourse.org/t/how-to-add-html-markup-or-meta-tags-for-external-search-engine/28220 def self.crawler?(user_agent) - !/Googlebot|Mediapartners|AdsBot|curl|Twitterbot|facebookexternalhit|bingbot|Baiduspider|ia_archiver|Wayback Save Page|360Spider|Swiftbot|YandexBot/.match(user_agent).nil? + !/Googlebot|Mediapartners|AdsBot|curl|HTTrack|Twitterbot|facebookexternalhit|bingbot|Baiduspider|ia_archiver|Wayback Save Page|360Spider|Swiftbot|YandexBot/.match(user_agent).nil? end end From 058cde1fc552e39ff97a5a20795622f2e062538b Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 16 May 2017 17:18:45 +0100 Subject: [PATCH 004/106] Use `bundle exec` for docker_dev rake --- bin/docker/rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/docker/rake b/bin/docker/rake index d70b45d765..146a824bb2 100755 --- a/bin/docker/rake +++ b/bin/docker/rake @@ -1,5 +1,5 @@ #!/bin/bash PARAMS="$@" -CMD="cd /src && RUBY_GLOBAL_METHOD_CACHE_SIZE=131072 LD_PRELOAD=/usr/lib/libjemalloc.so RAILS_ENV=${RAILS_ENV:=development} rake $PARAMS" +CMD="cd /src && RUBY_GLOBAL_METHOD_CACHE_SIZE=131072 LD_PRELOAD=/usr/lib/libjemalloc.so RAILS_ENV=${RAILS_ENV:=development} bundle exec rake $PARAMS" docker exec -it -u discourse:discourse discourse_dev /bin/bash -c "$CMD" From 0306863d7180e7f3ed3a6db493a61ebd38f46a2a Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 15 May 2017 19:47:40 +0800 Subject: [PATCH 005/106] Fix the build on travis. --- Gemfile.lock | 2 +- lib/socket_server.rb | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 13664e3eb1..aa006ab031 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -80,7 +80,7 @@ GEM diff-lcs (1.3) discourse-qunit-rails (0.0.9) railties - discourse_image_optim (0.24.4) + discourse_image_optim (0.24.5) exifr (~> 1.2, >= 1.2.2) fspath (~> 3.0) image_size (~> 1.5) diff --git a/lib/socket_server.rb b/lib/socket_server.rb index de18d22c84..0cd4ffadd5 100644 --- a/lib/socket_server.rb +++ b/lib/socket_server.rb @@ -16,7 +16,8 @@ class SocketServer end def stop - @server.close if @server + @server&.close rescue nil + FileUtils.rm_f(@socket_path) @server = nil @blk = nil end @@ -26,9 +27,14 @@ class SocketServer def new_accept_thread server = @server Thread.new do - done = false - while !done - done = !accept_connection(server) + begin + done = false + while !done + done = !accept_connection(server) + end + ensure + self.stop + Rails.logger.info("Cleaned up socket server at #{@socket_path}") end end end From 8febaa8be75c6df759b6546e271a8262f582a486 Mon Sep 17 00:00:00 2001 From: Rimian Perkins Date: Fri, 19 May 2017 12:38:36 +1000 Subject: [PATCH 006/106] FEATURE: Require spec helpers for plugins * Follows any symlinked plugins --- spec/rails_helper.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 985bea4fb6..9d88029d04 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -29,6 +29,14 @@ Spork.prefork do Dir[Rails.root.join("spec/support/**/*.rb")].each {|f| require f} Dir[Rails.root.join("spec/fabricators/*.rb")].each {|f| require f} + # Require plugin helpers at plugin/[plugin]/spec/plugin_helper.rb (includes symlinked plugins). + if ENV['LOAD_PLUGINS'] == "1" + Dir[Rails.root.join("plugins/**")].each do |plugin| + helper = "#{plugin}/spec/plugin_helper.rb" + require helper if File.exists?(helper) + end + end + # let's not run seed_fu every test SeedFu.quiet = true if SeedFu.respond_to? :quiet From 2b5dfb6e8eb27f54c22e4c84ef4d288686d6bdae Mon Sep 17 00:00:00 2001 From: Rimian Perkins Date: Mon, 22 May 2017 14:50:53 +1000 Subject: [PATCH 007/106] avoid double lookup for plugin helpers --- spec/rails_helper.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 9d88029d04..359727768d 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -31,9 +31,8 @@ Spork.prefork do # Require plugin helpers at plugin/[plugin]/spec/plugin_helper.rb (includes symlinked plugins). if ENV['LOAD_PLUGINS'] == "1" - Dir[Rails.root.join("plugins/**")].each do |plugin| - helper = "#{plugin}/spec/plugin_helper.rb" - require helper if File.exists?(helper) + Dir[Rails.root.join("plugins/*/spec/plugin_helper.rb")].each do |f| + require f end end From f350dd4fd16b1a396571c046476d406b7b396d47 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Mon, 22 May 2017 15:18:13 -0400 Subject: [PATCH 008/106] FIX: possible data leaking from one site to another in multisite in PendingFlagsReminder --- app/jobs/scheduled/pending_flags_reminder.rb | 46 +++++++++++--------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/app/jobs/scheduled/pending_flags_reminder.rb b/app/jobs/scheduled/pending_flags_reminder.rb index 11c28277e1..2a33c51785 100644 --- a/app/jobs/scheduled/pending_flags_reminder.rb +++ b/app/jobs/scheduled/pending_flags_reminder.rb @@ -7,30 +7,35 @@ module Jobs every 1.hour def execute(args) - if SiteSetting.notify_about_flags_after > 0 && - PostAction.flagged_posts_count > 0 && - flag_ids.size > 0 && last_notified_id.to_i < flag_ids.max + if SiteSetting.notify_about_flags_after > 0 - mentions = active_moderator_usernames.size > 0 ? - "@#{active_moderator_usernames.join(', @')} " : "" + flagged_posts_count = PostAction.flagged_posts_count - PostCreator.create( - Discourse.system_user, - target_group_names: Group[:moderators].name, - archetype: Archetype.private_message, - subtype: TopicSubtype.system_message, - title: I18n.t('flags_reminder.subject_template', { count: PostAction.flagged_posts_count }), - raw: mentions + I18n.t('flags_reminder.flags_were_submitted', { count: SiteSetting.notify_about_flags_after }) - ) + return unless flagged_posts_count > 0 - self.last_notified_id = flag_ids.max + flag_ids = pending_flag_ids + + if flag_ids.size > 0 && last_notified_id.to_i < flag_ids.max + + usernames = active_moderator_usernames + mentions = usernames.size > 0 ? "@#{usernames.join(', @')} " : "" + + PostCreator.create( + Discourse.system_user, + target_group_names: Group[:moderators].name, + archetype: Archetype.private_message, + subtype: TopicSubtype.system_message, + title: I18n.t('flags_reminder.subject_template', { count: flagged_posts_count }), + raw: mentions + I18n.t('flags_reminder.flags_were_submitted', { count: SiteSetting.notify_about_flags_after }) + ) + + self.last_notified_id = flag_ids.max + end end - - true end - def flag_ids - @_flag_ids ||= FlagQuery.flagged_post_actions('active') + def pending_flag_ids + FlagQuery.flagged_post_actions('active') .where('post_actions.created_at < ?', SiteSetting.notify_about_flags_after.to_i.hours.ago) .pluck(:id) end @@ -44,12 +49,11 @@ module Jobs end def self.last_notified_key - "last_notified_pending_flag_id" + "last_notified_pending_flag_id".freeze end def active_moderator_usernames - @_active_moderator_usernames ||= - User.where(moderator: true) + User.where(moderator: true) .human_users .order('last_seen_at DESC') .limit(3) From b23fc2bf84f6d40ccb47c9ec04610be3553357a0 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 22 May 2017 12:23:04 -0400 Subject: [PATCH 009/106] Helper to find the final destination for a URL --- app/jobs/regular/crawl_topic_link.rb | 49 +------ docs/TESTING.md | 36 ----- lib/final_destination.rb | 119 +++++++++++++++ spec/components/final_destination_spec.rb | 170 ++++++++++++++++++++++ 4 files changed, 293 insertions(+), 81 deletions(-) create mode 100644 lib/final_destination.rb create mode 100644 spec/components/final_destination_spec.rb diff --git a/app/jobs/regular/crawl_topic_link.rb b/app/jobs/regular/crawl_topic_link.rb index a5fb8907e0..0532108181 100644 --- a/app/jobs/regular/crawl_topic_link.rb +++ b/app/jobs/regular/crawl_topic_link.rb @@ -1,55 +1,13 @@ require 'open-uri' require 'nokogiri' require 'excon' +require 'final_destination' module Jobs class CrawlTopicLink < Jobs::Base class ReadEnough < StandardError; end - # Retrieve a header regardless of case sensitivity - def self.header_for(head, name) - header = head.headers.detect do |k, _| - name == k.downcase - end - header[1] if header - end - - def self.request_headers(uri) - { "User-Agent" => "Mozilla/5.0 (Windows NT 6.2; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1667.0 Safari/537.36", - "Accept" => "text/html", - "Host" => uri.host } - end - - # Follow any redirects that might exist - def self.final_uri(url, limit=5) - return if limit < 0 - - uri = URI(url) - return if uri.blank? || uri.host.blank? - return unless ['https', 'http'].include?(uri.scheme) - return unless [80, 443].include?(uri.port) - - headers = CrawlTopicLink.request_headers(uri) - head = Excon.head(url, read_timeout: 20, headers: headers) - - # If the site does not allow HEAD, just try the url - return uri if head.status == 405 - - if head.status == 200 - uri = nil unless header_for(head, 'content-type') =~ /text\/html/ - return uri - end - - location = header_for(head, 'location') - if location - location = "#{uri.scheme}://#{uri.host}#{location}" if location[0] == "/" - return final_uri(location, limit - 1) - end - - nil - end - def self.max_chunk_size(uri) # Amazon leaves the title until very late. Normally it's a bad idea to make an exception for # one host but amazon is a big one. @@ -64,7 +22,8 @@ module Jobs # Never crawl in test mode return if Rails.env.test? - uri = final_uri(url) + fd = FinalDestination.new(url) + uri = fd.resolve return "" unless uri result = "" @@ -76,7 +35,7 @@ module Jobs # that matter!) raise ReadEnough.new if result.size > (CrawlTopicLink.max_chunk_size(uri) * 1024) end - Excon.get(uri.to_s, response_block: streamer, read_timeout: 20, headers: CrawlTopicLink.request_headers(uri)) + Excon.get(uri.to_s, response_block: streamer, read_timeout: 20, headers: fd.request_headers) result rescue Excon::Errors::SocketError => ex diff --git a/docs/TESTING.md b/docs/TESTING.md index b2e97601f7..f62818d5c9 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -2,42 +2,6 @@ Some notes about testing Discourse: -## FakeWeb - -We use the [FakeWeb](https://github.com/chrisk/fakeweb) gem to fake external web -requests. -For example, check out the specs on `specs/components/oneboxer`. - -This has several advantages to making real requests: - -* We freeze the expected response from the remote server. -* We don't need a network connection to run the specs. -* It's faster. - -So, if you need to define a spec that makes a web request, you'll have to record -the real response to a fixture file, and tell FakeWeb to respond with it for the -URI of your request. - -Check out `spec/components/oneboxer/amazon_onebox_spec.rb` for an example on -this. - -### Recording responses - -To record the actual response from the remote server, you can use curl and save the response to a file. We use the `-i` option to include headers in the output - - curl -i http://en.m.wikipedia.org/wiki/Ruby > wikipedia.response - -If you need to specify the User-Agent to send to the server, you can use `-A`: - - curl -i -A 'Mozilla/5.0 (iPhone; CPU iPhone OS 5_0_1 like Mac OS X) AppleWebKit/534.46 (KHTML, like Gecko) Version/5.1 Mobile/9A405 Safari/7534.48.3' http://en.m.wikipedia.org/wiki/Ruby > wikipedia.response - -If the remote server is responding with a redirect, you'll need to fake both the -original request and the one for the destination. Check out the -`wikipedia.response` and `wikipedia_redirected.response` files in -`spec/fixtures/oneboxer` for an example. You can also consider working directly -with the final URL for simplicity. - - ## MailCatcher Discourse depends heavily on (sending) email for notifications. We use [MailCatcher](http://mailcatcher.me/) diff --git a/lib/final_destination.rb b/lib/final_destination.rb new file mode 100644 index 0000000000..23e76033a5 --- /dev/null +++ b/lib/final_destination.rb @@ -0,0 +1,119 @@ +require "socket" +require "ipaddr" +require 'excon' + +# Determine the final endpoint for a Web URI, following redirects +class FinalDestination + + attr_reader :status + + def initialize(url, opts = nil) + @uri = URI(url) rescue nil + @opts = opts || {} + @opts[:max_redirects] ||= 5 + @opts[:lookup_ip] ||= lambda do |host| + begin + IPSocket::getaddress(host) + rescue SocketError + nil + end + end + @limit = @opts[:max_redirects] + @status = :ready + end + + def redirected? + @limit < @opts[:max_redirects] + end + + def request_headers + { "User-Agent" => "Mozilla/5.0 (Windows NT 6.2; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36", + "Accept" => "text/html", + "Host" => @uri.hostname } + end + + def resolve + if @limit < 0 + @status = :too_many_redirects + return nil + end + + return nil unless validate_uri + headers = request_headers + head = Excon.head(@uri.to_s, read_timeout: 20, headers: headers) + + # If the site does not allow HEAD, just try the url + return @uri if head.status == 405 + + if head.status == 200 + @uri = nil unless FinalDestination.header_for(head, 'content-type') =~ /text\/html/ + @status = :resolved + return @uri + end + + location = FinalDestination.header_for(head, 'location') + if location + location = "#{@uri.scheme}://#{@uri.host}#{location}" if location[0] == "/" + @uri = URI(location) rescue nil + @limit -= 1 + return resolve + end + + nil + end + + def validate_uri + validate_uri_format && is_public? + end + + def validate_uri_format + return false unless @uri + return false unless ['https', 'http'].include?(@uri.scheme) + + if @uri.scheme == 'http' + return @uri.port == 80 + elsif @uri.scheme == 'https' + return @uri.port == 443 + end + + false + end + + def is_public? + return false unless @uri && @uri.host + + address_s = @opts[:lookup_ip].call(@uri.hostname) + return false unless address_s + + address = IPAddr.new(address_s) + private_match = FinalDestination.private_ranges.any? {|r| r === address } + + if private_match + @status = :invalid_address + return false + end + + true + end + + def self.private_ranges + @private_ranges ||= [ + IPAddr.new('127.0.0.1'), + IPAddr.new('172.16.0.0/12'), + IPAddr.new('192.168.0.0/16'), + IPAddr.new('10.0.0.0/8'), + IPAddr.new('fc00::/7') + ] + end + + def self.lookup_ip(host) + IPSocket::getaddress(host) + end + + def self.header_for(head, name) + header = head.headers.detect do |k, _| + name == k.downcase + end + header[1] if header + end +end diff --git a/spec/components/final_destination_spec.rb b/spec/components/final_destination_spec.rb new file mode 100644 index 0000000000..0b4aed46b1 --- /dev/null +++ b/spec/components/final_destination_spec.rb @@ -0,0 +1,170 @@ +require 'rails_helper' +require 'final_destination' + +describe FinalDestination do + + let(:opts) do + { # avoid IP lookups in test + lookup_ip: lambda do |host| + case host + when 'eviltrout.com' then '52.84.143.152' + when 'codinghorror.com' then '91.146.108.148' + when 'discourse.org' then '104.25.152.10' + when 'private-host.com' then '192.168.10.1' + else + host + end + end + } + end + + before do + FinalDestination.stubs(:lookup_ip) do |host| + end + end + + let(:doc_response) do + { body: "document", + headers: { "Content-Type" => "text/html" } } + end + + def redirect_response(from, dest) + stub_request(:head, from).to_return( + status: 302, + headers: { "Location" => dest } + ) + end + + describe '.resolve' do + + it "has a ready status code before anything happens" do + expect(FinalDestination.new('https://eviltrout.com').status).to eq(:ready) + end + + it "returns nil an invalid url" do + expect(FinalDestination.new(nil, opts).resolve).to be_nil + expect(FinalDestination.new('asdf', opts).resolve).to be_nil + end + + context "without redirects" do + before do + stub_request(:head, "https://eviltrout.com").to_return(doc_response) + end + + it "returns the final url" do + fd = FinalDestination.new('https://eviltrout.com', opts) + expect(fd.resolve.to_s).to eq('https://eviltrout.com') + expect(fd.redirected?).to eq(false) + expect(fd.status).to eq(:resolved) + end + end + + context "with a couple of redirects" do + before do + redirect_response("https://eviltrout.com", "https://codinghorror.com/blog") + redirect_response("https://codinghorror.com/blog", "https://discourse.org") + stub_request(:head, "https://discourse.org").to_return(doc_response) + end + + it "returns the final url" do + fd = FinalDestination.new('https://eviltrout.com', opts) + expect(fd.resolve.to_s).to eq('https://discourse.org') + expect(fd.redirected?).to eq(true) + expect(fd.status).to eq(:resolved) + end + end + + context "with too many redirects" do + before do + redirect_response("https://eviltrout.com", "https://codinghorror.com/blog") + redirect_response("https://codinghorror.com/blog", "https://discourse.org") + stub_request(:head, "https://discourse.org").to_return(doc_response) + end + + it "returns the final url" do + fd = FinalDestination.new('https://eviltrout.com', opts.merge(max_redirects: 1)) + expect(fd.resolve).to be_nil + expect(fd.redirected?).to eq(true) + expect(fd.status).to eq(:too_many_redirects) + end + end + + context "with a redirect to an internal IP" do + before do + redirect_response("https://eviltrout.com", "https://private-host.com") + stub_request(:head, "https://private-host.com").to_return(doc_response) + end + + it "returns the final url" do + fd = FinalDestination.new('https://eviltrout.com', opts) + expect(fd.resolve).to be_nil + expect(fd.redirected?).to eq(true) + expect(fd.status).to eq(:invalid_address) + end + end + end + + describe '.validate_uri' do + context "host lookups" do + it "works for various hosts" do + expect(FinalDestination.new('https://private-host.com', opts).validate_uri).to eq(false) + expect(FinalDestination.new('https://eviltrout.com:443', opts).validate_uri).to eq(true) + end + end + end + + describe ".validate_url_format" do + it "supports http urls" do + expect(FinalDestination.new('http://eviltrout.com', opts).validate_uri_format).to eq(true) + end + + it "supports https urls" do + expect(FinalDestination.new('https://eviltrout.com', opts).validate_uri_format).to eq(true) + end + + it "doesn't support ftp urls" do + expect(FinalDestination.new('ftp://eviltrout.com', opts).validate_uri_format).to eq(false) + end + + it "returns false for schemeless URL" do + expect(FinalDestination.new('eviltrout.com', opts).validate_uri_format).to eq(false) + end + + it "returns false for nil URL" do + expect(FinalDestination.new(nil, opts).validate_uri_format).to eq(false) + end + + it "returns false for invalid ports" do + expect(FinalDestination.new('http://eviltrout.com:21', opts).validate_uri_format).to eq(false) + expect(FinalDestination.new('https://eviltrout.com:8000', opts).validate_uri_format).to eq(false) + end + + it "returns true for valid ports" do + expect(FinalDestination.new('http://eviltrout.com:80', opts).validate_uri_format).to eq(true) + expect(FinalDestination.new('https://eviltrout.com:443',opts).validate_uri_format).to eq(true) + end + end + + describe ".is_public" do + it "returns false for a valid ipv4" do + expect(FinalDestination.new("https://52.84.143.67", opts).is_public?).to eq(true) + expect(FinalDestination.new("https://104.25.153.10", opts).is_public?).to eq(true) + end + + it "returns true for private ipv4" do + expect(FinalDestination.new("https://127.0.0.1", opts).is_public?).to eq(false) + expect(FinalDestination.new("https://192.168.1.3", opts).is_public?).to eq(false) + expect(FinalDestination.new("https://10.0.0.5", opts).is_public?).to eq(false) + expect(FinalDestination.new("https://172.16.0.1", opts).is_public?).to eq(false) + end + + it "returns true for public ipv6" do + expect(FinalDestination.new("https://[2001:470:1:3a8::251]", opts).is_public?).to eq(true) + end + + it "returns true for private ipv6" do + expect(FinalDestination.new("https://[fdd7:b450:d4d1:6b44::1]", opts).is_public?).to eq(false) + end + end + +end From 9edc490d3fa224d5cad8d858e8a6781d476fa76b Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Mon, 22 May 2017 16:26:18 -0400 Subject: [PATCH 010/106] FIX: remove memoized values in jobs --- app/jobs/onceoff/grant_emoji.rb | 2 +- app/jobs/onceoff/grant_first_reply_by_email.rb | 2 +- app/jobs/onceoff/grant_onebox.rb | 2 +- app/jobs/scheduled/pending_queued_posts_reminder.rb | 12 +++++++----- app/jobs/scheduled/poll_feed.rb | 2 +- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/app/jobs/onceoff/grant_emoji.rb b/app/jobs/onceoff/grant_emoji.rb index e99cdbb2e7..696fd1f952 100644 --- a/app/jobs/onceoff/grant_emoji.rb +++ b/app/jobs/onceoff/grant_emoji.rb @@ -26,7 +26,7 @@ module Jobs end def badge - @badge ||= Badge.find(Badge::FirstEmoji) + Badge.find(Badge::FirstEmoji) end end diff --git a/app/jobs/onceoff/grant_first_reply_by_email.rb b/app/jobs/onceoff/grant_first_reply_by_email.rb index a7d98cdfdb..ae769f681c 100644 --- a/app/jobs/onceoff/grant_first_reply_by_email.rb +++ b/app/jobs/onceoff/grant_first_reply_by_email.rb @@ -25,7 +25,7 @@ module Jobs end def badge - @badge ||= Badge.find(Badge::FirstReplyByEmail) + Badge.find(Badge::FirstReplyByEmail) end end diff --git a/app/jobs/onceoff/grant_onebox.rb b/app/jobs/onceoff/grant_onebox.rb index 6e4a92d69f..0c63e6b7b8 100644 --- a/app/jobs/onceoff/grant_onebox.rb +++ b/app/jobs/onceoff/grant_onebox.rb @@ -35,7 +35,7 @@ module Jobs end def badge - @badge ||= Badge.find(Badge::FirstOnebox) + Badge.find(Badge::FirstOnebox) end end diff --git a/app/jobs/scheduled/pending_queued_posts_reminder.rb b/app/jobs/scheduled/pending_queued_posts_reminder.rb index 7eb3e3b8e8..37b9afe6f4 100644 --- a/app/jobs/scheduled/pending_queued_posts_reminder.rb +++ b/app/jobs/scheduled/pending_queued_posts_reminder.rb @@ -6,17 +6,19 @@ module Jobs def execute(args) return true unless SiteSetting.notify_about_queued_posts_after > 0 && SiteSetting.contact_email - if should_notify_ids.size > 0 && last_notified_id.to_i < should_notify_ids.max - message = PendingQueuedPostsMailer.notify(count: should_notify_ids.size) + queued_post_ids = should_notify_ids + + if queued_post_ids.size > 0 && last_notified_id.to_i < queued_post_ids.max + message = PendingQueuedPostsMailer.notify(count: queued_post_ids.size) Email::Sender.new(message, :pending_queued_posts_reminder).send - self.last_notified_id = should_notify_ids.max + self.last_notified_id = queued_post_ids.max end true end def should_notify_ids - @_should_notify_ids ||= QueuedPost.new_posts.visible.where('created_at < ?', SiteSetting.notify_about_queued_posts_after.hours.ago).pluck(:id) + QueuedPost.new_posts.visible.where('created_at < ?', SiteSetting.notify_about_queued_posts_after.hours.ago).pluck(:id) end def last_notified_id @@ -28,7 +30,7 @@ module Jobs end def self.last_notified_key - "last_notified_queued_post_id" + "last_notified_queued_post_id".freeze end end end diff --git a/app/jobs/scheduled/poll_feed.rb b/app/jobs/scheduled/poll_feed.rb index c30f68cd2d..fcd0243aae 100644 --- a/app/jobs/scheduled/poll_feed.rb +++ b/app/jobs/scheduled/poll_feed.rb @@ -19,7 +19,7 @@ module Jobs end def feed_key - @feed_key ||= "feed-modified:#{Digest::SHA1.hexdigest(SiteSetting.feed_polling_url)}" + "feed-modified:#{Digest::SHA1.hexdigest(SiteSetting.feed_polling_url)}" end def poll_feed From 4c690f7089180311614f70e43952372c028a6dc4 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 22 May 2017 16:42:19 -0400 Subject: [PATCH 011/106] Use `FinalDestination` to ensure public redirects for onebox --- config/initializers/100-onebox_options.rb | 3 ++- lib/oneboxer.rb | 7 +++++-- spec/components/final_destination_spec.rb | 7 +++++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/config/initializers/100-onebox_options.rb b/config/initializers/100-onebox_options.rb index 81e4c93fde..9726a5aadc 100644 --- a/config/initializers/100-onebox_options.rb +++ b/config/initializers/100-onebox_options.rb @@ -1,5 +1,6 @@ require_dependency 'twitter_api' Onebox.options = { - twitter_client: TwitterApi + twitter_client: TwitterApi, + redirect_limit: 1 } diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index c117331f22..d90475c36b 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -1,4 +1,6 @@ -require_dependency "#{Rails.root}/lib/onebox/discourse_onebox_sanitize_config" +require_dependency "onebox/discourse_onebox_sanitize_config" +require_dependency 'final_destination' + Dir["#{Rails.root}/lib/onebox/engine/*_onebox.rb"].sort.each { |f| require f } module Oneboxer @@ -140,8 +142,9 @@ module Oneboxer end def self.onebox_raw(url) + Rails.cache.fetch(onebox_cache_key(url), expires_in: 1.day) do - uri = URI(url) rescue nil + uri = FinalDestination.new(url).resolve return blank_onebox if uri.blank? || SiteSetting.onebox_domains_blacklist.include?(uri.hostname) options = { cache: {}, max_width: 695, sanitize_config: Sanitize::Config::DISCOURSE_ONEBOX } r = Onebox.preview(url, options) diff --git a/spec/components/final_destination_spec.rb b/spec/components/final_destination_spec.rb index 0b4aed46b1..78c4473d3f 100644 --- a/spec/components/final_destination_spec.rb +++ b/spec/components/final_destination_spec.rb @@ -19,8 +19,11 @@ describe FinalDestination do end before do - FinalDestination.stubs(:lookup_ip) do |host| - end + WebMock.reset! + end + + after do + WebMock.reset! end let(:doc_response) do From b8d78b33c67d590bb09a4a2b1ca19546db511b39 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 22 May 2017 16:51:37 -0400 Subject: [PATCH 012/106] FIX: Other content types like images are fine --- lib/final_destination.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/final_destination.rb b/lib/final_destination.rb index 23e76033a5..2f68030eb3 100644 --- a/lib/final_destination.rb +++ b/lib/final_destination.rb @@ -46,7 +46,6 @@ class FinalDestination return @uri if head.status == 405 if head.status == 200 - @uri = nil unless FinalDestination.header_for(head, 'content-type') =~ /text\/html/ @status = :resolved return @uri end From a8d1e44943484103435da5a0ee5290632ecab4c2 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 22 May 2017 16:52:26 -0400 Subject: [PATCH 013/106] FIX: Onebox will do a HEAD request first for redirects --- spec/components/oneboxer_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/components/oneboxer_spec.rb b/spec/components/oneboxer_spec.rb index a01449f21e..6579e1f6e2 100644 --- a/spec/components/oneboxer_spec.rb +++ b/spec/components/oneboxer_spec.rb @@ -5,6 +5,7 @@ describe Oneboxer do it "returns blank string for an invalid onebox" do stub_request(:get, "http://boom.com").to_return(body: "") + stub_request(:head, "http://boom.com").to_return(body: "") expect(Oneboxer.preview("http://boom.com")).to eq("") expect(Oneboxer.onebox("http://boom.com")).to eq("") From d4b16b487e46ed28809fc14f9653585a13b66c9f Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 22 May 2017 17:00:19 -0400 Subject: [PATCH 014/106] FIX: Another onebox head request --- spec/components/email_cook_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/components/email_cook_spec.rb b/spec/components/email_cook_spec.rb index b0baf88087..48e38169e9 100644 --- a/spec/components/email_cook_spec.rb +++ b/spec/components/email_cook_spec.rb @@ -29,6 +29,7 @@ LONG_COOKED it 'autolinks' do stub_request(:get, "https://www.eviltrout.com").to_return(body: "") + stub_request(:head, "https://www.eviltrout.com").to_return(body: "") expect(EmailCook.new("https://www.eviltrout.com").cook).to eq("https://www.eviltrout.com
") end From 9dddb81cf688109908f56eb08c403e935035ea1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 22 May 2017 23:35:41 +0200 Subject: [PATCH 015/106] FIX: remove memoization on class method used in a job --- lib/email/receiver.rb | 25 ++++++++++------------ spec/components/email/receiver_spec.rb | 29 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index cc333ab41f..eba4409dd0 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -461,17 +461,16 @@ module Email true end - def self.reply_by_email_address_regex - @reply_by_email_address_regex ||= begin - reply_addresses = [ - SiteSetting.reply_by_email_address, - *(SiteSetting.alternative_reply_by_email_addresses.presence || "").split("|") - ] - escaped_reply_addresses = reply_addresses.select(&:present?) - .map { |a| Regexp.escape(a) } - .map { |a| a.gsub(Regexp.escape("%{reply_key}"), "(\\h{32})") } - Regexp.new(escaped_reply_addresses.join("|")) - end + def self.reply_by_email_address_regex(extract_reply_key=true) + reply_addresses = [SiteSetting.reply_by_email_address] + reply_addresses << (SiteSetting.alternative_reply_by_email_addresses.presence || "").split("|") + + reply_addresses.flatten! + reply_addresses.select!(&:present?) + reply_addresses.map! { |a| Regexp.escape(a) } + reply_addresses.map! { |a| a.gsub(Regexp.escape("%{reply_key}"), "(\\h{32})") } + + /#{reply_addresses.join("|")}/ end def group_incoming_emails_regex @@ -529,9 +528,7 @@ module Email end def post_action_for(body) - if likes.include?(body.strip.downcase) - PostActionType.types[:like] - end + PostActionType.types[:like] if likes.include?(body.strip.downcase) end def create_topic(options={}) diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index 1b6c2228b0..3242e55a41 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -526,4 +526,33 @@ describe Email::Receiver do end + context "#reply_by_email_address_regex" do + + before do + SiteSetting.reply_by_email_address = nil + SiteSetting.alternative_reply_by_email_addresses = nil + end + + it "is empty by default" do + expect(Email::Receiver.reply_by_email_address_regex).to eq(//) + end + + it "uses 'reply_by_email_address' site setting" do + SiteSetting.reply_by_email_address = "foo+%{reply_key}@bar.com" + expect(Email::Receiver.reply_by_email_address_regex).to eq(/foo\+(\h{32})@bar\.com/) + end + + it "uses 'alternative_reply_by_email_addresses' site setting" do + SiteSetting.alternative_reply_by_email_addresses = "alt.foo+%{reply_key}@bar.com" + expect(Email::Receiver.reply_by_email_address_regex).to eq(/alt\.foo\+(\h{32})@bar\.com/) + end + + it "combines both 'reply_by_email' settings" do + SiteSetting.reply_by_email_address = "foo+%{reply_key}@bar.com" + SiteSetting.alternative_reply_by_email_addresses = "alt.foo+%{reply_key}@bar.com" + expect(Email::Receiver.reply_by_email_address_regex).to eq(/foo\+(\h{32})@bar\.com|alt\.foo\+(\h{32})@bar\.com/) + end + + end + end From b51126dd5e3691d56144f1b69163d9f727611008 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 22 May 2017 17:52:13 -0400 Subject: [PATCH 016/106] FIX: Reset the WebMock after before every test --- spec/components/final_destination_spec.rb | 8 -------- spec/rails_helper.rb | 1 + 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/spec/components/final_destination_spec.rb b/spec/components/final_destination_spec.rb index 78c4473d3f..1e723e6cfa 100644 --- a/spec/components/final_destination_spec.rb +++ b/spec/components/final_destination_spec.rb @@ -18,14 +18,6 @@ describe FinalDestination do } end - before do - WebMock.reset! - end - - after do - WebMock.reset! - end - let(:doc_response) do { body: "document", headers: { "Content-Type" => "text/html" } } diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 359727768d..e6ed08fb57 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -124,6 +124,7 @@ Spork.prefork do Discourse.clear_readonly! I18n.locale = :en + WebMock.reset! end class TestCurrentUserProvider < Auth::DefaultCurrentUserProvider From 80af54460a81bf1593cacb1f0b23e0e0a42c8076 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 22 May 2017 18:19:20 -0400 Subject: [PATCH 017/106] FIX: Use Excon to do its own stubbing --- spec/components/final_destination_spec.rb | 23 ++++++++++------------- spec/rails_helper.rb | 5 +++++ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/spec/components/final_destination_spec.rb b/spec/components/final_destination_spec.rb index 1e723e6cfa..93fd9cb681 100644 --- a/spec/components/final_destination_spec.rb +++ b/spec/components/final_destination_spec.rb @@ -24,10 +24,7 @@ describe FinalDestination do end def redirect_response(from, dest) - stub_request(:head, from).to_return( - status: 302, - headers: { "Location" => dest } - ) + Excon.stub({ method: :head, hostname: from }, { status: 302, headers: { "Location" => dest } }) end describe '.resolve' do @@ -43,7 +40,7 @@ describe FinalDestination do context "without redirects" do before do - stub_request(:head, "https://eviltrout.com").to_return(doc_response) + Excon.stub({ method: :head, hostname: 'eviltrout.com' }, doc_response) end it "returns the final url" do @@ -56,9 +53,9 @@ describe FinalDestination do context "with a couple of redirects" do before do - redirect_response("https://eviltrout.com", "https://codinghorror.com/blog") - redirect_response("https://codinghorror.com/blog", "https://discourse.org") - stub_request(:head, "https://discourse.org").to_return(doc_response) + redirect_response("eviltrout.com", "https://codinghorror.com/blog") + redirect_response("codinghorror.com", "https://discourse.org") + Excon.stub({ method: :head, hostname: 'discourse.org' }, doc_response) end it "returns the final url" do @@ -71,9 +68,9 @@ describe FinalDestination do context "with too many redirects" do before do - redirect_response("https://eviltrout.com", "https://codinghorror.com/blog") - redirect_response("https://codinghorror.com/blog", "https://discourse.org") - stub_request(:head, "https://discourse.org").to_return(doc_response) + redirect_response("eviltrout.com", "https://codinghorror.com/blog") + redirect_response("codinghorror.com", "https://discourse.org") + Excon.stub({ method: :head, hostname: 'discourse.org' }, doc_response) end it "returns the final url" do @@ -86,8 +83,8 @@ describe FinalDestination do context "with a redirect to an internal IP" do before do - redirect_response("https://eviltrout.com", "https://private-host.com") - stub_request(:head, "https://private-host.com").to_return(doc_response) + redirect_response("eviltrout.com", "https://private-host.com") + Excon.stub({ method: :head, hostname: 'private-host.com' }, doc_response) end it "returns the final url" do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index e6ed08fb57..27c8b06800 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -124,7 +124,12 @@ Spork.prefork do Discourse.clear_readonly! I18n.locale = :en + Excon.defaults[:mock] = true + end + + config.after(:each) do WebMock.reset! + Excon.stubs.clear end class TestCurrentUserProvider < Auth::DefaultCurrentUserProvider From 35bb4ae9957bc86eabedfbaab144143ab3f5752d Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 23 May 2017 11:09:50 -0400 Subject: [PATCH 018/106] FIX: wizard was not showing up if more than 1 system user --- config/initializers/006-ensure_login_hint.rb | 2 +- lib/wizard.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/initializers/006-ensure_login_hint.rb b/config/initializers/006-ensure_login_hint.rb index fb0a526c7e..e177601737 100644 --- a/config/initializers/006-ensure_login_hint.rb +++ b/config/initializers/006-ensure_login_hint.rb @@ -1,5 +1,5 @@ # Some sanity checking so we don't count on an unindexed column on boot -if User.limit(20).count < 20 && User.where(admin: true).count == 1 +if User.limit(20).count < 20 && User.where(admin: true).human_users.count == 0 notice = if GlobalSetting.developer_emails.blank? "Congratulations, you installed Discourse! Unfortunately, no administrator emails were defined during setup, so finalizing the configuration may be difficult." diff --git a/lib/wizard.rb b/lib/wizard.rb index 298525ce57..e7a74f190c 100644 --- a/lib/wizard.rb +++ b/lib/wizard.rb @@ -87,7 +87,7 @@ class Wizard end first_admin_id = User.where(admin: true) - .where.not(id: Discourse.system_user.id) + .human_users .joins(:user_auth_tokens) .order('user_auth_tokens.created_at') .pluck(:id).first From c9028f517a63abed9381a3fc7b2afbef5cd6664a Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Tue, 23 May 2017 11:25:32 -0400 Subject: [PATCH 019/106] UX: headings in banners had no margins --- .../discourse/templates/components/discourse-banner.hbs | 2 +- app/assets/stylesheets/common/base/topic-post.scss | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/discourse/templates/components/discourse-banner.hbs b/app/assets/javascripts/discourse/templates/components/discourse-banner.hbs index 306f2924b4..b46760d5c1 100644 --- a/app/assets/javascripts/discourse/templates/components/discourse-banner.hbs +++ b/app/assets/javascripts/discourse/templates/components/discourse-banner.hbs @@ -1,8 +1,8 @@ {{#if visible}}