Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid shell injection problems by using subprocess.run() #95

Closed
wants to merge 2 commits into from
Closed

Avoid shell injection problems by using subprocess.run() #95

wants to merge 2 commits into from

Conversation

kousu
Copy link
Contributor

@kousu kousu commented Nov 20, 2020

No description provided.

@kousu
Drop run_shell_command in favour of subprocess.run
18ba2de 
run_shell_command was parsing the command line passed, so it was subject to shell injections
and, on Windows, wouldn't handle filenames correctly -- it would split on spaces (which are common
in Windows filenames) and interpret \s (also common in Windows file names) as control codes.

To do this, I got rid of defaceTpl, which was a shell command template, in favour of hardcoding
pydeface as the anonymizing algorithm, and as a result of that I also bumped the major version.
@kousu
Copy link
Contributor Author

kousu commented Nov 20, 2020

As I said in the commit message, this has the unfortunate side effect of forcing out defaceTpl. My guessing tells me that not that many people are using defaceTpl, and those that are are using pydeface anyway. If someone really needs to customize their anonymizer, I would direct them towards use some shell scripting of their own before running dcm2bids: something like find dataset/ -type f -name "*.nii.gz" -exec pydeface {} {}.defaced \; Letting people indirectly write shell scripts through python is always going to be hard.

Since this changes the API, I did a major version bump. Which is unfortunate because there isn't really any other major change to speak of in here, and because probably most people aren't using this part of the API anyway. I'm not sure if it's a good idea, I'd like some feedback one way or the other.

I could probably resurrect defaceTpl and still get it used safely with subprocess.run(). It will just be less nice code. I will if you think it's necessary.

@kousu kousu mentioned this pull request Nov 20, 2020
Closed
@kousu
Make pydeface a requirement.
bf9003d 
Also, remove the layer of indirection of putting the requirements outside of setup.py.
@arnaudbore
Copy link
Contributor

arnaudbore commented Nov 20, 2020 •
edited
Loading

I don't think I like the idea of having dependencies like pydeface.
If we can think about a way to include a structure (dict, list, ... ) within the config file to run a command that would be nicer. I would add a check if the command exists. It does not have to be complicated and it will be more flexible. I'm thinking about something more generic and maybe be specific to a folder (anat, dwi, fmap,...) or a modality (T1w, FLAIR, dwi, epi, ...).

@arnaudbore
Copy link
Contributor

We can split the PR into two PR. We can get rid of defaceTpl and suggest a new PR with something I described above.
What do you think ?

@arnaudbore
Copy link
Contributor

Hi @kousu,
Can you take five minutes and merge master into your PR ?
You can also get rid of the "versionning" I'll take care of this.
After almost 2 years I think it's time. 👍

@arnaudbore arnaudbore mentioned this pull request Apr 17, 2022
Merged
@kousu
Copy link
Contributor Author

kousu commented Apr 23, 2022

We can split the PR into two PR. We can get rid of defaceTpl and suggest a new PR with something I described above. What do you think ?

I'm sorry, I'm not sure why I never responded to this!! You answered it the day of and then I just dropped it, even though I asked for feedback. Sometimes Github gets lost in my inbox? I'm not sure really. I think it might be a problem with my spam filter, maybe being overly picky about some of Github's MXes and not others, or maybe I just wasn't paying attention.

Hi @kousu, Can you take five minutes and merge master into your PR ? You can also get rid of the "versionning" I'll take care of this. After almost 2 years I think it's time. +1

Yeah I'd be happy to take care of this!

@SamGuay
Copy link
Member

SamGuay commented Apr 23, 2022

Hi @kousu, Arnaud worked on another PR (#158) inspired by yours which I think covers what you had done in this in addition to fixing some paths issues we had on Windows. I've tested on Windows but as I am not really a Windows user, if you could test it and review the PR, that'd be great!
Thanks

@kousu
Copy link
Contributor Author

kousu commented Apr 24, 2022

Well, that's great news. I'm not a Windows user either so I can't help you out there, but congrats on the tidy work :)

@kousu kousu closed this Apr 24, 2022
@SamGuay
Copy link
Member

SamGuay commented Apr 24, 2022

Oh sorry, I assume you were a Windows user because you mentioned opened #83 regarding windows paths 😅 .

@kousu
Copy link
Contributor Author

kousu commented Apr 24, 2022

Oh geez I forgot about #83. I was helping someone in my lab who was a Windows user. But it was a long time ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbedetti cbedetti Awaiting requested review from cbedetti

@arnaudbore arnaudbore Awaiting requested review from arnaudbore

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kousu @arnaudbore @SamGuay