{"thread":{"id":"42511","subject":"Mark remote `gc --auto` error messages","startedAt":"2016-06-02T19:05:29Z","lastAt":"2016-06-05T09:36:38Z","messageCount":9,"participants":["Lukas Fleischer","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"288146","messageId":"146489432847.688.11121862368709034386@typhoon","threadId":"42511","inReplyTo":null,"subject":"Mark remote `gc --auto` error messages","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2016-06-02T19:05:29Z","receivedAt":"2016-06-02T19:05:29Z","isPatch":false,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"When running `git push`, it might occur that error messages are\ntransferred from the server to the client. While most messages (those\nexplicitly sent on sideband 2) are prefixed with \"remote:\", it seems\nthat error messages printed during the automatic householding performed\nby git-gc(1) are displayed without any additional decoration. Thus, such\nmessages can easily be misinterpreted as git-gc failing locally, see [1]\nfor an actual example of where that happened.\n\nDo we want anything like the following patch (completely untested)?\n\n-- 8< --\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a744437..15c323a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1775,9 +1775,20 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n const char *argv_gc_auto[] = {\n \"gc\", \"--auto\", \"--quiet\", NULL,\n };\n- int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;\n+ struct child_process proc = CHILD_PROCESS_INIT;\n+\n+ proc.no_stdin = 1;\n+ proc.stdout_to_stderr = 1;\n+ proc.err = use_sideband ? -1 : 0;\n+ proc.git_cmd = 1;\n+ proc.argv = argv_gc_auto;\n+\n close_all_packs();\n- run_command_v_opt(argv_gc_auto, opt);\n+ if (!start_command(&proc)) {\n+ if (use_sideband)\n+ copy_to_sideband(proc.err, -1, NULL);\n+ finish_command(&proc);\n+ }\n }\n if (auto_update_server_info)\n update_server_info(0);\n-- 8< --\n\nMore generally, do we care about making *all* \"remote\" strings easily\ndistinguishable from \"local\" strings? Even though it is unlikely to use\nthis for an actual attack, it seems that a malicious server can\ncurrently trick a user into performing an action by printing a message\nthat looks like something coming from \"local\" Git. Prefixing every\nserver message by \"remote:\" might look a bit ugly but maybe we can\nsimply use a different color instead and fall back to the prefix on\nterminals without color support. Opinions?\n\n[1] https://lists.archlinux.org/pipermail/aur-general/2016-June/032340.html\n"},{"id":"288148","messageId":"xmqqinxrtmgi.fsf@gitster.mtv.corp.google.com","threadId":"42511","inReplyTo":"146489432847.688.11121862368709034386@typhoon","subject":"Re: Mark remote `gc --auto` error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T19:33:33Z","receivedAt":"2016-06-02T19:33:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lukas Fleischer writes:\n\n> When running `git push`, it might occur that error messages are\n> transferred from the server to the client. While most messages (those\n> explicitly sent on sideband 2) are prefixed with \"remote:\", it seems\n> that error messages printed during the automatic householding performed\n> by git-gc(1) are displayed without any additional decoration. Thus, such\n> messages can easily be misinterpreted as git-gc failing locally, see [1]\n> for an actual example of where that happened.\n\nSounds like a sensible goal to me.\n"},{"id":"288154","messageId":"146489800609.1944.4398103814754920753@typhoon.lan","threadId":"42511","inReplyTo":"xmqqinxrtmgi.fsf@gitster.mtv.corp.google.com","subject":"Re: Mark remote `gc --auto` error messages","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2016-06-02T20:06:46Z","receivedAt":"2016-06-02T20:06:46Z","isPatch":false,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote:\n> Lukas Fleischer writes:\n> \n> > When running `git push`, it might occur that error messages are\n> > transferred from the server to the client. While most messages (those\n> > explicitly sent on sideband 2) are prefixed with \"remote:\", it seems\n> > that error messages printed during the automatic householding performed\n> > by git-gc(1) are displayed without any additional decoration. Thus, such\n> > messages can easily be misinterpreted as git-gc failing locally, see [1]\n> > for an actual example of where that happened.\n> \n> Sounds like a sensible goal to me.\n\nWhat exactly are you referring to (you only quoted the introduction)?\nDo you think we should fix the git-gc issue but keep the general\nbehavior of printing messages unaltered? Do you think it would be\nworthwhile to make server messages distinguishable in general?\n"},{"id":"288155","messageId":"CAPc5daXVx1=ptsKJEfEzXbjCNvwYxjAPyp_pob9CeR+Qr3tG_g@mail.gmail.com","threadId":"42511","inReplyTo":"146489800609.1944.4398103814754920753@typhoon.lan","subject":"Re: Mark remote `gc --auto` error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T20:14:02Z","receivedAt":"2016-06-02T20:14:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Jun 2, 2016 at 1:06 PM, Lukas Fleischer wrote:\n> On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote:\n>> Lukas Fleischer writes:\n>>\n>> > When running `git push`, it might occur that error messages are\n>> > transferred from the server to the client. While most messages (those\n>> > explicitly sent on sideband 2) are prefixed with \"remote:\", it seems\n>> > that error messages printed during the automatic householding performed\n>> > by git-gc(1) are displayed without any additional decoration. Thus, such\n>> > messages can easily be misinterpreted as git-gc failing locally, see [1]\n>> > for an actual example of where that happened.\n>>\n>> Sounds like a sensible goal to me.\n>\n> What exactly are you referring to (you only quoted the introduction)?\n> Do you think we should fix the git-gc issue but keep the general\n> behavior of printing messages unaltered? Do you think it would be\n> worthwhile to make server messages distinguishable in general?\n\nThe latter, which I think was what your implementation was attempting to do\nif I read it correctly.\n"},{"id":"288163","messageId":"20160602214834.GB13356@sigill.intra.peff.net","threadId":"42511","inReplyTo":"CAPc5daXVx1=ptsKJEfEzXbjCNvwYxjAPyp_pob9CeR+Qr3tG_g@mail.gmail.com","subject":"Re: Mark remote `gc --auto` error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-02T21:48:35Z","receivedAt":"2016-06-02T21:48:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 02, 2016 at 01:14:02PM -0700, Junio C Hamano wrote:\n\n> On Thu, Jun 2, 2016 at 1:06 PM, Lukas Fleischer wrote:\n> > On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote:\n> >> Lukas Fleischer writes:\n> >>\n> >> > When running `git push`, it might occur that error messages are\n> >> > transferred from the server to the client. While most messages (those\n> >> > explicitly sent on sideband 2) are prefixed with \"remote:\", it seems\n> >> > that error messages printed during the automatic householding performed\n> >> > by git-gc(1) are displayed without any additional decoration. Thus, such\n> >> > messages can easily be misinterpreted as git-gc failing locally, see [1]\n> >> > for an actual example of where that happened.\n> >>\n> >> Sounds like a sensible goal to me.\n> >\n> > What exactly are you referring to (you only quoted the introduction)?\n> > Do you think we should fix the git-gc issue but keep the general\n> > behavior of printing messages unaltered? Do you think it would be\n> > worthwhile to make server messages distinguishable in general?\n> \n> The latter, which I think was what your implementation was attempting to do\n> if I read it correctly.\n\nI think the implementation is doing much more, but it is probably a good\nthing.\n\nRight now we do not send auto-gc output over the sideband, and its\nstderr goes to receive-pack's stderr. But that is a different place for\ndifferent protocols. For git-over-https, it is probably apache's error\nlog, or /dev/null if the server admin configured it. For ssh, it may be\nback over the ssh stderr channel, or it may go to a log or nowhere if\nthe server admin intercepts receive-pack and redirects it.\n\nSo the greater question is not \"should this output be marked\" but\n\"should auto-gc data go over the sideband so that all clients see it\n(and any server-side stderr does not)\". And I think the answer is\nprobably yes. And that fixes the \"remote: \" thing as a side effect.\n\nIf it were no, then this is not the right solution, and the solution is\nto swap out copy_to_sideband() for something that copies to stderr with\n\"remote: \" prepended, or something.\n\n-Peff\n"},{"id":"288164","messageId":"20160602215317.GC13356@sigill.intra.peff.net","threadId":"42511","inReplyTo":"CAPc5daXVx1=ptsKJEfEzXbjCNvwYxjAPyp_pob9CeR+Qr3tG_g@mail.gmail.com","subject":"Re: Mark remote `gc --auto` error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-02T21:53:18Z","receivedAt":"2016-06-02T21:53:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 02, 2016 at 01:14:02PM -0700, Junio C Hamano wrote:\n\n> > What exactly are you referring to (you only quoted the introduction)?\n> > Do you think we should fix the git-gc issue but keep the general\n> > behavior of printing messages unaltered? Do you think it would be\n> > worthwhile to make server messages distinguishable in general?\n> \n> The latter, which I think was what your implementation was attempting to do\n> if I read it correctly.\n\nAnd btw, I don't think this patch fixes the general case. E.g., if\nreceive-pack hits any of its die(\"BUG\") lines, they will not be\nprefixed. Most clients wouldn't see them, but ssh ones would.\n\nTo fix that you'd have to do a whole async process wrapping\n`receive-pack` that just reads its stdout and stderr and muxes it back\nover the sideband. But I can think of two roadblocks there:\n\n - I think the original design of receive-pack was _not_ to share all\n of stderr with the user, because it might contain secret-ish\n server-side things. That's why we have rp_error() which copies to\n the sideband.\n\n I don't know how useful that is in practice. We copy the stderr\n wholesale from sub-processes like index-pack, so things like file\n paths are likely to get leaked there.\n\n - the implementation is a bit tricky, because the die() will take\n down the mux thread, too.\n\n-Peff\n"},{"id":"288166","messageId":"xmqqmvn3s148.fsf@gitster.mtv.corp.google.com","threadId":"42511","inReplyTo":"20160602214834.GB13356@sigill.intra.peff.net","subject":"Re: Mark remote `gc --auto` error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-02T21:59:51Z","receivedAt":"2016-06-02T21:59:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So the greater question is not \"should this output be marked\" but\n> \"should auto-gc data go over the sideband so that all clients see it\n> (and any server-side stderr does not)\". And I think the answer is\n> probably yes. And that fixes the \"remote: \" thing as a side effect.\n\nThanks for stating this a lot more clearly than I could, and I agree\nthat sending this to the other side regardless of the protocol is\nthe right thing. I somehow doubt that server operators would check\nApache logs to decide when to do a proper GC, so I do not consider\nit a true loss ;-)\n"},{"id":"288168","messageId":"20160602220454.GA17202@sigill.intra.peff.net","threadId":"42511","inReplyTo":"xmqqmvn3s148.fsf@gitster.mtv.corp.google.com","subject":"Re: Mark remote `gc --auto` error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-02T22:04:55Z","receivedAt":"2016-06-02T22:04:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 02, 2016 at 02:59:51PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So the greater question is not \"should this output be marked\" but\n> > \"should auto-gc data go over the sideband so that all clients see it\n> > (and any server-side stderr does not)\". And I think the answer is\n> > probably yes. And that fixes the \"remote: \" thing as a side effect.\n> \n> Thanks for stating this a lot more clearly than I could, and I agree\n> that sending this to the other side regardless of the protocol is\n> the right thing. I somehow doubt that server operators would check\n> Apache logs to decide when to do a proper GC, so I do not consider\n> it a true loss ;-)\n\nI definitely agree. I'd wonder more about \"would they want their users\nto see these details\". I dunno. I am only intimately familiar with one\ngit hosting site, and we turn off auto-gc completely.\n\n-Peff\n"},{"id":"288392","messageId":"20160605093638.16533-1-lfleischer@lfos.de","threadId":"42511","inReplyTo":"146489432847.688.11121862368709034386@typhoon","subject":"[PATCH] receive-pack: send auto-gc output over sideband 2","fromName":"Lukas Fleischer","fromEmail":"lfleischer@lfos.de","sentAt":"2016-06-05T09:36:38Z","receivedAt":"2016-06-05T09:36:38Z","isPatch":true,"sender":{"key":"lfleischer@lfos.de","avatar":"https://avatars.githubusercontent.com/u/5530842?v=4"},"body":"Redirect auto-gc output to the sideband such that it is visible to all\nclients. As a side effect, all auto-gc error messages are now prefixed\nwith \"remote: \" before being printed to stderr on the client-side which\nmakes it easier to understand that those error messages originate from\nthe server.\n\nSigned-off-by: Lukas Fleischer \n---\n builtin/receive-pack.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a744437..15c323a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1775,9 +1775,20 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \t\t\tconst char *argv_gc_auto[] = {\n \t\t\t\t\"gc\", \"--auto\", \"--quiet\", NULL,\n \t\t\t};\n-\t\t\tint opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR;\n+\t\t\tstruct child_process proc = CHILD_PROCESS_INIT;\n+\n+\t\t\tproc.no_stdin = 1;\n+\t\t\tproc.stdout_to_stderr = 1;\n+\t\t\tproc.err = use_sideband ? -1 : 0;\n+\t\t\tproc.git_cmd = 1;\n+\t\t\tproc.argv = argv_gc_auto;\n+\n \t\t\tclose_all_packs();\n-\t\t\trun_command_v_opt(argv_gc_auto, opt);\n+\t\t\tif (!start_command(&proc)) {\n+\t\t\t\tif (use_sideband)\n+\t\t\t\t\tcopy_to_sideband(proc.err, -1, NULL);\n+\t\t\t\tfinish_command(&proc);\n+\t\t\t}\n \t\t}\n \t\tif (auto_update_server_info)\n \t\t\tupdate_server_info(0);\n-- \n2.8.3\n"}]}