fix(github_hooks): improve GitHub App webhook handling (#82)

Author: lucaspin

github_hooks/app/controllers/projects_controller.rb

@@ -20,6 +20,16 @@ def repo_host_post_commit_hook
         head :ok and return
       end
 
+      if webhook_filter.github_app_webhook? && App.check_github_app_webhook?
+        signature = repo_host_request.headers["X-Hub-Signature-256"]
+        secret = Semaphore::GithubApp::Credentials.github_app_webhook_secret
+
+        if Semaphore::GithubApp::Hook.webhook_signature_valid?(secret, signature, repo_host_request.body.string) != :ok
+          logger.error("Webhook validation failed")
+          head :not_found and return
+        end
+      end
+
       if webhook_filter.github_app_installation_webhook?
         Watchman.increment("repo_host_post_commit_hooks.controller.github_app_webhook")
         logger.info("Github App Webhook")

github_hooks/config/app/development.rb

@@ -11,6 +11,8 @@
 
   config.github_app_id = SemaphoreConfig.github_app_id
   config.github_secret_id = SemaphoreConfig.github_secret_id
+  config.github_app_webhook_secret = SemaphoreConfig.github_app_webhook_secret
+  config.check_github_app_webhook = SemaphoreConfig.check_github_app_webhook
 
   config.bitbucket_app_id = SemaphoreConfig.bitbucket_app_id
   config.bitbucket_secret_id = SemaphoreConfig.bitbucket_secret_id

github_hooks/config/app/production.rb

@@ -13,6 +13,8 @@
 
   config.github_app_id = SemaphoreConfig.github_app_id
   config.github_secret_id = SemaphoreConfig.github_secret_id
+  config.github_app_webhook_secret = SemaphoreConfig.github_app_webhook_secret
+  config.check_github_app_webhook = SemaphoreConfig.check_github_app_webhook.to_s == "true"
 
   config.bitbucket_app_id = SemaphoreConfig.bitbucket_app_id
   config.bitbucket_secret_id = SemaphoreConfig.bitbucket_secret_id

github_hooks/config/app/test.rb

@@ -9,6 +9,8 @@
 
   config.github_app_id = "bd59c3a0c448179b5f3f"
   config.github_secret_id = "c40e646d16dca15d4a5155397e4e66b928678f15"
+  config.github_app_webhook_secret = "lkasjdlkjKSJHKsa123lskdfn"
+  config.check_github_app_webhook = true
 
   config.bitbucket_app_id = "G3cXBDsDEwVp25rCXL"
   config.bitbucket_secret_id = "LNNfhaLKsfuzjYEeJLkN5Y93cNDb2ej4"

github_hooks/config/config.yml

@@ -9,6 +9,7 @@ secret_token: "jfieitmv83yfv73osjkdjfvy8213kskjdfo834jv"
 
 github_app_id: "bd59c3a0c448179b5f3f"
 github_secret_id: "c40e646d16dca15d4a5155397e4e66b928678f15"
+github_app_webhook_secret: "lkasjdlkjKSJHKsa123lskdfn"
 
 bitbucket_app_id: "G3cXBDsDEwVp25rCXL"
 bitbucket_secret_id: "LNNfhaLKsfuzjYEeJLkN5Y93cNDb2ej4"

github_hooks/lib/semaphore/github_app/credentials.rb

@@ -4,6 +4,7 @@ module Semaphore::GithubApp::Credentials
   @github_application_id = nil
   @github_client_id = nil
   @github_client_secret = nil
+  @github_app_webhook_secret = nil
 
   def self.private_key
     @private_key ||= Local.pem || InstanceConfigClient.pem
@@ -25,6 +26,10 @@ def self.github_client_secret
     @github_client_secret ||= InstanceConfigClient.github_client_secret || Local.github_client_secret
   end
 
+  def self.github_app_webhook_secret
+    @github_app_webhook_secret ||= InstanceConfigClient.github_app_webhook_secret || Local.github_app_webhook_secret
+  end
+
   class Local
     def self.pem
       if File.exist?(App.github_application_key_path) # function
@@ -47,6 +52,10 @@ def self.github_client_id
     def self.github_client_secret
       App.github_secret_id.presence
     end
+
+    def self.github_app_webhook_secret
+      App.github_app_webhook_secret.presence
+    end
   end
 
   class InstanceConfigClient
@@ -92,6 +101,10 @@ def self.github_client_id
       get_field("client_id")
     end
 
+    def self.github_app_webhook_secret
+      get_field("webhook_secret")
+    end
+
     def self.github_client_secret
       get_field("client_secret")
     end

github_hooks/lib/semaphore/github_app/hook.rb

@@ -124,5 +124,16 @@ def self.remove_repositories(installation_id, repositories)
     def self.get_installation(installation_id)
       GithubAppInstallation.find_by!(:installation_id => installation_id)
     end
+
+    def self.webhook_signature_valid?(secret, signature, payload)
+      signing_secret = secret
+      computed_signature = "sha256=#{OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), signing_secret, payload)}"
+
+      if Rack::Utils.secure_compare(computed_signature, signature)
+        :ok
+      else
+        :not_verified
+      end
+    end
   end
 end

github_hooks/spec/controllers/projects_controller_spec.rb

@@ -26,6 +26,9 @@ def post_app_payload(payload)
     context "when GitHub hook" do
       let(:workflow) { double(Workflow, :id => 111, :update_attribute => nil) }
       let(:payload) { RepoHost::Github::Responses::Payload.post_receive_hook_pull_request }
+      let(:signature) do
+        "sha256=#{OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), "secret", payload)}"
+      end
       let(:payload_request) do
         body = StringIO.new
         body.puts payload
@@ -34,14 +37,15 @@ def post_app_payload(payload)
                  "User-Agent" => "GitHub-Hookshot/xxx",
                  "X-Github-Event" => event,
                  "X-GitHub-Hook-Installation-Target-Type" => installation_target_type,
-                 "X-Hub-Signature-256" => "sha256=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17"
+                 "X-Hub-Signature-256" => signature
                }, :body => body, :raw_post => "Hello, World!")
       end
 
       before do
         allow(controller).to receive(:repo_host_request) { payload_request }
         allow(Semaphore::RepoHost::Hooks::Recorder).to receive(:record_hook).and_return(workflow)
         allow(Semaphore::RepoHost::Hooks::Handler).to receive(:enqueue_job)
