From b83248d3cdc7b6cf7d7fbd2262c224186b79ab3d Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:04:18 +0200
Subject: [PATCH 1/5] New file_can_view_or_download() function

file_can_view_bug_attachments() and file_can_download_bug_attachments()
have nearly identical code, the only difference being the names of the
configs.

Adding a new internal File API function to avoid code duplication.

Issue #27299
---
 core/file_api.php | 60 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/core/file_api.php b/core/file_api.php
index a4bee8989..2517aaba2 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -206,30 +206,72 @@ function file_bug_has_attachments( $p_bug_id ) {
 	}
 }
 
+/**
+ * Check if the current user can view or download attachments.
+ *
+ * Generic call used by
+ * - {@see file_can_view_bug_attachments()}
+ * - {@see file_can_view_bugnote_attachments}
+ * - {@see file_can_download_bug_attachments()}
+ * - {@see file_can_download_bugnote_attachments}
+ *
+ * @param string   $p_action            'view' or 'download'
+ * @param int      $p_bug_id            A bug identifier
+ * @param int      $p_uploader_user_id  The user who uploaded the attachment
+ *
+ * @return bool
+ *
+ * @internal Should not be used outside of File API.
+ */
+function file_can_view_or_download( $p_action, $p_bug_id, $p_uploader_user_id ) {
+	switch( $p_action ) {
+		case 'view':
+			$t_threshold_global = 'view_attachments_threshold';
+			$t_threshold_own = 'allow_view_own_attachments';
+			break;
+		case 'download':
+			$t_threshold_global = 'download_attachments_threshold';
+			$t_threshold_own = 'allow_download_own_attachments';
+			break;
+		default:
+			trigger_error( ERROR_GENERIC, ERROR );
+	}
+
+	$t_project_id = bug_get_field( $p_bug_id, 'project_id' );
+	$t_access_global = config_get( $t_threshold_global,null, null, $t_project_id );
+
+	$t_can_access = access_has_bug_level( $t_access_global, $p_bug_id );
+	if( $t_can_access ) {
+		return true;
+	}
+
+	$t_uploaded_by_me = auth_get_current_user_id() == $p_uploader_user_id;
+	$t_view_own = config_get( $t_threshold_own, null, null, $t_project_id );
+	return $t_uploaded_by_me && $t_view_own;
+}
+
 /**
  * Check if the current user can view attachments for the specified bug.
+ *
  * @param integer $p_bug_id           A bug identifier.
  * @param integer $p_uploader_user_id A user identifier.
+ *
  * @return boolean
  */
 function file_can_view_bug_attachments( $p_bug_id, $p_uploader_user_id = null ) {
-	$t_uploaded_by_me = auth_get_current_user_id() === $p_uploader_user_id;
-	$t_can_view = access_has_bug_level( config_get( 'view_attachments_threshold' ), $p_bug_id );
-	$t_can_view = $t_can_view || ( $t_uploaded_by_me && config_get( 'allow_view_own_attachments' ) );
-	return $t_can_view;
+	return file_can_view_or_download( 'view', $p_bug_id, $p_uploader_user_id );
 }
 
 /**
  * Check if the current user can download attachments for the specified bug.
+ *
  * @param integer $p_bug_id           A bug identifier.
- * @param integer $p_uploader_user_id A user identifier.
+ * @param integer $p_uploader_user_id The user who uploaded the attachment.
+ *
  * @return boolean
  */
 function file_can_download_bug_attachments( $p_bug_id, $p_uploader_user_id = null ) {
-	$t_uploaded_by_me = auth_get_current_user_id() === $p_uploader_user_id;
-	$t_can_download = access_has_bug_level( config_get( 'download_attachments_threshold', null, null, bug_get_field( $p_bug_id, 'project_id' ) ), $p_bug_id );
-	$t_can_download = $t_can_download || ( $t_uploaded_by_me && config_get( 'allow_download_own_attachments', null, null, bug_get_field( $p_bug_id, 'project_id' ) ) );
-	return $t_can_download;
+	return file_can_view_or_download( 'download', $p_bug_id, $p_uploader_user_id );
 }
 
 /**
-- 
2.25.1


From 1a24845b321ac3a5c62bc11524aaab770adc89be Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:09:06 +0200
Subject: [PATCH 2/5] Functions to check view/download ability at bugnote level

2 new File API functions:
- file_can_view_bugnote_attachments()
- file_can_download_bugnote_attachments

Prerequisite to fix issue #27039
---
 core/file_api.php | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/core/file_api.php b/core/file_api.php
index 2517aaba2..63de3e487 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -218,12 +218,13 @@ function file_bug_has_attachments( $p_bug_id ) {
  * @param string   $p_action            'view' or 'download'
  * @param int      $p_bug_id            A bug identifier
  * @param int      $p_uploader_user_id  The user who uploaded the attachment
+ * @param int|null $p_bugnote_id        If specified, will check at bugnote level
  *
  * @return bool
  *
  * @internal Should not be used outside of File API.
  */
