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

Check our Apple "privacy manifest" #5847

Closed
gnprice opened this issue Apr 3, 2024 · 1 comment · Fixed by #5859
Closed

Check our Apple "privacy manifest" #5847

gnprice opened this issue Apr 3, 2024 · 1 comment · Fixed by #5859

Comments

@gnprice
Copy link
Member

gnprice commented Apr 3, 2024

Just in the last couple of weeks — mid-March — Apple started sending emails warning about a requirement that affects the App Store and TestFlight starting May 1, four weeks from tomorrow. I learned this because we got such an email after today's v0.0.13 release of our Flutter beta app:

The issue there isn't specific to Flutter, though, and it's going to be a very similar story for all package ecosystems used for building iOS apps. That includes React Native.

Our last release of this app was v27.225 on 2024-02-27, a couple of weeks before the warning emails started. It's very likely that as of that version, some of our dependencies lack privacy manifests, and that therefore if we uploaded the same build today we'd get a warning along these lines. In that case if we were to try to make a release after 2024-05-01 with no changes dealing with this, we wouldn't be able to publish it for iOS.

We should try to get ahead of this issue and resolve any such warnings, to maintain our ability to get a new release out promptly if anything urgent comes up. Similarly to the steps at zulip/zulip-flutter#612 :

  • The first step is to upgrade our dependencies with yarn upgrade. That might resolve the issue…
  • … except we're on a fairly old version of React Native at this point (because upgrading to new React Native releases has always been a substantial amount of work, and this app is now in maintenance mode). So it's quite likely that, at least in order to resolve this automatically, there are upstream RN changes we'd need that were made in the last year, likely some made in the last few weeks.
  • So after a yarn upgrade we'll need to try an upload, and see what warnings we get. Then, very likely, the lowest-effort solution is going to be to create our privacy manifest by hand, giving reasons in each of the flagged categories. To identify the reasons, we can do a combination of:
    • look at each of our dependencies (RN itself as well as others), at their latest versions, and see what they put in privacy manifests there;
    • go grepping through our dependencies' source, and/or their binaries (using nm as mentioned in "Missing API declaration" warning from Apple zulip-flutter#612 ), to find what APIs our dependencies use, and then try to determine how they do so.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 4, 2024
This is step one toward zulip#5847.

We hold back react-native-screens at 3.29.0. That's because 3.30
breaks compatibility with RN v0.68 in a certain way, and although
the 3.31 release candidate addresses one source of that
incompatibility --
  software-mansion/react-native-screens#2088
-- there seems to be another. In particular, after the app starts up
on Android, I get errors:

  Error: Requiring module
    "node_modules/react-native-screens/src/index.tsx", which threw an
    exception: Error: Exception in HostObject::get(propName:RNSModule):
    java.lang.UnsatisfiedLinkError:
    dalvik.system.PathClassLoader[DexPathList[[zip file "/data/app/com.zulipmobile.debug-cCKhUjMGcecB6tGNtf5jiQ==/base.apk"],nativeLibraryDirectories=[/data/app/com.zulipmobile.debug-cCKhUjMGcecB6tGNtf5jiQ==/lib/arm64, /data/app/com.zulipmobile.debug-cCKhUjMGcecB6tGNtf5jiQ==/base.apk!/lib/arm64-v8a, /system/lib64]]]
    couldn't find "librnscreens.so"

    […]

and

  TypeError: undefined is not an object (evaluating '_$$_REQUIRE(_dependencyMap[16], "react-native-screens").screensEnabled')

  This error is located at:
      in CardStack (at StackView.tsx:462)
      in KeyboardManager (at StackView.tsx:458)
      in SafeAreaProviderCompat (at StackView.tsx:455)
      in RNGestureHandlerRootView (at GestureHandlerRootView.android.tsx:22)
      in GestureHandlerRootView (at StackView.tsx:454)
      in StackView (at createStackNavigator.tsx:87)
      in StackNavigator (at AppNavigator.js:140)
      in AppNavigator (at ZulipNavigationContainer.js:65)
      in EnsureSingleNavigator (at BaseNavigationContainer.tsx:411)
      in BaseNavigationContainer (at NavigationContainer.tsx:91)
      in ThemeProvider (at NavigationContainer.tsx:90)
      in NavigationContainer (at ZulipNavigationContainer.js:58)
      in ZulipAppContainer (at ZulipMobile.js:83)

      […]

