diff --git a/github_hooks/app/controllers/projects_controller.rb b/github_hooks/app/controllers/projects_controller.rb index 30e09c253..8e8131d9d 100644 --- a/github_hooks/app/controllers/projects_controller.rb +++ b/github_hooks/app/controllers/projects_controller.rb @@ -78,6 +78,21 @@ def repo_host_post_commit_hook next end + signature = repo_host_request.headers["X-Hub-Signature-256"] + + # Rubocop insisted on making this a one big if, istead of 2 nested if statements + # We are validating a signature from each webhook coming from github app above + # in this function, (currently line 29), and we are validating signatures for every webhook + # coming from a repository within the hook handler. + + # Only exception to the signature verification checks are repository level webhooks that do not generate + # a workflow (for example new member added to repo, or repository metadata changed). That will be covered + # in the following if block. + if (webhook_filter.repository_webhook? || (webhook_filter.member_webhook? && !webhook_filter.github_app_webhook?)) && !Semaphore::RepoHost::Hooks::Handler.webhook_signature_valid?(logger, project.organization.id, project.repository.id, repo_host_request.raw_post, signature) + logger.error("Webhook validation for repository changed and repository member events failed") + next + end + if webhook_filter.member_webhook? Watchman.increment("repo_host_post_commit_hooks.controller.member_webhook") logger.info("Member Webhook") @@ -103,9 +118,7 @@ def repo_host_post_commit_hook end workflow = Semaphore::RepoHost::Hooks::Recorder.record_hook(hook_params, project) - logger.add(:post_commit_request_id => workflow.id) - logger.info("Saved Request") if webhook_filter.unavailable_payload? @@ -122,12 +135,6 @@ def repo_host_post_commit_hook Watchman.increment("IncommingHooks.processed", { external: true }) - signature = repo_host_request.headers["X-Hub-Signature-256"] - - unless signature - Watchman.increment("hooks.processing.missing_signature", tags: [project.id, project.organization.id]) - end - sidekiq_job_id = Semaphore::RepoHost::Hooks::Handler::Worker.perform_async(workflow.id, repo_host_request.raw_post, signature, 0) logger.info("Enqueud in Sidekiq", :sidekiq_job_id => sidekiq_job_id) diff --git a/github_hooks/spec/controllers/projects_controller_spec.rb b/github_hooks/spec/controllers/projects_controller_spec.rb index 31399e395..c6a3d5971 100644 --- a/github_hooks/spec/controllers/projects_controller_spec.rb +++ b/github_hooks/spec/controllers/projects_controller_spec.rb @@ -306,8 +306,9 @@ def post_app_payload(payload) end it "publish event" do + repository = double("Repository", :id => "repo-123") project = double(Project, :id => "96b0a57c-d9ae-453f-b56f-3b154eb10cda", :organization => @organization, - :repo_owner_and_name => "foo/bar") + :repo_owner_and_name => "foo/bar", :repository => repository) expect(Project).to receive(:find_by).and_return(project) expect(Semaphore::Events::ProjectCollaboratorsChanged).to receive(:emit).and_call_original @@ -373,10 +374,19 @@ def post_app_payload(payload) end it "publish event" do + repository = double(Repository, :id => "repo-123") project = double(Project, :id => "96b0a57c-d9ae-453f-b56f-3b154eb10cda", :organization => @organization, - :repo_owner => "foo") + :repo_owner => "foo", :repository => repository) expect(Project).to receive(:find_by).and_return(project) + expect(Semaphore::RepoHost::Hooks::Handler).to receive(:webhook_signature_valid?).with( + anything, + project.organization.id, + project.repository.id, + anything, + anything + ).and_return(true) + expect(Semaphore::Events::RemoteRepositoryChanged).to receive(:emit).and_call_original post_payload(payload) @@ -406,14 +416,35 @@ def post_app_payload(payload) end it "publish event" do + repository = double(Repository, :id => "repo-123") project = double(Project, :id => "96b0a57c-d9ae-453f-b56f-3b154eb10cda", :organization => @organization, - :repo_owner_and_name => "foo/bar") + :repo_owner_and_name => "foo/bar", :repository => repository) expect(Project).to receive(:find_by).and_return(project) + expect(Semaphore::RepoHost::Hooks::Handler).to receive(:webhook_signature_valid?).with( + anything, + project.organization.id, + project.repository.id, + anything, + anything + ).and_return(true) + expect(Semaphore::Events::RemoteRepositoryChanged).to receive(:emit).and_call_original post_payload(payload) end + + it "does not publish event when signature is invalid" do + repository = double("Repository", :id => "repo-123") + project = double(Project, :id => "96b0a57c-d9ae-453f-b56f-3b154eb10cda", :organization => @organization, + :repo_owner_and_name => "foo/bar", :repository => repository) + expect(Project).to receive(:find_by).and_return(project) + + expect(Semaphore::RepoHost::Hooks::Handler).to receive(:webhook_signature_valid?).and_return(false) + expect(Semaphore::Events::RemoteRepositoryChanged).not_to receive(:emit) + + post_payload(payload) + end end context "when github_app installation event occurs" do