Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Search,Dropdown,XSS] prevent JS execution from manual entries or untrusted remote data #298

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Dec 11, 2018

Description

This PR sanitizes the probably fetched and vulnerable HTML Data for dropdown user additions aswell as for search response values.

For search an additional option preserveHTML (default true) (adopted from dropdown where this is already available). When this is set to false every result value from the response stream will be sanitized in the default templates.

Testcase

unfixed dropdown
https://jsfiddle.net/9x8tqdam/

fixed dropdown
https://jsfiddle.net/9x8tqdam/1/

To test search you need to setup something locally to fake evil remote api data:

<div class="ui search">
  <div class="ui left icon input">
    <input class="prompt" type="text" placeholder="Search GitHub">
    <i class="github icon"></i>
  </div>
</div>

Initialize the search specifying a url which points to some evil prepared fakeapi.json-file to simulate a vulnerable remote api site

$('.ui.search')
  .search({
	preserveHTML: false, //add this to be safe to untrusted remote data
	apiSettings: {
	  url: 'fakeapi.json'
	},
	fields: {
	  title: 'name' //,
	},
	searchFields: [
	  'name'
	]
  })
;

The evil fakeapi.json could look like

{
  "success": true,
  "results": [{
    "name": "<script>alert('gotcha')</script>African Elephant",
    "value": "african-elephant"
  }, {
    "name": "African Wild Dog",
    "value": "african-wild dog"
  }, {
    "name": "Albacore Tuna",
    "value": "albacore-tuna"
  }]
}

Closes

Semantic-Org/Semantic-UI#4498
Semantic-Org/Semantic-UI#6571
Semantic-Org/Semantic-UI#3318

Relates to

Semantic-Org/Semantic-UI#6570
Semantic-Org/Semantic-UI#5376
Semantic-Org/Semantic-UI#1033
Semantic-Org/Semantic-UI#4592

To fulfill the demands of the above mentioned issues i will add appropriate security-informations to the docs once this PR is approved and merged

@lubber-de lubber-de added lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews tag/sui-issue Taken from an existing Issue/PR of SUI labels Dec 11, 2018
y0hami
y0hami previously approved these changes Dec 11, 2018
Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami added the type/security Anything related to security label Dec 11, 2018
@lubber-de lubber-de added the state/awaiting-docs Pull requests which need doc changes/additions label Dec 11, 2018
prudho
prudho previously approved these changes Dec 12, 2018
Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

LGTM

@lubber-de lubber-de dismissed stale reviews from prudho and y0hami via 23dd6cb December 12, 2018 08:55
@lubber-de
Copy link
Member Author

I forgot to sanitize the dropdown templates respecting the preserveHTML setting.
Please review again

Copy link
Member

@y0hami y0hami left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

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

LGTM

@y0hami y0hami removed the state/awaiting-reviews Pull requests which are waiting for reviews label Dec 13, 2018
@y0hami y0hami merged commit 0000ce9 into fomantic:develop Dec 13, 2018
@lubber-de lubber-de deleted the search_dropdown_xss_and_script_injection_prevention branch December 13, 2018 10:20
@lubber-de lubber-de added state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo and removed state/awaiting-docs Pull requests which need doc changes/additions labels Dec 13, 2018
This was referenced Dec 21, 2018
@lubber-de lubber-de added this to the 2.7.0 milestone Feb 6, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
lang/javascript Anything involving JavaScript state/has-docs A issue/PR which requires documentation changes and has the corresponding PR open in the docs repo tag/sui-issue Taken from an existing Issue/PR of SUI type/security Anything related to security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants