Redact unsubscribe links in issuable texts

It's possible that user pastes accidentally also unsubscribe link
which is included in footer of notification emails. This unsubscribe
link contains personal token which attacker then use to act as the
original user (e.g. for sending comments under his/her identity).

app/models/concerns/issuable.rb

@@ -9,6 +9,7 @@
 module Issuable
   extend ActiveSupport::Concern
   include Gitlab::SQL::Pattern
+  include Redactable
   include CacheMarkdownField
   include Participable
   include Mentionable
@@ -32,6 +33,8 @@ module Issuable
     cache_markdown_field :title, pipeline: :single_line
     cache_markdown_field :description, issuable_state_filter_enabled: true
+    redact_field :description
+
     belongs_to :author, class_name: "User"
     belongs_to :updated_by, class_name: "User"
     belongs_to :last_edited_by, class_name: 'User'

app/models/concerns/redactable.rb

+# frozen_string_literal: true
+
+# This module searches and redacts sensitive information in
+# redactable fields. Currently only unsubscribe link is redacted.
+# Add following lines into your model:
+#
+#     include Redactable
+#     redact_field :foo
+#
+module Redactable
+  extend ActiveSupport::Concern
+
+  UNSUBSCRIBE_PATTERN = %r{/sent_notifications/\h{32}/unsubscribe}
+
+  class_methods do
+    def redact_field(field)
+      before_validation do
+        redact_field!(field) if attribute_changed?(field)
+      end
+    end
+  end
+
+  private
+
+  def redact_field!(field)
+    text = public_send(field) # rubocop:disable GitlabSecurity/PublicSend
+    return unless text.present?
+
+    redacted = text.gsub(UNSUBSCRIBE_PATTERN, '/sent_notifications/REDACTED/unsubscribe')
+
+    public_send("#{field}=", redacted) # rubocop:disable GitlabSecurity/PublicSend
+  end
+end

app/models/note.rb

@@ -10,6 +10,7 @@ class Note < ActiveRecord::Base
   include Awardable
   include Importable
   include FasterCacheKeys
+  include Redactable
   include CacheMarkdownField
   include AfterCommitQueue
   include ResolvableNote
@@ -33,6 +34,8 @@ class Note < ActiveRecord::Base
   cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true
+  redact_field :note
+
   # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes.
   # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10392/diffs#note_28719102
   alias_attribute :last_edited_at, :updated_at

app/models/snippet.rb

@@ -2,6 +2,7 @@
 class Snippet < ActiveRecord::Base
   include Gitlab::VisibilityLevel
+  include Redactable
   include CacheMarkdownField
   include Noteable
   include Participable
@@ -18,6 +19,8 @@ class Snippet < ActiveRecord::Base
   cache_markdown_field :description
   cache_markdown_field :content
+  redact_field :description
+
   # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with snippets.
   # See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10392/diffs#note_28719102
   alias_attribute :last_edited_at, :updated_at

changelogs/unreleased/redact-links-dev.yml

+---
+title: Redact personal tokens in unsubscribe links.
+merge_request:
+author:
+type: security

db/post_migrate/20181014121030_enqueue_redact_links.rb

+# frozen_string_literal: true
+
+class EnqueueRedactLinks < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 1000
+  DELAY_INTERVAL = 5.minutes.to_i
+  MIGRATION = 'RedactLinks'
+
+  disable_ddl_transaction!
+
+  class Note < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'notes'
+    self.inheritance_column = :_type_disabled
+  end
+
+  class Issue < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'issues'
+    self.inheritance_column = :_type_disabled
+  end
+
+  class MergeRequest < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'merge_requests'
+    self.inheritance_column = :_type_disabled
+  end
+
+  class Snippet < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'snippets'
+    self.inheritance_column = :_type_disabled
+  end
+
+  def up
+    disable_statement_timeout do
+      schedule_migration(Note, 'note')
+      schedule_migration(Issue, 'description')
+      schedule_migration(MergeRequest, 'description')
+      schedule_migration(Snippet, 'description')
+    end
+  end
+
+  def down
+    # nothing to do
+  end
+
+  private
+
+  def schedule_migration(model, field)
+    link_pattern = "%/sent_notifications/" + ("_" * 32) + "/unsubscribe%"
+
+    model.where("#{field} like ?", link_pattern).each_batch(of: BATCH_SIZE) do |batch, index|
+      start_id, stop_id = batch.pluck('MIN(id)', 'MAX(id)').first
+
+      BackgroundMigrationWorker.perform_in(index * DELAY_INTERVAL, MIGRATION, [model.name.demodulize, field, start_id, stop_id])
+    end
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20181013005024) do
+ActiveRecord::Schema.define(version: 20181014121030) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

