Missing e-mail obfuscation in ticket properties + diff view

[osimons]

Discovered two more missing pieces of e-mail obfuscation:

1. When the owner or reporter is an e-mail address and gets updated, the change property when rendering the comment will not obfuscate the e-mail for those without permission to view it. Need to change in ticket.html.

2. If the description is updated, the change record will include a link to diff the change. This diff also contains any changed properties, and with the same use-case as above; if this includes owner or reporter with e-mail, it will not be obfuscated. This is a different template rendering the same properties - diff_div.html.

Added a patch that fixes this. The ticket.html changes should be fine, but I'm not quite sure on the possible uses of the div_diff.html elsewhere - here I've also added author to the list of names to obfuscate thinking that may be useful, but haven't actually seen anywhere that uses such a property name in diffs. Still, better safe than sorry - it will likely be a username if it does appear from somewhere that reuses the diff viewer?

Comments before I commit?

[osimons]

More e-mail obfuscation in ticket changelog and diff viewer.

[osimons]

Knowing what fields to obfuscate when diff'ing properties is not easy - certainly not when diff_div.html is intended for reuse by modules, including unkown plugins with unknown fields and content.

Instead of listing the fields specifically, would it be better to extend the .props data to contain an obfuscate flag? So that the new dataset (partial) looked like this:

...
             .props       - a list of property changes
               .name      - name of the property
               .diff      - rendered difference
               .obfuscate - boolean to enable obfuscation of content
               ...

BTW: Any particular reason why the diff_div.html template is not made as a macro? Would it not be easier to maintain and resuse that way and using argument passing, rather than depending on such a large set of specifically named global variables being present?

[Christian Boos]

Why not obfuscate before rendering and place it in $prop.rendered.content? Obfuscation is a kind of specific rendering after all.

And diff_div.html template is not going to be a macro as we're trying to get away from macros in favor of including templates, see ticket:6374#comment:1.

I'd be OK though to move all the global names (changes, diff, longcol, shortcol, no_id) to a single diff global name (the old diff becoming diff.options), so that a <py:with> around the <xi:include> would only have to rename one variable, if necessary.

[osimons]

Replying to cboos:

Why not obfuscate before rendering and place it in $prop.rendered.content? Obfuscation is a kind of specific rendering after all.

Does this not go contrary to the strategy of having full values in the data dictionary? So that other code for instance in post-processors can actually access and read data? Isn't that why we leave obfuscation to latest possible time, and have all current obfuscation happen in templates?

And diff_div.html template is not going to be a macro as we're trying to get away from macros in favor of including templates, see ticket:6374#comment:1.

I'd be OK though to move all the global names (changes, diff, longcol, shortcol, no_id) to a single diff global name (the old diff becoming diff.options), so that a <py:with> around the <xi:include> would only have to rename one variable, if necessary.

Right. OK. I have a plan for reusing the diff for one of my other plugins, and it makes more sense to make such changes in the context of such development (for me at least). Anyway, this is strictly not related to the obfuscation issue this ticket is about - more a discussion on the side.

⇒ Obfuscate in data or in template? My vote is still for template, but I'm game for anything. First dev to vote gets to have their way :-)

I want to plug this email-leak in time for 0.11 release.

[Christian Boos]

Replying to osimons:

Replying to cboos:

Why not obfuscate before rendering and place it in $prop.rendered.content? Obfuscation is a kind of specific rendering after all.

Does this not go contrary to the strategy of having full values in the data dictionary?

Even if $prop.rendered.content is set, this doesn't prevent $prop.value to be set as well.

Looking at _render_diff​, it seems that by refactoring this code and _render_property_diff, it's possible to achieve this.

_{(fighting with the wiki syntax - add some vspace here thanks)}

So that other code for instance in post-processors can actually access and read data?

Right, this should be possible.

[Christian Boos]

More precisely: for fixing this issue

trac/ticket/web_ui.py

@@ -1244 +1244 @@
         old_list, new_list = None, None
         render_elt = lambda x: x
         sep = ', '
-        if field == 'cc':
+        if field in ('cc', 'owner', 'reporter'):
             chrome = Chrome(self.env)
             old_list, new_list = chrome.cc_list(old), chrome.cc_list(new)
             if not (Chrome(self.env).show_email_addresses or

seems to be enough.

Now I've also verified that simply putting the old and new values in prop in all cases as I said above isn't going to work, as we already have some logic in the template to react on the presence or absence of either of them (set, removed …).

So if you think it's worth having the non-rendered original values somewhere, we can stick them in other keys, but note that this was not done so far (i.e. e-mail obfuscation isn't a special case).

[Christian Boos]

Follow-up to comment:5:

