Deprecate attachment upload by file path #302
Conversation
strpos() works faster than preg_match()
Art4
changed the title
| return (false !== strpos($path, '/uploads.json')) or (false !== strpos($path, '/uploads.xml')); | ||
| } | ||
|
|
||
| private function isUploadCallAndFilepath(string $path, string $body): bool |
There was a problem hiding this comment.
The name of the method is not very clear to me. What do you mean with "AndFilePath"?
There was a problem hiding this comment.
The method checks if $path is an upload call and if $body contains a valid file path using is_file(), but was previously named isUploadCall() which does not fully describe the function. I extracted the upload check into its own method (named isUploadCall()) and now I needed a name that better described the meaning of the previous method. That is the reason why I came up with isUploadCallAndFilepath().
There was a problem hiding this comment.
Maybe we should remove the isUploadCall() from this method and rename isUploadCallAndFilepath() to isFilepath()?
There was a problem hiding this comment.
isValidFilePath ?
isAuthorizedFilePath ?
There was a problem hiding this comment.
I will remove the call of isUploadCall() and rename it to isValidFilePath().
Co-authored-by: Kevin Saliou <kevin@saliou.name>
Co-authored-by: Kevin Saliou <kevin@saliou.name>
Co-authored-by: Kevin Saliou <kevin@saliou.name>
oh? why not a |
Removing this functionality is a breaking change and requires a major version. |
Uploading an attachment could be made by
$client->getApi('attachment')->upload('file content');as documented in the docs: https://github.com/kbsali/php-redmine-api/blob/v2.0.1/docs/usage.mdHowever in #222 a new way of uploading files via filepath was introduced as a "fix". This way has led to a number of problems like #235 and #264.
This method can additionally lead to a potential security problem: Assume someone wants to upload the file
file.txt, but the file contains the path to another file:file.txt
If one now runs
$client->getApi('attachment')->upload(file_get_contents('./file.txt'));the library will check withis_file()for the existence of the/etc/passwdfile and will upload this file instead.As a solution I would recommend to deprecate the attachment upload by filename and remove it in the next major version.
This PR will fix #264.
@kbsali Depending on how critical this issue is rated, I would also release version 3.0 very soon.