[PATCH] Data that are passed to `add_script_data` are escaped by Genshi

[Jun Omae]

The Data that are passed to add_script_data are escaped by Genshi.

Code;

    add_script_data(req, {'test': '&<>"\''})

Result in html;

    <script type="text/javascript"> 
      var test="&amp;&lt;&gt;\"'";
    </script> 

The two values are not the same.

I think that to_json() should encode &, < and > to \u0026, \u003c and \u003e. And these characters are not escaped by Genshi. javascript_quote() has the same issue.

attachment:javascript-genshi.diff​

[Remy Blank]

Yes, they are escaped by Genshi, but the resulting string in JavaScript is still correct IIRC, as they are expanded by the browser when parsing the page (they are not in a CDATA section).

Do you actually have a use case for add_script_data() where a string is not passed correctly (i.e. intact) to JavaScript?

[Jun Omae]

I don't have any use cases, but it's able to confirm the issue.

Add the below code;

    add_script_data(req, {'test': '&<>"\''})

Then input javascript:alert(test) to the location bar in your browser.

I expect that the below text is shown.

&<>"'

[Remy Blank]

Ok, I thought I had tested this, but obviously I didn't quite succeed :) Could it be browser-specific? Anyway, I'll look into it.

[Jun Omae]

I think it isn't browser-specific. I tried in some browsers, the same happens in IE7, Firefox 3.6, Google Chrome 5, Safari 4 and Opera 10.

[Remy Blank]

Fixed an issue with query field labels, refactored.

[Remy Blank]

The patch above is slightly refactored and fixes an issue with query field labels:

• Moved escaping out of $.template(), as that function is generic and can be used for other purposes than generating HTML to be passed to $().
• Escaped the text passed to createLabel().
• Simplified a few expressions.

All tests pass (including the new ones), and working with ticket fields whose names and values contain "≠&" characters seems to work well in the custom query.

Would you mind testing the updated patch? Thanks.

[Christian Boos]

Replying to rblank:

The patch above is slightly refactored and fixes an issue with query field labels:

• Moved escaping out of $.template(), as that function is generic and can be used for other purposes than generating HTML to be passed to $().

… but is it really used for something else? I added it for that purpose only and don't expect that we would create HTML piecewise (i.e. pass HTML fragments as template parameters). If the name sounds too generic, we can use htmltemplate instead. Having to add explicitly htmlEscape to all parameters like you do in the patch doesn't feel optimal.

[Remy Blank]

The name is misleading, then. Also, that's what made me notice the missing escaping in createLabel(). But yeah, it's more typing, and prone to being forgotten.

About not creating HTML piecewise, we do it in Python (although mainly for translations), so I'd expect that to be useful in JavaScript as well at some point. Speaking of that, shouldn't babel.translate() also escape its arguments? My feeling is yes, except when we do want to insert HTML fragments. Maybe we should escape %s arguments, and have a separate %h format string for non-escaped HTML fragments?

Having said that, we could just have both $.template() and $.htmltemplate() (or just $.html(), even shorter), and use the latter instead of string concatenation in createLabel(). And maybe even rename $.template() to $.format() for consistency.

I'll update the patch shortly.

[Christian Boos]

Replying to rblank:

Having said that, we could just have both $.template()

Yes, better keep it anyway unchanged, for backwards compatibility.

and $.htmltemplate() (or just $.html(), even shorter)

$.html() would be too easily confused with ​$().html.

even rename $.template() to $.format() for consistency.

Yes, why not, $.format and $.htmlformat, with template kept as a backward compatibility alias for format.

[Remy Blank]

Ok. What about babel.format()?

[Christian Boos]

I'm not that fluent in Javascript, but I wonder if we couldn't find a similar approach to the Genshi Markup trick (special "subclass" of String that wouldn't be escaped). That way, we don't need to do anything special in the common case, but we would still have a possibility to pass markup when we really need it. Failing to do that, "%h" would be my fine as well.

[Remy Blank]

Introduce separate $.format() and $.htmlformat().

[Remy Blank]

The patch above has been updated according to comment:8.

About babel.format(), on second thought I don't think we should do any automatic escaping there, as its result is sometimes used as an argument to $.htmlformat(), where it would be escaped a second time. So I'd rather leave it as-is, and add explicit escaping if necessary.

Ok to apply?

[Christian Boos]

Two last nit picks: I've seen that every other methods we already have are capitalized, so we should use htmlFormat to be consistent. And we could avoid creating an identify function each time:

trac/htdocs/js/trac.js

@@ -83 +83 @@
   function format(str, args, escape) {
     var kwargs = args[args.length - 1];
     return str.replace(/\${?(\w+)}?/g, function(_, k) {
+      var repl;
       if (k.length == 1 && k >= '0' && k <= '9')
-        return escape(args[k - '0']);
+        repl = args[k - '0'];
       else
-        return escape(kwargs[k]);
+        repl = kwargs[k];
+      return escape ? escape(repl) : repl;
     });
   }
 
   // Expand positional ($1 .. $9) and keyword ($name) arguments in a string.
   // The htmlformat() version HTML-escapes arguments prior to substitution.
   $.format = function(str) {
-    return format(str, arguments, function(s) { return s; });
+    return format(str, arguments);
   }
 
   $.htmlformat = function(str) {

[Remy Blank]

Replying to cboos:

I've seen that every other methods we already have are capitalized, so we should use htmlFormat to be consistent.

I thought that was on purpose :)

And we could avoid creating an identify function each time:

Patch applied (including these two changes) in [9822].

[Christian Boos]

Replying to rblank:

Replying to cboos:

I've seen that every other methods we already have are capitalized, so we should use htmlFormat to be consistent.

I thought that was on purpose :)

No, I just wrote comment:6 after/while reading patches on Mercurial-devel ;-)