which I don't really understand but I suspect might require us to
adopt React Native's "Fabric" system which we don't want to bother
with for this legacy app.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 4, 2024
This is step one toward zulip#5847.

We hold back react-native-screens at 3.29.0. That's because 3.30
breaks compatibility with RN v0.68 in a certain way, and although
the 3.31 release candidate addresses one source of that
incompatibility --
  software-mansion/react-native-screens#2088
-- there seems to be another. In particular, after the app starts up
on Android, I get errors:

  Error: Requiring module
    "node_modules/react-native-screens/src/index.tsx", which threw an
    exception: Error: Exception in HostObject::get(propName:RNSModule):
    java.lang.UnsatisfiedLinkError:
    dalvik.system.PathClassLoader[DexPathList[[zip file "/data/app/com.zulipmobile.debug-cCKhUjMGcecB6tGNtf5jiQ==/base.apk"],nativeLibraryDirectories=[/data/app/com.zulipmobile.debug-cCKhUjMGcecB6tGNtf5jiQ==/lib/arm64, /data/app/com.zulipmobile.debug-cCKhUjMGcecB6tGNtf5jiQ==/base.apk!/lib/arm64-v8a, /system/lib64]]]
    couldn't find "librnscreens.so"

    […]

and

  TypeError: undefined is not an object (evaluating '_$$_REQUIRE(_dependencyMap[16], "react-native-screens").screensEnabled')

  This error is located at:
      in CardStack (at StackView.tsx:462)
      in KeyboardManager (at StackView.tsx:458)
      in SafeAreaProviderCompat (at StackView.tsx:455)
      in RNGestureHandlerRootView (at GestureHandlerRootView.android.tsx:22)
      in GestureHandlerRootView (at StackView.tsx:454)
      in StackView (at createStackNavigator.tsx:87)
      in StackNavigator (at AppNavigator.js:140)
      in AppNavigator (at ZulipNavigationContainer.js:65)
      in EnsureSingleNavigator (at BaseNavigationContainer.tsx:411)
      in BaseNavigationContainer (at NavigationContainer.tsx:91)
      in ThemeProvider (at NavigationContainer.tsx:90)
      in NavigationContainer (at ZulipNavigationContainer.js:58)
      in ZulipAppContainer (at ZulipMobile.js:83)

      […]

which I don't really understand but I suspect might require us to
adopt React Native's "Fabric" system which we don't want to bother
with for this legacy app.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 15, 2024
…test)

Changelog:
  https://github.com/react-native-async-storage/async-storage/releases

This new version has a "privacy manifest", which should help us
toward zulip#5847.

For why this upgrade is unlikely to break things, see:
  zulip#5309 (comment)

For why the version was constrained with ~ instead of ^ see
47b7ead, an Expo SDK upgrade:

  [T]he `expo upgrade` command
  went through a phase it described as "Updating packages to
  compatible versions (where known)," and that changed several of our
  dependency ranges, even of non-Expo things.

It would actually be better to remove the dependency entirely. It's
our legacy way of storing data on the device, and we have a
migration to the new way that's now been active for over two years
(see caf3bf9). The proportion of our users who still benefit from
what this dependency offers will be vanishingly small at this point.

But I'm not totally comfortable removing it without more careful
thought than we have time for right now in this legacy codebase.
(One step we'd want to consider, if doing that, is adding and
releasing a migration to clear out the contents of the legacy
storage.)

Tested on my iPhone and on the office Android device, on both a
first and a subsequent startup of the app. No crashes or errors
observed.

Related: zulip#5847
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 15, 2024
This unfortunately causes a peer-dep warning:

  warning " > @react-navigation/material-top-tabs@6.6.13" has
    incorrect peer dependency "@react-navigation/native@^6.0.0".

