Re: [Extra] AD Review of draft-ietf-extra-imap4rev2

[Alexey Melnikov <[e-mail]>]

Hi Murray,

Thank you for your detailed review! Below I removed some comments I 
still need to act upon/investigate. Detailed replies below:

On 04/01/2021 08:18, Murray S. Kucherawy wrote:
>
> At long last. Thanks for your patience. This is a lot of material to 
> sift through! This AD review is based on -22.  I’m coming at this as 
> someone who has made almost no direct use of IMAP as a user and 
> certainly never implemented it, so expect some neophyte-style feedback 
> here.  I also (briefly) reviewed the diff between RFC 3501 and this 
> version.
>
> Away we go...
>
>
> Section 1.2
>
>
> Should the “C:” and “S:” paragraph also mention that CRLF termination 
> is implicit in examples?  I realize Section 2.2 makes it clear that’s 
> the protocol anyway, so maybe this isn’t necessary, but I’ve seen it 
> made explicit in other documents so I thought I’d mention it.
>
CRLF is implied by use of term "line" (which is defined later in the 
document), but I clarified this here.
>
> Section 2.2.1
>
>   Clients MUST follow the syntax outlined in this specification
>
>    strictly.  It is a syntax error to send a command with missing or
>
>    extraneous spaces or arguments.
>
>
> It seems unnecessary to say this in a standards track document.  
> There’s a similar paragraph in Section 2.2.2.
>
I think the text is there due to various bugs in implementations, so I 
would rather keep it as is.

> Section 2.2.2
>
>    In the case of certain server data, the data MUST be recorded.
>
>
> This seems misplaced; the MUST should be in the descriptions for the 
> specific cases where this requirement exists.
>
I think this text is actually Ok where it is, as it warns/requires the 
client to implement proper cashing. It is more a statement of intent.

If you think use of RFC 2119 language here is not right, we can discuss 
that.

>
> Section 2.3
>
>
> Most of the subsections below here start with a clause that defines 
> the term, but it’s not a sentence.  This may be personal preference, 
> but I suggest adjusting them slightly so they’re sentences.  I’ve 
> included an example below where I have an adjacent suggestion and thus 
> fixed the whole paragraph.
>
I think I fixed these.
>
> Section 2.3.1.1
>
> “STRONGLY ENCOURAGES” in all caps seems awkward.  I’m tempted to say 
> something about it not being valid BCP 14, etc.
>
Changed.
>
> Section 2.3.2
>
>
> The opening sentence isn’t a sentence.  Also later, “Keyword” is 
> introduced without indicating that it’s one of the two possible flag 
> types (which I believe to be the case).  I suggest:
>
>   A message has associated with it a list of zero or more named tokens,
>
>    known as “flags”. A flag is set by its addition to this list, and 
> is cleared by its
>
>    removal. There are two types of flags in IMAP4rev2: system flags, 
> and keywords.
>
>    A flag of either type can also be permanent or session-only.
>
This is much better. Thank you.
>
> Second paragraph: s/begin/begins/
>
>
> s/in used in/in use in/
>
Applied, thanks
>
> The text surrounding $Phishing goes further into human factors work 
> than other working groups have felt appropriate.  (DKIM and DMARC come 
> to mind.)  Are we sure that belongs here?
>
Yes, considering this is a security consideration, a good example of UI 
is necessary here.
>
> It seems strange to oblige a client detecting both $Junk and $NotJunk 
> being set to deal with that condition by fixing it.  The advice to 
> ignore them is fine, however.
>
This is how they were defined when they were registered. I appreciate 
that this might be strange, but changing semantics at this point is not 
going to work.

If you suggest changing the description without changing the semantics, 
we should consider this.

