Kimirock/osTicket
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fetcher: Mail Fetcher / Parser Error Handling

Browse Source 
Prior to osTicket `v1.17.x` email fetching required and used php-imap module to
decode and parse emails, whereas locally or remotely piped emails (via API
endpoint) used a Raw Email Parser ("Mail Parser").

Using two separate backends meant duplicated code, logic and inconsistent email
processing.

Starting with osTicket `v1.17.0`, we overhauled the Email Backend to OAuth2
support and in the process we switched toxusing a unified Mail Parser for both
fetched and piped emails. To determined how the email got delivered in order to
handle errors for the interfaces accodingly, we relied on `PHP_SAP`I /
`php_sapi_name()`.

That was shortsighted because of the value of `PHP_SAPI` can be different in
the following cases;

1. Email Fetching can be triggered by autocron (Staff Activity) or by a remote
task (API)

2. `PHP_SAPI` cannot be relied on for php installations using php-fpm, php-cgi
or custom binaries for php CLI executable

3. Cron Job / Task entry "user-name" or "command path" might use separate php
executables (related to `#2`)

4. Triggering cron/php via php-fpm socket connection e.g using FastCGI Client

Since emails are fetched in batches - Email Fetching, unlike stateless API
requests, should be considered a CLI operation regardless of how it gets
trigerred, this necessary so the system can gracefully handle fetch and parse
errors.

This commit does just that and a few other minor logic / code cleanup.

~ Peter - with love!
This commit is contained in:
Peter Rotich
2022-11-18 10:14:38 +00:00
parent 8fbc7eea6a
commit d4d9c4242f
7 changed files with 222 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
api/pipe.php
View File

@@ -23,5 +23,5 @@ if (!osTicket::is_cli())
die(__('pipe.php only supports local piping - use http -> api/tickets.email'));
require_once(INCLUDE_DIR.'api.tickets.php');
PipeApiController::process();
PipeApiController::process('cli');
?>

25
include/api.cron.php
View File

