From 9fc4d080418a000a0cdec1df51effcb6de10be1b Mon Sep 17 00:00:00 2001
From: Jared Hancock <jared@osticket.com>
Date: Wed, 27 Aug 2014 14:47:43 -0500
Subject: [PATCH] forms: Unify attachment settings
Attachment settings are now exclusively handled by the settings on the
ThreadEntry field on the ticket details form. Enabling attachments as well
as tuning attachment validation settings are all handled from the one place
now.
---
include/ajax.draft.php | 21 ++++++++-
include/ajax.forms.php | 4 +-
include/api.tickets.php | 39 +++++++++--------
include/class.file.php | 13 +-----
include/class.forms.php | 87 ++++++++++++++++++++++++++++++++++---
include/class.mailfetch.php | 27 ++++++++----
include/class.osticket.php | 18 --------
include/class.thread.php | 6 ++-
8 files changed, 149 insertions(+), 66 deletions(-)
diff --git a/include/ajax.draft.php b/include/ajax.draft.php
index e24be81ef..41fde2be2 100644
--- a/include/ajax.draft.php
+++ b/include/ajax.draft.php
@@ -67,6 +67,8 @@ class DraftAjaxAPI extends AjaxController {
}
function _uploadInlineImage($draft) {
+ global $cfg;
+
if (!isset($_POST['data']) && !isset($_FILES['file']))
Http::response(422, "File not included properly");
@@ -76,9 +78,26 @@ class DraftAjaxAPI extends AjaxController {
$_FILES['image'][$k] = array($v);
unset($_FILES['file']);
- $file = AttachmentFile::format($_FILES['image'], true);
+ $file = AttachmentFile::format($_FILES['image']);
# TODO: Detect unacceptable attachment extension
# TODO: Verify content-type and check file-content to ensure image
+ $type = $file[0]['type'];
+ if (strpos($file[0]['type'], 'image/') !== 0)
+ return Http::response(403,
+ JsonDataEncoder::encode(array(
+ 'error' => 'File type is not allowed',
+ ))
+ );
+
+ # TODO: Verify file size is acceptable
+ if ($file[0]['size'] > $cfg->getMaxFileSize())
+ return Http::response(403,
+ JsonDataEncoder::encode(array(
+ 'error' => 'File is too large',
+ ))
+ );
+
+
if (!($ids = $draft->attachments->upload($file))) {
if ($file[0]['error']) {
return Http::response(403,
diff --git a/include/ajax.forms.php b/include/ajax.forms.php
index da6bb6560..070606298 100644
--- a/include/ajax.forms.php
+++ b/include/ajax.forms.php
@@ -92,14 +92,14 @@ class DynamicFormsAjaxAPI extends AjaxController {
Http::response(400, 'Upload to a non file-field');
return JsonDataEncoder::encode(
- array('id'=>$impl->upload())
+ array('id'=>$impl->ajaxUpload())
);
}
function attach() {
$field = new FileUploadField();
return JsonDataEncoder::encode(
- array('id'=>$field->upload())
+ array('id'=>$field->ajaxUpload(true))
);
}
}
diff --git a/include/api.tickets.php b/include/api.tickets.php
index ae0450a2d..47d978a49 100644
--- a/include/api.tickets.php
+++ b/include/api.tickets.php
@@ -60,30 +60,33 @@ class TicketApiController extends ApiController {
if(!parent::validate($data, $format, $strict) && $strict)
$this->exerr(400, __('Unexpected or invalid data received'));
- //Nuke attachments IF API files are not allowed.
- if(!$ost->getConfig()->allowAPIAttachments())
+ // Use the settings on the thread entry on the ticket details
+ // form to validate the attachments in the email
+ $tform = TicketForm::objects()->one()->getForm();
+ $messageField = $tform->getField('message');
+ $fileField = $messageField->getWidget()->getAttachments();
+
+ // Nuke attachments IF API files are not allowed.
+ if (!$messageField->isAttachmentsEnabled())
$data['attachments'] = array();
//Validate attachments: Do error checking... soft fail - set the error and pass on the request.
- if($data['attachments'] && is_array($data['attachments'])) {
- foreach($data['attachments'] as &$attachment) {
- if(!$ost->isFileTypeAllowed($attachment))
- $attachment['error'] = sprintf(__('Invalid file type (ext) for %s'),Format::htmlchars($attachment['name']));
- elseif ($attachment['encoding'] && !strcasecmp($attachment['encoding'], 'base64')) {
- if(!($attachment['data'] = base64_decode($attachment['data'], true)))
- $attachment['error'] = sprintf(__('%s: Poorly encoded base64 data'), Format::htmlchars($attachment['name']));
+ if ($data['attachments'] && is_array($data['attachments'])) {
+ foreach($data['attachments'] as &$file) {
+ if ($file['encoding'] && !strcasecmp($file['encoding'], 'base64')) {
+ if(!($file['data'] = base64_decode($file['data'], true)))
+ $file['error'] = sprintf(__('%s: Poorly encoded base64 data'),
+ Format::htmlchars($file['name']));
}
- if (!$attachment['error']
- && ($size = $ost->getConfig()->getMaxFileSize())
- && ($fsize = $attachment['size'] ?: strlen($attachment['data']))
- && $fsize > $size) {
- $attachment['error'] = sprintf('File %s (%s) is too big. Maximum of %s allowed',
- Format::htmlchars($attachment['name']),
- Format::file_size($fsize),
- Format::file_size($size));
+ // Validate and save immediately
+ try {
+ $file['id'] = $fileField->uploadAttachment($file);
+ }
+ catch (FileUploadError $ex) {
+ $file['error'] = $file['name'] . ': ' . $ex->getMessage();
}
}
- unset($attachment);
+ unset($file);
}
return true;
diff --git a/include/class.file.php b/include/class.file.php
index 702936c7d..15f4ea17a 100644
--- a/include/class.file.php
+++ b/include/class.file.php
@@ -534,9 +534,8 @@ class AttachmentFile {
/*
Method formats http based $_FILE uploads - plus basic validation.
- @restrict - make sure file type & size are allowed.
*/
- function format($files, $restrict=false) {
+ function format($files) {
global $ost;
if(!$files || !is_array($files))
@@ -562,16 +561,6 @@ class AttachmentFile {
$file['error'] = 'File upload error #'.$file['error'];
elseif(!$file['tmp_name'] || !is_uploaded_file($file['tmp_name']))
$file['error'] = 'Invalid or bad upload POST';
- elseif($restrict) { // make sure file type & size are allowed.
- if(!$ost->isFileTypeAllowed($file))
- $file['error'] = 'Invalid file type for '.Format::htmlchars($file['name']);
- elseif($ost->getConfig()->getMaxFileSize()
- && $file['size']>$ost->getConfig()->getMaxFileSize())
- $file['error'] = sprintf('File %s (%s) is too big. Maximum of %s allowed',
- Format::htmlchars($file['name']),
- Format::file_size($file['size']),
- Format::file_size($ost->getConfig()->getMaxFileSize()));
- }
}
unset($file);
diff --git a/include/class.forms.php b/include/class.forms.php
index 790a48be9..b994c738f 100644
--- a/include/class.forms.php
+++ b/include/class.forms.php
@@ -997,12 +997,17 @@ class ThreadEntryField extends FormField {
'label'=>__('Enable Attachments'),
'default'=>$cfg->allowOnlineAttachments(),
'configuration'=>array(
- 'desc'=>__('Enables attachments submitted with the issue details'),
+ 'desc'=>__('Enables attachments on tickets, regardless of channel'),
),
)),
)
+ $attachments->getConfigurationOptions();
}
+
+ function isAttachmentsEnabled() {
+ $config = $this->getConfiguration();
+ return $config['attachments'];
+ }
}
class PriorityField extends ChoiceField {
@@ -1278,7 +1283,10 @@ class FileUploadField extends FormField {
);
}
- function upload() {
+ /**
+ * Called from the ajax handler for async uploads via web clients.
+ */
+ function ajaxUpload($bypass=false) {
$config = $this->getConfiguration();
$files = AttachmentFile::format($_FILES['upload'],
@@ -1289,8 +1297,12 @@ class FileUploadField extends FormField {
$file = array_shift($files);
$file['name'] = urldecode($file['name']);
- // TODO: Check allowed type / size.
- // Return HTTP/413, 415, 417 or similar
+ if (!$bypass && !$this->isValidFileType($file['name']))
+ Http::response(415, 'File type is not allowed');
+
+ $config = $this->getConfiguration();
+ if (!$bypass && $file['size'] > $config['size'])
+ Http::response(413, 'File is too large');
if (!($id = AttachmentFile::upload($file)))
Http::response(500, 'Unable to store file: '. $file['error']);
@@ -1298,6 +1310,65 @@ class FileUploadField extends FormField {
return $id;
}
+ /**
+ * Called from FileUploadWidget::getValue() when manual upload is used
+ * for browsers which do not support the HTML5 way of uploading async.
+ */
+ function uploadFile($file) {
+ if (!$this->isValidFileType($file['name']))
+ throw new FileUploadError(__('File type is not allowed'));
+
+ $config = $this->getConfiguration();
+ if ($file['size'] > $config['size'])
+ throw new FileUploadError(__('File size is too large'));
+
+ return AttachmentFile::upload($file);
+ }
+
+ /**
+ * Called from API and email routines and such to handle attachments
+ * sent other than via web upload
+ */
+ function uploadAttachment(&$file) {
+ if (!$this->isValidFileType($file['name']))
+ throw new FileUploadError(__('File type is not allowed'));
+
+ if (is_callable($file['data']))
+ $file['data'] = $file['data']();
+ if (!isset($file['size'])) {
+ // bootstrap.php include a compat version of mb_strlen
+ if (extension_loaded('mbstring'))
+ $file['size'] = mb_strlen($file['data'], '8bit');
+ else
+ $file['size'] = strlen($file['data']);
+ }
+
+ $config = $this->getConfiguration();
+ if ($file['size'] > $config['size'])
+ throw new FileUploadError(__('File size is too large'));
+
+ if (!$id = AttachmentFile::save($file))
+ throw new FileUploadError(__('Unable to save file'));
+
+ return $id;
+ }
+
+ function isValidFileType($name, $type=false) {
+ $config = $this->getConfiguration();
+
+ // Return true if all file types are allowed (.*)
+ if (strpos($config['extensions'], '.*') || !$config['extensions'])
+ return true;
+
+ $allowed = array_map('trim', explode(',', strtolower($config['extensions'])));
+
+ $ext = strtolower(pathinfo($name, PATHINFO_EXTENSION));
+
+ //TODO: Check MIME type - file ext. shouldn't be solely trusted.
+
+ return ($ext && is_array($allowed) && in_array(".$ext", $allowed));
+ }
+
function getFiles() {
if (!isset($this->attachments) && ($a = $this->getAnswer())
&& ($e = $a->getEntry()) && ($e->get('id'))
@@ -1785,8 +1856,10 @@ class FileUploadWidget extends Widget {
// Handle manual uploads (IE<10)
if ($_SERVER['REQUEST_METHOD'] == 'POST' && isset($_FILES[$this->name])) {
foreach (AttachmentFile::format($_FILES[$this->name]) as $file) {
- if ($id = AttachmentFile::upload($file))
- $ids[] = $id;
+ try {
+ $ids[] = $this->field->uploadFile($file);
+ }
+ catch (FileUploadError $ex) {}
}
return array_merge($ids, parent::getValue() ?: array());
}
@@ -1798,4 +1871,6 @@ class FileUploadWidget extends Widget {
}
}
+class FileUploadError extends Exception {}
+
?>
diff --git a/include/class.mailfetch.php b/include/class.mailfetch.php
index 105b5438d..45758c75c 100644
--- a/include/class.mailfetch.php
+++ b/include/class.mailfetch.php
@@ -665,8 +665,14 @@ class MailFetcher {
$errors=array();
$seen = false;
+ // Use the settings on the thread entry on the ticket details
+ // form to validate the attachments in the email
+ $tform = TicketForm::objects()->one()->getForm();
+ $messageField = $tform->getField('message');
+ $fileField = $messageField->getWidget()->getAttachments();
+
// Fetch attachments if any.
- if($ost->getConfig()->allowEmailAttachments()) {
+ if ($messageField->isAttachmentsEnabled()) {
// Include TNEF attachments in the attachments list
if ($this->tnef) {
foreach ($this->tnef->attachments as $at) {
@@ -680,15 +686,11 @@ class MailFetcher {
}
}
$vars['attachments'] = array();
- foreach($attachments as $a ) {
+
+ foreach ($attachments as $a) {
$file = array('name' => $a['name'], 'type' => $a['type']);
- //Check the file type
- if(!$ost->isFileTypeAllowed($file)) {
- $file['error'] = sprintf(_S('Invalid file type (ext) for %s'),
- Format::htmlchars($file['name']));
- }
- elseif (@$a['data'] instanceof TnefAttachment) {
+ if (@$a['data'] instanceof TnefAttachment) {
$file['data'] = $a['data']->getData();
}
else {
@@ -701,6 +703,15 @@ class MailFetcher {
}
// Include the Content-Id if specified (for inline images)
$file['cid'] = isset($a['cid']) ? $a['cid'] : false;
+
+ // Validate and save immediately
+ try {
+ $file['id'] = $fileField->uploadAttachment($file);
+ }
+ catch (FileUploadError $ex) {
+ $file['error'] = $file['name'] . ': ' . $ex->getMessage();
+ }
+
$vars['attachments'][] = $file;
}
}
diff --git a/include/class.osticket.php b/include/class.osticket.php
index e3901f25d..59dc66c6c 100644
--- a/include/class.osticket.php
+++ b/include/class.osticket.php
@@ -133,24 +133,6 @@ class osTicket {
return ($token && !strcasecmp($token, $this->getLinkToken()));
}
- function isFileTypeAllowed($file, $mimeType='') {
-
- if(!$file || !($allowedFileTypes=$this->getConfig()->getAllowedFileTypes()))
- return false;
-
- //Return true if all file types are allowed (.*)
- if(trim($allowedFileTypes)=='.*') return true;
-
- $allowed = array_map('trim', explode(',', strtolower($allowedFileTypes)));
- $filename = is_array($file)?$file['name']:$file;
-
- $ext = strtolower(pathinfo($filename, PATHINFO_EXTENSION));
-
- //TODO: Check MIME type - file ext. shouldn't be solely trusted.
-
- return ($ext && is_array($allowed) && in_array(".$ext", $allowed));
- }
-
/* Replace Template Variables */
function replaceTemplateVariables($input, $vars=array()) {
diff --git a/include/class.thread.php b/include/class.thread.php
index f99c0f45f..f7751f78e 100644
--- a/include/class.thread.php
+++ b/include/class.thread.php
@@ -539,7 +539,11 @@ Class ThreadEntry {
*/
function saveAttachment(&$file) {
- if(!($fileId=is_numeric($file)?$file:AttachmentFile::save($file)))
+ if (is_numeric($file))
+ $fileId = $file;
+ elseif (is_array($file) && isset($file['id']))
+ $fileId = $file['id'];
+ elseif (!($fileId = AttachmentFile::save($file)))
return 0;
$inline = is_array($file) && @$file['inline'];
--
GitLab