diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 2aa52bbaeea..a808f9ad4ad 100644 --- a/app/models/concerns/issuable.rb +++ b/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' diff --git a/app/models/concerns/redactable.rb b/app/models/concerns/redactable.rb new file mode 100644 index 00000000000..5ad96d6cc46 --- /dev/null +++ b/app/models/concerns/redactable.rb @@ -0,0 +1,33 @@ +# 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 diff --git a/app/models/note.rb b/app/models/note.rb index e1bd943e8e4..990689a95f5 100644 --- a/app/models/note.rb +++ b/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 diff --git a/app/models/snippet.rb b/app/models/snippet.rb index e9533ee7c77..1c5846b4023 100644 --- a/app/models/snippet.rb +++ b/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 diff --git a/changelogs/unreleased/redact-links-dev.yml b/changelogs/unreleased/redact-links-dev.yml new file mode 100644 index 00000000000..338e7965465 --- /dev/null +++ b/changelogs/unreleased/redact-links-dev.yml @@ -0,0 +1,5 @@ +--- +title: Redact personal tokens in unsubscribe links. +merge_request: +author: +type: security diff --git a/db/post_migrate/20181014121030_enqueue_redact_links.rb b/db/post_migrate/20181014121030_enqueue_redact_links.rb new file mode 100644 index 00000000000..1ee4703c88a --- /dev/null +++ b/db/post_migrate/20181014121030_enqueue_redact_links.rb @@ -0,0 +1,65 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 7ee2c483e54..4b741e25fe8 100644 --- a/db/schema.rb +++ b/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" diff --git a/lib/gitlab/background_migration/redact_links.rb b/lib/gitlab/background_migration/redact_links.rb new file mode 100644 index 00000000000..f5d3bcdd517 --- /dev/null +++ b/lib/gitlab/background_migration/redact_links.rb @@ -0,0 +1,62 @@ +# 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 diff --git a/spec/lib/gitlab/background_migration/redact_links_spec.rb b/spec/lib/gitlab/background_migration/redact_links_spec.rb new file mode 100644 index 00000000000..a40e68069cc --- /dev/null +++ b/spec/lib/gitlab/background_migration/redact_links_spec.rb @@ -0,0 +1,96 @@ +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 diff --git a/spec/migrations/enqueue_redact_links_spec.rb b/spec/migrations/enqueue_redact_links_spec.rb new file mode 100644 index 00000000000..a5da76977b7 --- /dev/null +++ b/spec/migrations/enqueue_redact_links_spec.rb @@ -0,0 +1,42 @@ +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 diff --git a/spec/models/concerns/redactable_spec.rb b/spec/models/concerns/redactable_spec.rb new file mode 100644 index 00000000000..7d320edd492 --- /dev/null +++ b/spec/models/concerns/redactable_spec.rb @@ -0,0 +1,69 @@ +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 "

#{expected}

" + 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