system-messages displayed after redirect escape HTML

[ebray]

When retrieving a notice/warning from a session attribute after a redirect, if that message was originally contained HTML, that HTML gets escaped. Here's how I fixed it:

trac/web/chrome.py

@@ -744 +744 @@
             for type_ in ['warnings', 'notices']:
                 try:
                     for i in itertools.count():
-                        req.chrome[type_].append(
-                            req.session.pop('chrome.%s.%d' % (type_, i)))
+                        req.chrome[type_].append(Markup(
+                            req.session.pop('chrome.%s.%d' % (type_, i))))
                 except KeyError:
                     pass

I noticed this while working on an unrelated issue, that I'll go ahead and mention here. When a user has TICKET_CREATE but not TICKET_VIEW, the default action of redirecting back to the new ticket page after the user submits a ticket is fine. But but there should be some notification to the user that their ticket was submitted successfully. This was a problem in one environment where this is actually a use case for some reason.

trac/ticket/web_ui.py

@@ -45 +45 @@
 from trac.versioncontrol.diff import get_diff_options, diff_blocks
 from trac.web import arg_list_to_args, parse_arg_list, IRequestHandler
 from trac.web.chrome import add_link, add_script, add_stylesheet, \
-                            add_warning, add_ctxtnav, prevnext_nav, Chrome, \
-                            INavigationContributor, ITemplateProvider
+                            add_notice, add_warning, add_ctxtnav, \
+                            prevnext_nav, Chrome, INavigationContributor, \
+                            ITemplateProvider
 from trac.wiki.formatter import format_to, format_to_html, format_to_oneliner
 
 class InvalidTicket(TracError):
@@ -1174 +1175 @@
             req.redirect(req.href.attachment('ticket', ticket.id,
                                              action='new'))
         if 'TICKET_VIEW' not in req.perm('ticket', ticket.id):
+            add_notice(req,
+                       tag_('Your ticket (%(ticketref)s) has been created.',
+                            ticketref=tag.a('#', ticket.id,
+                                            href=req.href.ticket(ticket.id))))
             req.redirect(req.href.newticket())
         req.redirect(req.href.ticket(ticket.id))

[Remy Blank]

Good catch, thanks Erik.

I'm not sure if wrapping all messages in Markup is the right solution, though. What if a non-HTML message contains a litteral "<"? I am afraid that we will have to store the type of the message in addition to the message itself, something like a "T" prefix for plain text and an "M" prefix for markup.

[Remy Blank]

… or even simpler, escape plain text messages when storing, and wrap in Markup on retrieval. Fixed in [8667].

[Remy Blank]

About the second issue: does it really make sense to have a link to the ticket in the message? Clicking on it will generate a permission error.

[Christian Boos]

In the unusual situation where someone can create a ticket without being able to see it afterwards, I think it's nevertheless useful that an unique id is given back for further reference and communication between the reporter and the ticket owner. To that end, an URL is better than just the ticket number, in case there would be multiple possible projects.

And the permission error also serves as an explicit confirmation that the user has not the permission to view the ticket. Maybe the notice could also be explicit about this, "Your ticket #… has been created but you don't have the permission to view it".

[Remy Blank]

I hadn't thought about being able to get the link. Good ideas. Will do.

[Remy Blank]

Improved notice committed to trunk in [8675], as the redirect to /newticket is only available there.

[ebray]

Replying to cboos:

In the unusual situation where someone can create a ticket without being able to see it afterwards, I think it's nevertheless useful that an unique id is given back for further reference and communication between the reporter and the ticket owner. To that end, an URL is better than just the ticket number, in case there would be multiple possible projects.

And the permission error also serves as an explicit confirmation that the user has not the permission to view the ticket. Maybe the notice could also be explicit about this, "Your ticket #… has been created but you don't have the permission to view it".

Couldn't have said it better myself ;) Yes, that was the reasoning behind making it a link.