From 685c354d0eaa48d2871ed723d0749244e60cb4a7 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Sat, 18 Jan 2020 10:44:35 +0100 Subject: [PATCH] several changes: - added tests for all 4 cases: output to string or into element vs first param contains link or not - cleaned up logic - skip HTML entity encoding only if we can ensure insertion to text node / when output to string, we always encode - DOMpurify sanitizes gopher, ws & wss links, which we previosly had tested for --- js/common.js | 2 +- js/privatebin.js | 44 ++++++++++--------- js/test/AttachmentViewer.js | 2 +- js/test/I18n.js | 84 +++++++++++++++++++++++++++++++++---- tpl/bootstrap.php | 2 +- tpl/page.php | 2 +- 6 files changed, 105 insertions(+), 31 deletions(-) diff --git a/js/common.js b/js/common.js index a13a6da..b153dfc 100644 --- a/js/common.js +++ b/js/common.js @@ -36,7 +36,7 @@ var a2zString = ['a','b','c','d','e','f','g','h','i','j','k','l','m', return c.toUpperCase(); }) ), - schemas = ['ftp','gopher','http','https','ws','wss'], + schemas = ['ftp','http','https'], supportedLanguages = ['de', 'es', 'fr', 'it', 'no', 'pl', 'pt', 'oc', 'ru', 'sl', 'zh'], mimeTypes = ['image/png', 'application/octet-stream'], formats = ['plaintext', 'markdown', 'syntaxhighlighting'], diff --git a/js/privatebin.js b/js/privatebin.js index 9db8b00..fda2d56 100644 --- a/js/privatebin.js +++ b/js/privatebin.js @@ -631,28 +631,35 @@ jQuery.PrivateBin = (function($, RawDeflate) { // messageID may contain links, but should be from a trusted source (code or translation JSON files) let containsLinks = args[0].indexOf(' 0) may never contain HTML as they may come from untrusted parties - if (i > 0 || !containsLinks) { - args[i] = Helper.htmlEntities(args[i]); + + // prevent double encoding, when we insert into a text node + if (!containsLinks || $element === null) { + for (let i = 0; i < args.length; ++i) { + // parameters (i > 0) may never contain HTML as they may come from untrusted parties + if (i > 0 || !containsLinks) { + args[i] = Helper.htmlEntities(args[i]); + } } } - // format string let output = Helper.sprintf.apply(this, args); - // if $element is given, apply text to element + if (containsLinks) { + // only allow tags/attributes we actually use in translations + output = DOMPurify.sanitize( + output, { + ALLOWED_TAGS: ['a', 'br', 'i', 'span'], + ALLOWED_ATTR: ['href', 'id'] + } + ); + } + + // if $element is given, insert translation if ($element !== null) { if (containsLinks) { - // only allow tags/attributes we actually use in our translations - $element.html( - DOMPurify.sanitize(output, { - ALLOWED_TAGS: ['a', 'br', 'i', 'span'], - ALLOWED_ATTR: ['href', 'id'] - }) - ); + $element.html(output); } else { - // avoid HTML entity encoding if translation contains no links + // text node takes care of entity encoding $element.text(output); } } @@ -1946,11 +1953,10 @@ jQuery.PrivateBin = (function($, RawDeflate) { */ me.createPasteNotification = function(url, deleteUrl) { - $('#pastelink').html( - I18n._( - 'Your paste is %s (Hit [Ctrl]+[c] to copy)', - url, url - ) + I18n._( + $('#pastelink'), + 'Your paste is %s (Hit [Ctrl]+[c] to copy)', + url, url ); // save newly created element $pasteUrl = $('#pasteurl'); diff --git a/js/test/AttachmentViewer.js b/js/test/AttachmentViewer.js index f3b3bb5..842945f 100644 --- a/js/test/AttachmentViewer.js +++ b/js/test/AttachmentViewer.js @@ -88,7 +88,7 @@ describe('AttachmentViewer', function () { if (prefix.indexOf('').html(prefix + $.PrivateBin.Helper.htmlEntities(filename) + postfix).html(); + result = prefix + $.PrivateBin.Helper.htmlEntities(filename) + postfix; } if (filename.length) { results.push( diff --git a/js/test/I18n.js b/js/test/I18n.js index 061f05b..0086cc2 100644 --- a/js/test/I18n.js +++ b/js/test/I18n.js @@ -3,6 +3,7 @@ var common = require('../common'); describe('I18n', function () { describe('translate', function () { + this.timeout(30000); before(function () { $.PrivateBin.I18n.reset(); }); @@ -45,13 +46,13 @@ describe('I18n', function () { 'string', function (prefix, params, postfix) { prefix = prefix.replace(/%(s|d)/g, '%%'); - params[0] = params[0].replace(/%(s|d)/g, '%%').replace(/%'; + params[0] = params[0].replace(/%(s|d)/g, '%%'); postfix = postfix.replace(/%(s|d)/g, '%%'); - var translation = $.PrivateBin.Helper.htmlEntities(prefix) + params[0] + $.PrivateBin.Helper.htmlEntities(postfix); + const translation = DOMPurify.sanitize( + prefix + $.PrivateBin.Helper.htmlEntities(params[0]) + '' + postfix, { + ALLOWED_TAGS: ['a', 'br', 'i', 'span'], + ALLOWED_ATTR: ['href', 'id'] + } + ); + params.unshift(prefix + '%s' + postfix); + const result = $.PrivateBin.I18n.translate.apply(this, params); + $.PrivateBin.I18n.reset(); + const alias = $.PrivateBin.I18n._.apply(this, params); + $.PrivateBin.I18n.reset(); + return translation === result && translation === alias; + } + ); + jsc.property( + 'replaces %s in strings with first given parameter into an element, encoding all, when no link is in the messageID', + 'string', + '(small nearray) string', + 'string', + function (prefix, params, postfix) { + prefix = prefix.replace(/%(s|d)/g, '%%'); + params[0] = params[0].replace(/%(s|d)/g, '%%').replace(/'); + params.unshift($('#i18n')); + $.PrivateBin.I18n.translate.apply(this, params); + const result = $('#i18n').text(); $.PrivateBin.I18n.reset(); - var alias = $.PrivateBin.I18n._.apply(this, params); + clean(); + clean = jsdom(); + $('body').html('
'); + params[0] = $('#i18n'); + $.PrivateBin.I18n._.apply(this, params); + const alias = $('#i18n').text(); $.PrivateBin.I18n.reset(); + clean(); + return translation === result && translation === alias; + } + ); + jsc.property( + 'replaces %s in strings with first given parameter into an element, encoding params only, when a link is part of the messageID inserted', + 'string', + '(small nearray) string', + 'string', + function (prefix, params, postfix) { + prefix = prefix.replace(/%(s|d)/g, '%%'); + params[0] = params[0].replace(/%(s|d)/g, '%%'); + postfix = postfix.replace(/%(s|d)/g, '%%'); + const translation = $('
').html(DOMPurify.sanitize( + prefix + $.PrivateBin.Helper.htmlEntities(params[0]) + '' + postfix, { + ALLOWED_TAGS: ['a', 'br', 'i', 'span'], + ALLOWED_ATTR: ['href', 'id'] + } + )).html(); + let args = Array.prototype.slice.call(params); + args.unshift(prefix + '%s' + postfix); + let clean = jsdom(); + $('body').html('
'); + args.unshift($('#i18n')); + $.PrivateBin.I18n.translate.apply(this, args); + const result = $('#i18n').html(); + $.PrivateBin.I18n.reset(); + clean(); + clean = jsdom(); + $('body').html('
'); + args[0] = $('#i18n'); + $.PrivateBin.I18n._.apply(this, args); + const alias = $('#i18n').html(); + $.PrivateBin.I18n.reset(); + clean(); return translation === result && translation === alias; } ); diff --git a/tpl/bootstrap.php b/tpl/bootstrap.php index 90607b8..04f0594 100644 --- a/tpl/bootstrap.php +++ b/tpl/bootstrap.php @@ -72,7 +72,7 @@ endif; ?> - + diff --git a/tpl/page.php b/tpl/page.php index 1cae5e5..c83374b 100644 --- a/tpl/page.php +++ b/tpl/page.php @@ -50,7 +50,7 @@ endif; ?> - +