> It’s also curious to put a normative SHOULD on registering keywords, 
> as IANA activity is outside of the protocol.
>
I've seen this done in other RFCs. This is a requirement on 
implementors, which might look odd, but I think this is still a 
legitimate use of RFC 2119 keyword.
>
> It’s more typical to say you SHOULD ignore keywords you don’t 
> recognize, and then make a reference to the registry.
>
>
> Should this section mention whether the system flags are permanent?  
> \Seen, for example, is clearly a system flag, but it could also be a 
> session flag.
>
No, implementations differ and sometimes even \Seen is not permanent. 
(The last sentence of the first paragraph already says "A flag of either 
type can also be permanent or session-only.")


> Section 2.3.3
>
>
> All the SHOULDs in here make me wonder when an implementor might 
> legitimately choose to do something different.  Or maybe instead of 
> “SHOULD be”, just say “is”?
>
I think for delivery by SMTP, I agree. For COPY/MOVE there is actually a 
posibility that some mailstores wouldn't preserve original values, so 
this is asking them to really think hard about preserving it.

And for APPEND, this is a similar requirement on MUAs to preserve this 
information. (Some don't and it is very awkward.)

So I prefer to keep the last 2 SHOULDs.

> Section 4.3
>
> 4th paragraph: s/alternate/alternative/
>
  [snip]
>
> Section 5.1.2.1
>
>
> Although both “Implementer” and “Implementor” appear to be correct, 
> you use the latter everywhere in the document except here.  (Actually, 
> Google docs suggests the former.)
>
I changed it to implementors and I am happy for RFC Editor to tell me 
which one they prefer ;-)
>
> Section 5.2
>
>
> “Sometimes, such behavior is REQUIRED.”  This isn’t a specific 
> normative statement, so I’d use the lowercase version here, and use 
> BCP 14 language when you’re presenting specific cases.
>
I agree. Changed.

  [snip]
>
> Section 6.1.2
>
>
> This section says NOOP can be used to reset the autologout timer 
> (which seems definite), but Section 5.1.4 says any command only SHOULD 
> reset the timer.  Do these line up?
>
(I assume you meant 5.4)

Well spotted. I changed text in 5.4 to readL

  The receipt of any command from the client during that interval resets 
the autologout timer.

>
> Section 6.2
>
>
> s/alternate/alternative/
>
Changed.
>
> Section 6.2.3
>
> I think there’s an “or” missing in the last paragraph.
>
This looks Ok to me. Can you elaborate?

  [snip]

> This section should make it more explicit, up front, that the response 
> names the extensions that were successfully enabled.  Right now the 
> lede is buried in about the 7th paragraph.
I didn't move the paragraph that talks about ENABLED response, but I 
clarified it.
>
> Section 6.3.2
>
>
> Where is “Paragraph 10”?
>
This is pointing to paragraph 10 within Section 7.1. (Server Responses - 
Status Responses). I.e. this is pointing to the CLOSED response code 
definition.


This looks like an xml2rfc glitch and I added a note in XML for RFC 
Editor to look at it.

>
> Section 6.3.4
>
>
> The all-caps “UNLESS” should probably be lowercase-ized.
>
>
> Section 6.3.5
>
>
> Same remark about “UNLESS”.
>
>
> Section 6.3.6
>
>
> Same remark about “UNLESS”.
>
Changed all 3.
>
> It’s weird to say “renaming INBOX is permitted”
>
This is saying that the server wouldn't return "BAD" to it.
>
> followed immediately by “some servers don’t let you do this”.
>
But this is saying that the server can still return "NO".
>
> Which is the standard for this version of IMAP?  If it’s an 
> implementation choice, I think that’s what this should say.
>
Yes, it is.
>
> Section 6.3.9
>
>
>    The LIST command returns a subset of names from the complete set of
>
>    all names available to the client.
>
>
> Names of what?
>
Mailbox names. I will clarify.
>
>    Clients SHOULD use the empty reference
>
>    argument.
>
>
> Why might I not do this?
>
If you want "change directory" kind of behaviour (where the directory is 
the reference name argument).
>
>     For example, here are some examples …
>
>
> You can probably drop “For example,”.
>
Done.
>
> Section 6.3.9.2
>
>
> This section (and the previous) require that \Subscribed be computed 
> accurately before the response is generated.  How does this square 
> with the text in Section 6.3.9 that insists replies come quickly?
>
You have to comply with both :-). It is quite important for \Subscribed 
to be accurate, so implementations need to do that while maintaining 
reasonable performance.


  [snip]

>
> The second paragraph also gets into the presentation of the reply.  
> This is human factors stuff the IETF tends to avoid.  Are we sure it 
> belongs here?
>
This is copy of the text from another RFC and I personally found this 
text to be useful when implementing.
>
> Section 6.3.9.6
>
>
>     Servers SHOULD ONLY return …
>
>
> I think “ONLY” should be lowercase here (confusion with BCP 14).
>
Changed.
>
> Section 6.3.9.7
>
>
>    another user renaming of deleting the
>
>
> s/of/or/
>
Fixed, thank you.


  [snip]

