Tidy up tools/, and harden the docs unpacking - #24
Merged
Merged
Conversation
The docs tarball is assembled by a CI runner and written into a tree that the upload key can reach, and the script unpacked it straight over /var/www/pgaweb/static/docs with an unquoted path and no checks at all, so the archive rather than the wrapper decided where those files landed. That is where the publishing wrapper's containment ended. The version argument is now checked for shape before it is interpolated into either the tarball path or the directory name, every variable is quoted, and the archive is vetted before anything is unpacked: every member must be a plain file or a directory, which rejects symlinks and hard links that would redirect a later member out of the tree, and every member must sit under the pgadmin4-<version>-docs directory, which rejects absolute paths and '..' traversal. Unpacking then happens into a staging directory alongside the live one, with --no-same-owner and --no-same-permissions, and the result is checked and moved into place only once it looks like the docs we asked for, so a bad tarball can no longer leave the site serving a half replaced tree. The comments explain what each check is for, since the next reader will otherwise have no way of knowing. Behaviour on a good tarball is unchanged apart from the loss of tar's verbose file listing, with the docloader call and the cache ban exactly as they were.
rebuild-apt-repo.sh and rebuild-yum-repo.sh are superseded by the parameterised versions at pkg/publish/ in pgadmin4, which are what the publishing wrapper actually runs; nothing here called them, and two copies of the repository signing logic is precisely the duplication the move to GitHub Actions set out to remove. sync-ftp-to-s3.py and purge_pgadmin_releases.sh operate on the download tree at /var/ftp/pgadmin4 and touch nothing in this Django application, so they belong beside the rest of the publishing machinery rather than here. The README described four scripts as being called by the Jenkins server, which is no longer true of any of them. The release time ones are now reached through the publishing wrapper from GitHub Actions, whilst update-docs.sh runs from cron on the web server and never had anything to do with Jenkins. The note about the AWS CLI goes with sync-ftp-to-s3.py, which was the only thing that used it.
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
1 participant
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.
Now that publishing runs through
pkg/publish/pga-publishin pgadmin4 rather than Jenkins,tools/carries a few scripts that either have a better home or are outright duplicates, andload-docs.shsits at the point where the wrapper's containment ends.Merge ordering, please read first
sync-ftp-to-s3.pyandpurge_pgadmin_releases.share being moved into pgadmin4 underpkg/publish/by work happening right now. This PR must not be merged until that change has landed and the new wrapper has been installed on the download server, because the wrapper finds the S3 sync by absolute path under/var/www/pgaweb/toolsand removing it early breaks thesync-s3verb.The other two commits have no such constraint, so if the timing is awkward the removals commit can be split.
What changed
Deleted
rebuild-apt-repo.shandrebuild-yum-repo.sh, which are superseded by the parameterised versions the wrapper actually runs atpkg/publish/in pgadmin4, and which nothing here called.Hardened
load-docs.sh. The docs tarball is assembled by a CI runner and written into a tree the upload key can reach, and the script unpacked it straight over/var/www/pgaweb/static/docs, unquoted and unchecked, so the archive rather than the wrapper chose where the files landed. It now validates the shape of the version argument before interpolating it into a path, vets the member list before unpacking anything (plain files and directories only, all underpgadmin4-<version>-docs), unpacks into a staging directory with--no-same-owner --no-same-permissions, and moves the result into place only once it looks like the docs that were asked for. A good tarball behaves as before, aside from losing tar's verbose file listing.Corrected
tools/README.md, which described four scripts as being called by Jenkins.For a reviewer to decide
The member vetting requires a single
pgadmin4-<version>-docstop level directory, which is what the published tarballs have carried and what thedocloadercall has always assumed..github/workflows/release-candidate.ymlin pgadmin4 builds the tarball withtar -czf ... -C artefacts/pgadmin4-html-build-output ., which would produce a flat archive instead; if that is what the first Actions release actually uploads, this script will refuse it rather than silently loading nothing, but the packaging step is the thing that wants fixing. Worth confirming against the next release candidate before it is a release.Testing
Verified against a real published tarball (
pgadmin4-9.18-docs.tar.gz), and against crafted archives containing an absolute path member,../traversal members, a symlink pointing outside the tree with a later member written through it, a flat archive with no top level directory, a second top level directory, and a setuid member. All four hostile cases are refused with nothing written outside the tree and nothing left behind, a hostile tarball arriving on top of good docs leaves the existing docs in place, and the setuid bit is dropped.