React to non-200 HTTP status codes

[radeksimko]

The current implementation of both GET & POST downloads allows downloading response, which is not 2xx. If there's SHA2 check in place for a cask, then it ends up in confusing error with SHAsum mismatch (not even noticing anything about original HTTP code).
I'm not sure what will happen in case of :no_check casks... tries of mounting HTML files like dmg or moving HTML file to ~/Applications? :)

I'm not entirely sure whether it should be limited to 200 only, but it should definitely react to codes like 5xx (e.g. 503).

The current implementation basically uses Curl with -f parameter, but as even the manual says "This method is not fail-safe and there are occasions where non-successful response codes will slip through".

-f, --fail
              (HTTP) Fail silently (no output at all) on server  errors.  This
              is  mostly done to better enable scripts etc to better deal with
              failed attempts. In normal cases when an HTTP  server  fails  to
              deliver  a  document,  it  returns  an  HTML document stating so
              (which often also describes why and more). This flag  will  pre-
              vent curl from outputting that and return error 22.

              This  method is not fail-safe and there are occasions where non-
              successful response codes will  slip  through,  especially  when
              authentication is involved (response codes 401 and 407).

It will react with meaningful messages to traditional error status codes like 404, but I just ran a test with 503 and it really downloads the remote page and stops on SHA2 mismatch. It shouldn't even get to any SHA2 checks at the first time.

Related: #7882

Maybe we can run a simple/quick HEAD request via curl -I -L first and check the HTTP code or are there any different/better plans for this?

Any suggestions about acceptable HTTP codes except 200?

[radeksimko]

btw. there are plans to remove the -f option from curl in the future:
https://github.com/bagder/curl/blob/2e05db347eb334a3f310fa18f5e35d5dd5937231/docs/TODO#L116

I believe it's being used quite heavily in many places, so I'd not expect it to be gone immediately, but we should keep that in mind as it says "Next major release".

[iggyvolz]

Also, couldn't we just check the MIME-Type just as a backup? Either make each cask specify a mime-type (e.g. mimetype "text/html" in the cask file) or auto-detect it from the installation method (or even better - allow a cask to specify the mime-type but fallback on the installation method).

[radeksimko]

@iggyvolz That could be also a solution, but I'm not convinced that it's better as you'll be downloading a file, which will be potentially trashed afterwards based on detected mime-type.

More importantly I'd say that user should get a meaningful message in all cases and if we download a file and then show an error message e.g. "Invalid MIME-type (expected application/x-apple-diskimage, text/html given)", I'm not sure it will be obvious what's the core of the problem.

I'd rather see a message saying something like "Invalid response from server (503 Service Unavailable)".

We can eventually check the Content-Type header, but I'm not sure how many servers will actually provide correct header -> not sure how much we can rely on that.

[Miladiir]

As stated in #7882 I was getting a non standard file served by Sourceforge, which in turn made the installation fail.

Sourceforge was in maintanance mode, which only serves specific projects and their files. I am not sure wether they return a proper status code, but if so, it would be almost mandatory to return a useful error and prevent idiots like me from thinking that the formula is broken.

[rolandwalker]

@radeksimko the analysis sounds right, and #7882 looks like a bug, but I'm not sure about the immediate fix. Maybe a HEAD request is helpful for now as you suggest. There will be a problem with false positives if the HEAD is done first. We might try doing the HEAD retrospectively iff the checksum fails.

Our download_strategy.rb doesn't contain much. It works by abusing Homebrew's classes and then calling into Homebrew to effect the download. The -f flag is originating from HOMEBREW_CURL_ARGS which is outside our project.

That entire architecture is a bad idea for many reasons. For example, our downloads and Homebrew's downloads end up in the same place, which is hard to manage. Per #5080, the best answer is to untangle our code from Homebrew's. That wouldn't fix this problem, but it would make it more possible for us to work on it.

[vitorgalvao]

Closing.

We’ll be merging with Homebrew, so this type of logic belongs there. Also, I’ve built a number of scripts that are in use to manage casks and other operations, and those also check the status code. Not every server responds to those requests, and while a good indicator to have when developing, it’s not good enough to use in production (i.e to affect the user). It is also not a common occurrence, and our usual methods of detection work fine.