getsentry / sentry-cocoa

The official Sentry SDK for iOS, tvOS, macOS, watchOS.
https://sentry.io/for/cocoa/
MIT License
798 stars 315 forks source link

App required to have NSAppleMusicUsageDescription key in Info.plist #2065

Closed Shiaulis closed 2 years ago

Shiaulis commented 2 years ago

Platform

iOS

Installed

CocoaPods

Version

7.23.0

Steps to Reproduce

With latest Sentry version our app cannot pass check during upload in Appstore Connect since we do not have NSAppleMusicUsageDescription in Info.plist file so we needed to revert to previous Sentry version:

ITMS-90683: Missing Purpose String in Info.plist - Your app‘s code references one or more APIs that access sensitive user data. The app‘s Info.plist file should contain a NSAppleMusicUsageDescription key with a user-facing purpose string explaining clearly and completely why your app needs the data. If you're using external libraries or SDKs, they may reference APIs that require a purpose string. While your app might not use these APIs, a purpose string is still required.

Is it suppose to work like that? Do you expect your customers to add this key?

Expected Result

App doesn't required to have NSAppleMusicUsageDescription

Actual Result

Application failed to upload to Appstore

brustolin commented 2 years ago

Hey @Shiaulis We definitely not expect the user to add this key. We check for permission, but we dont try to request permission. It's odd to me that apple doesn't even allow to check whether the permission exists. We gonna rollout this change.

Thanks for letting us know this.

kevinrenskers commented 2 years ago

The SDK reads MPMediaLibrary.authorizationStatus, which normally should not require that key to present. After all, we're not actually requesting authorization. But it seems App Store Connect's automatic checks have a different opinion. We need to revert this feature. Sorry for the inconvenience!

kevinrenskers commented 2 years ago

@Shiaulis when did you get that error? When you uploaded your archive to App Store Connect? Or when you submitted the app for review?

I just uploaded a small sample app that includes the Sentry SDK to App Store Connect, and I did not get any errors 🤔

Shiaulis commented 2 years ago

we uploading our builds every day to Appstore. We updated our sentry last Monday to latest version. After that every build were failed and we receive the error i've mentioned. So I cannot even see the build in list of uploaded builds. As soon as I reverted latest Sentry version, upload were successful without any errors

Shiaulis commented 2 years ago

Maybe Appstore has different level of checks for your sample app and our app? 🤔

kevinrenskers commented 2 years ago

Maybe a silly question, but did you try uploading it a second time when you got that error?

Could you please try to update the SDK again and upload a build? I'm really curious if that error you got was a fluke, as I am not able to reproduce it.

Shiaulis commented 2 years ago

@kevinrenskers i had about 5-6 failed attempts to upload

Shiaulis commented 2 years ago

Okay I will try once more with latest changes to upload once more. Is it possible that we had some version of SDK between releases? I wonder is it possible for us to rely only on release commits?

kevinrenskers commented 2 years ago

When you upload your build, do you have Bitcode and "strip Swift symbols" both selected?

Shiaulis commented 2 years ago

bitcode disabled, strip symbols enabled

kevinrenskers commented 2 years ago

I can add this code directly to my sample app:

import CoreLocation
import UIKit
import MediaPlayer
import Photos

class PermissionsViewController: UIViewController {
    override func viewDidLoad() {
        let a = MPMediaLibrary.authorizationStatus()
        let b = PHPhotoLibrary.authorizationStatus()
        let c = CLLocationManager.authorizationStatus()
        print("!!!!", a, b, c)

        UNUserNotificationCenter.current().getNotificationSettings { settings in
            print("!!!!", settings)
        }
    }
}

Without adding any kind of permission key in the Info.plist file:

Screen Shot 2022-08-11 at 12 08 13

Uploading the archive goes without a hitch, the build finished processing on App Store Connect. Of course I can't actually submit the sample app to Apple for review, but the upload at least goes through just fine.

Screen Shot 2022-08-11 at 12 19 54

I'm not sure if we should remove a very useful feature of the SDK at this moment without figuring out why you are getting this error. With bitcode disabled or enabled, both upload fine.