+        allow(Semaphore::GithubApp::Credentials).to receive(:github_app_webhook_secret).and_return("secret")
 
         @organization = FactoryBot.create(:organization)
         @project = FactoryBot.create(:project,
@@ -110,7 +114,7 @@ def post_app_payload(payload)
 
         it "calls execute on post commit service" do
           expect(Semaphore::RepoHost::Hooks::Recorder).to receive(:record_hook).and_return(workflow)
-          expect(Semaphore::RepoHost::Hooks::Handler::Worker).to receive(:perform_async).with(111, "Hello, World!", "sha256=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17", 0)
+          expect(Semaphore::RepoHost::Hooks::Handler::Worker).to receive(:perform_async).with(111, "Hello, World!", signature, 0)
 
           post_payload(payload)
         end
@@ -148,61 +152,62 @@ def post_app_payload(payload)
 
       context "when issue comment event occurs" do
         let(:event) { "issue_comment" }
+        let(:payload) { RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment }
 
         it "doesn't required authenticated_user" do
           expect(controller).not_to receive(:authenticate_user!)
 
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
         end
 
         it "doesn't publish event" do
           expect(Tackle).not_to receive(:publish)
           expect(Semaphore::GithubApp::Collaborators::Worker).not_to receive(:perform_async)
 
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
         end
 
         it "saves the hook payload" do
           expect(Semaphore::RepoHost::Hooks::Recorder).to receive(:record_hook)
 
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
         end
 
         it "calls execute on post commit service" do
           expect(Semaphore::RepoHost::Hooks::Recorder).to receive(:record_hook).and_return(workflow)
-          expect(Semaphore::RepoHost::Hooks::Handler::Worker).to receive(:perform_async).with(111, "Hello, World!", "sha256=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17", 0)
+          expect(Semaphore::RepoHost::Hooks::Handler::Worker).to receive(:perform_async).with(111, "Hello, World!", signature, 0)
 
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
         end
 
         it "saves workflow's result as OK" do
           expect(workflow).to receive(:update_attribute).with(:result, Workflow::RESULT_OK)
 
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
         end
 
         it "responds with OK" do
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
 
           expect(response).to be_ok
         end
 
         it "measures the execution duration" do
           expect(Watchman).to receive(:benchmark).with("repo_host_post_commit_hooks.controller.duration").and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
         end
 
         it "increments the handled hooks count" do
           expect(Watchman).to receive(:increment).with("IncommingHooks.processed", { external: true }).and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
         end
 
         it "increments the external metric for hooks count" do
           expect(Watchman).to receive(:increment).with("IncommingHooks.received", { external: true }).and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.post_receive_hook_issue_comment)
+          post_payload(payload)
         end
       end
 
@@ -271,17 +276,18 @@ def post_app_payload(payload)
       context "when repository event occurs" do
         let(:event) { "repository" }
         let(:installation_target_type) { "integration" }
+        let(:payload) { RepoHost::Github::Responses::Payload.repository_renamed_app_hook }
 
         it "response with head ok" do
-          post_payload(RepoHost::Github::Responses::Payload.repository_renamed_app_hook)
+          post_payload(payload)
 
           expect(response).to be_ok
         end
 
         it "measures the execution duration" do
           expect(Watchman).to receive(:benchmark).with("repo_host_post_commit_hooks.controller.duration").and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.repository_renamed_app_hook)
+          post_payload(payload)
         end
 
         it "increments the repository_webhook count" do
@@ -290,35 +296,36 @@ def post_app_payload(payload)
           expect(Watchman).to receive(:increment).with("repo_host_post_commit_hooks.controller.github_app_webhook").and_call_original
           expect(Watchman).to receive(:increment).with("repo_host_post_commit_hooks.controller.repository_webhook").and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.repository_renamed_app_hook)
