On Fri, Jan 9, 2015 at 2:39 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > The patched code will make it fail by hoping that queue_command() > that only handles "40-hex 40-hex ref" will reject the line that > begins with "push-cert". Instead of relying on such a hidden > dependency, wouldn't it be cleaner to actually parse the push-cert > block and then at the end notice and explictly say "Your requests > were syntactically correct, but I am not going to honor your request > to use the push-cert extension, because I never told you that I'd > offer you that capability", instead of rejecting the request with "I > was expecting old/new/ref but you sent a line with 'push-cert' on > it; what are you talking about?" When reading the documentation at first I understood it the way: "If I did not advertise the feature, I am expected to pretend it doesn't even exist". That lead me to the implementation as proposed. Your proposal to acknowledge the correctness of the message leads to more questions. How would we proceed? We would not accept the featured capability, but we could still do a normal push operation. This would not be desired by a authentic user, so aborting the whole push process would be ok. I expect such behavior only from malicious clients which actively want to abuse a feature which wasn't advertised, so I wouldn't mind being slightly dishonest to the client and pretend we don't know that capability. But relying on the hidden dependency is bad indeed. So maybe just answer more rudely "I did not advertise one of the capability you requested, go away! (closes connection)". > But the other side of the same coin is that it makes it harder to > diagnose the failure. The push-cert case is special compared to other capabilities as it is quite intrusive to the protocol compared to say "quiet", so more diagnosis may be helpful. > When the protocol exchange gets to this state, in practice, we know > we are talking with somebody who has push privilege into the > repository, Yeah but what is one repository compared to the whole server? (Just painting the devil on the wall now:) Say you could abuse one capability to gain access to the server or create a huge memory allocation such that the server becomes unresponsive for others. I know my argument is weak for the "parse and reject route", so I will see if I can adjust the patch to give diagnosis but still reject the un-advertised capability. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html