Quotes in saved graphs are expanded to HTML entities, which results in invalid query syntax for Graphite

[scianos]

We are currently following the development branch of Graphite-Web. After upgrading last week, we noticed that saved graphs with quotes in the entries result in the quotes being expanded to HTML entities. Example:
alias(scale(summarize(collectd.someserver.someservice.somemetric.time-to-prod,"1d","max"),0.0166667),"Daily+Maximum")

Should be:
alias(scale(summarize(collectd.someserver.someservice.somemetric.time-to-prod,"1d","max"),0.0166667),"Daily+Maximum")

This results in many saved graphs being broken. Unfortunately, I haven't been able to track back which package or change caused the difference in behavior.

[obfuscurity]

Perhaps #1662?

[dsongc]

We, scianos and I, made it working after removing escape in the code.
graphUrlParams[param[0]].append(escape(param[1]))
I think the issue is related to #1662

[scianos]

It definitely looks like the escaping of the quote to an HTML entity is related to the issue at hand. The question is how we implement a fix which both solves the XSS and allows existing graph URLs containing quotes to work (the internal patch we're using will re-open the XSS problem, so this isn't a solution we can/would want to commit back yet without further work).

[ybizeul]

I'm having lots of random issues recently with that kind of errors in the logs, could that be related?

I think it started happening with a recent Graphite update. I don't see any quote in the query at least in Grafana

KeyError: ({Suppress:("\") W:((){}...)}, u'netapp.perf.VYN.S18SAN.node.S18SAN-02.vol_summary.total_ops', 6, True, True)
KeyError: ("}", u'aliasByNode(highestAverage(netapp.perf.{CEL,HAN,MRI,NOR,ODH,PRI,SHA,TOK,VYN}.{S10SAN,S11SAN,S13SAN,S16SAN,S18SAN,S25SAN,S32SAN,S34SAN,S37SAN}.node.*.vol_summary.total_ops, 4), 5)', 75, True, True)
KeyError: ({{Suppress:("\") W:((){}...)} | W:(!#%$...)}, u'netapp.perf.VYN.S18SAN.node.S18SAN-02.vol_summary.total_ops', 12, False, True)
KeyError: ({{{Suppress:("\") W:((){}...)} | W:(!#%$...)}}..., u'netapp.perf.HAN.S37SAN.node.S37SAN-02.vol_summary.total_data', 15, False, True)
KeyError: ("\", u'aliasByNode(highestAverage(netapp.perf.{CEL,HAN,MRI,NOR,ODH,PRI,SHA,TOK,VYN}.{S10SAN,S11SAN,S13SAN,S16SAN,S18SAN,S25SAN,S32SAN,S34SAN,S37SAN}.node.*.vol_summary.avg_latency, 5), 5)', 33, False, True)

[mcostacano]

I have been thinking about this bug and I have arrived to the conclusion the xss protection from #1662 is broken. urlencode works because you do not want your users submiting '"<> and other garbage. But in graphite we need the " or '. And there is no way to protect from XSS if you have " in a url, eg: target=" onload="nasty()".

The threat model in #1470 does not apply to me, because we know personally everybody who has account and can save a graph. We use drawAsInfinite(events('deploy')) very much, so whe have undone the changes from #1662 and live is good again. But I understand graphite as a project can't ignore a XSS. I am also able to fix this if my patches have any posibility to get commited. I think there are various solutions

Adding a configuration variable XSS_PROTECTION that can have 3 values NONE, NAIVE and FULL. NONE is obvious, everything in, everything out. FULL is as now, with urlencode. NAIVE would be our own custom filter, allowing ' but removing garbage in literals (naive because I am pretty sure it will have some hole). Graphite-web ships with default FULL and document this.

Implementing literal binding? target=drawAsInfinite(events(\1))&literal=deploy and urlencode. Very complex, and I am not sure even if it is viable.

As is, but urldecoding in evaluator.evaluateTokens and fixing the client side to unquote when editing. As it is now, the browser when editing the target shows drawAsInfinite(events(&#39;deploy&#39;))

[obfuscurity]

I'm very much in favor of this approach, with Graphite continuing to default to FULL.

[swills]

Defaulting to FULL means not functioning properly out of the box, no? And also requiring that users potentially reduce security in order for graphite web to function properly, if I'm understanding right. I would hope there is a better solution.

[louwrentius]

I did a clean install from source on a Raspberry Pi and I just hit this issue too. Am I missing something?

It seems to me that Graphite is fundamentally broken due to #1662 because a graph is broken the moment you save it.

How do people deal with this other than removing #1662 ?

[deniszh]

@louwrentius : looks like majority of users using 3rd party dashboards now

[louwrentius]

@deniszh I guess so, but still a bit sad because that 3rd party dashboard is an additional requirement I would not need. I don't really see a solution either, I'm not that into web programming but I understand maybe just enough that I think that - because of how the GUI works - it may not be fixable.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.