Page MenuHomePhabricator

Allow certain flags to be required to appear before non-flag arguments
ClosedPublic

Authored by epriestley on Sep 14 2018, 6:50 PM.

Details

Summary

Ref T13098. Historically, we've seen some issues (particularly with Mercurial) where naming a branch something like --execute-remote-website=evil.com would evaluate even with "--" in the argument list (see T13012).

We're broadly better about this, but still need to parse arguments in multiple phases, and callers/users aren't always careful about using -- to terminate argument lists.

To reduce the danger of flags like --config, --load-library, etc., allow argument parsing to require that they appear before workflow arguments, so arc --config xyz diff is valid, but arc diff --config xyz is not. This reduces the chance that some user script somewhere which does arc diff %s instead of arc diff -- %s will ever be able to do anything truly dangerous.

Test Plan

See upcoming changes to Arcanist. (This does nothing on its own.)

Diff Detail

Repository
rPHU libphutil
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Sep 14 2018, 6:50 PM
epriestley requested review of this revision.Sep 14 2018, 6:51 PM
amckinley accepted this revision.Sep 14 2018, 7:51 PM
amckinley added inline comments.
src/parser/argument/PhutilArgumentParser.php
111–114

Just a nitpick, but this spacing is all over the place.

597–600

This is unused code which I'm assuming will get used in a coming revision.

This revision is now accepted and ready to land.Sep 14 2018, 7:51 PM
This revision was automatically updated to reflect the committed changes.