Listbox: support non ascii URL in all cases

product/ERP5Form/ListBox.py

@@ -2445,6 +2445,9 @@ class ListBoxHTMLRendererLine(ListBoxRendererLine):
           except AttributeError:
             pass
 
+      if isinstance(url, str):
+        url = unicode(url, encoding)
+
       if editable_field is not None:
         uid = self.getUid()
         key = '%s_%s' % (editable_field.getId(), uid)
@@ -2526,8 +2529,6 @@ class ListBoxHTMLRendererLine(ListBoxRendererLine):
         if url is not None:
           # JPS-XXX - I think we should not display a URL for objects
           # which do not have the View permission
-          if isinstance(url, str):
-            url = unicode(url, encoding)
           html = u'<a href="%s">%s</a>' % (url, html)
 
       html_list.append((html, original_value, error, editable_field, url))

[Jérome Perrin]

was !88 (merged)

[Romain Courteaud]

@jerome URL should be escaped.

cgi module could be used, like: cgi.escape(url, quote=True)

[Jérome Perrin]

@romain

We have two cases of URLs here:

• URLs generated by listbox internally, then there is a bit of manually escaping that does not handle &quote;
• URLs generated by URL method; then isn't the URL method responsible for escaping ? If we change listbox to escape URLs they might be escaped twice. Should Listbox be clever and try to detect this ?

( Just to understand correctly, this is not new behavior since that change, is it ? )

[Romain Courteaud]

@jerome this is not a new behaviour. But I just found this when looking at the commit.

There are some escaping in listbox but it doesn't seem to have any for urls.

I think it is responsability of the field to escape, or it will be impossible to trust it. It is already like this for all other fields (nobody manually escape field's default value)

[Jérome Perrin]

Listbox escapes the value because this is direct user input, so of course it has to be escaped, but in the case of URLs scripts, I feel it is a bit different because it is not user input but output from a method, so in a sense what we would not trust here is that the developer can write a correct URL method.

I used a script to list all URL columns, and some of them already escape, for example BusinessTemplate_getDiffUrl or AccountModule_getMirrorAccountUrl and some does not, for example ActivityTool_deleteMessage.

If we change listbox to quote URLs, there is a risk that we double encode, which is also incorrect. For example, if a script returns properly escaped URL such as /view?selection_index=3&selection_name=... and listbox escapes again this to /view?selection_index=3&selection_name=... that /view will recieve a parameter selection_index with value 3&selection_name=...

Anyway, I am OK to changing the rule to "listbox escapes URL from URL columns scripts", but this mean we have to change scripts which were already escaping not to escape.

[Jérome Perrin]

I noticed that link generated for anchor columns are already double escaped. I guess tal:attributes does some escaping.

@romain I cannot find time to work on that in the near future. How about we file a bug for now ?

[Jérome Perrin]

This anchore double escape was already reported as #1137