From 72903a65c3098dab96bd5ed686bf0b2afaefe940 Mon Sep 17 00:00:00 2001 From: David Golden Date: Thu, 21 Nov 2024 06:48:18 -0500 Subject: [PATCH 1/3] Revert "Add html sanitisation to front and backend" This reverts commit ecb0f5035041d7850898b2117d247b28d49cedd1. That commit escaped HTML to avoid XSS, but with the side effect that various special entities were encoded on save, and then re-encoded on the next save, leaving deck notes, etc. effectively un-editable from a practical perspective. --- app/Resources/views/layout.html.twig | 1 - src/AppBundle/Controller/BuilderController.php | 2 +- web/js/deck.v2.js | 2 +- web/js/decklist.v2.js | 6 +++--- web/js/sanitize.js | 11 ----------- web/js/zoom.js | 2 +- 6 files changed, 6 insertions(+), 18 deletions(-) delete mode 100644 web/js/sanitize.js diff --git a/app/Resources/views/layout.html.twig b/app/Resources/views/layout.html.twig index cb070f9f..d1eb908e 100755 --- a/app/Resources/views/layout.html.twig +++ b/app/Resources/views/layout.html.twig @@ -99,7 +99,6 @@ gtag('config', 'UA-131671930-1'); {% endif %} - diff --git a/src/AppBundle/Controller/BuilderController.php b/src/AppBundle/Controller/BuilderController.php index 134da914..a4a394a6 100755 --- a/src/AppBundle/Controller/BuilderController.php +++ b/src/AppBundle/Controller/BuilderController.php @@ -674,7 +674,7 @@ public function saveAction(Request $request, EntityManagerInterface $entityManag } $name = filter_var($request->get('name'), FILTER_SANITIZE_STRING, FILTER_FLAG_NO_ENCODE_QUOTES); $decklist_id = intval(filter_var($request->get('decklist_id'), FILTER_SANITIZE_NUMBER_INT)); - $description = filter_var(trim(($request->get('description'))), FILTER_SANITIZE_SPECIAL_CHARS); + $description = trim($request->get('description')); $tags = explode(',', filter_var($request->get('tags'), FILTER_SANITIZE_STRING, FILTER_FLAG_NO_ENCODE_QUOTES)); $mwl_code = $request->get('format_casual') ? null : $request->get('mwl_code'); diff --git a/web/js/deck.v2.js b/web/js/deck.v2.js index 1f217f31..24bb4514 100755 --- a/web/js/deck.v2.js +++ b/web/js/deck.v2.js @@ -462,7 +462,7 @@ $(function() { var converter = new Markdown.Converter(); $('#description').on('keyup', function() { $('#description-preview').html( - converter.makeHtml(escapeHtml($('#description').val()))); + converter.makeHtml($('#description').val())); }); $('#description').textcomplete([{ diff --git a/web/js/decklist.v2.js b/web/js/decklist.v2.js index 8b2c3e98..f21a7815 100755 --- a/web/js/decklist.v2.js +++ b/web/js/decklist.v2.js @@ -110,7 +110,7 @@ function setup_comment_form() { $('#comment-form-text').on( 'keyup', function () { - $('#comment-form-preview').html(converter.makeHtml(escapeHtml($('#comment-form-text').val()))); + $('#comment-form-preview').html(converter.makeHtml($('#comment-form-text').val())); } ); @@ -385,10 +385,10 @@ function edit_form() { var converter = new Markdown.Converter(); $('#publish-decklist-description-preview').html( - converter.makeHtml(escapeHtml($('#publish-decklist-description').val()))); + converter.makeHtml($('#publish-decklist-description').val())); $('#publish-decklist-description').on('keyup', function() { $('#publish-decklist-description-preview').html( - converter.makeHtml(escapeHtml($('#publish-decklist-description').val()))); + converter.makeHtml($('#publish-decklist-description').val())); }); $('#publish-decklist-description').textcomplete([{ diff --git a/web/js/sanitize.js b/web/js/sanitize.js deleted file mode 100644 index 5c4bcd13..00000000 --- a/web/js/sanitize.js +++ /dev/null @@ -1,11 +0,0 @@ -function escapeHtml(text) { - let map = { - '&': '&', - '<': '<', - '>': '>', - '"': '"', - "'": ''' - }; - - return text.replace(/[&<>"']/g, function(m) { return map[m]; }); -} diff --git a/web/js/zoom.js b/web/js/zoom.js index 38c86f18..f37b7c29 100755 --- a/web/js/zoom.js +++ b/web/js/zoom.js @@ -318,7 +318,7 @@ function write_review_open(event) { $('.review-form-text').on( 'keyup', function () { - $('.review-form-preview').html(converter.makeHtml(escapeHtml($('.review-form-text').val()))); + $('.review-form-preview').html(converter.makeHtml($('.review-form-text').val())); } ); From ba034e75f825b6d06c278f027ec6c084ec820e1b Mon Sep 17 00:00:00 2001 From: David Golden Date: Thu, 21 Nov 2024 07:58:28 -0500 Subject: [PATCH 2/3] Sanitize Javascript-rendered raw text Deck descriptions, comments, card reviews, etc. are stored in two forms. The 'rawtext' ('rawdescription', etc.) form has a mix of user-provided markdown and HTML. The 'text' form is pre-rendered HTML. When the HTML is rendered, it is converted from Markdown and sanitized to remove unwanted HTML entities, such as ` + diff --git a/web/js/deck.v2.js b/web/js/deck.v2.js index 24bb4514..b5aea833 100755 --- a/web/js/deck.v2.js +++ b/web/js/deck.v2.js @@ -462,7 +462,8 @@ $(function() { var converter = new Markdown.Converter(); $('#description').on('keyup', function() { $('#description-preview').html( - converter.makeHtml($('#description').val())); + DOMPurify.sanitize(converter.makeHtml($('#description').val())) + ); }); $('#description').textcomplete([{ diff --git a/web/js/decklist.v2.js b/web/js/decklist.v2.js index f21a7815..26f0d73d 100755 --- a/web/js/decklist.v2.js +++ b/web/js/decklist.v2.js @@ -110,7 +110,7 @@ function setup_comment_form() { $('#comment-form-text').on( 'keyup', function () { - $('#comment-form-preview').html(converter.makeHtml($('#comment-form-text').val())); + $('#comment-form-preview').html(DOMPurify.sanitize(converter.makeHtml($('#comment-form-text').val()))); } ); @@ -385,10 +385,12 @@ function edit_form() { var converter = new Markdown.Converter(); $('#publish-decklist-description-preview').html( - converter.makeHtml($('#publish-decklist-description').val())); + DOMPurify.sanitize(converter.makeHtml($('#publish-decklist-description').val())) + ); $('#publish-decklist-description').on('keyup', function() { $('#publish-decklist-description-preview').html( - converter.makeHtml($('#publish-decklist-description').val())); + DOMPurify.sanitize(converter.makeHtml($('#publish-decklist-description').val())) + ); }); $('#publish-decklist-description').textcomplete([{ diff --git a/web/js/deckview.v2.js b/web/js/deckview.v2.js index 5fbb4f3e..c5614355 100755 --- a/web/js/deckview.v2.js +++ b/web/js/deckview.v2.js @@ -57,7 +57,13 @@ $(function() { }); var converter = new Markdown.Converter(); - $('#description').html(converter.makeHtml(SelectedDeck.description ? SelectedDeck.description : 'No description.')); + $('#description').html( + DOMPurify.sanitize( + converter.makeHtml( + SelectedDeck.description ? SelectedDeck.description : 'No description.' + ) + ) + ); $('.btn-actions').on({ click: do_action_deck diff --git a/web/js/publish_deck_form.v2.js b/web/js/publish_deck_form.v2.js index 43c5e6f3..b5cba9dd 100644 --- a/web/js/publish_deck_form.v2.js +++ b/web/js/publish_deck_form.v2.js @@ -1,10 +1,13 @@ function initialize_publish_deck_form_typeahead() { var converter = new Markdown.Converter(); $('#publish-decklist-description-preview').html( - converter.makeHtml($('#publish-decklist-description').val())); + DOMPurify.sanitize(converter.makeHtml($('#publish-decklist-description').val())) + ); + $('#publish-decklist-description').on('keyup', function() { $('#publish-decklist-description-preview').html( - converter.makeHtml($('#publish-decklist-description').val())); + DOMPurify.sanitize(converter.makeHtml($('#publish-decklist-description').val())) + ); }); $('#publish-decklist-description').textcomplete([{ diff --git a/web/js/zoom.js b/web/js/zoom.js index f37b7c29..1ba96404 100755 --- a/web/js/zoom.js +++ b/web/js/zoom.js @@ -41,7 +41,7 @@ function add_ruling(event) { $('#add-ruling-form-text').on( 'keyup', function () { - $('#add-ruling-form-preview').html(converter.makeHtml($('#add-ruling-form-text').val())); + $('#add-ruling-form-preview').html(DOMPurify.sanitize(converter.makeHtml($('#add-ruling-form-text').val()))); } ); @@ -105,7 +105,7 @@ function edit_ruling(event) { $('#edit-ruling-form-text').on( 'keyup', function () { - $('#edit-ruling-form-preview').html(converter.makeHtml($('#edit-ruling-form-text').val())); + $('#edit-ruling-form-preview').html(DOMPurify.sanitize(converter.makeHtml($('#edit-ruling-form-text').val()))); } ); @@ -175,7 +175,7 @@ function write_comment(event) { $('.comment-form-text').on( 'keyup', function () { - $('.comment-form-preview').html(converter.makeHtml($('.comment-form-text').val())); + $('.comment-form-preview').html(DOMPurify.sanitize(converter.makeHtml($('.comment-form-text').val()))); } ); @@ -318,7 +318,7 @@ function write_review_open(event) { $('.review-form-text').on( 'keyup', function () { - $('.review-form-preview').html(converter.makeHtml($('.review-form-text').val())); + $('.review-form-preview').html(DOMPurify.sanitize(converter.makeHtml($('.review-form-text').val()))); } ); From 61620ef81e901c91d37bcb2898fd9c42bdab108e Mon Sep 17 00:00:00 2001 From: David Golden Date: Sat, 23 Nov 2024 09:53:14 -0500 Subject: [PATCH 3/3] sanitize when saving private deck descriptions --- src/AppBundle/Controller/BuilderController.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/AppBundle/Controller/BuilderController.php b/src/AppBundle/Controller/BuilderController.php index a4a394a6..f8c07854 100755 --- a/src/AppBundle/Controller/BuilderController.php +++ b/src/AppBundle/Controller/BuilderController.php @@ -12,6 +12,7 @@ use AppBundle\Service\CardsData; use AppBundle\Service\DeckManager; use AppBundle\Service\Judge; +use AppBundle\Service\TextProcessor; use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted; @@ -630,11 +631,12 @@ public function octgnExportAction(Deck $deck) * @param Request $request * @param EntityManagerInterface $entityManager * @param DeckManager $deckManager + * @param TextProcessor $textProcessor * @return \Symfony\Component\HttpFoundation\RedirectResponse|Response * * @IsGranted("IS_AUTHENTICATED_REMEMBERED") */ - public function saveAction(Request $request, EntityManagerInterface $entityManager, DeckManager $deckManager) + public function saveAction(Request $request, EntityManagerInterface $entityManager, DeckManager $deckManager, TextProcessor $textProcessor) { /** @var User $user */ $user = $this->getUser(); @@ -674,7 +676,7 @@ public function saveAction(Request $request, EntityManagerInterface $entityManag } $name = filter_var($request->get('name'), FILTER_SANITIZE_STRING, FILTER_FLAG_NO_ENCODE_QUOTES); $decklist_id = intval(filter_var($request->get('decklist_id'), FILTER_SANITIZE_NUMBER_INT)); - $description = trim($request->get('description')); + $description = $textProcessor->purify(trim($request->get('description'))); $tags = explode(',', filter_var($request->get('tags'), FILTER_SANITIZE_STRING, FILTER_FLAG_NO_ENCODE_QUOTES)); $mwl_code = $request->get('format_casual') ? null : $request->get('mwl_code');