Closed Bug 1543219 Opened 5 years ago Closed 5 years ago

Make TestMailCookie work

Categories

(MailNews Core :: Testing Infrastructure, defect)

Product:
MailNews Core
Component:
Testing Infrastructure
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(1 file, 16 obsolete files)

1543219-M-C-part.patch (v3c)
3.06 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review

Now that M-C is top source directory, we can run gTests. I noticed that TestMailCookie doesn't work when executing mach gtest TestMailCookie*.

I have a patch that will make it pass.

Attached patch 1543219-TestMailCookie.patch (obsolete) — — Splinter Review

With this patch, mach gtest TestMailCookie* passes. Note that this test was converted to a gTest in bug 1322072, and in an mid-air collision something similar was done in bug 1320861. But no one was ever able to run the tests, until today :-)

Turns out that it doesn't pass. The idea was to set a cookie for a mailbox: URL and then retrieve it and that should fail. But it doesn't. Equally, the same cookie can be retrieved via for the same host bug with a http: scheme.

I don't know what's right. Do you have a better idea, Magnus?

Assignee: nobody → jorgk
Attachment #9057070 - Flags: review?(mkmelin+mozilla)

Note that this appears to be an adaption of https://searchfox.org/mozilla-central/source/netwerk/test/TestCookie.cpp.

Removed unneeded include.

Attachment #9057070 - Attachment is obsolete: true
Attachment #9057070 - Flags: review?(mkmelin+mozilla)
Attachment #9057089 - Flags: review?(mkmelin+mozilla)

Rebased for bug 1466748.

Attachment #9057089 - Attachment is obsolete: true
Attachment #9057089 - Flags: review?(mkmelin+mozilla)
Attachment #9057099 - Flags: review?(mkmelin+mozilla)
Attachment #9057099 - Flags: feedback?(acelists)
Comment on attachment 9057099 [details] [diff] [review]
1543219-TestMailCookie.patch (v1c)

Review of attachment 9057099 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this also builds and passes for me, on top of the other patch.
But apparently this needs a change or decision on the test logic.
Attachment #9057099 - Flags: feedback?(acelists) → feedback+
Comment on attachment 9057099 [details] [diff] [review]
1543219-TestMailCookie.patch (v1c)

Review of attachment 9057099 [details] [diff] [review]:
-----------------------------------------------------------------

Technically perhaps there's nothing wrong with the cookie getting passed across the different protocols. 

But, can't see wanted use case for where setting cookies on mailbox: and others would be ok. I think we should make that fail. Perhaps it can be used for some tracking purposes.
Attachment #9057099 - Flags: review?(mkmelin+mozilla)

Yes, the test in its original form made sure cookies could be set for mailbox: URLs:
https://searchfox.org/comm-central/rev/29215b2012a35d80ea879bb5403290caa3f41242/mailnews/base/test/TestMailCookie.cpp#180

I don't know when the test last ran and passed in its current from, certainly it wasn't executed since January 2017 when it was ported to a gTest: https://hg.mozilla.org/comm-central/log/tip/mailnews/base/test/TestMailCookie.cpp.

Looks like M-C's TestCookie.cpp has been worked on mostly by Andrea, Ehsan, Olli and Valentin. So let's try Valentin.

Valentin, can you please take a look at TestMailCookie.cpp as referenced above. It doesn't pass in it's current form, the patch in attachment 9057099 [details] [diff] [review] makes it pass, but according to Magnus, storing cookies for mailbox: URLs is undesired. Can you give me a few pointers how to fix the code so the test passes again.

Flags: needinfo?(valentin.gosu)

Maybe Ehsan has a suggestion here. Not sure how disallowing cookies for mailbox URLs would work.

Flags: needinfo?(valentin.gosu) → needinfo?(ehsan)

For mailbox urls I doubt it's important to change this either way, since there would never be any corresponding http domain.

If imap there could be a domain. But there too unclear if there would ever be any usage of that cookie.

For the record: mailbox: URLs can have to forms. The first one is basically a file URL, so something like mailbox:///path/to/folder/Inbox?number=1, the second one is mailbox://user@domain@server/Folder?number=1, see for example:
https://searchfox.org/comm-central/rev/2513b89e1fb1f1bd1d75da6c8dc330ca2055d387/mailnews/local/test/unit/test_mailboxURL.js#14.

I don't know whether cookies would haver be used for mailbox: or imap: URLs, it's therefore unclear whether this test is useful at all, so we could either remove it or just make it pass as I suggested. That said, if the test ever ran and passed, it shows that the behaviour has changed at some stage and we may want to go back to the original behaviour.

Be ready for some code inspection (none of this is backed by any testing whatsoever).

