Skip to content

Commit

Permalink
Ignore certain data-* attributes in rails-ujs when element is content…
Browse files Browse the repository at this point in the history
…editable

There is a potential DOM based cross-site scripting issue in rails-ujs
which leverages the Clipboard API to target HTML elements that are
assigned the contenteditable attribute. This has the potential to occur
when pasting malicious HTML content from the clipboard that includes
a data-method, data-disable-with or data-remote attribute.

[CVE-2023-23913]
  • Loading branch information
fresh-eggs authored and jhawthorn committed Mar 13, 2023
1 parent 2174115 commit 8e34499
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 1 deletion.
10 changes: 10 additions & 0 deletions actionview/app/javascript/rails-ujs/features/disable.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
import { matches, getData, setData } from "../utils/dom"
import { stopEverything } from "../utils/event"
import { formElements } from "../utils/form"
import { isContentEditable } from "../utils/dom"

const handleDisabledElement = function(e) {
const element = this
Expand All @@ -24,6 +25,10 @@ const enableElement = (e) => {
element = e
}

if (isContentEditable(element)) {
return
}

if (matches(element, linkDisableSelector)) {
return enableLinkElement(element)
} else if (matches(element, buttonDisableSelector) || matches(element, formEnableSelector)) {
Expand All @@ -36,6 +41,11 @@ const enableElement = (e) => {
// Unified function to disable an element (link, button and form)
const disableElement = (e) => {
const element = e instanceof Event ? e.target : e

if (isContentEditable(element)) {
return
}

if (matches(element, linkDisableSelector)) {
return disableLinkElement(element)
} else if (matches(element, buttonDisableSelector) || matches(element, formDisableSelector)) {
Expand Down
5 changes: 5 additions & 0 deletions actionview/app/javascript/rails-ujs/features/method.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { isCrossDomain } from "../utils/ajax"
import * as csrf from "../utils/csrf"
import { stopEverything } from "../utils/event"
import { isContentEditable } from "../utils/dom"

// Handles "data-method" on links such as:
// <a href="/users/5" data-method="delete" rel="nofollow" data-confirm="Are you sure?">Delete</a>
Expand All @@ -9,6 +10,10 @@ const handleMethodWithRails = (rails) => function(e) {
const method = link.getAttribute("data-method")
if (!method) { return }

if (isContentEditable(this)) {
return
}

const href = rails.href(link)
const csrfToken = csrf.csrfToken()
const csrfParam = csrf.csrfParam()
Expand Down
6 changes: 6 additions & 0 deletions actionview/app/javascript/rails-ujs/features/remote.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { ajax, isCrossDomain } from "../utils/ajax"
import { matches, getData, setData } from "../utils/dom"
import { fire, stopEverything } from "../utils/event"
import { serializeElement } from "../utils/form"
import { isContentEditable } from "../utils/dom"

// Checks "data-remote" if true to handle the request through a XHR request.
const isRemote = function(element) {
Expand All @@ -21,6 +22,11 @@ const handleRemoteWithRails = (rails) => function(e) {
return false
}

if (isContentEditable(element)) {
fire(element, "ajax:stopped")
return false
}

const withCredentials = element.getAttribute("data-with-credentials")
const dataType = element.getAttribute("data-type") || "script"

Expand Down
16 changes: 15 additions & 1 deletion actionview/app/javascript/rails-ujs/utils/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,18 @@ const setData = function(element, key, value) {
// returns an Array
const $ = selector => Array.prototype.slice.call(document.querySelectorAll(selector))

export { matches, getData, setData, $ }
const isContentEditable = function(element) {
var isEditable = false
do {
if(element.isContentEditable) {
isEditable = true
break
}

element = element.parentElement
} while(element)

return isEditable
}

export { matches, getData, setData, $, isContentEditable }
23 changes: 23 additions & 0 deletions actionview/test/ujs/public/test/data-disable-with.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ QUnit.module('data-disable-with', {
'data-url': '/echo',
'data-disable-with': 'clicking...'
}))

$('#qunit-fixture').append($('<div />', {
id: 'edit-div', 'contenteditable': 'true'
}))
},
afterEach: function() {
$(document).unbind('iframe:loaded')
Expand Down Expand Up @@ -465,3 +469,22 @@ QUnit.test('button[data-remote][data-disable-with] re-enables when `ajax:error`
})
.triggerNative('click')
})

QUnit.test('form button with "data-disable-with" attribute and contenteditable is not modified', function(assert) {
const done = assert.async()
var form = $('form[data-remote]'), button = $('<button data-disable-with="submitting ..." name="submit2">Submit</button>')

var contenteditable_div = $('#qunit-fixture').find('div')
form.append(button)
contenteditable_div.append(form)

assert.enabledState(button, 'Submit')

setTimeout(function() {
assert.enabledState(button, 'Submit')
done()
}, 13)
form.triggerNative('submit')

assert.enabledState(button, 'Submit')
})
17 changes: 17 additions & 0 deletions actionview/test/ujs/public/test/data-method.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ QUnit.module('data-method', {
$('#qunit-fixture').append($('<a />', {
href: '/echo', 'data-method': 'delete', text: 'destroy!'
}))

$('#qunit-fixture').append($('<div />', {
id: 'edit-div', 'contenteditable': 'true'
}))
},
afterEach: function() {
$(document).unbind('iframe:loaded')
Expand Down Expand Up @@ -89,3 +93,16 @@ QUnit.test('link with "data-method" and cross origin', function(assert) {

assert.notEqual(data.authenticity_token, 'cf50faa3fe97702ca1ae')
})

QUnit.test('do not interact with contenteditable elements', function(assert) {
var contenteditable_div = $('#qunit-fixture').find('div')
contenteditable_div.append('<a href="http://www.shouldnevershowindocument.com" data-method="delete">')

var link = $('#edit-div').find('a')
link.triggerNative('click')

var collection = document.getElementsByTagName('form')
for (const item of collection) {
assert.notEqual(item.action, "http://www.shouldnevershowindocument.com/")
}
})
25 changes: 25 additions & 0 deletions actionview/test/ujs/public/test/data-remote.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ QUnit.module('data-remote', {
}))
.find('form').append($('<input type="text" name="user_name" value="john">'))

$('#qunit-fixture').append($('<div />', {
id: 'edit-div', 'contenteditable': 'true'
}))
}
})

Expand Down Expand Up @@ -574,3 +577,25 @@ QUnit.test('inputs inside disabled fieldset are not submitted on remote forms',
})
.triggerNative('submit')
})



QUnit.test('clicking on a link with contenteditable attribute does not fire ajaxyness', function(assert) {
const done = assert.async()
assert.expect(0)

var contenteditable_div = $('#qunit-fixture').find('div')
var link = $('a[data-remote]')
contenteditable_div.append(link)

link
.bindNative('ajax:beforeSend', function() {
assert.ok(false, 'ajax should not be triggered')
})
.bindNative('click', function(e) {
e.preventDefault()
})
.triggerNative('click')

setTimeout(function() { done() }, 20)
})

0 comments on commit 8e34499

Please sign in to comment.