Edgewall Software
Modify

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#8750 closed defect (fixed)

system-messages displayed after redirect escape HTML

Reported by: ebray Owned by: Remy Blank
Priority: low Milestone: 0.11.6
Component: general Version: devel
Severity: minor Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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

     
    744744            for type_ in ['warnings', 'notices']:
    745745                try:
    746746                    for i in itertools.count():
    747                         req.chrome[type_].append(
    748                             req.session.pop('chrome.%s.%d' % (type_, i)))
     747                        req.chrome[type_].append(Markup(
     748                            req.session.pop('chrome.%s.%d' % (type_, i))))
    749749                except KeyError:
    750750                    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

     
    4545from trac.versioncontrol.diff import get_diff_options, diff_blocks
    4646from trac.web import arg_list_to_args, parse_arg_list, IRequestHandler
    4747from trac.web.chrome import add_link, add_script, add_stylesheet, \
    48                             add_warning, add_ctxtnav, prevnext_nav, Chrome, \
    49                             INavigationContributor, ITemplateProvider
     48                            add_notice, add_warning, add_ctxtnav, \
     49                            prevnext_nav, Chrome, INavigationContributor, \
     50                            ITemplateProvider
    5051from trac.wiki.formatter import format_to, format_to_html, format_to_oneliner
    5152
    5253class InvalidTicket(TracError):
     
    11741175            req.redirect(req.href.attachment('ticket', ticket.id,
    11751176                                             action='new'))
    11761177        if 'TICKET_VIEW' not in req.perm('ticket', ticket.id):
     1178            add_notice(req,
     1179                       tag_('Your ticket (%(ticketref)s) has been created.',
     1180                            ticketref=tag.a('#', ticket.id,
     1181                                            href=req.href.ticket(ticket.id))))
    11771182            req.redirect(req.href.newticket())
    11781183        req.redirect(req.href.ticket(ticket.id))

Attachments (0)

Change History (7)

comment:1 by Remy Blank, 13 years ago

Milestone: 0.11.6
Owner: set to 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.

comment:2 by Remy Blank, 13 years ago

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

comment:3 by Remy Blank, 13 years ago

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.

comment:4 by Christian Boos, 13 years ago

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

comment:5 by Remy Blank, 13 years ago

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

comment:6 by Remy Blank, 13 years ago

Resolution: fixed
Status: newclosed

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

in reply to:  4 comment:7 by ebray, 13 years ago

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank 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.