Skip to content

Commit

Permalink
adds url-specific sanitation logic, avoiding subsets
Browse files Browse the repository at this point in the history
  • Loading branch information
@bvandgrift @kayline
bvandgrift authored and kayline committed Jun 23, 2020
1 parent c418823 commit b8b633c
Show file tree
Hide file tree
Showing 2 changed files with 84 additions and 34 deletions.
41 changes: 35 additions & 6 deletions app/services/mixpanel_service.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,41 @@ class << self
#
# MixpanelService.strip_all_from('what a day', ['a', 'w'])
# # => 'ht dy'
def strip_all_from(target, exclusions)
def strip_all_from_string(target, exclusions)
return unless target.present?

exclusions.reduce(target) { |acc, ex| acc.gsub(ex.to_s, '') }
end

##
# convenience method for stripping a list of substrings from a url while
# preserving its structure and query string structure
#
# @param [String] target the url to be strippped
# @param [Enumerable(String)] exclusions list of strings to be removed from target
#
# @example
#
# MixpanelService.strip_all_from('/this/dang/thing?query=remove-me', ['dang', 'remove-me'])
# # => '/this/***/thing?query=***'
def strip_all_from_url(url, exclusions)
exclusions = exclusions.map(&:to_s)
return unless url.present?
return url if exclusions.empty?

path, querystring = url.split('?')
path = path.split('/').map { |part| exclusions.include?(part) ? '***' : part }.join('/')

if querystring.present?
path << '?'
path << querystring.split('&').map do |pair|
k, v = pair.split('=')
[(exclusions.include?(k) ? '***' : k), (exclusions.include?(v) ? '***' : v)].join('=')
end.join('&')
end
path
end

##
# sends event to MixPanel
#
Expand Down Expand Up @@ -135,7 +164,7 @@ def data_from_controller(source)
def data_from_request(source, path_exclusions: [])
user_agent = DeviceDetector.new(source.user_agent)
major_browser_version = user_agent.full_version.try { |v| v.partition('.').first } rescue ""
os_major_version = user_agent.os_full_version.try { |v| v.partition('.').first } rescue ""
os_major_version = user_agent.os_full_version.try { |v| v.partition('.').first } rescue ""
{
browser_name: user_agent.name,
browser_full_version: user_agent.full_version,
Expand All @@ -150,10 +179,10 @@ def data_from_request(source, path_exclusions: [])
device_type: user_agent.device_type,
device_browser_version: "#{user_agent.os_name} #{user_agent.device_type} #{user_agent.name} #{major_browser_version}",
full_user_agent: source.user_agent,
path: strip_all_from(source.path, path_exclusions),
full_path: strip_all_from(source.fullpath, path_exclusions),
referrer: strip_all_from(source.referrer, path_exclusions),
referrer_domain: strip_all_from((URI.parse(source.referrer).host || "None" rescue "None"),path_exclusions),
path: strip_all_from_url(source.path, path_exclusions),
full_path: strip_all_from_url(source.fullpath, path_exclusions),
referrer: strip_all_from_url(source.referrer, path_exclusions),
referrer_domain: strip_all_from_url((URI.parse(source.referrer).host || "None" rescue "None"), path_exclusions),
}
end

Expand Down
77 changes: 49 additions & 28 deletions spec/services/mixpanel_service_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,37 @@
before do
allow(bare_request).to receive(:referrer).and_return("http://test-host.dev/remove-me/referred")
allow(bare_request).to receive(:path).and_return("/remove-me/resource")
allow(bare_request).to receive(:fullpath).and_return("/remove-me/resource?id=whatever")
allow(bare_request).to receive(:fullpath).and_return("/remove-me/resource?other_field=whatever")
end

describe "#strip_all_from" do
describe "#strip_all_from_string" do
it 'strips a list of strings from a target string' do
expect(MixpanelService.strip_all_from('what a day', ['a', 'w']))
expect(MixpanelService.strip_all_from_string('what a day', ['a', 'w']))
.to eq('ht dy')
end
end

describe '#strip_all_from_url' do
let(:path) { '/this/remove-me/path' }
let(:path_plus) { '/this/remove-me/path?first=no&second=also-me' }
let(:messy_path) { '/who/me/remove-me/path?remove=true&weird=me&dinner=meat&meet=home' }

it 'strips a list of strings from a target url path' do
expect(MixpanelService.strip_all_from_url(path, ['remove-me', 'immaterial']))
.to eq('/this/***/path')
end

it 'strips a list of strings from a target url path and query string' do
expect(MixpanelService.strip_all_from_url(path_plus, ['remove-me', 'also-me', 'immaterial']))
.to eq('/this/***/path?first=no&second=***')
end

it 'avoids stripping subsets of urls and query strings' do
expect(MixpanelService.strip_all_from_url(messy_path, ['me', 'meet']))
.to eq('/who/***/remove-me/path?remove=true&weird=***&dinner=meat&***=home')
end
end

describe "#send_event" do
it 'tracks an event by name and id' do
MixpanelService.send_event(event_id: event_id, event_name: event_name, data: {})
Expand Down Expand Up @@ -122,8 +143,8 @@
event_id,
event_name,
hash_including(
path: "//resource",
full_path: "//resource?id=whatever"
path: "/***/resource",
full_path: "/***/resource?other_field=whatever"
)
)
end
Expand Down Expand Up @@ -342,9 +363,9 @@ def zendesk_ticket_id; "1212123"; end
'72347235',
'req_test_event',
hash_including(
path: '/req_test//rest?the-id=',
full_path: '/req_test//rest?the-id=',
referrer: 'http://test.dev//rest'
path: '/req_test/***/rest?the-id=***',
full_path: '/req_test/***/rest?the-id=***',
referrer: 'http://test.dev/***/rest'
)
)
end
Expand All @@ -358,9 +379,9 @@ def zendesk_ticket_id; "1212123"; end
'72347235',
'req_test_event',
hash_including(
path: '/req_test//rest?the-id=',
full_path: '/req_test//rest?the-id=',
referrer: 'http://test.dev//rest'
path: '/req_test/***/rest?the-id=***',
full_path: '/req_test/***/rest?the-id=***',
referrer: 'http://test.dev/***/rest'
)
)
end
Expand All @@ -374,9 +395,9 @@ def zendesk_ticket_id; "1212123"; end
'72347235',
'req_test_event',
hash_including(
path: '/req_test//rest?the-id=',
full_path: '/req_test//rest?the-id=',
referrer: 'http://test.dev//rest'
path: '/req_test/***/rest?the-id=***',
full_path: '/req_test/***/rest?the-id=***',
referrer: 'http://test.dev/***/rest'
)
)
end
Expand All @@ -390,9 +411,9 @@ def zendesk_ticket_id; "1212123"; end
'72347235',
'req_test_event',
hash_including(
path: '/req_test//rest?the-id=',
full_path: '/req_test//rest?the-id=',
referrer: 'http://test.dev//rest'
path: '/req_test/***/rest?the-id=***',
full_path: '/req_test/***/rest?the-id=***',
referrer: 'http://test.dev/***/rest'
)
)
end
Expand All @@ -406,9 +427,9 @@ def zendesk_ticket_id; "1212123"; end
'72347235',
'req_test_event',
hash_including(
path: '/req_test//rest?the-id=',
full_path: '/req_test//rest?the-id=',
referrer: 'http://test.dev//rest'
path: '/req_test/***/rest?the-id=***',
full_path: '/req_test/***/rest?the-id=***',
referrer: 'http://test.dev/***/rest'
)
)
end
Expand All @@ -421,8 +442,8 @@ def zendesk_ticket_id; "1212123"; end
'72347236',
'inst_test_event',
hash_including(
path: '/inst_test//rest?the-id=',
full_path: '/inst_test//rest?the-id=',
path: '/inst_test/***/rest?the-id=***',
full_path: '/inst_test/***/rest?the-id=***',
)
)
end
Expand All @@ -436,23 +457,23 @@ def zendesk_ticket_id; "1212123"; end
'72347236',
'inst_test_event',
hash_including(
path: '/inst_test//rest?the-id=',
full_path: '/inst_test//rest?the-id=',
path: '/inst_test/***/rest?the-id=***',
full_path: '/inst_test/***/rest?the-id=***',
)
)
end

it 'strips current_diy_intake.id and current_diy_intake.ticket_id from paths' do
diy_intake = create(:diy_intake, ticket_id: 83225)
routes.draw { get "inst_test/:diy_intake_id/rest?the-id=83225" => "anonymous#inst_test" }
diy_intake = create(:diy_intake, ticket_id: 9999988)
routes.draw { get "inst_test/:diy_intake_id/rest?the-id=9999988" => "anonymous#inst_test" }
get :inst_test, params: { diy_intake_id: diy_intake.id }

expect(fake_tracker).to have_received(:track).with(
'72347236',
'inst_test_event',
hash_including(
path: '/inst_test//rest?the-id=',
full_path: '/inst_test//rest?the-id=',
path: '/inst_test/***/rest?the-id=***',
full_path: '/inst_test/***/rest?the-id=***',
)
)
end
Expand Down

0 comments on commit b8b633c

Please sign in to comment.