UI tests show it broke the highlighting of the current searched site…

It turns out autocompletion highlighting is broken on master. Go on the demo and try to autocomplete sp and some HTML code shows up briefly. Same for la and other combinations.

I think the problem is that the current autocomplete doesn't clear previous highlighted stuff, so it keeps adding <span> tags again and again (nesting them), messing up the HTML.

What I suggest is to disable the autocompletion highlighting for now in this pull request and try to fix it later?

(quoted from the other issue)

That being said what do we do? I'm guessing fixing #6714 is not gonna happen. Fixing 1. and 2. to bypass Twig's escaping could make sense but 1. is used by other JS components so making it non-escaped anymore might break something else…

Here's an idea for a simpler hack that doesn't require skipping escaping via twig/angular (I think that's what's going on in this pull request): For 1), maybe we can create a new global var that uses the correct, unescaped site name. And for 2) can we fix users of the sitename attribute? Do you think this would be a better solution?

A slight precision on what this PR is doing: escaping manually the sites returned by the HTTP API (in the Angular directive) so that they are over-escaped like the other variables. Then I can echo those variables in Angular by skipping escaping (because all of them are already escaped).

Does this mean they are double encoded? Or I guess escaped once so it can be just inserted in HTML? I don't know what over-escaped means.

Your suggestion is nice, except for the new global variable that duplicates the existing one, that's adding clutter and confusion. E.g. how do we name the new variable? siteRaw and siteNameRaw (I think both of these exist)? Reminds me a bit of mysql_real_escape_string() that fixes mysql_escape_string() :p But maybe that's still a better way so why not!

Maybe siteNameUnescaped? I was thinking the old variable could be deprecated (in some manner), and maybe put out of use eventually when angular is used more in the UI. Of course that's quite a long way away, and #6714 could be accomplished well before then, so I guess it doesn't really matter how this is solved right now :)

Answering @diosmosis's comments which for some reason ended up in my comment :)

Does this mean they are double encoded? Or I guess escaped once so it can be just inserted in HTML? I don't know what over-escaped means.

No basically they are fetched from the HTTP API not escaped, i.e. once in the Javascript variable (in the Angular directive) they are not escaped, which is correct (no reason for a value to be stored escaped). At that point I manually escape them, so they end up "escaped" in the javascript variable, which I call "over-encoded" because at that point there is no reason that they should be escaped.

I do that because all other variables are stored escaped in memory (which they shouldn't ideally, but…): that way everything is consistently escaped in memory.

So when the variable is echo'ed by Angular into the HTML, it is normally escaped first (by Angular) but I disable that (given variables are already all escaped).

Maybe siteNameUnescaped? I was thinking the old variable could be deprecated (in some manner), and maybe put out of use eventually when angular is used more in the UI. Of course that's quite a long way away, and #6714 could be accomplished well before then, so I guess it doesn't really matter how this is solved right now :)

Yep why not. As you said "I guess it doesn't really matter how this is solved right now" I agree :) I've already spent 2 days of work on this bug (which is kind of depressing) I'd rather leave it be with the currently implemented solution unless we think another solution is much better

I'd rather leave it be with the currently implemented solution unless we think another solution is much better

I think it's a good solution that fixes the bug + introduces some consistency, so 👍 from me

@tsteur would you mind review this one and +1 if it's valid solution (and xss safe)?

I think it's XSS safe.

For a better understanding: Could the AngularJS part be also solved by using selectedSiteNameHtml here? https://github.com/piwik/piwik/pull/7552/files#diff-df586ad4d60f9625a811f797e5547ea6R33 then it should be safe I reckon.

Not sure how the SitesManager Controller change https://github.com/piwik/piwik/pull/7552/files#diff-d4d6201aa71999510eb45b144a549ef3R129 is related or whether it introduces any XSS. Couldn't find where it is used.

@tsteur I have answered your comment in the code parts you mentionned

I have fixed the html_entity_decode and rebased to be able to merge.