Add in-app update check and upgrade for global admins - #1000
Conversation
7b196d9 to
15539f6
Compare
Local QA Results
|
|
Will rebase after PR #1001 is merged, and CI should pass nicely. |
15539f6 to
1d00db8
Compare
Adds an update-check page that queries the GitHub Releases API to show available updates with changelogs, and allows global admins to download and apply updates directly from the web UI. Depends on postfixadmin#999 for self-contained release tarballs.
1d00db8 to
c05750c
Compare
|
I'm tempted to request we remove the http calls via fopenwrappers/file_get_contents/fopen etc and use something like Guzzle ... but that would mean guzzle would become a hard dependency (it is currently pulled in via a dev dependency - phpcoveralls) |
|
Yeah, adding Guzzle as a hard dependency just for this feels heavy. The current approach uses cURL as primary (available on most PHP installs) with file_get_contents as fallback, which should cover most setups without adding dependencies. If you'd want to standardise HTTP calls across the project later, that could be a separate issue/PR. Happy to help with that too. |
|
Right, I suspect the implementation of this PR is OK - although I've not tested it yet. I am however waiting a week or so before deciding if the change should be merged, and hoping e.g. @cboltz might chip in with an opinion. I think most admins probably install postfixadmin through a package manager (e.g. via deb/rpm). Some may install it by following the web instructions. I suspect it'll normally then stay on a private/protected website, not be exposed to the internet and just do it's thing. I would imagine most people only update when they update the O/S ... or if a new package is published? Because of that, I'm not totally sure if having an auto-update feature is desirable/wanted..... (My arguments may be flawed. I'm hoping others might chip in opinions....) |
| $install_dir = dirname(__DIR__); | ||
| $tmp_dir = sys_get_temp_dir() . '/postfixadmin-update-' . uniqid(); | ||
|
|
||
| if (!mkdir($tmp_dir, 0700, true)) { |
There was a problem hiding this comment.
uniqid is predictable (time based) according https://www.php.net/uniqid which also means using it to create a tempdir could be a security risk.
A quick test shows that the if !mkdir can prevent symlink attacks (if the directory pre-exists as directory or symlink, mkdir will error out), so at least an attacker can "only" block the automated update.
Since I'm already commenting on this line - do you really think the true for recursively creating the directory is needed? IMHO it would be quite surprising if sys_get_temp_dir() points to a non-existing directory.
| $tmp_file = $tmp_base ? $tmp_base . '.tar.gz' : false; | ||
| if ($tmp_base === false || $tmp_file === false) { | ||
| flash_error('Failed to create temporary file.'); | ||
| } elseif (!rename($tmp_base, $tmp_file)) { |
There was a problem hiding this comment.
https://www.php.net/rename says that existing files will be overwritten. While it at least replaces an existing symlink (instead of giving a working symlink attack) (tested with PHP 8.4.19), this doesn't sound ideal. Maybe do the download into a temporary directory?
| opcache_reset(); | ||
| } | ||
| flash_info("Successfully updated to version " . htmlspecialchars($update_version) . ". Please run <a href='upgrade.php'>upgrade.php</a> to apply any database changes."); | ||
| header('Location: update-check.php'); |
There was a problem hiding this comment.
Better send users directly to upgrade.php to enforce the database upgrade. Otherwise they might ignore the message ("hidden" in a success message) and in worst case hit random errors in various list or edit pages if an expected database change is missing.
Actually there's not really a need to send the users there - require('upgrade.php'); should work.
(We only run check_db_version() on login.)
| <h4><span class="bi bi-arrow-repeat" aria-hidden="true"></span> {$PALANG.pUpdate_check_title|default:'Update Check'}</h4> | ||
| </div> | ||
| <div class="card-body"> | ||
| <p><strong>{$PALANG.pUpdate_current_version|default:'Current version'}:</strong> {$current_version}</p> |
There was a problem hiding this comment.
Instead of using |default:, please use the language-update.sh script to add all needed texts to the language files.
| // Check the install directory is writable | ||
| $install_dir = dirname(__DIR__); | ||
| if (!is_writable($install_dir)) { | ||
| flash_error('Installation directory is not writable by the web server. Cannot apply update.'); |
There was a problem hiding this comment.
This check might be "good enough", but I can imagine several reasons why replacing some or all files might fail nevertheless:
- files not owned by the webserver user (+ maybe sticky bit set on a directory)
- subdirectories not owned by the webserver user
- AppArmor or SELinux preventing write access.
Doing a "perfect" check is hard (and probably impossible when it comes to checking AppArmor or SELinux permissions), so your "good enough" solution might indeed be good enough.
|
Besides the technical details I mentioned above, let me add a general comment: I don't like the idea to allow an webapp to update itsself. Not only because people might install PostfixAdmin via a rpm or deb. The main reason is because self-updating means all files need to be writable for the webserver, which is a security risk. If we really add this feature, it should be behind a config option which is disabled by default. (Personally I'd vote against adding the self-update feature.) I know there are other PHP apps that contain a self-update feature, and I even understand why for example Wordpress self-update cronjobs make sense (attackers are faster than you can do manual updates), but PostfixAdmin is very different:
Depending on how strict the server config is, even doing the update check with HTTP requests might be blocked (by a firewall blocking outgoing traffic and/or disable_functions) or trigger some alerts. This also means that the update check should be behind a config option which switches between your "fancy" update check and a boring link to the github releases page. (I'd be ok with enabling your "fancy" update check by default.) |
knofte made a very good point that this comes down to sysadmin preference. Some want to be able to do a web-based "update me" method. It is quite easy, and can avoid some shell hassles on a lot of hosting services that are built around intra-app management. For those sysadmins (of which I am one one) who use package managers and work primary via shell, it's a feature we wouldn't use. The point I made and cboltz echoed about traditional webserver security (read-only webroots) make it non-functional without any real consequence. A config.local.php knob that hides the feature would provide a useful safety barrier for us console cowpokes deploying the app via management systems. :-) I think it's a useful feature, but one that needs to be fully optional and hideable. |
|
Thanks for the feedback everyone. I've split this into two parts:
|
|
There is a better solution for this approach; I will create a PR with the proposal I have in mind. |
5 participants
Summary
Closes #998
Depends on #999
Adds a web-based update check and upgrade page accessible to global admins. The page queries the GitHub Releases API to show available updates with full changelogs from the running version, and allows one-click download and installation of updates.
Features
$CONF['version']against GitHub Releases APIPharDataextension is availableconfig.local.phpupgrade.phpfor database migrationsHow it works
api.github.com/repos/postfixadmin/postfixadmin/releases.tar.gzasset is available (from Add GitHub Actions workflow to build self-contained release archives #999), shows "Download & Apply" buttonsys_get_temp_dir(), extracts via PharData, copies files over installation, resets OPcacheupgrade.phpFiles changed
public/update-check.php— new page with all update logictemplates/update-check.tpl— Smarty template with Bootstrap 5 UIconfigs/menu.conf— added URL mappingtemplates/menu.tpl— added menu item for global adminstemplates/index.tpl— footer "check update" link points to new page for global adminsTest plan
config.local.phpis preserved during upgrade