Tickets :: Comment on [#13153] Bugs in the current implementation for PARTIAL limiting (RFC 5267 [4.4]) in Horde's Imap

* Your Email Address
* Spam protection	Enter the letters below: .__.. .. ,.__ .__ \| \|\ / \./ [__)[__) \|__\| \/ \| [__)\|
Comment	>>> 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.
Attachment
Watch this ticket

Saved Queries