Merge branch 'security-redact-links' into 'master'
[master] Redact unsubscribe links in issuable texts See merge request gitlab/gitlabhq!2528
This commit is contained in:
commit
5b0b73d922
11 changed files with 382 additions and 1 deletions
|
@ -9,6 +9,7 @@
|
||||||
module Issuable
|
module Issuable
|
||||||
extend ActiveSupport::Concern
|
extend ActiveSupport::Concern
|
||||||
include Gitlab::SQL::Pattern
|
include Gitlab::SQL::Pattern
|
||||||
|
include Redactable
|
||||||
include CacheMarkdownField
|
include CacheMarkdownField
|
||||||
include Participable
|
include Participable
|
||||||
include Mentionable
|
include Mentionable
|
||||||
|
@ -32,6 +33,8 @@ module Issuable
|
||||||
cache_markdown_field :title, pipeline: :single_line
|
cache_markdown_field :title, pipeline: :single_line
|
||||||
cache_markdown_field :description, issuable_state_filter_enabled: true
|
cache_markdown_field :description, issuable_state_filter_enabled: true
|
||||||
|
|
||||||
|
redact_field :description
|
||||||
|
|
||||||
belongs_to :author, class_name: "User"
|
belongs_to :author, class_name: "User"
|
||||||
belongs_to :updated_by, class_name: "User"
|
belongs_to :updated_by, class_name: "User"
|
||||||
belongs_to :last_edited_by, class_name: 'User'
|
belongs_to :last_edited_by, class_name: 'User'
|
||||||
|
|
33
app/models/concerns/redactable.rb
Normal file
33
app/models/concerns/redactable.rb
Normal file
|
@ -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
|
|
@ -10,6 +10,7 @@ class Note < ActiveRecord::Base
|
||||||
include Awardable
|
include Awardable
|
||||||
include Importable
|
include Importable
|
||||||
include FasterCacheKeys
|
include FasterCacheKeys
|
||||||
|
include Redactable
|
||||||
include CacheMarkdownField
|
include CacheMarkdownField
|
||||||
include AfterCommitQueue
|
include AfterCommitQueue
|
||||||
include ResolvableNote
|
include ResolvableNote
|
||||||
|
@ -33,6 +34,8 @@ class Note < ActiveRecord::Base
|
||||||
|
|
||||||
cache_markdown_field :note, pipeline: :note, issuable_state_filter_enabled: true
|
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.
|
# 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
|
# See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10392/diffs#note_28719102
|
||||||
alias_attribute :last_edited_at, :updated_at
|
alias_attribute :last_edited_at, :updated_at
|
||||||
|
|
|
@ -2,6 +2,7 @@
|
||||||
|
|
||||||
class Snippet < ActiveRecord::Base
|
class Snippet < ActiveRecord::Base
|
||||||
include Gitlab::VisibilityLevel
|
include Gitlab::VisibilityLevel
|
||||||
|
include Redactable
|
||||||
include CacheMarkdownField
|
include CacheMarkdownField
|
||||||
include Noteable
|
include Noteable
|
||||||
include Participable
|
include Participable
|
||||||
|
@ -18,6 +19,8 @@ class Snippet < ActiveRecord::Base
|
||||||
cache_markdown_field :description
|
cache_markdown_field :description
|
||||||
cache_markdown_field :content
|
cache_markdown_field :content
|
||||||
|
|
||||||
|
redact_field :description
|
||||||
|
|
||||||
# Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with snippets.
|
# 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
|
# See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10392/diffs#note_28719102
|
||||||
alias_attribute :last_edited_at, :updated_at
|
alias_attribute :last_edited_at, :updated_at
|
||||||
|
|
5
changelogs/unreleased/redact-links-dev.yml
Normal file
5
changelogs/unreleased/redact-links-dev.yml
Normal file
|
@ -0,0 +1,5 @@
|
||||||
|
---
|
||||||
|
title: Redact personal tokens in unsubscribe links.
|
||||||
|
merge_request:
|
||||||
|
author:
|
||||||
|
type: security
|
65
db/post_migrate/20181014121030_enqueue_redact_links.rb
Normal file
65
db/post_migrate/20181014121030_enqueue_redact_links.rb
Normal file
|
@ -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
|
|
@ -11,7 +11,7 @@
|
||||||
#
|
#
|
||||||
# It's strongly recommended that you check this file into your version control system.
|
# 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
|
# These are extensions that must be enabled in order to support this database
|
||||||
enable_extension "plpgsql"
|
enable_extension "plpgsql"
|
||||||
|
|
62
lib/gitlab/background_migration/redact_links.rb
Normal file
62
lib/gitlab/background_migration/redact_links.rb
Normal file
|
@ -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
|
96
spec/lib/gitlab/background_migration/redact_links_spec.rb
Normal file
96
spec/lib/gitlab/background_migration/redact_links_spec.rb
Normal file
|
@ -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
|
42
spec/migrations/enqueue_redact_links_spec.rb
Normal file
42
spec/migrations/enqueue_redact_links_spec.rb
Normal file
|
@ -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
|
69
spec/models/concerns/redactable_spec.rb
Normal file
69
spec/models/concerns/redactable_spec.rb
Normal file
|
@ -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 "<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
|
Loading…
Reference in a new issue