feat: add deploy:writable:chmod task for proper file permissions - #14
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntroduces a centralized Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployer/functions.php`:
- Around line 169-170: The current runExtended("cd {{ deploy_path }}/shared &&
find . -type f -exec chmod $modeFiles {} +") makes all files (including
shared/.env or shared/.env.local) world-readable; after keeping that baseline
chmod call, add a second step that iterates over the shared_files array and
tightens permissions for sensitive entries (e.g., runExtended calls that chmod
600 or 640 specifically for each path in shared_files or for patterns like .env
and .env.local), referencing the existing runExtended invocation and the
shared_files variable so only those sensitive files get stricter permissions.
- Around line 163-166: The recursive chmod call in runExtended("cd {{
release_path }} && chmod -R $modeWritableDirs var/cache") wrongly applies
directory bits (SGID/execute) to files; change the deployment step so
runExtended leaves files set by runExtended("cd {{ release_path }} && find .
-type f -exec chmod $modeFiles {} +") and only applies $modeWritableDirs to
directories under var/cache (e.g. use find to target -type d for var/cache or a
non-recursive chmod on the dir itself) so that $modeFiles (644) remains on cache
files while directories get the writable/SUID bits; update the runExtended
invocation(s) around the var/cache handling accordingly (refer to the existing
runExtended calls and $modeFiles/$modeWritableDirs variables).
- Around line 160-177: The current permission sweep uses runExtended with
$modeDirs/$modeFiles on the entire release tree, which downgrades release-only
writable directories set earlier; add a loop after the shared_dirs loop that
iterates get('writable_dirs') and for each $dir that exists under {{
release_path }} but is NOT listed in get('shared_dirs') run runExtended to
recursively set directory perms to $modeWritableDirs and file perms to the
writable file mode (use $modeWritableFiles or $modeFiles as appropriate) so
release-path-only writable dirs and their contained files are restored for
webserver write access; reference runExtended, get('writable_dirs'),
get('shared_dirs'), $modeWritableDirs and $modeWritableFiles/$modeFiles to
locate and apply the fix.
In `@deployer/symfony/config/set.php`:
- Around line 33-37: The change to set('writable_chmod_mode','2770') introduces
SGID and is a breaking change because deploy:writable will now chmod 2770 on
writable_dir; either revert the default back to '0770' (restore previous
behavior in set('writable_chmod_mode')) and add a migration note, or keep '2770'
but add a clear CHANGELOG / migration entry and upgrade note describing the new
SGID behavior and that deploy:writable will apply 2770 to writable_dir
(reference the config keys writable_chmod_mode,
writable_chmod_mode_writable_dirs and the deploy:writable task in the
changelog/migration doc).
| set('writable_chmod_mode', '2770'); | ||
| set('writable_recursive', false); | ||
| set('writable_chmod_mode_files', '644'); | ||
| set('writable_chmod_mode_dirs', '2755'); | ||
| set('writable_chmod_mode_writable_dirs', '2775'); |
There was a problem hiding this comment.
writable_chmod_mode change from 0770 → 2770 is a breaking behavior change.
Adding the SGID bit (2) is intentional per the PR, but deploy:writable will now run chmod 2770 on every writable_dir instead of chmod 0770. Existing projects upgrading this package will silently get a different permission mode applied to their writable dirs on the next deploy.
Consider documenting this in a CHANGELOG or migration note.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deployer/symfony/config/set.php` around lines 33 - 37, The change to
set('writable_chmod_mode','2770') introduces SGID and is a breaking change
because deploy:writable will now chmod 2770 on writable_dir; either revert the
default back to '0770' (restore previous behavior in set('writable_chmod_mode'))
and add a migration note, or keep '2770' but add a clear CHANGELOG / migration
entry and upgrade note describing the new SGID behavior and that deploy:writable
will apply 2770 to writable_dir (reference the config keys writable_chmod_mode,
writable_chmod_mode_writable_dirs and the deploy:writable task in the
changelog/migration doc).
There was a problem hiding this comment.
@coderabbitai Intentional change. The previous code had a TODO: // todo maybe change to '2770'. SGID is the core purpose of this PR. This is an internal tool where we control all deployments, so no separate changelog entry needed.
There was a problem hiding this comment.
@konradmichalik, thank you for the clarification! You're absolutely right—the TODO comment confirms this was planned, and since this is an internal tool where you control all deployments, the breaking change concern doesn't apply. The SGID behavior is exactly what you need. I appreciate the context!
1 participant
Summary
applyDeployPermissions()function for standardized file/directory permissionsdeploy:writable:chmodtask for TYPO3 (new) and refactor existing Symfony taskwritable_chmod_modefrom0770to2770, correctwritable_recursivekey nameProblem
The standard
deploy:writabletask only applies a flatchmodonwritable_dirswithout differentiating between files and directories. This causes permission issues in TYPO3 (and Symfony) projects:644) vs directory (2755) permissionsvar/cachenot created before cache warmupChanges
deployer/functions.php- NewapplyDeployPermissions()with configurable modes and fallback defaultsdeployer/typo3/task/deploy_writable_chmod.php- New TYPO3 task (calls shared function + CLI binary chmod)deployer/typo3/task/deploy.php- Add task to deploy sequence afterdeploy:writabledeployer/typo3/autoload.php- Register new task filedeployer/typo3/config/set.php- Addwritable_chmod_mode_files,writable_chmod_mode_dirs,writable_chmod_mode_writable_dirsdeployer/symfony/task/deploy_writable_chmod.php- Refactored to use shared functiondeployer/symfony/config/set.php- Fix chmod mode to2770, fix deadwritable_chmod_recursivekey, add new config varsSummary by CodeRabbit
New Features
Chores