lib/gitlab/background_migration/redact_links.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class RedactLinks
+      module Redactable
+        extend ActiveSupport::Concern
+
+        def redact_field!(field)
+          self[field].gsub!(%r{/sent_notifications/\h{32}/unsubscribe}, '/sent_notifications/REDACTED/unsubscribe')
+
+          if self.changed?
+            self.update_columns(field => self[field],
+                                "#{field}_html" => nil)
+          end
+        end
+      end
+
+      class Note < ActiveRecord::Base
+        include EachBatch
+        include Redactable
+
+        self.table_name = 'notes'
+        self.inheritance_column = :_type_disabled
+      end
+
+      class Issue < ActiveRecord::Base
+        include EachBatch
+        include Redactable
+
+        self.table_name = 'issues'
+        self.inheritance_column = :_type_disabled
+      end
+
+      class MergeRequest < ActiveRecord::Base
+        include EachBatch
+        include Redactable
+
+        self.table_name = 'merge_requests'
+        self.inheritance_column = :_type_disabled
+      end
+
+      class Snippet < ActiveRecord::Base
+        include EachBatch
+        include Redactable
+
+        self.table_name = 'snippets'
+        self.inheritance_column = :_type_disabled
+      end
+
+      def perform(model_name, field, start_id, stop_id)
+        link_pattern = "%/sent_notifications/" + ("_" * 32) + "/unsubscribe%"
+        model = "Gitlab::BackgroundMigration::RedactLinks::#{model_name}".constantize
+
+        model.where("#{field} like ?", link_pattern).where(id: start_id..stop_id).each do |resource|
+          resource.redact_field!(field)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/redact_links_spec.rb

+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::RedactLinks, :migration, schema: 20181014121030 do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:issues) { table(:issues) }
+  let(:notes) { table(:notes) }
+  let(:snippets) { table(:snippets) }
+  let(:users) { table(:users) }
+  let(:merge_requests) { table(:merge_requests) }
+  let(:namespace) { namespaces.create(name: 'gitlab', path: 'gitlab-org') }
+  let(:project) { projects.create(namespace_id: namespace.id, name: 'foo') }
+  let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') }
+
+  def create_merge_request(id, params)
+    params.merge!(id: id,
+                  target_project_id: project.id,
+                  target_branch: 'master',
+                  source_project_id: project.id,
+                  source_branch: 'mr name',
+                  title: "mr name#{id}")
+
+    merge_requests.create(params)
+  end
+
+  def create_issue(id, params)
+    params.merge!(id: id, title: "issue#{id}", project_id: project.id)
+
+    issues.create(params)
+  end
+
+  def create_note(id, params)
+    params[:id] = id
+
+    notes.create(params)
+  end
+
+  def create_snippet(id, params)
+    params.merge!(id: id, author_id: user.id)
+
+    snippets.create(params)
+  end
+
+  def create_resource(model, id, params)
+    send("create_#{model.name.underscore}", id, params)
+  end
+
+  shared_examples_for 'redactable resource' do
+    it 'updates only matching texts' do
+      matching_text = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
+      redacted_text = 'some text /sent_notifications/REDACTED/unsubscribe more text'
+      create_resource(model, 1, { field => matching_text })
+      create_resource(model, 2, { field => 'not matching text' })
+      create_resource(model, 3, { field => matching_text })
+      create_resource(model, 4, { field => redacted_text })
+      create_resource(model, 5, { field => matching_text })
+
+      expected = { field => 'some text /sent_notifications/REDACTED/unsubscribe more text',
+                   "#{field}_html" => nil }
+      expect_any_instance_of("Gitlab::BackgroundMigration::RedactLinks::#{model}".constantize).to receive(:update_columns).with(expected).and_call_original
+
+      subject.perform(model, field, 2, 4)
+
+      expect(model.where(field => matching_text).pluck(:id)).to eq [1, 5]
+      expect(model.find(3).reload[field]).to eq redacted_text
+    end
+  end
+
+  context 'resource is Issue' do
+    it_behaves_like 'redactable resource' do
+      let(:model) { Issue }
+      let(:field) { :description }
+    end
+  end
+
+  context 'resource is Merge Request' do
+    it_behaves_like 'redactable resource' do
+      let(:model) { MergeRequest }
+      let(:field) { :description }
+    end
+  end
+
+  context 'resource is Note' do
+    it_behaves_like 'redactable resource' do
+      let(:model) { Note }
+      let(:field) { :note }
+    end
+  end
+
+  context 'resource is Snippet' do
+    it_behaves_like 'redactable resource' do
+      let(:model) { Snippet }
+      let(:field) { :description }
+    end
+  end
+end

spec/migrations/enqueue_redact_links_spec.rb

+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20181014121030_enqueue_redact_links.rb')
+
+describe EnqueueRedactLinks, :migration, :sidekiq do
+  let(:merge_requests) { table(:merge_requests) }
+  let(:issues) { table(:issues) }
+  let(:notes) { table(:notes) }
+  let(:projects) { table(:projects) }
+  let(:namespaces) { table(:namespaces) }
+  let(:snippets) { table(:snippets) }
+  let(:users) { table(:users) }
+  let(:user) { users.create!(email: 'test@example.com', projects_limit: 100, username: 'test') }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+
+    text = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
+    group = namespaces.create!(name: 'gitlab', path: 'gitlab')
+    project = projects.create!(namespace_id: group.id)
+
+    merge_requests.create!(id: 1, target_project_id: project.id, source_project_id: project.id, target_branch: 'feature', source_branch: 'master', description: text)
+    issues.create!(id: 1, description: text)
+    notes.create!(id: 1, note: text)
+    notes.create!(id: 2, note: text)
+    snippets.create!(id: 1, description: text, author_id: user.id)
+  end
+
+  it 'correctly schedules background migrations' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(5.minutes, "Note", "note", 1, 1)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(10.minutes, "Note", "note", 2, 2)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(5.minutes, "Issue", "description", 1, 1)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(5.minutes, "MergeRequest", "description", 1, 1)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(5.minutes, "Snippet", "description", 1, 1)
+        expect(BackgroundMigrationWorker.jobs.size).to eq 5
+      end
+    end
+  end
+end

spec/models/concerns/redactable_spec.rb

+require 'spec_helper'
+
+describe Redactable do
+  shared_examples 'model with redactable field' do
+    it 'redacts unsubscribe token' do
+      model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
+
+      model.save!
+
+      expect(model[field]).to eq 'some text /sent_notifications/REDACTED/unsubscribe more text'
+    end
+
+    it 'ignores not hexadecimal tokens' do
+      text = 'some text /sent_notifications/token/unsubscribe more text'
+      model[field] = text
+
+      model.save!
+
+      expect(model[field]).to eq text
+    end
+
+    it 'ignores not matching texts' do
+      text = 'some text /sent_notifications/.*/unsubscribe more text'
+      model[field] = text
+
+      model.save!
+
+      expect(model[field]).to eq text
+    end
+
+    it 'redacts the field when saving the model before creating markdown cache' do
+      model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
+
+      model.save!
+
+      expected = 'some text /sent_notifications/REDACTED/unsubscribe more text'
+      expect(model[field]).to eq expected
+      expect(model["#{field}_html"]).to eq "<p dir=\"auto\">#{expected}</p>"
+    end
+  end
+
+  context 'when model is an issue' do
+    it_behaves_like 'model with redactable field' do
+      let(:model) { create(:issue) }
+      let(:field) { :description }
+    end
+  end
+
+  context 'when model is a merge request' do
+    it_behaves_like 'model with redactable field' do
+      let(:model) { create(:merge_request) }
+      let(:field) { :description }
+    end
+  end
+
+  context 'when model is a note' do
+    it_behaves_like 'model with redactable field' do
+      let(:model) { create(:note) }
+      let(:field) { :note }
+    end
+  end
+
+  context 'when model is a snippet' do
+    it_behaves_like 'model with redactable field' do
+      let(:model) { create(:snippet) }
+      let(:field) { :description }
+    end
+  end
+end