Can you provide a pointer to your workaround? This code was written with the assumption that it is being called from script, namely script adding a 'deviceorientation' event listener. We were unaware of the dependency from media controls.

mlamouri@'s patch (currently WIP): https://chromium-review.googlesource.com/c/chromium/src/+/829733

As mentioned in another bug I CC'd you on, it happened that the integration test for the feature was disabled and assigned to a team member who left so was never fixed so I understand that you were unaware of this dependency.

However, it doesn't seem that this assumption is fair. The code could be run from various contexts and being called from a script should only happen at the API level IMO. I'm very surprised we even allow content/renderer/ to look for the RenderFrame based on the v8::Context, it seems like a footgun to me.

FWIW, the code lives in: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp?q=f:rotatetofullscreen&sq=package:chromium&dr

The media controls code is basically C++ behaving like JS but doesn't have a v8::context most of the time.

Not to trade accusations of layering violations but it's certainly surprising that we have C++ adding event listeners to DOM elements. This pattern seems isolated to the media code.

The need to find a LocalFrame comes from the requirement that requests for sensors from web content check whether that content has permission to access them.

The media controls behaviour is the result of long discussions regarding whether we should implement these in JS and the result with the Blink leads at the time was to use our own APIs but in C++. It's been usually a very good test case for our own work.

Regarding requiring a LocalFrame, couldn't you have the object as part of RenderFrame and have the interface as part of WebFrameClient instead of using the non-frame solution? Could you use the permission service from Blink otherwise? The permission service can be called from a frame/worker without distinction.

That assumption is being made at the API level, EventTarget::addEventListener is the web-exposed API for adding an event listener and is the method MediaControlsRotateToFullscreenDelegate is calling.

I'm trying to think through a workaround to satisfy both our constraints. Your patch seems like it will work fine for the time being but if we want to start actually restricting sensor permissions to web content (it is currently allowed by default) then we will need to consider how this will affect rotate-to-fullscreen.

The current code is complicated by the PlatformEventDispatcher layer which interposes between the frame adding the event listener and the source of those events, leading us down this path towards trying to guess the frame from the current context. I'd like to eventually onion-soup this and remove a number of these layers but we will still have the requirement of making sensor requests in a frame context.

I starting to feel like your workaround is the right solution long-term because even if we use the permission service these calls aren't coming from either a frame or worker scope, they're coming from Blink itself. Your workaround properly attributes them to the frame containing the video as if this were script in the page instead of native code.

reillyg@, if the only reason why you need to have access to the frame is to check if the website has permissions to use the API, why not do this from Blink using the permissions service?

Getting a connection to the permission service also requires a frame context so if the event pump can't find one on the stack then it will have to fail open. This seems like a source of security bugs.

Also, I prefer being able to make security decisions in the browser process. Even if Blink checks for permissions the browser should still be able to double-check that the origin the render process is navigated to has that permission.

You don't need a frame context to connect to the permission service. The Permissions API connect from the service from a frame or a worker.

The browser process should ultimately make the decision and kill a renderer that tries to use the feature when not allowed. Though, regardless, isn't the current call in the renderer process?

I had another look and I do not think the permission check in the browser process adds any security given that the render frame (if corrupted) can pretend to be any origin. Using the running v8 context in order to get a frame for this seems a very high price to pay for a false impression of security.

I would suggest that the architecture is changed to:
- move the permission check to Blink
- when DeviceOrientation is accessed, ignore the call if the permission is rejected
- move the SensorProviderProxyImpl to the RenderProcessHost and drop the v8 context dependency

This would be a first step into onion souping this code. Ideally, the SensorProviderProxyImpl should be accessed from Blink in a way that offers more information about the current context. If the API is meant to never work outside of the frame context, it might be fine to move it back to RenderFrameHost. Though, it would be a shame to prevent workers to access this information by design.

In general, it's wrong to use a current context in the code base (except a couple of cases where you're pretty sure that you're already in a right context). In this case I don't think it's guarantee that you're in a right context when GetRenderFrame() gets called.

Why is it hard to pass in RenderFrame explicitly to device_motion_event_pump (or explicitly associate device_motion_event_pump with RenderFrame)?

DeviceMotionEventPump is a PlatformEventObserver and the "platform event" infrastructure is all built to only create one instance of each event source per process and share it between frames. This makes it difficult (and questionably correct) to associate one of these event sources with a particular frame. Passing the RenderFrame explicitly will require a lot of plumbing.

There's a discussion I've had with dcheng@ a couple of times about how much we care about not trusting the renderer about its origin given that PlzNavigate and Site Isolation make this more of a possibility. I think the feeling is that we still don't care too much and so I'm not opposed to moving the permission check into the renderer as long as security folks sign off.

What's the status of this bug?

Using a wrong context has a risk of security exploit, so this must be fixed asap.

Even with site isolation, we'd strongly prefer not to plumb origin over IPC. Doing this means we need to continually audit callsites that do this and ensure they do a manual sanity check on the receiver side. This is not a pattern we'd like to encourage.

I wrote a CL during the holidays that getting ride of the service used for permissions. In other words, content/renderer/ would directly use the device services. I ended up working on usual stuff afterwards. reillyg@ and juncai@, do you have any plans on this?

I think I need to have a face-to-face with dcheng@ to completely go over the security model here and decide the best option given the constraints. If you would like I can include you mlamouri@ in that meeting.

I would be happy to join. Let's schedule this over email.

An update based on my meeting with mlamouri@ which dcheng@ was unfortunately unable to attend:

The rotate to fullscreen feature requires device orientation events because screen orientation events are only generated when the orientation in which the screen is rendered changes and so won't be fired after full screen landscape video playback mode is entered.

The possibility of using the Generic Sensor API's AbsoluteOrientationSensor or RelativeOrientationSensor objects was raised because they take an ExecutionContext explicitly and so don't require a v8::Context on the stack. They also allow callers to specify a sample rate of less than the 60Hz default which is much more than this feature requires. mlamouri@ provided some feedback on the API which I will translate into spec issues. He will investigate this possible workaround. A constructor that bypasses the secure origin check will be required.

My thought after this meeting is that we are particularly considered about sensor permissions at the moment because of the privacy impact. If this feature only requires a low-frequency and rough description of the current screen orientation then this could be provided as a separate sensor type with looser controls. The proposed sensor would be similar to the Windows "SimpleOrientation" sensor which only returns the values "Facedown", "Faceup", "NotRotated", "Rotated180DegreesCounterclockwise", "Rotated270DegreesCounterclockwise" and "Rotated90DegreesCounterclockwise". It's maximum frequency could be as low as 1Hz.

Any update on this bug?

I noticed that there are multiple call sites using WebLocalFrame::FrameForCurrentContext in a wrong way (which has a security risk). We should really deprecate WebLocalFrame::FrameForCurrentContext since people are likely to misuse it...

Ping on this.

This may cause a security bug.

Currently, DeviceOrientationEventPump uses a LocalFrame passed down from a DeviceOrientationController:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/device_orientation/device_orientation_event_pump.cc?l=78

Can this ticket be resolved or are there still dependencies we need to eliminate?

We should now be able to remove lines 58-65 where we set up a ScriptState::Scope before calling addEventListener() in third_party/blink/renderer/modules/media_controls/media_controls_rotate_to_fullscreen_delegate.cc.