-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't run composer as root #5689
Conversation
Awesome, thank you! I've been wanting to handle this issue for ages and didn't get around to it. Ping me back when it's ready! |
669f696
to
477c609
Compare
@snipe do you think it will be good to add COMPOSER_PROCESS_TIMEOUT=2000 or any different number for users that have a slow internet connection? I have the following on the customised OS |
3953e6b
to
9bda831
Compare
As you're poking at the script, you should be able to use the Ubuntu 16.04 section for 18.04 as well, I made the edit locally and it seems to be behaving. |
19ac69e
to
b57f219
Compare
b57f219
to
29c7e73
Compare
@snipe I think i'm done with the changes this round. Tested against the boxes in Vagrantfile. Composer is installed globally using Let me know what you think and if you want any changes. |
snipeit.sh
Outdated
usermod -a -G "$apache_group" "$APP_USER" | ||
} | ||
|
||
run_as () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might rename this run_as_webuser() or similar. it's confusing when reading through the code to know that "run_as php -r blah blah blah" does not mean to run the command a user named php.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, I’ll make the change later tonight.
@tiagom62 saw that it has not merged with the master; therefore, don't know if you still can update the script and modify line 208 SELinux label httpd_sys_script_rw_t, the label should be httpd_sys_rw_content_t at least for the storage and public directory. |
@tiagom62 I was waiting until it looked like you were done - is this ready for review/merge? |
@EarlRamirez Why is this change needed? That should probably be a separate PR. @snipe Yup, it is ready for review/merge. |
@tiagom62 the current label is invalid; therefore, those with SELinux enable will get permission errors for the logs and when uploading any files to public/upload. I will try and do a separate pull request |
Still needs to be thoroughly tested. Please don't merge yet.