Revision: 5145
Author: [email protected]
Date: Thu Nov 8 15:14:44 2012
Log: restrict form.submit
http://codereview.appspot.com/6493115
currently, guest code can call form.submit() at any time.
this does not seem to be a serious problem, because those
submits usually trigger the pop-up blocker, but there
might still be potential for mischief.
this change restricts it so that the submit is ignored
unless it's executed during a user action, like in an
event handler responding to a mouse click, similar to
how we limit calls to element.focus().
this also narrows the restriction for focus() to just
click-like events, so you can't steal focus with, say,
a mousemove handler.
there's no automated test for this behavior yet, just a TODO,
because I tried several ways to write a purely html+js test
for it and failed miserably each time.
I did add a manual playground test, so it's not too hard to
check that it works as expected.
at some point, I'll write up some webdriver test logic for
this (and some other things that aren't being tested well).
[email protected]
http://code.google.com/p/google-caja/source/detail?r=5145
Added:
/trunk/src/com/google/caja/demos/playground/examples/submit.html
Modified:
/trunk/src/com/google/caja/demos/playground/client/ui/Example.java
/trunk/src/com/google/caja/plugin/domado.js
/trunk/tests/com/google/caja/plugin/es53-test-domado-dom-guest.html
=======================================
--- /dev/null
+++ /trunk/src/com/google/caja/demos/playground/examples/submit.html Thu
Nov 8 15:14:44 2012
@@ -0,0 +1,21 @@
+<p>Scripts can sometimes submit forms without user interaction.</p>
+
+<form id="f" action="http://www.google.com/"; target="_blank">
+ <input id="q" name="q" type="hidden" value="caja submit test">
+</form>
+
+<button id="b">submit</button>
+
+<script>
+ function sub(value) {
+ var f = document.getElementById('f');
+ var q = document.getElementById('q');
+ q.value = 'caja submit test ' + value;
+ f.submit();
+ }
+
+ var b = document.getElementById('b');
+ b.addEventListener('click', function() { sub('click'); }, false);
+ b.addEventListener('mouseover', function() { sub('mouseover'); }, false);
+ sub('auto');
+</script>
=======================================
--- /trunk/src/com/google/caja/demos/playground/client/ui/Example.java Tue
Jun 5 15:50:03 2012
+++ /trunk/src/com/google/caja/demos/playground/client/ui/Example.java Thu
Nov 8 15:14:44 2012
@@ -24,6 +24,9 @@
Type.ATTACK, "Redirecting the window"),
COOKIES("examples/cookies.html",
Type.ATTACK, "Stealing cookies"),
+ SUBMIT("examples/submit.html",
+ Type.ATTACK, "Automatic form submit"),
+
CLOCK("examples/clock.html",
Type.APPS, "Canvas Clock"),
UNBOXED("examples/unboxed/index.html",
=======================================
--- /trunk/src/com/google/caja/plugin/domado.js Thu Nov 8 14:46:38 2012
+++ /trunk/src/com/google/caja/plugin/domado.js Thu Nov 8 15:14:44 2012
@@ -1539,7 +1539,8 @@
}
var domicile = {
- isProcessingEvent: false
+ // True when we're executing a handler for events like click
+ handlingUserAction: false
};
var pluginId;
@@ -3481,17 +3482,13 @@
np(this).feral.blur();
});
TameElement.prototype.focus = nodeMethod(function () {
- if (domicile.isProcessingEvent) {
- np(this).feral.focus();
- }
+ return domicile.handlingUserAction && np(this).feral.focus();
});
// IE-specific method. Sets the element that will have focus when
the
// window has focus, without focusing the window.
if (docEl.setActive) {
TameElement.prototype.setActive = nodeMethod(function () {
- if (domicile.isProcessingEvent) {
- np(this).feral.setActive();
- }
+ return domicile.handlingUserAction && np(this).feral.setActive();
});
}
// IE-specific method.
@@ -4575,9 +4572,10 @@
});
}
});
+ // TODO(felix8a): need to test handlingUserAction.
TameFormElement.prototype.submit = nodeMethod(function () {
np(this).policy.requireEditable();
- return np(this).feral.submit();
+ return domicile.handlingUserAction && np(this).feral.submit();
});
TameFormElement.prototype.reset = nodeMethod(function () {
np(this).policy.requireEditable();
@@ -6159,8 +6157,8 @@
* Function called from rewritten event handlers to dispatch an event
safely.
*/
function plugin_dispatchEvent(thisNode, event, pluginId, handler) {
- event = makeDOMAccessible(
- event || bridalMaker.getWindow(thisNode,
makeDOMAccessible).event);
+ var window = bridalMaker.getWindow(thisNode, makeDOMAccessible);
+ event = makeDOMAccessible(event || window.event);
// support currentTarget on IE[678]
if (!event.currentTarget) {
event.currentTarget = thisNode;
@@ -6168,18 +6166,48 @@
var imports = rulebreaker.getImports(pluginId);
var domicile = windowToDomicile.get(imports);
var node = domicile.tameNode(thisNode);
+ var isUserAction = eventIsUserAction(event, window);
try {
- return plugin_dispatchToHandler(
- pluginId, handler, [ node, domicile.tameEvent(event), node ]);
+ return dispatch(
+ isUserAction, pluginId, handler,
+ [ node, domicile.tameEvent(event), node ]);
} catch (ex) {
imports.onerror(ex.message, 'unknown', 0);
}
}
+ /**
+ * Return true if event is a user action that can be expected to do
+ * click(), focus(), etc.
+ */
+ function eventIsUserAction(event, window) {
+ if (!(event instanceof window.UIEvent)) { return false; }
+ switch (event.type) {
+ case 'click':
+ case 'dblclick':
+ case 'keypress':
+ case 'keydown':
+ case 'keyup':
+ case 'mousedown':
+ case 'mouseup':
+ case 'touchstart':
+ case 'touchend':
+ return true;
+ }
+ return false;
+ }
+
+ /**
+ * Called when user clicks on a javascript: link.
+ */
function plugin_dispatchToHandler(pluginId, handler, args) {
- var sig = ('' + handler).match(/^function\b[^\)]*\)/);
+ return dispatch(true, pluginId, handler, args);
+ }
+
+ function dispatch(isUserAction, pluginId, handler, args) {
var domicile =
windowToDomicile.get(rulebreaker.getImports(pluginId));
if (domicile.domitaTrace & 0x1 && typeof console != 'undefined') {
+ var sig = ('' + handler).match(/^function\b[^\)]*\)/);
console.log(
'Dispatch pluginId=' + pluginId +
', handler=' + (sig ? sig[0] : handler) +
@@ -6199,14 +6227,15 @@
throw new Error(
'Expected function as event handler, not ' + typeof handler);
}
- domicile.isProcessingEvent = true;
+ domicile.handlingUserAction = isUserAction;
try {
return handler.call.apply(handler, args);
} catch (ex) {
// guard against IE discarding finally blocks
+ domicile.handlingUserAction = false;
throw ex;
} finally {
- domicile.isProcessingEvent = false;
+ domicile.handlingUserAction = false;
}
}
=======================================
--- /trunk/tests/com/google/caja/plugin/es53-test-domado-dom-guest.html Thu
Nov 8 14:46:38 2012
+++ /trunk/tests/com/google/caja/plugin/es53-test-domado-dom-guest.html Thu
Nov 8 15:14:44 2012
@@ -2392,8 +2392,9 @@
if (input.setActive) {
input.setActive();
}
- // TODO: can't verify focus changes in webdriver, because webdriver
runs
- // without focusing the browser window, so onfocus never gets fired.
+ // TODO(felix8a): can't verify focus changes in webdriver, because
+ // webdriver runs without focusing the browser window, so onfocus never
+ // gets fired.
pass('testFocusBlur');
});
</script>