@@ -3,7 +3,6 @@
include_once INCLUDE_DIR.'class.cron.php';
class CronApiController extends ApiController {
function execute() {
if (!($key=$this->requireApiKey()) || !$key->canExecuteCron())
@@ -12,26 +11,32 @@ class CronApiController extends ApiController {
$this->run();
}
/* private */
function run() {
global $ost;
private function run() {
Cron::run();
$ost->logDebug(__('Cron Job'),__('Cron job executed').' ['.$_SERVER['REMOTE_ADDR'].']');
// TODO: Add elapsed time to the debug log
$this->debug(__('Cron Job'),
sprintf('%s [%s]', __('Cron job executed'), $this->getRemoteAddr()));
$this->response(200,'Completed');
}
}
class LocalCronApiController extends CronApiController {
function response($code, $resp) {
protected function isCli() {
return true;
}
protected function getRemoteAddr() {
// Local Cron doesn't have IP Addr set
return 'CLI';
}
protected function response($code, $response) {
if ($code == 200) //Success - exit silently.
exit(0);
//On error echo the response (error)
echo $resp;
echo $response;
exit(1);
}

113
include/api.tickets.php
View File

@@ -110,17 +110,20 @@ class TicketApiController extends ApiController {
$ticket = null;
if (!strcasecmp($format, 'email')) {
# Handle remote piped emails - could be a reply...etc.
$ticket = $this->processEmail();
// Process remotely piped emails - could be a reply...etc.
$ticket = $this->processEmailRequest();
} else {
# Parse request body
$ticket = $this->createTicket($this->getRequest($format));
// Get and Parse request body data for the format
$ticket = $this->createTicket($this->getEmailRequest($format));
}
if(!$ticket)
return $this->exerr(500, __("Unable to create new ticket: unknown error"));
if ($ticket)
$this->response(201, $ticket->getNumber());
else
$this->exerr(500, _S("unknown error"));
}
/* private helper functions */
@@ -131,37 +134,41 @@ class TicketApiController extends ApiController {
$alert = (bool) (isset($data['alert']) ? $data['alert'] : true);
$autorespond = (bool) (isset($data['autorespond']) ? $data['autorespond'] : true);
# Assign default value to source if not defined, or defined as NULL
$data['source'] = $data['source'] ?? $source;
// Assign default value to source if not defined, or defined as NULL
$data['source'] ??= $source;
# Create the ticket with the data (attempt to anyway)
// Create the ticket with the data (attempt to anyway)
$errors = array();
$ticket = Ticket::create($data, $errors, $data['source'], $autorespond, $alert);
# Return errors (?)
if (count($errors)) {
if(isset($errors['errno']) && $errors['errno'] == 403) {
// If CLI then throw a TicketDenied Exception. Mail Fetcher
// will handle logging the message as needed
if (PHP_SAPI == 'cli') {
$msg = sprintf("%s: %s\n\n%s",
__('Ticket denied'),
$data['email'],
$errors['err']);
throw new TicketDenied($msg);
}
return $this->exerr(403, __('Ticket denied'));
} else
return $this->exerr(
400,
__("Unable to create new ticket: validation errors").":\n"
.Format::array_implode(": ", "\n", $errors)
);
} elseif (!$ticket) {
return $this->exerr(500, __("Unable to create new ticket: unknown error"));
}
if (($ticket = Ticket::create($data, $errors, $data['source'],
$autorespond, $alert)) && !$errors)
return $ticket;
// Ticket create failed Bigly - got errors?
$title = null;
// Got errors?
if (count($errors)) {
// Ticket denied? Say so loudly so it can standout from generic
// validation errors
if (isset($errors['errno']) && $errors['errno'] == 403) {
$title = _S('Ticket denied');
$error = sprintf("%s: %s\n\n%s",
$title, $data['email'], $errors['err']);
} else {
// unpack the errors
$error = Format::array_implode("\n", "\n", $errors);
}
} else {
// unknown reason - default
$error = _S('unknown error');
}
$error = sprintf('%s :%s',
_S('Unable to create new ticket'), $error);
return $this->exerr(500, $error, $title);
}
function processEmailRequest() {
return $this->processEmail();
}
function processEmail($data=false) {
@@ -200,15 +207,22 @@ class TicketApiController extends ApiController {
// be created via the web interface or the API
try {
return $this->createTicket($data, 'Email');
} catch (TicketDenied $ex) {
// Log the mid so we don't process this email again!
} catch (TicketApiError $err) {
// Check if the ticket was denied by a filter or banlist
if ($err->isDenied() && $data['mid']) {
// We need to log the Message-Id (mid) so we don't
// process the same email again in subsequent fetches
$entry = new ThreadEntry();
$entry->logEmailHeaders(0, $data['mid']);
// rethrow the exception
throw $ex;
// throw TicketDenied exception so the caller can handle it
// accordingly
throw new TicketDenied($err->getMessage());
} else {
// otherwise rethrow this bad baby as it is!
throw $err;
}
}
}
}
//Local email piping controller - no API key required!
@@ -217,7 +231,8 @@ class PipeApiController extends TicketApiController {
// Overwrite grandparent's (ApiController) response method.
function response($code, $resp) {
//Use postfix exit codes - instead of HTTP
// It's important to use postfix exit codes for local piping instead
// of HTTP's so the piping script can process them accordingly
switch($code) {
case 201: //Success
$exitcode = 0;
@@ -242,20 +257,30 @@ class PipeApiController extends TicketApiController {
default: //Temp (unknown) failure - retry
$exitcode = 75;
}
//echo "$code ($exitcode):$resp";
//We're simply exiting - MTA will take care of the rest based on exit code!
exit($exitcode);
}
static function process() {
$pipe = new PipeApiController();
static function process($sapi=null) {
$pipe = new PipeApiController($sapi);
if (($ticket=$pipe->processEmail()))
return $pipe->response(201,
is_object($ticket) ? $ticket->getNumber() : $ticket);
return $pipe->exerr(416, __('Request failed - retry again!'));
}
static function local() {
return self::process('cli');
}
}
class TicketApiError extends Exception {
// Check if exception is because of denial
public function isDenied() {
return ($this->getCode() === 403);
}
}
class TicketDenied extends Exception {}

11
include/class.ajax.php
View File

@@ -24,14 +24,19 @@ require_once (INCLUDE_DIR.'class.api.php');
* call controller should inherit from this class in order to maintain
* consistency.
*/
class AjaxController extends Controller {
class AjaxController extends ApiController {
function staffOnly() {
global $thisstaff;
if (!$thisstaff || !$thisstaff->isValid()) {
Http::response(401,sprintf(__('Access Denied. IP %s'),$_SERVER['REMOTE_ADDR']));
$this->exerr(401, sprintf('%s (%s)',
__('Access Denied'), $_SERVER['REMOTE_ADDR']));
}
return true;
}
/**
* Convert a PHP array into a JSON-encoded string
*/
@@ -44,6 +49,6 @@ class AjaxController extends Controller {
}
function get($var, $default=null) {
return (isset($_GET[$var])) ? $_GET[$var] : $default;
return $_GET[$var] ?: $default;
}
}

135
include/class.api.php
View File

@@ -169,26 +169,49 @@ class API {
class ApiController extends Controller {
var $apikey;
private $sapi;
private $key;
public function __construct($sapi = null) {
// Set API Interface the request is coming from
$this->sapi = $sapi ?: (osTicket::is_cli() ? 'cli' : 'http');
}
public function isCli() {
return (strcasecmp($this->sapi, 'cli') === 0);
}
private function getInputStream() {
return $this->isCli() ? 'php://stdin' : 'php://input';
}
protected function getRemoteAddr() {
return $_SERVER['REMOTE_ADDR'];
}
protected function getApiKey() {
return $_SERVER['HTTP_X_API_KEY'];
}
function requireApiKey() {
# Validate the API key -- required to be sent via the X-API-Key
# header
if(!($key=$this->getApiKey()))
// Require a valid API key sent as X-API-Key HTTP header
// see getApiKey method.
if (!($key=$this->getKey()))
return $this->exerr(401, __('Valid API key required'));
elseif (!$key->isActive() || $key->getIPAddr()!=$_SERVER['REMOTE_ADDR'])
elseif (!$key->isActive() || $key->getIPAddr() != $this->getRemoteAddr())
return $this->exerr(401, __('API key not found/active or source IP not authorized'));
return $key;
}
function getApiKey() {
function getKey() {
// Lookup record using sent API Key && IP Addr
if (!$this->key
&& ($key=$this->getApiKey())
&& ($ip=$this->getRemoteAddr()))
$this->key = API::lookupByKey($key, $ip);
if (!$this->apikey && isset($_SERVER['HTTP_X_API_KEY']) && isset($_SERVER['REMOTE_ADDR']))
$this->apikey = API::lookupByKey($_SERVER['HTTP_X_API_KEY'], $_SERVER['REMOTE_ADDR']);
return $this->apikey;
return $this->key;
}
/**
@@ -197,15 +220,19 @@ class ApiController extends Controller {
* work will be done for XML requests
*/
function getRequest($format, $validate=true) {
$input = osTicket::is_cli()?'php://stdin':'php://input';
$input = $this->getInputStream();
if (!($stream = @fopen($input, 'r')))
$this->exerr(400, __("Unable to read request body"));
$this->exerr(400, sprintf('%s (%s)',
__("Unable to read request body"), $input));
return $this->parseRequest($stream, $format, $validate);
}
function getEmailRequest() {
return $this->getRequest('email', false);
if (!($data=$this->getRequest('email', false)))
$this->exerr(400, __("Unable to read email request"));
return $data;
}
function parseRequest($stream, $format, $validate=true) {
@@ -213,7 +240,7 @@ class ApiController extends Controller {
switch(strtolower($format)) {
case 'xml':
if (!function_exists('xml_parser_create'))
$this->exerr(501, __('XML extension not supported'));
return $this->exerr(501, __('XML extension not supported'));
$parser = new ApiXmlDataParser();
break;
case 'json':
@@ -228,7 +255,6 @@ class ApiController extends Controller {
if (!($data = $parser->parse($stream))) {
$this->exerr(400, $parser->lastError());
throw new Exception($parser->lastError());
}
//Validate structure of the request.
@@ -265,14 +291,11 @@ class ApiController extends Controller {
} elseif (in_array($key, $structure)) {
continue;
}
if ($strict)
return $this->exerr(400, sprintf(__("%s: Unexpected data received in API request"), "$prefix$key"));
else
$ost->logWarning(__('API Unexpected Data'),
sprintf(__("%s: Unexpected data received in API request"), "$prefix$key"),
false);
}
$error = sprintf(__("%s: Unexpected data received in API request"),
"$prefix$key");
$this->onError(400, $error, __('Unexpected Data'), !$strict);
}
return true;
}
@@ -288,34 +311,66 @@ class ApiController extends Controller {
$strict);
}
protected function debug($subj, $msg) {
return $this->log($subj, $msg, LOG_DEBUG);
}
protected function logError($subj, $msg, $fatal=false) {
// If error is not fatal then log it as a warning
return $this->log($subj, $msg, $fatal ? LOG_ERR : LOG_WARN);
}
protected function log($title, $msg, $level = LOG_WARN) {
global $ost;
switch ($level) {
case LOG_WARN:
case LOG_ERR:
// We are disabling email alerts on API errors / warnings
// due to potential abuse or loops that might cause email DOS
$ost->log($level, $title, $msg, false);
break;
case LOG_DEBUG:
default:
$ost->logDebug($title, $msg);
}
return true;
}
/**
* API error & logging and response!
*
* final - don not override downstream.
*/
final public function onError($code, $error, $title = null,
$logOnly = false) {
/* If possible - DO NOT - overwrite the method downstream */
function exerr($code, $error='') {
global $ost;
// Unpack the errors to string if error is an array
if ($error && is_array($error))
$error = Format::array_implode(": ", "\n", $error);
//Log the error as a warning - include api key if available.
// Log the error
$msg = $error;
if($_SERVER['HTTP_X_API_KEY'])
$msg.="\n*[".$_SERVER['HTTP_X_API_KEY']."]*\n";
$ost->logWarning(__('API Error')." ($code)", $msg, false);
// TODO: Only include API Key when in debug mode to avoid
// potentialialy leaking a valid key in system logs
if (($key=$this->getApiKey()))
$msg .= "\n[$key]\n";
if (PHP_SAPI != 'cli') {
$this->response($code, $error); //Responder should exit...
}
return false;
}
$title = sprintf('%s (%s)', $title ?: __('API Error'), $code);
$this->logError($title, $msg, $logOnly);
//Default response method - can be overwritten in subclasses.
function response($code, $resp) {
Http::response($code, $resp);
exit();
// If the error is not deemed fatal then simply return
if ($logOnly)
return;
// If the API Interface is CLI then throw a TicketApiError exception
// so the caller can handle the error gracefully.
// Note that we set the error code as well
if ($this->isCli())
throw new TicketApiError($error, $code);
// Respond and exit since HTTP endpoint requests
// are considered stateless
$this->response($code, $error);
}
}

39
include/class.controller.php
View File

@@ -23,33 +23,22 @@ abstract class Controller {
return true;
}
/**
* error & logging and response!
*
*/
function exerr($code, $error='') {
global $ost;
if ($error && is_array($error))
$error = Format::array_implode(": ", "\n", $error);
//Log the error as a warning - include api key if available.
$msg = $error;
if ($_SERVER['HTTP_X_API_KEY'])
$msg.="\n*[".$_SERVER['HTTP_X_API_KEY']."]*\n";
$ost->logWarning(__('Error')." ($code)", $msg, false);
if (PHP_SAPI == 'cli') {
fwrite(STDERR, "({$code}) $error\n");
} else {
$this->response($code, $error); //Responder should exit...
}
return false;
// Wrapper for onFatalError
public function exerr($code, $error='') {
return $this->onFatalError($code, $error);
}
//Default response method - can be overwritten in subclasses.
function response($code, $resp) {
Http::response($code, $resp);
public function onFatalError($code, $error) {
$this->onError($code, $error);
// On error should exit but we're making doubly sure
$this->response($code, $error);
exit();
}
protected function response($code, $response) {
Http::response($code, $response);
exit();
}
abstract function onError($code, $error, $title=null, $logOnly=false);
}

10
include/class.mailfetch.php
View File

@@ -56,9 +56,17 @@ class Fetcher {
return $this->account->canDeleteEmails();
}
function getTicketsApi() {
// We're forcing CLI interface - this is absolutely necessary since
// Email Fetching is considered a CLI operation regardless of how
// it's triggered (cron job / task or autocron)
// Please note that PHP_SAPI cannot be trusted for installations
// using php-fpm or php-cgi binaries for php CLI executable.
if (!isset($this->api))
$this->api = new \TicketApiController();
$this->api = new \TicketApiController('cli');
return $this->api;
}