kevinrenskers commented 2 years ago

Is it possible that we had some version of SDK between releases? I wonder is it possible for us to rely only on release commits?

I'm not sure how you are integrating the SDK? If you use the master branch then yeah that is possible, but normal releases are immutable. So if you use Cocoapods or Carthage or SPM and stick to release numbers, you shouldn't get a weird in between version :)

kevinrenskers commented 2 years ago

For now I would advice you to simply add that NSAppleMusicUsageDescription key with a dummy text (something like "we don't actually need permission" or whatever). I think that would be the quickest route to new builds with an up-to-date SDK.

In the meantime we can investigate this issue more and see if anyone else is seeing the same problem.

Shiaulis commented 2 years ago

Regarding distribution looks like we were relying on your latest master instead of latest stable version. And I assume it was the core of the issue that we have. I still will check on your latest master if everything works well, but for now I will change the way we pick your changes and will rely only on latest stable version instead

Shiaulis commented 2 years ago

@kevinrenskers still received the same issue on commit a8a2317be9af05f5cc50bcc458edeffb5248d9c7

image
Shiaulis commented 2 years ago

For now I would advice you to simply add that NSAppleMusicUsageDescription key with a dummy text (something like "we don't actually need permission" or whatever). I think that would be the quickest route to new builds with an up-to-date SDK.

In the meantime we can investigate this issue more and see if anyone else is seeing the same problem.

For us we fixed this by relying on latest stable Sentry version that doesn't have this issue

kevinrenskers commented 2 years ago

Right, 7.23.0 doesn't include this feature yet. We'll make sure to thoroughly investigate this before creating a new release. Thanks for the info!

kevinrenskers commented 2 years ago

Hey @Shiaulis, I spoke to Apple about this issue, about whether or not us reading MPMediaLibrary.authorizationStatus would trip up any checks, and if this indeed requires that purpose string to be present. He's pretty much convinced it can't be the SDK:

Based on the detailed description you’ve given, the likelihood is that the single developer who encountered the "Missing Purpose String” rejection did so for a reason unrelated to your SDK.

Specifically, it sounds like the developer might be using another 3rd party framework that (perhaps) contains music playback functionality that the developer isn’t using or even aware of, but is being detected at App Store Connect ingest. It’s even remotely possible that your SDK refers to some APIs that aren’t apparently music-playback-specific, but share the same privacy string as music. (Some of the media library APIs might work this way, for example.)

My recommendation is that you ask this developer to try uploading an app version that does not link your SDK, to see if the error persists. If it does, that suggests it’s their problem not yours.

If it doesn’t, the next step might be to ask the developer to open a TSI about the issue. We’d probably refer them initially to the App Review technical investigations team, but the request would need to come from the developer who controls the app.

I explained that the issue went away when you downgraded the Sentry SDK, so it can't be another SDK at fault here. I have 2 questions for you:

When you downgraded the SDK and uploaded a new build, was that the only change you made? Or did you happen to roll back some other changes in your own code maybe? Just making sure 😅

And the second question is a big favor to ask: would you be willing to open a Technical Support Incident with Apple? You only get two of these per year so we totally understand if you can't or don't want to do this, but it would greatly help us to figure out what exactly is triggering this error. We don't want to release a problematic feature in the SDK of course.

I've uploaded our own sample app to App Store Connect without triggering any errors so sadly we can't request this help ourselves.

kevinrenskers commented 2 years ago

Actually, a third question. In this comment you posted the error you received, and that seems to be an email. When exactly did you get this? Not during the actual upload and validation of the build to App Store Connect, or it would be an error within Xcode. Do you get it after the build is processed? When you make it available to TestFlight? When you submit it for review?

Max-Job commented 2 years ago

I got this error today too when trying to submit an app to testflight.

Sentry 7.23.0

kevinrenskers commented 2 years ago

Sentry 7.23.0 doesn't include the feature yet where we read MPMediaLibrary.authorizationStatus, so I am not sure this is related to the SDK? (And @Shiaulis didn't get the error with Sentry 7.23.0.)

Could you ask the App Review technical investigations team to look into this?

Shiaulis commented 2 years ago

@kevinrenskers

  1. The only thing I reverted was Sentry. And the error is gone.
  2. Sorry, not sure if I have time to open an incident with apple
  3. This error was received to my email. Usually I receive them if anything wrong with particular build i'm trying to upload.

@Max-Job Can you check what commit of Sentry library did you have? Currently our app contains version 7.23.0 and everything works well with it

Max-Job commented 2 years ago

@Max-Job Can you check what commit of Sentry library did you have? Currently our app contains version 7.23.0 and everything works well with it

@Shiaulis Not sure how I can figure out a specific commit. This is how I install the dependency: pod 'Sentry', :git => 'https://github.com/getsentry/sentry-cocoa.git', :modular_headers => true

After installation in podfile.lock:

PODS:
  - Sentry (7.23.0):
    - Sentry/Core (= 7.23.0)
  - Sentry/Core (7.23.0)

SPEC CHECKSUMS:
  Sentry: a0d4563fa4ddacba31fdcc35daaa8573d87224d6
kevinrenskers commented 2 years ago

Yeah that is just the 7.23.0 release 👍

Max-Job commented 2 years ago

I added the tag and the build loaded and is now available in testflight

:tag => '7.21.0'

Shiaulis commented 2 years ago

Well, before we had in our Podfile such line

pod 'Sentry', :git => 'https://github.com/getsentry/sentry-cocoa.git'

and target commit was f895f1b333e9cea6417e6d32284528a0896d66e4

Now I've removed git link to your repo and cocoapods just referencing the last stable version

Max-Job commented 2 years ago

Well, before we had in our Podfile such line

pod 'Sentry', :git => 'https://github.com/getsentry/sentry-cocoa.git'

and target commit was f895f1b

Now I've removed git link to your repo and cocoapods just referencing the last stable version

And did it work? Did this solve the problem?

Shiaulis commented 2 years ago

Latest successful build upload was with this commit https://github.com/getsentry/sentry-cocoa/commit/29fe66621dfa7e0207f186bdb1d0f54420d0fa11 Later today I will be able to try upload with release version

Shiaulis commented 2 years ago

@Max-Job Latest upload with this release works well

Sentry: a0d4563fa4ddacba31fdcc35daaa8573d87224d6
kevinrenskers commented 2 years ago

@Max-Job gets the error with the 7.23 release @Shiaulis does not get the error with the 7.23 release, but does get it with the master branch

I don't think we can pinpoint the exact reason behind this problem until one or both of you involves the App Review technical investigations team, or until we can trigger the same error with a sample app (which we haven't been able to do so far).

kevinrenskers commented 2 years ago

I've added sentry-cocoa to a real app (using the master branch), and it uploaded without errors, and it just finished processing as well. No errors. Same with one of our sample apps; it all uploads and processes just fine.

According to my conversation with Apple, our checking of MPMediaLibrary.authorizationStatus doesn't need the purpose string and shouldn't trigger any errors from App Store Connect either. At this point there is not a lot we can do here. Like I said on Friday: I don't think we can pinpoint the exact reason behind this problem until one or both of you involves the App Review technical investigations team.

Shiaulis commented 2 years ago

I'm okay if you will skip this report for now. Relying on latest release version is fine for us

kevinrenskers commented 2 years ago

Yeah but what's on the master branch will become the next release version of course. And it's the master branch that is triggering the error for you (but only you, so far).

Shiaulis commented 2 years ago

can it be that only we are affected just because the rest of apps are relying on latest stable release?

kevinrenskers commented 2 years ago

That is our fear. That once we create a new release, that we'll get many more reports. On the other hand I've now uploaded 6 builds across 2 apps with the master branch of sentry-cocoa and never get any errors, and Apple assures me it can't be our code. I'm pretty sure it's a bug in App Store Connect's validation, but we'll never find out for sure until someone challenges this error.

Our other option is to remove a useful feature from the upcoming release but even that might not be the solution as Max is getting the same error on release 7.23.0 which doesn't include any sort of music library code. We would really appreciate if you can open a TSI about this.

Shiaulis commented 2 years ago

