Problem/Motivation

On 3.1.x, DownloadManager::fetchResponse() treats every requested file as an image:

  • it guesses the extension from a fixed list of image extensions, and
  • it accepts a response only when its Content-Type starts with image/, otherwise it throws Image could not be detected.

Guessing the extension is right for an image style derivative, whose source may have been converted (.jpg to .webp/.avif). It is wrong for everything else: no path in the module, drush stage-file-proxy:dl included, can fetch a video, a subtitle track, a PDF or an HTML file.

4.0.x does not have this defect - its fetchResponse() already branches on $is_image and fetches any other file under the name it was requested with. Only 3.1.x has to catch up.

Steps to reproduce

  1. Configure Stage File Proxy with an origin, in download mode.
  2. Request a file that is not an image and does not exist locally - an asset of an extracted SCORM package, a video, a PDF - or run drush stage-file-proxy:dl for it.
  3. Nothing is downloaded, and the log records Image could not be detected.

Proposed resolution

Backport the 4.0.x structure to 3.1.x:

  • try the extension list only when the requested file is an image, and fetch any other file under the name it was actually requested with;
  • keep the image/ Content-Type check for images.

Unlike 4.0.x, which accepts any 200 for both cases, the guard that the content-type check provides is kept for other files too: a web server that cannot serve a file may still answer 200 with an HTML error or login page, and storing that under the requested name would cache the wrong bytes. HTML is therefore accepted only when HTML is what was requested.

Relation to #3490259 and the base of the merge request

The merge request starts from 3a5d2e4 on the branch of !99 (#3490259) - the iteration of that issue which still applies to the 3.1.6 release - so it shows that merge request's commits up to that point as well. Only the last commit belongs to this issue.

Since that iteration, !99 has moved on: it merged 4.0.x into its own branch, and its head now carries an $is_image split of its own. The extension half of this issue therefore already exists there. What !99 does not have is the guard described above - its head returns on any 200 for a non-image, so an HTML error or login page is still stored under the requested file name.

If !99 lands in its current shape, the useful remainder of this issue is that guard alone, and the merge request should be reduced to it. If instead a 3.1.x-only iteration of !99 is what lands, this issue supplies the missing half.

Remaining tasks

  • Decide with the maintainers how this relates to !99 - reduce to the content-type guard, or keep the full backport.
  • Review the merge request.
  • Test coverage for the non-image path.

User interface changes

None.

API changes

None. fetchResponse() is protected and keeps its signature.

Data model changes

None.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

daniel.pernold created an issue. See original summary.

smustgrave’s picture

Version: 3.1.x-dev » 4.0.x-dev

Fixes need to land in 4.0.x first

daniel.pernold’s picture

Issue summary: View changes
Status: Active » Needs review
daniel.pernold’s picture

Version: 4.0.x-dev » 3.1.x-dev
smustgrave’s picture

Version: 3.1.x-dev » 4.0.x-dev
Status: Needs review » Needs work

#2 still stands.