From 4cec7a5549beea0dacd0c397d497c9056d42c7a7 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Wed, 21 Dec 2016 00:07:47 +0100 Subject: [PATCH] Added content validation to x-zammad mail headers and postmaster filters. --- app/controllers/ticket_articles_controller.rb | 8 +- app/models/application_model.rb | 36 +++++ app/models/channel/email_parser.rb | 51 +++++-- app/models/channel/filter/database.rb | 1 + app/models/channel/filter/identify_sender.rb | 8 +- app/models/channel/filter/trusted.rb | 10 ++ .../communicate_email/background_job.rb | 9 +- app/models/ticket.rb | 6 +- app/models/ticket/article.rb | 48 +++++++ test/unit/email_postmaster_test.rb | 124 ++++++++++++++++++ test/unit/email_process_test.rb | 45 +++++-- 11 files changed, 309 insertions(+), 37 deletions(-) diff --git a/app/controllers/ticket_articles_controller.rb b/app/controllers/ticket_articles_controller.rb index a130f91da..3baf53f0c 100644 --- a/app/controllers/ticket_articles_controller.rb +++ b/app/controllers/ticket_articles_controller.rb @@ -254,15 +254,11 @@ class TicketArticlesController < ApplicationController article = Ticket::Article.find(params[:id]) article_permission(article) - list = Store.list( - object: 'Ticket::Article::Mail', - o_id: params[:id], - ) + file = article.as_raw # find file - return if !list + return if !file - file = Store.find(list.first) send_data( file.content, filename: file.filename, diff --git a/app/models/application_model.rb b/app/models/application_model.rb index 51d4426a4..27920c755 100644 --- a/app/models/application_model.rb +++ b/app/models/application_model.rb @@ -418,6 +418,42 @@ returns =begin +reference if association id check + + model = Model.find(123) + attributes = model.association_id_check('attribute_id', value) + +returns + + true | false + +=end + + def association_id_check(attribute_id, value) + return true if value.nil? + + attributes.each { |key, _value| + next if key != attribute_id + + # check if id is assigned + key_short = key[ key.length - 3, key.length ] + next if key_short != '_id' + key_short = key[ 0, key.length - 3 ] + + self.class.reflect_on_all_associations.map { |assoc| + next if assoc.name.to_s != key_short + item = assoc.class_name.constantize + return false if !item.respond_to?(:find_by) + ref_object = item.find_by(id: value) + return false if !ref_object + return true + } + } + true + end + +=begin + set created_by_id & updated_by_id if not given based on UserInfo (current session) Used as before_create callback, no own use needed diff --git a/app/models/channel/email_parser.rb b/app/models/channel/email_parser.rb index e2d42fa26..699f04ad1 100644 --- a/app/models/channel/email_parser.rb +++ b/app/models/channel/email_parser.rb @@ -538,13 +538,7 @@ returns article.save! # store mail plain - Store.add( - object: 'Ticket::Article::Mail', - o_id: article.id, - data: msg, - filename: "ticket-#{ticket.number}-#{article.id}.eml", - preferences: {} - ) + article.save_as_raw(msg) # store attachments if mail[:attachments] @@ -580,6 +574,29 @@ returns [ticket, article, session_user, mail] end + def self.check_attributes_by_x_headers(header_name, value) + class_name = nil + attribute = nil + if header_name =~ /^x-zammad-(.+?)-(followup-|)(.*)$/i + class_name = $1 + attribute = $3 + end + return true if !class_name + if class_name.downcase == 'article' + class_name = 'Ticket::Article' + end + return true if !attribute + key_short = attribute[ attribute.length - 3, attribute.length ] + return true if key_short != '_id' + + class_object = Object.const_get(class_name.to_classname) + return if !class_object + class_instance = class_object.new + + return false if !class_instance.association_id_check(attribute, value) + true + end + def set_attributes_by_x_headers(item_object, header_name, mail, suffix = false) # loop all x-zammad-hedaer-* headers @@ -597,6 +614,8 @@ returns if suffix header = "x-zammad-#{header_name}-#{suffix}-#{key_short}" end + + # only set value on _id if value/reference lookup exists if mail[ header.to_sym ] Rails.logger.info "header #{header} found #{mail[ header.to_sym ]}" item_object.class.reflect_on_all_associations.map { |assoc| @@ -606,15 +625,29 @@ returns Rails.logger.info "ASSOC found #{assoc.class_name} lookup #{mail[ header.to_sym ]}" item = assoc.class_name.constantize + assoc_object = nil + assoc_has_object = false if item.respond_to?(:name) + assoc_has_object = true if item.lookup(name: mail[ header.to_sym ]) - item_object[key] = item.lookup(name: mail[ header.to_sym ]).id + assoc_object = item.lookup(name: mail[ header.to_sym ]) end elsif item.respond_to?(:login) + assoc_has_object = true if item.lookup(login: mail[ header.to_sym ]) - item_object[key] = item.lookup(login: mail[ header.to_sym ]).id + assoc_object = item.lookup(login: mail[ header.to_sym ]) end end + + next if assoc_has_object == false + + if assoc_object + item_object[key] = assoc_object.id + next + end + + # no assoc exists, remove header + mail.delete(header.to_sym) } end end diff --git a/app/models/channel/filter/database.rb b/app/models/channel/filter/database.rb index 5d7c7d00f..aea04d4f6 100644 --- a/app/models/channel/filter/database.rb +++ b/app/models/channel/filter/database.rb @@ -42,6 +42,7 @@ module Channel::Filter::Database next if !all_matches_ok filter[:perform].each { |key, meta| + next if !Channel::EmailParser.check_attributes_by_x_headers(key, meta['value']) Rails.logger.info " perform '#{key.downcase}' = '#{meta.inspect}'" mail[ key.downcase.to_sym ] = meta['value'] } diff --git a/app/models/channel/filter/identify_sender.rb b/app/models/channel/filter/identify_sender.rb index 7da0b7023..5226e4c8c 100644 --- a/app/models/channel/filter/identify_sender.rb +++ b/app/models/channel/filter/identify_sender.rb @@ -6,7 +6,7 @@ module Channel::Filter::IdentifySender customer_user_id = mail[ 'x-zammad-ticket-customer_id'.to_sym ] customer_user = nil - if !customer_user_id.empty? + if customer_user_id.present? customer_user = User.lookup(id: customer_user_id) if customer_user Rails.logger.debug "Took customer form x-zammad-ticket-customer_id header '#{customer_user_id}'." @@ -16,10 +16,10 @@ module Channel::Filter::IdentifySender end # check if sender exists in database - if !customer_user && !mail[ 'x-zammad-customer-login'.to_sym ].empty? + if !customer_user && mail[ 'x-zammad-customer-login'.to_sym ].present? customer_user = User.find_by(login: mail[ 'x-zammad-customer-login'.to_sym ]) end - if !customer_user && !mail[ 'x-zammad-customer-email'.to_sym ].empty? + if !customer_user && mail[ 'x-zammad-customer-email'.to_sym ].present? customer_user = User.find_by(email: mail[ 'x-zammad-customer-email'.to_sym ]) end if !customer_user @@ -64,7 +64,7 @@ module Channel::Filter::IdentifySender # find session user session_user_id = mail[ 'x-zammad-session-user-id'.to_sym ] session_user = nil - if !session_user_id.empty? + if session_user_id.present? session_user = User.lookup(id: session_user_id) if session_user Rails.logger.debug "Took session form x-zammad-session-user-id header '#{session_user_id}'." diff --git a/app/models/channel/filter/trusted.rb b/app/models/channel/filter/trusted.rb index 69d78aff8..74fdf0515 100644 --- a/app/models/channel/filter/trusted.rb +++ b/app/models/channel/filter/trusted.rb @@ -11,7 +11,17 @@ module Channel::Filter::Trusted next if key !~ /^x-zammad/i mail.delete(key) } + return end + # verify values + mail.each { |key, value| + next if key !~ /^x-zammad/i + + # no assoc exists, remove header + next if Channel::EmailParser.check_attributes_by_x_headers(key, value) + mail.delete(key.to_sym) + } + end end diff --git a/app/models/observer/ticket/article/communicate_email/background_job.rb b/app/models/observer/ticket/article/communicate_email/background_job.rb index 888ffb903..3d2c637b3 100644 --- a/app/models/observer/ticket/article/communicate_email/background_job.rb +++ b/app/models/observer/ticket/article/communicate_email/background_job.rb @@ -69,14 +69,7 @@ class Observer::Ticket::Article::CommunicateEmail::BackgroundJob record.save! # store mail plain - Store.add( - object: 'Ticket::Article::Mail', - o_id: record.id, - data: message.to_s, - filename: "ticket-#{ticket.number}-#{record.id}.eml", - preferences: {}, - created_by_id: record.created_by_id, - ) + record.save_as_raw(message.to_s) # add history record recipient_list = '' diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 5ad23f19c..6ea7ddba2 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -49,6 +49,7 @@ class Ticket < ApplicationModel last_contact_at: true, last_contact_agent_at: true, last_contact_customer_at: true, + preferences: true, } ) @@ -414,7 +415,7 @@ get count of tickets and tickets which match on selector generate condition query to search for tickets based on condition - query_condition, bind_condition = selector2sql(params[:condition], current_user) + query_condition, bind_condition, tables = selector2sql(params[:condition], current_user) condition example @@ -879,7 +880,8 @@ result return if !customer_id - customer = User.find(customer_id) + customer = User.find_by(id: customer_id) + return if !customer return if organization_id == customer.organization_id self.organization_id = customer.organization_id diff --git a/app/models/ticket/article.rb b/app/models/ticket/article.rb index a152563e5..f859ac11d 100644 --- a/app/models/ticket/article.rb +++ b/app/models/ticket/article.rb @@ -164,6 +164,54 @@ get body as text with quote sign "> " at the beginning of each line body_as_text.word_wrap.message_quote end +=begin + +get article as raw (e. g. if it's a email, the raw email) + + article = Ticket::Article.find(123) + article.as_raw + +returns: + + file # Store + +=end + + def as_raw + list = Store.list( + object: 'Ticket::Article::Mail', + o_id: id, + ) + return if !list + return if list.empty? + return if !list[0] + list[0] + end + +=begin + +save article as raw (e. g. if it's a email, the raw email) + + article = Ticket::Article.find(123) + article.save_as_raw(msg) + +returns: + + file # Store + +=end + + def save_as_raw(msg) + Store.add( + object: 'Ticket::Article::Mail', + o_id: id, + data: msg, + filename: "ticket-#{ticket.number}-#{id}.eml", + preferences: {}, + created_by_id: created_by_id, + ) + end + private # strip not wanted chars diff --git a/test/unit/email_postmaster_test.rb b/test/unit/email_postmaster_test.rb index c744a20ea..e17fa6b44 100644 --- a/test/unit/email_postmaster_test.rb +++ b/test/unit/email_postmaster_test.rb @@ -270,6 +270,130 @@ Some Text" PostmasterFilter.destroy_all + PostmasterFilter.create( + name: 'used', + match: { + from: { + operator: 'contains', + value: 'me@example.com', + }, + }, + perform: { + 'X-Zammad-Ticket-group_id' => { + value: group1.id, + }, + 'x-Zammad-Article-Internal' => { + value: true, + }, + 'x-Zammad-Ticket-customer_id' => { + value: '', + value_completion: '', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: ME Bob +To: customer@example.com +Subject: some subject + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + assert_equal(group1.name, ticket.group.name) + assert_equal('2 normal', ticket.priority.name) + assert_equal('some subject', ticket.title) + assert_equal('me@example.com', ticket.customer.email) + + PostmasterFilter.destroy_all + PostmasterFilter.create( + name: 'used', + match: { + from: { + operator: 'contains', + value: 'me@example.com', + }, + }, + perform: { + 'X-Zammad-Ticket-group_id' => { + value: group1.id, + }, + 'x-Zammad-Article-Internal' => { + value: true, + }, + 'x-Zammad-Ticket-customer_id' => { + value: 999_999, + value_completion: 'xxx', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: ME Bob +To: customer@example.com +Subject: some subject + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + assert_equal(group1.name, ticket.group.name) + assert_equal('2 normal', ticket.priority.name) + assert_equal('some subject', ticket.title) + assert_equal('me@example.com', ticket.customer.email) + + PostmasterFilter.destroy_all + PostmasterFilter.create( + name: 'used', + match: { + from: { + operator: 'contains', + value: 'me@example.com', + }, + }, + perform: { + 'X-Zammad-Ticket-group_id' => { + value: group1.id, + }, + 'X-Zammad-Ticket-priority_id' => { + value: 888_888, + }, + 'x-Zammad-Article-Internal' => { + value: true, + }, + 'x-Zammad-Ticket-customer_id' => { + value: 999_999, + value_completion: 'xxx', + }, + }, + channel: 'email', + active: true, + created_by_id: 1, + updated_by_id: 1, + ) + + data = 'From: ME Bob +To: customer@example.com +Subject: some subject + +Some Text' + + parser = Channel::EmailParser.new + ticket, article, user = parser.process({ group_id: group_default.id, trusted: false }, data) + assert_equal(group1.name, ticket.group.name) + assert_equal('2 normal', ticket.priority.name) + assert_equal('some subject', ticket.title) + assert_equal('me@example.com', ticket.customer.email) + assert_equal('2 normal', ticket.priority.name) + + PostmasterFilter.destroy_all end end diff --git a/test/unit/email_process_test.rb b/test/unit/email_process_test.rb index ecd4d192f..489001af4 100644 --- a/test/unit/email_process_test.rb +++ b/test/unit/email_process_test.rb @@ -5,7 +5,7 @@ require 'test_helper' class EmailProcessTest < ActiveSupport::TestCase test 'process simple' do Ticket.destroy_all - + files = [ { data: 'From: me@example.com @@ -2018,8 +2018,9 @@ Some Text', success: false, }, ] - process(files) + assert_process(files) end + test 'process trusted' do files = [ { @@ -2062,8 +2063,36 @@ Some Text', }, }, }, + { + data: 'From: me@example.com +To: customer@example.com +Subject: some subject +X-Zammad-Ticket-Followup-State: closed +X-Zammad-Ticket-priority_id: 777777 +X-Zammad-Article-sender_id: 999999 +x-Zammad-Article-type: phone +x-Zammad-Article-Internal: true + +Some Text', + channel: { + trusted: true, + }, + success: true, + result: { + 0 => { + state: 'new', + priority: '2 normal', + title: 'some subject', + }, + 1 => { + sender: 'Customer', + type: 'phone', + internal: true, + }, + }, + }, ] - process(files) + assert_process(files) end test 'process not trusted' do @@ -2097,7 +2126,7 @@ Some Text', }, }, ] - process(files) + assert_process(files) end test 'process inactive group - a' do @@ -2133,7 +2162,7 @@ Some Text', }, }, ] - process(files) + assert_process(files) end test 'process inactive group - b' do @@ -2167,7 +2196,7 @@ Some Text', }, }, ] - process(files) + assert_process(files) Group.all.each {|group| next if !group_active_map.key?(group.id) @@ -2176,7 +2205,7 @@ Some Text', } end - def process(files) + def assert_process(files) files.each { |file| result = Channel::EmailParser.new.process(file[:channel]||{}, file[:data], false) if file[:success] @@ -2215,7 +2244,7 @@ Some Text', end end else - assert(false, 'ticket not created', file) + assert(false, 'ticket not created') end elsif !file[:success] if result && result.class == Array && result[1]