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

Unable to upload a/o create folder on any other disk than C:\ on Win10 #994

Closed
LaurentGrenet opened this issue Dec 12, 2022 · 16 comments · Fixed by #1310
Closed

Unable to upload a/o create folder on any other disk than C:\ on Win10 #994

LaurentGrenet opened this issue Dec 12, 2022 · 16 comments · Fixed by #1310

Comments

@LaurentGrenet
Copy link

LaurentGrenet commented Dec 12, 2022

Hi,
I'm using miniserve on a Windows 10 laptop

When miniserve's path to serve is on C:\ disk, upload of file a/o directory creation works fine, and is possible, at least as soon as appropriate access rights are granted on local NTFS disk.

But if the path to serve is on another local disk (eg "miniserve D:\Temp\ -u -U" ), upload of files a/o directory creation systematically fail with the error message "Insufficient permissions to create file in \?\D:\Temp", and this even if full control has been granted to everyone at OS level in this target folder.
On the other hand, it works "sometimes" even on disk D:
For example, if I create a new folder D:\Test (at Win10 level), then launch "miniserve D:\Test -u -U", I'm able to upload files a/o create directories... while access rights are exactly the same at OS level for D:\Test and D:\Temp....

Moreover, in case access rights are NOT granted to users in target dir, upload and directory creation are (hopefully...) not possible, but the error message is not at all the same, but rather "Failed to create mysubdir / caused by: Accès refusé. (os error 5)", or "Failed to create \?\D:\Test\P1080514.JPG / caused by: Accès refusé. (os error 5)"

@ViRb3
Copy link
Contributor

ViRb3 commented Jan 25, 2023

For me on Windows 11, not even C:\Users\user\Downloads works, same error is output.

@svenstaro
Copy link
Owner

Can you try running miniserve using admin permissions?

@ViRb3
Copy link
Contributor

ViRb3 commented Jan 27, 2023

Same result when run as administrator.

@svenstaro
Copy link
Owner

Frankly I got no clue then. I don't do anything special in miniserve here. I'm not sure whether I have to somehow request special permissions from Windows here or whatever. Could you research this a bit and maybe figure out what's wrong? I can then try to fix it.

@LaurentGrenet
Copy link
Author

I confirm that on my side too, running it as admin doesn't change anything

@ViRb3
Copy link
Contributor

ViRb3 commented Feb 19, 2023

@svenstaro Did some research and found out the problem.

You have a check for the target directory being readonly, and abort if true:

From Rust docs:

On Windows this returns FILE_ATTRIBUTE_READONLY. If FILE_ATTRIBUTE_READONLY is set then writes to the file will fail but the user may still have permission to change this flag. If FILE_ATTRIBUTE_READONLY is not set then writes may still fail due to lack of write permission. The behavior of this attribute for directories depends on the Windows version.

I checked a bunch of folders on my system, and to my surprise, guess what?

Screenshot 2023-02-19 at 20 00 56

This applies to EVERY SINGLE folder in my computer! I quickly spun up a VM and installed a brand new, fresh Windows 11, and it had the exact same behavior. I have to note, the checkbox has three states, the above one meaning "default", as opposed to "enabled":

Screenshot 2023-02-19 at 20 08 13

So, it turns out that Rust detects both the "default" and "enabled" state as "readonly".

I dug some more, and found out this:

apparently it was a "feature" of Windows Defender that decided to turn itself on after the update. Go to Windows Defender > Virus protection > Settings > Control access to folders (or stuff like this) and turn if off

The read only checkbox for folders in File Explorer doesn't do anything other than tell FE to look for a desktop.ini inside the folder. You can ignore it

Whatever is the cause, I think the solution is simple — skip readonly check for Windows. I'd even dare to go a step further and drop the check for all platforms, since a folder may not be readonly but write still fail due to insufficient permissions. Also who knows how many other platforms can have edge cases like this. Just let the OS error us if writing can't happen.

@LaurentGrenet
Copy link
Author

I think the solution is simple — skip readonly check for Windows.

+1

This check doesn't make any sense in windows, in which the R attribute on a folder doesn't mean at all that the folder is "read only". This attribute doesn't prevent at all to create new files in the folder, nore remove files from the folder, nor to update files content in the folder.
It only means that the potential desktop.ini has to be taken into consideration.
For example, if in your desktop.ini there is an entry like

[.ShellClassInfo]
LocalizedResourceName=.....

to localize folder's name in Win explorer, folder's name is actually localized ONLY if the folder has the R attribute. Without this attribute, folder's name remains unchanged.

@svenstaro
Copy link
Owner

Great analysis! Would one of you folks like to try to implement a fix?

@ViRb3
Copy link
Contributor

ViRb3 commented Feb 20, 2023

My solution is to remove readonly check altogether for all platforms. If you're happy with that, can send a PR.

@svenstaro
Copy link
Owner

I wonder if we can still properly handle the specific errors we get from the OSs in case writing fails due to an actual read only situation. I'd like to be able to still show users good error output if possible. Could you check what that error output actually looks like if attempting to write a read only location?

Other than that, I'm fine with it.

@svenstaro
Copy link
Owner

@ViRb3 Hey, are you still interested in sending a PR?

@LaurentGrenet
Copy link
Author

Hi @svenstaro and @ViRb3

I'm surprised that this bug is not fixed yet since we know what to do to fix it.
Personally, I'm not skilled enough to do the change, but I would surprised it is difficult to implement for people having programation skills....

@svenstaro
Copy link
Owner

The thing is I don't have a Windows machine to develop on and I don't want to get one just for this issue. I could try to do a blind fix but then someone on Windows needs to verify it which might be annoying if I need multiple attempts.

@ViRb3
Copy link
Contributor

ViRb3 commented Jan 7, 2024

Sorry, I haven't had the time to do this, and it's not a priority since I use a different file hosting app now.

@LaurentGrenet
Copy link
Author

The thing is I don't have a Windows machine to develop on and I don't want to get one just for this issue. I could try to do a blind fix but then someone on Windows needs to verify it which might be annoying if I need multiple attempts.

If you do so ("blind" fix), I'm OK to run testing.
And also, (provided the corresponding changes are clearly identified in source files) I can review them : I'm not skilled enough to "write" the change, but I am to "understand" the code and verify it.

So if you do it, pls send me a mail stating what are exactly the source files updated by the fix (and provide me a compiled executable "miniserve-xxxx-x86_64-pc-windows-msvc.exe")

@ViRb3
Copy link
Contributor

ViRb3 commented Jan 11, 2024

Released a fix in PR linked above.

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 a pull request may close this issue.

3 participants