1690613 - Allow access to url/title/favIconUrl without "tabs" permission in session API

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, enhancement, P5)

Description

[Rob Wu [:robwu]]

Bug 1679688 changed some internals so that the tab's url, title and favIconUrl of various APIs are shown if the extension has host permissions, instead of only when the extension has the "tabs" permission (or activeTab).

The tab descriptors returned by the sessions API have url, title and favIconUrl properties too, but these still require the "tabs" permission. For consistency with the rest of the extension API, we can also consider exposing details of the tab when the extension has host permissions.

Fixing this bug is not difficult:

• Replace the check at https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/browser/components/extensions/parent/ext-browser.js#956 (see the linked bug for examples)
• Add a unit test that verifies that the properties are present without the "tabs" permissions, at https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js

Once this bug is fixed, we should update the documentation at:

• https://github.com/mdn/content/blob/c1a1777e56efd9f3bcec94befc2507506154408d/files/en-us/mozilla/add-ons/webextensions/api/windows/window/index.html#L46
• this page is rendered at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/sessions/Session#type

Comment 1

[james]

Hey,
I want to work on this bug but idk where to get started, in the description it is written that the check needs to be replaced, I don't really have an idea as to how, it would be helpful if you can provide me with some example or documentation as to how to approach this, also any documentation on how to write an unit test would be helpful.
Thank you.

Comment 2

[Rob Wu [:robwu]]

Hi James,

In the linked bug (bug 1679688), there is a reference to a Phabricator revision with the full context of the original code review. Somewhere at the end there is also an automated comment that links to the commit after the patch was merged. The commit is https://hg.mozilla.org/mozilla-central/rev/a5b3fa068d5e. You can look at this patch, and search (Ctrl-F) for .hasPermission("tabs") to find similar code and see try to understand what it did before, and what it replacement looks like.

Once you understand the issue, you can try to work on a patch. You can either try to write a unit test first, or fix the bug first (and then undo the bugfix to see if your new test is indeed confirming that the bugfix patch did indeed change something).

There is documentation available on getting started with setting up a development environment and running tests, see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Comment 3

[james]

Hi Rob,
Thanks for the help, I'll get started right away and try my best to see what I can do.

Comment 4

[james]

(In reply to Rob Wu [:robwu] from comment #2)

Hi James,

In the linked bug (bug 1679688), there is a reference to a Phabricator revision with the full context of the original code review. Somewhere at the end there is also an automated comment that links to the commit after the patch was merged. The commit is https://hg.mozilla.org/mozilla-central/rev/a5b3fa068d5e. You can look at this patch, and search (Ctrl-F) for .hasPermission("tabs") to find similar code and see try to understand what it did before, and what it replacement looks like.

Once you understand the issue, you can try to work on a patch. You can either try to write a unit test first, or fix the bug first (and then undo the bugfix to see if your new test is indeed confirming that the bugfix patch did indeed change something).

There is documentation available on getting started with setting up a development environment and running tests, see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Hey Rob,
I can't really understand as to how these functions work, specifically the .hasPermission and .hasTabPermission (I noticed that in the bug you mentioned as reference, the .hasPermission('Tabs') has been replaced by tab.hasTabPermission and a hasTabPermission() has been created which calls the matchesHostPermission() { return this.extension.allowedOrigins.matches(this._url);} ) I don't really understand as to what this.extension.allowedOrigins.matches(this._url);} does, it would be helpful if you could explain this to me or provide me with some documentation about this,
Thanks

Comment 5

[Rob Wu [:robwu]]

Apologies for the delay, I missed the last message.

(In reply to james from comment #4)

I can't really understand as to how these functions work, specifically the .hasPermission and .hasTabPermission (I noticed that in the bug you mentioned as reference, the .hasPermission('Tabs') has been replaced by tab.hasTabPermission

This is indeed how this bug can be resolved.

and a hasTabPermission() has been created which calls the matchesHostPermission() { return this.extension.allowedOrigins.matches(this._url);} ) I don't really understand as to what this.extension.allowedOrigins.matches(this._url);} does,

The details are in the referenced bug, bug 1679688. For the work here, it suffices to know that the hasTabPermission getter accounts for the "tabs" permission, the activeTab permission and host permissions. The last one is enforced via the this.extension.allowedOrigins.matches(this._url) call. If you need to know anything more specific, try to search for the implementation on Searchfox.org or post a specific question here.

Comment 6

[james]

