From acb91c79d2059562c24539f9cd7d255591582a75 Mon Sep 17 00:00:00 2001 From: Benjamin Neff Date: Wed, 27 Jan 2016 03:19:32 +0100 Subject: [PATCH] improve pod connection check * use port for check * respect entries in /etc/hosts * test /.well-known/host-meta * don't allow redirects to other domains --- .../javascripts/app/pages/admin_pods.js | 5 + .../javascripts/app/views/pod_entry_view.js | 3 +- app/assets/stylesheets/admin.scss | 6 + .../templates/pod_table_entry_tpl.jst.hbs | 2 +- app/controllers/admin/pods_controller.rb | 1 + app/models/pod.rb | 11 +- app/presenters/pod_presenter.rb | 1 + config/locales/javascript/javascript.en.yml | 3 + lib/connection_tester.rb | 41 +++--- .../controllers/admin/pods_controller_spec.rb | 6 +- spec/lib/connection_tester_spec.rb | 124 ++++++++++-------- spec/models/pod_spec.rb | 45 ++++++- 12 files changed, 148 insertions(+), 100 deletions(-) diff --git a/app/assets/javascripts/app/pages/admin_pods.js b/app/assets/javascripts/app/pages/admin_pods.js index 4c4b14dd7..a4880354e 100644 --- a/app/assets/javascripts/app/pages/admin_pods.js +++ b/app/assets/javascripts/app/pages/admin_pods.js @@ -34,6 +34,11 @@ app.pages.AdminPods = app.views.Base.extend({ .append(Diaspora.I18n.t("admin.pods.unchecked", {count: gon.uncheckedCount})); msgs.appendChild(unchecked[0]); } + if( gon.versionFailedCount && gon.versionFailedCount > 0 ) { + var versionFailed = $("
") + .append(Diaspora.I18n.t("admin.pods.version_failed", {count: gon.versionFailedCount})); + msgs.appendChild(versionFailed[0]); + } if( gon.errorCount && gon.errorCount > 0 ) { var errors = $("
") .append(Diaspora.I18n.t("admin.pods.errors", {count: gon.errorCount})); diff --git a/app/assets/javascripts/app/views/pod_entry_view.js b/app/assets/javascripts/app/views/pod_entry_view.js index c8c402fb8..112b6708f 100644 --- a/app/assets/javascripts/app/views/pod_entry_view.js +++ b/app/assets/javascripts/app/views/pod_entry_view.js @@ -31,7 +31,8 @@ app.views.PodEntry = app.views.Base.extend({ has_no_errors: (this.model.get("status")==="no_errors"), has_errors: (this.model.get("status")!=="no_errors"), status_text: Diaspora.I18n.t("admin.pods.states."+this.model.get("status")), - pod_url: (this.model.get("ssl") ? "https" : "http") + "://" + this.model.get("host"), + pod_url: (this.model.get("ssl") ? "https" : "http") + "://" + this.model.get("host") + + (this.model.get("port") ? ":" + this.model.get("port") : ""), response_time_fmt: this._fmtResponseTime() /* jshint camelcase: true */ }); diff --git a/app/assets/stylesheets/admin.scss b/app/assets/stylesheets/admin.scss index 1859d1c06..ea933e6e6 100644 --- a/app/assets/stylesheets/admin.scss +++ b/app/assets/stylesheets/admin.scss @@ -39,6 +39,12 @@ /** pod list **/ #pod-list { + .pod-title { + max-width: 200px; + overflow: hidden; + text-overflow: ellipsis; + } + th.added, td.added, td.actions { white-space: nowrap; } diff --git a/app/assets/templates/pod_table_entry_tpl.jst.hbs b/app/assets/templates/pod_table_entry_tpl.jst.hbs index 9daa5b331..e43ee6c6f 100644 --- a/app/assets/templates/pod_table_entry_tpl.jst.hbs +++ b/app/assets/templates/pod_table_entry_tpl.jst.hbs @@ -7,7 +7,7 @@ {{/if}} -{{host}} +{{host}} diff --git a/app/controllers/admin/pods_controller.rb b/app/controllers/admin/pods_controller.rb index 3f81dd582..1b4d2bf2f 100644 --- a/app/controllers/admin/pods_controller.rb +++ b/app/controllers/admin/pods_controller.rb @@ -10,6 +10,7 @@ module Admin format.html do gon.preloads[:pods] = pods_json gon.unchecked_count = Pod.unchecked.count + gon.version_failed_count = Pod.version_failed.count gon.error_count = Pod.check_failed.count render "admins/pods" diff --git a/app/models/pod.rb b/app/models/pod.rb index f3d65ccf8..d957d4ffb 100644 --- a/app/models/pod.rb +++ b/app/models/pod.rb @@ -24,7 +24,7 @@ class Pod < ActiveRecord::Base has_many :people scope :check_failed, lambda { - where(arel_table[:status].gt(Pod.statuses[:no_errors])) + where(arel_table[:status].gt(Pod.statuses[:no_errors])).where.not(status: Pod.statuses[:version_failed]) } class << self @@ -32,10 +32,8 @@ class Pod < ActiveRecord::Base uri = URI.parse(opts.fetch(:url)) port = DEFAULT_PORTS.include?(uri.port) ? nil : uri.port find_or_initialize_by(host: uri.host, port: port).tap do |pod| - unless pod.persisted? - pod.ssl = (uri.scheme == "https") - pod.save - end + pod.ssl ||= (uri.scheme == "https") + pod.save end end @@ -63,7 +61,7 @@ class Pod < ActiveRecord::Base def test_connection! result = ConnectionTester.check uri.to_s - logger.info "testing pod: '#{uri}' - #{result.inspect}" + logger.debug "tested pod: '#{uri}' - #{result.inspect}" transaction do update_from_result(result) @@ -95,6 +93,7 @@ class Pod < ActiveRecord::Base end def attributes_from_result(result) + self.ssl ||= result.ssl self.error = result.failure_message[0..254] if result.error? self.software = result.software_version[0..254] if result.software_version.present? self.response_time = result.rt diff --git a/app/presenters/pod_presenter.rb b/app/presenters/pod_presenter.rb index c59823c39..7f08b3770 100644 --- a/app/presenters/pod_presenter.rb +++ b/app/presenters/pod_presenter.rb @@ -3,6 +3,7 @@ class PodPresenter < BasePresenter { id: id, host: host, + port: port, ssl: ssl, status: status, checked_at: checked_at, diff --git a/config/locales/javascript/javascript.en.yml b/config/locales/javascript/javascript.en.yml index 17ae35f06..b9b5dc3d4 100644 --- a/config/locales/javascript/javascript.en.yml +++ b/config/locales/javascript/javascript.en.yml @@ -72,6 +72,9 @@ en: unchecked: one: "There is still one pod that hasn't been checked at all." other: "There are still <%= count %> pods that haven't been checked at all." + version_failed: + one: "There is one pod that has no version (old pod, no NodeInfo)." + other: "There are <%= count %> pods that have no version (old pods, no NodeInfo)." errors: one: "The connection test returned an error for one pod." other: "The connection test returned an error for <%= count %> pods." diff --git a/lib/connection_tester.rb b/lib/connection_tester.rb index d7656261f..0783b3a93 100644 --- a/lib/connection_tester.rb +++ b/lib/connection_tester.rb @@ -53,7 +53,7 @@ class ConnectionTester result.reachable = false when SSLFailure result.reachable = true - result.ssl_status = false + result.ssl = false when HTTPFailure result.reachable = true when NodeInfoFailure @@ -80,11 +80,8 @@ class ConnectionTester # Perform the DNS query, the IP address will be stored in the result # @raise [DNSFailure] caused by a failure to resolve or a timeout def resolve - with_dns_resolver do |dns| - addr = dns.getaddress(@uri.host) - @result.ip = addr.to_s - end - rescue Resolv::ResolvError, Resolv::ResolvTimeout => e + @result.ip = IPSocket.getaddress(@uri.host) + rescue SocketError => e raise DNSFailure, "'#{@uri.host}' - #{e.message}" rescue StandardError => e raise Failure, e.inspect @@ -96,13 +93,15 @@ class ConnectionTester # * is the SSL certificate valid (only on HTTPS) # * does the server return a successful HTTP status code # * is there a reasonable amount of redirects (3 by default) + # * is there a /.well-known/host-meta (this is needed to work, this can be replaced with a mandatory NodeInfo later) # (can't do a HEAD request, since that's not a defined route in the app) # # @raise [NetFailure, SSLFailure, HTTPFailure] if any of the checks fail # @return [Integer] HTTP status code def request with_http_connection do |http| - response = capture_response_time { http.get("/") } + capture_response_time { http.get("/") } + response = http.get("/.well-known/host-meta") handle_http_response(response) end rescue HTTPFailure => e @@ -129,7 +128,7 @@ class ConnectionTester find_software_version(nd_resp.body) end rescue Faraday::ResourceNotFound, JSON::JSONError => e - raise NodeInfoFailure, e.message[0..255] + raise NodeInfoFailure, e.message[0..255].encode(Encoding.default_external, undef: :replace) rescue StandardError => e raise Failure, e.inspect end @@ -150,39 +149,29 @@ class ConnectionTester yield(@http) if block_given? end - def with_dns_resolver - dns = Resolv::DNS.new - yield(dns) if block_given? - ensure - dns.close - end - def http_uri?(uri) uri.is_a?(URI::HTTP) || uri.is_a?(URI::HTTPS) end - def uses_ssl? - @uses_ssl - end - # request root path, measure response time # measured time may be skewed, if there are redirects # # @return [Faraday::Response] def capture_response_time - start = Time.zone.now + start = Time.zone.now resp = yield if block_given? @result.rt = ((Time.zone.now - start) * 1000.0).to_i # milliseconds resp end def handle_http_response(response) - @uses_ssl = (response.env.url.scheme == "https") @result.status_code = Integer(response.status) if response.success? - @result.reachable = true - @result.ssl_status = @uses_ssl + raise HTTPFailure, "redirected to other hostname: #{response.env.url}" unless @uri.host == response.env.url.host + + @result.reachable = true + @result.ssl = (response.env.url.scheme == "https") else raise HTTPFailure, "unsuccessful response code: #{response.status}" end @@ -225,7 +214,7 @@ class ConnectionTester end Result = Struct.new( - :ip, :reachable, :ssl_status, :status_code, :rt, :software_version, :error + :ip, :reachable, :ssl, :status_code, :rt, :software_version, :error ) do # @!attribute ip # @return [String] resolved IP address from DNS query @@ -233,8 +222,8 @@ class ConnectionTester # @!attribute reachable # @return [Boolean] whether the host was reachable over the network - # @!attribute ssl_status - # @return [Boolean] indicating how the SSL verification went + # @!attribute ssl + # @return [Boolean] whether the host has working ssl # @!attribute status_code # @return [Integer] HTTP status code that was returned for the HEAD request diff --git a/spec/controllers/admin/pods_controller_spec.rb b/spec/controllers/admin/pods_controller_spec.rb index b5fae1f3c..b5741b1ba 100644 --- a/spec/controllers/admin/pods_controller_spec.rb +++ b/spec/controllers/admin/pods_controller_spec.rb @@ -25,11 +25,11 @@ describe Admin::PodsController, type: :controller do end it "returns the json data" do - pods = (0..2).map { FactoryGirl.create(:pod).reload } # normalize timestamps - pods.unshift remote_raphael.pod + 3.times { FactoryGirl.create(:pod) } + get :index, format: :json - expect(response.body).to eql(PodPresenter.as_collection(pods).to_json) + expect(response.body).to eql(PodPresenter.as_collection(Pod.all).to_json) end end diff --git a/spec/lib/connection_tester_spec.rb b/spec/lib/connection_tester_spec.rb index 737003a80..f6cce045b 100644 --- a/spec/lib/connection_tester_spec.rb +++ b/spec/lib/connection_tester_spec.rb @@ -2,6 +2,10 @@ require "spec_helper" describe ConnectionTester do + let(:url) { "https://pod.example.com" } + let(:result) { ConnectionTester::Result.new } + let(:tester) { ConnectionTester.new(url, result) } + describe "::check" do it "takes a http url and returns a result object" do res = ConnectionTester.check("https://pod.example.com") @@ -29,102 +33,108 @@ describe ConnectionTester do end describe "#resolve" do - before do - @result = ConnectionTester::Result.new - @dns = instance_double("Resolv::DNS") - allow(@dns).to receive(:close).once - end - it "resolves the IP address" do - tester = ConnectionTester.new("https://pod.example.com", @result) - expect(tester).to receive(:with_dns_resolver).and_yield(@dns) - expect(@dns).to receive(:getaddress).and_return("192.168.1.2") + expect(IPSocket).to receive(:getaddress).with("pod.example.com").and_return("192.168.1.2") tester.resolve - expect(@result.ip).to eq("192.168.1.2") + expect(result.ip).to eq("192.168.1.2") + end + + it "raises DNSFailure if host is unknown" do + expect(IPSocket).to receive(:getaddress).with("pod.example.com").and_raise(SocketError.new("Error!")) + + expect { tester.resolve }.to raise_error(ConnectionTester::DNSFailure, "'pod.example.com' - Error!") end end describe "#request" do - before do - @url = "https://pod.example.com" - @stub = - @result = ConnectionTester::Result.new - @tester = ConnectionTester.new(@url, @result) - end + it "performs a successful GET request on '/' and '/.well-known/host-meta'" do + stub_request(:get, url).to_return(status: 200, body: "Hello World!") + stub_request(:get, "#{url}/.well-known/host-meta").to_return(status: 200, body: "host-meta") - it "performs a successful GET request on '/'" do - stub_request(:get, @url).to_return(status: 200, body: "Hello World!") - - @tester.request - expect(@result.rt).to be > -1 - expect(@result.reachable).to be_truthy - expect(@result.ssl_status).to be_truthy + tester.request + expect(result.rt).to be > -1 + expect(result.reachable).to be_truthy + expect(result.ssl).to be_truthy end it "receives a 'normal' 301 redirect" do - stub_request(:get, @url).to_return(status: 301, headers: {"Location" => "#{@url}/redirect"}) - stub_request(:get, "#{@url}/redirect").to_return(status: 200, body: "Hello World!") + stub_request(:get, url).to_return(status: 301, headers: {"Location" => "#{url}/redirect"}) + stub_request(:get, "#{url}/redirect").to_return(status: 200, body: "Hello World!") + stub_request(:get, "#{url}/.well-known/host-meta").to_return(status: 200, body: "host-meta") - @tester.request + tester.request + end + + it "updates ssl after https redirect" do + tester = ConnectionTester.new("http://pod.example.com/", result) + stub_request(:get, "http://pod.example.com/").to_return(status: 200, body: "Hello World!") + stub_request(:get, "http://pod.example.com/.well-known/host-meta") + .to_return(status: 301, headers: {"Location" => "#{url}/.well-known/host-meta"}) + stub_request(:get, "#{url}/.well-known/host-meta").to_return(status: 200, body: "host-meta") + + tester.request + expect(result.ssl).to be_truthy + end + + it "rejects other hostname after redirect redirect" do + stub_request(:get, url).to_return(status: 200, body: "Hello World!") + stub_request(:get, "#{url}/.well-known/host-meta") + .to_return(status: 301, headers: {"Location" => "https://example.com/.well-known/host-meta"}) + stub_request(:get, "https://example.com/.well-known/host-meta").to_return(status: 200, body: "host-meta") + + expect { tester.request }.to raise_error(ConnectionTester::HTTPFailure) end it "receives too many 301 redirects" do - stub_request(:get, @url).to_return(status: 301, headers: {"Location" => "#{@url}/redirect"}) - stub_request(:get, "#{@url}/redirect").to_return(status: 301, headers: {"Location" => "#{@url}/redirect1"}) - stub_request(:get, "#{@url}/redirect1").to_return(status: 301, headers: {"Location" => "#{@url}/redirect2"}) - stub_request(:get, "#{@url}/redirect2").to_return(status: 301, headers: {"Location" => "#{@url}/redirect3"}) - stub_request(:get, "#{@url}/redirect3").to_return(status: 200, body: "Hello World!") + stub_request(:get, url).to_return(status: 301, headers: {"Location" => "#{url}/redirect"}) + stub_request(:get, "#{url}/redirect").to_return(status: 301, headers: {"Location" => "#{url}/redirect1"}) + stub_request(:get, "#{url}/redirect1").to_return(status: 301, headers: {"Location" => "#{url}/redirect2"}) + stub_request(:get, "#{url}/redirect2").to_return(status: 301, headers: {"Location" => "#{url}/redirect3"}) + stub_request(:get, "#{url}/redirect3").to_return(status: 200, body: "Hello World!") - expect { @tester.request }.to raise_error(ConnectionTester::HTTPFailure) + expect { tester.request }.to raise_error(ConnectionTester::HTTPFailure) end it "receives a 404 not found" do - stub_request(:get, @url).to_return(status: 404, body: "Not Found!") - expect { @tester.request }.to raise_error(ConnectionTester::HTTPFailure) + stub_request(:get, url).to_return(status: 404, body: "Not Found!") + expect { tester.request }.to raise_error(ConnectionTester::HTTPFailure) end it "cannot connect" do - stub_request(:get, @url).to_raise(Faraday::ConnectionFailed.new("Error!")) - expect { @tester.request }.to raise_error(ConnectionTester::NetFailure) + stub_request(:get, url).to_raise(Faraday::ConnectionFailed.new("Error!")) + expect { tester.request }.to raise_error(ConnectionTester::NetFailure) end it "encounters an invalid SSL setup" do - stub_request(:get, @url).to_raise(Faraday::SSLError.new("Error!")) - expect { @tester.request }.to raise_error(ConnectionTester::SSLFailure) + stub_request(:get, url).to_raise(Faraday::SSLError.new("Error!")) + expect { tester.request }.to raise_error(ConnectionTester::SSLFailure) end end describe "#nodeinfo" do - before do - @url = "https://diaspora.example.com" - @result = ConnectionTester::Result.new - @tester = ConnectionTester.new(@url, @result) - - @ni_wellknown = {links: [{rel: ConnectionTester::NODEINFO_SCHEMA, - href: "/nodeinfo"}]} - @ni_document = {software: {name: "diaspora", version: "a.b.c.d"}} - end - it "reads the version from the nodeinfo document" do - stub_request(:get, "#{@url}#{ConnectionTester::NODEINFO_FRAGMENT}") - .to_return(status: 200, body: JSON.generate(@ni_wellknown)) - stub_request(:get, "#{@url}/nodeinfo").to_return(status: 200, body: JSON.generate(@ni_document)) + ni_wellknown = {links: [{rel: ConnectionTester::NODEINFO_SCHEMA, href: "/nodeinfo"}]} + ni_document = {software: {name: "diaspora", version: "a.b.c.d"}} - @tester.nodeinfo - expect(@result.software_version).to eq("diaspora a.b.c.d") + stub_request(:get, "#{url}#{ConnectionTester::NODEINFO_FRAGMENT}") + .to_return(status: 200, body: JSON.generate(ni_wellknown)) + stub_request(:get, "#{url}/nodeinfo").to_return(status: 200, body: JSON.generate(ni_document)) + + tester.nodeinfo + expect(result.software_version).to eq("diaspora a.b.c.d") end it "handles a missing nodeinfo document gracefully" do - stub_request(:get, "#{@url}#{ConnectionTester::NODEINFO_FRAGMENT}") + stub_request(:get, "#{url}#{ConnectionTester::NODEINFO_FRAGMENT}") .to_return(status: 404, body: "Not Found") - expect { @tester.nodeinfo }.to raise_error(ConnectionTester::NodeInfoFailure) + expect { tester.nodeinfo }.to raise_error(ConnectionTester::NodeInfoFailure) end it "handles a malformed document gracefully" do - stub_request(:get, "#{@url}#{ConnectionTester::NODEINFO_FRAGMENT}") + stub_request(:get, "#{url}#{ConnectionTester::NODEINFO_FRAGMENT}") .to_return(status: 200, body: '{"json"::::"malformed"}') - expect { @tester.nodeinfo }.to raise_error(ConnectionTester::NodeInfoFailure) + expect { tester.nodeinfo }.to raise_error(ConnectionTester::NodeInfoFailure) end end end diff --git a/spec/models/pod_spec.rb b/spec/models/pod_spec.rb index a1e851d26..f612a6152 100644 --- a/spec/models/pod_spec.rb +++ b/spec/models/pod_spec.rb @@ -1,19 +1,45 @@ require "spec_helper" describe Pod, type: :model do - describe "::find_or_create_by" do + describe ".find_or_create_by" do it "takes a url, and makes one by host" do - pod = Pod.find_or_create_by(url: "https://joindiaspora.com/maxwell") - expect(pod.host).to eq("joindiaspora.com") + pod = Pod.find_or_create_by(url: "https://example.org/u/maxwell") + expect(pod.host).to eq("example.org") end - it "sets ssl boolean (side-effect)" do - pod = Pod.find_or_create_by(url: "https://joindiaspora.com/maxwell") + it "saves the port" do + pod = Pod.find_or_create_by(url: "https://example.org:3000/") + expect(pod.host).to eq("example.org") + expect(pod.port).to eq(3000) + end + + it "ignores default ports" do + pod = Pod.find_or_create_by(url: "https://example.org:443/") + expect(pod.host).to eq("example.org") + expect(pod.port).to be_nil + end + + it "sets ssl boolean" do + pod = Pod.find_or_create_by(url: "https://example.org/") + expect(pod.ssl).to be true + end + + it "updates ssl boolean if upgraded to https" do + pod = Pod.find_or_create_by(url: "http://joindiaspora.com/") + expect(pod.ssl).to be false + pod = Pod.find_or_create_by(url: "https://joindiaspora.com/") + expect(pod.ssl).to be true + end + + it "does not update ssl boolean if downgraded to http" do + pod = Pod.find_or_create_by(url: "https://joindiaspora.com/") + expect(pod.ssl).to be true + pod = Pod.find_or_create_by(url: "http://joindiaspora.com/") expect(pod.ssl).to be true end end - describe "::check_all!" do + describe ".check_all!" do before do @pods = (0..4).map do double("pod").tap do |pod| @@ -75,4 +101,11 @@ describe Pod, type: :model do end end end + + describe "#url_to" do + it "appends the path to the pod-url" do + pod = FactoryGirl.create(:pod) + expect(pod.url_to("/receive/public")).to eq("https://#{pod.host}/receive/public") + end + end end