Closed Bug 1347598 Opened 7 years ago Closed 7 years ago

Images sent from Hotmail/Outlook aren't displayed [embedded ("cid:") image with attribute crossorigin="anonymous" not displayed]

Categories

(Thunderbird :: Message Reader UI, defect)

45 Branch
defect
Not set
normal

Tracking

(thunderbird_esr5254+ fixed, thunderbird53 wontfix, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 54+ fixed
thunderbird53 --- wontfix
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: jik, Assigned: jorgk-bmo)

Details

Attachments

(2 files, 20 obsolete files)

14.82 KB, text/plain
Details
18.85 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
Attached file sample.eml
This works in a Thunderbird message to display an inline image:

<img src="cid:7c1c11e9-e067-492a-bc60-32cf380c219f">

This does _not_ work:

<img crossorigin="anonymous" src="cid:7c1c11e9-e067-492a-bc60-32cf380c219f">

See attached sample message.
Status: NEW → UNCONFIRMED
Ever confirmed: false
Hotmail.com generates emails with crossorigin="anonymous" in inline image tags, so this affects a lot of messages read by a lot of people.
(How come your bugs don't get created as "New" automatically?).

TB 45 shows the attached message with a broken image icon, TB 52 beta and trunk show nothing. Not so nice. Replying to the test message in TB 45 shows nothing, replying in TB 52 shows the image.

Magnus, any idea that that crossorigin="anonymous" is about and why those images don't get rendered? Some newish security "feature"?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mkmelin+mozilla)
Version: Trunk → 45 Branch
>(How come your bugs don't get created as "New" automatically?).

They do, but I was told way back when I first started submitting code and that started happening that I was supposed to set them to "UNCONFIRMED" until someone else verified them. Perhaps that policy has changed, or perhaps I misunderstood the policy.
Well, the people with edit rights usually submit NEW bugs since they can be trusted somehow. I trust you ;-) And hey, interesting bug, have you tried various versions like I have? And under which circumstances in Hotmail generating this? Any estimate on the percentage? Or is it for *all* embedded images now? And do you have any background information on this?

Just doing a brief search on this
http://searchfox.org/mozilla-central/search?q=crossorigin&case=true&regexp=false&path=
reveals that this attribute triggers some special behaviour.

FF 51 displays a broken image for this:
<html>
<body>
<img crossorigin="anonymous" src='http://www.jorgk.com/images/usflag.png'><br>
</body>
</html>
I've told you all I know. ;-)

I had assumed that the behavior I observed in Hotmail messages was true for all messages generated by Hotmail, but I can't say for certain.
I sent an e-mail with an embedded image from Hotmail, result: crossorigin="anonymous" and no display in TB.

Boris, sorry to trouble you, who could we consult about the image crossorigin attribute? Please take a look at comment #4 which details that FF displays an image with that attribute as broken. I'd have to research why in TB we don't even see that broken image icon any more when we did see it in TB 45.
Flags: needinfo?(bzbarsky)
> who could we consult about the image crossorigin attribute?

Consulting me probably works.  ;)

Per HTML spec, having the "crossorigin" attribute set at all means that the load only succeeds if it's either same-origin or the response has the right CORS HTTP headers to allow a cross-origin load to happen.  In particular the load will always fail for cases when it's not same-origin and not being done via HTTP, since you can't produce HTTP headers otherwise.

The idea is to have a way to load images that are then safe to paint into a canvas without tainting it.

crossorigin="anonymous" means the load is done without sending cookies.

I have no idea why hotmail is doing this with its emails, honestly.

Anyway, from Thunderbird's point of view the obvious options are either for the loads to be considered "same origin" (that is for nsIPrincipal::CheckMayLoad to succeed when called on the relevant page principal with the URI of the image) or to somehow avoid having the crossorigin path get triggered at all (e.g. by stripping out that attribute or something).
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> Anyway, from Thunderbird's point of view the obvious options are either for
> the loads to be considered "same origin" (that is for
> nsIPrincipal::CheckMayLoad to succeed when called on the relevant page
> principal with the URI of the image) ...
Thanks Boris. Given that you're a DXR wizard, maybe you can give me some indication of how that could be achieved. As you know, we get called with ShouldLoad() at
https://dxr.mozilla.org/comm-central/rev/d03f6d1f69374c85d4939d0c7cb92021614600f4/mailnews/base/src/nsMsgContentPolicy.cpp#145
That's about the only thing I know.

> ... or to somehow avoid having the crossorigin path get triggered at
> all (e.g. by stripping out that attribute or something).
That would most likely have to happen in our MIME code, where we convert the CID reference to a reference to a mailnews URL (like mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Trash?number=7&part=1.2). Maybe somewhere around here:
https://dxr.mozilla.org/comm-central/rev/d03f6d1f69374c85d4939d0c7cb92021614600f4/mailnews/mime/src/mimemrel.cpp#848
But that's not pretty since we don't appear to be interpreting attributes of images (not that I looked closely).
I've changed the summary to make it easier to find the bug.
Summary: Thunderbird can't display inline ("cid:") image if "img" tag has "crossorigin='anonymous'" → Images sent from Hotmail/Outlook aren't displayed [embedded ("cid:") image with attribute crossorigin="anonymous" not displayed]
> Given that you're a DXR wizard, maybe you can give me some indication of how that could be achieved.

Well, you have control over two things here:

1)  The principal of the page.
2)  The nsIURI created for cid: stuff.

In this case, what is the principal?  What is the URI?

It sounds like the URI is mailbox:///something.  Does the "something" contain enough information to identify the message involved, and in particular its principal?  If so, making the URI QI to nsIURIWithPrincipal and having the principal it hands out be the message's principal would make things Just Work....

Note that mailbox:// URIs set ORIGIN_IS_FULL_SPEC so normally only two completely identical mailbox:// URIs are considered same-origin.  Is the URI of the message also mailbox://stuff?  Is that what its principal is based on?
Thank you for your answer, Boris. As you know, I'm not a specialist in that field, but I'll try to dig out some answers to your questions. BTW, the URI we replace for the cid: reference is shown in comment #8:
  mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Trash?number=7&part=1.2
So we reach into a message part via the number of the message in the mailbox and the part number in the message. Of course the scheme will be different for imap or news messages.

You're asking whether we can identify the message via the URI. Yes, we can:
Message: mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Trash?number=7
Image:   mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Trash?number=7&part=1.2

For our three protocols mailbox, imap and nntp(news) we indeed set ORIGIN_IS_FULL_SPEC:
https://dxr.mozilla.org/comm-central/search?q=ORIGIN_IS_FULL_SPEC&redirect=false