+          post_payload(payload)
         end
 
         it "perform repository sync" do
           expect(Semaphore::GithubApp::Repositories::Worker).to receive(:perform_async).and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.repository_renamed_app_hook)
+          post_payload(payload)
         end
       end
 
       context "when repository renamed event occurs" do
         let(:event) { "repository" }
+        let(:payload) { RepoHost::Github::Responses::Payload.repository_renamed_hook }
 
         it "response with head ok" do
-          post_payload(RepoHost::Github::Responses::Payload.repository_renamed_hook)
+          post_payload(payload)
 
           expect(response).to be_ok
         end
 
         it "measures the execution duration" do
           expect(Watchman).to receive(:benchmark).with("repo_host_post_commit_hooks.controller.duration").and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.repository_renamed_hook)
+          post_payload(payload)
         end
 
         it "increments the repository_renamed_webhook count" do
           expect(Watchman).to receive(:increment).with("repo_host_post_commit_hooks.controller.repository_webhook").and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.repository_renamed_hook)
+          post_payload(payload)
         end
 
         it "publish event" do
@@ -328,29 +335,30 @@ def post_app_payload(payload)
 
           expect(Semaphore::Events::RemoteRepositoryChanged).to receive(:emit).and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.repository_renamed_hook)
+          post_payload(payload)
         end
       end
 
       context "when default branch changed event occurs" do
         let(:event) { "repository" }
+        let(:payload) { RepoHost::Github::Responses::Payload.default_branch_changed }
 
         it "response with head ok" do
-          post_payload(RepoHost::Github::Responses::Payload.default_branch_changed)
+          post_payload(payload)
 
           expect(response).to be_ok
         end
 
         it "measures the execution duration" do
           expect(Watchman).to receive(:benchmark).with("repo_host_post_commit_hooks.controller.duration").and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.default_branch_changed)
+          post_payload(payload)
         end
 
         it "increments the default_branch_changed count" do
           expect(Watchman).to receive(:increment).with("repo_host_post_commit_hooks.controller.repository_webhook").and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.default_branch_changed)
+          post_payload(payload)
         end
 
         it "publish event" do
@@ -360,63 +368,65 @@ def post_app_payload(payload)
 
           expect(Semaphore::Events::RemoteRepositoryChanged).to receive(:emit).and_call_original
 
-          post_payload(RepoHost::Github::Responses::Payload.default_branch_changed)
+          post_payload(payload)
         end
       end
 
       context "when github_app installation event occurs" do
         let(:event) { "installation" }
+        let(:payload) { RepoHost::Github::Responses::Payload.installation_created }
 
         it "response with head ok" do
-          post_app_payload(RepoHost::Github::Responses::Payload.installation_created)
+          post_app_payload(payload)
 
           expect(response).to be_ok
         end
 
         it "measures the execution duration" do
           expect(Watchman).to receive(:benchmark).with("repo_host_post_commit_hooks.controller.duration").and_call_original
 
-          post_app_payload(RepoHost::Github::Responses::Payload.installation_created)
+          post_app_payload(payload)
         end
 
         it "increments the github_app_webhook count" do
           expect(Watchman).to receive(:increment).with("repo_host_post_commit_hooks.controller.github_app_webhook").and_call_original
 
-          post_app_payload(RepoHost::Github::Responses::Payload.installation_created)
+          post_app_payload(payload)
         end
 
         it "calls GithubApp hook processor" do
           expect(Semaphore::GithubApp::Hook).to receive(:process).and_call_original
 
-          post_app_payload(RepoHost::Github::Responses::Payload.installation_created)
+          post_app_payload(payload)
         end
       end
 
       context "when github_app installation_repositories event occurs" do
         let(:event) { "installation_repositories" }
+        let(:payload) { RepoHost::Github::Responses::Payload.installation_repositories_added }
 
         it "response with head ok" do
-          post_app_payload(RepoHost::Github::Responses::Payload.installation_repositories_added)
+          post_app_payload(payload)
 
           expect(response).to be_ok
         end
 
         it "measures the execution duration" do
           expect(Watchman).to receive(:benchmark).with("repo_host_post_commit_hooks.controller.duration").and_call_original
 
-          post_app_payload(RepoHost::Github::Responses::Payload.installation_repositories_added)
+          post_app_payload(payload)
         end
 
         it "increments the github_app_webhook count" do
           expect(Watchman).to receive(:increment).with("repo_host_post_commit_hooks.controller.github_app_webhook").and_call_original
 
-          post_app_payload(RepoHost::Github::Responses::Payload.installation_repositories_added)
+          post_app_payload(payload)
         end
 
         it "calls GithubApp hook processor" do
           expect(Semaphore::GithubApp::Hook).to receive(:process).and_call_original
 
-          post_app_payload(RepoHost::Github::Responses::Payload.installation_repositories_added)
+          post_app_payload(payload)
         end
       end