Pointer Lock Specification updated to restrict the API's availability in sandboxed iframes: http://dvcs.w3.org/hg/pointerlock/raw-file/default/index.html http://dvcs.w3.org/hg/pointerlock/log/default/index.html http://dvcs.w3.org/hg/pointerlock/diff/65dc7b7994e7/index.html

from the spec in comment 0 : "Sandboxed pointer lock flag is IN PROGRESS OF BEING DEFINED in HTML Sandboxing. In summary, pointer lock is disabled in sandboxed iframe documents unless sandbox="allow-pointer-lock" is added." does this really need its own 'allow', it can't be bundled with 'allow-scripts' for example, like 'automatic features' are ?

I'm happy to hear thoughts. I believe using only allow-scripts is poor as I can imagine developers wanting to run scripts in iframe content but block the nuisance of unwanted pointer lock. E.g. advertisement networks may well require scripts be enabled in iframed ads, but a hosting page may want to limit them.

(In reply to Vincent Scheib from comment #2) > I'm happy to hear thoughts. I believe using only allow-scripts is poor as I > can imagine developers wanting to run scripts in iframe content but block > the nuisance of unwanted pointer lock. E.g. advertisement networks may well > require scripts be enabled in iframed ads, but a hosting page may want to > limit them. yeah, i see your point about 'allow-scripts' for sure - my concern is that we don't end up with hundreds of 'allow-*' directives for iframe sandbox, I'm not yet convinced this is inevitable. I think blocking pointer lock for a sandboxed iframe makes sense, I need to think some more about the use case for 'allow-pointer-lock'. I think one would usually want 'allow-scripts' for things that use pointer lock, so the use case ends up being mostly about the unique origin, it seems. This seems like it should be a mailing list thread somewhere.. :)

(In reply to Ian Melven :imelven from comment #3) > This seems like it should be a mailing list thread somewhere.. :) A previous conversation is here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17838#c1 Use case <iframe sandbox="">: just show simple content, maximum limitations. Use case <iframe sandbox="allow-scripts">: The content requires scripts, but you still don't trust it at all and want it locked up. E.g. a non trivial advertisement. Use case <iframe sandbox="allow-scripts allow-pointer-lock">: You are a game portal and want to host a third party game in an iframe that uses pointer lock, e.g. newgrounds.com.

(In reply to Vincent Scheib (scheib@chromium.org) from comment #4) > (In reply to Ian Melven :imelven from comment #3) > > This seems like it should be a mailing list thread somewhere.. :) > > A previous conversation is here: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=17838#c1 Thanks for the link. Making a note here that https://www.w3.org/Bugs/Public/show_bug.cgi?id=18647 got spun off as "Add sandboxed pointer lock flag to HTML Sandboxing" from that discussion, in case others are following along.

I probably won't get to this any time but would be happy to mentor someone who was interested in working on it. Off the top of my head, fixing this requires : - adding a new sandbox flag for pointer lock in nsSandboxFlags.h (https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsSandboxFlags.h) - adding 'allow-pointer-lock' to nsContentUtils::ParseSandboxAttributeToFlags (https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp) - checking in the pointer lock code if the sandbox pointer lock flag is set, and not locking the pointer if it is

I'm not seeing this as a potential release blocker for 18, if there's some major user-facing fallout from this please illuminate me otherwise nominate patches for uplift consideration when they are ready.

From the testing that I've done the "allow-point-lock" sandbox flag seems to be working. When the flag is set the document can request PointerLock, if not it can't. I'm going to write some mochitests, but I just wanted to make sure I'm on the right track.

Comment on attachment 662196 [details] [diff] [review] p1 Review of attachment 662196 [details] [diff] [review]: ----------------------------------------------------------------- Looks great to me ! Thanks for working on this, looking forward to the tests :) ::: content/base/src/nsContentUtils.cpp @@ +919,5 @@ > SANDBOXED_FORMS | > SANDBOXED_SCRIPTS | > + SANDBOXED_AUTOMATIC_FEATURES | > + SANDBOXED_POINTER_LOCK; > + nit: extra whitespace on the blank line ::: content/base/src/nsDocument.cpp @@ +9641,5 @@ > return false; > } > > + if (mSandboxFlags & SANDBOXED_POINTER_LOCK) { > + NS_WARNING("ShouldLockPointer(): Sandbox allow-pointer-lock flag not enable"); maybe "document is sandboxed and doesn't allow pointer-lock" ?

Added mochitest. Try server run: https://tbpl.mozilla.org/?tree=Try&rev=231f677160ef

Comment on attachment 663368 [details] [diff] [review] p2 Review of attachment 663368 [details] [diff] [review]: ----------------------------------------------------------------- These tests look good to me with a couple of nits. There were a few suspicious oranges, reds, and purples on the try run so I reran a few things to see if they're intermittant. ::: dom/tests/mochitest/pointerlock/file_allowPointerLockSandboxFlag.html @@ +39,5 @@ > + , numberOfRuns = 0; > + > + function runTests () { > + is(pointerLocked, 1, "Pointer should only have been locked once. " + > + "Withouth allow-pointer-lock sanbox flag iframe should not be " + some nits: typo: Withouth -> Without typo: sanbox -> sandbox maybe "Without allow-pointer lock, a sandboxed document should not be" ? @@ +84,5 @@ > + }); > + > + iframeDiv.mozRequestFullScreen(); > + } > + nit: extra whitespace on this blank line

Fixed review comments.

Comment on attachment 663493 [details] [diff] [review] p3 Review of attachment 663493 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good to me. There's still some weirdness on try, but we can try to get the patch reviewed and then push to try again after the eventual r+.

Comment on attachment 663493 [details] [diff] [review] p3 Asking smaug to review.

(In reply to Diogo Golovanevsky Monteiro [:diogogmt] from comment #12) > Created attachment 663493 [details] [diff] [review] > p3 > > Fixed review comments. one thing : why do you need full screen in the tests ? is that required for pointer lock ? just curious.. :)

(In reply to Ian Melven :imelven from comment #15) > (In reply to Diogo Golovanevsky Monteiro [:diogogmt] from comment #12) > > Created attachment 663493 [details] [diff] [review] > > p3 > > > > Fixed review comments. > > one thing : why do you need full screen in the tests ? is that required for > pointer lock ? just curious.. :) Correct me if I'm wrong but right now pointerlock only works if the element is in fullscreen mode, there is a bug tracking the implementation of pointerlock for non fullscreen elements: https://bugzilla.mozilla.org/show_bug.cgi?id=737100 I think due to some security issues at the beginning it was decided to require fullscreen mode before requesting pointerlock, this way pointerlock ended up inheriting a lot of security checks done in fullscreen.

(In reply to Diogo Golovanevsky Monteiro [:diogogmt] from comment #16) > (In reply to Ian Melven :imelven from comment #15) > > (In reply to Diogo Golovanevsky Monteiro [:diogogmt] from comment #12) > > > Created attachment 663493 [details] [diff] [review] > > > p3 > > > > > > Fixed review comments. > > > > one thing : why do you need full screen in the tests ? is that required for > > pointer lock ? just curious.. :) > > Correct me if I'm wrong but right now pointerlock only works if the element > is in fullscreen mode, there is a bug tracking the implementation of > pointerlock for non fullscreen elements: > https://bugzilla.mozilla.org/show_bug.cgi?id=737100 > I think due to some security issues at the beginning it was decided to > require fullscreen mode before requesting pointerlock, this way pointerlock > ended up inheriting a lot of security checks done in fullscreen. Thanks for clarifying, I wasn't aware of that restriction on pointer lock but it makes a lot of sense to me.

Diogo, thanks again for working on this and getting the patch all the way to landing ! If you're interested in more bugs, please feel free to ping me on irc or drop me an email :D

(In reply to Ian Melven :imelven from comment #20) > Diogo, thanks again for working on this and getting the patch all the way to > landing ! > > If you're interested in more bugs, please feel free to ping me on irc or > drop me an email :D Thank you for helping me out with this bug. I sure will ask you for more bugs :) Cheers.

This caused bug 795914, please may someone take a look at it?

I landed a fix over in bug 795914. Thanks for tracking this Ed.