That doesn't appear to be correct, or is it? However, I removed this line
https://dxr.mozilla.org/comm-central/rev/d03f6d1f69374c85d4939d0c7cb92021614600f4/mailnews/local/src/nsMailboxService.cpp#537
and the image still doesn't show.

If I read your comment correctly, our code should provide a QueryInterface to nsIURIWithPrincipal? Since there isn't any textual occurrence of 'nsIURIWithPrincipal' in all of C-C, we don't appear to have that. Therefore I can't recall ever having seen code using this class. Is the inheritance model documented somewhere? [1]

As for the last set of questions: You're asking about the principle of all those mailnews/mailbox URIs. Sadly I don't know. Looking for 'principal' in nsMsgMailNewsUrl.cpp and nsMailboxUrl.cpp there isn't any match. I can tell you that the constructor of nsMsgMailNewsUrl (from which nsMailboxUrl inherits) only does this:
  m_baseURL = do_CreateInstance(NS_STANDARDURL_CONTRACTID);

[1] Sorry, https://dxr.mozilla.org/mozilla-central/search?q=nsIURIWithPrincipal doesn't give me a great deal of information. Sorry also for the ignorance, yesterday I was fighting with INTL issues (date/time formatting in JS and C++) and today this :-(
> That doesn't appear to be correct, or is it?

It's correct.  Otherwise two messages could end up same-origin, which is not what you want.

> If I read your comment correctly, our code should provide a QueryInterface to nsIURIWithPrincipal?

I think that's the simplest approach to solving this at least, yes.

> we don't appear to have that.

Sure.  nsIURIWithPrincipal was create for blob: URIs, which postdate all this comm-central code.  ;)

> Is the inheritance model documented somewhere?

It's just an interface you would implement on your URI class.  So inherit from it in addition to whatever else you already inherit from, then QI to it.  Then implement GetPrincipal.

> Sadly I don't know.

The way to find out is to breakpoint on the various failure returns from CheckMayLoad and see what things look like when your breakpoint is hit.
Flags: needinfo?(mkmelin+mozilla)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Hi Boris, I did what you said, well, I didn't understand one part:
> So inherit from it in addition to whatever else you already
> inherit from,
Sure, done.

> then QI to it.
How, why, when?

> Then implement GetPrincipal.
Sure, done.

With this patch, the images show. Please accept my apologies again, I don't have a clue how I should create a principal for those mailnews URLs. I tried a null principal, with the result that no images show, and then the other principal I know, the system principal, and there the images show. Pretty much everything in TB runs on one of those two (as you know from previous discussions).

Oh, Boris is not accepting feedback requests, so it needs to be an NI. And I just hope I have a decent commit message ;-)
Flags: needinfo?(bzbarsky)
Attachment #8848788 - Flags: review?(mkmelin+mozilla)
Didn't test this, but doesn't it give message content privileges to read say chrome uris it shouldn't, and file uris?
Comment on attachment 8848788 [details] [diff] [review]
1347598-hotmail-images.patch (v1).

