Patchwork D1136: templatefilters: defend against evil unicode strs in json filter

login
register
mail settings
Submitter phabricator
Date Oct. 17, 2017, 2:56 a.m.
Message ID <differential-rev-PHID-DREV-7ubbtx2nwtlnxzd2glz3-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/25073/
State Superseded
Headers show

Comments

phabricator - Oct. 17, 2017, 2:56 a.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We only want to do I/O in terms of bytes, so lets explode early
  instead of recursing forever.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/templatefilters.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 17, 2017, 12:23 p.m.
ryanmce added a comment.


  I'm not a python 3 expert by any means, but I'm not sold on this change.
  
  I think we need perhaps a programming error so it's clear its a bug and not something the user needs to fix in their command inputs.

INLINE COMMENTS

> templatefilters.py:241-242
> +        # hurt someone.
> +        raise TypeError(
> +            r'Mercurial only does output with bytes on Python 3: %r' % obj)
>      elif util.safehasattr(obj, 'keys'):

I fear that this is is an non-actionable error. How might I see it as a user? If I see it, what do I do?

Also, why is this a TypeError? Should this be a ProgrammingError instead? Or is there a way user input could trigger this?

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers
Cc: ryanmce, mercurial-devel
phabricator - Oct. 17, 2017, 12:39 p.m.
ryanmce added a comment.


  I'll queue this series except this patch so we can have more discussion on what we should do here.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers
Cc: ryanmce, mercurial-devel
phabricator - Oct. 17, 2017, 1:50 p.m.
yuja requested changes to this revision.
yuja added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ryanmce wrote in templatefilters.py:241-242
> I fear that this is is an non-actionable error. How might I see it as a user? If I see it, what do I do?
> 
> Also, why is this a TypeError? Should this be a ProgrammingError instead? Or is there a way user input could trigger this?

It's okay to be non actionable error because it's plain wrong
to pass unicode to Mercurial functions. But I agree it should be
a ProgrammingError. TypeError in filter functions is caught
at templater.runfilter().

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, yuja
Cc: yuja, ryanmce, mercurial-devel
phabricator - Oct. 18, 2017, 12:22 p.m.
yuja added a comment.


  Queued. I've added missed import and dropped r'' from the error message.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, yuja
Cc: yuja, ryanmce, mercurial-devel

Patch

diff --git a/mercurial/templatefilters.py b/mercurial/templatefilters.py
--- a/mercurial/templatefilters.py
+++ b/mercurial/templatefilters.py
@@ -233,6 +233,13 @@ 
         return pycompat.bytestr(obj)
     elif isinstance(obj, bytes):
         return '"%s"' % encoding.jsonescape(obj, paranoid=paranoid)
+    elif isinstance(obj, str):
+        # This branch is unreachable on Python 2, because bytes == str
+        # and we'll return in the next-earlier block in the elif
+        # ladder. On Python 3, this helps us catch bugs before they
+        # hurt someone.
+        raise TypeError(
+            r'Mercurial only does output with bytes on Python 3: %r' % obj)
     elif util.safehasattr(obj, 'keys'):
         out = ['"%s": %s' % (encoding.jsonescape(k, paranoid=paranoid),
                              json(v, paranoid))