ITicketManipulator.validate_ticket() return values should be able to include markup

[Daniel Kahn Gillmor <dkg-debian.org@…>]

Since the error messages returned from validate_ticket() by implementations of ITicketManipulator are displayed to the user, it would be good if plugins providing that interface could return some form of markup in the error message.

For example, the error message:

You can't make that change because you are not part of the security team.

Might be better rendered as:

You can't make that change because you are not part of the security team

But i can't find a way to do that in 0.11b1.

asked about this on IRC, coderanger suggested:

"it should probably be fixed to allow Markup() and genshi.builder elements"

[Remy Blank]

Good idea. I'll put it on my to-do list.

[Remy Blank]

To-do list cleanup.

[lp-san@…]

Need this feature, too. Are you guys just silently discarded it?

[Steffen Hoffmann]

Replying to lp-san@…:

Need this feature, too. Are you guys just silently discarded it?

While I agree on the essence of your complaint, your solution doesn't help. Code rules.

[Steffen Hoffmann]

allow rich text in ticket manipulator messages

[Steffen Hoffmann]

Replying to rblank:

Good idea. I'll put it on my to-do list.

Bad idea, because this was a trivial issue but an uncircumventable restriction for plugin authors.

Anyway, don't waste more time but review and apply the fix​ now, please.

[Steffen Hoffmann]

allow rich text in wiki page manipulator messages

[Steffen Hoffmann]

While at it I suggest to not miss the chance to do the same for wiki page manipulators​.

[Jun Omae]

We should use tag_(...) rather that tag(_(...)). Otherwise it leads XSS vulnerabilities. (sorry I misunderstood as Markup.)

-                    add_warning(req, tag(_("The Wiki page field '%(field)s' "
-                                           "is invalid: %(message)s",
-                                           field=field, message=message)))
+                    add_warning(req, tag_("The Wiki page field '%(field)s' "
+                                          "is invalid: %(message)s",
+                                          field=field, message=message))

[Ryan J Ollos]

Thanks for the patch and code review done so far. I'm working on adding a functional test. The change looks pretty straightforward, but I was wondering why we need to wrap message in tag for this case:

- add_warning(req, message)
+ add_warning(req, tag(message))

It appears that the message is already rendered correctly for that case. Is the change needed for proper sanitizing of the markup?

Here is a small plugin that I used to do some testing:

from genshi.builder import tag
from trac.core import Component, implements
from trac.ticket.api import ITicketManipulator
from trac.util.translation import tag_


class TicketValidator(Component):
        implements(ITicketManipulator)

        def prepare_ticket(self, req, ticket, fields, actions):
                pass

        def validate_ticket(self, req, ticket):
                field = 'reporter'
                yield None, tag_("This ticket %(field)s is %(status)s.",
                                 field=tag.tt(field),
                                 status=tag.b("invalid"))
                yield field, tag_("This ticket %(field)s is %(status)s.",
                                  field=tag.tt(field),
                                  status=tag.b("invalid"))

[Jun Omae]

- add_warning(req, message)
+ add_warning(req, tag(message))

It appears that the message is already rendered correctly for that case. Is the change needed for proper sanitizing of the markup?

That change is not needed for sanitizing. The messages will be sanitized in tags/trac-1.0.1/trac/templates/theme.html@:53-54,60-61#L49​.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t6634.

[Jun Omae]

I like tag.strong rather than tag.b in [e3d32ba2/rjollos.git] for trunk.

Also, we should put a space between req and tag_ in trac/ticket/web_ui.py.

-                    add_warning(req,tag_("The ticket field %(field)s"
+                    add_warning(req, tag_("The ticket field %(field)s"

[Ryan J Ollos]

Thanks. I made the changes suggested in comment:12. Committed to 1.0-stable in [12738] and merged to trunk in [12739]. Change to markup applied to trunk in [12740]. Created ticket #11598 to replace b tags in the codebase.

[Ryan J Ollos]

This ticket overlooked IAttachmentManipulator. See #12840.