Skip to content
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

Automatically apply permissions for stuff provisioned with ynh_setup_source. #1955

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

orhtej2
Copy link
Contributor

@orhtej2 orhtej2 commented Sep 27, 2024

The problem

  • ynh_setup_source "$install_dir/subpath" is excluded from default permissions
  • adding subdirectory to $install_dir is exempt from default permissions, unlike adding a file there.

Solution

Subdirectories of $install_dir and $data_dir get $app:$app o+rwX ownership & permissions by default, in line with files added there.

PR Status

It worked once when installing YunoHost-Apps/syncserver-rs_ynh#58

How to test

Install an app that uses helpers v2.1 (duh) and provisions sources into a subdirectory.

chown "$app:$app" "$target"
if (is_in_dir "$target" "${install_dir:-}" || is_in_dir "$target" "${data_dir:-}" || is_in_dir "$target" "/etc/$app"); then
if [ -d "$target" ]; then
chmod -R o+rwX "$target"
Copy link
Member

Choose a reason for hiding this comment

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

Seems a security hole. If we do ynh_setup_source or ynh_setup_source "$install_dir" we apply here a o+rwX permission by default, but we should not allowed other users to access this dir (by default) i guess.

In more adding a -R options onto data_dir could be looooong (if there is a lot of file), so this should be done only one time by yunohost install or upgrade...

I suggest to study all chown, chmod apps to understand what should be done by default and how give advice in the right way to packagers if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was supposed to be u not o x_x

Copy link
Member

@zamentur zamentur left a comment

Choose a reason for hiding this comment

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

Remove the chmod -R o+rwX

@orhtej2
Copy link
Contributor Author

orhtej2 commented Oct 15, 2024

Remove the chmod -R o+rwX

chmod is sort of heart of this change, but it was meant to be chmod u not o (fixed).

@alexAubin
Copy link
Member

So eeeeh, should we move forward with this ?

@orhtej2
Copy link
Contributor Author

orhtej2 commented Oct 22, 2024

So eeeeh, should we move forward with this ?

Honestly 🤷 , I guess zamentur has a valid idea to assess if these are permissions right for my one app or general defaults for the apps in catalogue.

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

Successfully merging this pull request may close these issues.

3 participants