But no issues have yet appeared in manual testing, and that's
consistent with the note on the React Navigation upgrade guide that
says, "To make upgrading easier, it is possible to mix packages from
the `6.x.x` and `5.x.x` version ranges.":
  https://reactnavigation.org/docs/upgrading-from-5.x#note-on-mixing-react-navigation-5-and-react-navigation-6-packages

Changelog:
  https://github.com/react-navigation/react-navigation/blob/main/packages/material-top-tabs/CHANGELOG.md

Done by reading and following the relevant parts of the React Nav
upgrade guide, including general information at the top and also the
section specific to Material Top Tabs:
  https://reactnavigation.org/docs/upgrading-from-5.x/#material-top-tab-navigator

Done now because it lets us get rid of react-native-reanimated, as
foreshadowed in our React Nav 6 upgrade issue:
  zulip#4936 (comment)

That's helpful because the old version of react-native-reanimated
that we're on uses a certain iOS API that Apple identifies as
privacy-sensitive, triggering a "Privacy Manifest" requirement:
  software-mansion/react-native-reanimated#5819
For zulip#5847, we're working on reducing and handling such requirements.

That API usage is removed in v2.9.0-rc.0 of Reanimated (see
just-linked PR), but I didn't pursue upgrading it because that path
seems to require abandoning remote JS debugging, at least according
to a note from 2022. For details on that, search for
"react-native-reanimated" here:
  zulip#5441

Related: zulip#5847
Fixes-partly: zulip#4936
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 16, 2024
Unfortunately this causes a peer-dependency warning:

  warning " > react-native-simple-toast@3.3.1" has incorrect peer
    dependency "react-native@>=0.71.0".

but no issues are observed in manual testing of this library's
functionality on iOS or Android.

While we're at it, use TsFlower for the types. The types tell us
that `Toast.show`'s duration parameter is required; so, include it,
even though the implementation has logic to fall back to Toast.SHORT
if the argument isn't passed.

This lets us pull in the latest version of the "Toast" pod for iOS;
that's version 4.1.1. That version has a "privacy manifest" --

  scalessec/Toast@401141d79
  scalessec/Toast@607b12954
  scalessec/Toast@85b492dd8

-- which should hopefully help us with zulip#5847. Granted, the Toast
pod's privacy manifest file seems pretty boring; I think it just
declares that the library doesn't access any sensitive data. Still,
"Toast" is explicitly included in a list published by Apple of
"commonly used SDKs" that require their own privacy manifest:
  https://developer.apple.com/support/third-party-SDK-requirements/
(In fact, I think it's the only one of zulip-mobile's dependencies
that appears in that list.)
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 16, 2024
Unfortunately this causes a peer-dependency warning:

  warning " > react-native-simple-toast@3.3.1" has incorrect peer
    dependency "react-native@>=0.71.0".

but no issues are observed in manual testing of this library's
functionality on iOS or Android.

While we're at it, use TsFlower for the types. The types tell us
that `Toast.show`'s duration parameter is required; so, include it,
even though the implementation has logic to fall back to Toast.SHORT
if the argument isn't passed.

This lets us pull in the latest version of the "Toast" pod for iOS;
that's version 4.1.1. That version has a "privacy manifest" --

  scalessec/Toast@401141d79
  scalessec/Toast@607b12954
  scalessec/Toast@85b492dd8

-- which should hopefully help us with zulip#5847. Granted, the Toast
pod's privacy manifest file seems pretty boring; I think it just
declares that the library doesn't access any sensitive data. Still,
"Toast" is explicitly included in a list published by Apple of
"commonly used SDKs" that require their own privacy manifest:
  https://developer.apple.com/support/third-party-SDK-requirements/
(In fact, I think it's the only one of zulip-mobile's dependencies
that appears in that list.)

Related: zulip#5847
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 23, 2024
… 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: "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)

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)
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 23, 2024
… 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: "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)

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
@gnprice
Copy link
Member Author

gnprice commented Apr 26, 2024

Seems like #5859 indeed fixed this — we sent v27.227 to beta today, including in TestFlight, and didn't get one of these warning emails.

(That contrasts with v27.226, an iOS-only alpha-only release we made a couple of weeks ago in order to test this issue, where we did.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant