This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1483

globalopts: make special flags ineffective after '--' (BC)
AbandonedPublic

Authored by quark on Nov 21 2017, 2:14 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Summary

Flags after -- should not be effective. That`s a universal rule across
common software. People should be able to hg log a file named --debugger.
Besides, hg ... -- --profile --traceback should not enable profiling or
traceback.

This patch changes the handling of --debugger, --profile, --traceback
so they are ineffective after --. This makes other software easier to deal
with command line flags injection by adding -- before user input. See
https://hackerone.com/reports/288704

Those flags are only useful for developers. So this BC looks fine.

.. bc::

Certain global flags (`--debugger`, `--profile`, and `--traceback`) are
no longer effective after `--`.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Nov 21 2017, 2:14 PM
quark updated this revision to Diff 3740.Nov 21 2017, 2:14 PM
quark edited the summary of this revision. (Show Details)Nov 21 2017, 2:18 PM
quark edited the summary of this revision. (Show Details)Nov 21 2017, 2:20 PM
dlax added a subscriber: dlax.Nov 21 2017, 3:00 PM
dlax added inline comments.
mercurial/dispatch.py
716

I find the algorithm a bit clumsy (usage of both in and .index() for the same value), how about a for loop like that:

for arg in args:
    if arg == optname:
        return True
    if arg == '--':
        return False
return False
quark added inline comments.Nov 21 2017, 3:27 PM
mercurial/dispatch.py
716

Good advice! Will change.

quark updated this revision to Diff 3744.Nov 21 2017, 3:30 PM
quark edited the summary of this revision. (Show Details)Nov 21 2017, 5:21 PM
lothiraldan added a subscriber: lothiraldan.Nov 22 2017, 2:22 AM
lothiraldan added inline comments.
mercurial/dispatch.py
707

Just for the sake of testing, could you add a doctest with --debugger before --?

yuja requested changes to this revision.Nov 22 2017, 8:11 AM
yuja added a subscriber: yuja.

A similar patch is already in stable. Can you send a follow-up if you found
a problem?

https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-November/107567.html

This revision now requires changes to proceed.Nov 22 2017, 8:11 AM
quark abandoned this revision.Nov 22 2017, 12:28 PM

I didn't notice it's fixed in stable. Thanks!

Revision Contents
Changeset List

PathPackages
Mmercurial/dispatch.py (32 lines)
Mtests/test-globalopts.t (7 lines)

Diff 3739

mercurial/dispatch.py

elif req.ui: elif req.ui:
ferr = req.ui.ferr ferr = req.ui.ferr
else: else:
ferr = util.stderr ferr = util.stderr
try: try:
if not req.ui: if not req.ui:
req.ui = uimod.ui.load() req.ui = uimod.ui.load()
if '--traceback' in req.args: if _earlypeekboolopt('--traceback', req.args):
req.ui.setconfig('ui', 'traceback', 'on', '--traceback') req.ui.setconfig('ui', 'traceback', 'on', '--traceback')
# set ui streams from the request # set ui streams from the request
if req.fin: if req.fin:
req.ui.fin = req.fin req.ui.fin = req.fin
if req.fout: if req.fout:
req.ui.fout = req.fout req.ui.fout = req.fout
if req.ferr: if req.ferr:
req.args[0] != '-R' or req.args[0] != '-R' or
req.args[1].startswith('--') or req.args[1].startswith('--') or
req.args[2] != 'serve' or req.args[2] != 'serve' or
req.args[3] != '--stdio'): req.args[3] != '--stdio'):
raise error.Abort( raise error.Abort(
_('potentially unsafe serve --stdio invocation: %r') % _('potentially unsafe serve --stdio invocation: %r') %
(req.args,)) (req.args,))
usedebugger = _earlypeekboolopt('--debugger', req.args)
try: try:
debugger = 'pdb' debugger = 'pdb'
debugtrace = { debugtrace = {
'pdb': pdb.set_trace 'pdb': pdb.set_trace
} }
debugmortem = { debugmortem = {
'pdb': pdb.post_mortem 'pdb': pdb.post_mortem
} }
# read --config before doing anything else # read --config before doing anything else
# (e.g. to change trust settings for reading .hg/hgrc) # (e.g. to change trust settings for reading .hg/hgrc)
cfgs = _parseconfig(req.ui, _earlygetopt(['--config'], req.args)) cfgs = _parseconfig(req.ui, _earlygetopt(['--config'], req.args))
if req.repo: if req.repo:
# copy configs that were passed on the cmdline (--config) to # copy configs that were passed on the cmdline (--config) to
# the repo ui # the repo ui
for sec, name, val in cfgs: for sec, name, val in cfgs:
req.repo.ui.setconfig(sec, name, val, source='--config') req.repo.ui.setconfig(sec, name, val, source='--config')
# developer config: ui.debugger # developer config: ui.debugger
debugger = ui.config("ui", "debugger") debugger = ui.config("ui", "debugger")
debugmod = pdb debugmod = pdb
if not debugger or ui.plain(): if not debugger or ui.plain():
# if we are in HGPLAIN mode, then disable custom debugging # if we are in HGPLAIN mode, then disable custom debugging
debugger = 'pdb' debugger = 'pdb'
elif '--debugger' in req.args: elif usedebugger:
# This import can be slow for fancy debuggers, so only # This import can be slow for fancy debuggers, so only
# do it when absolutely necessary, i.e. when actual # do it when absolutely necessary, i.e. when actual
# debugging has been requested # debugging has been requested
with demandimport.deactivated(): with demandimport.deactivated():
try: try:
debugmod = __import__(debugger) debugmod = __import__(debugger)
except ImportError: except ImportError:
pass # Leave debugmod = pdb pass # Leave debugmod = pdb
debugtrace[debugger] = debugmod.set_trace debugtrace[debugger] = debugmod.set_trace
debugmortem[debugger] = debugmod.post_mortem debugmortem[debugger] = debugmod.post_mortem
# enter the debugger before command execution # enter the debugger before command execution
if '--debugger' in req.args: if usedebugger:
ui.warn(_("entering debugger - " ui.warn(_("entering debugger - "
"type c to continue starting hg or h for help\n")) "type c to continue starting hg or h for help\n"))
if (debugger != 'pdb' and if (debugger != 'pdb' and
debugtrace[debugger] == debugtrace['pdb']): debugtrace[debugger] == debugtrace['pdb']):
ui.warn(_("%s debugger specified " ui.warn(_("%s debugger specified "
"but its module was not found\n") % debugger) "but its module was not found\n") % debugger)
with demandimport.deactivated(): with demandimport.deactivated():
debugtrace[debugger]() debugtrace[debugger]()
try: try:
return _dispatch(req) return _dispatch(req)
finally: finally:
ui.flush() ui.flush()
except: # re-raises except: # re-raises
# enter the debugger when we hit an exception # enter the debugger when we hit an exception
if '--debugger' in req.args: if usedebugger:
traceback.print_exc() traceback.print_exc()
debugmortem[debugger](sys.exc_info()[2]) debugmortem[debugger](sys.exc_info()[2])
raise raise
return _callcatch(ui, _runcatchfunc) return _callcatch(ui, _runcatchfunc)
def _callcatch(ui, func): def _callcatch(ui, func):
"""like scmutil.callcatch but handles more high-level exceptions about """like scmutil.callcatch but handles more high-level exceptions about
elif arg[:2] in shortopts: elif arg[:2] in shortopts:
# short option can have no following space, e.g. hg log -Rfoo # short option can have no following space, e.g. hg log -Rfoo
values.append(args.pop(pos)[2:]) values.append(args.pop(pos)[2:])
argcount -= 1 argcount -= 1
else: else:
pos += 1 pos += 1
return values return values
def _earlypeekboolopt(optname, args):
"""Return True or False for given boolean flag. Do not modify args.
>>> args = [b'x', b'--debugger', b'y']
>>> _earlypeekboolopt(b'--debugger', args)
True
>>> args = [b'x', b'--', b'--debugger', b'y']
>>> _earlypeekboolopt(b'--debugger', args)
False
lothiraldanUnsubmitted
Not Done

Just for the sake of testing, could you add a doctest with --debugger before --?

lothiraldan: Just for the sake of testing, could you add a doctest with `--debugger` before `--`?
"""
if optname in args:
pos = args.index(optname)
if '--' in args:
argcount = args.index('--')
return pos < argcount
else:
return True
return False
dlaxUnsubmitted
Not Done

I find the algorithm a bit clumsy (usage of both in and .index() for the same value), how about a for loop like that:

for arg in args:
    if arg == optname:
        return True
    if arg == '--':
        return False
return False
dlax: I find the algorithm a bit clumsy (usage of both `in` and `.index()` for the same value), how…
quarkAuthorUnsubmitted
Not Done

Good advice! Will change.

quark: Good advice! Will change.
def runcommand(lui, repo, cmd, fullargs, ui, options, d, cmdpats, cmdoptions): def runcommand(lui, repo, cmd, fullargs, ui, options, d, cmdpats, cmdoptions):
# run pre-hook, and abort if it fails # run pre-hook, and abort if it fails
hook.hook(lui, repo, "pre-%s" % cmd, True, args=" ".join(fullargs), hook.hook(lui, repo, "pre-%s" % cmd, True, args=" ".join(fullargs),
pats=cmdpats, opts=cmdoptions) pats=cmdpats, opts=cmdoptions)
try: try:
ret = _runcommand(ui, options, cmd, d) ret = _runcommand(ui, options, cmd, d)
# run post-hook, passing command result # run post-hook, passing command result
hook.hook(lui, repo, "post-%s" % cmd, False, args=" ".join(fullargs), hook.hook(lui, repo, "post-%s" % cmd, False, args=" ".join(fullargs),
rpath = _earlygetopt(["-R", "--repository", "--repo"], args) rpath = _earlygetopt(["-R", "--repository", "--repo"], args)
path, lui = _getlocal(ui, rpath) path, lui = _getlocal(ui, rpath)
uis = {ui, lui} uis = {ui, lui}
if req.repo: if req.repo:
uis.add(req.repo.ui) uis.add(req.repo.ui)
if '--profile' in args: if _earlypeekboolopt('--profile', args):
for ui_ in uis: for ui_ in uis:
ui_.setconfig('profiling', 'enabled', 'true', '--profile') ui_.setconfig('profiling', 'enabled', 'true', '--profile')
profile = lui.configbool('profiling', 'enabled') profile = lui.configbool('profiling', 'enabled')
with profiling.profile(lui, enabled=profile) as profiler: with profiling.profile(lui, enabled=profile) as profiler:
# Configure extensions in phases: uisetup, extsetup, cmdtable, and # Configure extensions in phases: uisetup, extsetup, cmdtable, and
# reposetup # reposetup
extensions.loadall(lui) extensions.loadall(lui)

tests/test-globalopts.t

subrepos Subrepositories subrepos Subrepositories
templating Template Usage templating Template Usage
urls URL Paths urls URL Paths
(use 'hg help -v' to show built-in aliases and global options) (use 'hg help -v' to show built-in aliases and global options)
Not tested: --debugger Not tested: --debugger
Flags should not be effective after --:
$ hg add -- --debugger --profile --traceback
--debugger: No such file or directory
--profile: No such file or directory
--traceback: No such file or directory
[1]