>
> Section 6.3.10
>
>
>    The NAMESPACE command causes a single ungagged NAMESPACE …
>
>
> You probably meant “untagged”, unless NAMESPACE is generally noisy.
>
Thank you :-)
>
>   In this example, a server supports 2 Personal Namespaces.  …
>
>
> s/2/two/
>
>
> There’s a reference to X-PARAM in here that probably needs updating in 
> light of BCP 178.
>
>
>       refer to sectionsSection 7,
>
>  Section 7.3.1 for more information about …
>
>
> Remove “sections” and stick an “and” between them.
>
Changed.
>
> Section 6.3.12
>
>
> I’m not sure that 310 is the correct length for the first example.  I 
> get 312.
>
I just checked and I got 310.
>
> Am I misunderstanding how this gets determined?  I get this by adding 
> the length of each line (including the empty ones) plus two for each 
> line representing the terminating CRLF.
>
Correct. But note that the CRLF at the end of the command (it appears as 
an empty line) doesn't count, because it is part of the command line, 
not the literal itself.
>
> Using that method, the second example also appears to be off by two.
>
>
> Section 6.3.13
>
>
>   Without the IDLE command a client requires to poll the server for
>
>
> s/requires/needs/ (or maybe “would need”)
>
>
>   responses at any time.  If the server choose to send unsolicited
>
>
> s/choose/chooses/
>
Fixed.
>
> The specific advice about reissuing IDLE every 29 minutes presumes the 
> auto-logout timeout is fixed at 30 minutes.  Since I think I read 
> higher up that this could be some other time limit, it might be a good 
> thing to negotiate somehow via a capability, or the continuation reply 
> to IDLE, or something like that.
>
The limit can't be smaller than 30 minutes, without violating IMAP4rev2. 
So 29 limit is a safe default that should work in all cases.

Unfortunately there is no way to advertise the limit at the moment.

> Section 6.4.2
>
>
>    The UNSELECT command frees server's resources associated with the
>
>    selected mailbox and returns the server to the authenticated state.
>
>
> I suggest changing “server’s” to “session’s”.  As written here, it 
> could be read to say you’re deleting the selected mailbox.
>
Ok.
>
> Section 6.4.4
>
>
> The use of “(*)” to make a footnote that’s actually just the next 
> paragraph can probably be eliminated by just merging the paragraphs.
>
>
> OLD:
>
>   Individual options may contain parameters enclosed in
>
>    parentheses (*).  If an option has parameters, they consist of atoms
>
>    and/or strings and/or lists in a specific order.  Any options not
>
>    defined by extensions that the server supports must be rejected with
>
>    a BAD response.
>
>
>    (*) - if an option has a mandatory parameter, which can always be
>
>    represented as a number or a sequence-set, the option parameter does
>
>    not need the enclosing ().  See the ABNF for more details.
>
>
> NEW:
>
> Individual options may contain parameters enclosed in parentheses.  
> (However, if an option has a mandatory parameter, which can always be 
> represented as a number or a sequence-set, the option parameter does 
> not need the enclosing parentheses.  See the ABNF for more details).  
> If an option has parameters, they consist of atoms and/or strings 
> and/or lists in a specific order.  Any options not defined by 
> extensions that the server supports MUST be rejected with a BAD response.
>
>
> (END)
>
I used your text. Thank you.
>
> In “Return number of the messages”, I think it should be “Return the 
> number of messages”.
>
Changed.
>
> The definitions of several search keys should be reviewed to ensure 
> use of the terms “header field” (not simply “header”) and “header 
> field value” appropriately, matching current convention.
>
Ok, I think I found all cases. If you find anything I miss, please let 
me know.
>
> Does TEXT matching ignore line breaks?  For instance, does searching 
> for “foo bar” match “foo<newline>bar”? Given the text about flexible 
> matching, I was wondering.
>
It might. Some implementations canonicalize whitespace, so that would 
match. This is implementation specific.
>
> Section 6.4.4.2
>
>
> “as described in by Section 5.5.” -- s/in by/in/
>
Fixed.

  [snip]

> Section 7.1
>
>
> Why would CONTACTADMIN be returned as part of an OK message, such as 
> in the example given?  I read that as “Your login was successful, but 
> you should contact your help desk.”
>
You are right, using CONTACTADMIN with NO is more useful. So I've 
updated the example.

[snip]


> Section 7.4.2
>
> In the ENVELOPE section, there are references to “header” that should 
> be “header field”.
>
I hope I caught most of these as well.
>
> Section 13.1
>
> RFCs 2152 and 2180 are normative downrefs; please make sure the 
> shepherd writeup calls this out.
>
>
> -MSK