Don't leak build tokens in build logs

5 files changed, 113 insertions, 15 deletions

diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb
index 77934ff9962..9ce5b4de42f 100644
--- a/app/controllers/projects/builds_controller.rb
+++ b/app/controllers/projects/builds_controller.rb
@@ -35,7 +35,11 @@ class Projects::BuildsController < Projects::ApplicationController
     respond_to do |format|
       format.html
       format.json do
-        render json: @build.to_json(methods: :trace_html)
+        render json: {
+          id: @build.id,
+          status: @build.status,
+          trace_html: @build.trace_html
+        }
       end
     end
   end
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 57ef4646d24..8a9d7555393 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -241,12 +241,7 @@ module Ci
     end
 
     def trace
-      trace = raw_trace
-      if project && trace.present? && project.runners_token.present?
-        trace.gsub(project.runners_token, 'xxxxxx')
-      else
-        trace
-      end
+      hide_secrets(raw_trace)
     end
 
     def trace_length
@@ -259,6 +254,7 @@ module Ci
 
     def trace=(trace)
       recreate_trace_dir
+      trace = hide_secrets(trace)
       File.write(path_to_trace, trace)
     end
 
@@ -272,6 +268,8 @@ module Ci
     def append_trace(trace_part, offset)
       recreate_trace_dir
 
+      trace_part = hide_secrets(trace_part)
+
       File.truncate(path_to_trace, offset) if File.exist?(path_to_trace)
       File.open(path_to_trace, 'ab') do |f|
         f.write(trace_part)
@@ -490,5 +488,11 @@ module Ci
 
       pipeline.config_processor.build_attributes(name)
     end
+
+    def hide_secrets(trace)
+      trace = Ci::MaskSecret.mask(trace, project.runners_token) if project
+      trace = Ci::MaskSecret.mask(trace, token)
+      trace
+    end
   end
 end
diff --git a/lib/ci/mask_secret.rb b/lib/ci/mask_secret.rb
new file mode 100644
index 00000000000..3da04edde70
--- /dev/null
+++ b/lib/ci/mask_secret.rb
@@ -0,0 +1,9 @@
+module Ci::MaskSecret
+  class << self
+    def mask(value, token)
+      return value unless value.present? && token.present?
+
+      value.gsub(token, 'x' * token.length)
+    end
+  end
+end
diff --git a/spec/lib/ci/mask_secret_spec.rb b/spec/lib/ci/mask_secret_spec.rb
new file mode 100644
index 00000000000..518de76911c
--- /dev/null
+++ b/spec/lib/ci/mask_secret_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe Ci::MaskSecret, lib: true do
+  subject { described_class }
+
+  describe '#mask' do
+    it 'masks exact number of characters' do
+      expect(subject.mask('token', 'oke')).to eq('txxxn')
+    end
+
+    it 'masks multiple occurrences' do
+      expect(subject.mask('token token token', 'oke')).to eq('txxxn txxxn txxxn')
+    end
+
+    it 'does not mask if not found' do
+      expect(subject.mask('token', 'not')).to eq('token')
+    end
+  end
+end
diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb
index 8eab4281bc7..e7864b7ad33 100644
--- a/spec/models/build_spec.rb
+++ b/spec/models/build_spec.rb
@@ -88,9 +88,7 @@ describe Ci::Build, models: true do
   end
 
   describe '#trace' do
-    subject { build.trace_html }
-
-    it { is_expected.to be_empty }
+    it { expect(build.trace).to be_nil }
 
     context 'when build.trace contains text' do
       let(:text) { 'example output' }
@@ -98,16 +96,80 @@ describe Ci::Build, models: true do
         build.trace = text
       end
 
-      it { is_expected.to include(text) }
-      it { expect(subject.length).to be >= text.length }
+      it { expect(build.trace).to eq(text) }
+    end
+
+    context 'when build.trace hides runners token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.update(trace: token)
+        build.project.update(runners_token: token)
+      end
+
+      it { expect(build.trace).not_to include(token) }
+      it { expect(build.raw_trace).to include(token) }
+    end
+
+    context 'when build.trace hides build token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.update(trace: token)
+        build.update(token: token)
+      end
+
+      it { expect(build.trace).not_to include(token) }
+      it { expect(build.raw_trace).to include(token) }
+    end
+  end
+
+  describe '#raw_trace' do
+    subject { build.raw_trace }
+
+    context 'when build.trace hides runners token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.project.update(runners_token: token)
+        build.update(trace: token)
+      end
+
+      it { is_expected.not_to include(token) }
+    end
+
+    context 'when build.trace hides build token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.update(token: token)
+        build.update(trace: token)
+      end
+
+      it { is_expected.not_to include(token) }
+    end
+  end
+
+  context '#append_trace' do
+    subject { build.trace_html }
+
+    context 'when build.trace hides runners token' do
+      let(:token) { 'my_secret_token' }
+
+      before do
+        build.project.update(runners_token: token)
+        build.append_trace(token, 0)
+      end
+
+      it { is_expected.not_to include(token) }
     end
 
-    context 'when build.trace hides token' do
+    context 'when build.trace hides build token' do
       let(:token) { 'my_secret_token' }
 
       before do
-        build.project.update_attributes(runners_token: token)
-        build.update_attributes(trace: token)
+        build.update(token: token)
+        build.append_trace(token, 0)
       end
 
       it { is_expected.not_to include(token) }