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

MacOS sandboxing feature #16090

Merged
merged 22 commits into from
Jul 19, 2024
Merged

MacOS sandboxing feature #16090

merged 22 commits into from
Jul 19, 2024

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Jun 23, 2024

What does the pull request do?

This PR resurrects idea of old PR - #6540.
In short, in order to make macOS app working with the AppStore sandbox, developers need to be careful with how they interact with the File System:

  • Each operation should be wrapped in secure scope session - [NSURL startAccessingSecurityScopedResource] + [NSURL stopAccessingSecurityScopedResource].
  • When any file or folder are opened via picker, developers need to bookmark access to these files in order to reuse them between app sessions. Simply saving full file path won't work, as OS might not allow direct file access.

Old PR was created when we didn't have a better idea how this API should look like. And later it was implemented for mobile and browser platforms with new StorageProvider API, where we fully support sandboxed bookmarks. macOS platform was temporary skipped due to complexity and chance of introducing bugs to the existing backend (+it was low priority).

How to test

  1. Open ControlCatalog, Dialogs page.
  2. Open any file or folder
  3. Copy "bookmark" text field
  4. Reopen the app, dialogs page, input the same bookmark value.
  5. Press "read folder/file bookmark".
  6. The same folder/file should be opened.
  7. Additionally, all other storage related operations should function as they did before without regressions.

Extra: use https://stackoverflow.com/questions/24947661/os-x-app-testing-for-sandbox-violations

Breaking changes

Two breaking changes:

  1. BclStorageFile/Folder.SaveBookmarkAsync now returns encoded string instead of original path (only Windows and Linux). MacOS implementation also returns an actual bookmark from the OS.
  2. By default, macOS now uses [NSURL startAccessingSecurityScopedResource] each time when user accesses file, including stream operations (we have a stream wrapper for that). It's not a breaking change by itself, but it's rather a big impact that might introduce unwanted bugs. It's possible to disable this behavior by passing AvaloniaNativePlatformOptions.AppSandboxEnabled = false, which will revert to old non-sandboxed APIs.

Fixed issues

Fixes #6537
Reopens #6540

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049199-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@kekekeks
Copy link
Member

What is wrong with BCL provider to have support for "fake" bookmarks? Weren't those supposed to make it easier to debug relevant code paths on Windows/Linux?

@kekekeks
Copy link
Member

Maybe we could return some mangled path that can't be used as a filename? i. e. just use base64 encoding.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0049947-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member Author

@kekekeks re-enabled BCL bookmarks, as base64 strings.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050065-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050083-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@kekekeks
Copy link
Member

Can we also prefix bookmarks, so we could distinguish between types of those if we make changes later? Since we already have to make behavioral breaking changes here, I'd like to future-proof this area.

@maxkatz6
Copy link
Member Author

@kekekeks bookmarks are now prefixed on all platforms.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050119-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050123-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050128-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 requested a review from kekekeks July 16, 2024 06:06
{
private const int PrefixLength = 16;

private static ReadOnlySpan<byte> FakeBclBookmarkPlatform => "avalonia-bcl"u8;
Copy link
Member

@kekekeks kekekeks Jul 16, 2024

Choose a reason for hiding this comment

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

nit: won't it make more sense to have this prefix in the BCL provider itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

I intended to make EncodeBclBookmark/TryDecodeBclBookmark methods visible outside of BclStorageProvider. Like it's used in macOS backend now:
https://github.com/AvaloniaUI/Avalonia/pull/16090/files/47c3e3cdfa3496a46240fd5141358c81b7b5f340#diff-01a97878717fd9596b20885a6a096cda57ec8f4b1c98d00961d5a1fa348d2c77R137-R140

@kekekeks
Copy link
Member

Are prefixes separated via some character from the rest of the content? Would make more sense for it to be android:<content> than android<content>.

@maxkatz6
Copy link
Member Author

Are prefixes separated via some character from the rest of the content? Would make more sense for it to be android: than android.

Currently, "platform" part of the bookmark length is const 16 bytes.

return true;
}

localPath = null;
Copy link
Member

Choose a reason for hiding this comment

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

We should probably also check for the bookmark format created by older versions. Depending on the platform that would be a StartsWith("/") /Regex.IsMatch(@"^[A-Za-z]:[\\/]") check. Could also follow with a File.Exists check to be sure.

Copy link
Member Author

@maxkatz6 maxkatz6 Jul 16, 2024

Choose a reason for hiding this comment

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

Supporting older bookmarks would make this change much more difficult. Primarily because of iOS and Browser platforms, where bookmarks are the most important. We can't simply check if file exist there. And bookmarks there never been a file path.
BCL implementation of bookmarks is the least important.

Copy link
Member

Choose a reason for hiding this comment

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

So, we are planning to break all apps that were previously using the API, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kekekeks pushed couple of changes.

  1. All bookmarks are now prefixed with "ava."
  2. All bookmarks now have bookmark version information prefixed as well, i.e. "ava.v1.bcl"
  3. If bookmark doesn't have a prefix, attempt to load it as if it's an old format bookmarks (i.e. 11.0, 11.1 state)

So should be better now.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we will ever need to extend bookmarks format, i.e. add extra information or make header bigger or dynamic, v2 can be added without breaking v1.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0050186-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 merged commit 32c2f08 into master Jul 19, 2024
2 checks passed
@maxkatz6 maxkatz6 deleted the macos-bookmarks branch July 19, 2024 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mac App Store SandBox File Access with Bookmarks feature
3 participants