rest: upload files and dirs to workflow fixes #76
Conversation
| path = get_analysis_files_dir(workflow, file_type, | ||
| 'seed') | ||
| if len(full_file_name.split("/")) > 1 and not \ | ||
| os.path.isabs(full_file_name): | ||
| if len(full_file_name.split("/")) > 1: |
There was a problem hiding this comment.
What about working directory containement, such as when file_name passed is ../../../johndoe/secret? Could be sanitised earlier, but better to triple safe than sorry...
There was a problem hiding this comment.
Indeed, I had tested manually and paths starting with '..' were failing, but I added the explicit check in: reanahub/reana-client@7c82db0#diff-4f27f08c3f46f0b755c1d5ac1a1e2dcaR468
Maybe the warning could be more informative, but a better phrasing didn't come to me yet.
It looks like this now:
(reana-cwl-3.5) ➜ reana-demo-root6-roofit git:(master) ✗ reana-client code upload ../jq/autom4te.cache/ code
../jq/autom4te.cache/: Path must be under the upload type directory.
code/gendata.C was uploaded successfully.
code/code2 was uploaded successfully.
There was a problem hiding this comment.
It may be good to iterate over subdirs in r-w-controller anyway, and skip the ones equal to .. for additional safety. We could simply emit an error to boot if we find .. in path parts.
This would add additional safety for the future, because there can be other clients besides r-client calling our REST API server one day. Let's make the check on the server side as well then, inside r-w-controller for now, and later on perhaps in r-server.
BTW the following user scenario still looks OK:
$ cd ./docs/step17/
$ less some-instructions.txt
$ vim ../../code/step17/mycode.py
$ reana-client code upload ../../code/step17/mycode.pyPath must be under the upload type directory.
It may be more understandable to say: "path must be under the code directory" so that people understand the code vs inputs distinction. (However beware of use case scenario where user uses only one directory for both code and inputs.)
There was a problem hiding this comment.
Just pushed in https://github.com/reanahub/reana-client/pull/79/files this check:
(reana-cwl-3.5) ➜ reana-demo-root6-roofit git:(master) ✗ reana-client code upload code/../../jq/AUTHORS
code/../../jq/AUTHORS: Path cannot contain ".."
and removed the prepend of the upload type.
There was a problem hiding this comment.
@tiborsimko I just added the check for the paths including ../ here.
dinosk
force-pushed
the
67_input_seeding_with_absolute_path
branch
3 times, most recently
from
bdabd7e
to
554b121
Compare
* FIXES formatting of the paths that are created from uploading files or directories to a workflow. Signed-off-by: Dinos Kousidis <dinos.kousidis@cern.ch>
dinosk
force-pushed
the
67_input_seeding_with_absolute_path
branch
from
554b121
to
5dc9d80
Compare
from uploading files or directories to a workflow.
(Closes reanahub/reana-client#67)
Signed-off-by: Dinos Kousidis dinos.kousidis@cern.ch