Edgewall Software

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#10771 closed defect (fixed)

`to_json` generates malformed json on Python 2.4-2.5 if contained single quotes — at Version 18

Reported by: Jun Omae Owned by: Jun Omae
Priority: normal Milestone: 1.0
Component: general Version: 0.12
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Correctly handle single quote in string encoded with to_json()

API Changes:
Internal Changes:

Description (last modified by Christian Boos)

to_json generates malformed json on Python 2.4-2.5 if the argument is contained single quotes. to_json uses javascript_quote because Python 2.4-2.5 don't have json library.

Python 2.4.3 (#1, Jun 18 2012, 08:55:31)
[GCC 4.1.2 20080704 (Red Hat 4.1.2-52)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from trac.util.presentation import to_json
>>> print to_json({'json': "a ' single quote"})
{"json":"a \' single quote"}

However, single quote cannot be encoded to \', according to http://www.json.org/.

And, jQuery.parseJSON raises SyntaxError for such a json string. This brings a problem when Trac and Trac plugins response json as contents.

[12:46:48.548] jQuery.parseJSON("{\"json\": \"a \\' single quote\"}")
[12:46:48.553] SyntaxError: JSON.parse: bad escaped character
[12:46:51.864] jQuery.parseJSON("{\"json\": \"a ' single quote\"}")
[12:46:51.868] ({json:"a ' single quote"})
[12:52:32.423] jQuery.parseJSON("{\"json\": \"a \\u0027 single quote\"}")
[12:52:32.427] ({json:"a ' single quote"})

I propose that javascript_quote encodes a single quote to \u0027 (neither ' nor \') in order to avoid \' in json text.

Change History (18)

comment:1 by Jun Omae, 12 years ago

The patch [e615826b/jomae.git] for 1.0dev is here. I think we should apply for 0.12-stable.

comment:2 by Remy Blank, 12 years ago

Good catch! But why encode the single quote, then? The JSON specification allows it as-is in strings, as the string delimiter is always the double quote. Is it to allow using both the single and the double quote around quoted JavaScript strings?

comment:3 by Jun Omae, 12 years ago

You're right about JSON.

If a plugin uses javascript_quote like the following, it will get a problem.

<script>
  <!--! Enclose single quote -->
  blah_func('${ javascript_quote(blah_plugin_var) }');
</script>

in reply to:  3 ; comment:4 by Christian Boos, 12 years ago

Replying to jomae:

You're right about JSON.

If a plugin uses javascript_quote like the following, it will get a problem.

<script>
  <!--! Enclose single quote -->
  blah_func('${ javascript_quote(blah_plugin_var) }');
</script>

But that would be a problem in the script then, as JSON can contain plain single quotes. So if this is really what they want to do (stringify some JSON instead of letting it be interpreted as Javascript code, what it is normally supposed to be used for), they ought to do the escaping themselves:

blah_func('${ javascript_quote(blah_plugin_var).replace("'", "\'")}');

in reply to:  4 ; comment:5 by Jun Omae, 12 years ago

Replying to cboos:

But that would be a problem in the script then, as JSON can contain plain single quotes. So if this is really what they want to do (stringify some JSON instead of letting it be interpreted as Javascript code, what it is normally supposed to be used for), they ought to do the escaping themselves:

blah_func('${ javascript_quote(blah_plugin_var).replace("'", "\'")}');

If possible, yes. And the following is simple than it if plugin maintainers will change.

blah_func("${ javascript_quote(blah_plugin_var) }");

Or

blah_func(${ to_json(blah_plugin_var) });

If we don't expect to use javascript_quote at enclosing with single quotes, the function shouldn't have escaped single quotes….

Ok. But I think that we should keep escaping the single quotes in javascript_quote for the compatibility.

New patch, [78218477/jomae.git], adds safe parameter to javvscript_quote. As the following, specifies the characters that should not be quoted.

     def to_json(value):
         """Encode `value` to JSON."""
         if isinstance(value, basestring):
            return '"%s"' % javascript_quote(value, safe="'")

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

Replying to jomae:

But I think that we should keep escaping the single quotes in javascript_quote for the compatibility.

I rather think there's a misunderstanding about the intent of javascript_quote. The doc string and our usage says: "Quote strings for inclusion in javascript", not "Quote strings for inclusion in Javascript strings"! So I think not encoding single quotes would be more correct.

We could have a javascript_string_quote helper function, and if you really want to have blah_func('${ javascript_quote(blah_plugin_var) }') working as you expect, then I'd rather have something explicit like:

def javascript_code_quote(text):
    # same as current javascript_quote, "'" not present in _js_quote 

def javascript_string_quote(text):
    return javascript_code_quote.replace("'", "\'")

javascript_quote = javascript_string_quote # compatibility with buggy usage...

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

… or javascript_quote_for_code and javascript_quote_for_string.

in reply to:  6 ; comment:8 by Jun Omae, 12 years ago

Replying to cboos:

… So I think not encoding single quotes would be more correct.

Hmm, Ok. I rewrited the patch, repos:jomae.git:ticket10771/well-formed-json.3/trunk. In the patch, javascript_quote does no longer encode single quotes.

Do we have no problems other than it? I'm wondering if it would be ok to apply. I think that we should apply both 0.12-stable and trunk.

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

Replying to cboos:

Replying to jomae:

But I think that we should keep escaping the single quotes in javascript_quote for the compatibility.

I rather think there's a misunderstanding about the intent of javascript_quote. The doc string and our usage says: "Quote strings for inclusion in javascript", not "Quote strings for inclusion in Javascript strings"!

Actually, I managed to get confused myself, mixing javascript_quote and to_json itself… Sorry about that. Of course it's meant to be used within Javascript strings as that's the only way you can embed arbitrary text in Javascript code.

in reply to:  8 comment:10 by Christian Boos, 12 years ago

Replying to jomae:

Do we have no problems other than it? I'm wondering if it would be ok to apply. I think that we should apply both 0.12-stable and trunk.

What about this alternative solution:

  • leaving javascript_quote as is
  • have a new to_js_string which leaves the ' character unescaped and produces a Javascript string complete with the double quotes
  • use to_js_string in to_json

Fix the API doc accordingly so that easily confusable people like me could understand it ;-)

  • javascript_quote:
    """Quote strings for inclusion in single or double quote delimited Javascript strings
    """
    
  • to_js_string:
    """Embed the given string in a double quote delimited Javascript string
    (conform to the JSON spec)
    """
    

With this approach, I think we can address both the JSON conformance issue and the use case of comment:3.

comment:11 by Christian Boos, 12 years ago

Description: modified (diff)
Summary: `to_json` generates malformed json on Python 2.4-2.5 if contained single qoutes`to_json` generates malformed json on Python 2.4-2.5 if contained single quotes

(fix typos)

comment:12 by Jun Omae, 12 years ago

Your suggestion looks good. I reworked at [8dfc6fd4/jomae.git].

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

Replying to jomae:

Your suggestion looks good. I reworked at [8dfc6fd4/jomae.git].

Thanks Jun, this is nearly exactly what I meant, only I suggested in comment:10 that to_js_string should produce the complete Javascript string, i.e. the double quotes as well, which leaves no possibility of misuse.

comment:14 by Jun Omae, 12 years ago

Oh, my misreading…. See [13a0427a/jomae.git].

in reply to:  14 comment:15 by Christian Boos, 12 years ago

Replying to jomae:

Oh, my misreading…. See [13a0427a/jomae.git].

Perfect! All tests pass, feel free to commit.

comment:16 by Jun Omae, 12 years ago

Resolution: fixed
Status: newclosed

Thanks a lot, Christian. Applied.

comment:17 by Remy Blank, 12 years ago

Owner: set to Jun Omae

comment:18 by Alex Willmer <alex@…>, 12 years ago

Release Notes: modified (diff)
Note: See TracTickets for help on using tickets.