Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve detection of file extensions in CurlDownloadStrategy #3916

Merged
merged 2 commits into from Mar 21, 2018
Merged

Improve detection of file extensions in CurlDownloadStrategy #3916

merged 2 commits into from Mar 21, 2018

Conversation

claui
Copy link
Contributor

@claui claui commented Mar 13, 2018

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The issue

Sometimes, brew cask fetch/install fails with an error message similar to this:

==> Downloading https://w3g3a5v6.ssl.hwcdn.net/upload2/game/214692/735
Error: Download failed on Cask 'steamed-hams' with message: Operation
not supported @ rb_sysopen -
/Users/claudia/Documents/dev/brew/var/homebrew/locks/steamed-hams--1.0
.com&Expires=1520937180&Signature=CGmDulxL8pmutKTlCleNTUY%2FyO9Xyl5u9y
VZUE0uWrjadjuz67Jp7zx3H7NEOhSyOhu8nzicEHRBjr3uSoOJzwkLC8LBLKnz%2B2X%2B
iq5m6IdwSVFcLp2Q1Hr2kR7ETn3rF1DIq5o0lHCyzMmyNe5giEKJNW8WF0KXriULhzLTWL
SA3ZTLCIofAdRiiGje1kNYY3C0SBqymQB8CG3ONn5kj7CIGbxrDOq5xI2ZSJdIyPysSX7S
LvEDBw2KdR24q9t1wfjS9LUzelf5TWk6ojj8p9%2FHjl%2Fi%2FVCXNN4o1mW%2FMayy2t
TY1qcC%2FTmqI1ulZS8SNuaSgr9Iys9oDF1%2BPK%2B4Sg==&hwexp=1520937440&hwsi
g=55bc66884b925ef22f8673c33bfcc33b.incomplete.lock

Steps to reproduce (in the lab)

To reproduce the issue, check out this branch and run the cask/download_strategy test suite:

brew tests --only=cask/download_strategy

Steps to reproduce (in the wild)

One real-life example would be a Cask whose URL has 1. no . character anywhere in the URL path itself (outside of the domain name), and 2. at least one query parameter with a . character in it; and 3. other query parameters following that with a combined length of more than 255 characters.

This combination may be uncommon but it exists, especially in URLs that change on every visit, for example steamed-hams from my claui/cask-games tap:

$ brew tap claui/cask-games

$ brew cask fetch steamed-hams

Analysis

In a nutshell, URL query strings sometimes look like a very long file extension to Homebrew, which it then proceeds to use as a file name.

In CurlDownloadStrategy, Homebrew seems to apply a heuristic to the cask URL in order to figure out a possibly meaningful file extension. Sometimes this heuristic produces immensely long file extensions, especially when there’s a query parameter with a . character in it but not in the URL path itself (outside of the domain name).

Homebrew then believes that everything after the . is a file extension. In one of the later steps, it tries to create a lock file containing that extension, which fails because HFS+ cannot handle files whose base file name has more than 255 characters.

Fix

This PR improves Homebrew’s extension detector a bit so that it won’t cross individual URL query param boundaries any longer. This is done by adding & as a stop character:

def ext
  Pathname.new(@url).extname[/[^?&]+/]
end

This appears to fix the issue for most (if not all) practical purposes.

Sometimes, `brew cask fetch`/`install` fails with an error message
similar to this:

```
==> Downloading https://w3g3a5v6.ssl.hwcdn.net/upload2/game/214692/735
Error: Download failed on Cask 'steamed-hams' with message: Operation
not supported @ rb_sysopen -
/Users/claudia/Documents/dev/brew/var/homebrew/locks/steamed-hams--1.0
.com&Expires=1520937180&Signature=CGmDulxL8pmutKTlCleNTUY%2FyO9Xyl5u9y
VZUE0uWrjadjuz67Jp7zx3H7NEOhSyOhu8nzicEHRBjr3uSoOJzwkLC8LBLKnz%2B2X%2B
iq5m6IdwSVFcLp2Q1Hr2kR7ETn3rF1DIq5o0lHCyzMmyNe5giEKJNW8WF0KXriULhzLTWL
SA3ZTLCIofAdRiiGje1kNYY3C0SBqymQB8CG3ONn5kj7CIGbxrDOq5xI2ZSJdIyPysSX7S
LvEDBw2KdR24q9t1wfjS9LUzelf5TWk6ojj8p9%2FHjl%2Fi%2FVCXNN4o1mW%2FMayy2t
TY1qcC%2FTmqI1ulZS8SNuaSgr9Iys9oDF1%2BPK%2B4Sg==&hwexp=1520937440&hwsi
g=55bc66884b925ef22f8673c33bfcc33b.incomplete.lock
```

To reproduce the issue, check out this branch and run the
`cask/download_strategy` test suite:

```
brew tests --only=cask/download_strategy
```

Or take a real-life example, which would be a Cask whose URL has
**1.** no `.` character anywhere in the URL path itself (outside of
the domain name), **and 2.** at least one query parameter with a `.`
character in it; **and 3.** other query parameters following that
with a combined length of more than 255 characters.

This combination may be uncommon but it exists, especially in
[URLs that change on every visit](https://github.com/caskroom/homebrew-cask/blob/1002d41242216ce7c71597765dc0a86cabd1fcf7/doc/cask_language_reference/stanzas/url.md#urls-that-change-on-every-visit),
for example
[`steamed-hams`](https://github.com/claui/homebrew-cask-games/blob/9d7df499cdc3665ca2c68a555e9ff9826a2200e7/Casks/steamed-hams.rb)
from my `claui/cask-games` tap:

    $ brew tap claui/cask-games

    $ brew cask fetch steamed-hams

In a nutshell, **URL query strings sometimes look like a very long
file extension to Homebrew,** which it then proceeds to use as a file
name.

In `CurlDownloadStrategy`, Homebrew seems to apply a heuristic to the
cask URL in order to figure out a possibly meaningful file extension.
Sometimes this heuristic produces immensely long file extensions,
especially when there’s a query parameter with a `.` character in it
but not in the URL path itself (outside of the domain name).

Homebrew then believes that everything after the `.` is a file
extension. In one of the later steps, it tries to create a lock file
containing that extension, which fails because HFS+ cannot handle
files whose base file name has more than 255 characters.

One solution would be to improve Homebrew’s extension detector a bit
so that it won’t cross individual URL query param boundaries any
longer. This is done by adding `&` as a stop character:

```
def ext
  Pathname.new(@url).extname[/[^?&]+/]
end
```

This appears to fix the issue for most (if not all) practical
purposes.
This commit improves Homebrew’s extension detector in `cask/lib/hbc/download_strategy.rb` a bit
so that it won’t cross individual URL query param boundaries any
longer:

```
def ext
  Pathname.new(@url).extname[/[^?&]+/]
end
```
@MikeMcQuaid
Copy link
Member

Looks good to me! It would be really good if we could get Cask using Homebrew's download strategies, though!

@MikeMcQuaid MikeMcQuaid merged commit 799a05c into Homebrew:master Mar 21, 2018
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants