On 05/01, Jonathan Tan wrote: > On 05/01/2017 04:29 PM, Junio C Hamano wrote: > >Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > > > >>Thanks for your comments. If you're referring to the codepath > >>involving write_sha1_file() (for example, builtin/hash-object -> > >>index_fd or builtin/unpack-objects), that is fine because > >>write_sha1_file() invokes freshen_packed_object() and > >>freshen_loose_object() directly to check if the object already exists > >>(and thus does not invoke the new mechanism in this patch). > > > >Is that a good thing, though? It means that you an attacker can > >feed one version to the remote object store your "grab blob" hook > >gets the blobs from, and have you add a colliding object locally, > >and the usual "are we recording the same object as existing one?" > >check is bypassed. > > If I understand this correctly, what you mean is the situation where > the hook adds an object to the local repo, overriding another object > of the same name? If yes, I think that is the nature of executing an > arbitrary command. If we really want to avoid that, we could drop > the hook functionality (and instead, for example, provide the URL of > a Git repo instead from which we can communicate using a new > fetch-blob protocol), although that would reduce the usefulness of > this, especially during the transition period in which we don't have > any sort of batching of requests. If I understand correctly this is where we aim to be once all is said and done. I guess the question is what are we willing to do during the transition phase. -- Brandon Williams