I will keep TSI in my backlog 🙂 And maybe @Max-Job will also have time to file TSI 🙂

armcknight commented 2 years ago

I wonder if it's because we're simply linking the MediaPlayer/Photos/CoreLocation/UserNotifications frameworks. I found this here:

If you’re using external libraries or SDKs, they may reference APIs that require a purpose string. While your app might not use these APIs, a purpose string is still required for App Review.

We could try dynamically linking them at runtime but I'm not sure that will make it past app review.

ETA: on second thought, I misread that sentence. They mean the external lib may use the APIs, which we don't.

kevinrenskers commented 2 years ago

The guy at Apple said that shouldn't be it, and then we'd also get those same validation errors, right? 🤔

armcknight commented 2 years ago

Yeah I actually misinterpreted that statement. I see no reason we are violating this rule.

kevinrenskers commented 2 years ago

This is what I heard from Apple today:

I don’t see anything in that source file that would trigger that rejection. Do you have evidence that an API in that file is related to the rejection?

Perhaps the next step is for the developer with the rejection to contact App Review directly for clarification of what API is triggering the rejection. The developer can use the "Contact the App Review Team” form:

https://developer.apple.com/appstore/contact/?topic=clarification

When completing the form above, they should include the name of their app and its associated Apple ID.

Unless you can find a way to reproduce the problem on your own developer account, the developer who was rejected should start the investigation.

choulepoka commented 2 years ago

Continuation of original comment

I have emailed Apple App Store Team, asking them to investigate this.

However, perhaps a stopgap measure would be to remove the feature completely, or at least make it optional somehow (like an optional plugin)

Right now, my only valid solution is to fallback to 7.23.0

choulepoka commented 2 years ago

Alternative solution that works:

kevinrenskers commented 2 years ago

Or add NSAppleMusicUsageDescription to your Info.plist, even though it's not actually used. Reading the permissions does not trigger any dialog to the user.

EdgarDegas commented 2 years ago

Also having this issue with Sentry 7.24.0.

The build was uploaded successfully. But after a couple of minutes, I received an email about a missing purpose string:

Dear Developer,

We identified one or more issues with a recent delivery for your app, "<app name, censored>" 3.1.1 (2). Please correct the following issues, then upload again.

ITMS-90683: Missing Purpose String in Info.plist - Your app‘s code references one or more APIs that access sensitive user data. The app‘s Info.plist file should contain a NSAppleMusicUsageDescription key with a user-facing purpose string explaining clearly and completely why your app needs the data. If you're using external libraries or SDKs, they may reference APIs that require a purpose string. While your app might not use these APIs, a purpose string is still required. For details, visit: https://developer.apple.com/documentation/uikit/protecting_the_user_s_privacy/requesting_access_to_protected_resources

Best regards,

The App Store Team

Then I downgraded Sentry to 7.23.0 and everything is OK now.

To sum up, I uploaded 3 builds, with no other changes but sentry's version.

In the end, in App Store Connect, I can see only 2 of them, the first one and the third one (app icon censored):

截屏2022-09-05 16 27 21

Reply if you need any more info.

kevinrenskers commented 2 years ago

Can you please use the "Contact the App Review Team” form: https://developer.apple.com/appstore/contact/?topic=clarification. That way hopefully we can get to the bottom of why the SDK is generating these errors, which should not be happening according to Apple themselves.

EdgarDegas commented 2 years ago

Sure.

malcommac commented 2 years ago

Same here with 7.24. Going back to 7.23, it's okay.

Zandor300 commented 2 years ago

Experiencing the same with a single app of mine. When on 7.24.0, it breaks but 7.23.0 is fine. Currently using 7.23.0 to get my builds through. I updated to 7.24.0 on other apps but those go through no problem.

choulepoka commented 2 years ago

May I propose a solution that is to be implemented directly in the SentrySDK:

That way, people who actually cares about permission being reported by Sentry will get them reported. And people like me who actually do not care for that information being reported by sentry won't get flagged by Apple.

brustolin commented 2 years ago

@choulepoka We are removing the Media permission check until we find an optimal solution. A secondary package sounds like an option.