Skip to content

fix(migrations): read and write custom.ini through checked handles in the upload progress migration - #248

Open
mrrobot47 wants to merge 4 commits into
EasyEngine:developfrom
mrrobot47:fix/upload-progress-migration-safe-io
Open

mrrobot47 wants to merge 4 commits into
EasyEngine:developfrom
mrrobot47:fix/upload-progress-migration-safe-io

Conversation

@mrrobot47

@mrrobot47 mrrobot47 commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

What

The v4.13.3 upload-progress migration (20261021120000_site-type-wp_disable_wp_session_upload_progress) reads and writes each site's custom.ini through checked handles, so it can no longer be redirected outside the site's config volume.

Why

The migration still runs on every upgrade from a version before v4.13.3. custom.ini lives in the site's config_php volume, which the php container mounts at /usr/local/etc, so the container can replace custom.ini, conf.d or php/ with a symlink. The migration checked only custom.ini itself (is_link()), then used file_get_contents()/file_put_contents() by path: with a symlinked conf.d it read, rewrote or created custom.ini in whatever host directory the link pointed at, and its rollback deleted a file it had created there.

How

  • Reads and writes go through site-command's get_container_file_contents() and put_container_file_contents() (fix(site): stop following symlinks from a site's php config volume in restore, update --php and migrations site-command#522): one checked handle, a regular file in the real conf.d, written in place, nothing created, and a file that changed since it was read is left alone.
  • With an older site-command that lacks those helpers, the migration checks that custom.ini is a regular file and that no directory above it is a symlink before each read and write. That closes a link that is already there, not one swapped in during the run.
  • A missing custom.ini is no longer created, only warned about with the manual fix, as site-command's re-check migration (fix(restore): keep session upload progress off after restoring an older backup site-command#520) does: PHP can't create a file safely in a directory the container can swap.
  • The backup copy is written from the content that was read, not copied by path; a failed write no longer triggers a restore over a file the container changed meanwhile, and the backup is kept (named in the warning) in case putting the old content back failed too. The fallback checks the path again before such a put-back.
  • A custom.ini that the two lines would push over the 1 MB read limit is skipped with a warning, so rollback can always read back what was written.

Testing

  • Against throwaway site dirs on PHP 7.4 and 8.5, with site-command's helpers and without them: a regular custom.ini gets the Off line and rollback restores it; a missing one is warned about and not created; a symlinked custom.ini and a symlinked conf.d are refused with a warning and the target directory stays byte for byte the same. The released migration fails the last three.
  • #520's re-check migration matrix (733 checks) passes with this migration as the v4.13.3 step.
  • On a runner with core develop, the migration re-run on a WordPress site whose php container swapped conf.d for a symlink to a host directory: develop's migration writes the Off line into the host directory's custom.ini; this branch's leaves it untouched and warns, with develop's site-command and with site-command#522; after the link is removed, the re-run adds the line at the top. Behat passes with this checkout and site-command#522.

Release order

Works with any site-command; the race-safe path needs EasyEngine/site-command#522 (which needs #519 and #520), so ship them in the same release.

… the upload progress migration

The site's php container can swap custom.ini, conf.d or php/ in its config volume for a symlink, and the migration followed it: it read, rewrote, created or (on rollback) deleted custom.ini wherever the link pointed. It now uses site-command's checked helpers (get_container_file_contents() and put_container_file_contents()), and with an older site-command that lacks them it checks that custom.ini is a regular file and that no directory above it is a symlink. A missing custom.ini is no longer created, only warned about, as the site-command re-check migration does.
If putting the old content back failed too, the backup is the only copy, so it is kept and named in the warning; the fallback for an older site-command now reports a failed put-back.
…he size limit

Rollback reads custom.ini back with the same limit, so content larger than it after the two lines are added is skipped with a warning instead of written.
With an older site-command, a failed write put the old content back by path without checking it again, so a symlink swapped in meanwhile was followed; the backup is kept and named in the warning instead.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Adding the setting can exceed the fallback's read limit, preventing rollback from restoring an otherwise accepted file.

1 open issue
What changed in this PR

This PR hardens the WordPress upload-progress migration against symlink redirection when accessing a site's custom.ini.

Changes:

  • Uses checked file helpers when available, with path checks for older site-command versions.
  • Stops creating missing configuration files and warns with manual instructions.
  • Backs up the content already read and updates write-failure recovery and rollback handling.
File Description
migrations/​container/​20261021120000_site-type-wp_disable_wp_session_upload_progress.php Adds guarded configuration access and revises backup and restoration logic.

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Privileged host-file writes and race protection that depends on a coordinated site-command release warrant human security review.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Privileged writes to container-controlled paths, the race-prone compatibility fallback, and coordinated site-command dependencies warrant human security review.

No open issues 🎉

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants