1653408 - Trouble with activeTab and allFrame permissions

Status: NEW

(WebExtensions :: Untriaged, defect, P2)

Description

[photodow]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/83.0.4103.116 Safari/537.36

Steps to reproduce:

1. Have a page with multiple iframes.
2. Set activeTab under permissions in manifest.yml
3. Insert a script into all frames for a given tab using the following code.

browser.tabs.executeScript({
code: console.log('location:', window.location.href);,
allFrames: true
});

I did manage to get it to work, but only after giving explicit permission to all URLs within the manifest. I don't have to ask for such broad permissions in Chrome because the extension works properly over there, and inserts into all iframes within a tab.

Actual results:

It would only inject the code/file into the parent 0 frame.

Expected results:

It should have been injected into the parent frame, but also into the other iframes on the page with the same origin.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Alexandra Martin]

Hi,

Thank you for taking the time to submit this bug report. Unfortunately I couldn't reproduce the issue, as I don't have that much knowledge in the matter. Is it possible for you to give a test extension and maybe a screencast, in order to understand the issue better?

Thank you and take care!

Comment 3

[photodow]

Attachment: activetab-reduced-test-case.zip

Attaching a quick reduced test case in hopes to help you reproduce.

Comment 4

[photodow]

Attachment: chrome-screen-recording.mov

Here is a quick screen capture of the issue. Notice how in Chrome the script injects twice. This additional injection is a same-origin iframe located within the storybook tab.

Comment 5

[photodow]

Attachment: firefox-screen-recording.mov

Attaching a quick screen capture of the problem on Firefox. Notice how the same page tested on Chrome only injects 1 time in Firefox. Same code, same permissions, same origin, same iframe, but just different results.

Comment 6

[Alexandra Martin]

Hi again,

Thank you for the extension and screencasts, it was very helpful. After testing on Windows 10 x64-bit and MacOS 10.14 with Nightly 80.0a1 (20200724093206), Beta 79 (20200720193547) and Release 78.0.2 (20200708170202), I managed to reproduce the issue you were describing.

I tried to also find a regression range, but the bisection was skipping builds, so I could only get a last good build from 2018-01-10 running with Nightly 59.0a1 (20180110221942) and a first bad build from 2018-01-11 running with Nightly 59.0a1 (20180111220102).

Pushlog first bad build: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e4de69553e3faf8136eb9bb7f2f741e1b7e6f866&tochange=8142a68bf0a7b44c2502888ba6b2a930edf428fd

Comment 7

[Tomislav Jovanovic :zombie]

Good news/bad news: our handling of activeTab + allFrames has been broken since October 23 2019.

Unfortunately (1) we don't keep artifacts from that far back so the pushlog between two Nightly builds has over 300 bugs :(
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=563f437f24&tochange=82274c2025

In theory, tabs.executeScript (with activeTab permission) is supposed to inject into all iframes that were part of the original page, but not script-injected iframes, as that would allow escalation of privileges, gory details in bug 1396399. Unfortunately (2), that was a security bug, so the test for the designed behavior wasn't landed with the fix. Unfortunately (3) again, it probably slipped through the cracks and never landed.

From Andrew's comments, our behavior was based on Chrome's, but it seems they have since changed their implementation to only inject into same-origin iframes. regardless if they are static or added by scripts.

Good news is, since this has been broken for almost a year without reports, we can probably safely change the original design to match current Chrome's behavior, which I would expect as an addon developer, and prefer as a web extensions engineer.

Bonus points: the patch that implemented our previous design was (necessarily) messy and all over the place, so we get some code cleanup to go along a simpler design for free.

Finally, we should also verify the behavior of Safari Preview, and try to get them to align if possible.

Comment 8

[Tomislav Jovanovic :zombie]

Attachment: activeTab-allFrames.zip

Simplified extension to test across browsers (Chrome, Firefox, Safari Preview) on:
https://dull-skinny-achillobator.glitch.me/

Comment 9

[Tomislav Jovanovic :zombie]

Attachment: test-extension-on-safari14-preview.png

It seems we have a consensus between Chrome and Safari that activeTab+allFrames should only apply to same origin frames.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 11

[Marco Castelluccio [:marco]]

Are there any updates on this? Is this actually S2?

Comment 12

[Shane Caraveo (:mixedpuppy)]

Given a lack of reports on this issue, reducing P/S. Will leave ni? for Tom in case he has a different opinion.