From b83248d3cdc7b6cf7d7fbd2262c224186b79ab3d Mon Sep 17 00:00:00 2001 From: Damien Regad 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 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 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 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 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