Page MenuHomeDevCentral

Add quotes to command arguments
ClosedPublic

Authored by xcombelle on Aug 7 2016, 20:36.
Referenced Files
Unknown Object (File)
Wed, Mar 27, 08:21
Unknown Object (File)
Wed, Mar 27, 07:43
Unknown Object (File)
Wed, Mar 27, 07:04
Unknown Object (File)
Tue, Mar 26, 12:52
Unknown Object (File)
Sat, Mar 23, 07:35
Unknown Object (File)
Fri, Mar 22, 16:50
Unknown Object (File)
Fri, Mar 22, 15:32
Unknown Object (File)
Fri, Mar 22, 15:16

Details

Summary

The purpose of this commit is that without quote, if variable contains
characters meaningful for shell, such as space, '$', ';' they are
interpreted. The quoting avoid this interpretation.

Diff Detail

Repository
rDPHAB Docker image for Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

xcombelle retitled this revision from to adding quote to command arguments.
xcombelle updated this object.
xcombelle edited the test plan for this revision. (Show Details)
xcombelle added a reviewer: dereckson.
xcombelle retitled this revision from adding quote to command arguments to add quote to command arguments.Aug 7 2016, 20:55
dereckson retitled this revision from add quote to command arguments to Add quotes to command arguments.Aug 7 2016, 20:58
dereckson updated this object.
dereckson edited edge metadata.

Rebased against master.

files/usr/local/bin/setup-phabricator
13

See comment line 22.

22

Here we should probably check it's a valid domain and fail loudly if not.

Spaces, $ not allowed.

files/usr/local/bin/setup-phabricator
13

at least semicolon (;) are valid on url and a lot of meta shell chars (I don't have the list off my head.)

see comment line 22 for rest of answer

22

Additional check to proper format would be welcome. Additional commit can be added

However there is nothing to lose to proper escaping arguments in using quote even if we are 100% sure it will not happen.

It is even very good programming practice. One particular reason is that now we might know that there can't be shell injection, but whoever know in the future.

A way to conduct testing

set $MYSQL_ENV_MYSQL_ROOT_PASSWORD and others variables to nothing; echo dangerous command

in previous version the line should be echoed.

in new version nothing should appear.

It could be automatized for non regression

This comment was removed by xcombelle.
This comment was removed by xcombelle.
Sandlayth added inline comments.
files/usr/local/bin/setup-phabricator
6

Maybe add a commentary:

# mysql.host variable is fixed and should always be set as 'mysql', according to https://devcentral.nasqueron.org/diffusion/DPHAB/ :
# > a MySQL container linked as 'mysql' (--link <your MySQL container>:mysql), which could be the official MySQL image or our nasqueron/mysql image, optimized for Phabricator.
Sandlayth added a reviewer: xcombelle.
xcombelle edited reviewers, added: Sandlayth; removed: xcombelle.
Sandlayth edited edge metadata.
This revision is now accepted and ready to land.Aug 17 2016, 21:45
This revision was automatically updated to reflect the committed changes.