On 11/08/2014 11:30 a.m., Alex Rousskov wrote: > On 08/10/2014 06:11 AM, Amos Jeffries wrote: <snip> >> * please use tok.prefix(CharacterSet::ALPHA) to parse the FTP command >> instead of BlackSpace. Then explicit delimiter check to validate the input. >> - RFC 959 is clear: >> "The command codes themselves are alphabetic characters terminated >> by the character <SP> (Space) if parameters follow and Telnet-EOL >> otherwise." > > Not changed. I do not see a compelling reason for a general purpose > relay to police traffic by default. Squid itself does not care if there > is some other character present in some command name. Why increase the > chances of breaking what was working without Squid by rejecting commands > by default? > > Yes, it is possible that some command that Squid does care about would > have invalid trailing characters in it, preventing Squid from > recognizing it, but, with your approach, Squid would have to reject that > command anyway, so we would not break something that would work in a > policing Squid. In the worst case, we would just make triage more difficult. > > If you insist on Squid rejecting RFC-invalid command names, I will > change the code, but I suggest leaving it permissive for now. >
I disagree be should be quite so tolerant in the current Internet without an explicit reason. But that is not a blocker on this patch, we can fix it later. RFC 959 is quite explicit. Section 5.3 documents what consists a valid command (alphabet characters only, case insensitive, 4 or fewer). Also, the basic FTP commands are a fairly well-known set. Everything else must be listed in FEAT - which we can filter for offerings of commands not fitting the supported syntax. +1. Branch looks good enough for merge now. Please do, so I can re-work the other concurrent submissions on the new Tokenizer/CharacterSet/ConnStateData APIs. Amos