(In reply to Magnus Melin from comment #14)
> Didn't test this, but doesn't it give message content privileges to read say
> chrome uris it shouldn't, and file uris?
Damn, yes, it does, I tested with a file URL. So I think Boris needs to help me here. What principal should I invent for those mailnews URLs?
Attachment #8848788 - Flags: review?(mkmelin+mozilla)
If things get sent down to nsMsgContentPolicy.cpp you could check and ignore the attribute there. If not, when doing the cid -> mailbox conversion you could remove the attribute. Unless there's a solution with a principal. But in general, we don't want to increase the privileges mailbox uris have.
(In reply to Magnus Melin from comment #16)
> If things get sent down to nsMsgContentPolicy.cpp you could check and ignore
> the attribute there.
Testing this now, I can see that ShouldLoad() is called:
=== nsMsgContentPolicy::ShouldLoad: URI=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Crossorigin?number=1&part=1.2|
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
=== nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Crossorigin?number=1|
=== returning (4) 1
=== BasePrincipal::CheckMayLoad: URI=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Crossorigin?number=1&part=1.2|
=== BasePrincipal::CheckMayLoad: exit with NS_ERROR_DOM_BAD_URI

"=== returning (4) 1" comes from this debug I added:
  if (IsExposedProtocol(aContentLocation))
  {
    *aDecision = nsIContentPolicy::ACCEPT;
    printf("=== returning (4) %d\n", (int) *aDecision); return NS_OK;
  }

So despite returning 1 (ACCEPT) from ShouldLoad() for the image, the image doesn't load. I assume that's what Boris was saying. Without the mailnews URLs having a principal, some other part of the system (CheckMayLoad(), see above) will block it.

> If not, when doing the cid -> mailbox conversion you could remove the attribute.
Yes, that would be the MIME approach mentioned in comment #8.

> Unless there's a solution with a principal.
That's Boris idea. And I'd appreciate further assistance as to what that principal should be.

> But in general, we don't want to increase the privileges mailbox uris have.
Sure.
>> then QI to it.
> How, why, when?

I meant "implement the QueryInterface bits so that other code can QI you to that interface".

> I don't have a clue how I should create a principal for those mailnews URLs.

The key part is that you want the principal for the attachment to be same-origin with the principal for the message.

Given that the origin for mailbox: URLs is the full spec, the simplest thing here, I think, is to make sure they're both nsPrincipals for the same exact mailbox: URL.

How you do that depends on the structure of these mailbox: URLs, which I'm not very familiar with.  But it looked to me, based on the bits pasted above, that the URL pointing to the embedded part can be used to figure out the URL of the message itself.  Then you'd presumably create a codebase principal for that URL.

You _could_ do a nullprincipal, but you'd need to ensure that it was the same nullprincipal for the message and all the parts.  That sounds hard.  It would also change the principal of the message from what it is right now, which is probably not desirable.

You absolutely do not want to give all this stuff system principals.  ;)
Flags: needinfo?(bzbarsky)
Thanks for the reply, Boris.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #18)
> I meant "implement the QueryInterface bits so that other code can QI you to
> that interface".
I still don't understand that. What is there to implement? This comes for free with all the declarative magic that I already have in the patch, right?
+class NS_MSG_BASE nsMsgMailNewsUrl : public nsIMsgMailNewsUrl,
+                                     public nsIURIWithPrincipal
 {
 public:
     nsMsgMailNewsUrl();
 
     NS_DECL_THREADSAFE_ISUPPORTS
     NS_DECL_NSIMSGMAILNEWSURL
     NS_DECL_NSIURI
     NS_DECL_NSIURL
+    NS_DECL_NSIURIWITHPRINCIPAL

I've just coded:
  nsCOMPtr<nsIMsgMailNewsUrl> mailnewsUrl = do_QueryInterface(m_runningUrl);
  nsCOMPtr<nsIURIWithPrincipal> u = do_QueryInterface(mailnewsUrl);
somewhere and that compiles and links. So that QI is magically provided.

Thanks for the rest of the reply, I'll be looking at creating code based principals as you suggested.
> What is there to implement?

The actual "yes, I implement that interface if you ask me via QI" bit.

> This comes for free with all the declarative magic that I already have in the patch, right

Well, not with the part you quoted, no.  But you _also_ change the "NS_IMPL_ISUPPORTS" line in nsMsgMailNewsUrl.cpp to include the new interface, which is the relevant bit to make it actually work.

> somewhere and that compiles and links

If you had not changed NS_IMPL_ISUPPORTS in the patch, it would compile and link, and "u" would always end up null at runtime.
Thanks a million!
OK, here now with a code-based principal.

I wasn't sure whether to create a new principal every time or store it in a member variable. Also, I don't know what GetPrincipalUri() is for. I have two implementations, one commented out and one in effect. That method isn't called.

Yes, I should remove the |&part=1.2| bit from the spec before creating the principal so that image (part) and message have the same origin. But so far that doesn't seem to be the problem.

The problem is that nsMsgMailNewsUrl::GetPrincipal() gets called a million times and the program hangs in an endless loop until I kill it. This debug repeats forever:

=== nsMsgMailNewsUrl::GetPrincipal: mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Crossorigin?number=1

There isn't even a call for the mailbox: URL with the image part. There was no endless loop when returning a null principal or a system principal, so somehow the caller doesn't like the code principal.

What's even harder to understand is that I see the debug again and again. If called with the same mailbox: URL object, the principal should be supplied from m_principal. So therefore this seems to be called with different mailbox: URL objects, but all with the same spec. Weird.

Boris, can you see what I'm doing obviously wrong?

So funny, when I started two years ago I was pestering Ehsan with editor questions and now I'm pestering Boris with docshell and security questions.
Attachment #8848788 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
CreateCodebasePrincipalFromOrigin expects to be passed a string that was created from a principal's GetOrigin.  Passing it a random URI may not do the right thing.  You want to use CreateCodebasePrincipal, no?

> But so far that doesn't seem to be the problem.

That's ... very odd.  What spec strings are coming through here?

> The problem is that nsMsgMailNewsUrl::GetPrincipal() gets called a million times and the program hangs
> in an endless loop until I kill it.

Presumably something is trying to dig down to some "real" URI and so returning a principal whose URI QIs to "uri with principal" doesn't do the right thing.

Ideally, you'd only do the "uri with principal" bit for the attachments, not the main message itself.  I expect that would solve your problem.

> Also, I don't know what GetPrincipalUri() is for.

It's just a shortcut for GetPrincipal()->GetURI(), afaict, in the one implementation of it in the tree.
Flags: needinfo?(bzbarsky)
Attached file 1347598-endless-recursion.txt (obsolete) —
I looked at this in the debugger, there is an endless recursion, stack enclosed.

nsMsgMailNewsUrl::GetPrincipal()
  nsScriptSecurityManager::CreateCodebasePrincipalFromOrigin()
    BasePrincipal::CreateCodebasePrincipal()
      BasePrincipal::CreateCodebasePrincipal() // different parameters
        nsMsgMailNewsUrl::GetPrincipal() on a different object.

Hmm.

This is the code inside BasePrincipal::CreateCodebasePrincipal() which causes the recursion:
  // Check whether the URI knows what its principal is supposed to be.
  nsCOMPtr<nsIURIWithPrincipal> uriPrinc = do_QueryInterface(aURI);
  if (uriPrinc) {
    nsCOMPtr<nsIPrincipal> principal;
    uriPrinc->GetPrincipal(getter_AddRefs(principal));

So it we don't use a code-based principal (instead null or system) that endless recursion is not observed.

The problem appears to be that uriPrinc->GetPrincipal() is queried with a different object every time, otherwise it would just return the principal already stored in the object's m_principal member variable.
OK, here is another version calling mozilla::BasePrincipal::CreateCodebasePrincipal() directly from nsMsgMailNewsUrl::GetPrincipal().

This shortens the recursion to:
nsMsgMailNewsUrl::GetPrincipal(nsIPrincipal * * aPrincipal) Line 87	C++
mozilla::BasePrincipal::CreateCodebasePrincipal() Line 646	C++

since BasePrincipal::CreateCodebasePrincipal() QI's the nsIURI to a nsIURIWithPrincipal and then calls GetPrincipal() on that, so the very same function that called it.

This time its the same object. To answer the question from comment #23: The spec is always the same:
mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Crossorigin?number=1

So the problem is that creating a code-based principal QI's the nsIURI to a nsIURIWithPrincipal and then calls GetPrincipal() on that.

Even if I moved this into the constructor, calling the creation of the code-based principal won't work. I will again call GetPrincipal(), I'd have to return nullptr since I don't have the principal yet (since I just want to create it) and then BasePrincipal::CreateCodebasePrincipal() will create and return a null principal here:
  // Check whether the URI knows what its principal is supposed to be.
  nsCOMPtr<nsIURIWithPrincipal> uriPrinc = do_QueryInterface(aURI);
  if (uriPrinc) {
    nsCOMPtr<nsIPrincipal> principal;
    uriPrinc->GetPrincipal(getter_AddRefs(principal));
    if (!principal) {
      return nsNullPrincipal::Create(aAttrs);  <====
    }

So the conclusion seems to be: If the class can be queried to nsIURIWithPrincipal it can never create a code-based principal.

Boris, is this intentional?
Flags: needinfo?(bzbarsky)
> So the problem is that creating a code-based principal QI's the nsIURI to a nsIURIWithPrincipal
> and then calls GetPrincipal() on that.

Yes, that's pretty obvious from the stacks above.

> Boris, is this intentional?

Yes.  The idea is that you use nsIURIWithPrincipal to implement URIs that delegate to some other URI for their security stuff.

Quoting myself in comment 23:

> ideally, you'd only do the "uri with principal" bit for the attachments, not the main message itself

As in, the main message URI should _not_ QI to nsIURIWithPrincipal.
Flags: needinfo?(bzbarsky)
Note that at that point doing this in the conversion code might be simpler...
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #26)
> Quoting myself in comment 23:
> > ideally, you'd only do the "uri with principal" bit for the attachments, not the main message itself
> As in, the main message URI should _not_ QI to nsIURIWithPrincipal.
Of course I read your comment ;-)
However, message URL and message part URLs are the same class and I can't introduce a different class for the latter.

One thing I can do is to implement the QI myself instead of using the magic, but I wouldn't know how. When I get a mailbox: URL for a message, I query that to null, so this block here:
  nsCOMPtr<nsIURIWithPrincipal> uriPrinc = do_QueryInterface(aURI);
  if (uriPrinc) {
won't run and I'd get my code based principal created.

For message parts, I query to the URI of the owning message and then GetPrincipal() will automatically return the principal we need. That's doing what you suggested in a hacky way. What do you think?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #27)
> Note that at that point doing this in the conversion code might be simpler...
Yes, if the previous idea doesn't fly.
(In reply to Jorg K (GMT+1) from comment #28)
> One thing I can do is to implement the QI myself instead of using the magic,
> but I wouldn't know how. When I get a mailbox: URL for a message, I query
> that to null, ...
> For message parts, I query to the URI of the owning message and then
> GetPrincipal() will automatically return the principal we need. That's doing
> what you suggested in a hacky way. What do you think?

I looked at this a bit. Something like:

// NS_IMPL_ISUPPORTS(nsMsgMailNewsUrl, nsIMsgMailNewsUrl, nsIURL, nsIURI,
//                  nsIURIWithPrincipal)
NS_IMPL_ADDREF(nsMsgMailNewsUrl)
NS_IMPL_RELEASE(nsMsgMailNewsUrl)

// We need to implement QueryInterface ourselves since we want to do something
// special when querying to a nsIURIWithPrincipal: Full messages are queried to nullptr,
// parts are queried so we can return the principal of the owning message in GetPrincipal().
NS_IMETHODIMP nsMsgMailNewsUrl::QueryInterface(REFNSIID aIID, void** aInstancePtr)
{
  return NS_OK;
}

Of course I'd have to write the code for QueryInterface(), but this compiles and links.

What do you think?
Flags: needinfo?(bzbarsky)
I think I'll try writing nsMsgMailNewsUrl::QueryInterface(). We're implementing a few QI functions "manually", so why not here. Clearing NI for the time being.
Flags: needinfo?(bzbarsky)
> However, message URL and message part URLs are the same class and I can't introduce a different class for the latter.

That's true.  See the NS_INTERFACE_MAP_ENTRY_CONDITIONAL macro and its uses, for cases when you want different instances of the same class to have different QI behavior and slightly nicer code than just totally hand-writing QI.
OK, here is the new approach. QI done by hand, it's not a lot of code. Thanks for the suggestion to use a macro, but coding it up seemed straight forward.

mailbox: URL which address a full message can't be queried to nsIURIWithPrincipal, those which contain a part can.

This is what I observe:
QI is only ever called for mailbox: URLs which address the whole message, there isn't a call for a part URL. Consequently, GetPrincipal() is also not called, since nothing got queried to a part URL.

So the "just works" bit from comment #10 (quote)
> ... making the URI QI to nsIURIWithPrincipal and having the
> principal it hands out be the message's principal would make
> things Just Work....
hasn't arrived yet ;-(

Note since the entire message can't be queried to nsIURIWithPrincipal it also can't hand out a principal.

Other debug shows:
=== BasePrincipal::CheckMayLoad: URI=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Crossorigin?number=1&part=1.2|
=== BasePrincipal::CheckMayLoad: exit with NS_ERROR_DOM_BAD_URI

So question time again ;-)

In hindsight I'm a bit surprised about the:
> ideally, you'd only do the "uri with principal" bit for the attachments,
> not the main message itself
and
> As in, the main message URI should _not_ QI to nsIURIWithPrincipal.
That's exactly what I'm doing, but how will he "same origin" principal be applied if the message (owner of the part) doesn't return a principle. Comment #10 talked about the "message's principal", but it doesn't have one.
Attachment #8849259 - Attachment is obsolete: true
Attachment #8849306 - Attachment is obsolete: true
Attachment #8849327 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
> Thanks for the suggestion to use a macro, but coding it up seemed straight forward.

Please use the macro.  There are plans to change how QI works, and the code using macros will Just Work in the new setup, while hand-coded things will need to be hand-fixed.

Other comments on the code:

1)  You're still calling CreateCodebasePrincipalFromOrigin.  You do not want to be doing this, see above.

2)  Your GetPrincipalUri will crash if GetPrincipal hasn't been called already.

I can't comment on the various string manipulation bits.

> QI is only ever called for mailbox: URLs which address the whole message, there isn't a call for a part URL
>=== BasePrincipal::CheckMayLoad: URI=|mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Crossorigin?number=1&part=1.2|

OK, so CheckMayLoad is called.  What is the type of "this" at that point?  Is it an nsPrincipal?  If so, we should immediately end up in nsPrincipal::MayLoadInternal which immediately does:

  nsCOMPtr<nsIURIWithPrincipal> uriWithPrin = do_QueryInterface(aURI);

which should be doing a QI for your part URI.  Anyway, what does Kind() return at the top of that CheckMayLoad call?

> but how will he "same origin" principal be applied if the message (owner of the part) doesn't return a principle.

It will then get a codebase principal for its URI automatically.  See nsScriptSecurityManager::GetChannelResultPrincipal which is called for all document loads.

> Comment #10 talked about the "message's principal", but it doesn't have one.

It has one, trust me.  You're just not providing it yourself.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #33)
> 1)  You're still calling CreateCodebasePrincipalFromOrigin.  You do not want
> to be doing this, see above.
Here I don't follow you. Which "above" and "why"? I can short-circuit it with what I had in one of the patches:
m_principal = mozilla::BasePrincipal::CreateCodebasePrincipal(this, attrs);

But CreateCodebasePrincipalFromOrigin() calls down into mozilla::BasePrincipal::CreateCodebasePrincipal() in the end. It wasn't working due to the recursion which is solved now.

I'll get the debugging results from nsPrincipal::MayLoadInternal() now.
> Which "above" and "why"?

The "above" where I said:

  CreateCodebasePrincipalFromOrigin expects to be passed a string that was created from a principal's
  GetOrigin.  Passing it a random URI may not do the right thing.  You want to use
  CreateCodebasePrincipal, no?

> m_principal = mozilla::BasePrincipal::CreateCodebasePrincipal(this, attrs);

Yes, that is what you should do.

> But CreateCodebasePrincipalFromOrigin() calls down into mozilla::BasePrincipal::CreateCodebasePrincipal()

A different overload.  One which expects its input string to be something that is NOT a URI.
I've done some debugging in nsPrincipal::MayLoadInternal(). Turned out that the URI passed in is a nsMailboxUrl and not a nsMsgMailNewsUrl, so it's a class derived from nsMsgMailNewsUrl where I implemented that QI.

So apparently that derived class can't be queried to nsIURIWithPrincipal.
This addresses a few of the issues raise by Boris:
1) Macro NS_INTERFACE_MAP_ENTRY_CONDITIONAL used.
2) Principle created with mozilla::BasePrincipal::CreateCodebasePrincipal()
3) nsMsgMailNewsUrl::GetPrincipalUri() won't crash.

