Skip to content

Commit

Permalink
composer: fix user saved graphs target escaping
Browse files Browse the repository at this point in the history
saved graphs targets were html-escaped in the json response
to fix an XSS vulnerability in graphite-project#1662

... but that was not really the right place to escape the graph targets,
it broke targets using quotes: graphite-project#1801 graphite-project#2334

so effectively revert the original fix, and instead html-escape the
targets just before rendering them in the GraphDataWindow Ext.ListView

also skip the `str()` around `graph.url`, it's already a string,
in both python2 and python3
  • Loading branch information
ploxiln committed Apr 20, 2020
1 parent c262680 commit 2dd8f49
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 28 deletions.
9 changes: 8 additions & 1 deletion webapp/content/js/composer_widgets.js
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,14 @@ var GraphDataWindow = {
hideHeaders: true,
width: 385,
height: 140,
columns: [ {header: 'Graph Targets', width: 1.0, dataIndex: 'value'} ],
columns: [
{
header: 'Graph Targets',
width: 1.0,
dataIndex: 'value',
tpl: '{value:htmlEncode}'
}
],
listeners: {
contextmenu: this.targetContextMenu,
afterrender: this.targetChanged,
Expand Down
29 changes: 2 additions & 27 deletions webapp/graphite/browser/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
from graphite.util import json
from graphite.logger import log
from hashlib import md5
from six.moves.urllib.parse import urlencode, urlparse, parse_qsl


def header(request):
Expand Down Expand Up @@ -138,19 +137,7 @@ def myGraphLookup(request):
else:
m = md5()
m.update(name.encode('utf-8'))

# Sanitize target
urlEscaped = str(graph.url)
graphUrl = urlparse(urlEscaped)
graphUrlParams = {}
graphUrlParams['target'] = []
for param in parse_qsl(graphUrl.query):
if param[0] != 'target':
graphUrlParams[param[0]] = param[1]
else:
graphUrlParams[param[0]].append(escape(param[1]))
urlEscaped = graphUrl._replace(query=urlencode(graphUrlParams, True)).geturl()
node.update( { 'id' : str(userpath_prefix + m.hexdigest()), 'graphUrl' : urlEscaped } )
node.update( { 'id' : str(userpath_prefix + m.hexdigest()), 'graphUrl' : graph.url } )
node.update(leafNode)

nodes.append(node)
Expand Down Expand Up @@ -237,22 +224,10 @@ def userGraphLookup(request):
m = md5()
m.update(nodeName.encode('utf-8'))

# Sanitize target
urlEscaped = str(graph.url)
graphUrl = urlparse(urlEscaped)
graphUrlParams = {}
graphUrlParams['target'] = []
for param in parse_qsl(graphUrl.query):
if param[0] != 'target':
graphUrlParams[param[0]] = param[1]
else:
graphUrlParams[param[0]].append(escape(param[1]))
urlEscaped = graphUrl._replace(query=urlencode(graphUrlParams, True)).geturl()

node = {
'text' : escape(nodeName),
'id' : username + '.' + prefix + m.hexdigest(),
'graphUrl' : urlEscaped,
'graphUrl' : graph.url,
}
node.update(leafNode)

Expand Down

0 comments on commit 2dd8f49

Please sign in to comment.