(In reply to Valentin Gosu [:valentin] from comment #8)

Maybe Ehsan has a suggestion here. Not sure how disallowing cookies for mailbox URLs would work.

The cookie service doesn't get in the business of allowing or disallowing specific URL schemes at all. Any such thing that may happen for certain URLs is accidental. Based on some code inspection there is probably a serious bug in Thunderbird that would allow server in comment 10 to plant HTTP cookies if we create an HTTP connection over to a mailbox URL or use the cookie manager API to set a cookie on a mailbox URL.

Here is how this would work. From nsCookieService::SetCookieStringCommon() we try to compute the base domain of the host URI https://searchfox.org/comm-central/rev/7c6567ce75e77c61bab4faebf67404872af3e994/mozilla/netwerk/cookie/nsCookieService.cpp#2148 (let's imagine it's mailbox://user@domain@server/Folder?number=1). That brings us to here: https://searchfox.org/comm-central/rev/7c6567ce75e77c61bab4faebf67404872af3e994/mozilla/netwerk/dns/nsEffectiveTLDService.cpp#138. Then we call GetAsciiHost() on the URI: https://searchfox.org/comm-central/rev/7c6567ce75e77c61bab4faebf67404872af3e994/mozilla/netwerk/base/nsNetUtil.h#767 to get the host name. This class implements mailbox URLs: https://searchfox.org/comm-central/rev/7c6567ce75e77c61bab4faebf67404872af3e994/mailnews/local/src/nsMailboxUrl.h#18 so this is the relevant GetAsciiHost() function: https://searchfox.org/comm-central/rev/7c6567ce75e77c61bab4faebf67404872af3e994/mailnews/base/util/nsMsgMailNewsUrl.cpp#610. Now the question is, what is m_baseURL here? If you read https://searchfox.org/comm-central/rev/7c6567ce75e77c61bab4faebf67404872af3e994/mailnews/base/util/nsMsgMailNewsUrl.cpp#463 you'll see that m_baseURL is basically created through throwing our url spec at the standard URL parser. So... GetAsciiHost() called on our URI will happily return "server". Oops. :-( From that point on the effective TLD service will conclude the base domain is also "server" and the cookie service will go ahead and store a cookie for that host. If that ends up being a valid Internet domain, you now have a cookie which will be reused when making HTTP connections.

This is a bug in nsMailboxUrl. It needs to override GetAsciiHost() and return an error code and stop pretending that this URL format has a valid host name (similar for other functions that return the host name). I didn't audit the rest of this code but my guess is that nsMailboxUrl may have more places where it is not abiding by the nsIURI. Until such time that it abides by the nsIURI API it is not reasonable to expect any part of Gecko to be able to safely handle mailbox:// URIs.

Hope this helps!

Flags: needinfo?(ehsan)

Thanks Ehsan. Sadly the https://searchfox.org/comm-central/ ... /mozilla/ ... URLs don't work.

So we have
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/netwerk/cookie/nsCookieService.cpp#2148
calling GetBaseDomain() and then from
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/netwerk/dns/nsEffectiveTLDService.cpp#138
calling NS_GetInnermostURIHost() onto here:
https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/netwerk/base/nsNetUtil.h#767

I don't think we can have GetAsciiHost() return an error since we use it to get parts of our URLs:
https://searchfox.org/comm-central/search?q=GetAsciiHost&case=false&regexp=false&path=mailnews

The mailbox: URL as well as imap: and news: URLs do have hosts. After midnight on a Friday I won't take the issue much further, but as I said in comment #10, if the test ever passed, which I don't know, I'd be interested to understand what changed in the last years in that area. Maybe we should start looking at an older version, like here:
https://dxr.mozilla.org/mozilla-esr17/source/netwerk/cookie/nsCookieService.cpp#1493
which is when the test appeared here:
https://dxr.mozilla.org/comm-esr17/source/mailnews/base/test/TestMailCookie.cpp
Maybe it passed back then.

Note that I think the bigger concern here is how the cookie manager would deal with mailbox:// URLs in practice. Making TestMailCookie.cpp pass is a secondary concern (e.g. you could make it pass by dropping the checks for mailbox:// URLs of course! but that would only wallpaper the very real problem I alluded to in comment 11.)

Feel free to do whatever you will with this info! I can assure you that the cookie manager has never had any URI-scheme specific checks to outlaw mailbox:// URIs specifically (note that before Firefox 57 add-ons could also provide a protocol handler implementation so such checks would not work with URI schemes provided by add-ons anyway.)

I'm thoroughly confused now. I've run this in TB's console:

var cookieService = Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService);
var httpurl=Services.io.newURI("http://www.jorgk3.com");
cookieService.setCookieStringFromHttp(httpurl, null, null, "http=55", null, null);
cookieService.getCookieStringFromHttp(httpurl, null, null);
returns http=55.

var mailboxurl=Services.io.newURI("mailbox://www.jorgk4.com");
cookieService.setCookieStringFromHttp(mailboxurl, null, null, "mail=66", null, null);
cookieService.getCookieStringFromHttp(mailboxurl, null, null);
That returns null.