Also, nsMsgMailNewsUrl::GetPrincipal() now gets called for the part. Sadly now the endless recursion is back since BasePrincipal::CreateCodebasePrincipal() does cast to a nsIURIWithPrincipal and then asks again.
Attachment #8849568 - Attachment is obsolete: true
Victory!

This finally "just works" as Boris promised it.

I had to use secMan->CreateCodebasePrincipalFromOrigin(spec, ...) since the whole point was to trigger the creation of a principal with a full-message origin, so that the URI *cannot* be queried to a nsIURIWithPrincipal. Otherwise we get the endless recursion.

So with this patch, the image with crossorigin="anonymous" finally shows.

Boris, could you please look over the patch.
Attachment #8849747 - Attachment is obsolete: true
OK, it can be done with
  mozilla::BasePrincipal::CreateCodebasePrincipal(spec);

Boris, this one should be good to look over.
Attachment #8849756 - Attachment is obsolete: true
Sorry about the noise, made the comment match the code.
Attachment #8849761 - Attachment is obsolete: true
> So apparently that derived class can't be queried to nsIURIWithPrincipal.

Whyever not?  It's QI impl ends with NS_INTERFACE_MAP_END_INHERITING(nsMsgMailNewsUrl), so it should work fine.

> Sadly now the endless recursion is back since BasePrincipal::CreateCodebasePrincipal() does cast
> to a nsIURIWithPrincipal and then asks again.

The point is the codebase URI you pass in there should not QI to nsIURIWithPrincipal.  You were passing the original URI of the "part", not the URI of the message, and you wanted the latter. Looks like you got that sorted out.

> since the whole point was to trigger the creation of a principal with a full-message origin

A URI spec is not an "origin" in the sense of CreateCodebasePrincipalFromOrigin.  You should not be calling CreateCodebasePrincipalFromOrigin in this patch, period.

Neither should you be calling the string-argument overload of BasePrincipal::CreateCodebasePrincipal.  It takes an _origin_, not a _URL_ as an argument.  You're passing a URL.

You probably want to NS_NewURI and pass that.  I can't speak to your origin attributes situation.  Passing empty ones is likely fine.

I also can't speak to the correctness of the various string manipulations in nsMsgMailNewsUrl::GetPrincipal.  The printf there should probably go away.

In nsMsgMailNewsUrl::GetPrincipalUri you still crash if CreateCodebasePrincipal returns null.  Which in theory it can in various cases.

The comment on the QI impl should probably say something like:

// We want part URLs to QI to nsIURIWithPrincipal so we can make 
// them same-origin with the owning message.  Messages URLs should
// not QI to nsIURIWithPrincipal, though.

or something.
Boris, thank you so much for your help. This patch addresses all your comments.

I sometimes leave print statements in the code so the reviewer can see what's going on, they are of course removed when landing the patch.

The string manipulations are fine, I strip all known query parts from the URL, we do this elsewhere, too:
https://dxr.mozilla.org/comm-central/rev/1df067640ceebf58b01ab1705b0496c43f732d57/mailnews/imap/src/nsImapProtocol.cpp#9244

> > So apparently that derived class can't be queried to nsIURIWithPrincipal.
> Whyever not?  It's QI impl ends with NS_INTERFACE_MAP_END_INHERITING(nsMsgMailNewsUrl),
> so it should work fine.
I guess there was a problem with the hand-written QI? After switching to the macros, it started working. I haven't looked into it further.

Maybe you could stick an f+ onto it as indication for the reviewer. Thank you so much again. Without you, we never would have implemented this fine solution and would be forever digging around in the MIME code to strip the attribute.
Attachment #8849767 - Attachment is obsolete: true
Comment on attachment 8849905 [details] [diff] [review]
1347598-hotmail-images.patch (v8).

I only need one TB reviewer ;-)

Boris, perhaps you could assure the TB guys that the security bits are correct now so that they only have to check the string manipulation for the URL.
Flags: needinfo?(bzbarsky)
Attachment #8849905 - Flags: review?(rkent)
Attachment #8849905 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8849905 [details] [diff] [review]
1347598-hotmail-images.patch (v8).

>+  m_partURL = false;

I'd call it m_isPartURL, but that's just me.

>+NS_IMETHODIMP nsMsgMailNewsUrl::GetPrincipal(nsIPrincipal **aPrincipal)

Worth asserting that m_isPartURL.

>+  nsCOMPtr<nsIURI> uri;
>+  m_principal->GetURI(getter_AddRefs(uri));
>+  uri.forget(aPrincipalUri);
>+  return NS_OK;

  return m_principal->GetURI(aPrincipalURI);

seems better.

Looks good to me if the string-munging bits are OK.
Flags: needinfo?(bzbarsky)
Attachment #8849905 - Flags: feedback+
Boris, thank you so much for your comments, I've incorporated *all* of them, even the change of variable name, since yours was better (which is sometimes easier to see with some distance).

Carrying forward Boris' f+.
Attachment #8849905 - Attachment is obsolete: true
Attachment #8849905 - Flags: review?(rkent)
Attachment #8849905 - Flags: review?(mkmelin+mozilla)
Attachment #8850456 - Flags: review?(rkent)
Attachment #8850456 - Flags: review?(mkmelin+mozilla)
Attachment #8850456 - Flags: feedback+
Just to summarise my discussions with Kent on IRC:

