fix: an archive and an export are private from the moment they exist - #903
Merged
Merged
Conversation
ArchiveHandler and XmlExport both chmod 0600 after the write completes, and buildFromDirectory() walks the whole application tree first — so on an installation of any size the finished file sits at the process umask, measured 0644, for as long as building it took, and a run that dies in between leaves it that way for good. Those files hold the database dump with every account's encrypted secret and the master-password hash, config.xml with the credentials and crypto keys, and for the export with no password the name, login, URL and notes of every account in the clear. FileBackupHandlersFactory already shows the right shape, restricting database.sql before any write. PharData offers no equivalent: it does not create the archive when constructed, so there is nothing to chmod beforehand, and compress() refuses when its target already exists. Narrowing the umask for the duration covers the tar, the gz and the export alike, with no window at all, and it is restored in a finally — leaking 0177 into the rest of the request would quietly make every later file owner-only. The chmods stay as the guarantee. The mutation result separates the two: with the chmod removed the archive is still private, and only removing the umask as well fails it.
blaipr
deleted the
fix/an-export-is-private-from-the-moment-it-exists
branch
September 3, 2026 00:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The backup archives and the XML export are restricted to their owner — but only once they are
written.
ArchiveHandlerandXmlExportbothchmod 0600after the write completes, andbuildFromDirectory()walks the whole application tree first, so on an installation of any size thefinished file sits at the process umask — measured 0644, which on a shared host is every local user
— for as long as building it took. A run that dies in between leaves it that way for good.
What is in those files: the database dump, with every account's encrypted secret and the
master-password hash;
config.xml, with the database credentials and the crypto keys; and for theexport, when no export password was given, the name, login, URL and notes of every account in the
clear.
FileBackupHandlersFactoryalready shows the right shape — it opensdatabase.sqland restricts itbefore any write, with a comment saying why. These are the same window, left open.
Why the umask, and not a chmod before the write
PharDatagives no way to restrict an archive beforehand, and I checked both halves of that ratherthan assuming:
compress()refuses outright when its target already exists — "phar … exists and must beunlinked prior to conversion" — so the
.gzcannot be pre-created either.Narrowing the umask for the duration covers the tar, the gz and the export alike, with no window at
all. The explicit chmods stay as the guarantee: the umask decides what a newly created file gets,
and nothing here should depend on that alone.
The umask is restored in a
finally. Leaking0177into the rest of the request would make everylater file owner-only too — including the cache and the compiled container — and that is the kind of
change that only shows up much later.
Tests
ArchiveHandlerTest, new. The mutation result is the interesting part, because it separates the twomechanisms:
chmod, keep the umaskSo the umask is demonstrably doing the work, rather than the chmod quietly covering for it. The
other tests pin the umask being restored — including when the archive cannot be built, which is what
the
finallyis for — that the uncompressed tar does not survive, and that Phar honours the umaskat all, since the whole fix rests on that and a future PHP could stop.
Two things noticed and deliberately left
compressDirectory()returnsphar:///…/archive.tar.gz/<first entry>— a URL for a file insidethe archive, not the archive's own path. Every caller discards it, so it is a wart, not a defect.
?string $regex = nulldefault reachesPharData::buildFromDirectory(), which requires astring, so passing no regex is aTypeError. The one production caller always passes one, so itis unreachable today. Both belong in their own change.