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

actions: saveas: Fix crash at access without permission #3082

Merged
merged 1 commit into from
Dec 10, 2023

Conversation

JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Dec 9, 2023

In the moment os.Stat() fails due to not existing permission fileinfo.Name() will be access, which we need to prevent. In that case we can't be sure, if the file exists or not anyway. Missing privileges should lead to a call of saveBufToFile since this will take care of extending privileges (e.g. by invoking sudo).

Currently the behavior ignores existing files which can't be accessed due to missing permissions. They will be overwritten in the moment sudo is invoked without any further check if the user wants to overwrite possible existing ones.
On micro start with such a files as parameter we will receive the complaint:

stat [FILENAME]: permission denied

Press enter to continue

So maybe we need a more plausible but more complex solution to 1. stat with more privileges and 2. ask to load/read the file with more privileges.

Fixes #3080

@dustdfg
Copy link
Contributor

dustdfg commented Dec 10, 2023

Hello

  1. Go docs say that new code shouldn't use os.IsPermission and use errors.Is(err, fs.ErrPermission) instead. At the same time the error is 100% returned by os package but just wanted to let you know
  2. Adding || os.IsPermission(err) was the first "solution" I tried but it didn't work so I tried to add loging which didn't log anything:
if os.IsNotExist(err) || os.IsPermission(err) {
	if os.IsPermission(err) {
		WriteLog(err.Error())
	}

Does os.IsPermission(err) change anything for you? I tried all four situations without any results:

  1. perm: yes, exist: no
  2. perm: yes, exist: yes
  3. perm: no, exist: no
  4. perm: no, exist: yes

The else really works and solves the issue

  1. stat with more privileges

What do you mean? How would it look?

  1. ask to load/read the file with more privileges.

Do you mean separate function for file loading that will silently load file if can or prompt user for permissions?

@JoeKar
Copy link
Collaborator Author

JoeKar commented Dec 10, 2023

  1. I will update. Would be nice to force such updates with deprecated warnings or such.
  2. Yes, because it doesn't change the problem caused by unconditional access to the fileinfo.Name(), but you will need it too.
    2.1. In the moment you use SaveAs without the permission your action will be silently ignored, because there is no further prompt. By adding this condition we will invoke sudo or equiv. to do the save.
    2.2. That's the reason why we should add a functionality to check if the non accessible file exists, because silently overwriting an existing file can cause other problems...even in the moment the user explicitly used super user rights. Sometimes the user doesn't see the consequences coming, so I recommend to do a small check including prompt before overwriting.

Do you mean separate function for file loading that will silently load file if can or prompt user for permissions?

The load/check needs to be done with more privileges, otherwise we can't test. Afterwards the possible YN prompt for overwrite can be displayed.

@JoeKar JoeKar force-pushed the fix/save-as-crash-no-permission branch from dd0ea99 to a4ca078 Compare December 10, 2023 11:48
@dustdfg
Copy link
Contributor

dustdfg commented Dec 10, 2023

Hmmm.... We have two different "permission errors". I messed up them and thought that we have only one

  1. Folder is readable by user but user don't have rights for file within
  2. Folder isn't readable so user even can't know if file exist

I experimented with 1 kind so I didn't notice any changes with adding || os.IsPermission(err) but the 2 kind behaves as you described: silent ignore.

I understand now, thank you!

@zyedidia zyedidia merged commit 2d82362 into zyedidia:master Dec 10, 2023
3 checks passed
@JoeKar JoeKar deleted the fix/save-as-crash-no-permission branch December 11, 2023 19:23
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.

[BUG 100% reproducible] Crash when creating non-existing file without permissions for it
3 participants