(In reply to Rob Wu [:robwu] from comment #5)

Apologies for the delay, I missed the last message.

(In reply to james from comment #4)

I can't really understand as to how these functions work, specifically the .hasPermission and .hasTabPermission (I noticed that in the bug you mentioned as reference, the .hasPermission('Tabs') has been replaced by tab.hasTabPermission

This is indeed how this bug can be resolved.

and a hasTabPermission() has been created which calls the matchesHostPermission() { return this.extension.allowedOrigins.matches(this._url);} ) I don't really understand as to what this.extension.allowedOrigins.matches(this._url);} does,

The details are in the referenced bug, bug 1679688. For the work here, it suffices to know that the hasTabPermission getter accounts for the "tabs" permission, the activeTab permission and host permissions. The last one is enforced via the this.extension.allowedOrigins.matches(this._url) call. If you need to know anything more specific, try to search for the implementation on Searchfox.org or post a specific question here.

I made the changes, taking the referenced bug as a guide line.

I get the following error: 925:6 error Expected JSDoc for 'tab' but found 'extension'. valid-jsdoc (eslint)

What is this error?

Comment 7

[Rob Wu [:robwu]]

(In reply to james from comment #6)

I get the following error: 925:6 error Expected JSDoc for 'tab' but found 'extension'. valid-jsdoc (eslint)

What is this error?

The warning is self-explanatory, but if it is not clear, you can look up the more detailed version of the error message by searching for valid-jsdoc, and finding https://eslint.org/docs/rules/valid-jsdoc

Comment 8

[james]

(In reply to Rob Wu [:robwu] from comment #7)

(In reply to james from comment #6)

I get the following error: 925:6 error Expected JSDoc for 'tab' but found 'extension'. valid-jsdoc (eslint)

What is this error?

The warning is self-explanatory, but if it is not clear, you can look up the more detailed version of the error message by searching for valid-jsdoc, and finding https://eslint.org/docs/rules/valid-jsdoc

Hey, I have commited the changes but how do I push only a single commit as a phabricator patch?
When I run moz-phab it also starts submitting the previous bug which I worked on and it shows me a warning that it might reopen the bug

Comment 9

[james]

Attachment: Bug 1690613 - made the required changes at browser/components/extensions/parent/ext-browser.js#956 and added the test files. r=robwu

Comment 10

[james]

Hey I made the required changes and added the test file, Plz go through it and tell me where I went wrong and what I can do to fix it up

Comment 11

[james]

Attachment: Bug 1690613 - Only expose details when extention has host permission. r=robwu

Changed the if condtion check at browser/components/extentions/parent/ext-browser.js

Comment 12

[james]

Attachment: Bug 1690613 - Made the required changes to ext-browser.js and adeed a test. r=robwu

Comment 13

[james]

Apologies for so many patches, I stacked the commits on top of each other and didn't really know how to make it all in one

Comment 14

[Rob Wu [:robwu]]

(In reply to james from comment #13)

Apologies for so many patches, I stacked the commits on top of each other and didn't really know how to make it all in one

See the list at "Submitting a Patch" at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch, specifically the first item. It explains how the patch is created and how you can squash all changes into one commit. Please do so and mark the other revisions as "Abandoned" in Phabricator (via the dropdown menu above the comment box at the bottom of the page).

Comment 15

[james]

I squashed all the changes in one commit, amended that and pushed the phabricator patch. Also marked the other two revisions as abandoned.
Please let me know if I did something wrong

Comment 16

[james]

How do I get the properties of a tab and verify them?

Comment 17

[Rob Wu [:robwu]]

(In reply to james from comment #16)

How do I get the properties of a tab and verify them?

I assume that you're referring to the following part of comment #0 :

Add a unit test that verifies that the properties are present without the "tabs" permissions, at https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js

The test that I linked uses the browser.sessions API (specifically browser.sessions.getRecentlyClosed()) to retrieve a list of recently closed tabs.

This part of the test confirms that the expected properties are present with the right permissions ("sessions", "tabs"): https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js#17-25,46-51,108,113-117,126-136,138

This part of the test confirms that without the right permissions (only "sessions" but no "tabs"), the properties are not present: https://searchfox.org/mozilla-central/rev/927e525f481a93a8f63d27a78ae6201e42b1b1fb/browser/components/extensions/test/browser/browser_ext_sessions_getRecentlyClosed_tabs.js#140-164

To add a unit test for this bug, you have to:

1. Open a new window with a tab with a http:-URL (e.g. http://example.com/testpage) - note the unit tests do not actually load the real example.com page, but a dummy value from the local test-only server)
2. Close the window.
3. Load an extension with the "sessions", "http://example.com/*" permissions and use the browser.sessions.getRecentlyClosed() to query the set of closed tabs, and confirm that the expected properties (url / title) are present.

Comment 19

[Caitlin Neiman [:caitmuenster]]

Hey James! Are you still interested in working on this bug?

Comment 20

[Caitlin Neiman [:caitmuenster]]

Hi James, we're going to open this issue up to other contributors. :) If you'd like to come back to continue working on it, feel free to submit an updated PR!

Comment 21

[karim]

Hello,

I'm interested in working on this!

Comment 22

[karim]

Attachment: Bug 1690613 - Allow access to url/title/favIconUrl with extension host permission in Tab.convertFromSessionStoreClosedData; add unit test. r=robwu

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3bc13f11ddad
Allow access to url/title/favIconUrl with extension host permission in Tab.convertFromSessionStoreClosedData; add unit test. r=robwu

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3bc13f11ddad

Comment 25

[Caitlin Neiman [:caitmuenster]]

Woohoo, thanks so much for the patch, Karim! 🎉 Your contribution has been added to our recognition wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition.

Hope to see you around the project in the future!

Comment 26

[karim]

Many thanks! 🎉🎉