Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

#9965 closed defect (fixed)

'&' in milestone name breaks custom report

Reported by: pyrhockz+trac@… Owned by: Remy Blank
Priority: normal Milestone: 0.12.2
Component: general Version: 0.12-stable
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

If a milestone is named 'Beta Foo&Bar', and a custom query is created (via the point-and-click custom query interface) with a constrain on the milestone being 'milestone = 'Beta Foo&Bar', then a query is broken and produces something like the following:

query:?owner=fooser
&
status=accepted
&
status=assigned
&
status=new
&
status=reopened
&
milestone=Beta+Foo
&
Bar
&
groupdesc=1

Attachments (2)

9965-query-urlencode-r10416.patch (4.5 KB ) - added by Remy Blank 12 years ago.
Don't quote safe characters in query URL when saving as a report.
9965-href-safe-r10416.patch (4.9 KB ) - added by Remy Blank 12 years ago.
Don't quote safe characters in Href.

Download all attachments as: .zip

Change History (10)

comment:1 by Remy Blank, 12 years ago

Milestone: 0.12.2
Owner: set to Remy Blank

Confirmed. Some escaping is in order.

comment:2 by Remy Blank, 12 years ago

This is due to [8465], fixing #8561. So that change wasn't so innocent after all. I'll revert the change, and still try to make the resulting query more readable (e.g. by not escaping "~").

comment:3 by anonymous, 12 years ago

Thank you.

by Remy Blank, 12 years ago

Don't quote safe characters in query URL when saving as a report.

by Remy Blank, 12 years ago

Attachment: 9965-href-safe-r10416.patch added

Don't quote safe characters in Href.

comment:4 by Remy Blank, 12 years ago

9965-query-urlencode-r10416.patch reverts [8465], and makes the generated query URLs slightly more readable by not quoting safe characters ("unreserved characters" as per RFC 2396, i.e. "-_.!~*'()"). A simpler solution could have been to un-quote those characters in Query.to_string() after retrieving the URL with get_href(), but read on.

According to RFC 2396:

Unreserved characters can be escaped without changing the semantics of the URI, but this should not be done unless the URI is being used in a context that does not allow the unescaped character to appear.

So it seems that we are quoting more than strictly necessary ("-_." aren't quoted by default, but the others are). 9965-href-safe-r10416.patch changes that default and avoids quoting all unreserved characters in Href. This only broke a handful of unit tests, which were easily fixed.

I would prefer the second solution, to follow the RFC. In that case, we could even make path_safe and query_safe module-level variables instead of attributes, as they will probably never change.

Opinions?

comment:5 by Christian Boos, 12 years ago

I verified the fix and I think the changes are safe. We could indeed hardcode the default, and nevertheless make it possible to override them by doing something like:

  • trac/web/href.py

    diff -r 3efb9c7de986 trac/web/href.py
    a b class Href(object):  
    132132    '/milestone/<look,here%3E?param=%3Chere,too>'
    133133    """
    134134
    135     def __init__(self, base, path_safe="/!~*'()", query_safe="!~*'()"):
     135    path_safe = "/!~*'()"
     136    query_safe = "!~*'()"
     137
     138    def __init__(self, base, path_safe=None, query_safe=None):
    136139        self.base = base.rstrip('/')
    137         self.path_safe = path_safe
    138         self.query_safe = query_safe
     140        if path_safe is not None:
     141            self.path_safe = path_safe
     142        if query_safe is not None:
     143            self.query_safe = query_safe
    139144        self._derived = {}
    140145
    141146    def __call__(self, *args, **kw):

in reply to:  5 ; comment:6 by Remy Blank, 12 years ago

Replying to cboos:

We could indeed hardcode the default, and nevertheless make it possible to override them by doing something like:

Isn't that exactly the same as default arguments, only more verbose? What I meant with "hardcode" was to make path_safe and query_safe module-level constants, with no provision to change them. This would make sense if we don't expect to ever change these values on a per-instance basis.

in reply to:  6 comment:7 by Christian Boos, 12 years ago

Replying to rblank:

Replying to cboos:

We could indeed hardcode the default, and nevertheless make it possible to override them by doing something like:

Isn't that exactly the same as default arguments, only more verbose?

Well, that's slightly more light-weight as the attributes won't be created unless needed. But you're probably right about the lack of need to change them.

This would make sense if we don't expect to ever change these values on a per-instance basis.

And if we ever need to, we would then still be able to set those attributes externally on a given Href instance.

comment:8 by Remy Blank, 12 years ago

Resolution: fixed
Status: newclosed

I have kept the more readable (IMO) version of using default arguments in [10420].

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.