… we can stick them in other keys, but note that this was not done so far (i.e. e-mail obfuscation isn't a special case).

For example:

trac/ticket/web_ui.py

@@ -706 +706 @@
             if k not in text_fields:
                 old, new = old_ticket[k], new_ticket[k]
                 if old != new:
-                    props.append({'name': k})
+                    prop = {'name': k, 'raw': {'old': old, 'new': new}}
                     rendered = self._render_property_diff(req, ticket, k,
                                                           old, new, tnew)
                     if rendered:
-                        props[-1]['diff'] = tag.li('Property ', tag.strong(k),
-                                                   ' ', rendered)
+                        prop['diff'] = tag.li('Property ', tag.strong(k),
+                                              ' ', rendered)
                     else:
-                        props[-1]['old'] = {'name': k, 'value': old}
-                        props[-1]['new'] = {'name': k, 'value': new}
+                        prop['old'] = {'name': k, 'value': old}
+                        prop['new'] = {'name': k, 'value': new}
+                    props.append(prop)
         changes.append({'props': props, 'diffs': [],
                         'new': version_info(tnew),
                         'old': version_info(told)})

[osimons]

Doing obfuscation rendering in web_ui.py instead.

[osimons]

cboos, followed your pointer in comment:5. The 'cc' rendering worked, but it obviously used list-rendering instead of the plain field change rendering text. I ended up splitting it into its own if for 'owner' and 'reporter' that uses the same text as before for rendered content.

As far as I can see, inside 'changes' this makes non-obfuscated 'new' and 'old' values available in addition to the obfuscated 'rendered' fragment. That seems to be just fine, does it not?

OK with committing attachment:t6888-r6748-b.diff​ to solve both variations of property rendering (1 and 2 in description)?

[Christian Boos]

Replying to osimons:

cboos, followed your pointer in comment:5. The 'cc' rendering worked, but it obviously used list-rendering instead of the plain field change rendering text. I ended up splitting it into its own if for 'owner' and 'reporter' that uses the same text as before for rendered content.

Yes, I thought it was not a big deal to reuse the handling of CC list even in this case where the list had only one element, but if you think it's clearer to have a specific handling for reporter and owner, I'm OK with that.

As far as I can see, inside 'changes' this makes non-obfuscated 'new' and 'old' values available in addition to the obfuscated 'rendered' fragment. That seems to be just fine, does it not?

I don't think so, there will be either a .rendered field or the .new and .old ones. The diff_div.html template even relies on the fact that those .new and .old field can be there or not (the set or deleted logic). With the patch in comment:6, I've added a .raw field, with .raw.old and .raw.new values.

OK with committing attachment:t6888-r6748-b.diff​ to solve both variations of property rendering (1 and 2 in description)?

Sure.

[osimons]

Oh. I see. In my template debug I had both a 'change' and 'changes' variables - it existed in 'change' but not in 'changes' used by diff_div.html.

I did some minor modifications related to comment:6 that does not involve new fields, only adding 'new', 'old' and 'diff' if all exists and then checking the 'diff' first in the <py:when> checks in templates. That means use 'diff' if that exists, if not generate the change from 'new' and 'old'. Here is just that change:

trac/ticket/web_ui.py

@@ -713 +713 @@
                     if rendered:
                         props[-1]['diff'] = tag.li('Property ', tag.strong(k),
                                                    ' ', rendered)
-                    else:
-                        props[-1]['old'] = {'name': k, 'value': old}
-                        props[-1]['new'] = {'name': k, 'value': new}
+                    props[-1]['old'] = {'name': k, 'value': old}
+                    props[-1]['new'] = {'name': k, 'value': new}
         changes.append({'props': props, 'diffs': [],
                         'new': version_info(tnew),
                         'old': version_info(told)})

trac/templates/diff_div.html

@@ -49 +49 @@
             <py:with vars="one = prop.old or prop.new;
                            both = prop.old and prop.new;
                            action = both and 'changed from ' or not prop.old and 'set' or 'deleted'">
+              <py:when test="prop.diff">$prop.diff</py:when>
               <li py:when="one">
                 Property <strong py:attrs="one.rendered and one.rendered.name_attributes">
                   ${one.rendered and one.rendered.name or prop.name}</strong> $action
@@ -63 +64 @@
                 <py:if test="prop.new"> to ${render_property(prop.new)}</py:if>
               </li>
             </py:with>
-            <py:when test="prop.diff">$prop.diff</py:when>
           </py:for>
         </ul>
         <table py:if="item.diffs" class="$diff.style" summary="Differences" cellspacing="0"

I'm happy with this patch + attachment:t6888-r6748-b.diff​, and seems to work well in all the cases I could find to test. Still OK for commit?

[Christian Boos]

Yes, still OK, though I'd prefer something along the lines of

    prop = {'name': k, 
            'old': {'name': k, 'value': old},
            'new': {'name': k, 'value': new}}
    if rendered:
        prop['diff'] = rendered
    props.append(prop)
...

in ticket/web_ui.py, as I think it's clearer than the [-1] stuff.

Of course, ideally we should be able to go one step further and have something like

    prop = {'name': k, 'old': old, 'new': new}

but that would require farther reaching changes (in diff_div.html but also in wiki/web_ui.py, versioncontrol/web_ui/changeset.py), so I think better leave that alone for now unless you feel brave ;-)

[osimons]

Replying to cboos:

Yes, still OK, though I'd prefer something along the lines of…

Agree. Much more readable.

Of course, ideally we should be able to go one step further and have something like…

Nope. Not that brave just now - don't want to introduce new bugs with pending 0.11 release :-)

Committed in [6750]. Thanks for help, cboos :-)