Due to the attribute "crossorigin", M-C takes use to BasePrincipal::CheckMayLoad() for the part URL. To make this function succeed (and not fail, see comment #17), this approach provides a code-based principal for the part URL with the same URI that the full message uses, basically: protocol://server/folder?number=key. The full message is already given such a code-based principal (comment #33, last line) outside our control.

So the only problem is to derive the full message URL from the part URL. I do this be stripping all known "query additions". Alternatively one could extract the ?number=key, strip the entire query and add only the extracted query.

Something similar was done here for [?|&]part=:
https://dxr.mozilla.org/comm-central/rev/1df067640ceebf58b01ab1705b0496c43f732d57/mailnews/imap/src/nsImapProtocol.cpp#9244
(In reply to Jorg K (GMT+1) from comment #46)
> Alternatively one could extract the ?number=key, strip the entire
> query and add only the extracted query.
Actually, that won't work for IMAP:
imap://jorgk%40jorgk%2Ecom@someserver.de:143/fetch%3EUID%3E.INBOX.HUHU%3E7?part=1.2
Or nicer:
imap://jorgk@jorgk.com@someserver.de:143/fetch>UID>.INBOX.HUHU>7?part=1.2

I noticed that the patch doesn't work for a message opened from a .eml file. There the part URL is:
mailbox:///D:/Desktop/sample_2.eml?number=0&part=1.2
and I bet the message URL is just mailbox:///D:/Desktop/sample_2.eml.
Nope, not quite. Looks like for saved messages we get:
Full message:
mailbox:///D:/Desktop/sample_2.eml?type=application/x-message-display&number=0
Part:
mailbox:///D:/Desktop/sample_2.eml?number=0&part=1.2
So stripping alone doesn't get us there.
(In reply to Jorg K (GMT+1) from comment #48)
> Nope, not quite. Looks like for saved messages we get:
> Full message:
> mailbox:///D:/Desktop/sample_2.eml?type=application/x-message-
> display&number=0
> Part:
> mailbox:///D:/Desktop/sample_2.eml?number=0&part=1.2
> So stripping alone doesn't get us there.

Yes that is the problem I was looking for. And I also cannot get the .eml version to work.

Where are you getting the full message URL? And I still cannot figure out how nsIPrincipal::Subsumes(uriPrin) is supposed to work, I don't understand the C++. Seems like that returns the default null implementation, which is always false, so I am missing something.
> And I still cannot figure out how nsIPrincipal::Subsumes(uriPrin) is supposed to work

It basically checks for this == uriPrin, and if not calls BasePrincipal::Subsumes which starts doing the real work.  It's defined via the  DECL_FAST_INLINE_HELPER(Subsumes) bit, which lives at http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/caps/nsIPrincipal.idl#24

Anyway, once you're in BasePrincipal::Subsumes, if they're both ContentPrincipals for the same URI, with no interesting OriginAttributes going on, that will return true.
(In reply to Kent James (:rkent) from comment #49)
> Yes that is the problem I was looking for.
OK, so message from files don't work yet. I can fix that somehow.

> And I also cannot get the .eml version to work.
Meaning you want to improve the patch?
 
> Where are you getting the full message URL?
I don't understand the question. I'm not getting it anywhere. I'm in the code that manipulates it, nsMsgMailNewsUrl.cpp, other code prepares that spec, for example
  imap://jorgk@jorgk.com@someserver.de:143/fetch>UID>.INBOX.HUHU>7?part=1.2
for IMAP.

As for the funny type=application/x-message-display, I can't see where it's added in a hurry
https://dxr.mozilla.org/comm-central/search?q=type%3Dapplication%2Fx-message-display&redirect=false
but I see many call sites which search for it and strip it, for example here:
https://dxr.mozilla.org/comm-central/rev/1df067640ceebf58b01ab1705b0496c43f732d57/mailnews/base/src/nsMessenger.cpp#669
There you also find out friends /;section and ?section ;-)

> And I still cannot figure out how nsIPrincipal::Subsumes(uriPrin) is supposed to work, ...
Amazing that Boris, "still a bit busy", is watching us from the distance - again, thanks a million for your continued help!
(In reply to Jorg K (GMT+1) from comment #51)
> I can't see where it's added in a hurry ...
Here?
https://dxr.mozilla.org/comm-central/rev/1df067640ceebf58b01ab1705b0496c43f732d57/mail/base/content/mailWindowOverlay.js#2250
before opening the window.
Fixed case of message opened from file. Use interdiff to see added lines:
+    // Finally, for messages opened from a file, add the
+    // type=application/x-message-display. Nice hack.
+    MsgReplaceSubstring(spec, "?number=0", "?type=application/x-message-display&number=0");
Attachment #8850456 - Attachment is obsolete: true
Attachment #8850456 - Flags: review?(rkent)
Attachment #8850456 - Flags: review?(mkmelin+mozilla)
Attachment #8851204 - Flags: review?(rkent)
Comment on attachment 8851204 [details] [diff] [review]
1347598-hotmail-images.patch (v9).

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

This whole patch is a gigantic kludge, or as you say "Nice hack". This whole concept of the full URI being the principal does not match how we actually use a message URI, which is to add processing or metadata attributes on as query items.

I really hate to make a problematic situation worse. May I suggest a variation that might move us in the right direction at least? Instead of using m_isPartURL as a flag for the QI to nsIURIWithPrincipal, let's introduce the concept of a canonical message principal URI, which is a URI in the form that represents the message principal. That form would be folderURI?number=X. If the URI is in that form, it does not QI to nsIURIWithPrincipal. Otherwise, you parse the URI to get the folder and number, and return a principal in the canonical URI format. That would get rid of all of the special-case attribute stripping and adding, and make this less prone to regressions when we modify one of the query attributes. So instead of m_isPartURL, you could have m_isCanonicalPrincipalURI.

Would you be willing to try that approach and see if it works?
I'm not sure I understand your comment.

Let me summarise the task at hand:

We have various types of message URLs. Typically a part is addressed by ?part=m.n or &part=m.n:
IMAP:
imap://jorgk@jorgk.com@someserver.de:143/fetch>UID>.INBOX.HUHU>7
IMAP part:
imap://jorgk@jorgk.com@someserver.de:143/fetch>UID>.INBOX.HUHU>7?part=1.2

Mailbox:
mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Trash?number=7
Mailbox part:
mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Trash?number=7&part=1.2

File:
mailbox:///D:/Desktop/sample_2.eml?type=application/x-message-display&number=0
mailbox:///D:/Desktop/sample_2.eml?number=0&part=1.2

News:
news://news.mozilla.org:119/?group=xx&key=yy ... (haven't looked how it's done).

JS-Account:
??

The task is to generate the message URL from the part URL. The message URL is fixed and cemented in in other parts of the system, so as much as I hate "mailbox:///D:/Desktop/sample_2.eml?type=application/x-message-display&number=0", I can't change that.

As you can see from IMAP, the canonical form is *not* folderURI?number=X.

So surely, taking the part URL, I can look at what scheme/protocol I'm dealing with (although "file" can only be detected from the "number=0". Then, depending on the protocol, I can construct a "canonical" from, and from that "canonical" from I can derive the message URL. In fact, it wouldn't be done this way, but each protocol's URL would have to implement GetPrincipal(), so for nsImapUrl, nsMailboxUrl, nsNntpUrl and JaBaseCppUrl.

As I said, I'd have to analyse the various protocols separately, and the "nice hack" of adding "type=application/x-message-display" will not go away since the URL for a message from a saved file has that (I'm repeating myself).

Comparing the two approaches:

JK:
Strip known query parts to get to the "canonical" message URL.
No distinguishing of protocols necessary, can be done centrally.
Messages from files need to be detected and the "nice hack" needs to be applied.

RKJ:
Look at the scheme of the URL. Based on the scheme, parse the URL to
get the scheme-specific "canonical" message URL.
Or more correctly: Implement GetPrincipal() at least four times, each instance does its own parsing.
Messages from files need to be detected and the "nice hack" needs to be applied.

Do you really prefer the latter? I'd have to shift the processing into the various protocols. So I'd need changes in
imap/src/nsImapUrl.cpp
local/src/nsMailboxUrl.cpp
news/src/nsNntpUrl.cpp
jsaccount/src/JaUrl.cpp

While there is nsParseImapMessageURI(), nsParseLocalMessageURI() and nsParseNewsMessageURI() (only returns group and key, so not useful), I haven't see anything similar for JS-Account.

Please let me know what you think.
Flags: needinfo?(rkent)
I guess this is what Kent wanted. Implemented for mailbox:// only for now.

Debug shows:
=== nsMsgMailNewsUrl::GetPrincipal: Org spec: mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/AAA%20Crossorigin?number=1
=== nsMsgMailNewsUrl::GetPrincipal: New spec: mailbox://nobody@Local%20Folders/AAA%20Crossorigin?number=1&principal

So indeed, I create a "normalised" URL. Sadly, nothing works from then onwards. I see no debugging for getting the principal of the part, but worse:
[1452] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file c:/mozilla-source/comm-central/mailnews/local/src/nsMailboxUrl.cpp, line 386
That's in nsresult nsMailboxUrl::ParseUrl() where it parses the URL to get the file name/path. And since this seems to be "nobody@Local%20Folders/AAA%20Crossorigin" which doesn't exist, that's of course a problem.

So apparently the normalised form needs to be pretty close to the original and we can't just invent any old format.
Attachment #8853563 - Flags: feedback?(rkent)
Since normalising didn't work, I'm just using the old hand-made clean-up for now. Still, no nsMsgMailNewsUrl::GetPrincipal() for the part URL and hence no image shows.
Right now, we can't work on this bug since *ALL* embedded (cid:) images are broken due to bug 1351966.

Yes, I landed bug 968342 today which also tightens content policy, but backing this out, embedded images still doesn't show. Besides, I tested that before bug 1351966 (caused by bug 1340710) arrived.
I think we can clear NI here now. I've had a long discussion with Kent on IRC and he explained his approach, which is mostly implemented in attachment 8853563 [details] [diff] [review]. Note the copy/paste error there:
+  nsCOMPtr<nsIMsgMailNewsUrl> mailnewsURL;
+  QueryInterface(NS_GET_IID(nsIMsgMessageUrl), getter_AddRefs(mailnewsURL));
That should be:
+  QueryInterface(NS_GET_IID(nsIMsgMailNewsUrl), getter_AddRefs(mailnewsURL));

Parsing the URL in the subclasses has the advantage that the subclass knows better what needs to be done. For messages from files we can return, for example, mailbox:///D:/Desktop/sample_2.eml?number=0.

We'll have to see how "normalised" we can make the URLs, since we saw in comment #56 that "inventing" a greatly changed format, for example:
mailbox://nobody@Local%20Folders/AAA%20Crossorigin?number=1
doesn't seem to work.

Of course not seeing any embedded cid: images due to bug 1351966 doesn't help proceedings here.
Flags: needinfo?(rkent)
This implements what Kent had in mind. It works for IMAP and NNTP, I can't try JS-Account.

It also works for mailbox:// since providing the principal for the parts magically resolves bug 1352701.

What still needs to be done is to improve the four GetPrincipalSpec() functions.
Attachment #8851204 - Attachment is obsolete: true
Attachment #8853563 - Attachment is obsolete: true
Attachment #8853571 - Attachment is obsolete: true
Attachment #8851204 - Flags: review?(rkent)
Attachment #8853563 - Flags: feedback?(rkent)
Attachment #8853728 - Flags: feedback?(rkent)
(In reply to Jorg K (GMT+2) from comment #60)
> It also works for mailbox:// since providing the principal for the parts
> magically resolves bug 1352701.
Oh nonsense, I forgot that I had backed out some M-C stuff locally to work around bug 1352701.
So this should read:
Once bug 1352701 is fixed, this will also work for mailbox://. Anyway, the images with crossorigin="anonymous" now show everywhere.

Repeating: What still needs to be done is to improve the four GetPrincipalSpec() functions.
Here is what you asked for, ready, as promised, for a weekend review ;-)

You'll be pleased that the hacks are gone. Especially the URL used for a message opened from a file is now:
Org: mailbox:///D:/Desktop/Mozilla%20sort/sample.eml?type=application/x-message-display&number=0
New: mailbox:///D:/Desktop/Mozilla%20sort/sample.eml?number=0&principal

Happy?
Attachment #8853728 - Attachment is obsolete: true
Attachment #8853728 - Flags: feedback?(rkent)
Attachment #8855352 - Flags: review?(rkent)
Comment on attachment 8855352 [details] [diff] [review]
WIP: 1347598-hotmail-images.patch - NEW approach (v4).

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

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +107,5 @@
> +      GetSpec(spec);
> +    }
> +
> +    // Append our marker, so we can detect "normalised" principal URLs.
> +    spec += NS_LITERAL_CSTRING("&principal");

Just tacking "&principal" on at the end is not a very nice solution. Besides, should check for parameters and use ? instead if appropriate. And add a value for it, like =true.

Maybe just ensure that it has the number as parameter, and nothing else... but yes I see that won't work for news...

::: mailnews/news/src/nsNntpUrl.cpp
@@ +305,5 @@
> +NS_IMETHODIMP nsNntpUrl::GetPrincipalSpec(nsACString& aPrincipalSpec)
> +{
> +  // URLs look like this:
> +  // news://server:port/folder?group=ggg&key=nnn [ &part=ppp &filename=fff ].
> +  // Just strip the part which will also remove the filename.

maybe true in our code, but just for the record, parameters can occur in all different orders...
(In reply to Magnus Melin from comment #63)
> Maybe just ensure that it has the number as parameter, and nothing else...
> but yes I see that won't work for news...
You mean IMAP, right?
imap://user@domain@server:port/fetch>UID>folder>nn
No |number=| there.

We need a special marker that this is a URL created for a principal, to tacking something to the end is the way to go.

I'll address the other comments now.
OK, pretty much like the previous patch. Now appending
- ?principal=true when there was no ? before, as in IMAP.
- &principal=true if we have ?number= or ?group= before.
Still works the same.

The question is whether Kent wants to improve the "parsing" for NNTP or JS-Account.
Attachment #8855352 - Attachment is obsolete: true
Attachment #8855352 - Flags: review?(rkent)
Attachment #8855474 - Flags: review?(rkent)
Comment on attachment 8855474 [details] [diff] [review]
1347598-hotmail-images.patch - NEW approach (v4b).

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

Please address my comment below, as it affects the whole approach.

::: mailnews/base/util/nsMsgMailNewsUrl.cpp
@@ +412,5 @@
>  
>  NS_IMETHODIMP nsMsgMailNewsUrl::SetSpec(const nsACString &aSpec)
>  {
>    nsAutoCString spec(aSpec);
> +  m_isPrincipalURL = (spec.Find("?principal=") != kNotFound) ||

I don't understand why you need to add this.

Can't you just say (after setting the spec):

nsAutoCString principalSpec;
GetPrincipalSpec(principalSpec)
if (spec.Equals(principalSpec))
  m_isPrincipalURL = true;
I think we agree that m_isPrincipalURL needs to be set for a mailnews URL so the URL knows whether it is used for a principal or not. This is needed in the conditional QI table:

+NS_INTERFACE_MAP_BEGIN(nsMsgMailNewsUrl)
+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIMsgMailNewsUrl)
+  NS_INTERFACE_MAP_ENTRY(nsIMsgMailNewsUrl)
+  NS_INTERFACE_MAP_ENTRY(nsIURL)
+  NS_INTERFACE_MAP_ENTRY(nsIURI)
+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIURIWithPrincipal, !m_isPrincipalURL)
+NS_INTERFACE_MAP_END

So what's the best way to set m_isPrincipalURL? Ideally the CTOR would already set it, but the object gets constructed without the URL passed in. So the next best spot is in nsMsgMailNewsUrl::SetSpec().

Your code doesn't compile, since GetPrincipalSpec() is provided by the subclasses, so you'd have to QI to nsIMsgMessageUrl first. Let's assume we do.

So then you get the "principal spec" again, which seems a little inefficient.

Remember that my original approach was to distinguish a "part URL" from a "whole message URL", where the part URL could be QI'ed to nsIURIWithPrincipal and would return the same code-based principal that the whole message URL was already using based on its spec.

As far as I understood your approach, we wanted to equip every "real world" part or whole message URL with a nice normalised "principal URL", which cannot be QI'ed to nsIURIWithPrincipal (see table above). My "principal URLs" are detected by having the [?|&]principal=true in their spec.

Now you seem to be saying that you want to lose this distinction between "real world URL" and "principal URL". So some reasonably clean "real world" URLs will already be in normalised form, and others need some normalising.

I guess that can work, too. Let me spend 10 minutes on that ;-)
This works, too ;-)
Attachment #8860689 - Flags: review?(rkent)
Fixed logic error in previous version.
Attachment #8860689 - Attachment is obsolete: true
Attachment #8860689 - Flags: review?(rkent)
Attachment #8860711 - Flags: review?(rkent)
Comment on attachment 8860711 [details] [diff] [review]
WIP: 1347598-hotmail-images.patch - NEW approach (v5b).

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

Thanks for your patience, yes this is what makes sense to me. We are explicitly defining what we want to principal URL to be for each type of message URL (since they vary), without adding to the query. I like it!

You'll need to remove any debug output, but otherwise this is fine.

One optional change: you are assuming that the JsAccount URLs do not add additional query items, but in general this is not true. An implementation can fix this in the JS override, but really the mailbox GetPrincipalSpec implementation is better than the default implementation you gave for JsAccount, so I would prefer if you just copied that to JsAccount. Up to you.
Attachment #8860711 - Flags: review?(rkent) → review+
Comment on attachment 8855474 [details] [diff] [review]
1347598-hotmail-images.patch - NEW approach (v4b).

We're not going to use this version IIUC.
Attachment #8855474 - Flags: review?(rkent)
OK, here we go with debug removed and better normalisation for JsAccount.
Attachment #8855474 - Attachment is obsolete: true
Attachment #8860711 - Attachment is obsolete: true
Attachment #8860832 - Flags: review+
Moved query extraction function to nsMsgUtils.cpp.
Attachment #8860832 - Attachment is obsolete: true
Attachment #8860865 - Flags: review+
https://hg.mozilla.org/comm-central/rev/291bc010edbf0fcc00600201fef1da7865555bc3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Attachment #8860865 - Flags: approval-comm-esr52?
Attachment #8860865 - Flags: approval-comm-beta+
Attachment #8847661 - Attachment mime type: application/x-extension-eml → text/plain
People are running into this problem in Thunderbird 52, which is a public, stable release being pushed out the world. Can we get this fix backported, please?
TB 45 is also affected, and apart from you, no one has complained to far. That said, the fix is scheduled for TB 52.2 which will be released in a few weeks.
(In reply to Jorg K (GMT+2) from comment #77)
> apart from you, no one has complained to far.

Two different users of my Send Later add-on complained to me this morning that signatures are not working properly for them.

As far as I can tell, the problems they are having have nothing to do with my add-on per se, but rather have to do with the fact that they are trying to edit drafts previously scheduled with my add-on, and those drafts have images embedded in their signatures.

I am just guessing here, because they haven't provided me with enough detail to be able to confirm, but I think there's a good chance that the problem they are seeing is related to this issue.
(In reply to Jorg K (GMT+2) from comment #77)
> the fix is scheduled for TB 52.2

I don't really fully understand how all this release management stuff works, but I don't see anything about about this fix being landed on a TB52 branch. Am I missing something?
All I can say it this: You filed the bug, we fixed it, and you can see that it wasn't easy. It's now available on TB 54 beta and the next step is to ship it in TB 52.2.

All the fixes that got into TB 52.1 were in TB 53 beta, and what ever went into TB 54 beta will go to TB 52.2. It's not forgotten. I uplifted 19 bugs this morning and this one will be next.
Thanks. Didn't mean to criticize, just trying to understand how it all works.
Attachment #8860865 - Flags: approval-comm-esr52? → approval-comm-esr52+
TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/22543bf7440283d7482dabf33073b38a5d86ae9b

Patch needed some massaging to apply, let's see whether it works ;-)

> ... just trying to understand how it all works.
Yep, release management is white-man's magic. To say the least, I've had so much discussion with the release/code manager for TB 45.x that they got sick of me and made me the manager for 52.x ;-). That's why I also get to write the release notes these days.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: