Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Applies DOMSanitize.sanitize to innerHTML write #8388

Merged
merged 3 commits into from
Oct 29, 2020

Conversation

LukeWood
Copy link
Contributor

This commit sanitizes the text applied to innerHTML of the linkNode
attribute on OpenUriCommandHandler. This is an attempt to limit the
xss sink surface in Theia.

If there is no reason to do so or this breaks some intended behavior
please leave a comment indicating so.

Signed-off-by: Luke Wood lukewood@google.com

Change-Id: Ibee9829b3a44f87d15c9d46b9bc342332e5ccf08

What it does

sanitizes text assigned to innerHTML in an anchor element

How to test

This is not to address a specific bug - just a precaution.

@LukeWood
Copy link
Contributor Author

just accepted CLA

@LukeWood LukeWood force-pushed the commandfix branch 2 times, most recently from 6794ab6 to de798d9 Compare August 14, 2020 20:02
@akosyakov akosyakov added security issues related to security vscode issues related to VSCode compatibility labels Aug 17, 2020
These eslint rules attempt to surface XSS sinks from eslint.
This commit is a first pass on
eclipse-theia#8398

Change-Id: I142bbba9785c4567dfbea1380f5a980560cf7413
Signed-off-by: LukeWood <lukewoodcs@gmail.com>
@LukeWood
Copy link
Contributor Author

Any update on this one?

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Sorry about the delay, I didn't get the notification 👍
The changes look good to me, thank you for taking care of it!

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@LukeWood can you include the dompurify dependency as part of @theia/plugin-ext (at the moment it is only provided by @theia/preview)? This will ensure that if a downstream app includes @theia/plugin-ext and not @theia/preview it will still work correctly.

@vince-fugnitto
Copy link
Member

@LukeWood would you like me to take care of the outstanding issue #8388 (review) so it can be merged?

@LukeWood
Copy link
Contributor Author

apologies - I didn't realize there were still outstanding issues. If you could do that it would be great - I though I pushed a local copy but I must be mistaken

vince-fugnitto and others added 2 commits October 28, 2020 19:59
This commit sanitizes the text applied to innerHTML of the linkNode
attribute on OpenUriCommandHandler.  This is an attempt to limit the
xss sink surface in Theia.

If there is no reason to do so or this breaks some intended behavior
please leave a comment indicating so.

Change-Id: Ibee9829b3a44f87d15c9d46b9bc342332e5ccf08
Signed-off-by: Luke Wood <lukewoodcs@gmail.com>
@vince-fugnitto
Copy link
Member

apologies - I didn't realize there were still outstanding issues. If you could do that it would be great - I though I pushed a local copy but I must be mistaken

No problem! I updated the pull-request to include the necessary dependencies in the plugin-ext extension. I'll wait for CI to pass and likely merge tomorrow morning.

@vince-fugnitto vince-fugnitto merged commit fb3694b into eclipse-theia:master Oct 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security issues related to security vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants