On Fri, Mar 15, 2013 at 4:59 AM, Jeff King <peff@xxxxxxxx> wrote: > I tried pushing to a repository at Google Code for the first time today, > and I encountered some weird behavior with respect to asking for > credentials. > > If I use the url "https://code.google.com/r/repo/";, everything works; I > get prompted for my username/password. > > But if I instead use the url "https://myuser@xxxxxxxxxxxxxxx/r/repo/";, > it doesn't work. I get: > > $ git push > fatal: remote error: Invalid username/password. > You may need to use your generated googlecode.com password; see > https://code.google.com/hosting/settings > > without any prompt at all. I bisected it to 986bbc0 (http: don't always > prompt for password, 2011-11-04), but I think that is a red herring. It > worked before that commit because we always asked for the password, > before we even talked to the server. > > After that commit, we should be reacting to an HTTP 401. But it seems that > Google Code's 401 behavior is odd. When t5540 checks this, the > conversation goes something like: > > 1. We do a GET with no "Authorization" header. > > 2. The server returns a 401, asking for credentials. > > 3. Curl repeats the GET request, putting the username into the > Authorization header. > > 4. The server returns a 401, again, as our credential is not > sufficient. > > 5. Curl returns the 401 to git; git prompts for the credential, feeds > it to curl, and then repeats the request. It works. > > But with Google Code, the first three steps are identical. But then for > step 4, the server returns this: > > < HTTP/1.1 200 OK > < Content-Type: application/x-git-receive-pack-advertisement > < X-Content-Type-Options: nosniff > < Date: Fri, 15 Mar 2013 11:43:14 GMT > < Server: git_frontend > < Content-Length: 175 > < X-XSS-Protection: 1; mode=block > < > * Connection #0 to host code.google.com left intact > packet: git< # service=git-receive-pack > packet: git< 0000 > packet: git< ERR Invalid username/password [...] > > That seems kind of crazy to me. It's generating an HTTP 200 just to tell > us the credentials are wrong. Which kind of makes sense; it's the only > way to convince the git client to show a custom message when it aborts > (rather than producing its own client-side "authorization failed" > message). Actually the reason here wasn't to use a custom message. It was to avoid the client from entering into the old /info/refs fallback path that existed between 703e6e76 (Git 1.6.6.2) and 6ac964a62 (Git 1.7.12.3). During these versions non-200 responses from the smart request usually led to a useless error in the client. I suggested to the Google Code team when they implemented Git support to use a 200 response with the ERR header to give the end-user something easier to understand than what the Git client have otherwise printed. But in hindsight maybe I should have told them to always return 401 and let the client handle the error. FWIW this same "misfeature" probably exists in Gerrit Code Review and does exist on {gerrit,android}.googlesource.com because it also came from me. > But it takes the retry decision out of the client's hands. And > in this case, it is wrong; the failed credential is a result of curl > trying the username only, and git never even gets a chance to provide > the real credential. > > Technically this did work before 986bbc0, so it could be considered a > regression in git, but I really think that Google Code is in the wrong > here. It should either: > > 1. Always return a 401 for bad credentials. This means it would lose > the custom message. But I think that is a good indication that the > client should be putting more effort into showing the body of the > 401. Probably not all the time, as we do not want to spew HTML at > the user, but we could perhaps relay error bodies if the > content-type is "application/x-git-error" or something. > > The downside is that even if we make such a client change and the > the Google Code server change sit's behavior, people on older git > clients will lose the benefit of the message. I don't think this is the way to handle errors in the protocol. I prefer the existing approach of sending a 200 OK with the ERR header to indicate the message to show to the client. It works since 1.6.6 when smart HTTP was introduced, and it has a very specific meaning. The 200 is stating the transport worked, and the ERR is stating the Git operation did not. :-) Where we have an issue is on a recoverable authentication error. Recoverable authentication errors should use 401 so the client can try again. I don't know how older (e.g. 1.6.6) clients behave here with a 401 response. I guess I should crack out a 1.6.6 build and test. > 2. Treat a credential with a non-empty username and an empty password > specially, and return a 401. This covers the specific case of > https://user@host/, but continues to show the custom message when > the user provides a wrong password. It does mean that the custom > message will not be shown if the user actually enters a blank > password at the prompt (but it will still be shown if they get > prompted for both username and password and leave both blank). > > So it's a little hacky, but I think it would work OK in practice. I don't work on Google Code, but I have asked the team to consider making at least this change. We haven't deployed it yet, but I made the change for {android,gerrit}.googlesource.com, and should try to get the same thing into Gerrit Code Review. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html