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

[Security] Use textContent instead innerHTML to remediate DOM based XSS #10537

Merged
merged 3 commits into from Oct 26, 2017

Conversation

Projects
None yet
4 participants
@qazbnm456
Contributor

qazbnm456 commented Sep 17, 2017

This commit is a security improvement for #10076, as @javan suggested, we should use textContent instead. Besides, I also revise some code to improve code quality.

Ref: https://www.owasp.org/index.php/DOM_based_XSS_Prevention_Cheat_Sheet

@qazbnm456 qazbnm456 changed the title from [Security] Use textContent instead innerHTML to remediateDOM based XSS to [Security] Use textContent instead innerHTML to remediate DOM based XSS Sep 17, 2017

const wrapper = `(function (code) {
function init() {
var styleElement = document.createElement('style');
styleElement.setAttribute('type', 'text/css');

This comment has been minimized.

@sindresorhus

sindresorhus Sep 17, 2017

Contributor

You don't have to set this. It's the default.

const runStylesheet = function (url, code) {
const wrapper = `(function (code) {
function init() {
var styleElement = document.createElement('style');

This comment has been minimized.

@sindresorhus

sindresorhus Sep 17, 2017

Contributor

var => const

@@ -23,12 +23,30 @@ const runContentScript = function (extensionId, url, code) {
return compiledWrapper.call(this, context.chrome)
}
const runStylesheet = function (url, code) {
const wrapper = `(function (code) {

This comment has been minimized.

@sindresorhus

sindresorhus Sep 17, 2017

Contributor

(code => {

// Run injected scripts.
// https://developer.chrome.com/extensions/content_scripts
const injectContentScript = function (extensionId, script) {
if (!script.matches.some(matchesPattern)) return
if (script.js) {
if (script.js.length) {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 17, 2017

Member

We want this to be script.js, if the length is 0 we don't care, the loop won't execute.

We need to check if the js property is defined at all

node.innerHTML = code
window.document.body.appendChild(node)
})
if (script.css.length) {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 17, 2017

Member

Same as above 👍

process.once('document-start', fire)
} else if (script.runAt === 'document_end') {
process.once('document-end', fire)
} else if (script.runAt === 'document_idle') {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Sep 17, 2017

Member

This should just be an else, the spec says document_idle is the default 👍

@qazbnm456

This comment has been minimized.

Contributor

qazbnm456 commented Sep 17, 2017

Thanks for your suggestions, @sindresorhus and @MarshallOfSound. I've made the changes already. 😄

@jkleinsc jkleinsc merged commit beb06c0 into electron:master Oct 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@qazbnm456 qazbnm456 deleted the qazbnm456:improve-content_scripts.css branch Oct 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment