Vincent Scheib (scheib@chromium.org) Reporter
	Description • 12 years ago

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

	Ian Melven :imelven
	Comment 1 • 12 years ago

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 ?

	Vincent Scheib (scheib@chromium.org) Reporter
	Comment 2 • 12 years ago

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.

	Ian Melven :imelven
	Comment 3 • 12 years ago

(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.. :)

	Vincent Scheib (scheib@chromium.org) Reporter
	Comment 4 • 12 years ago

(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.

	Ian Melven :imelven
	Comment 5 • 12 years ago

(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.

	Ian Melven :imelven
	Comment 6 • 12 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 7 • 12 years ago

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.

	Diogo Golovanevsky Monteiro [:diogogmt] Assignee
	Comment 8 • 12 years ago

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.

	Ian Melven :imelven
	Comment 9 • 12 years ago

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" ?

	Diogo Golovanevsky Monteiro [:diogogmt] Assignee
	Comment 10 • 12 years ago

Added mochitest.

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

	Ian Melven :imelven
	Comment 11 • 12 years ago

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

	Diogo Golovanevsky Monteiro [:diogogmt] Assignee
	Comment 12 • 12 years ago

Fixed review comments.

	Ian Melven :imelven
	Comment 13 • 12 years ago

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+.

	Ian Melven :imelven
	Comment 14 • 12 years ago

Comment on attachment 663493 [details] [diff] [review]
p3

Asking smaug to review.

	Ian Melven :imelven
	Comment 15 • 12 years ago

(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.. :)

	Diogo Golovanevsky Monteiro [:diogogmt] Assignee
	Comment 16 • 12 years ago

(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.

	Ian Melven :imelven
	Comment 17 • 12 years ago

(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.

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/57fe0a88997c

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/57fe0a88997c

	Ian Melven :imelven
	Comment 20 • 12 years ago

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

	Diogo Golovanevsky Monteiro [:diogogmt] Assignee
	Comment 21 • 12 years ago

(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.

	Ed Morley [:emorley]
	Comment 22 • 12 years ago

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

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 23 • 12 years ago

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