Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

Patch XSS security vulnerability #1

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Patch XSS security vulnerability #1

merged 2 commits into from
Aug 24, 2023

Conversation

t-reedy
Copy link

@t-reedy t-reedy commented Aug 21, 2023

The vulnerability is described here: https://security.snyk.io/vuln/SNYK-JS-SEMANTICUI-174699.

There is no fix for this in the main Semantic UI repo, so this fix is inspired by the fix from Fomantic UI: fomantic/Fomantic-UI#298.

We can choose to merge this into this repository or not - this fix is included in the PR to move Semantic UI into the main repo: https://github.com/stqry/ticketing-cms/pull/533

@t-reedy t-reedy force-pushed the security-vulnerability branch from d6e0902 to bc1d586 Compare August 21, 2023 21:54
@t-reedy t-reedy force-pushed the security-vulnerability branch from bc1d586 to bb20f98 Compare August 21, 2023 21:55

// generates dropdown from select values
dropdown: function(select) {
var
placeholder = select.placeholder || false,
values = select.values || {},
html = ''
values = select.values || [],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did the default value change from an object to an array?

Copy link
Author

@t-reedy t-reedy Aug 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like a bug fix, previously var values was set but never used because it looped directly over select.values instead: https://github.com/stqry/Semantic-UI-CSS/pull/1/files#diff-449ac400a1bf6f602a60c591e25d914d36f949eecfc9fd282cb19a3e803e91cfL3845.

This change is taken from https://github.com/fomantic/Fomantic-UI/pull/298/files#diff-5ee8757d7a7e983335e14411dcc4f6dda835d6132a1e7f12ce38b2a6c7215493R4031. I assume changing the default was just a minor refactor, either way the result should be the same because it's only looped by the $.each, and the array or object is empty so nothing happens.

Copy link

@greendrake greendrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the new htmlEntities/escape function duplicated?
Also I second Bevan's question re the object/array.

@t-reedy
Copy link
Author

t-reedy commented Aug 22, 2023

Why is the new htmlEntities/escape function duplicated? Also I second Bevan's question re the object/array.

Deduplicated

@t-reedy t-reedy requested review from greendrake and BevanR August 22, 2023 04:00
Copy link

@greendrake greendrake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (assuming it doesn't break the DDLs and actually fixes the vulnerability, I didn't check).

@BevanR
Copy link

BevanR commented Aug 23, 2023

@greendrake Could you please manually test this one? I think you'll do a much more thorough test than I could. Testing can wait till it's on d1tix if you like.

@greendrake
Copy link

@BevanR I trust that Therese has tested it properly, so let's get it to D1tix for final verification. The test itself is plain simple and can't be "thorough" in principle: you just paste a <script> into the DDL and see whether/what it does in the console.

@BevanR BevanR merged commit a756d3c into master Aug 24, 2023
@BevanR BevanR deleted the security-vulnerability branch August 24, 2023 02:07
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants