Closed Bug 1079280 Opened 7 years ago Closed 3 years ago

Thunderbird does not handle a possible "NO / BAD" answer to a non-literal+ append command

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 63.0

People

(Reporter: thomas.cataldo, Assigned: thomas.cataldo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [patchlove])

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.101 Safari/537.36

Steps to reproduce:

Tried to append a Draft with a big (bigger than a configured server ratio) attachment, the server refuses with <tag> NO message is too big.


Actual results:

Thunderbird pops a notification with the "message is too big" text, but proceeds with the literal upload. Of course the server parser is not expecting a literal at this point.


Expected results:

The upload should have been aborted.
Tb's behavior when NO to append, when BAD to fllowing data send, is knwn in Bug 693353.
Setting dependncy to this bug.
Blocks: 693353
Thanks for the link to the other bug. I don't know how I could speed up the review process for my patch.
Attachment #8501085 - Attachment mime type: text/x-patch → text/plain
Status: UNCONFIRMED → NEW
Ever confirmed: true
Put review flag on patch and ask one of peers/owners of thunderbird module
https://wiki.mozilla.org/Modules/Thunderbird
Attachment #8501085 - Flags: review?(mozilla)
Comment on attachment 8501085 [details] [diff] [review]
Patch for the nsImapProtocol.cpp issue

David is no longer active in reviews
Attachment #8501085 - Flags: review?(mozilla) → review?(mkmelin+mozilla)
Attachment #8501085 - Attachment is patch: true
Assignee: nobody → thomas.cataldo
Comment on attachment 8501085 [details] [diff] [review]
Patch for the nsImapProtocol.cpp issue

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

Passing to Irving. With my very limited knowledge of this, the patch looks ok though
Attachment #8501085 - Flags: review?(mkmelin+mozilla) → review?(irving)
Comment on attachment 8501085 [details] [diff] [review]
Patch for the nsImapProtocol.cpp issue

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

Thanks for the patch. I'll r+ this for landing, but if you're interested it would be great to have an automated test case for this. You can see an example of a test for IMAP append in http://dxr.mozilla.org/comm-central/source/mailnews/imap/test/unit/test_saveImapDraft.js.
Attachment #8501085 - Flags: review?(irving) → review+
Thanks for the review. Would be happy to add a test case but this one requires carefully crafted server responses, eg:
C: A0 login bla bla 
S: A0 OK Complete.
C: A1 APPEND Drafts {6666666666}
S: A1 NO message is too big.

Then ensure that the client does not send anything after that.

I don't really see how to craft this dialog from saveImapDraft.js.
Bug 47311 was found. It looks pretty old problem.
Setting dependency for ease of tracking.
Depends on: 47311
I don't think my patch depends #47311 but it fixes it too.
> I don't really see how to craft this dialog [test] from saveImapDraft.js.

Gene, can you offer advice?
Blocks: 47311
No longer depends on: 47311
Flags: needinfo?(gds)
Flags: in-testsuite?
Whiteboard: [patchlove]
(In reply to Wayne Mery (:wsmwk) from comment #10)
> > I don't really see how to craft this dialog [test] from saveImapDraft.js.
> 
> Gene, can you offer advice?

I haven't looked closely at this yet but I don't think this involves just saving a draft that is too big. I think what this is about is moving a too large message from account A to B or dragging a too large .eml file to a folder so that the imap append command occurs and returns NO or BAD response.

I will try to duplicate this with a large email, maybe 30M or larger depending on server, not sure.

It looks like the attached patch is from v45 so it will also requires some changes to work with the latest.

Making a test for this would probably require changes to the fake server code. Is it worth it?

Another thing not clear to me yet is what problem does the user actually see. Only a pop-up error is mentioned, I think. Don't think tb hangs or crashes when this occurs.
Here's an updated patch. I have tested this with a real imap server (charter) that rejected my appended message as too large. I still see the helpful pop-up error message but, after tb sees the bad append response, it now no longer transmits the full email (e.g., 60Mbytes of data) that was already rejected by the server.

Note: This fix has no effect on the append of a too large message if literal+ capability is supported. In that case, the RFCs suggest that the server send an untagged imap BYE or tcp disconnect to stop the client's email transmission.

Testing with another server supporting literal+ appends (outlook), the full email is transmitted and then the server reports the email is too big in the BYE response and disconnects. Then tb reconnects and tries the append again, which it shouldn't. I don't see an error pop-up in this case at all. The same full email transmission occurs (but twice!) as without this patch.

Actually, the literal+ case looks like a bigger bug! Maybe a more effective fix would be to keep this patch but don't do literal+ when emails are appended regardless of the server's capability. It looks like literal+ only actually saves 1 response message from the server.

Also, I don't see a reason to include a test for this as part of the patch since it would require a change to fake server and this fix is better tested with real servers, like I just did.
Attachment #8999071 - Flags: review?(jorgk)
Comment on attachment 8999071 [details] [diff] [review]
1079280-review-append-fix-v0.patch

Thanks, but this should be attributed to the original author, Thomas Cataldo <thomas.cataldo@blue-mind.net>.
Attachment #8999071 - Flags: review?(jorgk) → review+
Flags: needinfo?(gds)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/069924792281
Don't transmit email when imap append response is bad. r=Irving,GeneSmith DONTBUILD
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Thanks, Gene, I added you as reviewer since you tested this.
Target Milestone: --- → Thunderbird 63.0
Thanks Gene! Would you mind filing a new bug based on your comment 12 (for the cases not covered by the patch landed)?
(In reply to Magnus Melin from comment #16)
> Thanks Gene! Would you mind filing a new bug based on your comment 12 (for
> the cases not covered by the patch landed)?

Well, after testing with my many tb accounts, it appear the the problem I saw in comment 12 really only applies to outlook. Only outlook receives the file up to the maximum append size, flags an error and then disconnects. The disconnect is what causes the lack of an error notification.

All other servers that I tried that support literal+ (yahoo, aol, mdaemon zimbra dovecot ovh, (beta)newCloud) receive the whole message regardless of length, report the error and do not disconnect so the notification occurs. However, I'm not certain that there are not other servers or versions of these servers that have the same problem as outlook.

I have put this in new Bug 1484400.
Duplicate of this bug: 47311
Attachment #8501085 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.