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

Update suggested permissions for .shortcuts/icons/ #69

Merged
merged 2 commits into from
Dec 17, 2021

Conversation

SuspectTyrannosaurus
Copy link
Contributor

Change the chmod command suggested for the ~/. shortcuts/icons/ directory from 600 to 700. Without u+x, users are not able to cp files into the directory.

Change the `chmod` command suggested for the `~/. shortcuts/icons/` directory from `600` to `700`. Without `u+x`, users are not able to `cp` files into the directory.
@SuspectTyrannosaurus
Copy link
Contributor Author

I created Issue #68 to document this PR. (I can't find the button to link the PR to the Issue, so I am just mentioning it in a comment.)

Copy link
Member

@agnostic-apollo agnostic-apollo left a comment

Choose a reason for hiding this comment

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

Thanks for noticing the mistake for the wrong directory permission but change the command to chmod -R a-x,u=rwX,go-rwx /data/data/com.termux/files/home/.shortcuts/icons. It was originally done so that icon files didn't have execute permissions. This will result in directories having 700 and files having 600.

https://stackoverflow.com/a/17091831/14686958

@agnostic-apollo agnostic-apollo linked an issue Dec 12, 2021 that may be closed by this pull request
@SuspectTyrannosaurus
Copy link
Contributor Author

@agnostic-apollo

This is a sample command in a walkthrough creating the directory with no contents, so the proposed change to the chmod would (in theory) have no effect, since it only affects the directory and its current contents.

The example command is, however, written in a way such that it won't cause an error if the directory exists (mkdir -p), and the recursive flag on chmod would indeed affect any existing files in that case.

Perhaps the tutorial's sample commands should be rewritten to not be so graceful, so as to prevent making any existing files executable?

To be really safe, I think the app itself would need to be modified to scan for permissions and refuse to proceed if unsafe executable flags are set.

@agnostic-apollo
Copy link
Member

Well, if the directory didn't exist, then obv chmod won't do any good, and directory should automatically be 700 too. I added permissions since users are quite often running random commands from the internet including permission and ownership mess ups, so providing commands that users followed would help in fixing those so that issues aren't opened for them. Checking or fixing permissions recursively while showing choosers is a bit much for the app, and java probably doesn't have good support for such granular permission stuff, only for user, not for group and others.

It's our responsibility to just tell user what correct permissions are, specially considering some run selinux in permissive mode too. Rest is on them.

Images have no reason to be executable, so any existing files under existing directory would get fixed.

Script execution permissions are in fact checked and only fixed if its under shortcuts directory. Termux:Tasker allows scripts outside tasker directory, so that directory check is more useful for it.

https://github.com/termux/termux-widget/blob/v0.13.0/app/src/main/java/com/termux/widget/TermuxWidgetProvider.java#L195

Perhaps the tutorial's sample commands should be rewritten to not be so graceful, so as to prevent making any existing files executable?

I didn't get you, can you explain.

Fix the `chmod` command suggested for the `~/. shortcuts/icons/` directory. Previously it would not allow users to copy files into the directory.
Copy link
Contributor Author

@SuspectTyrannosaurus SuspectTyrannosaurus left a comment

Choose a reason for hiding this comment

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

I made the change recommended. In the odd case where someone has already created and populated the directory before running the command to create it, the pre-existing files should be set with the most appropriate permissions.

@agnostic-apollo
Copy link
Member

Sorry, forgot about this. Merging. Thanks again!

@agnostic-apollo agnostic-apollo merged commit a5e1f43 into termux:master Dec 17, 2021
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.

Icons directory permissions issue with recommended setup
2 participants