{"thread":{"id":"30827","subject":"[PATCH] symbolic-ref: check format of given reference","startedAt":"2012-06-17T20:26:37Z","lastAt":"2012-06-19T14:56:25Z","messageCount":8,"participants":["Michael Schubert","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"193768","messageId":"4FDE3D7D.4090502@elegosoft.com","threadId":"30827","inReplyTo":null,"subject":"[PATCH] symbolic-ref: check format of given reference","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2012-06-17T20:26:37Z","receivedAt":"2012-06-17T20:26:37Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"Currently, it's possible to update HEAD with a nonsense reference since\nno strict validation is performed. Example:\n\n\t$ git symbolic-ref HEAD 'refs/heads/master\n >\n >\n > '\n\nFix this by checking the given reference with check_refname_format().\n\nSigned-off-by: Michael Schubert \n---\n\nThis was discussed earlier this year:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/189715\n\nWhat about pointing at non-existing references? Should this\nstill be allowed?\n\nAdditionally, I had to reindent two lines to make git-am happy\n(indent with spaces).\n\n builtin/symbolic-ref.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex 801d62e..22362e0 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -43,16 +43,18 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options,\n-\t\t\t git_symbolic_ref_usage, 0);\n-\tif (msg &&!*msg)\n+\t\t\t\tgit_symbolic_ref_usage, 0);\n+\tif (msg && !*msg)\n \t\tdie(\"Refusing to perform update with empty message\");\n \tswitch (argc) {\n \tcase 1:\n \t\tcheck_symref(argv[0], quiet);\n \t\tbreak;\n \tcase 2:\n+\t\tif (check_refname_format(argv[1], 0))\n+\t\t\tdie(\"No valid reference format: '%s'\", argv[1]);\n \t\tif (!strcmp(argv[0], \"HEAD\") &&\n-\t\t prefixcmp(argv[1], \"refs/\"))\n+\t\t\tprefixcmp(argv[1], \"refs/\"))\n \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n \t\tcreate_symref(argv[0], argv[1], msg);\n \t\tbreak;\n-- \n1.7.11.rc3.11.g7dba3f7.dirty\n"},{"id":"193769","messageId":"7vaa017j51.fsf@alter.siamese.dyndns.org","threadId":"30827","inReplyTo":"4FDE3D7D.4090502@elegosoft.com","subject":"Re: [PATCH] symbolic-ref: check format of given reference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-17T20:55:54Z","receivedAt":"2012-06-17T20:55:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Schubert writes:\n\n> This was discussed earlier this year:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/189715\n>\n> What about pointing at non-existing references? Should this\n> still be allowed?\n\nHow else would you reimplement \"checkout --orphan\" in your own\nPorcelain using symbolic-ref?\n\n>\n> Additionally, I had to reindent two lines to make git-am happy\n> (indent with spaces).\n\nI doubt that it is needed; the '-' lines show runs of HT followed by\nfewer than 8 SP, which should not trigger \"indent with spaces\".\n\n> builtin/symbolic-ref.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\n> index 801d62e..22362e0 100644\n> --- a/builtin/symbolic-ref.c\n> +++ b/builtin/symbolic-ref.c\n> @@ -43,16 +43,18 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n> \n> \tgit_config(git_default_config, NULL);\n> \targc = parse_options(argc, argv, prefix, options,\n> -\t\t\t git_symbolic_ref_usage, 0);\n> -\tif (msg &&!*msg)\n> +\t\t\t\tgit_symbolic_ref_usage, 0);\n> +\tif (msg && !*msg)\n> \t\tdie(\"Refusing to perform update with empty message\");\n> \tswitch (argc) {\n> \tcase 1:\n> \t\tcheck_symref(argv[0], quiet);\n> \t\tbreak;\n> \tcase 2:\n> +\t\tif (check_refname_format(argv[1], 0))\n> +\t\t\tdie(\"No valid reference format: '%s'\", argv[1]);\n> \t\tif (!strcmp(argv[0], \"HEAD\") &&\n> -\t\t prefixcmp(argv[1], \"refs/\"))\n> +\t\t\tprefixcmp(argv[1], \"refs/\"))\n> \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n> \t\tcreate_symref(argv[0], argv[1], msg);\n> \t\tbreak;\n"},{"id":"193778","messageId":"4FDF18E5.7020908@elegosoft.com","threadId":"30827","inReplyTo":"7vaa017j51.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] symbolic-ref: check format of given reference","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2012-06-18T12:02:45Z","receivedAt":"2012-06-18T12:02:45Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"On 06/17/2012 10:55 PM, Junio C Hamano wrote:\n> Michael Schubert writes:\n> \n>> This was discussed earlier this year:\n>>\n>> http://thread.gmane.org/gmane.comp.version-control.git/189715\n>>\n>> What about pointing at non-existing references? Should this\n>> still be allowed?\n> \n> How else would you reimplement \"checkout --orphan\" in your own\n> Porcelain using symbolic-ref?\n\nForgot about that.\n\n>>\n>> Additionally, I had to reindent two lines to make git-am happy\n>> (indent with spaces).\n> \n> I doubt that it is needed; the '-' lines show runs of HT followed by\n> fewer than 8 SP, which should not trigger \"indent with spaces\".\n\nI've only noticed because git-am was telling me when I tried to\napply the patch.? Am I missing something?\n"},{"id":"193782","messageId":"7vr4tc4lsc.fsf@alter.siamese.dyndns.org","threadId":"30827","inReplyTo":"4FDF18E5.7020908@elegosoft.com","subject":"Re: [PATCH] symbolic-ref: check format of given reference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-18T16:39:15Z","receivedAt":"2012-06-18T16:39:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Schubert writes:\n\n>>> Additionally, I had to reindent two lines to make git-am happy\n>>> (indent with spaces).\n>> \n>> I doubt that it is needed; the '-' lines show runs of HT followed by\n>> fewer than 8 SP, which should not trigger \"indent with spaces\".\n>\n> I've only noticed because git-am was telling me when I tried to\n> apply the patch.? Am I missing something?\n\nPerhaps, but I cannot tell exactly what you are doing wrong.\n\nIf you didn't touch lines you did not have to in a way to break\nindentation and cause \"indent with spaces\", \"am\" would not have\ncomplained (it only looks at \"+\" lines).\n\nAttached is a patch based on your patch but removes the unnecessary\nre-indentation part, and \"git am\" happily applies it to my tree\nwithout complaining. Does it apply for you (obviously to a revision\nwithout your patch) cleanly without complaint? Otherwise it could\nbe that whitespace categories that are specified for the file in\nyour local attributes file may be different from mine (i.e. an empty\nset).\n\n-- >8 --\nFrom: Michael Schubert \nDate: Sun, 17 Jun 2012 22:26:37 +0200\nSubject: [PATCH] symbolic-ref: check format of given reference\n\nCurrently, it's possible to update HEAD with a nonsense reference since\nno strict validation is performed. Example:\n\n\t$ git symbolic-ref HEAD 'refs/heads/master\n >\n >\n > '\n\nFix this by checking the given reference with check_refname_format().\n\nSigned-off-by: Michael Schubert \nSigned-off-by: Junio C Hamano \n---\n builtin/symbolic-ref.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex 801d62e..a529541 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_symbolic_ref_usage, 0);\n-\tif (msg &&!*msg)\n+\tif (msg && !*msg)\n \t\tdie(\"Refusing to perform update with empty message\");\n \tswitch (argc) {\n \tcase 1:\n \t\tcheck_symref(argv[0], quiet);\n \t\tbreak;\n \tcase 2:\n+\t\tif (check_refname_format(argv[1], 0))\n+\t\t\tdie(\"No valid reference format: '%s'\", argv[1]);\n \t\tif (!strcmp(argv[0], \"HEAD\") &&\n \t\t prefixcmp(argv[1], \"refs/\"))\n \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n-- \n1.7.11\n"},{"id":"193784","messageId":"7vipeo4kcp.fsf@alter.siamese.dyndns.org","threadId":"30827","inReplyTo":"7vr4tc4lsc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] symbolic-ref: check format of given reference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-18T17:10:14Z","receivedAt":"2012-06-18T17:10:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> From: Michael Schubert \n> Date: Sun, 17 Jun 2012 22:26:37 +0200\n> Subject: [PATCH] symbolic-ref: check format of given reference\n>\n> Currently, it's possible to update HEAD with a nonsense reference since\n> no strict validation is performed. Example:\n>\n> \t$ git symbolic-ref HEAD 'refs/heads/master\n> >\n> >\n> > '\n\nIt would be nice to add a new test or two to t1401. 1401.3 was\nalready trying to catch a malformed reference with this test:\n\n\ttest_must_fail git symbolic-ref HEAD foo\n\nand it did trigger thanks to the prefixcmp(argv[1], \"refs/\") test we\nalready have. Probably something like\n\n\tgit symbolic-ref HEAD \"refs/heads/.foo\"\n\tgit symbolic-ref HEAD \"refs/heads/-foo\"\n\nwould be a good start.\n\nTo make the latter _correctly_ work requires a bit of work, though.\nWe should make sure all the check_refname_format() callers pass the\nfull path to a ref, get rid of ALLOW_ONELEVEL, and redo commits like\n6348624 (disallow branch names that start with a hyphen, 2010-09-14)\nand 4f0accd (tag: disallow '-' as tag name, 2011-05-10).\n\nFor that matter, shouldn't symbolic-ref be forbidden to point\noutside refs/heads/, not just restricted in refs/ like the current\ncode does?\n"},{"id":"193839","messageId":"20120619144712.GB12085@sigill.intra.peff.net","threadId":"30827","inReplyTo":"7vipeo4kcp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] symbolic-ref: check format of given reference","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-19T14:47:12Z","receivedAt":"2012-06-19T14:47:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 18, 2012 at 10:10:14AM -0700, Junio C Hamano wrote:\n\n> For that matter, shouldn't symbolic-ref be forbidden to point\n> outside refs/heads/, not just restricted in refs/ like the current\n> code does?\n\nWe tried that already but reverted it due to topgit. See:\n\n commit e9cc02f0e41fd5d2f51e3c3f2b4f8cfa9e434432\n Author: Jeff King \n Date: Fri Feb 13 13:26:09 2009 -0500\n\n symbolic-ref: allow refs/ in HEAD\n\n Commit afe5d3d5 introduced a safety valve to symbolic-ref to\n disallow installing an invalid HEAD. It was accompanied by\n b229d18a, which changed validate_headref to require that\n HEAD contain a pointer to refs/heads/ instead of just refs/.\n Therefore, the safety valve also checked for refs/heads/.\n\n As it turns out, topgit is using refs/top-bases/ in HEAD,\n leading us to re-loosen (at least temporarily) the\n validate_headref check made in b229d18a. This patch does the\n corresponding loosening for the symbolic-ref safety valve,\n so that the two are in agreement once more.\n\n-Peff\n"},{"id":"193840","messageId":"20120619145238.GC12085@sigill.intra.peff.net","threadId":"30827","inReplyTo":"20120619144712.GB12085@sigill.intra.peff.net","subject":"Re: [PATCH] symbolic-ref: check format of given reference","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-19T14:52:38Z","receivedAt":"2012-06-19T14:52:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 19, 2012 at 10:47:12AM -0400, Jeff King wrote:\n\n> On Mon, Jun 18, 2012 at 10:10:14AM -0700, Junio C Hamano wrote:\n> \n> > For that matter, shouldn't symbolic-ref be forbidden to point\n> > outside refs/heads/, not just restricted in refs/ like the current\n> > code does?\n> \n> We tried that already but reverted it due to topgit. See:\n> \n> commit e9cc02f0e41fd5d2f51e3c3f2b4f8cfa9e434432\n> Author: Jeff King \n> Date: Fri Feb 13 13:26:09 2009 -0500\n> \n> symbolic-ref: allow refs/ in HEAD\n> \n> Commit afe5d3d5 introduced a safety valve to symbolic-ref to\n> disallow installing an invalid HEAD. It was accompanied by\n> b229d18a, which changed validate_headref to require that\n> HEAD contain a pointer to refs/heads/ instead of just refs/.\n> Therefore, the safety valve also checked for refs/heads/.\n> \n> As it turns out, topgit is using refs/top-bases/ in HEAD,\n> leading us to re-loosen (at least temporarily) the\n> validate_headref check made in b229d18a. This patch does the\n> corresponding loosening for the symbolic-ref safety valve,\n> so that the two are in agreement once more.\n\nThe \"at least temporarily\" in that commit message merited a little\ninvestigation. There was some discussion of changing topgit to record\nits information using a different scheme, but there was no clear\noutcome:\n\n http://thread.gmane.org/gmane.comp.version-control.git/109581\n\nSo if somebody wanted to re-tighten this check, they would want\nto at least check topgit's current behavior, and see which versions of\nit we would be breaking. I tend to think it is not worth the effort.\n\n-Peff\n"},{"id":"193841","messageId":"4FE09319.8000306@elegosoft.com","threadId":"30827","inReplyTo":"7vr4tc4lsc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] symbolic-ref: check format of given reference","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2012-06-19T14:56:25Z","receivedAt":"2012-06-19T14:56:25Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"On 06/18/2012 06:39 PM, Junio C Hamano wrote:\n> Michael Schubert writes:\n> \n>>>> Additionally, I had to reindent two lines to make git-am happy\n>>>> (indent with spaces).\n>>>\n>>> I doubt that it is needed; the '-' lines show runs of HT followed by\n>>> fewer than 8 SP, which should not trigger \"indent with spaces\".\n>>\n>> I've only noticed because git-am was telling me when I tried to\n>> apply the patch.? Am I missing something?\n> \n> Perhaps, but I cannot tell exactly what you are doing wrong.\n\nThunderbird replaced the tabs (but did not do that before). Sorry\nfor the noise.\n\n> If you didn't touch lines you did not have to in a way to break\n> indentation and cause \"indent with spaces\", \"am\" would not have\n> complained (it only looks at \"+\" lines).\n> \n> Attached is a patch based on your patch but removes the unnecessary\n> re-indentation part, and \"git am\" happily applies it to my tree\n> without complaining. Does it apply for you (obviously to a revision\n> without your patch) cleanly without complaint?\n\nWorks, Thanks.\n\n> -- >8 --\n> From: Michael Schubert \n> Date: Sun, 17 Jun 2012 22:26:37 +0200\n> Subject: [PATCH] symbolic-ref: check format of given reference\n> \n> Currently, it's possible to update HEAD with a nonsense reference since\n> no strict validation is performed. Example:\n> \n> \t$ git symbolic-ref HEAD 'refs/heads/master\n> >\n> >\n> > '\n> \n> Fix this by checking the given reference with check_refname_format().\n> \n> Signed-off-by: Michael Schubert \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/symbolic-ref.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\n> index 801d62e..a529541 100644\n> --- a/builtin/symbolic-ref.c\n> +++ b/builtin/symbolic-ref.c\n> @@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n> \tgit_config(git_default_config, NULL);\n> \targc = parse_options(argc, argv, prefix, options,\n> \t\t\t git_symbolic_ref_usage, 0);\n> -\tif (msg &&!*msg)\n> +\tif (msg && !*msg)\n> \t\tdie(\"Refusing to perform update with empty message\");\n> \tswitch (argc) {\n> \tcase 1:\n> \t\tcheck_symref(argv[0], quiet);\n> \t\tbreak;\n> \tcase 2:\n> +\t\tif (check_refname_format(argv[1], 0))\n> +\t\t\tdie(\"No valid reference format: '%s'\", argv[1]);\n> \t\tif (!strcmp(argv[0], \"HEAD\") &&\n> \t\t prefixcmp(argv[1], \"refs/\"))\n> \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n> \n"}]}