{"thread":{"id":"28810","subject":"Reference has invalid format: check maybe a bit to harsh?","startedAt":"2011-10-31T19:14:25Z","lastAt":"2011-11-01T09:59:59Z","messageCount":4,"participants":["Peter Oberndorfer","Junio C Hamano","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"178573","messageId":"60007404.ge1WXNp2Qn@soybean","threadId":"28810","inReplyTo":null,"subject":"Reference has invalid format: check maybe a bit to harsh?","fromName":"Peter Oberndorfer","fromEmail":"kumbayo84@arcor.de","sentAt":"2011-10-31T19:14:25Z","receivedAt":"2011-10-31T19:14:25Z","isPatch":false,"sender":{"key":"kumbayo84@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1041267?v=4"},"body":"Hi,\n\ni am using the next branch for testing and i noticed the following:\nabout any git command i execute in a certain repo dies with\nfatal: Reference has invalid format: \n'refs/patches/obd_development/blah:_various_improvements_remote_debugging'\n\nThis is probably caused by\ndce4bab6567de7c458b334e029e3dedcab5f2648 add_ref(): verify that the refname is \nformatted correctly\n\nThe invalid refs(about 30, loose and packed) containing a ':' were created by \nstgit a long time ago(Dec 2006)\n\nPersonally i do not care too much, i patched my git to not die at this point \nbut to only display a error.\n-> The invalid refs are not accessible, but the rest of the repo still works.\n\nBut i'm just wondering if dieing when seeing a single invalid ref might be a \nbit too harsh since no git tools can be used anymore on this repo at all.\n\n\nSmall side note:\nIt seems t1402-check-ref-format.sh contains not test\nfor the invalid ref char ':' yet.\n(i do not know if it is tested somewhere else...)\n\nThanks,\nGreetings Peter\n"},{"id":"178574","messageId":"7vty6pos20.fsf@alter.siamese.dyndns.org","threadId":"28810","inReplyTo":"60007404.ge1WXNp2Qn@soybean","subject":"Re: Reference has invalid format: check maybe a bit to harsh?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-31T19:54:31Z","receivedAt":"2011-10-31T19:54:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Oberndorfer <kumbayo84@arcor.de> writes:\n\n> The invalid refs(about 30, loose and packed) containing a ':' were created by \n> stgit a long time ago(Dec 2006)\n\nI think even back then colon was one of the forbidden letters in a\nrefname. Of course, it is entirely possible that broken third-party tools\nmay have created such file that is not a ref in .git/refs hierarchy by\nhand, and we may not be carefully rejecting such broken refs for a long\ntime.\n\n    ... Goes and asks \"git blame\" ...\n\n03feddd (git-check-ref-format: reject funny ref names., 2005-10-13)\nstarted disallowing control characters and other characters that are used\nfor range operators and the separator between LHS and RHS of refspecs,\nfurther tightened by 6828399 (Forbid pattern maching characters in\nrefnames., 2005-12-15).\n\n> But i'm just wondering if dieing when seeing a single invalid ref might be a \n> bit too harsh since no git tools can be used anymore on this repo at all.\n\nI agree that we would want to give users an escape hatch.  That is, if we\ncan make something like this to work:\n\n    c=$(git rev-parse --force refs/patches/obd_development/blah:_vari...)\n    git update-ref refs/patches/obd_development/blah--various-improvements $c\n\nI think we would be in a good shape.\n"},{"id":"178576","messageId":"7vpqhcq5h2.fsf@alter.siamese.dyndns.org","threadId":"28810","inReplyTo":"7vty6pos20.fsf@alter.siamese.dyndns.org","subject":"Re: Reference has invalid format: check maybe a bit to harsh?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-31T20:19:21Z","receivedAt":"2011-10-31T20:19:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I agree that we would want to give users an escape hatch.  That is, if we\n> can make something like this to work:\n>\n>     c=$(git rev-parse --force refs/patches/obd_development/blah:_vari...)\n>     git update-ref refs/patches/obd_development/blah--various-improvements $c\n\nAlso we would need to be able to say\n\n    git update-ref -d refs/patches/obd_development/blah:_vari...\n\nto get rid of the offending one.\n\n> I think we would be in a good shape.\n\nHaving said all that, I think we should in general loosen the checks done\non the reading side a lot more. The \"checks\" themselves should stay, can\ngive loud warnings, and even can error out when appropriate, but in an\noperation that is necessary to recover from _existing_ breakage (like the\none in this thread, a file with a colon in its name in .git/refs/), the\nability to read and to remove is essential for recovery.\n\nI vaguely recall we had to apply a fix in the same spirit to loosen\nreading side after the offending topic was merged to 'master' during this\ncycle about $GIT_DIR/config not possibly being a ref getting warned, or\nsomething.\n\nMichael, what do you think?\n"},{"id":"178619","messageId":"4EAFC31F.4090206@alum.mit.edu","threadId":"28810","inReplyTo":"7vpqhcq5h2.fsf@alter.siamese.dyndns.org","subject":"Re: Reference has invalid format: check maybe a bit to harsh?","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-11-01T09:59:59Z","receivedAt":"2011-11-01T09:59:59Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/31/2011 09:19 PM, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>> I agree that we would want to give users an escape hatch.  That is, if we\n>> can make something like this to work:\n>>\n>>     c=$(git rev-parse --force refs/patches/obd_development/blah:_vari...)\n>>     git update-ref refs/patches/obd_development/blah--various-improvements $c\n> \n> Also we would need to be able to say\n> \n>     git update-ref -d refs/patches/obd_development/blah:_vari...\n> \n> to get rid of the offending one.\n> \n>> I think we would be in a good shape.\n> \n> Having said all that, I think we should in general loosen the checks done\n> on the reading side a lot more. The \"checks\" themselves should stay, can\n> give loud warnings, and even can error out when appropriate, but in an\n> operation that is necessary to recover from _existing_ breakage (like the\n> one in this thread, a file with a colon in its name in .git/refs/), the\n> ability to read and to remove is essential for recovery.\n> \n> I vaguely recall we had to apply a fix in the same spirit to loosen\n> reading side after the offending topic was merged to 'master' during this\n> cycle about $GIT_DIR/config not possibly being a ref getting warned, or\n> something.\n> \n> Michael, what do you think?\n\nSupporting invalid reference names some places, but not others, and this\nperhaps (as in the case of \":\") platform-dependent would be a big can of\nworms (as can be seen by my attempt to summarize the situation, a few\nparagraphs below).\n\nI see the situation as a conflict between security and reliability on\nthe one hand and backwards-compatibility and/or the ability to recover\nfrom old mistakes on the other hand.  For example, reference names like\nthe following are problematic in earlier git releases:\n\n* \"refs/heads/foo/../../../../etc/passwd\" -- here be dragons\n\n* \"refs/heads/foo.lock/bar\" -- screws up the locking of a reference\ncalled \"refs/heads/foo\"\n\n* \"refs/heads/prn:/waste/my/paper\" -- is questionable on Windows, I believe\n\n* \"refs/heads//foo\" -- would be normalized to \"refs/heads/foo\" in\ncertain circumstances but not others\n\nPerhaps instead of arguing about exactly what command arguments are\ntreated strictly vs laxly and attempting to half-support invalid\nreference names, we could solve 98% of the problem with a couple of\nlocalized measures:\n\n1. Something in the git_snpath() callchain could prevent paths referring\nto files outside of $GIT_DIR from being generated (by normalizing the\npaths, stripping out \".\" and \"..\", etc).  I suppose that this change\nwould remove a lot of potential security issues at a stroke, and make it\nless important for refname handling to be paranoid.\n\n2. Invalid references could be detected and fixed via some mode of \"git\nfsck\".  This would then be the only codepath that has to handle invalid\nreference names, and would take that burden off of the rest of the code.\n \"git fsck\" could sanitize the names of invalid references and move them\nto some kind of \"lost+found\" namespace.  (Disadvantage: it could be\nimpractical to run \"git fsck\" on a remote repository to which one\ndoesn't have filesystem access.)\n\n\nIf one would want to plunge in with a complicated solution, things will\nget messy.  Here is an attempt at an exhaustive summary of the changes\nsince v1.7.7 that affect how strictly refnames are checked for validity,\nand ideas for how one could work around problems of backwards\ncompatibility in the particular cases.\n\n1. read_packed_refs() -- the old behavior was to accept *anything* found\nin the packed-refs file; however, invalid references could not be worked\nwith reliably.  Now checks that packed-refs refnames are valid and\ndie()s if not.\n\n   a. The old behavior could be restored for now.\n   a'. The old behavior could be restored, except that renames with\nleading, trailing, or duplicate slashes could silently be normalized.\nThis could lead to collisions between unnormalized and normalized names\nthat were previously distinct.  Such a collision is harmless if the two\nsymbols have the same value, but currently causes a fatal error if they\nhave distinct values.\n   b. Invalid references could be silently ignored (this would cause\nthem to be silently discarded at the next pack-refs).\n   c. Invalid references could be ignored with a warning (the warning\ncould include the SHA1).  This would cause a lot of warnings to be\noutput until the next pack-refs or the repository is fixed some other way.\n\n2. get_ref_dir() (reads loose refs) -- the old behavior was to accept\nanything that was found as a filesystem path under \"$GIT_DIR/refs\"\nexcept paths with components starting with \".\" or ending with \".lock\".\nNow checks that loose-ref refnames are valid and die()s if not.\n\n   a. The old behavior could be restored for now.\n   b. Invalid references could be silently ignored (this would cause\nthem to be carried around forever in the local repository, including\nafter a pack-refs, but omitted when the local repository is cloned).\n   c. Invalid references could be ignored with a warning (the warning\ncould include the SHA1).  This would cause a lot of warnings to be\noutput until the repository is fixed somehow.\n\n3. add_extra_ref() -- old behavior was to accept anything.  But since\nextra refs are generated locally and the names are not used, this\nbehavior shouldn't be a problem and could be restored for now.\n\n4. check_refname_format() -- now disallows any refname component than\nends with \".lock\" (previously only the last component was checked).  Now\ndisallows DEL character in refnames, in agreement with the old\nspecification.\n\n5. resolve_ref() -- previously passed its argument to git_snpath() and\ntried to open the path without any verification whatsoever.  Treated the\ncontents of symbolic refs similarly.  Now checks the validity of\nrefnames at both steps.\n\n   a. The old behavior could be restored, but this would be very\nquestionable given how refs like \"foo/../../../etc/passed\" might be handled.\n   b. There could be some laxer level of checking of security-relevant\nissues without enforcing all of the refname rules.\n\n6. The infamous change that caused files that looked like loose\nreferences but have invalid *contents* to cause a warning to be emitted\n(strictly speaking, this doesn't affect refnames but reference contents).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}