-function file_can_view_or_download( $p_action, $p_bug_id, $p_uploader_user_id ) {
+function file_can_view_or_download( $p_action, $p_bug_id, $p_uploader_user_id, $p_bugnote_id = null ) {
 	switch( $p_action ) {
 		case 'view':
 			$t_threshold_global = 'view_attachments_threshold';
@@ -240,7 +241,11 @@ function file_can_view_or_download( $p_action, $p_bug_id, $p_uploader_user_id )
 	$t_project_id = bug_get_field( $p_bug_id, 'project_id' );
 	$t_access_global = config_get( $t_threshold_global,null, null, $t_project_id );
 
-	$t_can_access = access_has_bug_level( $t_access_global, $p_bug_id );
+	if( $p_bugnote_id === null ) {
+		$t_can_access = access_has_bug_level( $t_access_global, $p_bug_id );
+	} else {
+		$t_can_access = access_has_bugnote_level( $t_access_global, $p_bugnote_id );
+	}
 	if( $t_can_access ) {
 		return true;
 	}
@@ -262,6 +267,22 @@ function file_can_view_bug_attachments( $p_bug_id, $p_uploader_user_id = null )
 	return file_can_view_or_download( 'view', $p_bug_id, $p_uploader_user_id );
 }
 
+/**
+ * Check if the current user can view attachments for the specified bug note.
+ *
+ * @param integer $p_bugnote_id       A bugnote identifier.
+ * @param integer $p_uploader_user_id The user who uploaded the attachment.
+ *
+ * @return boolean
+ */
+function file_can_view_bugnote_attachments( $p_bugnote_id, $p_uploader_user_id = null ) {
+	if( $p_bugnote_id == 0 ) {
+		return true;
+	}
+	$t_bug_id = bugnote_get_field( $p_bugnote_id, 'bug_id' );
+	return file_can_view_or_download( 'view', $t_bug_id, $p_uploader_user_id );
+}
+
 /**
  * Check if the current user can download attachments for the specified bug.
  *
@@ -274,6 +295,22 @@ function file_can_download_bug_attachments( $p_bug_id, $p_uploader_user_id = nul
 	return file_can_view_or_download( 'download', $p_bug_id, $p_uploader_user_id );
 }
 
+/**
+ * Check if the current user can download attachments for the specified bug note.
+ *
+ * @param integer $p_bugnote_id       A bugnote identifier.
+ * @param integer $p_uploader_user_id The user who uploaded the attachment.
+ *
+ * @return boolean
+ */
+function file_can_download_bugnote_attachments( $p_bugnote_id, $p_uploader_user_id = null ) {
+	if( $p_bugnote_id == 0 ) {
+		return true;
+	}
+	$t_bug_id = bugnote_get_field( $p_bugnote_id, 'bug_id' );
+	return file_can_view_or_download( 'download', $t_bug_id, $p_uploader_user_id, $p_bugnote_id );
+}
+
 /**
  * Check if the current user can delete attachments from the specified bug.
  * @param integer $p_bug_id           A bug identifier.
-- 
2.25.1


From 2a44141d82ec9aed9e14431b895ee181b9194fc4 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:21:09 +0200
Subject: [PATCH 3/5] Check ability to download attachments at bugnote level

This prevents users authorized to download attachments but not to view
private bugnotes, from accessing files attached to a private note via
`file_download.php?file_id={FILE_ID}&type=bug`.

Includes some minor code cleanup in file_get_visible_attachments():
- use a foreach loop
- reuse variables instead of derefenrcing array

Fixes #27039
---
 core/file_api.php | 30 ++++++++++--------------------
 file_download.php |  4 +++-
 2 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/core/file_api.php b/core/file_api.php
index 63de3e487..245b30353 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -451,31 +451,21 @@ function file_get_visible_attachments( $p_bug_id ) {
 
 	$t_preview_text_ext = config_get( 'preview_text_extensions' );
 	$t_preview_image_ext = config_get( 'preview_image_extensions' );
-	$t_attachments_view_threshold = config_get( 'view_attachments_threshold' );
 
 	$t_image_previewed = false;
-	for( $i = 0;$i < $t_attachments_count;$i++ ) {
-		$t_row = $t_attachment_rows[$i];
+	foreach( $t_attachment_rows as $t_row ) {
 		$t_user_id = (int)$t_row['user_id'];
+		$t_attachment_note_id = (int)$t_row['bugnote_id'];
 
-		# This covers access checks for issue attachments
-		if( !file_can_view_bug_attachments( $p_bug_id, $t_user_id ) ) {
+		if( !file_can_view_bug_attachments( $p_bug_id, $t_user_id )
+		|| !file_can_view_bugnote_attachments( $t_attachment_note_id, $t_user_id )
+		) {
 			continue;
 		}
 
-		# This covers access checks for issue note attachments
-		$t_attachment_note_id = (int)$t_row['bugnote_id'];
-		if( $t_attachment_note_id !== 0 ) {
-			if( bugnote_get_field( $t_attachment_note_id, 'view_state' ) != VS_PUBLIC ) {
-				if( !access_has_bugnote_level( $t_attachments_view_threshold, $t_attachment_note_id ) ) {
-					continue;
-				}
-			}
-		}
-
 		$t_id = (int)$t_row['id'];
 		$t_filename = $t_row['filename'];
-		$t_filesize = $t_row['filesize'];
+		$t_filesize = (int)$t_row['filesize'];
 		$t_diskfile = file_normalize_attachment_path( $t_row['diskfile'], bug_get_field( $p_bug_id, 'project_id' ) );
 		$t_date_added = $t_row['date_added'];
 
@@ -483,14 +473,14 @@ function file_get_visible_attachments( $p_bug_id ) {
 		$t_attachment['id'] = $t_id;
 		$t_attachment['user_id'] = $t_user_id;
 		$t_attachment['display_name'] = file_get_display_name( $t_filename );
-		$t_attachment['size'] = (int)$t_filesize;
+		$t_attachment['size'] = $t_filesize;
 		$t_attachment['date_added'] = $t_date_added;
 		$t_attachment['diskfile'] = $t_diskfile;
 		$t_attachment['file_type'] = $t_row['file_type'];
-		$t_attachment['bugnote_id'] = (int)$t_row['bugnote_id'];
+		$t_attachment['bugnote_id'] = $t_attachment_note_id;
 
-		$t_attachment['can_download'] = file_can_download_bug_attachments( $p_bug_id, (int)$t_row['user_id'] );
-		$t_attachment['can_delete'] = file_can_delete_bug_attachments( $p_bug_id, (int)$t_row['user_id'] );
+		$t_attachment['can_download'] = file_can_download_bug_attachments( $p_bug_id, $t_user_id );
+		$t_attachment['can_delete'] = file_can_delete_bug_attachments( $p_bug_id, $t_user_id );
 
 		if( $t_attachment['can_download'] ) {
 			$t_attachment['download_url'] = 'file_download.php?file_id=' . $t_id . '&type=bug';
diff --git a/file_download.php b/file_download.php
index d423681b0..d7404548a 100644
--- a/file_download.php
+++ b/file_download.php
@@ -110,7 +110,9 @@ if( $f_type == 'bug' ) {
 # Check access rights
 switch( $f_type ) {
 	case 'bug':
-		if( !file_can_download_bug_attachments( $v_bug_id, (int)$v_user_id ) ) {
+		if( !file_can_download_bug_attachments( $v_bug_id, $v_user_id )
+		|| !file_can_download_bugnote_attachments( $v_bugnote_id, $v_user_id )
+		) {
 			access_denied();
 		}
 		break;
-- 
2.25.1


From 8d9f4346963b2dc5462fb27700f4c60ea4a80a1e Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:24:21 +0200
Subject: [PATCH 4/5] Improve PHPDoc for file_get_visible_attachments()

---
 core/file_api.php | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/core/file_api.php b/core/file_api.php
index 245b30353..67e4cecef 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -422,19 +422,21 @@ function file_normalize_attachment_path( $p_diskfile, $p_project_id ) {
 
 /**
  * Gets an array of attachments that are visible to the currently logged in user.
+ *
  * Each element of the array contains the following:
- * display_name - The attachment display name (i.e. file name dot extension)
- * size - The attachment size in bytes.
- * date_added - The date where the attachment was added.
- * can_download - true: logged in user has access to download the attachment, false: otherwise.
- * diskfile - The name of the file on disk.  Typically this is a hash without an extension.
- * download_url - The download URL for the attachment (only set if can_download is true).
- * exists - Applicable for DISK attachments.  true: file exists, otherwise false.
- * can_delete - The logged in user can delete the attachments.
- * preview - true: the attachment should be previewable, otherwise false.
- * type - Can be "image", "text" or empty for other types.
- * alt - The alternate text to be associated with the icon.
- * icon - array with icon information, contains 'url' and 'alt' elements.
+ * - display_name - The attachment display name (i.e. file name dot extension)
+ * - size - The attachment size in bytes.
+ * - date_added - The date where the attachment was added.
+ * - can_download - true: logged in user has access to download the attachment, false: otherwise.
+ * - diskfile - The name of the file on disk.  Typically this is a hash without an extension.
+ * - download_url - The download URL for the attachment (only set if can_download is true).
+ * - exists - Applicable for DISK attachments.  true: file exists, otherwise false.
+ * - can_delete - The logged in user can delete the attachments.
+ * - preview - true: the attachment should be previewable, otherwise false.
+ * - type - Can be "image", "text" or empty for other types.
+ * - alt - The alternate text to be associated with the icon.
+ * - icon - array with icon information, contains 'url' and 'alt' elements.
+ *
  * @param integer $p_bug_id A bug identifier.
  * @return array
  */
-- 
2.25.1


From 32f1a5f3f4c7e78e4655ca0faaa616d97170c1c9 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:25:06 +0200
Subject: [PATCH 5/5] Fix PHPStorm undefined variable warnings

---
 file_download.php | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/file_download.php b/file_download.php
index d7404548a..9ed9b5f44 100644
--- a/file_download.php
+++ b/file_download.php
@@ -99,6 +99,18 @@ if( false === $t_row ) {
 	error_parameters( $c_file_id );
 	trigger_error( ERROR_FILE_NOT_FOUND, ERROR );
 }
+/**
+ * @var int    $v_bug_id
+ * @var int    $v_project_id
+ * @var string $v_diskfile
+ * @var string $v_filename
+ * @var int    $v_filesize
+ * @var string $v_file_type
+ * @var string $v_content
+ * @var int    $v_date_added
+ * @var int    $v_user_id
+ * @var int    $v_bugnote_id
+ */
 extract( $t_row, EXTR_PREFIX_ALL, 'v' );
 
 if( $f_type == 'bug' ) {
-- 
2.25.1