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

2017-11-22 Thread quark (Jun Wu) 
quark abandoned this revision.
quark added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1483

To: quark, #hg-reviewers, yuja
Cc: yuja, lothiraldan, dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-11-22 Thread yuja (Yuya Nishihara) 
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1483

To: quark, #hg-reviewers, yuja
Cc: yuja, lothiraldan, dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-11-21 Thread lothiraldan (Boris Feld) 
lothiraldan added inline comments.

INLINE COMMENTS

> dispatch.py:706
> +>>> _earlypeekboolopt(b'--debugger', args)
> +False
> +"""

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1483

To: quark, #hg-reviewers
Cc: lothiraldan, dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-11-21 Thread quark (Jun Wu) 
quark updated this revision to Diff 3744.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D1483?vs=3740=3744

REVISION DETAIL
  https://phab.mercurial-scm.org/D1483

AFFECTED FILES
  mercurial/dispatch.py
  tests/test-globalopts.t

CHANGE DETAILS

diff --git a/tests/test-globalopts.t b/tests/test-globalopts.t
--- a/tests/test-globalopts.t
+++ b/tests/test-globalopts.t
@@ -459,3 +459,10 @@
 
 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]
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -147,7 +147,7 @@
 try:
 if not req.ui:
 req.ui = uimod.ui.load()
-if '--traceback' in req.args:
+if _earlypeekboolopt('--traceback', req.args):
 req.ui.setconfig('ui', 'traceback', 'on', '--traceback')
 
 # set ui streams from the request
@@ -250,6 +250,7 @@
 _('potentially unsafe serve --stdio invocation: %r') %
 (req.args,))
 
+usedebugger = _earlypeekboolopt('--debugger', req.args)
 try:
 debugger = 'pdb'
 debugtrace = {
@@ -275,7 +276,7 @@
 if not debugger or ui.plain():
 # if we are in HGPLAIN mode, then disable custom debugging
 debugger = 'pdb'
-elif '--debugger' in req.args:
+elif usedebugger:
 # This import can be slow for fancy debuggers, so only
 # do it when absolutely necessary, i.e. when actual
 # debugging has been requested
@@ -289,7 +290,7 @@
 debugmortem[debugger] = debugmod.post_mortem
 
 # enter the debugger before command execution
-if '--debugger' in req.args:
+if usedebugger:
 ui.warn(_("entering debugger - "
 "type c to continue starting hg or h for help\n"))
 
@@ -305,7 +306,7 @@
 ui.flush()
 except: # re-raises
 # enter the debugger when we hit an exception
-if '--debugger' in req.args:
+if usedebugger:
 traceback.print_exc()
 debugmortem[debugger](sys.exc_info()[2])
 raise
@@ -693,6 +694,24 @@
 pos += 1
 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
+"""
+for arg in args:
+if arg == optname:
+return True
+if arg == '--':
+return False
+return False
+
 def runcommand(lui, repo, cmd, fullargs, ui, options, d, cmdpats, cmdoptions):
 # run pre-hook, and abort if it fails
 hook.hook(lui, repo, "pre-%s" % cmd, True, args=" ".join(fullargs),
@@ -780,7 +799,7 @@
 if req.repo:
 uis.add(req.repo.ui)
 
-if '--profile' in args:
+if _earlypeekboolopt('--profile', args):
 for ui_ in uis:
 ui_.setconfig('profiling', 'enabled', 'true', '--profile')
 



To: quark, #hg-reviewers
Cc: dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-11-21 Thread quark (Jun Wu) 
quark added inline comments.

INLINE COMMENTS

> dlax wrote in dispatch.py:715
> 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

Good advice! Will change.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1483

To: quark, #hg-reviewers
Cc: dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-11-21 Thread dlax (Denis Laxalde) 
dlax added inline comments.

INLINE COMMENTS

> dispatch.py:715
> +return True
> +return False
> +

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1483

To: quark, #hg-reviewers
Cc: dlax, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-11-21 Thread quark (Jun Wu) 
quark updated this revision to Diff 3740.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D1483?vs=3739=3740

REVISION DETAIL
  https://phab.mercurial-scm.org/D1483

AFFECTED FILES
  mercurial/dispatch.py
  tests/test-globalopts.t

CHANGE DETAILS

diff --git a/tests/test-globalopts.t b/tests/test-globalopts.t
--- a/tests/test-globalopts.t
+++ b/tests/test-globalopts.t
@@ -459,3 +459,10 @@
 
 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]
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -147,7 +147,7 @@
 try:
 if not req.ui:
 req.ui = uimod.ui.load()
-if '--traceback' in req.args:
+if _earlypeekboolopt('--traceback', req.args):
 req.ui.setconfig('ui', 'traceback', 'on', '--traceback')
 
 # set ui streams from the request
@@ -250,6 +250,7 @@
 _('potentially unsafe serve --stdio invocation: %r') %
 (req.args,))
 
+usedebugger = _earlypeekboolopt('--debugger', req.args)
 try:
 debugger = 'pdb'
 debugtrace = {
@@ -275,7 +276,7 @@
 if not debugger or ui.plain():
 # if we are in HGPLAIN mode, then disable custom debugging
 debugger = 'pdb'
-elif '--debugger' in req.args:
+elif usedebugger:
 # This import can be slow for fancy debuggers, so only
 # do it when absolutely necessary, i.e. when actual
 # debugging has been requested
@@ -289,7 +290,7 @@
 debugmortem[debugger] = debugmod.post_mortem
 
 # enter the debugger before command execution
-if '--debugger' in req.args:
+if usedebugger:
 ui.warn(_("entering debugger - "
 "type c to continue starting hg or h for help\n"))
 
@@ -305,7 +306,7 @@
 ui.flush()
 except: # re-raises
 # enter the debugger when we hit an exception
-if '--debugger' in req.args:
+if usedebugger:
 traceback.print_exc()
 debugmortem[debugger](sys.exc_info()[2])
 raise
@@ -693,6 +694,26 @@
 pos += 1
 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
+"""
+if optname in args:
+pos = args.index(optname)
+if '--' in args:
+argcount = args.index('--')
+return pos < argcount
+else:
+return True
+return False
+
 def runcommand(lui, repo, cmd, fullargs, ui, options, d, cmdpats, cmdoptions):
 # run pre-hook, and abort if it fails
 hook.hook(lui, repo, "pre-%s" % cmd, True, args=" ".join(fullargs),
@@ -780,7 +801,7 @@
 if req.repo:
 uis.add(req.repo.ui)
 
-if '--profile' in args:
+if _earlypeekboolopt('--profile', args):
 for ui_ in uis:
 ui_.setconfig('profiling', 'enabled', 'true', '--profile')
 



To: quark, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-11-21 Thread quark (Jun Wu) 
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION 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 used for developers. So this BC looks fine.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1483

AFFECTED FILES
  mercurial/dispatch.py
  tests/test-globalopts.t

CHANGE DETAILS

diff --git a/tests/test-globalopts.t b/tests/test-globalopts.t
--- a/tests/test-globalopts.t
+++ b/tests/test-globalopts.t
@@ -459,3 +459,10 @@
 
 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]
diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -147,7 +147,7 @@
 try:
 if not req.ui:
 req.ui = uimod.ui.load()
-if '--traceback' in req.args:
+if _earlypeekboolopt('--traceback', req.args):
 req.ui.setconfig('ui', 'traceback', 'on', '--traceback')
 
 # set ui streams from the request
@@ -250,6 +250,7 @@
 _('potentially unsafe serve --stdio invocation: %r') %
 (req.args,))
 
+usedebugger = _earlypeekboolopt('--debugger', req.args)
 try:
 debugger = 'pdb'
 debugtrace = {
@@ -263,6 +264,7 @@
 # (e.g. to change trust settings for reading .hg/hgrc)
 cfgs = _parseconfig(req.ui, _earlygetopt(['--config'], req.args))
 
+
 if req.repo:
 # copy configs that were passed on the cmdline (--config) to
 # the repo ui
@@ -275,7 +277,7 @@
 if not debugger or ui.plain():
 # if we are in HGPLAIN mode, then disable custom debugging
 debugger = 'pdb'
-elif '--debugger' in req.args:
+elif usedebugger:
 # This import can be slow for fancy debuggers, so only
 # do it when absolutely necessary, i.e. when actual
 # debugging has been requested
@@ -289,7 +291,7 @@
 debugmortem[debugger] = debugmod.post_mortem
 
 # enter the debugger before command execution
-if '--debugger' in req.args:
+if usedebugger:
 ui.warn(_("entering debugger - "
 "type c to continue starting hg or h for help\n"))
 
@@ -305,7 +307,7 @@
 ui.flush()
 except: # re-raises
 # enter the debugger when we hit an exception
-if '--debugger' in req.args:
+if usedebugger:
 traceback.print_exc()
 debugmortem[debugger](sys.exc_info()[2])
 raise
@@ -693,6 +695,26 @@
 pos += 1
 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
+"""
+if optname in args:
+pos = args.index(optname)
+if '--' in args:
+argcount = args.index('--')
+return pos < argcount
+else:
+return True
+return False
+
 def runcommand(lui, repo, cmd, fullargs, ui, options, d, cmdpats, cmdoptions):
 # run pre-hook, and abort if it fails
 hook.hook(lui, repo, "pre-%s" % cmd, True, args=" ".join(fullargs),
@@ -780,7 +802,7 @@
 if req.repo:
 uis.add(req.repo.ui)
 
-if '--profile' in args:
+if _earlypeekboolopt('--profile', args):
 for ui_ in uis:
 ui_.setconfig('profiling', 'enabled', 'true', '--profile')
 



To: quark, #hg-reviewers
Cc: mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel