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

Question: Why the file dragged into a UWP app is read-only? #2421

Closed
0x7c13 opened this issue May 10, 2020 · 14 comments
Closed

Question: Why the file dragged into a UWP app is read-only? #2421

0x7c13 opened this issue May 10, 2020 · 14 comments

Comments

@0x7c13
Copy link
Member

0x7c13 commented May 10, 2020

Hey folks,

I have a question bothers me for quite a while: "If user drag a storage file into a UWP app, it will be read-only by default. If you try to save changes on top of this file, you will get UnauthorizedAccessException, but why?"

For example, this is how you get the file(s) from DragEventArgs:

var storageItems = await args.DataView.GetStorageItemsAsync();

And this will be true for the files you dragged in:

public static bool IsFileReadOnly(StorageFile file)
{
    return (file.Attributes & Windows.Storage.FileAttributes.ReadOnly) != 0;
}

And you will see exception if you do this:

using (var stream = await file.OpenStreamForWriteAsync()) { }

This does not make any sense to me, because if user drag a file into an app, he or she is fully aware of the operation and should just give the app same permission just like what the file open picker does. Another approach would be asking for broadSystemAccess, but it makes little sense to me as well for just enabling write to a dragged file? (+ @soumyamahunt for visibility)

Last year, when I started my notepads app. I did some research online and found out that not only me, but also thousands of developers voted for this change on MS Voice (I am not sure if it is this forum) but never received official response from MSFT. Not only that, the website is now deprecated so I cannot find and paste the link here.

Now here comes the interesting part: I recently knows that from @yaichenbaum that if you use PathIO API to write, you can workaround this limitation. Then we have a community member (@maickonn) sent out a PR for this workaround, let me paste the interesting workaround logic here:

if (IsFileReadOnly(file) || !await FileIsWritable(file))
{
    // For file(s) dragged into Notepads, they are read-only
    // StorageFile API won't work but can be overwritten by Win32 PathIO API (exploit?)
    // In case the file is actually read-only, WriteBytesAsync will throw UnauthorizedAccessException exception
    var content = encoding.GetBytes(text);
    var result = encoding.GetPreamble().Concat(content).ToArray();
    await PathIO.WriteBytesAsync(file.Path, result);
}
else // Use StorageFile API to save 
{
    using (var stream = await file.OpenStreamForWriteAsync())
    using (var writer = new StreamWriter(stream, encoding))
    {
        stream.Position = 0;
        await writer.WriteAsync(text);
        await writer.FlushAsync();
        // Truncate
        stream.SetLength(stream.Position);
    }
}

So basically PathIO.WriteBytesAsync can write to the file although it is marked as read-only for the StorageFile item. This looks like an exploit to me and I am very confused.

So, here are my questions:

  1. Why the file dragged into a UWP app is read-only, what is the reason behind it? Why we never change this behavior for so many years? This literally stops any notepad like or VSCode like document editing UWP app from working properly with dragged file. And further making them weaker comparing to their win32 counterparts.
  2. Why PathIO.WriteBytesAsync can write to a read-only StorageFile? I know I am using the "file.Path" here instead of the runtime object, but it should be caught by the file security model, isn't it? Or is this intended?
  3. Is there any plan for changing this behavior in the future or is there any plan to address it as part of WinUI 3.0 or post WinUI 3.0?

Note: If the file is already "read-only" before dragging or becoming "read-only" after dragging, none of these API will work anyway. So we are safe here. There is no bugs or concerns regarding to that topic.

Thanks,
Jackie

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label May 10, 2020
@RobbyRobbyRobby
Copy link

Yeah, this is a royal pain and I can't see any logic to it. I figure it must be a mistake.

Please can we get this fixed.

@soumyamahunt
Copy link

I think this has to do with DataPackage and DataPackageView classes especially how DataPackage.SetStorageItems method is implemented. The default behavior of SetStorageItems is to make the provided storage items readonly unless the app explicitly provides readonly attribute as false when drag of an item is starting from a given app. MS has to change this default behavior or make changes to set the readonly attribute as false in file explorer code behind.

@benstevens48
Copy link

benstevens48 commented May 10, 2020

Yes, this is extremely annoying. My users expect to be able to open files by drag and drop and then edit them. In my case I use a workaround if the file happens to be in the Pictures library because I have the Pictures library capability, so when the files are dropped I check if they are read only, and if so I try to load them from the file path. This is very cumbersome and only works if the files are in the Pictures library.

The read-only attribute isn't adding much security because it's possible to complete the drag-drop operation as a move operation (and not actually move the files anywhere), in which case the original files are deleted even though they were marked as read-only.

PS I imagine that the reason PathIO.WriteBytesAsync works for the file path is because you have access to that file path by some other means, e.g. Pictures library capability.

@michael-hawker
Copy link
Collaborator

This definitely seems like a bug in drag-n-drop, the developer should have access to a file if a user drops it into the app. This seems no different than opening the file via activation arguments, which works fine right?

Also, can't the developer still take the StorageFile from the drag-n-drop arguments and add it to the app's Future Access List and still open/access it later for both read & write?

@0x7c13
Copy link
Member Author

0x7c13 commented May 11, 2020

This definitely seems like a bug in drag-n-drop, the developer should have access to a file if a user drops it into the app. This seems no different than opening the file via activation arguments, which works fine right?

Also, can't the developer still take the StorageFile from the drag-n-drop arguments and add it to the app's Future Access List and still open/access it later for both read & write?

You can if you add it to the future access list but it won't take effect for the current run tho.

@michael-hawker
Copy link
Collaborator

@Jasonstein even if you close the reference from the DataView and just re-open the file directly from the access list?

@0x7c13
Copy link
Member Author

0x7c13 commented May 11, 2020

@Jasonstein even if you close the reference from the DataView and just re-open the file directly from the access list?

I have not tried that way but I would assume no, not for the current run. @yaichenbaum I think you tried that, did you?

@0x7c13
Copy link
Member Author

0x7c13 commented May 11, 2020

I tried adding the storage item to the future access list but it didn't help, I didn't try closing the reference though so it could be that would help.

Even if that works, I would still prefer to use PathIO since it is more straight forward anyway :)

@ranjeshj
Copy link
Contributor

@chrisglein @Austin-Lamb can you route this bug ? With win32 support perhaps some of these restrictions can be relaxed.

@ranjeshj ranjeshj removed the needs-triage Issue needs to be triaged by the area owners label May 13, 2020
@0x7c13
Copy link
Member Author

0x7c13 commented May 15, 2020

Btw, same applies to StorageFile.RenameAsync API. But there is literally no alternative way to rename a dragged file.

@RobbyRobbyRobby
Copy link

I've resorted to warning the users that they need to enable BroadFileSystemAccess just to accept dragged file properly (in apps where it needs to save over/rename, etc)

frustrating and not a good experience for dev or user.

@0x7c13
Copy link
Member Author

0x7c13 commented May 15, 2020

I've resorted to warning the users that they need to enable BroadFileSystemAccess just to accept dragged file properly (in apps where it needs to save over/rename, etc)

frustrating and not a good experience for dev or user.

Exactly, and it makes no sense to let user give BroadFileSystemAccess to a simple notepad app at all.

@marcelwgn
Copy link
Collaborator

@StephenLPeters @jevansaks Would it make sense to move this issue over to the project reunion repository as this is related to the app model?

@github-actions
Copy link

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants