`Livecheck::Strategy.page_headers` uses `Utils::Curl.curl_headers` but
the method only handles `HEAD` and `GET` requests. I recently added
`POST` support to livecheck but forgot to update `curl_headers` in the
process, so `livecheck` blocks using the `HeaderMatch` strategy along
with `post_form` or `post_json` will fail because curl doesn't allow
both `--head` and `--data`/`--json` arguments.
This addresses the issue by updating `curl_headers` to handle `POST`
requests and skip the `GET` retry logic.
This adds more tests to `curl_spec.rb` to increase test coverage.
This brings almost all of the methods that don't make network
requests up to 100% line and branch coverage (the exception being
some guards in `parse_curl_output` that shouldn't happen under
normal circumstances).
In the process of writing more tests for `parse_curl_response`, I
made some tweaks to remove checks for conditions that shouldn't ever
be true (e.g., `match["code"]` isn't optional, so it will be present
if `HTTP_STATUS_LINE_REGEX` matches) and to refactor some others. I
contributed this method a while back (9171eb2), so this is me coming
back to clarify some behavior.
This upgrades `utils/curl.rb` to `typed: strict`, which requires
a number of changes to pass `brew typecheck`. The most
straightforward are adding type signatures to methods, adding type
annotations (e.g., `T.let`) to variables that need them, and ensuring
that methods always use the expected return type.
I had to refactor areas where we call a `Utils::Curl` method and use
array destructuring on a `SystemCommand::Result` return value
(e.g., `output, errors, status = curl_output(...)`), as Sorbet
doesn't understand implicit array conversion. As suggested by Markus,
I've switched these areas to use `#stdout`, `#stderr`, and `#status`.
This requires the use of an intermediate variable (`result`) in some
cases but this was a fairly straightforward substitution.
I also had to refactor how `Cask::URL::BlockDSL::PageWithURL` works.
It currently uses `page.extend PageWithURL` to add a `url` attribute
but this reworks it to subclass `SimpleDelegator` and use an
`initialize` method instead. This achieves the same goal but in a way
that Sorbet can understand.
`#curl_http_content_headers_and_checksum` contains code that works
with a `Content-Type` header in a response but it expects there to
always be only one header in the response. This is normally a
reasonable assumption but we've come across a server that is giving
a response with multiple `Content-Type` headers in the response, so
this produces an error and causes `brew audit` to fail when checking
the URL.
This works around the issue by naively using the last `Content-Type`
header in the response when there's more than one. It's not something
that should normally occur but this will handle the situation when it
does.
The `curl --head --request GET` causes a full download to happen on
`curl` from 8.7.0 to 8.9.1[^1] which causes poor UX due to slow
Cask downloads that can take almost twice as long as they should.
[^1]: https://github.com/Homebrew/brew/issues/18213
I previously added the 8 curl exit code (weird server reply) to the
list of non-success exit codes that `#curl_headers` will handle.
We're now seeing failures with a 56 exit code (failure in receiving
network data), where the server returns a 4xx response for a `HEAD`
request but the same request using `GET` works as expected (e.g.,
casks like `beeper`, `get-api`, `odrive`, `ui`, etc.).
This adds 56 to the list of exit codes in `#curl_headers`, so a
response with a 4xx HTTP status will be automatically retried using
`GET`.
- Previously I thought that comments were fine to discourage people from
wasting their time trying to bump things that used `undef` that Sorbet
didn't support. But RuboCop is better at this since it'll complain if
the comments are unnecessary.
- Suggested in https://github.com/Homebrew/brew/pull/18018#issuecomment-2283369501.
- I've gone for a mixture of `rubocop:disable` for the files that can't
be `typed: strict` (use of undef, required before everything else, etc)
and `rubocop:todo` for everything else that should be tried to make
strictly typed. There's no functional difference between the two as
`rubocop:todo` is `rubocop:disable` with a different name.
- And I entirely disabled the cop for the docs/ directory since
`typed: strict` isn't going to gain us anything for some Markdown
linting config files.
- This means that now it's easier to track what needs to be done rather
than relying on checklists of files in our big Sorbet issue:
```shell
$ git grep 'typed: true # rubocop:todo Sorbet/StrictSigil' | wc -l
268
```
- And this is confirmed working for new files:
```shell
$ git status
On branch use-rubocop-for-sorbet-strict-sigils
Untracked files:
(use "git add <file>..." to include in what will be committed)
Library/Homebrew/bad.rb
Library/Homebrew/good.rb
nothing added to commit but untracked files present (use "git add" to track)
$ brew style
Offenses:
bad.rb:1:1: C: Sorbet/StrictSigil: Sorbet sigil should be at least strict got true.
^^^^^^^^^^^^^
1340 files inspected, 1 offense detected
```
Take 2 of https://github.com/Homebrew/brew/pull/17692 but with:
- provide and document `HOMEBREW_NO_VERIFY_ATTESTATIONS`
- don't try to run unless there's GitHub credentials
- don't try to run unless `gh` is installed
- don't try to run in CI
While we're here:
- split out a `Homebrew::EnvConfig.devcmdrun?` helper method
- add some missing `Homebrew::EnvConfig.github_api_token` presence
checks
I recently noticed that ~23 `livecheck` blocks using the `HeaderMatch`
strategy were failing. Looking into it, these fail when using a `HEAD`
request and retry with `GET` but the resulting response with the
headers we want is simply discarded because the `exit_status` from
curl is 8 ("weird server reply").
This resolves the issue by adding a special case for this exit status,
so `#curl_headers` will return the headers in this scenario.
`#curl_headers` was recently introduced into `Strategy#page_headers`
but only the call was modified and the method wasn't updated to
correctly work with the new return value, so all `HeaderMatch` checks
immediately started failing with an error.
This commit includes changes that return `#page_headers` to a working
state. I've removed the `result.assert_success!` call because it
prevents a few checks from being retried with `GET` (`firefox-cn`,
`krisp`, `prepros`).
- Some casks have URL arguments like "referer" (spelled wrong, that's
intentional in the HTTP spec).
- The audit for one such cask, `iThoughtsX`, was failing because the
"referer" wasn't getting passed through to cURL so the access would
404.
----
Before:
```
❯ brew audit --cask --online --appcast --signing 'ithoughtsx'
[...]
audit for ithoughtsx: failed
- The binary URL https://cdn.toketaware.com?download=iThoughtsX.zip is not reachable (HTTP status code 404)
- Version '9.2.0' differs from '9.3.0' retrieved by livecheck.
- Version '9.2.0' differs from '9.3.0' retrieved by livecheck.
Error: 2 problems in 1 cask detected
```
After:
```
❯ brew audit --cask --online --appcast --signing 'ithoughtsx'
[...]
audit for ithoughtsx: failed
- Version '9.2.0' differs from '9.3.0' retrieved by livecheck.
- Version '9.2.0' differs from '9.3.0' retrieved by livecheck.
Error: 1 problem in 1 cask detected
```
Update base URL when there is an absolute location, so that following
relative locations are considered relative to the new base.
Consider below cURL output for https://example_one.com:
HTTP/1.1 302 Moved Temporarily
Location: https://example_two.com
HTTP/1.1 302 Moved Temporarily
Location: /foo/
HTTP/1.1 200 OK
The final URL should be https://example_two.com/foo/ rather than
https://example_one.com/foo/.
