Add test to verify that the default context of the cookies API is incognito for scripts in incognito tabs

RESOLVED FIXED

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: robwu, Unassigned)

Tracking

(Blocks 1 bug)

52 Branch
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

ext-cookies.js uses the context.incognito property to get the default storeId if it is not specified. This feature has no test coverage and does not work (i.e. the context is always non-private, even if the caller is in incognito mode). In the past context.incognito *was* defined, but it was removed as a part of https://hg.mozilla.org/mozilla-central/rev/598895fae31d because the property was unused.

The only situation where this is relevant is when extension tabs are opened in a private window.
Specifically, for the cookies API, the following contexts are NOT relevant:
- content scripts (does not support chrome.cookies API)
- background pages ("split incognito mode" is not supported, so background pages are always non-private).
- extension subframes in non-extension tabs (same as content scripts - chrome.cookies not available).

Suggested resolution:
- Add an "incognito" property to BaseContext, initialized to null.
- Let BaseContext's setContentWindow set the incognito property if it was null, using PrivateBrowsingUtils.
- Update the implementation of inIncognitoContext in ext-c-context.js use context.incognito.
- Send the "incognito" property through ChildAPIManager to ParentAPIManager and assign it to ProxyContext (similar to how it was done before in the patch linked above).
- Add tests to verify that the default context is correctly inherit when called from an incognito extension context.


This bug is up for grabs, if you need help I'm happy to assist. Otherwise I may eventually implement the above myself after finishing bug 1287007.
This incognito property can also be useful for bug 1309620.
Depends on: 1254221
Assignee: nobody → kmaglione+bmo
Priority: -- → P1
Whiteboard: triaged
Note: Matt is implementing context.incognito in bug 1247435.
The current patch does not contain regression tests for this bug, but I can pick that up if needed.
Depends on: 1247435
(In reply to Rob Wu [:robwu] from comment #2)
> Note: Matt is implementing context.incognito in bug 1247435.

That bug is now fixed. Does this bug just cover writing tests at this point? (Maybe fix the bug Summary if so?)
Flags: needinfo?(rob)
Also https://developer.mozilla.org/en-US/Add-ons/WebExtensions does not mention 'incognito' under "Manifest keys". Can someone who knows what they're doing add that, even if it just says "only incognito: spanning" is accepted". (And maybe add other missing keys?)
Bug 1309637(In reply to Jonathan Watt [:jwatt] from comment #3)
> (In reply to Rob Wu [:robwu] from comment #2)
> > Note: Matt is implementing context.incognito in bug 1247435.
> 
> That bug is now fixed. Does this bug just cover writing tests at this point?
> (Maybe fix the bug Summary if so?)

Done.

The tests cannot be written yet because incognito cookies in tests disappear due to bug 1309637.

(In reply to Jonathan Watt [:jwatt] from comment #4)
> Also https://developer.mozilla.org/en-US/Add-ons/WebExtensions does not
> mention 'incognito' under "Manifest keys". Can someone who knows what
> they're doing add that, even if it just says "only incognito: spanning" is
> accepted". (And maybe add other missing keys?)

I think that it would be good to make the documentation update part of bug 1280027.
Depends on: 1309637
Flags: needinfo?(rob)
Priority: P1 → P2
Summary: context.incognito is undefined - default storeId in cookies is always non-private → Add test to verify that the default context of the cookies API is incognito for scripts in incognito tabs
Mass wontfix for bugs affecting firefox 52.
Assignee: kmaglione+bmo → nobody
Priority: P2 → P3
Blocks: 1460738
Product: Toolkit → WebExtensions
Blocks: webext-incognito
No longer blocks: 1460738

I suspect we have what we need for this in D11699 from bug 1345474.

Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.