ios: Add privacy manifest, copying deps' reasons for "required reason APIs" #5859
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisbobbe
added a commit
to chrisbobbe/zulip-flutter
that referenced
this pull request
Apr 23, 2024
Actually, I have a hunch that this will be enough for zulip#612, '"Missing API declaration" warning from Apple'. As that issue says, the only category of "required reason APIs" that Apple is alerting about is "user defaults APIs". There's just one API in this category: `NSUserDefaults`. Using Step 4 in these instructions from a Flutter maintainer -- flutter/flutter#145269 (comment) -- I found that firebase_messaging and flutter_local_notifications, but no other dependencies, have calls to NSUserDefaults without declaring reasons in this category in a "privacy manifest" file. Moreover, unlike in zulip-mobile, our Podfile calls `use_frameworks!`, which should mean we don't have to manually copy anything from our dependencies into a privacy manifest file of our own. See zulip/zulip-mobile#5859 for a link to a mention in the Flutter project about `use_frameworks!`. So I'm hoping that the following upgrades, made here, will address Apple's alerts: - firebase_messaging to v14.9.0, in which that dependency gets a privacy manifest that declares a reason for using "user defaults APIs": https://github.com/firebase/flutterfire/releases/tag/firebase_messaging-v14.9.0 - flutter_local_notifications to v17.1.0. In v17.0.1, a "user defaults APIs" reason declaration was added to this dependency's privacy manifest: https://github.com/MaikuB/flutter_local_notifications/releases Fixes: zulip#612
gnprice
pushed a commit
to chrisbobbe/zulip-flutter
that referenced
this pull request
Apr 24, 2024
Actually, I have a hunch that this will be enough for zulip#612, '"Missing API declaration" warning from Apple'. As that issue says, the only category of "required reason APIs" that Apple is alerting about is "user defaults APIs". There's just one API in this category: `NSUserDefaults`. Using Step 4 in these instructions from a Flutter maintainer -- flutter/flutter#145269 (comment) -- I found that firebase_messaging and flutter_local_notifications, but no other dependencies, have calls to NSUserDefaults without declaring reasons in this category in a "privacy manifest" file. Moreover, unlike in zulip-mobile, our Podfile calls `use_frameworks!`, which should mean we don't have to manually copy anything from our dependencies into a privacy manifest file of our own. See zulip/zulip-mobile#5859 for a link to a mention in the Flutter project about `use_frameworks!`. So I'm hoping that the following upgrades, made here, will address Apple's alerts: - firebase_messaging to v14.9.0, in which that dependency gets a privacy manifest that declares a reason for using "user defaults APIs": https://github.com/firebase/flutterfire/releases/tag/firebase_messaging-v14.9.0 - flutter_local_notifications to v17.1.0. In v17.0.1, a "user defaults APIs" reason declaration was added to this dependency's privacy manifest: https://github.com/MaikuB/flutter_local_notifications/releases Fixes: zulip#612
gnprice
reviewed
Apr 25, 2024
There was a problem hiding this comment.
Thanks for taking care of this!
These changes and the detailed reasoning all look good, except one below.
(I also went through the linked doc at https://developer.apple.com/documentation/bundleresources/privacy_manifest_files/describing_use_of_required_reason_api?language=objc , looking for each of the relevant categories at each of the reasons we're not declaring, and I agree those reasons don't apply.)
ios/PrivacyInfo.xcprivacy
Outdated
| <key>NSPrivacyAccessedAPITypeReasons</key> | ||
| <array> | ||
| <string>3B52.1</string> | ||
| <string>0A2A.1</string> |
There was a problem hiding this comment.
0A2A.1: "Declare this reason if your third-party SDK is providing a
wrapper function around file timestamp API(s) for the app to use,
and you only access the file timestamp APIs when the app calls
your wrapper function. This reason may only be declared by
third-party SDKs. This reason may not be declared if your
third-party SDK was created primarily to wrap required reason
API(s).
Information accessed for this reason, or any derived information,
may not be used for your third-party SDK’s own purposes or sent
off-device by your third-party SDK."
expo-file-system (using the privacy manifest added recently in
expo/expo@b8e40614b, which we haven't taken because Expo
upgrades take a lot of effort)
I think this reason is appropriate for expo-file-system's privacy manifest, but not for ours. As that doc says:
This reason may only be declared by third-party SDKs.
Instead, I think the appropriate thing for our app is to declare whatever reasons apply to our use of that wrapper function. I believe we don't ultimately use that wrapper function at all, so that means we just leave this reason out and don't add any others in its place.
(The other two reasons still belong here, because it's possible we do use these APIs in those ways.)
There was a problem hiding this comment.
I believe we don't ultimately use that wrapper function at all,
Concretely:
The one place expo-file-system appears to expose a file timestamp is in its FileInfo type, returned only by its getInfoAsync:
$ rg -iC12 time node_modules/expo-file-system/src/
$ rg -C4 FileInfo node_modules/expo-file-system/src/
The only references to that function in our code or dependencies are in expo-file-system itself, in jest-expo which is dev-only, and in expo-asset:
$ rg -wl getInfoAsync src/ node_modules/
The mentions within expo-file-system are all part of the implementation of the function, not call sites. And the one call site in expo-asset doesn't look at the timestamp:
$ rg -wC2 getInfoAsync node_modules/expo-asset/src/
So even if we do ever invoke that code in expo-asset (which I suspect we don't) and so call that wrapper function, we never look at the resulting timestamp.
There was a problem hiding this comment.
Cool, makes sense; I'll note this briefly in the commit message and link here for the explanation. Thanks!
… APIs" Apple has a new requirement to declare reasons for using certain potentially privacy-sensitive APIs. It calls these "required reason APIs": https://developer.apple.com/documentation/bundleresources/privacy_manifest_files/describing_use_of_required_reason_api?language=objc Starting May 1, we risk being prevented from publishing until we properly do that: zulip#5847. We don't have much iOS code of our own, and none of it uses these APIs (I've checked). But some of our dependencies do, and we have to handle that. Specifically, we expect to have to aggregate the declared reasons of all our dependencies that use these APIs, and declare them ourself. (That is, of dependencies that are statically linked -- which we expect them to be, since we don't call `use_frameworks!` in our Podfile.) The dependencies declare them with a "privacy manifest" file, and the place we declare them is in a privacy manifest of our own. Below are the reasons we're copying over, noted with the dependencies that declare them: ---- File timestamp APIs ---- (rn-fetch-blob uses some of these APIs, but that project hasn't yet merged a privacy manifest. Currently the only reason code we haven't declared in this category is DDA9.1, and I don't think that applies. The Apple doc says, "Declare this reason to display file timestamps to the person using the device," and we don't do that.) C617.1: "Declare this reason to access the timestamps, size, or other metadata of files inside the app container, app group container, or the app’s CloudKit container." @react-native-async-storage/async-storage (using the privacy manifest from the later version we take in zulip#5856, which is not yet merged as of writing) @sentry/react-native expo-application (using the privacy manifest added recently in expo/expo@b8e40614b, which we haven't taken because Expo upgrades take a lot of effort) react-native (using the privacy manifest added very recently in facebook/react-native@d39712f54, which we haven't taken because RN upgrades take a lot of effort) ~~0A2A.1~~ We considered including this because of expo-file-system but concluded that it's not appropriate for our privacy manifest; details here: zulip#5859 (comment) 3B52.1: "Declare this reason to access the timestamps, size, or other metadata of files or directories that the user specifically granted access to, such as using a document picker view controller." expo-file-system (using the privacy manifest added recently in expo/expo@b8e40614b, which we haven't taken because Expo upgrades take a lot of effort) ---- System boot time APIs ---- 35F9.1: "Declare this reason to access the system boot time in order to measure the amount of time that has elapsed between events that occurred within the app or to perform calculations to enable timers. Information accessed for this reason, or any derived information, may not be sent off-device. There is an exception for information about the amount of time that has elapsed between events that occurred within the app, which may be sent off-device." @sentry/react-native react-native (using the privacy manifest added very recently in facebook/react-native@d39712f54, which we haven't taken because RN upgrades take a lot of effort) ---- Disk space APIs ---- (rn-fetch-blob uses some of these APIs, but that project hasn't merged a privacy manifest yet. Currently the only reasons we haven't declared in this category are 7D9E.1 and B728.1, and I don't think those apply.) 85F4.1: "Declare this reason to display disk space information to the person using the device. Disk space may be displayed in units of information (such as bytes) or units of time combined with a media type (such as minutes of HD video). Information accessed for this reason, or any derived information, may not be sent off-device. There is an exception that allows the app to send disk space information over the local network to another device operated by the same person only for the purpose of displaying disk space information on that device; this exception only applies if the user has provided explicit permission to send disk space information, and the information may not be sent over the Internet." expo-file-system (using the privacy manifest added recently in expo/expo@b8e40614b, which we haven't taken because Expo upgrades take a lot of effort) E174.1: "Declare this reason to check whether there is sufficient disk space to write files, or to check whether the disk space is low so that the app can delete files when the disk space is low. The app must behave differently based on disk space in a way that is observable to users. Information accessed for this reason, or any derived information, may not be sent off-device. There is an exception that allows the app to avoid downloading files from a server when disk space is insufficient." expo-file-system (using the privacy manifest added recently in expo/expo@b8e40614b, which we haven't taken because Expo upgrades take a lot of effort) ---- User defaults APIs ---- (expo-error-recovery uses some of these APIs, but that project hasn't merged a privacy manifest yet, and we don't expect it to because it is deprecated and not used in recent versions of Expo; see expo/expo@87ffd749a. I don't *think* our app ends up using expo-error-recovery; its README makes it sound like something we would be aware of if it were active: "`expo-error-recovery` helps you gracefully handle crashes caused by fatal JavaScript errors.".) CA92.1: "Declare this reason to access user defaults to read and write information that is only accessible to the app itself. This reason does not permit reading information that was written by other apps or the system, or writing information that can be accessed by other apps." @sentry/react-native expo-constants (using the privacy manifest added recently in expo/expo@b8e40614b, which we haven't taken because Expo upgrades take a lot of effort) expo-file-system (using the privacy manifest added recently in expo/expo@b8e40614b, which we haven't taken because Expo upgrades take a lot of effort) react-native (using the privacy manifest added very recently in facebook/react-native@d39712f54, which we haven't taken because RN upgrades take a lot of effort) Fixes: zulip#5847
chrisbobbe
force-pushed
the
pr-privacy-manifest-take-1
branch
from
09a8c96
to
647ccc5
Compare
|
Cool, thanks for the review! Revision pushed. If the note in the commit message seems right, please go ahead and merge. |
|
Thanks! LGTM; merging. |
satyam372
pushed a commit
to satyam372/zulip-flutter
that referenced
this pull request
May 1, 2024
Actually, I have a hunch that this will be enough for zulip#612, '"Missing API declaration" warning from Apple'. As that issue says, the only category of "required reason APIs" that Apple is alerting about is "user defaults APIs". There's just one API in this category: `NSUserDefaults`. Using Step 4 in these instructions from a Flutter maintainer -- flutter/flutter#145269 (comment) -- I found that firebase_messaging and flutter_local_notifications, but no other dependencies, have calls to NSUserDefaults without declaring reasons in this category in a "privacy manifest" file. Moreover, unlike in zulip-mobile, our Podfile calls `use_frameworks!`, which should mean we don't have to manually copy anything from our dependencies into a privacy manifest file of our own. See zulip/zulip-mobile#5859 for a link to a mention in the Flutter project about `use_frameworks!`. So I'm hoping that the following upgrades, made here, will address Apple's alerts: - firebase_messaging to v14.9.0, in which that dependency gets a privacy manifest that declares a reason for using "user defaults APIs": https://github.com/firebase/flutterfire/releases/tag/firebase_messaging-v14.9.0 - flutter_local_notifications to v17.1.0. In v17.0.1, a "user defaults APIs" reason declaration was added to this dependency's privacy manifest: https://github.com/MaikuB/flutter_local_notifications/releases Fixes: zulip#612
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here's an attempt at #5847. It seems like we may have to manually aggregate our dependencies' reason codes and copy them into our own privacy manifest. See for example flutter/flutter#145269 (comment) :
zulip-mobile's Podfile does not call
use_frameworks!.If we have to do this manual copying anyway, maybe it doesn't actually matter whether our dependencies come with the necessary privacy manifests? Maybe we can deal with #5847 with just this PR, then, and forget about the following PRs:
Fixes: #5847