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

Correct Mimetype for LCPL UTI #194

Closed
tooolbox opened this issue Apr 25, 2020 · 6 comments
Closed

Correct Mimetype for LCPL UTI #194

tooolbox opened this issue Apr 25, 2020 · 6 comments
Labels
bug Something isn't working

Comments

@tooolbox
Copy link
Contributor

The app's plist file defines the org.readium.lcpl UTI. It gives as the mimetype for this UTI application/vnd.readium.lcp.license.v1.0 which is incorrect, because per the spec the correct one is application/vnd.readium.lcp.license.v1.0+json.

By a series of unfortunate events, if both the test app and another app (with correct mimetype) are installed on a device, the test app can hijack the UTI (since it can only be registered once with the system).

Further, then, if a particular OPDS feed contains acquisition links with no .lcpl extension, the Publication.downloadLinks getter then falls back to mimetype, and if your mimetype is hijacked and wrong, then you can't download anything, which is possibly confusing :)

Point is, would be good to correct the LCPL mimetype in the plist.

mickael-menu referenced this issue in readium/r2-testapp-swift Apr 25, 2020
@mickael-menu mickael-menu added the bug Something isn't working label Apr 25, 2020
@mickael-menu
Copy link
Member

mickael-menu commented Apr 25, 2020

Ha nice catch, I quickly fixed it in develop.

By a series of unfortunate events, if both the test app and another app (with correct mimetype) are installed on a device, the test app can hijack the UTI (since it can only be registered once with the system).

I'm actually more worried about the apps that copied the test app's Info.plist and won't update it :/

Further, then, if a particular OPDS feed contains acquisition links with no .lcpl extension, the Publication.downloadLinks getter then falls back to mimetype, and if your mimetype is hijacked and wrong, then you can't download anything, which is possibly confusing :)

I've been working on a Swift implementation of a new Format API proposal which will replace the media type check done in Publication.downloadLinks, and not rely on the UTIs in Info.plist. However, I forgot about this DocumentTypes which should be deprecated as well.

Your issue gave me a cool idea: registering dynamically the UTIs referenced in Info.plist to Format to have them automatically sniffed when calling Format.of() with the right extension or media type.

Meaning that if you have that in your Info.plist:

<dict>
    <key>UTTypeIdentifier</key>
    <string>org.readium.lcpl</string>
    <key>UTTypeDescription</key>
    <string>Lightweight Content Protection License</string>
    <key>UTTypeTagSpecification</key>
    <dict>
        <key>public.filename-extension</key>
        <array>
            <string>lcpl</string>
        </array>
        <key>public.mime-type</key>
        <string>application/vnd.readium.lcp.license.v1.0+json</string>
    </dict>
</dict>

Then it will automatically be registered in Format such that:

let format1 = Format.of(mediaTypes: ["application/vnd.readium.lcp.license.v1.0+json"])
let format2 = Format.of(fileExtensions: ["lcpl"])

format1 == format2 == Format(
    name: "Lightweight Content Protection License",
    mediaType: MediaType("application/vnd.readium.lcp.license.v1.0+json"),
    fileExtension: "lcpl"
)

@tooolbox
Copy link
Contributor Author

tooolbox commented Apr 25, 2020

Ha nice catch, I quickly fixed it in develop.

Nice that was fast!

I've been working on a Swift implementation of a new Format API proposal which will replace the media type check done in Publication.downloadLinks, and not rely on the UTIs in Info.plist.

Wow, that's quite a thorough document. I confess I didn't read the whole thing, but seems like you're handling all of the corner cases.

Your issue gave me a cool idea: registering dynamically the UTIs referenced in Info.plist to Format to have them automatically sniffed when calling Format.of() with the right extension or media type.

Sounds good as well.

One further idea, is that in the the DocumentTypes business in R2, the mimetypes are gotten from the UTIs via a PreferredValueForKey (or whatever) call instead of AllValuesForKey so you only get one mimetype. If you changed it to AllValues then you could have in the .plist the version with & without the +json which may be slightly more backwards-compatible, in case there are people out there using the wrong version.

Of course, that would merely be a stopgap until your new Format API goes into effect, but it's an idea.

@mickael-menu
Copy link
Member

Wow, that's quite a thorough document. I confess I didn't read the whole thing, but seems like you're handling all of the corner cases.

Yes in particular for the media type comparison, we tried to be a bit more systematic. The gist of the improvements that this API brings are:

  • a centralized place for everything related to formats, it was a bit spread out
  • content sniffing to detect a format when you only have a file without extension
  • extensibility for custom formats provided by reading apps
  • handling edge cases with media types comparisons, simple equality check is not enough since they can have parameters in different order and they are case-sensitive only for the parameter values

One further idea, is that in the the DocumentTypes business in R2, the mimetypes are gotten from the UTIs via a PreferredValueForKey (or whatever) call instead of AllValuesForKey so you only get one mimetype. If you changed it to AllValues then you could have in the .plist the version with & without the +json which may be slightly more backwards-compatible, in case there are people out there using the wrong version.

Good idea, actually this will also be useful after the new Format API is available, since reading apps might still use DocumentTypes.contentTypes despite the deprecation warning.

@tooolbox
Copy link
Contributor Author

Okay great.

If you fixed the plist entry I suppose this ticket is done, good to close?

@mickael-menu
Copy link
Member

Yes, I just wanted to make the changes mentioned in DocumentTypes. I just pushed them, you can do a quick review if you want:

I actually kept DocumentTypes for this part, because I think it makes more sense to recognize only the formats declared by the reading app, instead of all the formats supported by Readium. Because an app might want to support only a subset of Readium.

You're free to close this issue if there's nothing further to discuss with these changes.

@tooolbox
Copy link
Contributor Author

Okay, I read through those two commits/diffs. Wow, that's quite a lot of code! It looks good to me. I agree with the concept of consulting the info.plist internally rather than what has been registered with the system by potentially multiple apps.

We're good now, closing!

@mickael-menu mickael-menu transferred this issue from readium/r2-testapp-swift Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants