prePackagedDownloadPath and EEGChunksPath config #9523
Conversation
laemtl
force-pushed
the
prePackagedDownloadPath-config
branch
from
4c571ff to
915513b
Compare
|
Both files tested; error-free. |
There was a problem hiding this comment.
I think the pipelinearchive endpoint should instead be (auto-)handled in a new file, called pipelinearchive.class.inc
I personally would not extend Endpoint in the PipelineArchive class
I also think there should be a default value in the Config table for prePackagedDownloadPath
@jeffersoncasimir I think you are right about that, unless we want an endpoint to download archive files. @driusan @cmadjar thoughts?
We can do that if we want the BIDS archives to be always outside the assembly_bids folder. @cmadjar thoughts? @cmadjar PipelineArchive is an attempt to replace the htdocs/mri/jiv/get_file.php logic for imaging archives. It is only used in the EEG ecosystem for now, is there a scenario it would be useful for imaging as well? |
laemtl
changed the title
|
Since this has SQL changes please rebase to the main branch |
| use \LORIS\Http\Endpoint; | ||
|
|
||
| /** | ||
| * This class handles pipeline archives downlaod. It should |
There was a problem hiding this comment.
This class appears to be specific to the EEG browser and should go in the appropriate module. If it's intended to be a generic way to download files, I don't think the name "PipelineArchive" is appropriate.
There was a problem hiding this comment.
This class is specific to EEG and MRI BIDS and maybe MRI TARCHIVES, so I don't think having it inside the EEG Browser is ideal. What would be the appropriate location? Maybe changing the name for ImagingArchive will make it clearer?
Its aims is to replace the logic in htdocs/mri/jiv
| * | ||
| * @license http://www.gnu.org/licenses/gpl-3.0.txt GPLv3 | ||
| */ | ||
| class PipelineArchive extends Endpoint |
There was a problem hiding this comment.
As an endpoint design, the FilesPassthruHandler (ie. as used by the media module) https://github.com/aces/Loris/blob/main/modules/media/php/files.class.inc) seems to be much simpler and uses existing infrastructure such as the FilesDownloadHandler (which does validation to address the security issues mentioned in this PR and has more generic support for any mime type.)
| } | ||
|
|
||
| $data_path = \NDB_Config::singleton()->getSetting('dataDirBasepath'); | ||
| $fullpath = $data_path . "/" . $request->getQueryParams()["filename"]; |
There was a problem hiding this comment.
This is vulnerable to a path traversal attack. Anyone who has access to this endpoint can download anything on the filesystem by crafting a filename like "../../../../etc/passwd"
|
|
||
| if (!file_exists($fullpath)) { | ||
| $package_path = \NDB_Config::singleton()->getSetting('prePackagedDownloadPath'); | ||
| $fullpath = $package_path . "/" . $pipeline . ($version ? "/" . $version : "") . "/" . $request->getQueryParams()["filename"]; |
There was a problem hiding this comment.
This is also uses direct string concatenation of user input and allows an attacker to traverse to an arbitrary file on the file system by either manipulatig the filename, pipeline, or version sent in the request.
| $fullpath = $data_path . "/" . $request->getQueryParams()["filename"]; | ||
|
|
||
| if (!file_exists($fullpath)) { | ||
| $package_path = \NDB_Config::singleton()->getSetting('prePackagedDownloadPath'); |
There was a problem hiding this comment.
If this is not set, "$fullpath" will be relative to "/" because of the string concatenation below.
| $ext = $path_parts['extension']; | ||
| switch ($ext) { | ||
| case 'zip': | ||
| $mime = "application/x-zip"; |
There was a problem hiding this comment.
I don't think this mime type is correct. zip (without x-) is a standardized mime type.
Add prePackagedDownloadPath and EEGChunksPath configs to LORIS.
Those options are available in LORIS-MRI since v24 and we omitted to add it into LORIS.