*** Bug 721336 has been marked as a duplicate of this bug. ***

Created attachment 265635 [details] [review]
Proposed patch (v1)

Though it is possible I missed some addition places where other strings should also be escaped, the attached patch fixes all the issues I have observed.

Review of attachment 265635 [details] [review]:

I think that we should use an "html_escape" api instead, because I suspect we'll be hit by any special character, not just quotes.
(Maybe I'm wrong?)

Created attachment 265887 [details] [review]
Proposed patch (v2)

Created attachment 265888 [details] [review]
Proposed patch (v2)

This version of the patch also escapes the necessary HTML characters, and does so in a slightly cleaner manner than v1. I am not sure this is the proper place to put a function of this sort, but it looked like a decent candidate.

(Sorry for the double-post of v2 of the patch - the first attempt threw a 500 error code at me, but apparently worked anyway.)

Created attachment 266042 [details] [review]
Proposed patch (v2)

Update commit message to reflect that the patch also escapes HTML characters.

Comment on attachment 266042 [details] [review]
Proposed patch (v2)

Thank you for your patch. It works well except for one oddity:
an '&' sign gets translated to 'and' in the report. In my language (Dutch) that gives odd results:
'Lonen & Wedden' suddenly becomes 'Lonen and Wedden'

I checked the generated html: it already contains the conversion. Instead of '&' it has 'and'. So it seems this conversion happens somewhere in our guile code (or is even something internal to guile). Note that I'm running guile 2. You may see a different result with guile 1.8.

Contrary to Derek's earlier review, I'm not even sure we should escape the html entities. The labels and titles aren't used in html directly, but as variable assignments in javascript.

Can you check into this ?

Hmm, I looked a bit closer at this. The '&' sign gets escaped in src/report/standard-reports/account-piecharts.scm, line 488 and further. So your escape function never sees it. It was like that before your patch as well. It's a wrong approach, but an indepdendent bug, so I'll ignore it here.

While looking, I also found that src/report/report-system/html-piechart.scm already contained an escape function, but due to a coding mistake it was actually never used: the escape function starts in line 161, it is used in line 204 to create a string of all escaped labels concatenated. However in line 236 the original label list is used again (probably because a concatenated stringlist wouldn't work here).

I haven't looked any further, but I suspect the other chart reports and jqplot chart generators have similar escape functions.

I'm in favour of your approach of have only one escape function that can be reused in several spots. But we should take care that the escape action actually matches the language in which the string will end up. So we may need two escape functions: one for strings that end up in an html context and one for strings that will end up in the javascript context.

Yes, the strings being escaped that this patch touches are directly being inserted into Javascript strings. Unfortunately, jqplot then turns these strings into generated HTML. If you attempt to generate a chart with a '<' in the middle of the account string, it will generate invalid HTML. I think I remember that on my system the renderer ended up dealing with it "gracefully" by truncating the rest of the account name.

I think I'll have to re-read that 'catenate-escaped-strings' function a few times later tonight when I have more time to figure out exactly what it's doing - not being fluent in scheme, I thought it was just concatenating lists of strings and not really doing any escaping itself.

Isn't there a javascript function to escape html sensitive characters ? That would probably be a cleaner solution and handle more cases. The only escaping that would have to happen beforehand would then be the quotes escaping (and perhaps the backslash).

I found this page: http://www.the-art-of-web.com/javascript/escape/
which compares 3 different javascript escape functions (and some php ones not relevant here). Could any of these be useful ?

Unfortunately, all of the javascript escaping functions at that page handle doing URL encoding, rather than escaping the characters so they will properly display when displayed as HTML.

I found some documentation that seems to say jqplot does some sort of escaping of HTML characters in some places, but I can't find any documentation saying it works for the labels as they're being used in the legend here (and grepping through the code didn't easily reveal anything either).

At any rate, Adding a function like the following to the Javascript on the page and then calling it on the string of each label seems to work (though lots of calls to it scattered everywhere through the Javascript code seems a little ugly to me):
function escape_html(to_escape) {
  return $("<div />").text(to_escape).html()
}

You were correct to assume that there is a copy of catenate-escaped-strings in each of html-piechart.scm, -barchart, and -linechart.

Even upon further inspection, I still don't know exactly what that function is doing. It appears it is only escaping spaces and backslashes, but I don't claim to understand more than that.

Unfortunately, if this patch is going to involve writing Scheme any more complex than my existing patch, I think it will be better for everyone involved if I'm not the one to do it. I understand Scheme only superficially as someone who uses plenty of other programming languages and can guess his way to something that works, not as someone who actually understands the mechanics of the language.

(In reply to comment #13)
> You were correct to assume that there is a copy of catenate-escaped-strings in
> each of html-piechart.scm, -barchart, and -linechart.
> 
> Even upon further inspection, I still don't know exactly what that function is
> doing. It appears it is only escaping spaces and backslashes, but I don't claim
> to understand more than that.
> 
From what I understand they do exactly that and then concatenates all the strings in the list together into one string. This string is then never used (at least not in the piechart code)

> Unfortunately, if this patch is going to involve writing Scheme any more
> complex than my existing patch, I think it will be better for everyone involved
> if I'm not the one to do it. I understand Scheme only superficially as someone
> who uses plenty of other programming languages and can guess his way to
> something that works, not as someone who actually understands the mechanics of
> the language.

That was not my intention. Your code is good and your answers to my questions confirmed that even more. My intention is to use your code and drop the other code bits I pointed out because they're either not used or doing something we don't want. If there was a javascript native solution I would have liked that even more, but we haven't found one.

(In reply to comment #14)
> That was not my intention. Your code is good and your answers to my questions
> confirmed that even more. My intention is to use your code and drop the other
> code bits I pointed out because they're either not used or doing something we
> don't want. If there was a javascript native solution I would have liked that
> even more, but we haven't found one.

Okay, are you proposing to merge this patch and perform the cleanup in a separate one, or would you rather both be done at the same time?

I doubt we are going to find a JavaScript solution cleaner than the one at the end of comment 12 short of patching jqplot itself to do HTML escaping on these strings before inserting them as HTML.

Comment on attachment 266042 [details] [review]
Proposed patch (v2)

I have committed the patch as is in r23754. I see no evidence that we have to escape the html entities, but it doesn't hurt either. The reports work fine either way. I suppose javascript is smart enough to know what to do.

Thank you for your contribution !

By the way the catenate-escaped-strings is used more than I initially thought. Specifically in the bar chart it's used to convert a guile array of row/column labels in on single string that can be included in a javascript code snippet. So rightfully it does similar escape actions to your function. But it wraps this in a list-to-single-string function.

I'll just leave this code alone for now as it seems to work correctly. So I'll just close this bug as fixed. Thanks again!

GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=721768. Please update any external references or bookmarks.