-
Notifications
You must be signed in to change notification settings - Fork 546
Conversation
if (data.jobEnvs) { | ||
for (let key in data.jobEnvs) { | ||
if (data.jobEnvs.hasOwnProperty(key)) { | ||
jobEnvs = jobEnvs.concat(key, '=', data.jobEnvs[key], '\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No encode needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are indeed injection risks, javascript injection and bash injection.
I will take more investigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's some kind of complicated.
We pass the env.list to docker command, which would quote.
https://github.com/Microsoft/pai/blob/4491119258197ccd6700f295b3ff518ed4a83c81/src/rest-server/src/templates/yarnContainerScript.mustache#L277
So if we quote it with single/double quote, the value would corrupt, as below:
hayua@stcvl-131:~$ cat env.list
double_quote="with double quote"
single_quote='with single quote'
hayua@stcvl-131:~$ sudo docker run -it --env-file env.list ubuntu env |grep quote
double_quote="with double quote"
single_quote='with single quote'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Gerhut,
Now the "env list' was passing to 'docker run', which would take care of 'encoding and escaping'.
We should pass it as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have documentation for this feature?
Not yet, I will add doc. |
fix #1527.