+++ 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?

Actually, you can just add |a@a.b| and see that it doesn't work.

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.

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().

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.

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.

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.

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.

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

(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.

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.

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.

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().

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.

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.

(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.

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.

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?

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

(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.

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).

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.

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 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.

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.

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.

(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 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.

Refreshed for bug 1329958.

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.

We can take that in a followup bug.

(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..

Comment on attachment 8825561 [details] [diff] [review] 1329494-remove-questionmark.patch (v2). This one applies to latest trunk.

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.

(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.

Could someone else check if this still works in TB54. I am not seeing any new permissions added in SeaMonkey 2.51.

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.

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

Worked for another message not in the inbox. Forget it for now. Need to investigate further.

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.