Chrome ERR_INVALID_RESPONSE on unauthorized Git archive download

Zendesk: https://gitlab.zendesk.com/agent/tickets/15148

Previous issue: https://gitlab.com/gitlab-org/gitlab-workhorse/issues/23

When trying to visit a link to an archive download in Chrome for a project that the I have no access to I see an unfriendly Chrome error page.

My best guess why this happens is because of an incorrect Content-Type header on the response.

% 	curl -v 'http://localhost:3000/root/grrr/repository/archive.zip?ref=master' 
*   Trying ::1...
* connect to ::1 port 3000 failed: Connection refused
*   Trying fe80::1...
* connect to fe80::1 port 3000 failed: Connection refused
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 3000 (#0)
> GET /root/grrr/repository/archive.zip?ref=master HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.43.0
> Accept: */*
> 
< HTTP/1.1 401 Unauthorized
< Cache-Control: no-cache
< Content-Type: application/zip; charset=utf-8
< Date: Tue, 02 Feb 2016 10:20:09 GMT
< Status: 401 Unauthorized
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-Request-Id: f90f8c27-bada-4621-ba8b-533ba315562a
< X-Runtime: 0.074683
< X-Xss-Protection: 1; mode=block
< Content-Length: 49
< 
* Connection #0 to host localhost left intact
You need to sign in or sign up before continuing.% 

Note that the content-type is 'application/zip' but the response body is plain text.

It seems that the 401 response is generated by Devise. I have tried using http://stackoverflow.com/a/18792108 to modify the content-type but my special after_action in SessionsController never got called.

Devise has an interesting test case that might explain what is happening: https://github.com/plataformatec/devise/blob/dd5de829c944dd955e4b35074c60798c3ad4b947/test/integration/http_authenticatable_test.rb#L41

  test 'uses the request format as response content type' do
    sign_in_as_new_user_with_http("unknown")
    assert_equal 401, status
    assert_equal "application/xml; charset=utf-8", headers["Content-Type"]
    assert_match "<error>Invalid email or password.</error>", response.body
  end

I think the actual behavior is happening in https://github.com/plataformatec/devise/blob/v3.5.4/lib/devise/failure_app.rb (the Devise 'failure app').

At the moment it is unclear to me how to override the self.content_type = request.format.to_s behavior of Devise.

cc @patricio

mentioned in issue gitlab-workhorse#23 (closed)

@patricio I am not sure what to try next. Maybe somebody else on our team has more experience with Devise than me.

@rspeicher do you know more about Devise and can help us here? Or know of someone within the team that has the required expertise?

Not off the top of my head but I'll dig into it a bit.

mentioned in merge request !2828 (merged)

When you attempt to access an internal or private archive as a guest, the calls go like this:

1. :authenticate_user! in ApplicationController calls super which goes to...
2. Devise::Controller::Helpers dynamic method which calls...
3. Warden::Proxy#authenticate! which throws, and then...
4. (a bunch of internal stuff happens)
5. We end up in Devise::FailureApp#respond:

• http_auth? returns true because this line evaluates to !(:zip && false)
• http_auth does the damage by setting content_type and response_body.

The solution I came up with can be seen in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2828. We override the request_format accessor and basically return :html when it would have been :zip, which allows the standard FailureApp behavior to navigate the user to the signin page.

This feels super hacky to me, but works.

Another solution was to add :zip to the list of navigational formats but then when you tried to access the archive it redirected to /users/sign_in.zip, which is fixable but also would probably require some hackiness.

On another note, ideally shouldn't we be rendering a 404 so as to not leak the existence of the repository?

Maybe in the long run we need an ArchivesController that only handles archive downloading and that inherits from a metal ActionController instead of our ApplicationController with all its before_action madness, and then we can carefully define its access.

@jacobvosmaer @patricio Any thoughts on my hacky solution?

Ok. Guess this wasn't as important as it seemed.

@rspeicher that sounds ingenious but I am not sure if the 'archive download' feature is worth the complexity. Especially if you consider that we would be working around our authentication system (Devise), which brings a risk of breaking security.

@jacobvosmaer It's not quite working around it -- a custom failure app is a supported feature of Devise. See the linked MR for details.

@rspeicher I commented in !2828 (merged), it looks great. The complexity and risk feels proportional to the problem we are trying to solve.

mentioned in commit 5844a21a

Status changed to closed by commit 5844a21a

mentioned in commit cb81c8a5

Reassigned to @rspeicher

Milestone changed to 8.6

Mentioned in commit pfjason/gitlab-ce@5844a21a

Mentioned in commit pfjason/gitlab-ce@cb81c8a5

mentioned in merge request !13136 (merged)