Files S3 Storage PR Review

Files in S3-compatible storage - PR review

PR #318 by cloudcogsio · Issue #283 · Verdict: merge + fix · 5.2.0

Files → Settings → Storage can now keep files in S3-compatible storage - AWS S3, Backblaze B2, Wasabi, Cloudflare R2, MinIO and others - as well as a folder on the server. Changing the location moves the files already stored, safely, in the background. User page: Files § Storage. How it works: Files storage - Developer Guide.

This is the first of a series the contributor proposed: after it come System → Storage for the other stores (#284), a recycle bin (#293), offline copies (#285), and Backups (#286).

✅ What was good - keep it this way

What	Why it matters
A driver interface, no SDK (StorageDriver, LocalDriver, S3Driver over curl)	One small seam. #284 can reuse it for every other store. No Composer, nothing to update.
A SigV4 signer checked against the AWS test-suite vector	Signing bugs are silent until a provider refuses. A known-answer test catches them first.
Reads come from the old place until a file is verified (filesStoreFor())	Nothing is ever unreadable in the middle of a move.
Copy, check size and SHA-256, write, check again at the new place	A damaged or truncated copy never becomes "the" copy.
An outage charges no attempt	An hour-long outage does not turn every file 'failed'.
Old copies are deleted only when someone chooses to	Remove and Finish are separate. Nothing is deleted by default.
The secret is encrypted, masked, never returned to the browser, and kept when the field comes back unchanged	The usual way secrets leak - an API answer or a log - is closed.
Headers only after the store answers (filesServeBytes())	An unreachable store gives a clean 503, not a broken half-download.
Upgrades are invisible	With no new setting saved, Files behaves exactly as before.
Tests for every part, database ones in a rolled-back transaction	The fixes below could be pinned with a few more checks in the same files.

🔧 What was fixed when merging - and why

The review: comment on #318. The fixes are in c6b6b4058, on top of the merge.

1. The same-place probe failed open, and cleanup trusted it 🔴

Was: storageSameLocation() returned false ("not the same place") on any error. Before removing old copies, the cleanup asked it and then deleted every verified key, without checking each new copy existed.

Could have: on two settings that are really one bucket (one bucket behind a flaky or eventually-consistent gateway, reached through two endpoint spellings), a probe that hit an error said "different". Remove then deleted the only copy of every file.

Now: storageLocationRelation() answers same, different (proved both ways) or unknown. Cleanup deletes only on different. Before each delete it checks the new copy is there at its recorded size. If it isn't, the old copy stays and the file is queued to copy again.

Lesson: before an irreversible step, a check must prove it is safe. It must not just fail to prove it is unsafe. "I could not tell" is never "go ahead".

2. Ending a move dropped rows still waiting 🟠

Was: ending a move (after Remove, or Finish) ran DELETE FROM storage_migration_items.

Could have: an upload that chose its store just before the location changed is queued by queueIfMoved(). If that happened while the move was ending, its row was deleted. The file was then read from the new place, where it never arrived (410), and nothing recorded where it really was.

Now: cleanup copies pending rows first. storageMigrateEnd() clears only finished rows, and an unfinished one keeps the move running. Files skipped as missing are named in the audit entry.

Lesson: when ending a process, delete what is finished, not everything. A row nobody expected is exactly the one that matters.

3. The address check could be got round 🟠

Was: Test connection checked the endpoint's own host name, IPv4 only, before the request.

Could have:

• A virtual-hosted request goes to <bucket>.<host>, so a bucket name could steer it to the cloud metadata address.
• A name with only IPv6 records was not checked.
• curl looked the name up again after the check (DNS rebinding).

It was blind (no redirects followed, no body shown), but still a path to the server's own credentials.

Now: every S3 request checks the host it really connects to (IPv4 and IPv6) and pins that address with CURLOPT_RESOLVE. Only http and https are allowed.

Lesson: check the address you connect to, at the moment you connect, and pin it. A check of what was typed proves little.

Smaller

• Buttons are now one word each - Test, Retry, Skip, Finish, Remove (UI guideline). Remove's confirm is red.
• The Storage tab still opens when the old location's settings cannot be decrypted (the encryption key changed). It is where someone goes to see what is wrong.
• Four tests compared Windows paths (\) with the driver's forward slashes and failed on Windows. They now normalise.
• The schema comment lists the skipped state.

Left as follow-ups

• download.php / view.php send the recorded size_bytes rather than the file's real size. A truncated file would give a response shorter than its Content-Length.
• Without ListBucket rights, S3 answers 403 rather than 404 for a missing key, so Skip cannot tell that file is missing.
• The permission. Changing storage needs Files → Storage in System → Roles, which is marked sensitive. It is also included in the Files "manage everything" umbrella. Whoever holds it can have every file copied past the folder permissions. The plan is a dedicated Storage and backups permission, never ticked by "Select all", with System → Storage (#284) and Backups (#286).

🧪 What was checked

Question	How	Result
His tests, unchanged code	prtest, Windows	Failed only on path separators (fixed in the tests)
His tests and the new checks, after the fixes	prtest	local driver 12, S3 signing 34, backend 11, settings 16, migration 27 - all pass
A real upload and download through the endpoints	prtest, an administrator's session	Exact bytes back; a range request gives 206 and just the range; an analyst without a grant on the folder gets 403
Generated index file	gen_db_verify_indexes.php --check	Up to date

Merged with git merge --no-ff under the contributor's name (6feb21bbf), then the follow-up commit (c6b6b4058).

Still to do for the whole storage series: Storage series - follow-ups and gaps.