Hotfix for dev/core#747 To fix generation of contact image urls #13663
Conversation
|
(Standard links)
|
7d1745d to
f81ae76
Compare
f81ae76 to
a0013a8
Compare
| list($path, $mimeType) = CRM_Core_BAO_File::path($fileId, $entityId); | ||
| if (!empty($fileName)) { | ||
| $mimeType = ''; | ||
| $path = CRM_Core_Config::singleton()->customFileUploadDir . $fileName; |
There was a problem hiding this comment.
I think this needs a bit more care to avoid ../ trickery?
There was a problem hiding this comment.
@totten this was exactly as it was before see 2762985#diff-b4dd4746dff586ca84ff742ac60afdacL42
There was a problem hiding this comment.
Good thing we removed it! 😜
More seriously... i've seen several formulations that effectively prohibit .., although they differ a bit on what files and subfolders are allowed. Examples:
- Assert that
$fileNamedoes not (a) begin with a dot or (b) include any/or any\. (This prohibits all subfolders, parent folders, and dot files -- it only allows regular files stored directly in the data folder.) - Assert that
$fileNamematchesbasename($fileName). (This also only allows regular files from the regular data folder.) - Assert that
$fileNamematches a stricter affirmative pattern (like "alphanumerics plus one dot plus 3-5 alphanumerics). But we don't have a large organic corpus of examples or a valid formal spec, so it can be hard to make a good stricter pattern.
Note: Take care that Unix systems are consistent about using /, but PHP-Windows can be inconsistent about \ and /, so a more defensive approach is to check both.
There was a problem hiding this comment.
@seamuslee001 I pushed up a change with a basename() trick.
CRM/Core/Page/File.php
Outdated
| $fileId = CRM_Utils_Request::retrieve('id', 'Positive', $this, FALSE); // File ID | ||
| $fileName = CRM_Utils_Request::retrieve('filename', 'String', $this, FALSE); | ||
| if (empty($fileName) && (empty($entityId) || empty($fileId))) { | ||
| CRM_Core_Error::statusBounce("Either FIlename or Entity ID and FIle Id need to be passed in to retrieve files"); |
There was a problem hiding this comment.
There are some typos ("FIle" / "File"). Maybe: Must pass either "Filename" or the combination of "Entity ID" + "File ID" ?
The idea here is that `id+eid+fcs` and `filename` are two separate modes. In `filename` mode, you don't need warnings about the missing `fcs`.
| $cases[] = ['.helloworld', FALSE]; | ||
| $cases[] = ['smartwatch_1736683_1280_9af3657015e8660cc234eb1601da871.jpg', TRUE]; | ||
| return $cases; | ||
| } |
There was a problem hiding this comment.
Oooh, dataProvider-based unit test! 👍
There was a problem hiding this comment.
now now @totten you aren't the ONLY one who writes those...
|
I've done some r-run to check activity attachments and security (with some weird filenames)... latest revision seems good on those fronts. |
|
merge on pass to reflect @totten comments & chat discussion |
|
unrelated fails merging |
Overview
This aims to fix the urls generated for displaying of contact images on the contact summary screen
Before
Image URLs broken
After
Image URLs fixed
ping @eileenmcnaughton @KarinG @totten