1378290 - Download rest of message not working after fetch headers only or partial download (do not download messages larger than ...) - POP account - take 2

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1361020 +++

In bug 1361020 we introduced a sophisticated scheme to compare the content principal specs of the "Download rest of message" link and the message containing that link:
https://hg.mozilla.org/comm-central/rev/92e89bd8daf85910e04d9c44098ba9279735a8a7#l1.30

It turned out that this check fails, if the OS folder where the mail data is stored contains a "&" character.

See: https://support.mozilla.org/en-US/questions/1158546, posts after July 3rd, 2017.

The SUMO reported ran a special debug version and this is the result (slightly edited):
=== nsMsgContentPolicy::ShouldLoad: URI=|mailbox://[e-mail]/Inbox?number=9531& ...
=== aRequestingLocation = mailbox:///C:/Users/XX/AppData/Ops%20?number=9531
=== nsMsgContentPolicy::ShouldLoad: Request Principal Sys/Code/Null Principal: 0/1/0
=== nsMsgContentPolicy::ShouldLoad: CodebasePrincipal=|mailbox:///C:/Users/XX/AppData/Ops%20?number=9531|
=== content principal spec: |mailbox:///C:/Users/XX/AppData/Ops%20&%20Admin/NS%20Mail/example.com/Inbox?number=9531|
=== request principal spec: |mailbox:///C:/Users/XX/AppData/Ops%20?number=9531|
=== returning (3B) -1 <=== rejection

We can see that user XX has his e-mail store in a directory
C:/Users/XX/AppData/Ops & Admin/NS Mail

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1378290-dont-remove-and.patch (v1).

OK, when we introduced the principal spec, we stripped too much from the URLs. To remove the query part, it should be sufficient to cut after a ?, that will take any further queries with & away.

Cutting from & will destroy file/folder names, at least for mailbox.

To test this:
Send and e-mail to an account which receives headers only (or has some size limit).
For this account, move the received message to a folder called "A&B".
Click on the "Download rest of message" link. Ain't working without the patch.

It's a simple removal, so hopefully easy to review.

Comment 2

[Kent James (:rkent)]

Comment on attachment 8885010 [details] [diff] [review]
1378290-dont-remove-and.patch (v1).

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

Isn't the real problem here that we are are scanning the entire URL string for these characters, rather than only the tail of the URL (after the last slash)? For example, won't you have the same problem if the folder name contains a question mark?

Comment 3

[Jorg K (CEST = GMT+2)]

No. A folder "A ? B" creates this request:
=== aRequestingLocation = mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/A%204f83d0bd?number=1

A folder "A & B" currently creates this request:
=== aRequestingLocation = mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/A%20?number=1

The truncation after & is simply unnecessary.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: content-policy-debug_3.patch - Debug patch

Comment 5

[Kent James (:rkent)]

(In reply to Jorg K (GMT+2) from comment #3)
> No. A folder "A ? B" creates this request:
> === aRequestingLocation =
> mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.
> testing/Mail/Local%20Folders/A%204f83d0bd?number=1
> 
> A folder "A & B" currently creates this request:
> === aRequestingLocation =
> mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.
> testing/Mail/Local%20Folders/A%20?number=1
> 
> The truncation after & is simply unnecessary.

Sorry, I don't understand this.

Isn't "A & B" A%20&%20B

How does "A ? B" become A%204f83d0bd

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #5)
> Isn't "A & B" A%20&%20B
Yes, and it gets truncated: A%20?number=1 (please look closely at the debug).

> How does "A ? B" become A%204f83d0bd
No idea, but it does. (I'm in transit right now and can't research it (and it also doesn't matter)).

Comment 7

[Jorg K (CEST = GMT+2)]

> How does "A ? B" become A%204f83d0bd
I've done some digging in DXR/Searchfox. Apparently the file path as part of the mailbox: URL gets encoded via MsgEscapeURL() in nsMailboxService::PrepareMessageUrl() (and nsMailboxService::ParseMailbox()).

When we construct the principal spec, we also do it here (copied from nsMailboxService):
https://dxr.mozilla.org/comm-central/rev/b8853a304067656d5b43ecf9aeb572475adb09b4/mailnews/local/src/nsMailboxUrl.cpp#160

Without debugging it I'd assume that "?" gets encoded since it's not a valid URL character but "&" doesn't. So when we later come to construct the principal spec we cut through the file/path/folder name.

No one has noticed this since embedded CID images display even when the folder has a "&". In that case the requesting URL for the message and the requested URL for the part/image both get mangled equally. We only notice it for the "download rest of message" link where the requesting URL gets mangled, but the freshly constructed part/link URL, constructed at nsMailboxUrl.cpp#160 doesn't. This code was introduced here:
https://hg.mozilla.org/comm-central/rev/92e89bd8daf85910e04d9c44098ba9279735a8a7#l2.31

Does this answer your question?

Comment 8

[Jorg K (CEST = GMT+2)]

Some debug where from nsMailboxService::PrepareMessageUrl():
=== /C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local Folders/A & B
=== /C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/A%20&%20B
=== /C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local Folders/A 4f83d0bd
=== /C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/A%204f83d0bd
shows that comment #7 was all wrong. Apparently when creating the folder with the "?" it already gets named "A 4f83d0bd" since that's what it's called in the file system.

So we still don't know where the "A 4f83d0bd" comes from. In any case, "?" is not a valid character in a file name and we have some logic somewhere to avoid it in file/path names. So I'd say it's still safe to cut the URL at the "?" but not the "&".

Do you want more research?

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #8)
> Do you want more research?
OK, just for our peace of mind: If you rename a folder, Mork (nsMsgBrkMBoxStore::RenameFolder()) calls  NS_MsgHashIfNecessary():
https://dxr.mozilla.org/comm-central/rev/b8853a304067656d5b43ecf9aeb572475adb09b4/mailnews/base/util/nsMsgUtils.cpp#376
If the name contains
FILE_PATH_SEPARATOR, that is / or \
FILE_ILLEGAL_CHARACTERS, that is "/:*?\"<>|"
ILLEGAL_FOLDER_CHARS, that is ; or #
is will be encoded.

"?" is one of them, so we don't expect "?" in a file/path, so we can safely cut the URL at the "?".
"&" is not one of them, so this can occur as part of a file/path name. If we cut there, which is entirely unnecessary, we create a problem.

The patch removes this cut and the problem. I hope you're convinced now.

Comment 10

[Kent James (:rkent)]

Comment on attachment 8885010 [details] [diff] [review]
1378290-dont-remove-and.patch (v1).

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

OK, let's try this.

What I am still afraid of is that there will be queries that invalidly add &foo=bar without a preceding ?love=hate. Though I suppose those would be bugs, I just have little confidence that our existing code does the right thing. But that is no reason to hold this up.

Comment 11

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/25b49e872ac53975737e857be31f5c676c586007

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8885010 [details] [diff] [review]
1378290-dont-remove-and.patch (v1).

[Approval Request Comment]
Regression caused by (bug #): Bug 1361020.
This needs uplift, but perhaps no rush.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8885010 [details] [diff] [review]
1378290-dont-remove-and.patch (v1).

Not doing another TB 55 beta.

Comment 14

[Jorg K (CEST = GMT+2)]

TB 52.3 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/8719024fe6e7

Comment 15

[jmacauslan]

What does the notation "esr" in the version label denote?  (How will I know whether TB's offer to upgrade my version of itself contains the fix, or doesn't?)

Comment 16

[Jorg K (CEST = GMT+2)]

As comment #14 suggests, this will be fixed in TB 52.3 coming towards the end of the week.