Remote content exceptions for white-listing e-mail addresses broken, test failure: TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_accountMigration.js

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
General
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 53.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 months ago
+++ This bug was initially created as a clone of Bug #1328847 +++

From bug 1328847 comment #41:

If I list my friend henk@henk.com.au in TB 52, I see chrome://messenger/content/messenger.xul in the "Execeptions - Remote Content" now on TB 53 trunk.

At a guess, this is what makes
mozilla/mach xpcshell-test mailnews/base/test/unit/test_accountMigration.js
fail, since that migrates an address and then checks permissions for
  "chrome://messenger/content/?email=no@test.invalid"
which is a white-listed e-mail hacked together in bug 1193200.

Magnus, can I assign this to you?
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Updated

4 months ago
Summary: Remote content exceptions for white-listing e-mail addressed broken → Remote content exceptions for white-listing e-mail addresses broken
(Assignee)

Comment 1

4 months ago
Actually, you can just add |a@a.b| and see that it doesn't work.
(Assignee)

Comment 2

4 months ago
I checked the regression range here:

Last good: 53.0a1 (2017-01-04) (64-bit)
M-C: https://hg.mozilla.org/mozilla-central/rev/57ac9f63fc6953f4efeb0cc84a60192d3721251f

First bad: 53.0a1 (2017-01-05) (64-bit)
M-C: https://hg.mozilla.org/mozilla-central/rev/f13abb8ba9f366c9f32a3146245adf642528becd

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=57ac9f63fc69&tochange=f13abb8ba9f3

I assume this is also caused by bug 1182569, as is bug 1328847, whose investigation of the failure in mailnews/base/test/unit/test_accountMigration.js brought us here in the first place.

Updated

4 months ago
Assignee: nobody → mkmelin+mozilla
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 3

4 months ago
Oh, I just saw that Magnus assigned himself. I was looking at it for a little while.

permissions.sqlite has this data in moz_perms:
chrome://messenger/content/?email=henk@henk.com.au
chrome://messenger/content/?email=jorgk@jorgk.com

However, dumping out principal.origin after
mail/components/preferences/permissions.js:432
I get
=== Origin chrome://messenger/content/messenger.xul

Christoph and/or Boris: Does that look like something caused by bug 1182569? I doesn't seem to be related to AsyncOpen2().
Flags: needinfo?(ckerschb)
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 4

4 months ago
Actually, I should mention that blocking remote images in e-mail is currently broken altogether. All images are shown on trunk when TB52 blocks them on the same profile. I don't know whether that's due to this bug or bug 1329288.
(Assignee)

Comment 5

4 months ago
I forgot to add: I'm quite surprised that our test-general-content-policy.js test hasn't failed due to this and only the obscure address migration shows a problem.
(In reply to Jorg K (GMT+1) from comment #4)
> Actually, I should mention that blocking remote images in e-mail is
> currently broken altogether. All images are shown on trunk when TB52 blocks
> them on the same profile. I don't know whether that's due to this bug or bug
> 1329288.

Most likely this was also caused by Bug 1182569. Working on a fix for Bug 1329288 at the moment.
Flags: needinfo?(ckerschb)
It's really not clear to me what the connection to asyncOpen2 is here, but given the regression range it does seem likely.  

I'd still like an answer to my question from bug 1328847 comment 34, I think.  But failing that, logging all the calls that come through the asyncOpen2 callsite in URILoader during this test might be a good start at figuring out which ones might be relevant here and aren't obvious.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 8

4 months ago
Adding the test failure
mozilla/mach xpcshell-test mailnews/base/test/unit/test_accountMigration.js
to this bug since I closed bug 1328847.

I'll do some further investigation according to comment #7.
Summary: Remote content exceptions for white-listing e-mail addresses broken → Remote content exceptions for white-listing e-mail addresses broken, test failure: TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_accountMigration.js

Comment 9

4 months ago
I didn't have time to investigate yet, but based on comment 3 it would be good to know if it's that the format of the origin changed for chrome:// urls, or if it's somehow now not using what's passed in. 
https://dxr.mozilla.org/comm-central/rev/bb704755f3e81797b2f6566de9956277a3ac5d39/mailnews/base/util/mailnewsMigrator.js#133
(Assignee)

Comment 10

4 months ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> I'd still like an answer to my question from bug 1328847 comment 34, ...
Here comes around
http://searchfox.org/comm-central/source/mailnews/base/util/mailnewsMigrator.js#133

 0:01.56 PROCESS_OUTPUT: Thread-1 (pid:1508) "created URI for chrome://messenger/content/?email=yes@test.invalid"
 0:01.57 PROCESS_OUTPUT: Thread-1 (pid:1508) "created URI for chrome://messenger/content/?email=yes2@test.invalid"
So as I suspected, the valid entries are migrated, the invalid one is not, that's why checking it should fail.

But since there's gone something wrong with returning/checking the hacked chrome URIs, all checks pass. Remember, the test checks:
  let uriAllowed = Services.io.newURI("chrome://messenger/content/?email=yes@test.invalid", null, null);
  let uriAllowed2 = Services.io.newURI("chrome://messenger/content/?email=yes2@test.invalid", null, null);
  let uriDisallowed = Services.io.newURI("chrome://messenger/content/?email=no@test.invalid", null, null);
and the check for the last one succeeds although it should fail since nothing is migrated for that address.

I'll look at "asyncOpen2 callsite in URILoader during this test" next.
(Assignee)

Comment 11

4 months ago
No calls to ASyncOpen2() from nsURILoader::OpenURI() in nsURILoader.cpp.

I think my best bet is to dig into where the test goes:
  Services.perms.testPermission(uriDisallowed, "image")
and check what's happening there.

As we've seen in comment #3, Services.perms.enumerator returns funny permission with funny principals and origins, well, funny may be in the eye of the beholder, but it's not returning what's in the database.
(Assignee)

Comment 12

4 months ago
Here's more debug:

"=== nsPermissionManager::CommonTestPermission called with spec chrome://messenger/content/?email=yes@test.invalid"
"=== nsPermissionManager::CommonTestPermission returning (4) *aPermission = 1"
"=== nsPermissionManager::CommonTestPermission called with spec chrome://messenger/content/?email=yes2@test.invalid"
"=== nsPermissionManager::CommonTestPermission returning (4) *aPermission = 1"
"=== nsPermissionManager::CommonTestPermission called with spec chrome://messenger/content/?email=no@test.invalid"
"=== nsPermissionManager::CommonTestPermission returning (4) *aPermission = 1"

Here the code that goes with the second debug line:

  *aPermission = aIncludingSession
                   ? entry->GetPermission(typeIndex).mPermission
                   : entry->GetPermission(typeIndex).mNonSessionPermission;

  printf("=== nsPermissionManager::CommonTestPermission returning (4) *aPermission = %d\n", *aPermission);
  return NS_OK;
}

I'll attach the debug patch once I'm done.
(Assignee)

Comment 13

4 months ago
Looks like I found it:
"=== nsPermissionManager::CommonTestPermission called with spec chrome://messenger/content/?email=no@test.invalid"
"=== PermissionKey::CreateFromPrincipal origin=|chrome://messenger/content/|"
"=== nsPermissionManager::CommonTestPermission returning (4) *aPermission = 1"

So despite the right origin is passed in, that is,
  chrome://messenger/content/?email=no@test.invalid,
the permission key which is produced in nsPermissionManager::PermissionKey::CreateFromPrincipal() which uses GetOriginFromPrincipal(aPrincipal, origin); returns
  chrome://messenger/content/.
Hmm, that's not going to be a total success. If the query part is stripped, all hacked e-mail chrome URIs are going to return the same key.

Same symptom as seen in comment #3, where the permission's origin lost the query part "?email=...".

So let's see what changed in GetOriginFromPrincipal().
(Assignee)

Comment 14

4 months ago
OK, I'm done here, Boris and Christoph, you're off the hook ;-)
(not that you were ever on the hook since no one ever saw the connection to AsyncOpen2()).

Here's your regression unfortunately in the same range as bug 1182569:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=57ac9f63fc69&tochange=f13abb8ba9f3

Da, dah!!
https://hg.mozilla.org/mozilla-central/rev/974a431df67c#l1.19 - bug 1321550

So in summary, the horrible hack of using chrome URIs to store e-mail addresses in the permission manager has fallen apart since the origin should not really contain the query part. I'll leave it to Magnus to hack it further.
Blocks: 1321550
No longer blocks: 1182569, 1328847
No longer depends on: 1329288
(Assignee)

Comment 15

4 months ago
Created attachment 8825426 [details] [diff] [review]
nsPermissionManager-debug.patch

Debug patch just for the record.
Arguably, the code from bug 1321550 is buggy because it changed the chrome:// behavior, even though the comments in GetOriginInternal _explicitly_ talk about using the full URL for the chrome:// case.  That is, the truncation stuff should perhaps have been conditioned on !isChrome.

That said, I do think the new setup makes a bit more sense, even for chrome:// urls in general.  Certainly for the '#' case; I'm less sure about the '?' case.
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(amarchesini)

Comment 17

4 months ago
(In reply to Jorg K (GMT+1) from comment #14)
> 
> So in summary, the horrible hack of using chrome URIs to store e-mail
> addresses in the permission manager has fallen apart since the origin should
> not really contain the query part. I'll leave it to Magnus to hack it
> further.

Your past and current commentary on this is completely invalid and you should knock it off. It is totally valid to adapt urls for mailids to fit permissions manager in this manner, it is a completely valid and necessary use case to key off mailids, and it would be an enormous mistake to write a custom permissions manager to handle this mailnews use case due to m-c removing such past accommodations.

Even if this regression was not a mistake (like comment 16 implies) the url should still continue to be adapted to permissions manager for mailid purposes.
(Assignee)

Comment 18

4 months ago
Assembling a chrome URI with an e-mail address is and remains a hack:
  chrome://messenger/content/?email=yes@test.invalid.
Such URIs don't exist and are not addressing anything. The current implementation was done against the *express* advice of various M-C staff, see bug 1193200 comment #65 (where Magnus himself spoke of a "fake"). Please read the comments following bug 1193200 comment #65, especially bug 1193200 comment #70. What was predicted has just occurred. The solution is based on unstable grounds. Since there is no testing for our "creative use" in M-C, it will just break, as can be seen here.

Comment 19

4 months ago
I'm familiar with all that. How about addressing "it would be an enormous mistake to write a custom permissions manager to handle this mailnews use case due to m-c removing such past accommodations"?  Is that your solution?  As opposed to a one liner that exempts chrome:// from the new rule?
(Assignee)

Comment 20

4 months ago
Sure, you're right with that. My preferred solution would have been to remove white/black-listing by e-mail address altogether. However, I suggested a different hack in bug 1193200 comment #33 using
  http://mailtoNN-xx.yy.com

Comment 21

4 months ago
(In reply to Jorg K (GMT+1) from comment #20)
> Sure, you're right with that. My preferred solution would have been to
> remove white/black-listing by e-mail address altogether.

I really disagree with that.

> However, I
> suggested a different hack in bug 1193200 comment #33 using
>   http://mailtoNN-xx.yy.com

That seems more brittle than simply removing the ? and going with a chrome url that can be constructed (note the word choice) to work.
I'm OK with fixing the comment but I also would like to keep the current behavior. In this way, file URLs and chrome URLs follow the same pattern and both work fine with QuotaManager and any other component who uses getOriginInternal.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 23

4 months ago
Created attachment 8825527 [details] [diff] [review]
1329494-remove-questionmark.patch (v1)

This is following Alta88's suggestion to just remove the "?" from the carefully constructed chrome URI. I need to wait for my build to finish to test it.

If we remove the "?" or change it to a ";" or something else, we of course have a migration problem (one SQL statement, so not hard at all).
(Assignee)

Comment 24

4 months ago
Created attachment 8825561 [details] [diff] [review]
1329494-remove-questionmark.patch (v2).

Oops, I had missed some in the first patch.

OK, removing the "?" from the chrome URI makes the test pass and you can also add e-mail addresses as exceptions. The panel works and the blocking/unblocking of content seems to work as well.

So it's all like it was before, all working with a carefully crafted chrome URI.
Attachment #8825527 - Attachment is obsolete: true
Attachment #8825561 - Flags: review?(mkmelin+mozilla)

Comment 25

4 months ago
Merely removing the ? is certainly good enough. The remainder of the construct falls within spec for http uri origins, particularly = and @.

But the true future proofer might change = to / since = doesn't add anything really and is a dodgy character anyway..

And no one is paranoid enough to think chrome:// or @ gets banned from perm manager origins.

Comment 26

4 months ago
Comment on attachment 8825561 [details] [diff] [review]
1329494-remove-questionmark.patch (v2).

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

Will this again ignore all existing stored permissions and cause the user to have to store them anew? If yes, we should relnote it or something.
If the permission manager does not return the stored URIs correctly when queried (cuts off the ? part already), we can't even make a one-time migration. Or is it just that it does not store new URIs with ? corretly, but those that are already in are returned properly?

Alta's suggestion seems fine. If we can replace = with some more safe character then we should do it now. Every change of the string risks that users loose their stored email perms.
Attachment #8825561 - Flags: feedback+

Comment 27

4 months ago
rs=me for whatever is needed to unbreak current builds. But longer term I hope that we can arrive at a solution that won't break every time some mozilla.com person sneezes.
(Assignee)

Comment 28

4 months ago
Thanks for all your input. Please note that Magnus is the assignee, so we really need a word from him. I merely spent a bit of time last night to identify the "call sites" using the "?" and making sure it works without it.

Alta88:
Thanks for checking "=" and "@". As I said, I'll let Magnus decide what he wants to do. I hope he does it quickly, since Daily is unusable due to this (and bug 1329288 to a lesser extent).

Aceman:
I think we *must* do a migration here since the already stored chrome URIs cause the system to malfunction, since they are truncated in the permission manager as pointed out. A simple SQL execution on the table:
  update moz_perms set origin= ... where origin like "chrome://messenger/content/?"
(details need to be determined) will do it. And yes, the one time migration needs to use SQL since retrieving the permissions from the permissions manager (via Services.perms.enumerator) will return them damaged. For testing I removed those records manually in my "DB Browser for SQLite".

Philip:
You sound a little desperate (and so am I). What do you mean by "unbreak current builds"? Are you referring the remote content exceptions not working or something else? I'd be happy to land this with Magnus' OK and let him prepare a follow-up patch that does what he wants to do and adds the migration. If he wants to use "/" instead of "=", I can of course do that now to reduce the churn.

Comment 29

4 months ago
(In reply to alta88 from comment #25)
> And no one is paranoid enough to think chrome:// or @ gets banned from perm
> manager origins.

Well I suppose @ could be cause trouble... (for http and ftp the part before would be username)

Comment 30

4 months ago
Comment on attachment 8825561 [details] [diff] [review]
1329494-remove-questionmark.patch (v2).

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

But I suppose this is good enough.
Attachment #8825561 - Flags: review?(mkmelin+mozilla) → review+

Comment 31

4 months ago
Created attachment 8825941 [details] [diff] [review]
1329494-remove-questionmark.patch

Refreshed for bug 1329958.
Attachment #8825561 - Attachment is obsolete: true
Attachment #8825941 - Flags: review+
(Assignee)

Comment 32

4 months ago
Thanks for the refresh. So I'll land this now to get the X's green (tree looks terrible, almost all red).

What about a migration? - we really need that.

Comment 33

4 months ago
We can take that in a followup bug.
Assignee: mkmelin+mozilla → jorgk

Comment 34

4 months ago
(In reply to Magnus Melin from comment #29)
> (In reply to alta88 from comment #25)
> > And no one is paranoid enough to think chrome:// or @ gets banned from perm
> > manager origins.
> 
> Well I suppose @ could be cause trouble... (for http and ftp the part before
> would be username)

I don't think username:password@ type urls are themselves stored in history or perms, only the referred urls they resolve to.  But yes, even @ could be crafted to guarantee never coming here again..
(Assignee)

Comment 35

4 months ago
Comment on attachment 8825561 [details] [diff] [review]
1329494-remove-questionmark.patch (v2).

This one applies to latest trunk.
Attachment #8825561 - Attachment is obsolete: false
(Assignee)

Comment 36

4 months ago
Comment on attachment 8825941 [details] [diff] [review]
1329494-remove-questionmark.patch

And this one doesn't. I'm not sure what you've done here, Magnus.
Attachment #8825941 - Attachment is obsolete: true
(Assignee)

Comment 37

4 months ago
https://hg.mozilla.org/comm-central/rev/6e08e4db98838348b9dbc54fe83f25e195a91096
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Updated

4 months ago
Blocks: 1330640
(Assignee)

Updated

4 months ago
QA Contact: jorgk
Version: 52 Branch → Trunk
(Assignee)

Updated

4 months ago
QA Contact: jorgk

Comment 38

4 months ago
(In reply to Jorg K (GMT+1) from comment #28)
> Philip:
> You sound a little desperate (and so am I). What do you mean by "unbreak
> current builds"? Are you referring the remote content exceptions not working
Yes the remote content exceptions. Sorry about the lack of clarity. Just getting fed up with spending all my time fire-fighting breakages caused by mozilla-central.

Updated

3 months ago
Blocks: 1331477
Flags: needinfo?(valentin.gosu)

Comment 39

3 months ago
Could someone else check if this still works in TB54. I am not seeing any new permissions added in SeaMonkey 2.51.
(Assignee)

Comment 40

3 months ago
What is the question exactly?

If I add xx@yy.com, it gets added, also visible in the database as
  chrome://messenger/content/email=xx@yy.com

So yes, this is working. And remote content is unblocked based on the list.

Comment 41

3 months ago
Thanks. Might be broken in SeaMonkey then. Not added to DB there as far as I see.

Comment 42

3 months ago
Worked for another message not in the inbox. Forget it for now. Need to investigate further.
(Assignee)

Comment 43

2 months ago
Beta (TB 52):
https://hg.mozilla.org/releases/comm-beta/rev/e45db4bd3b072abde58d95ebba256407e53846e4

This needed to be uplifted due to a merge conflict of bug 1337071 and bug 1330640.
status-thunderbird52: --- → fixed
status-thunderbird53: --- → fixed
You need to log in before you can comment on or make changes to this bug.