8 Commits

Author	SHA1	Message	Date
ignacionelson	d6fd5a917d	Send downloads the way the web server in front of us understands ... 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.	2026-08-31 22:31:27 -03:00
ignacionelson	5117511946	Merge pull request #1732 from denkfabrik-li/fix/public-preview-log-debounce ... FileThumbnailController::preview() writes at most one FilePreviewed row per viewer per file per five minutes, because watching a video is a single deliberate act that the browser turns into dozens of Range requests. Its docblock ended by naming the route where the same act happens without an account -- PublicGroupsController::preview -- and that route logged unconditionally. Five requests for the same public file wrote five rows where the signed-in twin wrote one, so one visitor watching one clip buried the public half of the activity log, which is the half an operator reads to see what the outside world is doing. The window moves into a shared PreviewLog, next to PreviewKind, which those two routes already share for the same reason. Keying is unchanged for a signed-in viewer. An anonymous visitor has no account to key on, so the request IP stands in -- the same substitute ApiServiceProvider's rate limiter makes for an unauthenticated caller. It is a cache key with a five-minute life and never reaches the log, which keeps its own decision about recording an IP. Downloads are deliberately untouched and stay one row per download: each is a transfer, and DownloadAllowance::used() counts those rows to enforce a per-file cap, so swallowing one would hand out free downloads. The limit this leaves open, since the IP is a stand-in and not an identity: two anonymous visitors behind one address share a key, so within five minutes the second one's view of the same file is not recorded. That is the same trade the signed-in side has always made per account, and the alternative is the row-per-Range-request this fixes. Verified before merging: 24 passed across the public-preview and thumbnail suites on the trial-merge, which also confirms this co-exists with #1725 -- the two share both controllers and change different methods in each. With app/ reset and PreviewLog deleted, 1 failed / 9 passed. The signed-in route's existing debounce tests pass unchanged, which is what says the shared class did not move that side. request()->ip() honours the trusted-proxy configuration, so a forged X-Forwarded-For cannot defeat the window from outside. Reported and fixed by @denkfabrik-li.	2026-08-28 17:13:05 -03:00
denkfabrik-li	fc758c701a	Write a rendition through a temporary file, and never serve an empty one ... Both thumbnail routes treat "the file exists" as "the rendition is cached", and nothing ever invalidates one: RenderedImageCache::flush() runs on ImageRenderingChanged, which no core code raises. Whatever is at the path is what every later viewer gets. ThumbnailGenerator encoded straight onto that path. A render that died partway -- a full volume, a killed worker -- left a half-written file that was then served as the rendition for good, and two requests rendering the same file at once encoded into one path together. It now writes beside the destination and renames into place. rename() within a directory is atomic and replaces what is there, so the path is either the previous rendition or a complete new one, and the loser of a race leaves a whole image rather than a mixture of two. The temporary file is removed on the way out either way. The read side gets the other half: an empty file is not a rendition, so both routes replace one rather than serve it. Writing through a temporary file means this state can no longer be created here, but an installation that ran an older version can already have it on disk, and nothing else will ever clear it. Three tests: an empty rendition is replaced on the signed-in route and on the public one, and a successful render leaves nothing half-written behind. Without the fix the first two go red; the third is about the fix's own temporary file and passes either way.	2026-08-28 06:40:48 +02:00
denkfabrik-li	c2dd2c758a	Debounce the public preview log the way the signed-in one already is ... FileThumbnailController::preview() writes at most one FilePreviewed row per viewer per file per five minutes, because a browser turns one video into a long tail of Range requests against the same URL. Its docblock names the anonymous route as the place the same act happens without an account -- and that route logs unconditionally. Measured: five requests for the same public file, five PublicFilePreviewed rows, against one for the signed-in twin. One visitor watching one clip buries the public half of the activity log, which is also the half an operator reads to see what the outside world is doing. The window is now a shared PreviewLog, next to PreviewKind, which the two preview routes already share for the same reason. Keying is unchanged for a signed-in viewer; an anonymous one has no account to key on, so the request IP stands in -- the same substitute the API's rate limiter makes for an unauthenticated caller. It is a cache key with a five-minute life and never reaches the log, which keeps its own decision about recording an IP (ActivityLogger::shouldRecordIp, Setting::DownloadIpLogging). Three tests: the replay is one row, two visitors are two rows, and the window is per file. Without the fix the first goes red.	2026-08-28 06:40:45 +02:00
denkfabrik-li	a8b1987e2c	Hand over a public download from the disk the file is on ... 5754016 moved this controller's thumbnail() and preview() onto StoredFileResponse and left download(), the last method in the same class, building its own response: 'X-Accel-Redirect' => '/protected-files/'.$file->path, That prefix is nginx's internal location for the local files disk, and $file->disk is never consulted. On an install with external storage switched on it names a path nothing ever wrote, so the public download fails — while the same file downloads correctly from the file manager and from a share link, and previews correctly from this very page, because all three go through the object that knows the rule. That commit's own message names the shape: 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". It is an object now, and this is the call site that was not moved onto it. StoredFileResponse is already injected here as $this->bytes — preview(), two methods above, uses it — and attachment() is the method FileDownloadController and PublicShareController already call. Nothing changes for a local install: attachment() emits the same four headers this method wrote by hand, through the same ContentDisposition call. The return type widens to Response|RedirectResponse because a non-local disk answers with a redirect to a presigned URL, which is the signature preview() already declares. The regression test fails against the unfixed controller — checked in both directions rather than assumed. The existing local-disk case grew assertions for the other three headers, so "unchanged for local" is pinned rather than argued: it passes before and after.	2026-08-26 03:20:28 +02:00
ignacionelson	57540164fa	Read a file from the disk it is actually on, everywhere ... 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.	2026-08-24 16:24:39 -03:00
ignacionelson	88c182cf3b	Preview video, audio and PDF, not only images ... v1 could preview four kinds of file in a modal — images, video, audio and PDF. v2 previewed only images, and not by decision: preview shipped as part of the image *thumbnail* work (1c68aa1), so "previewable" quietly became a synonym for "GD can decode it". FileThumbnailController::preview() gated on ThumbnailGenerator::SUPPORTED_MIME_TYPES, the frontend mirrored the same four types, and the dialog was a hardcoded <img>. Rather than widen that list — it drives pathFor(), extensionFor(), generate() and FileDiskCleanup, and a video reaching getimagesize() is a 500 — this separates the two questions. PreviewKind now answers "may these bytes be served inline, and what element renders them?", while ThumbnailGenerator keeps answering the narrower "can this app decode it itself?", which is what renditions, the cache and the watermark hook actually depend on. Image delegates to it so the two cannot drift. The allowlist stays a security boundary: mime_type is sniffed from the bytes, so text/html and image/svg+xml remain excluded, and PreviewKind is deliberately narrower than "formats a browser might cope with" — no quicktime, avi or matroska, because an embedded player for those shows a black rectangle. Those still download exactly as before. docs/security-audit-2026-08-05.md finding 1 recorded that adding application/pdf "should be a conscious decision". This is that decision, and three things were measured rather than assumed: - An <iframe sandbox> cannot be used. Chrome refuses to run its PDF viewer in a sandboxed frame at all (ERR_BLOCKED_BY_CLIENT, with or without allow-same-origin) — the attribute removes the feature, it does not harden it. - nginx's `Content-Security-Policy: sandbox; default-src 'none'` on /protected-files/ does work (a <video> frame lands in an opaque origin), but Chrome exempts its PDF viewer from it, so it is not what protects the PDF case. - What does is the allowlist plus the browser's own PDF sandbox, where PDF JavaScript has no DOM and no cookies. Range requests were verified end to end: 206 with a correct Content-Range, a byte-perfect file reassembled from three ranges, and a real browser seeking to 10s of a 20s clip. nginx drops the upstream Content-Length on the X-Accel path, so there is no collision. Two settings, both defaulting on so no installation loses what it has: clients_can_preview_files and public_listing_preview_enabled. Staff are never gated. The anonymous side needed a route of its own — there was no public preview endpoint — with its own throttle bucket, since a bare throttle: shares one counter across that whole block. A preview now logs at most one FilePreviewed per viewer per file per five minutes: a <video> turns one deliberate act into a long tail of Range requests, and a row each would bury the log. Also fixes a layout bug the tests could never catch. A portal file row was flex justify-between with three children — name, comment trigger, download — so the middle one settled wherever the name happened to end and the comment icon sat at a different place on every row. The name block now takes the slack and every action lives in one trailing group, with the comment trigger in a fixed-width slot so the icons form a column. And because half the previewable files have no thumbnail to click — a PDF, an mp3 and an mp4 all render as a generic icon — every row gains an explicit PreviewAction beside DownloadAction, matching whatever style that theme gives its download control.	2026-08-21 14:14:23 -03:00
ignacionelson	6e47d76ba6	ProjectSend 2.0.0 ... Client file sharing, rebuilt from the ground up: a private area per client, resumable uploads, folders, groups and categories, sharing with expiry dates and download limits, comments, file versions, an activity log, a REST API, and sixteen languages. This repository begins here. ProjectSend 2 was developed privately, and that development history is not published — the previous generation remains available, with its own history, at projectsend/legacy. Free software under the GNU General Public License v2, or (at your option) any later version.	2026-08-14 01:38:12 -03:00