[Tickets #13153] Re: Bugs in the current implementation for PARTIAL limiting (RFC 5267 [4.4]) in Horde's Imap_Client library
noreply at bugs.horde.org
noreply at bugs.horde.org
Thu May 1 11:25:34 UTC 2014
DO NOT REPLY TO THIS MESSAGE. THIS EMAIL ADDRESS IS NOT MONITORED.
Ticket URL: http://bugs.horde.org/ticket/13153
------------------------------------------------------------------------------
Ticket | 13153
Updated By | daniel_bistriceanu at yahoo.com
Summary | Bugs in the current implementation for PARTIAL limiting
| (RFC 5267 [4.4]) in Horde's Imap_Client library
Queue | Horde Framework Packages
Version | Git master
Type | Bug
State | Feedback
Priority | 2. Medium
Milestone |
Patch | 1
Owners | Michael Slusarz
------------------------------------------------------------------------------
daniel_bistriceanu at yahoo.com (2014-05-01 11:25) wrote:
>> To fix this it should just enforce the usage of the provided partial
>> if it is a valid sequence range ('range_start:range_end'), as
>> specified in RFC 5267 [4.4].
>
> This won't work, since 'partial' can be, for example, array(1, 2, 3).
> So we do need to convert to an Ids object first. We just need to
> catch the single case where the range contains a single ID
Why would you even want to allow the ability to specify partial ranges
as anything else but the actual literal representation? It's kind of
pointless to have this array option and allow an array(1, 2, 3) to be
specified here instead of the literal '1:3' because, as stated in RFC
5267 [4.4], the IMAP command accepts only a literal range
representation as a valid PARTIAL. In my opinion the array support
would just add additional overhead for converting back and forth for
no real gains. And it's a lot easier to use the literal representation
anyway, especially when you want to retrieve more than just a few
messages at a time: consider for example specifying '1:100' vs its
array(1,2,3,...,100) counterpart. Not to mention that handling an
array option also requires additional processing to prevent errors for
an edge case like the one reported. For me it seems that this
additional array option just ends up making things more complicated
and less clear; and also less intuitive when for a range like 'x:x' it
would also have to handle the option of array(x).
Partial ranges have nothing to do with message UIDs and I don't think
that trying to handle them the same way just for consistency is the
best idea. In case of specifying a set of message UIDs then it does
indeed make a lot of sense to use an Ids object. In that case you
don't always have a single sequence-set representation and the Ids
class handles things properly because that's what it had been designed
and implemented for, but the same does not apply to partial ranges. A
partial range must always contain exactly one single literal
sequence-set representation, so, like I already said, I don?t really
see the point of allowing the ability to specify it as an array as well.
Also you did not commit any changes to Horde_Imap_Client_Ids so at the
moment your current fix will always set the partials range to ':'
which is clearly invalid:
C: 4 UID SORT RETURN (PARTIAL :) (DATE) US-ASCII UNDELETED
S: 4 BAD Error in IMAP command UID SORT: PARTIAL range broken.
And last but not least, if you're hell bent on allowing this array
option, then you need even more additional processing, to make sure
that you're actually sending a proper partial range. For example
something like array(1,2,3,5,6,7), while valid as far as Ids class is
concerned, would still break your current fix because it does not have
a single sequence-set rage representation and after the conversion it
would get translated from '1:3,5:7'*, which is obviously still not a
valid partial range:
C: 4 UID SORT RETURN (PARTIAL 1:3,5:7) (DATE) US-ASCII UNDELETED
S: 4 BAD Error in IMAP command UID SORT: PARTIAL range broken.
*Assuming here that the 'range_string' property accessed through the
magic method was supposed to return the literal sequence-set
representation for the $_ids stored in the Ids object, similar to
'tostring'.
>> 2. In case there's a 'sort' option the current implementation
>> conditions the PARTIAL limiting to the availability of the 'SORT'
>> capability on the 'CONTEXT', even though it also states just below
>> the check that "RFC 5267 indicates RFC 4466 ESEARCH support,
>> notwithstanding RFC 4731 support."
>> and PARTIAL limiting relies on 'ESEARCH' not 'ESORT' to retrive the
>> message UIDs that correspond to the PARTIAL limiting range that was
>> specified in the UID SORT command, properly taking into account the
>> 'sort' criterion:
>> C: 4 UID SORT RETURN (PARTIAL 1:1) (DATE) US-ASCII UNDELETED
>> S: * ESEARCH (TAG "4") UID PARTIAL (1:1 1)
>> S: 4 OK Sort completed (0.000 secs).
>>
>> To fix this it should check for the availability of the 'SEARCH'
>> capability on the 'CONTEXT' as an alternative check that 'ESEARCH' is
>> available.
>
> This fix isn't correct.
>
> First, you're allowing an ESORT with PARTIAL return if CONTEXT=SORT
> is not available, which is not correct.
>
> Second, support for CONTEXT=SORT (or CONTEXT=SEARCH) isn't true 4466
> ESEARCH support. For the sort instance, ESEARCH-like behavior is
> available only if ESORT is available or CONTEXT=SORT is available.
> ESEARCH-like behavior is NOT available if we are using the SORT
> command and CONTEXT=SEARCH is available, since we are not using
> CONTEXT=SEARCH support in that code branch. Thus the specific check
> for SORT is correct in this instance - CONTEXT=SEARCH availability is
> irrelevant.
As stated in http://tools.ietf.org/html/rfc5267#section-4:
CONTEXT=SEARCH does in fact imply ESEARCH:
In the case of CONTEXT=SEARCH, the server supports the extended
SEARCH command syntax described in [IMAP-ABNF], and accepts three
additional return options.
CONTEXT=SORT does in fact imply ESORT:
Servers advertising CONTEXT=SORT also advertise the SORT capability,
as described in [SORT], support the extended SORT command syntax
described in Section 3, and accept three additional return options
for this extended SORT.
These additional return options allow for notifications of changes to
the results of SEARCH or SORT commands, and also allow for access to
partial results.
However the reverse is neither implied nor required for either cases.
For example you can have a server that supports ESORT but not
CONTEXT=SORT, which is exactly the case on my test server:
C: [LOGIN Command - username: test]
S: 1 OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID
ENABLE SORT SORT=DISPLAY THREAD=REFERENCES THREAD=REFS MULTIAPPEND
UNSELECT IDLE CHILDREN NAMESPACE UIDPLUS LIST-EXTENDED I18NLEVEL=1
CONDSTORE QRESYNC ESEARCH ESORT SEARCHRES WITHIN CONTEXT=SEARCH
LIST-STATUS] Logged in
As you can see it does support both ESEARCH and ESORT but only
CONTEXT=SEARCH, no CONTEXT=SORT.
Moreover, while on the $server_sort branch, PARTIAL return does NOT
require CONTEXT=SORT as well as ESORT, having only the latter
available is enough.
So allowing an ESORT with PARTIAL return even if CONTEXT=SORT is not
available is in fact correct.
Furthermore, the existing check makes the CONTEXT=SORT availability
mandatory, which in fact denies access to PARTIAL returns on servers
that do not support it but do support ESORT, which is in fact not
correct.
The entire check should be rewritten, because it is incorrectly
assuming that you could have a server that does not support ESORT and
still support CONTEXT=SORT. This can never happen, simply because if
the server does not support ESORT it will never support only
CONTEXT=SORT since the latter implies the former. Same holds true for
ESEARCH and CONTEXT=SEARCH for that matter. And yes, technically
CONTEXT=SEARCH is irrelevant on the $server_sort branch, but unlike
CONTEXT=SORT at least it does not needlessly block the PARTIAL return
on that branch in this particular case:)
The proper fix would be to change the existing check from:
if ((!$esearch || !empty($options['partial'])) &&
($cap = $this->queryCapability('CONTEXT')) &&
in_array('SORT|SEARCH', $cap)) {
to just a simple:
if (($esearch && !empty($options['partial'])) ) {
on both code branches, because on the $server_sort branch the $esearch
is set based on ESORT availability and on the other branch it is set
based on ESEARCH availability and those are the relevant capabilities,
like explained above.
Just for reference, I?ve manually added on my test server 101 messages
with subject headers like ?PARTIAL return test xxx?, making sure that
the partial range is different from the message UIDs (just for
exemplification purposes) and that the default mailbox order is also
be different than the custom sort orders used for testing. Afterwards
I did the following to confirm that PARTIAL return is working properly
when only ESORT is available, no CONTEXT=SORT:
1. Request a PARTIAL return, sorting by subject:
C: 4 UID SORT RETURN (PARTIAL 1:2) (SUBJECT) US-ASCII UNDELETED
S: * ESEARCH (TAG "4") UID PARTIAL (1:2 202,201)
S: 4 OK Sort completed (0.001 secs).
2. Fetch the message UIDs returned to confirm that the messages were
in fact first ordered by subject and only afterwards the PARTIAL range
was returned:
C: 5 UID FETCH 202,201 (ENVELOPE INTERNALDATE RFC822.SIZE)
S: * 100 FETCH (UID 201 INTERNALDATE "01-May-2014 11:05:03 +0000"
RFC822.SIZE 1827 ENVELOPE ("Thu, 01 May 2014 11:05:03 +0000" "PARTIAL
return test 002" ((NIL NIL "daniel_bistriceanu" "yahoo.com")) ((NIL
NIL "daniel_bistriceanu" "yahoo.com")) ((NIL NIL "daniel_bistriceanu"
"yahoo.com")) ((NIL NIL "test" "example.com")) NIL NIL NIL NIL))
S: * 101 FETCH (UID 202 INTERNALDATE "01-May-2014 11:05:04 +0000"
RFC822.SIZE 1827 ENVELOPE ("Thu, 01 May 2014 11:05:04 +0000" "PARTIAL
return test 001" ((NIL NIL "daniel_bistriceanu" "yahoo.com")) ((NIL
NIL "daniel_bistriceanu" "yahoo.com")) ((NIL NIL "daniel_bistriceanu"
"yahoo.com")) ((NIL NIL "test" "example.com")) NIL NIL NIL NIL))
S: 5 OK Fetch completed.
3. Request the exact same PARTIAL return, sorting by subject in reverse order:
C: 4 UID SORT RETURN (PARTIAL 1:2) (REVERSE SUBJECT) US-ASCII UNDELETED
S: * ESEARCH (TAG "4") UID PARTIAL (1:2 102:103)
S: 4 OK Sort completed (0.001 secs).
4. Fetch the message UIDs returned to confirm that the messages were
in fact first ordered by subject in reverse order and only afterwards
the PARTIAL range was returned:
C: 5 UID FETCH 102:103 (ENVELOPE INTERNALDATE RFC822.SIZE)
S: * 1 FETCH (UID 102 INTERNALDATE "01-May-2014 11:03:50 +0000"
RFC822.SIZE 1827 ENVELOPE ("Thu, 01 May 2014 11:03:48 +0000" "PARTIAL
return test 101" ((NIL NIL "daniel_bistriceanu" "yahoo.com")) ((NIL
NIL "daniel_bistriceanu" "yahoo.com")) ((NIL NIL "daniel_bistriceanu"
"yahoo.com")) ((NIL NIL "test" "example.com")) NIL NIL NIL NIL))
S: * 2 FETCH (UID 103 INTERNALDATE "01-May-2014 11:03:51 +0000"
RFC822.SIZE 1827 ENVELOPE ("Thu, 01 May 2014 11:03:50 +0000" "PARTIAL
return test 100" ((NIL NIL "daniel_bistriceanu" "yahoo.com")) ((NIL
NIL "daniel_bistriceanu" "yahoo.com")) ((NIL NIL "daniel_bistriceanu"
"yahoo.com")) ((NIL NIL "test" "example.com")) NIL NIL NIL NIL))
S: 5 OK Fetch completed.
More information about the bugs
mailing list