diff --git a/Gemfile b/Gemfile index eefd680..d86ba4a 100644 --- a/Gemfile +++ b/Gemfile @@ -1,8 +1,7 @@ -source 'https://rubygems.org' - -gem 'rails' +source "https://rubygems.org" group :development, :test do + gem "rails", "~> 6" gem "rspec-rails", "~> 3" gem "pry", "~> 0" end diff --git a/README.md b/README.md index 4190613..40603b3 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ You're all done. Try `.echo foo`, and you should see your client respond with `Echoing back to you: foo`. A hubot client implementation is available at - + ## Usage @@ -114,18 +114,19 @@ two environment variables to use this protocol: format. This environment variable will be the contents of a `.pub` file, newlines and all. -`CHATOPS_AUTH_BASE_URL` is the base URL of your server as the chatops client +`CHATOPS_AUTH_BASE_URL` is the base URLs of your servers as the chatops client sees it. This is specified as an environment variable since rails will trust client headers about a forwarded hostname. For example, if your chatops client has added the url `https://example.com/_chatops`, you'd set this to -`https://example.com`. +`https://example.com`. You can specify more than one base url divided by comma, +e.g. `https://example.com,https://example2.com` You can also optionally set `CHATOPS_AUTH_ALT_PUBLIC_KEY` to a second public key which will be accepted. This is helpful when rolling keys. ## Rails compatibility -This gem is intended to work with rails 4.x and 5.x. If you find a version +This gem is intended to work with rails 6.x and 7.x. If you find a version with a problem, please report it in an issue. ## Development diff --git a/chatops-controller.gemspec b/chatops-controller.gemspec index fda8ee0..ce5e50c 100644 --- a/chatops-controller.gemspec +++ b/chatops-controller.gemspec @@ -17,6 +17,7 @@ Gem::Specification.new do |s| s.files = Dir["{app,config,db,lib}/**/*", "README.md"] s.test_files = Dir["spec/**/*"] + s.add_dependency "rails" s.add_dependency "actionpack", ">= 6.0" s.add_dependency "activesupport", ">= 6.0" diff --git a/docs/why.md b/docs/why.md index 8875770..3624358 100644 --- a/docs/why.md +++ b/docs/why.md @@ -47,7 +47,7 @@ bridge, and we find this maps well to the asymmetric crypto authentication. ### Keyword Arguments -We pair this system with , which +We pair this system with , which provides generic argument support for long arguments, like `--argument foo`. While regexes create much more natural commands, rarely used options tend to create ugly regexes that are hard to test. If a command can potentially take 10 diff --git a/lib/chatops.rb b/lib/chatops.rb index 949bc77..5b67b6e 100644 --- a/lib/chatops.rb +++ b/lib/chatops.rb @@ -1,4 +1,18 @@ module Chatops + # THREAD_STYLES defines the various thread styles available to Hubot Chatops RPC. + # https://github.com/github/hubot-classic/blob/master/docs/rpc_chatops_protocol.md#executing-commands + THREAD_STYLES = { + # Channel thread style is a standard in-channel reply. + channel: 0, + # Threaded thread style will send the reply to a thread from the original message. + threaded: 1, + # Threaded and channel thread style will send the reply to a thread from the original message, + # and post an update into the channel as well (helpful when the original message in the thread is old). + threaded_and_channel: 2, + }.freeze + + ALLOWED_TIME_SKEW_MINS = 5 + def self.public_key ENV[public_key_env_var_name] end @@ -11,8 +25,8 @@ def self.alt_public_key ENV["CHATOPS_AUTH_ALT_PUBLIC_KEY"] end - def self.auth_base_url - ENV[auth_base_url_env_var_name] + def self.auth_base_urls + ENV.fetch(auth_base_url_env_var_name, "").split(",").map(&:strip) end def self.auth_base_url_env_var_name diff --git a/lib/chatops/controller.rb b/lib/chatops/controller.rb index 4af2ee1..e51a7ec 100644 --- a/lib/chatops/controller.rb +++ b/lib/chatops/controller.rb @@ -57,7 +57,7 @@ def setup_params! @jsonrpc_params = params.delete(:params) if params.has_key? :params - self.params = params.permit(:action, :chatop, :controller, :id, :mention_slug, :message_id, :method, :room_id, :user) + self.params = params.permit(:action, :chatop, :controller, :id, :mention_slug, :message_id, :method, :room_id, :user, :raw_command, :thread_id) end def jsonrpc_params @@ -118,26 +118,29 @@ def ensure_user_given end def ensure_chatops_authenticated - body = request.raw_post || "" - signature_string = [@chatops_url, @chatops_nonce, @chatops_timestamp, body].join("\n") - # We return this just to aid client debugging. - response.headers["Chatops-Signature-String"] = Base64.strict_encode64(signature_string) raise ConfigurationError.new("You need to add a client's public key in .pem format via #{Chatops.public_key_env_var_name}") unless Chatops.public_key.present? - if signature_valid?(Chatops.public_key, @chatops_signature, signature_string) || - signature_valid?(Chatops.alt_public_key, @chatops_signature, signature_string) + + body = request.raw_post || "" + + @chatops_urls.each do |url| + signature_string = [url, @chatops_nonce, @chatops_timestamp, body].join("\n") + # We return this just to aid client debugging. + response.headers["Chatops-Signature-String"] = Base64.strict_encode64(signature_string) + if signature_valid?(Chatops.public_key, @chatops_signature, signature_string) || + signature_valid?(Chatops.alt_public_key, @chatops_signature, signature_string) return true + end end + return jsonrpc_error(-32800, 403, "Not authorized") end def ensure_valid_chatops_url - unless Chatops.auth_base_url.present? + unless Chatops.auth_base_urls.present? raise ConfigurationError.new("You need to set the server's base URL to authenticate chatops RPC via #{Chatops.auth_base_url_env_var_name}") end - if Chatops.auth_base_url[-1] == "/" - raise ConfigurationError.new("Don't include a trailing slash in #{Chatops.auth_base_url_env_var_name}; the rails path will be appended and it must match exactly.") - end - @chatops_url = Chatops.auth_base_url + request.path + + @chatops_urls = Chatops.auth_base_urls.map { |url| url.chomp("/") + request.path } end def ensure_valid_chatops_nonce @@ -165,8 +168,8 @@ def ensure_valid_chatops_signature def ensure_valid_chatops_timestamp @chatops_timestamp = request.headers["Chatops-Timestamp"] time = Time.iso8601(@chatops_timestamp) - if !(time > 1.minute.ago && time < 1.minute.from_now) - return jsonrpc_error(-32803, 403, "Chatops timestamp not within 1 minute of server time: #{@chatops_timestamp} vs #{Time.now.utc.iso8601}") + if !(time > Chatops::ALLOWED_TIME_SKEW_MINS.minute.ago && time < Chatops::ALLOWED_TIME_SKEW_MINS.minute.from_now) + return jsonrpc_error(-32803, 403, "Chatops timestamp not within #{Chatops::ALLOWED_TIME_SKEW_MINS} minutes of server time: #{@chatops_timestamp} vs #{Time.now.utc.iso8601}") end rescue ArgumentError, TypeError # time parsing or missing can raise these diff --git a/lib/chatops/controller/test_case_helpers.rb b/lib/chatops/controller/test_case_helpers.rb index f6c4313..34d4933 100644 --- a/lib/chatops/controller/test_case_helpers.rb +++ b/lib/chatops/controller/test_case_helpers.rb @@ -42,6 +42,7 @@ def chatop(method, params = {}) def chat(message, user, room_id = "123", message_id = "456") get :list json_response = JSON.load(response.body) + raise "Invalid Chatop response - BODY: #{json_response}" unless json_response.key?("methods") matchers = json_response["methods"].map { |name, metadata| metadata = metadata.dup metadata["name"] = name @@ -75,6 +76,7 @@ def chatop_response def chatop_error json_response = JSON.load(response.body) + raise "There is no chatop error - BODY: #{json_response}" unless json_response.key?("error") json_response["error"]["message"] end diff --git a/lib/chatops/controller/version.rb b/lib/chatops/controller/version.rb index e990348..f27c53f 100644 --- a/lib/chatops/controller/version.rb +++ b/lib/chatops/controller/version.rb @@ -1,3 +1,3 @@ module ChatopsController - VERSION = "4.1.0" + VERSION = "5.4.0" end diff --git a/spec/lib/chatops/controller_spec.rb b/spec/lib/chatops/controller_spec.rb index 6225af3..e88cf5f 100644 --- a/spec/lib/chatops/controller_spec.rb +++ b/spec/lib/chatops/controller_spec.rb @@ -56,7 +56,7 @@ def ensure_app_given @private_key = OpenSSL::PKey::RSA.new(2048) ENV["CHATOPS_AUTH_PUBLIC_KEY"] = @private_key.public_key.to_pem - ENV["CHATOPS_AUTH_BASE_URL"] = "http://test.host" + ENV["CHATOPS_AUTH_BASE_URL"] = "http://old.host,http://test.host/" end def rails_flexible_post(path, outer_params, jsonrpc_params = nil) @@ -177,9 +177,9 @@ def rails_flexible_post(path, outer_params, jsonrpc_params = nil) expect(response.status).to eq 403 end - it "doesn't allow requests more than 1 minute old" do + it "doesn't allow requests more than 5 minute old" do nonce = SecureRandom.hex(20) - timestamp = 2.minutes.ago.utc.iso8601 + timestamp = 6.minutes.ago.utc.iso8601 request.headers["Chatops-Nonce"] = nonce request.headers["Chatops-Timestamp"] = timestamp digest = OpenSSL::Digest::SHA256.new @@ -188,12 +188,12 @@ def rails_flexible_post(path, outer_params, jsonrpc_params = nil) request.headers["Chatops-Signature"] = "Signature keyid=foo,signature=#{signature}" get :list expect(response.status).to eq 403 - expect(response.body).to include "Chatops timestamp not within 1 minute" + expect(response.body).to include "Chatops timestamp not within 5 minutes" end - it "doesn't allow requests more than 1 minute in the future" do + it "doesn't allow requests more than 5 minute in the future" do nonce = SecureRandom.hex(20) - timestamp = 2.minutes.from_now.utc.iso8601 + timestamp = 6.minutes.from_now.utc.iso8601 request.headers["Chatops-Nonce"] = nonce request.headers["Chatops-Timestamp"] = timestamp digest = OpenSSL::Digest::SHA256.new @@ -202,7 +202,7 @@ def rails_flexible_post(path, outer_params, jsonrpc_params = nil) request.headers["Chatops-Signature"] = "Signature keyid=foo,signature=#{signature}" get :list expect(response.status).to eq 403 - expect(response.body).to include "Chatops timestamp not within 1 minute" + expect(response.body).to include "Chatops timestamp not within 5 minutes" end it "does not add authentication to non-chatops routes" do @@ -315,7 +315,7 @@ def rails_flexible_post(path, outer_params, jsonrpc_params = nil) :room_id => "#someroom", :unknown_key => "few" # This should get ignored }, { - "app" => "foo" + "app" => "foo" } expect(json_response).to eq({ "jsonrpc" => "2.0", @@ -323,7 +323,7 @@ def rails_flexible_post(path, outer_params, jsonrpc_params = nil) "result" => "{\"params\":{\"action\":\"proxy_parameters\",\"chatop\":\"proxy_parameters\",\"controller\":\"anonymous\",\"mention_slug\":\"mention_slug_here\",\"message_id\":\"message_id_here\",\"room_id\":\"#someroom\",\"user\":\"foo\"},\"jsonrpc_params\":{\"app\":\"foo\"}}" }) expect(response.status).to eq 200 - end + end it "uses typical controller fun like before_action" do