#1733 explains its two lifetimes by contrasting a presigned URL with
X-Accel-Redirect, "nginx serves these bytes, now, to this request". That
was true when the branch was written and stopped being true on 1 September,
when FileDelivery gave the local path four methods — auto, nginx, xsendfile
and PHP streaming.
The argument survives intact: every one of those authorises exactly one
response and nothing that outlives it, which is the property the contrast
rests on. Only the naming was stale, and a docblock that says "nginx" to
an operator running Apache reads as "this does not apply to me".
Found resolving the merge, not by the author — the branch predates the
change it collided with.
Give a download's presigned URL a minute rather than an hour
Conflicted against FileDelivery, which landed on main after this branch
was written: main added a constructor where the branch added two
constants. Both belong; the resolution keeps each.
Uploads live outside the web root, so PHP authorizes every download and
then hands the file to the web server with a header naming it. Four
routes decided that for themselves and all four hard-coded nginx's
spelling. On Apache or LiteSpeed nothing acts on the header, so the
empty body PHP sent goes to the visitor: files upload fine, thumbnails
are broken images, and downloads arrive as 0 bytes, with every other
page working. Reported as #1765 from an Apache 2.4 install, and before
that as #1266, #1215, #870 and #1271.
It is also a regression from v1, which had a download_method setting --
php, apache_xsendfile, litespeed, nginx_xaccel -- defaulting to php. v1
therefore worked on any server out of the box and v2 did not, and a v1
Apache user migrating lost every download with nothing to tell them why.
So the four sites now go through one FileDelivery, and it picks:
auto (default) nginx when SERVER_SOFTWARE says nginx, else php
nginx X-Accel-Redirect, a URL path via the internal location
xsendfile X-Sendfile, an absolute path (Apache mod_xsendfile,
LiteSpeed)
php BinaryFileResponse
Defaulting to auto rather than nginx is the point of the change: a
default that assumes nginx leaves an Apache install exactly as broken as
it is today until somebody reads INSTALL.md. Slow beats empty.
Auto never picks xsendfile, even where the module is loaded.
mod_xsendfile also needs XSendFilePath to allow the storage directory,
which cannot be seen from here, and choosing it on the strength of the
module being present would trade a silent failure an administrator can
diagnose from the dashboard for one nobody can.
BinaryFileResponse rather than a readfile loop because it answers Range
requests. nginx does that itself on the fast path, so hand-rolling it
would have broken seeking through a video on exactly the installations
this fallback exists for. Verified end to end: 206 with the right
Content-Range through the live stack.
Two guards. Every method checks the path cannot climb out of the storage
area -- nginx resolves `..` in the URL it is handed as happily as PHP
would -- and the two methods that hand over a filesystem path resolve it
and prove it lands inside the root. Callers pass paths from rows they
just authorized, so this is a backstop; it is here because the cost of
being wrong once is handing over any file the web server can read.
The dashboard's System panel names the method, with a warning icon and a
dialog when PHP is doing the sending: what is happening, what it costs
(one worker held for the whole of each download, so a few large
simultaneous ones can occupy every worker while the processor sits
idle), why it is set that way, and the three ways out. Written to be
accurate rather than reassuring -- nothing is broken, it does not scale
-- and the notice stays even when php was chosen deliberately, because
the trade-off is the same either way. /system/settings/downloads repeats
it, which is where somebody coming from v1 goes looking for the
dropdown.
An environment variable rather than a stored setting: it describes the
server this installation runs on, not a preference, and a value in the
database travels to a different server in a restore and is wrong there.
Read only in config/projectsend.php, so config:cache cannot blank it.
The suite pins itself to nginx. Left at auto it would detect no server
at all, fall back to php, and quietly retire the coverage of the
mechanism most installations actually use.
StoredFileResponse hands external storage a presigned URL for an hour,
whatever the delivery is for. That URL is a bearer credential: whoever
holds it fetches the file without passing any of the caller's checks
again, and it outlives them. A download cap spent in the meantime, an
expires_at that falls inside the hour, an assignment withdrawn -- none of
them reach it, and nothing here can revoke one. It is also forwardable,
which the local path is not: X-Accel-Redirect authorises one response to
one request.
The two deliveries do not need the same window, so they no longer share
one.
A download has to survive being followed -- a redirect and a request --
which a minute covers with room to spare. An object store checks the
signature when the request arrives rather than while it runs, so a
transfer that starts inside the window finishes however long it takes.
A preview keeps the hour, because it is watched rather than fetched: the
player holds the URL and issues a Range request every time somebody seeks
past the buffer, so a minute would break playback of anything longer than
a minute. The class docblock now says that this is the trade being made,
instead of leaving it in a single number.
Two tests, one per window. Without the fix the download link is an hour
long.
Two routes still assumed every file sits on local disk, which stopped
being true the moment external storage was switched on. A share link
answered with X-Accel-Redirect whatever the file's disk said, pointing
nginx at a path it has nothing behind; a public listing built a
thumbnail from Storage::disk('files')->path(), which for an externally
stored file is a path nobody ever wrote. Both fail only for installs
using S3, and only on those two routes, so the same file downloading
correctly from the file manager made the share link look like the
broken thing rather than where the file lives.
Neither is a new rule. FileDownloadController and
FileThumbnailController already did it right, which is the actual
finding: the knowledge was sitting in a private method on one class and
inline in another, so the next caller could not inherit it and did not.
Both are now objects with one job.
StoredFileResponse replaces InlineFileResponse and grows an
attachment() alongside inline(), since the two differ only by
disposition. LocalSourceFile takes a closure rather than returning a
path: the version that returned one also left the caller to unlink it,
and both of those are exactly the mistakes made here.
The regression tests fail against the previous controllers — checked in
both directions rather than assumed.
ignacionelson