Skip to content

Critical XSS fixes#182

Open
hopperelec wants to merge 1 commit intoaliasaria:masterfrom
hopperelec:master
Open

Critical XSS fixes#182
hopperelec wants to merge 1 commit intoaliasaria:masterfrom
hopperelec:master

Conversation

@hopperelec
Copy link

@hopperelec hopperelec commented Feb 16, 2026

Thanks for the very quick response to the issue I created earlier, and sorry for my slower one!

sanitizer is currently being used to sanitize card content, but not to sanitize any other client-controlled values. As a result, cross-site scripting (XSS) is still possible through those other values.

Example reproduction steps

  1. Open any Scrumblr board
  2. Open the browser console
  3. Run the following JavaScript in the console:
createCard('xss', '', 0, 0, `0);" onmousemove="alert('XSS')" title="`, 'green', 'card');

This creates an arbitrary card with a rot that escapes the style attribute and injects an onmousemove event handler on the card element. This event handler could be used to execute any arbitrary JavaScript code. title=" is added at the end so that the following " is paired correctly.
4. Move the mouse over the card
5. An alert box with the message "XSS" will appear, confirming that the injected event handler was executed
6. Open the board as another user (e.g. in an incognito window) and move the mouse over the card to confirm that this is not just isolated to the user who created the card

Note that you could also inject more styles, such as to make the card take up the entire screen, to make the payload more impactful.

What this PR fixes

To enable this PR to be merged as quickly as possible, I have only added sanitization client-side and to values that I was able to achieve XSS with (i.e. those that are directly rendered in the DOM).

Specifically, it sanitizes these values (in order of appearance in script.js):

  • card ID
  • rot
  • card content
  • card colour (only to prevent XSS, see below)
  • stickerId (only to prevent XSS, see below)
  • theme
  • user name

What this PR does not fix

I recommend also making the following changes involving sanitization of client-controlled values, but I don't believe these can be used to achieve XSS so they are not as critical and I have not included them in this PR.

  • Also sanitize these values server-side, since it is generally best practice to sanitize untrusted input on both the client and server
    • Switch to a more robust sanitization library (such as DOMPurify) for server-side sanitization, since even the author of sanitizer does not recommend it, and it is no longer maintained
  • Prepend a prefix to card IDs (e.g. card-) to prevent a malicious user from creating an ID that matches an existing DOM element's ID, which could corrupt the DOM
    • then, load the card element using $(document.getElementById(cardId)) instead of $('#' + cardId), to prevent a malicious user from create a card with special characters in its ID which would also cause unexpected behaviour since it could target elements inside the card element instead of the card element itself
  • Validate that colour is in cardColours before loading the corresponding image, to prevent a malicious user from using an invalid colour which results in cards with a missing image
  • Validate stickerId before loading the corresponding image, to prevent a malicious user from using an invalid stickerId which results in stickers with a missing image
  • Validate theme before loading the corresponding CSS file, to prevent a malicious user from using an invalid theme which results in a missing CSS file and broken styling

…nt, card colour, stickerId, theme and user name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant