Change Details

We need to resolve the issues discovered in T148133 * Inconsistent encodeURIComponent-ing of url parts in js. Even if the data is from what's probably a safe source, encodeURIComponent should be used to catch special characters, and so we can tell at a glance that it is safe. This is especially true when using JSONP, where there is even the remotest possibility of overriding the callback parameter. * Using target=_blank for external links without specifying rel=noopener =Normal issues= * 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). This cannot be exploited at present, but if two values were inserted on the same line it would be, so this should definitely be fixed, in case the js ever gets run through a minimizer, or something like that. * https://gerrit.wikimedia.org/r/338763 * All target=_blank links should have rel=noopener. * https://gerrit.wikimedia.org/r/338759 * 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. * 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.

We need to resolve the issues discovered in T148133 * Inconsistent encodeURIComponent-ing of url parts in js. Even if the data is from what's probably a safe source, encodeURIComponent should be used to catch special characters, and so we can tell at a glance that it is safe. This is especially true when using JSONP, where there is even the remotest possibility of overriding the callback parameter. * Using target=_blank for external links without specifying rel=noopener =Normal issues= * 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). This cannot be exploited at present, but if two values were inserted on the same line it would be, so this should definitely be fixed, in case the js ever gets run through a minimizer, or something like that. * https://gerrit.wikimedia.org/r/338763 * All target=_blank links should have rel=noopener. * https://gerrit.wikimedia.org/r/338759 * 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. * TODO: Follow up needed * line 46 of gf-preview.js, encodeURIComponent missing from jsonp queries. * https://gerrit.wikimedia.org/r/#/c/338961 =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.