We need to resolve the issues discovered in T148133
> Overall* Inconsistent encodeURIComponent-ing of url parts in js. Even if the data is from what's probably a safe source, this looks good.encodeURIComponent should be used to catch special characters, By and large the code seemand so we can tell at a glance that it is safe,. and most of the issues are either low severityThis is especially true when using JSONP, or very difficult to triggerwhere there is even the remotest possibility of overriding the callback parameter.
>
> There are two issues I saw come up multiple times. Both are relatively minor as far as security issues go, but still should be fixed:* Using target=_blank for external links without specifying rel=noopener
=Normal issues=
> * Inconsistent encodeURIComponent-ing of url parts in js* The escaping of js values inserted into web/index.html is incorrect. They are being escaped for html, but should be escaped for javascript. In particular, ?s=%0A or ?s=foo\ can both cause syntax errors, and other special characters will be escaped in ways that are not correct (albeit not a security issue). Even if the data is from what's probably a safe sourceThis cannot be exploited at present, encodeURIComponent should be used to catch special charactersbut if two values were inserted on the same line it would be, and so we can tell at a glance that it is safe.so this should definitely be fixed, This is especially true when using JSONPin case the js ever gets run through a minimizer, where there is even the remotest possibility of overriding the callback parameteror something like that.
> * Using target=_blank for external links without specifying rel=noopener * https://gerrit.wikimedia.org/r/338763
>
>
> The full list of issues are as follows:* All target=_blank links should have rel=noopener.
>
>=Normal issues= * https://gerrit.wikimedia.org/r/338759
> * The escaping of js values inserted into web/index.html is incorrect. They are being escaped for html, but should be escaped for javascript. In particular, ?s=%0A or ?s=foo\ can both cause syntax errors,* Line 93 gf-preview.js - The find and replace of `</head>` could lead to XSS. and other special characters will be escaped in ways that are not correct (albeit not a security issue)Consider if someone could add html like <div title="</head><img src=x onerror=alert(1)"> to the page. This cannot be exploited at presentis perfectly valid and safe prior to the regex (albeit MW usually doesn't allow that sort of thing, but if two values were inserted on the same line it would beextensions might), so this should definitely be fixed,but unsafe after the regex. in cas(In practice the js ever gets run through a minimizeris would be very hard to pull off as you would have to be able to insert the tag prior to the actual <head/>, or something likebut that.
> * All target=_blank links's not something we should have rel=noopener.y on)
> * Line 93 gf-preview.js - The find and replace of `</head>` could lead to XSS. Consider if someone could add html like <div title="</head><img src=x onerror=alert(1)"> to the page. This is perfectly valid and safe prior to the regex (albeit MW usually doesn't allow that sort of thing, but extensions might), but unsafe after the regex. (In practice this would be very hard to pull off as you would have to be able to insert the tag prior to the actual <head/>, but that's not something we should rely on) * https://gerrit.wikimedia.org/r/338788
> * line 106 of gf-preview.js. This doesn't put the html text inside quote marks (And also doesn't escape quotes). This will cause the page to be executed as javascript, which is not what is desired.
> * line 46 of gf-preview.js, encodeURIComponent missing from jsonp queries. * TODO: Follow up needed
>
>* line 46 of gf-preview.js, encodeURIComponent missing from jsonp queries.
=Minor=
> * It would be nice if trusted data from other servers would be escaped (langauge_pairs, language_to_domain_mappings) or failing that, checked if they contain an apostrophe and throw an exception.
> * Since you're already loading mw.RegExp.escape, the writeCookie() function in web/static/cookies.js should probably use it. (This is very minor)
> * line 70 of gf-input.js - Please encodeURIComponent s and t.
> * line 63-ish of gf-preview.js - encodeURIComponent of all the parts of the url.
> * * ditto gf-create.js
> * wm-typeahead.js doesn't seem to escape titles properly when doing syntax highlighting. (Perhaps this is more something I should bring up with the portal people)
>
>=Other comments=
> * You may want to consider using CORS instead of JSONP for api communication. The mediawiki api supports anonymous CORS if you provide an &origin=* url parameter. From what I understand this is more reliable than jsonp, and also less scary as it doesn't rely on script execution.