zyedidia / micro

A modern and intuitive terminal-based text editor
https://micro-editor.github.io
MIT License
24.47k stars 1.16k forks source link

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

Closed JoeKar closed 7 months ago

JoeKar commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago
  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.

dustdfg commented 7 months ago

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!