search

calzoneman / sync

Node.JS Server and JavaScript/HTML Client for synchronizing online media
Other
1.47k stars 231 forks source link

XSS in emote names #981

Closed BackgroundPony closed 7 months ago

BackgroundPony commented 7 months ago

Description of the Problem

Name an emote "onload="alert('xss')" and send that emote name in chat. Works for any user connected until they reload the page, at which point the emote name isnt converted to the image and therefore not loaded again

Xaekai commented 7 months ago

I can not replicate this.

calzoneman commented 7 months ago

That's because I hotpatched it before I went to bed

On Wed, Apr 17, 2024, 17:50 Xaekai @.***> wrote:

I can not replicate this.

— Reply to this email directly, view it on GitHub https://github.com/calzoneman/sync/issues/981#issuecomment-2062792089, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6FCC3WU34EJ5QG2IWFELY54KFHAVCNFSM6AAAAABGKI2AHGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRSG44TEMBYHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

BackgroundPony commented 7 months ago

issue still persists "onload="alert('xss')", ensure no spaces in emote name Emote names created with single quotes ' ' cannot be deleted, or renamed

calzoneman commented 7 months ago

issue still persists

Indeed, the "add emote" path was missed in the previous hotfix. I believe this is more reliably addressed now since I had time to sit down and properly replace the HTML string-building code with DOM construction.

Emote names created with single quotes ' ' cannot be deleted, or renamed

This is a UI bug, I think. It actually does get deleted on the server, but the UI loses track of it due to inconsistent escaping of the emote name. Obviously by the existence of the XSS issue, the emote code was not really carefully written to handle syntax characters.