However, the gTest reduced to:

    SetACookieMail(cookieService, "mailbox://mail.co.uk/", nullptr, "test=mailnews", nullptr);
    GetACookieMail(cookieService, "mailbox://mail.co.uk/", nullptr, getter_Copies(cookie));
    EXPECT_TRUE(CheckResult(cookie.get(), MUST_BE_NULL));

fails.

But the gTest does exactly that:
https://searchfox.org/comm-central/source/mailnews/base/test/TestMailCookie.cpp

What am I missing?

Flags: needinfo?(ehsan)

Oh, I forgot to say, I ran the commands from comment #14 in TB 60 ESR, my production version. Running it on TB 67 beta, I get "mail=66" returned. And surely the gTest also runs on my local version of 68.

Alice, can you do me a favour. Please paste:

var cookieService = Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService);
var mailboxurl=Services.io.newURI("mailbox://www.jorgk4.com");
cookieService.setCookieStringFromHttp(mailboxurl, null, null, "mail=66", null, null);
cookieService.getCookieStringFromHttp(mailboxurl, null, null);

into the debug console in TB 60 and TB 67 and compare what it returns. Then find where the behaviour changes.

Flags: needinfo?(alice0775)

What is the debug console?

Flags: needinfo?(alice0775) → needinfo?(jorgk)

Tools > Developer Tools > Error Console (sorry, not Debug Console). There might be a pref you need to set so you can run JS commands in there.

Flags: needinfo?(jorgk)

Thanks, Alice, this is very helpful, now we understand what happened:
Bug 1517360 - Port bug 1517057, Part 1: Remove use of URI_FORBIDS_COOKIE_ACCESS.

M-C removed URI_FORBIDS_COOKIE_ACCESS, so we removed it from out code base. The comment read:
https://hg.mozilla.org/mozilla-central/rev/23a0332b18a1#l2.11

     /**
-     * This protocol handler forbids accessing cookies e.g. for mail related
-     * protocols.
-     */
-    const unsigned long URI_FORBIDS_COOKIE_ACCESS = (1<<15);
-

So maybe we should restore that code, even with an #ifdef MOZ_THUNDERBIRD. Ehsan?

That is a rather invasive change in m-c, I don't think that's something we're in a rush to support in the long term. Is it possible for Thunderbird to override the nsCookiePermission implementation we use instead in order to provide the necessary overrides? For example, I think it would be a lot cleaner if we mark nsCookiePermission as non-final and make a small patch to https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/extensions/cookie/nsCookiePermission.cpp#54 to return a different object (presumably of a class that inherits from nsCookiePermission) for Thunderbird.

Flags: needinfo?(ehsan)

Thanks for the comment. Let me just confirm. You call what is basically a 10-liner

#ifdef MOZ_THUNDERBIRD
  // Check this protocol doesn't allow cookies
  bool hasFlags;
  nsCOMPtr<nsIURI> uri;
  aPrincipal->GetURI(getter_AddRefs(uri));
  nsresult rv = NS_URIChainHasFlags(
      uri, nsIProtocolHandler::URI_FORBIDS_COOKIE_ACCESS, &hasFlags);
  if (NS_FAILED(rv) || hasFlags) {
    *aResult = ACCESS_DENY;
    return NS_OK;
  }
#endif

(+ const unsigned long URI_FORBIDS_COOKIE_ACCESS = (1<<31); somewhere in an include file, using 31 to not be subject to renumbering) an invasive change and would prefer a somewhat twisted inheritance solution that moves those ten lines into comm-central?

We'd also have to override nsCookiePermission::CanAccess(), so if you decided to discontinue that in the future, we'd have to come up with something else, only that the override function is in a different repository and not so easy to see.

Attached patch 1543219-m-c.patch (obsolete) — — Splinter Review

Just to explore Ehsan't idea I started with some tweaks, but it unravels quickly.

In order to override nsCookiePermission::GetOrCreate() we need to lose the static-ness of that function and it's downhill from there with sweeping, not to say invasive, changes to fix these compile errors (which I haven't done).

 0:08.38 c:/mozilla-source/comm-central/extensions/cookie/nsCookieModule.cpp(10,47): error: call to non-static member function without an object argument
 0:08.38 void CookieModuleDtor() { nsCookiePermission::Shutdown(); }
 0:08.38                           ~~~~~~~~~~~~~~~~~~~~^~~~~~~~

and the same in CookieServiceChild.cpp and AntiTrackingCommon.cpp.

Here's the C-C part, interesting are only the changes in nsMsgUtils.cpp where I put the derived class.

Obviously, due to the issues mentioned in the previous comment, the whole thing doesn't compile.

Ehsan, do you still think your suggestion is feasible and cleaner? What exactly don't you plan (quote, comment #20) "to support in the long term" and why would the block shown in comment #21 get in the way?

Flags: needinfo?(ehsan)

(In reply to Jorg K (GMT+2) from comment #21)

Thanks for the comment. Let me just confirm. You call what is basically a 10-liner

#ifdef MOZ_THUNDERBIRD
  // Check this protocol doesn't allow cookies
  bool hasFlags;
  nsCOMPtr<nsIURI> uri;
  aPrincipal->GetURI(getter_AddRefs(uri));
  nsresult rv = NS_URIChainHasFlags(
      uri, nsIProtocolHandler::URI_FORBIDS_COOKIE_ACCESS, &hasFlags);
  if (NS_FAILED(rv) || hasFlags) {
    *aResult = ACCESS_DENY;
    return NS_OK;
  }
#endif

(+ const unsigned long URI_FORBIDS_COOKIE_ACCESS = (1<<31); somewhere in an include file, using 31 to not be subject to renumbering) an invasive change and would prefer a somewhat twisted inheritance solution that moves those ten lines into comm-central?

That's right. This has nothing to do with the number of lines of codes changed, but to do with what semantics we would need to maintain in Gecko. The reason why part 1 of bug 1517057 removed that protocol handler flag was part 2 of that bug, otherwise part 2 of that bug would also need to perform the same checks, and we would need to maintain this logic forever into the future. This hopefully answers comment 24. I'm gonna leave some comments on your patches...

I would like this cost be paid where the benefits are reaped, that is in c-c not in m-c.

We'd also have to override nsCookiePermission::CanAccess(), so if you decided to discontinue that in the future, we'd have to come up with something else, only that the override function is in a different repository and not so easy to see.

I'm sorry about that, the way that Thunderbird code is structured and built isn't my (or probably your) top choice, but we have to live with what we have...

Flags: needinfo?(ehsan)
Comment on attachment 9060203 [details] [diff] [review]
1543219-m-c.patch

Review of attachment 9060203 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/cookie/nsCookiePermission.h
@@ +21,5 @@
>    NS_DECL_NSIOBSERVER
>  
> +  // Singleton accessor, overridden in Mailnews.
> +  virtual already_AddRefed<nsICookiePermission> GetOrCreate();
> +  virtual void Shutdown();

Umm, this completely changes the meaning of this code!  The GetOrCreate() function is supposed to be the thing that creates the only nsCookiePermission object, and Shutdown() is supposed to be the thing that deletes that object.  By making them virtual you're forcing the consumer to create an object first on the stack to call these methods on it, which isn't how these are called in m-c.

To simplify, your patch will break Firefox builds, so it's not good as is.  :-)

But more importantly, why did you need to do this in the first place?  What compile errors did you run into?
Comment on attachment 9060205 [details] [diff] [review]
1543219-nsCookiePermissionMailnews.patch

Don't forget CanAccessURI...

Feel free to add a new enum to m-c so that you don't have to carry around this `1<<31` business, wrapped in the necessary #ifdef's.  :-)

(In reply to :Ehsan Akhgari from comment #27)

Feel free to add a new enum to m-c so that you don't have to carry around
this 1<<31 business, wrapped in the necessary #ifdef's. :-)

(This is actually important in case m-c later on adds its own new protocol handler flag with the same value!)

(In reply to :Ehsan Akhgari from comment #26)

To simplify, your patch will break Firefox builds, so it's not good as is.
:-)

Sure, it doesn't compile :-(

But more importantly, why did you need to do this in the first place? What
compile errors did you run into?

Well, the idea is that Mailnews creates its own nsCookiePermissionMailnews class which overrides CanAccess(). So somehow GetOrCreate() will need to create that Mailnews object and hand it back. Therefore the failed attempt to override that, too.

Let me ask another way: What tweak exactly did you have in mind in comment #20 when you said:

... make a small patch to https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/extensions/cookie/nsCookiePermission.cpp#54 to return a different object (presumably of a class that inherits from nsCookiePermission) for Thunderbird.

BTW, that 1<<31 business was just temporary, I'll add nsIProtocolHandler::URI_FORBIDS_COOKIE_ACCESS back, but at the end.

(In reply to Jorg K (GMT+2) from comment #29)

But more importantly, why did you need to do this in the first place? What
compile errors did you run into?

Well, the idea is that Mailnews creates its own nsCookiePermissionMailnews class which overrides CanAccess(). So somehow GetOrCreate() will need to create that Mailnews object and hand it back. Therefore the failed attempt to override that, too.

Right. I was suggesting to make a small change here: https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/extensions/cookie/nsCookiePermission.cpp#56, something like:

#ifdef MOZ_THUNDERBIRD
  gSingleton = new nsCookiePermissionMailnews();
#else
  gSingleton = new nsCookiePermission();
#endif

Plus adding an #include for nsCookiePermissionMailnews, that's about all I think we need to change on the m-c side.

Let me ask another way: What tweak exactly did you have in mind in comment #20 when you said:

... make a small patch to https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/extensions/cookie/nsCookiePermission.cpp#54 to return a different object (presumably of a class that inherits from nsCookiePermission) for Thunderbird.

BTW, that 1<<31 business was just temporary, I'll add nsIProtocolHandler::URI_FORBIDS_COOKIE_ACCESS back, but at the end.

Great! Makes sense.

This is what I have so far. It compiles, but doesn't work. Looks like nsCookiePermissionMailnews::CanAccess[URI]() is never invoked. Frankly, I can't see a call site in M-C other than in AntiTrackingCommon.cpp, but I didn't look very carefully.

I'm not sure whether the

#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
mozilla::StaticRefPtr<nsCookiePermissionMailnews> gSingleton;
#else
mozilla::StaticRefPtr<nsCookiePermission> gSingleton;
#endif

is needed. I could imagine that only storing a pointer to the M-C class might lose the "Mailnews-ness" in case we compile it for TB.

%{C++
#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
    /**
     * This protocol handler forbids accessing cookies e.g. for mail related
     * protocols. We use the higest flag possible to avoid reshuffling
     * if more flags need to be added.
     */
    #define URI_FORBIDS_COOKIE_ACCESS (1<<31)
#endif
%}

is very unfortunate, but #ifdef only appears to work within %{C++ %} blocks. The IDL processor is producing an enum like so:

  enum {
    URI_STD = 0U,
    ...
    URI_DISALLOW_IN_PRIVATE_CONTEXT = 4194304U
  };

but I don't know how to extend this. Unless we just add URI_FORBIDS_COOKIE_ACCESS without #ifdef.

Of course the #define only works in C++ code, so the JS bits Ci.nsIProtocolHandler.URI_FORBIDS_COOKIE_ACCESS in the 1543219-reinstate-URI_FORBIDS_COOKIE_ACCESS-flags patch don't actually work.

Attachment #9057099 - Attachment is obsolete: true
Attachment #9060203 - Attachment is obsolete: true
Attachment #9060205 - Attachment is obsolete: true
Attachment #9060571 - Flags: feedback?(ehsan)

Sorry, forgot refresh before attaching patch.

Attachment #9060571 - Attachment is obsolete: true
Attachment #9060571 - Flags: feedback?(ehsan)
Attachment #9060574 - Flags: feedback?(ehsan)

(In reply to Jorg K (GMT+2) from comment #31)

... . It compiles, but doesn't work. Looks like nsCookiePermissionMailnews::CanAccess[URI]() is never invoked. Frankly, I can't see a call site in M-C other than in AntiTrackingCommon.cpp, but I didn't look very carefully.

I restored the code shown in comment #25 and all the bits that go with it and also added it to nsCookiePermission::CanAccessURI()(or more precisely, backed out M-C rev 23a0332b18a1 and C-C rev a95b5c837765 and added code to CanAccessURI()) and then I ran the JS test from comment #15. Result: "mail=66" :-(

IOW, neither restoring/adding to nsCookiePermission::CanAccess[URI]() nor overriding it fixes the problem since, as far as I can tell, those functions aren't run any more.

Oh, right. This is because after bug 1517057 nsCookiePermission became a thin wrapper around just checking for "cookie" permissions, and then stuff got refactored in bug 1525245 and as part of that now we check for the permissions directly: https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/netwerk/cookie/CookieSettings.cpp#137

Sorry about all of this. This stuff is under active development and even though I've been involved in it all it took me a while to put all of what Thunderbird needs together into a puzzleboard in my mind.

So long story short, the workaround discussed thusfar isn't really viable in m-c any more. (In fact we should probably kill part or all of nsCookiePermission at this point since the code is really pointless now.)

You also didn't like the idea in comment 11. I have to admit that I can't think of a third idea at the moment unfortunately, so let me know what suggestions you may have. You've dealt with more similar issues in the past so I hope you can think of something that may not be occurring to me right now...

Thanks!

Comment on attachment 9060574 [details] [diff] [review]
1543219-nsCookiePermissionMailnews-M-C.patch

Clearing the feedback request since the patch doesn't work.
Attachment #9060574 - Flags: feedback?(ehsan)

The (obvious?) suggestions are:

  1. Add some #ifdef MOZ_THUNDERBIRD code above https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/netwerk/cookie/CookieSettings.cpp#137, mostly what we have in comment #25. You didn't like the idea to add this code to CanAccess() so you might not like it here either.
  2. Instead of sub-classing nsCookiePermission we could sub-class CookieSettings and apply a similar trick, that is, to override CookieSettings::CookiePermission().

Please let me know what you think.

(In reply to Jorg K (GMT+2) from comment #38)

The (obvious?) suggestions are:

  1. Add some #ifdef MOZ_THUNDERBIRD code above https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/netwerk/cookie/CookieSettings.cpp#137, mostly what we have in comment #25. You didn't like the idea to add this code to CanAccess() so you might not like it here either.

Is that sufficient though? There are other places in the code where we directly query the permission manager for a "cookie" permission without going through the CookieSettings object. For example for service workers. But I guess Thunderbird doesn't care about that particular case.

Since it looks like we don't have any better alternatives (see below) I'm happy to rubberstamp the patch if you want to own this code, I just want to be up front that I haven't done all of the mental work to be completely sure that you wouldn't need something more sophisticated. :-)

(This patch would definitely takes care of the cookie database side of things, FWIW, but many things inside Gecko all obey the same cookie permissions and sometimes things break when they get out of sync because of hidden assumptions about the state of one after observing the state of another.)

  1. Instead of sub-classing nsCookiePermission we could sub-class CookieSettings and apply a similar trick, that is, to override CookieSettings::CookiePermission().

There are multiple creation points for CookieSettings, so I'm afraid this option would require even more patching in m-c!

Thanks, Ehsan, I'll look into it in the next few days. I see that you did some clean-up in bug 1547114.

Attached patch 1543219-M-C-part.patch (obsolete) — — Splinter Review

This makes the test pass together with the C-C patch I'm about to attach.

Attachment #9060572 - Attachment is obsolete: true
Attachment #9060573 - Attachment is obsolete: true
Attachment #9060574 - Attachment is obsolete: true
Attachment #9061136 - Flags: review?(ehsan)

This makes mach gTest TestMailCookie* pass again without modifications.

We have various tests with check protocol flags and I'm not sure that I got that part right yet. It appears that Ci.nsMsgProtocolHandler.URI_FORBIDS_COOKIE_ACCESS is not available. But we can fix that after Ehsan gave his OK on the M-C part.

EDIT: The C-C patch is basically a backout of https://hg.mozilla.org/comm-central/rev/a95b5c837765, only that the constant has moved to C-C. It's added again nine times, when that rev removed it nine times.

Attachment #9061138 - Flags: feedback?(acelists)

OK, now the tests pass. Looks like bitwise or in JS uses 32 bit integers:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Bitwise_Operators
Quote: The operands of all bitwise operators are converted to signed 32-bit integers.

That doesn't work since our flag is 1<<31, so MAXINT+1, so negative as a signed 32-bit integer.

Attachment #9061138 - Attachment is obsolete: true
Attachment #9061138 - Flags: feedback?(acelists)
Attachment #9061145 - Flags: review?(acelists)
Comment on attachment 9061145 [details] [diff] [review]
1543219-reinstate-URI_FORBIDS_COOKIE_ACCESS-flags.patch (v2)

Review of attachment 9061145 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/news/test/unit/test_nntpProtocols.js
@@ +3,5 @@
>   * Test suite for getting news urls via the protocol handler.
>   */
>  
> +// Bitwise or doesn't work for Ci.nsMsgProtocolHandler.URI_FORBIDS_COOKIE_ACCESS
> +// which is 1<<31.

So why not use 1<<30? It is still the maximum usable flag. 1<<31 is not usable as it breaks in JS bitwise operators and you had to hack around it with + instead of |. I wonder if other users will notice that they can use | in C++, but have to use + in JS. This just calls for problems. Ehsan, would you mind 1<<30 ?

I chose 1<<31 to the the last available bit just in case M-C are adding more flags here:
https://searchfox.org/mozilla-central/rev/444ee13e14fe30451651c0f62b3979c76766ada4/netwerk/base/nsIProtocolHandler.idl#332

Maybe we should include a comment in the M-C patch like:

// Note that Mailnews is using an additional flag with the value of 1<<31 (or 1<<30).

OK, let's go with 1<<30. I'll update the M-C patch in a minute.

Attachment #9061136 - Attachment is obsolete: true
Attachment #9061145 - Attachment is obsolete: true
Attachment #9061136 - Flags: review?(ehsan)
Attachment #9061145 - Flags: review?(acelists)
Attachment #9061173 - Flags: review?(acelists)
Attached patch 1543219-M-C-part.patch (v2) (obsolete) — — Splinter Review

Now using 1<<30 and with a big comment in nsIProtocolHandler.idl.

Attachment #9061174 - Flags: review?(ehsan)
Comment on attachment 9061174 [details] [diff] [review]
1543219-M-C-part.patch (v2)

Review of attachment 9061174 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsIProtocolHandler.idl
@@ +336,5 @@
> +     * 1<<31 cannot be used since in JavaScript bitwise OR uses signed 32bit
> +     * integers (and 1<<31 > MAXINT).
> +     * Please get in contact with the Thunderbird team should you need to
> +     * add new flags and require the 1<<30 slot.
> +     */

Not sure where the idea of using bit 31 (or 30 for that matter) came from.  There are many free bits here already, no need to get in trouble like this, why not use bit 23?

Also, instead of a comment which may be missed by the next person changing things here, please actually redefine the constant again and add a comment saying that this constant is only used in comm-central.  The presence of the constant itself in m-c doesn't hurt anyone.

::: netwerk/cookie/CookieSettings.cpp
@@ +147,5 @@
> +  nsCOMPtr<nsIURI> uri;
> +  aPrincipal->GetURI(getter_AddRefs(uri));
> +  rv = NS_URIChainHasFlags(uri, 1<<30, &hasFlags);
> +  if (NS_FAILED(rv) || hasFlags) {
> +    *aCookiePermission = nsPermissionManager::DENY_ACTION;

Your logic is backwards here.  You should *first* check the protocol flag, and if it is not set then check the permission.  If the protocol flag tells you the permission is denied you should continue with the rest of the code in this function.

One way to do this cleanly could be to move this #ifdef block before the `TestpermissionFromPrincipal` block above, and at the end of the if statement below add an else line so that in Thunderbird/SeaMonkey the logic would read:

If the protocol flag check failed or it existed, deny the permission, otherwise ask the permission manager.

And in Firefox it would be:

Ask the permission manager.
Attachment #9061174 - Flags: review?(ehsan) → review-
Attached patch 1543219-M-C-part.patch (v3) (obsolete) — — Splinter Review

Like this? The "dangling" else is a little dangerous so I added heaps of comments around it. I also ran clang formatting and this is the result.

Attachment #9061173 - Attachment is obsolete: true
Attachment #9061174 - Attachment is obsolete: true
Attachment #9061173 - Flags: review?(acelists)
Attachment #9061472 - Flags: review?(ehsan)
Comment on attachment 9061472 [details] [diff] [review]
1543219-M-C-part.patch (v3)

Review of attachment 9061472 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments fixed.  Note the first comment especially since without it the patch may do unexpected things (let cookie permissions override what the protocol handler specifies.)

::: netwerk/cookie/CookieSettings.cpp
@@ +145,5 @@
> +  rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_FORBIDS_COOKIE_ACCESS,
> +                           &hasFlags);
> +  if (NS_FAILED(rv) || hasFlags) {
> +    *aCookiePermission = nsPermissionManager::DENY_ACTION;
> +    rv = NS_OK;  // Reset, so it's not caught as a bad status after the `else`.

Here you should return NS_OK.  If the protocol handler says we don't have cookie access, that's the end of the permission lookup!

@@ +151,5 @@
> +#endif
> +
> +    // Note that the indentation is created by clang-formatting since
> +    // when compiled for Thunderbird/SeaMonkey, the following statement
> +    // is controlled by the `else` in the `#ifdef` block above.

Noted.  But please remove the comment before landing?  :-)
Attachment #9061472 - Flags: review?(ehsan) → review+

Thanks.

I'm confused now. You suggested to add the else so the other test wouldn't run of the protocol denied the access. If I do a return, then I don't need the else.

I thought the code that follows is needed, no? After TestPermissionFromPrincipal() you also don't return on a single rejection but instead pack the result into mCookiePermissions.

I intended to leave the comment in place. The indented call looks funny and people might not see the else above.

Flags: needinfo?(ehsan)

(In reply to Jorg K (GMT+2) from comment #51)

Thanks.

I'm confused now. You suggested to add the else so the other test wouldn't run of the protocol denied the access. If I do a return, then I don't need the else.

I thought the code that follows is needed, no? After TestPermissionFromPrincipal() you also don't return on a single rejection but instead pack the result into mCookiePermissions.

Oh yes, you're completely right! Sorry about that. I was going back and forth looking at the old code and ended up confusing myself. Thanks for catching me. :-)

I intended to leave the comment in place. The indented call looks funny and people might not see the else above.

Please don't. We use clang-format for all code now. Nobody cares about why whitespace is the way that it is, the answer to all such questions is: because clang-format did it. No need to write that answer in comments all over the code base. :-)

(Maybe this looks strange coming from c-c, but it's daily business in m-c!)

Flags: needinfo?(ehsan)
Attached patch 1543219-M-C-part.patch (v3b) (obsolete) — — Splinter Review

Adjusted comment, carrying forward Ehsan's r+.

Attachment #9061472 - Attachment is obsolete: true
Attachment #9062230 - Flags: review+
Keywords: checkin-needed
Comment on attachment 9062230 [details] [diff] [review]
1543219-M-C-part.patch (v3b)

># HG changeset patch
># User Jorg K <jorgk@jorgk.com>
># Date 1556813271 -7200
># Parent  09753a1a153d348d456a0c785fcffeb9a77dd065
>Bug 1543219 - Allow Mailnews to check protocol flags for cookie permissions. r=ehsan
>
>This reinstates nsIProtocolHandler::URI_FORBIDS_COOKIE_ACCESS removed in bug 1517057, Part 1, rev 23a0332b18a1
>
>diff --git a/netwerk/base/nsIProtocolHandler.idl b/netwerk/base/nsIProtocolHandler.idl
>--- a/netwerk/base/nsIProtocolHandler.idl
>+++ b/netwerk/base/nsIProtocolHandler.idl
>@@ -325,9 +325,15 @@ interface nsIProtocolHandler : nsISuppor
>      * The URIs for this protocol can be loaded by extensions.
>      */
>     const unsigned long URI_LOADABLE_BY_EXTENSIONS = (1 << 21);
> 
>     /**
>      * The URIs for this protocol can not be loaded into private contexts.
>      */
>     const unsigned long URI_DISALLOW_IN_PRIVATE_CONTEXT = (1 << 22);
>+
>+    /**
>+     * This protocol handler forbids accessing cookies e.g. for mail related
>+     * protocols. Only used in Mailnews (comm-central).
>+     */
>+    const unsigned long URI_FORBIDS_COOKIE_ACCESS = (1 << 23);
> };
>diff --git a/netwerk/cookie/CookieSettings.cpp b/netwerk/cookie/CookieSettings.cpp
>--- a/netwerk/cookie/CookieSettings.cpp
>+++ b/netwerk/cookie/CookieSettings.cpp
>@@ -2,16 +2,19 @@
> /* vim: set ts=8 sts=2 et sw=2 tw=80: */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "mozilla/net/CookieSettings.h"
> #include "mozilla/Unused.h"
> #include "nsGlobalWindowInner.h"
>+#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
>+#  include "nsIProtocolHandler.h"
>+#endif
> #include "nsPermission.h"
> #include "nsPermissionManager.h"
> 
> namespace mozilla {
> namespace net {
> 
> namespace {
> 
>@@ -129,18 +132,33 @@ CookieSettings::CookiePermission(nsIPrin
>   }
> 
>   // Let's ask the permission manager.
>   nsPermissionManager* pm = nsPermissionManager::GetInstance();
>   if (NS_WARN_IF(!pm)) {
>     return NS_ERROR_FAILURE;
>   }
> 
>-  rv = pm->TestPermissionFromPrincipal(aPrincipal, NS_LITERAL_CSTRING("cookie"),
>-                                       aCookiePermission);
>+#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
>+  // Check if this protocol doesn't allow cookies.
>+  bool hasFlags;
>+  nsCOMPtr<nsIURI> uri;
>+  aPrincipal->GetURI(getter_AddRefs(uri));
>+  rv = NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_FORBIDS_COOKIE_ACCESS,
>+                           &hasFlags);
>+  if (NS_FAILED(rv) || hasFlags) {
>+    *aCookiePermission = nsPermissionManager::DENY_ACTION;
>+    rv = NS_OK;  // Reset, so it's not caught as a bad status after the `else`.
>+  } else         // Note the tricky `else` which controls the call below.
>+#endif
>+
>+    // Note that when compiled for Thunderbird/SeaMonkey, the following
>+    // statement is controlled by the `else` in the `#ifdef` block above.

Could you please remove this comment?  Thanks!

>+    rv = pm->TestPermissionFromPrincipal(
>+        aPrincipal, NS_LITERAL_CSTRING("cookie"), aCookiePermission);
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> 
>   // Let's store the permission, also if the result is UNKNOWN in order to avoid
>   // race conditions.
> 
>   nsCOMPtr<nsIPermission> permission = nsPermission::Create(
Attachment #9062230 - Flags: review+ → review-
Keywords: checkin-needed
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/254f28b5aa72
Allow Mailnews to check protocol flags for cookie permissions. r=jorgk

Looks like wires crossed here by a matter of minutes and the comment got landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/254f28b5aa72#l2.47

As you had requested, I removed the part of the comment that referred to the indentation and the clang-formatting, but I left this:

+    // Note that when compiled for Thunderbird/SeaMonkey, the following
+    // statement is controlled by the `else` in the `#ifdef` block above.
+    rv = pm->TestPermissionFromPrincipal(
+        aPrincipal, NS_LITERAL_CSTRING("cookie"), aCookiePermission);

Personally I think this comment is necessary since the dangling else in the #ifdef block is somewhat hacky, or unusual, or at least a little dangerous. People might not visually inspect code that is clearly "npotb" (not part of the build) and place something something after the block.

If you feel very strongly about removing the comment, I'll file a follow-up patch to remove it, so please let me know.

Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)

Hmm, DONTBUILD would have been worth considering here.

Both changeset combined represent what was landed.

Attachment #9062230 - Attachment is obsolete: true
Attachment #9062283 - Flags: review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/37b96898406c
Back out rev a95b5c837765 from bug 1517360. a=backout

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

https://hg.mozilla.org/mozilla-central/rev/254f28b5aa72908bffdeb155e5ce94d65bbfd234 (M-C part).

Target Milestone: --- → Thunderbird 68.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: