readium / swift-toolkit

A toolkit for ebooks, audiobooks and comics written in Swift
https://readium.org/mobile/
BSD 3-Clause "New" or "Revised" License
222 stars 96 forks source link

[develop] Opening exploded archive audiobook inside App Group fails with `Publication.OpeningError.unsupportedFormat` in (Designed for iPad) Mac app #434

Closed domkm closed 1 month ago

domkm commented 1 month ago

Describe the bug

Warning: This is weird.

I am transitioning from normal local URLs created by appending path components to FileManager.default.url(for: .applicationSupportDirectory, in: .userDomainMask, appropriateFor: nil, create: false) to app group URLs created by appending path components to FileManager.default.containerURL(forSecurityApplicationGroupIdentifier: APP_GROUP_IDENTIFIER) but found it broke some of my tests. After some investigation, I found that exploded archive audiobooks fail to open when the directory is in an app group. After some more investigation, I found that it only fails while running on a Mac ( iOS simulator and hardware device work correctly).

In other words:

Any idea why this might be happening? If not, I'll work on a reproducible example.

How to reproduce?

Attempt to open a directory of audio files inside an App Group from a Designed for iPad app running on MacOS

Readium version

develop 2fdfea31

OS version

MacOS 14.5

Testing device

MacOS 14.5

Environment

macOS: 14.5
platform: arm64
Xcode 15.4
Build version 15F31d

Additional context

No response

mickael-menu commented 1 month ago

I would expect maybe permission issues to get the children of the directory URLs? But hard to say without debugging. Do you have an error message?

domkm commented 1 month ago

I don't see any error message, just the .unsupportedFormat failure from Streamer.open. The contents of the directory are found when I try in a breakpoint. Could you direct me to the part of the codebase which is failing to create a Publication from the directory so I can dig into what's happening there?

mickael-menu commented 1 month ago

You could take a look at ExplodedArchive and see if it contains the expected entries.

domkm commented 1 month ago

Thanks for the tip! That was what I needed to figure it out.

This is failing due to an insane behavior of App Group URLs on MacOS combined with what I think may be an unnecessary behavior in Readium.

The insane behavior is that there are apparently two types of App Group URLs. When one calls FileManager.containerURL(forSecurityApplicationGroupIdentifier: APP_GROUP_IDENTIFIER), it generates a URL pointing to ~/Library/GroupContainersAlias/APP_GROUP_IDENTIFIER/. However, that GroupContainersAlias component is, unsurprisingly, an alias. The true directory is ~/Library/Group Containers/APP_GROUP_IDENTIFIER/. Even weirder, I haven't been able to find any documentation, even unofficially, on this specific behavior. When listing the contents of a directory in an app group container, the results have aliases resolved. For example, the contents of ~/Library/GroupContainersAlias/APP_GROUP_IDENTIFIER/Assets/ID/Publication/ look like ~/Library/Group Containers/APP_GROUP_IDENTIFIER/Assets/ID/Publication/file.mp3.

The possibly unnecessary Readium behavior is that ExplodedArchive checks that each entry is a child of the root directory when making entries. I think this may be unnecessary because makeEntry is a private function that is only called by the lazy entries var when iterating over the contents of the root directory, so I think every time makeEntry is called the URL passed in must be a child of the root URL. Commenting out root.isParent(of: url) from the guard in makeEntry fixes the failure I am seeing. Regardless of this, perhaps it's worth reconsidering the functionality of FileURL.isParent. Checking the path prefix to determine parentage is not accurate without first resolving aliases in each component of both URLs. If you'd like, I'll contribute either or both of these changes.

mickael-menu commented 1 month ago

The possibly unnecessary Readium behavior is that ExplodedArchive checks that each entry is a child of the root directory when making entries. I think this may be unnecessary because makeEntry is a private function that is only called by the lazy entries var when iterating over the contents of the root directory

Agreed, it looks unnecessary. Feel free to remove the check if that helps with this issue.

It is useful when getting an entry though, to make sure we don't request for example ../../../private/file, so fixing FileURL.isParent might be useful as well. I think you could use URL.resolvingSymlinksInPath() for that. As this will query the filesystem, could you add a FIXME comment to make sure we change FileURL.isParent to async later? Thanks!

domkm commented 1 month ago

URL.resolvingSymlinksInPath() would work and is what I used in my project to work around this issue. Would it make sense to add that in FileURL.init instead of .isParent since there is already normalization occurring in .init, that normalization is relevant for other functionality as well, and it would prevent symlink resolution from needing to be done repeatedly?

https://github.com/readium/swift-toolkit/blob/a3635b5f1eb28a52e73aeb14240479f5979887bc/Sources/Shared/Toolkit/URL/Absolute%20URL/FileURL.swift#L14

- let url = url.standardizedFileURL
+ let url = url.standardizedFileURL.resolvingSymlinksInPath()
mickael-menu commented 1 month ago

I'd prefer to keep it outside the constructor because it will reach the file system and should be asynchronous.

In fact, maybe it would be better to keep isParent as is, and add instead a fileURL.resolvingSymlinks() async -> FileURL to be called before isParent().

domkm commented 1 month ago

Sure, I can do that if you want. I just thought that URL.standardizedFileURL (already called in init) might need to touch the file system to resolve "." and "..". Does it not do that? Anyway, I'll proceed with the separate async version you described unless you say otherwise.

mickael-menu commented 1 month ago

AFAIK it doesn't need to access the file system to resolve . and .., as it's purely syntactic. For example a/b/../c -> a/c.