Page MenuHomeDevCentral

Add quotes to command arguments
ClosedPublic

Authored by xcombelle on Aug 7 2016, 20:36.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

xcombelle updated this revision to Diff 1469.Aug 7 2016, 20:36
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 updated this revision to Diff 1470.Aug 8 2016, 07:08
dereckson edited edge metadata.

Rebased against master.

dereckson added inline comments.Aug 8 2016, 20:53
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.

xcombelle added inline comments.Aug 8 2016, 23:24
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 commandeered this revision.Aug 17 2016, 00:39
Sandlayth added a reviewer: xcombelle.
xcombelle commandeered this revision.Aug 17 2016, 14:50
xcombelle edited reviewers, added: Sandlayth; removed: xcombelle.
Sandlayth accepted this revision.Aug 17 2016, 21:45
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.