Edgewall Software
Modify

Opened 13 years ago

Closed 7 years ago

Last modified 3 years ago

#6634 closed enhancement (fixed)

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

Reported by: Daniel Kahn Gillmor <dkg-debian.org@…> Owned by: Steffen Hoffmann
Priority: normal Milestone: 1.0.2
Component: ticket system Version: 0.11b1
Severity: normal Keywords:
Cc: lp-san@…, Ryan J Ollos Branch:
Release Notes:

Markup returned by ITicketManipulator.validate_ticket IWikiPageManipulator.validate_wiki_page is formatted to HTML.

API Changes:
Internal Changes:

Description

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"

Attachments (2)

20140421_ticket_manipulator_enable-rich-text-msg.patch (914 bytes ) - added by Steffen Hoffmann 7 years ago.
allow rich text in ticket manipulator messages
20140421_wiki_manipulator_enable-rich-text-msg.patch (1.1 KB ) - added by Steffen Hoffmann 7 years ago.
allow rich text in wiki page manipulator messages

Download all attachments as: .zip

Change History (19)

comment:1 by Emmanuel Blot, 13 years ago

Type: defectenhancement
Version: 0.11b1

comment:2 by Remy Blank, 12 years ago

Milestone: 0.13
Owner: changed from Jonas Borgström to Remy Blank

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

comment:3 by Remy Blank, 9 years ago

Owner: Remy Blank removed

To-do list cleanup.

comment:4 by lp-san@…, 7 years ago

Cc: lp-san@… added

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

in reply to:  4 comment:5 by Steffen Hoffmann, 7 years ago

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.

by Steffen Hoffmann, 7 years ago

allow rich text in ticket manipulator messages

in reply to:  2 comment:6 by Steffen Hoffmann, 7 years ago

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.

by Steffen Hoffmann, 7 years ago

allow rich text in wiki page manipulator messages

comment:7 by Steffen Hoffmann, 7 years ago

Cc: Ryan J Ollos added

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

comment:8 by Jun Omae, 7 years ago

We should use tag_(...) rather that tag(_(...)). Otherwise it leads XSS vulnerabilities.

-                    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))
Version 0, edited 7 years ago by Jun Omae (next)

comment:9 by Ryan J Ollos, 7 years ago

Milestone: next-major-releases1.0.2
Owner: set to Ryan J Ollos
Status: newassigned

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"))

in reply to:  9 comment:10 by Jun Omae, 7 years ago

- 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.

comment:11 by Ryan J Ollos, 7 years ago

Proposed changes in log:rjollos.git:t6634.

comment:12 by Jun Omae, 7 years ago

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"

comment:13 by Ryan J Ollos, 7 years ago

Owner: changed from Ryan J Ollos to Steffen Hoffmann
Release Notes: modified (diff)

comment:14 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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.

comment:15 by Ryan J Ollos, 6 years ago

Release Notes: modified (diff)

comment:16 by Ryan J Ollos, 3 years ago

This ticket overlooked IAttachmentManipulator. See #12840.

comment:17 by Ryan J Ollos, 3 years ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Steffen Hoffmann to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.