Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > 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? No. write_sha1_file() pays attention to objects already in the local object store to avoid hash collisions that can be used to replace a known-to-be-good object and that is done as a security measure. What I am reading in your response was that this new mechanism bypasses that, and I was wondering if that is a good thing.