From cefe6848f295a549c1f03a4fff42609712101dd8 Mon Sep 17 00:00:00 2001
From: Peter Rotich <peter@enhancesoft.com>
Date: Thu, 22 Mar 2018 12:12:19 +0000
Subject: [PATCH] Make FAQ & Pages Attachments Viewable
Commit c4579277 introduced an extra administrative security feature to
restrict files access to signed in users only, even if a user has a valid
& signed download URL. The feature, however, did not take into account
public images & files associated with FAQs and pages such as
landing/thank-you pages.
This commit addresses the shortcoming by adding a reference ID (attachment ID)
to the download/access URL, that can be used to deduce the model/object type
that the file request is associated with. The technique will allow us in the
future to enforce ACL at the file level depending on privacy settings and
the security clearance of the user (agent).
---
file.php | 37 +++++++++++++------
include/ajax.draft.php | 15 +++++---
include/class.category.php | 4 +-
include/class.faq.php | 5 ++-
include/class.file.php | 37 ++++++++++++-------
include/class.format.php | 9 +++--
include/class.forms.php | 10 +++--
include/class.page.php | 2 +-
include/client/faq.inc.php | 3 +-
.../client/templates/thread-entry.tmpl.php | 3 +-
include/client/view.inc.php | 2 +-
include/staff/faq-view.inc.php | 3 +-
.../staff/templates/thread-entries.tmpl.php | 3 +-
include/staff/templates/thread-entry.tmpl.php | 3 +-
open.php | 3 +-
15 files changed, 89 insertions(+), 50 deletions(-)
diff --git a/file.php b/file.php
index 33ffec5ff..994b77a0c 100644
--- a/file.php
+++ b/file.php
@@ -26,21 +26,34 @@ if (!$_GET['key']
Http::response(404, __('Unknown or invalid file'));
}
-// Enforce security settings
-if ($cfg->isAuthRequiredForFiles() && !$thisclient) {
- if (!($U = StaffAuthenticationBackend::getUser())) {
- // Try and determine if a staff is viewing this page
- if (strpos($_SERVER['HTTP_REFERRER'], ROOT_PATH . 'scp/') !== false) {
- $_SESSION['_staff']['auth']['dest'] =
- '/' . ltrim($_SERVER['REQUEST_URI'], '/');
- Http::redirect(ROOT_PATH.'scp/login.php');
- }
- else {
- require 'secure.inc.php';
- }
+// Get the object type the file is attached to
+$type = '';
+if ($_GET['id']
+ && ($a=$file->attachments->findFirst(array(
+ 'id' => $_GET['id']))))
+ $type = $a->type;
+
+// Enforce security settings if enabled.
+if ($cfg->isAuthRequiredForFiles()
+ // FAQ & Page files allowed without login.
+ && !in_array($type, ['P', 'F'])
+ // Check user login
+ && !$thisuser
+ // Check staff login
+ && !StaffAuthenticationBackend::getUser()
+ ) {
+
+ // Try and determine if an agent is viewing the page / file
+ if (strpos($_SERVER['HTTP_REFERRER'], ROOT_PATH . 'scp/') !== false) {
+ $_SESSION['_staff']['auth']['dest'] =
+ '/' . ltrim($_SERVER['REQUEST_URI'], '/');
+ Http::redirect(ROOT_PATH.'scp/login.php');
+ } else {
+ require 'secure.inc.php';
}
}
+
// Validate session access hash - we want to make sure the link is FRESH!
// and the user has access to the parent ticket!!
if ($file->verifySignature($_GET['signature'], $_GET['expires'])) {
diff --git a/include/ajax.draft.php b/include/ajax.draft.php
index e1bb78a04..43e7f98b8 100644
--- a/include/ajax.draft.php
+++ b/include/ajax.draft.php
@@ -133,7 +133,8 @@ class DraftAjaxAPI extends AjaxController {
'content_id' => 'cid:'.$f->getKey(),
// Return draft_id to connect the auto draft creation
'draft_id' => $draft->getId(),
- 'filelink' => $f->getDownloadUrl(false, 'inline'),
+ 'filelink' => $f->getDownloadUrl(
+ ['type' => 'D', 'deposition' => 'inline']),
));
}
@@ -339,14 +340,14 @@ class DraftAjaxAPI extends AjaxController {
&& ($object = $thread->getObject())
&& ($thisstaff->canAccess($object))
) {
- $union = ' UNION SELECT f.id, a.`type`, a.`name` FROM '.THREAD_TABLE.' t
+ $union = ' UNION SELECT f.id, a.id as aid, a.`type`, a.`name` FROM '.THREAD_TABLE.' t
JOIN '.THREAD_ENTRY_TABLE.' th ON (th.thread_id = t.id)
JOIN '.ATTACHMENT_TABLE.' a ON (a.object_id = th.id AND a.`type` = \'H\')
JOIN '.FILE_TABLE.' f ON (a.file_id = f.id)
WHERE a.`inline` = 1 AND t.id='.db_input($_GET['threadId']);
}
- $sql = 'SELECT distinct f.id, COALESCE(a.type, f.ft), a.`name` FROM '.FILE_TABLE
+ $sql = 'SELECT distinct f.id, a.id as aid, COALESCE(a.type, f.ft), a.`name` FROM '.FILE_TABLE
.' f LEFT JOIN '.ATTACHMENT_TABLE.' a ON (a.file_id = f.id)
WHERE ((a.`type` IN (\'C\', \'F\', \'T\', \'P\') AND a.`inline` = 1) OR f.ft = \'L\')'
.' AND f.`type` LIKE \'image/%\'';
@@ -354,9 +355,11 @@ class DraftAjaxAPI extends AjaxController {
Http::response(500, 'Unable to lookup files');
$files = array();
- while (list($id, $type, $name) = db_fetch_row($res)) {
- $f = AttachmentFile::lookup((int) $id);
- $url = $f->getDownloadUrl();
+ while (list($id, $aid, $type, $name) = db_fetch_row($res)) {
+ if (!($f = AttachmentFile::lookup((int) $id)))
+ continue;
+
+ $url = $f->getDownloadUrl(['id' => $aid]);
$files[] = array(
// Don't send special sizing for thread items 'cause they
// should be cached already by the client
diff --git a/include/class.category.php b/include/class.category.php
index 3eeedfd14..c4b1ff0d1 100644
--- a/include/class.category.php
+++ b/include/class.category.php
@@ -55,7 +55,9 @@ class Category extends VerySimpleModel {
return $count;
}
function getDescription() { return $this->description; }
- function getDescriptionWithImages() { return Format::viewableImages($this->description); }
+ function getDescriptionWithImages() {
+ return Format::viewableImages($this->description);
+ }
function getNotes() { return $this->notes; }
function getCreateDate() { return $this->created; }
function getUpdateDate() { return $this->updated; }
diff --git a/include/class.faq.php b/include/class.faq.php
index 4eb1c2d83..555fd3aee 100644
--- a/include/class.faq.php
+++ b/include/class.faq.php
@@ -74,7 +74,7 @@ class FAQ extends VerySimpleModel {
function getQuestion() { return $this->question; }
function getAnswer() { return $this->answer; }
function getAnswerWithImages() {
- return Format::viewableImages($this->answer);
+ return Format::viewableImages($this->answer, ['type' => 'F']);
}
function getTeaser() {
return Format::truncate(Format::striptags($this->answer), 150);
@@ -194,7 +194,8 @@ class FAQ extends VerySimpleModel {
return $this->_getLocal('answer', $lang);
}
function getLocalAnswerWithImages($lang=false) {
- return Format::viewableImages($this->getLocalAnswer($lang));
+ return Format::viewableImages($this->getLocalAnswer($lang),
+ ['type' => 'F']);
}
function _getLocal($what, $lang=false) {
if (!$lang) {
diff --git a/include/class.file.php b/include/class.file.php
index bcdb9e0ed..ed7adb936 100644
--- a/include/class.file.php
+++ b/include/class.file.php
@@ -184,25 +184,31 @@ class AttachmentFile extends VerySimpleModel {
exit();
}
- function getDownloadUrl($minage=false, $disposition=false, $handler=false) {
- // XXX: Drop this when AttachmentFile goes to ORM
+ function getDownloadUrl($options=array()) {
+ // Add attachment ref id if object type is set
+ if (isset($options['type'])
+ && !isset($options['id'])
+ && ($a=$this->attachments->findFirst(array(
+ 'type' => $options['type']))))
+ $options['id'] = $a->getId();
+
return static::generateDownloadUrl($this->getId(),
- strtolower($this->getKey()), $this->getSignature(), $minage,
- $disposition, $handler);
+ strtolower($this->getKey()), $this->getSignature(),
+ $options);
}
- static function generateDownloadUrl($id, $key, $hash, $minage=false,
- $disposition=false, $handler=false
- ) {
- // Expire at the nearest midnight, allowing at least 12 hours access
- $minage = $minage ?: 43200;
- $gmnow = Misc::gmtime() + $minage;
+ static function generateDownloadUrl($id, $key, $hash, $options = array()) {
+
+ // Expire at the nearest midnight, allow at least12 hrs access
+ $minage = @$options['minage'] ?: 43200;
+ $gmnow = Misc::gmtime() + $options['minage'];
$expires = $gmnow + 86400 - ($gmnow % 86400);
// Generate a signature based on secret content
$signature = static::_genUrlSignature($id, $key, $hash, $expires);
- $handler = $handler ?: ROOT_PATH . 'file.php';
+ // Handler / base url
+ $handler = @$options['handler'] ?: ROOT_PATH . 'file.php';
// Return sanitized query string
$args = array(
@@ -211,10 +217,13 @@ class AttachmentFile extends VerySimpleModel {
'signature' => $signature,
);
- if ($disposition)
- $args['disposition'] = $disposition;
+ if (isset($options['disposition']))
+ $args['disposition'] = $options['disposition'];
+
+ if (isset($options['id']))
+ $args['id'] = $options['id'];
- return $handler . '?' . http_build_query($args);
+ return sprintf('%s?%s', $handler, http_build_query($args));
}
function verifySignature($signature, $expires) {
diff --git a/include/class.format.php b/include/class.format.php
index 50c8a7fdf..7d7d7d922 100644
--- a/include/class.format.php
+++ b/include/class.format.php
@@ -455,14 +455,17 @@ class Format {
}
- function viewableImages($html, $script=false) {
+ function viewableImages($html, $options=array()) {
$cids = $images = array();
+ $options +=array(
+ 'deposition' => 'inline');
return preg_replace_callback('/"cid:([\w._-]{32})"/',
- function($match) use ($script, $images) {
+ function($match) use ($options, $images) {
if (!($file = AttachmentFile::lookup($match[1])))
return $match[0];
+
return sprintf('"%s" data-cid="%s"',
- $file->getDownloadUrl(false, 'inline', $script), $match[1]);
+ $file->getDownloadUrl($options), $match[1]);
}, $html);
}
diff --git a/include/class.forms.php b/include/class.forms.php
index 2f8582855..161e06fdb 100644
--- a/include/class.forms.php
+++ b/include/class.forms.php
@@ -4513,13 +4513,15 @@ class FreeTextWidget extends Widget {
if (($attachments = $this->field->getFiles()) && count($attachments)) { ?>
<section class="freetext-files">
<div class="title"><?php echo __('Related Resources'); ?></div>
- <?php foreach ($attachments as $attach) { ?>
+ <?php foreach ($attachments as $attach) {
+ $filename = Format::htmlchars($attach->getFilename());
+ ?>
<div class="file">
<a href="<?php echo $attach->file->getDownloadUrl(); ?>"
- target="_blank" download="<?php echo $attach->file->getDownloadUrl();
- ?>" class="truncate no-pjax">
+ target="_blank" download="<?php echo $filename; ?>"
+ class="truncate no-pjax">
<i class="icon-file"></i>
- <?php echo Format::htmlchars($attach->getFilename()); ?>
+ <?php echo $filename; ?>
</a>
</div>
<?php } ?>
diff --git a/include/class.page.php b/include/class.page.php
index 3ea5ca0f9..a430fc2e6 100644
--- a/include/class.page.php
+++ b/include/class.page.php
@@ -70,7 +70,7 @@ class Page extends VerySimpleModel {
return $this->_getLocal('body', $lang);
}
function getBodyWithImages() {
- return Format::viewableImages($this->getLocalBody());
+ return Format::viewableImages($this->getLocalBody(), ['type' => 'P']);
}
function _getLocal($what, $lang=false) {
diff --git a/include/client/faq.inc.php b/include/client/faq.inc.php
index 9bc723a6e..176ae429a 100644
--- a/include/client/faq.inc.php
+++ b/include/client/faq.inc.php
@@ -43,7 +43,8 @@ $category=$faq->getCategory();
<strong><?php echo __('Attachments');?>:</strong>
<?php foreach ($attachments as $att) { ?>
<div>
- <a href="<?php echo $att->file->getDownloadUrl(); ?>" class="no-pjax">
+ <a href="<?php echo $att->file->getDownloadUrl(['id' => $att->getId()]);
+ ?>" class="no-pjax">
<i class="icon-file"></i>
<?php echo Format::htmlchars($att->getFilename()); ?>
</a>
diff --git a/include/client/templates/thread-entry.tmpl.php b/include/client/templates/thread-entry.tmpl.php
index fed92a5c6..ad9f3dc9f 100644
--- a/include/client/templates/thread-entry.tmpl.php
+++ b/include/client/templates/thread-entry.tmpl.php
@@ -60,7 +60,8 @@ if ($cfg->isAvatarsEnabled() && $user)
?>
<span class="attachment-info">
<i class="icon-paperclip icon-flip-horizontal"></i>
- <a class="no-pjax truncate filename" href="<?php echo $A->file->getDownloadUrl();
+ <a class="no-pjax truncate filename"
+ href="<?php echo $A->file->getDownloadUrl(['id' => $A->getId()]);
?>" download="<?php echo Format::htmlchars($A->getFilename()); ?>"
target="_blank"><?php echo Format::htmlchars($A->getFilename());
?></a><?php echo $size;?>
diff --git a/include/client/view.inc.php b/include/client/view.inc.php
index bc39255ab..fa48bdc3e 100644
--- a/include/client/view.inc.php
+++ b/include/client/view.inc.php
@@ -208,7 +208,7 @@ foreach (AttachmentFile::objects()->filter(array(
'attachments__inline' => true,
)) as $file) {
$urls[strtolower($file->getKey())] = array(
- 'download_url' => $file->getDownloadUrl(),
+ 'download_url' => $file->getDownloadUrl(['type' => 'H']),
'filename' => $file->name,
);
} ?>
diff --git a/include/staff/faq-view.inc.php b/include/staff/faq-view.inc.php
index a5dc4e560..531fbc2cd 100644
--- a/include/staff/faq-view.inc.php
+++ b/include/staff/faq-view.inc.php
@@ -41,7 +41,8 @@ if ($thisstaff->hasPerm(FAQ::PERM_MANAGE)) { ?>
<?php foreach ($attachments as $att) { ?>
<div>
<i class="icon-paperclip pull-left"></i>
- <a target="_blank" href="<?php echo $att->file->getDownloadUrl(); ?>"
+ <a target="_blank" href="<?php echo $att->file->getDownloadUrl(['id' =>
+ $att->getId()]); ?>"
class="attachment no-pjax">
<?php echo Format::htmlchars($att->getFilename()); ?>
</a>
diff --git a/include/staff/templates/thread-entries.tmpl.php b/include/staff/templates/thread-entries.tmpl.php
index 0d15aa0c9..9b267bbae 100644
--- a/include/staff/templates/thread-entries.tmpl.php
+++ b/include/staff/templates/thread-entries.tmpl.php
@@ -80,7 +80,8 @@ foreach (Attachment::objects()->filter(array(
if (!$A->inline)
continue;
$urls[strtolower($A->file->getKey())] = array(
- 'download_url' => $A->file->getDownloadUrl(),
+ 'download_url' => $A->file->getDownloadUrl(['id' =>
+ $A->getId()]),
'filename' => $A->getFilename(),
);
}
diff --git a/include/staff/templates/thread-entry.tmpl.php b/include/staff/templates/thread-entry.tmpl.php
index d36d2b7c9..5781db359 100644
--- a/include/staff/templates/thread-entry.tmpl.php
+++ b/include/staff/templates/thread-entry.tmpl.php
@@ -101,7 +101,8 @@ if ($entry->flags & ThreadEntry::FLAG_COLLABORATOR && $entry->type == 'N') {
?>
<span class="attachment-info">
<i class="icon-paperclip icon-flip-horizontal"></i>
- <a class="no-pjax truncate filename" href="<?php echo $A->file->getDownloadUrl();
+ <a class="no-pjax truncate filename" href="<?php echo
+ $A->file->getDownloadUrl(['id' => $A->getId()]);
?>" download="<?php echo Format::htmlchars($A->getFilename()); ?>"
target="_blank"><?php echo Format::htmlchars($A->getFilename());
?></a><?php echo $size;?>
diff --git a/open.php b/open.php
index 450b9d1f6..a507bc979 100644
--- a/open.php
+++ b/open.php
@@ -82,7 +82,8 @@ if ($ticket
echo Format::viewableImages(
$ticket->replaceVars(
$page->getLocalBody()
- )
+ ),
+ ['type' => 'P']
);
}
else {
--
GitLab