python / cpython

The Python programming language
https://www.python.org/
Other
60.91k stars 29.41k forks source link

Python 3.12 change results in Apple App Store rejection #120522

Open efroemling opened 3 weeks ago

efroemling commented 3 weeks ago

Bug report

Bug description:

This is not a bug in the traditional sense, but I recently went through an ordeal where updates to my app on Apple's App Store (Mac App Store to be specific) started to be rejected after updating my bundled version of Python from 3.11 to 3.12.

It took me quite a while to get to the bottom of this so I wanted to mention it here in case it saves others some pain.

Here is the rejection note I was getting:

Guideline 2.5.2 - Performance - Software Requirements The app installed or launched executable code. Specifically, the app uses the itms-services URL scheme to install an app.

Eventually I learned that the offending files were Lib/urllib/parse.py and its associated .pyc. It seems that an 'itms-services' string was added here in Python 3.12 and it seems that Apple is scanning for this string and auto-rejecting anything containing it (at least in my case).

After removing that string from my bundled copy of Python, my update finally passed review.

Has anyone else run into this? Would it be worth slightly obfuscating that string or something to avoid triggering that rejection? With Python set to officially support iOS in the next release it would be a bummer if it is unable to pass App Store reviews out of the box.

CPython versions tested on:

3.12

Operating systems tested on:

macOS

Linked PRs

ned-deily commented 3 weeks ago

@gpshead @freakboy3742

freakboy3742 commented 3 weeks ago

Whelp... App Store review is the gift that keeps on giving :-)

Thanks for the report @efroemling - and nice work narrowing the cause down to the "magic string". I know how difficult it can be to track these issues down; Apple can be somewhat... opaque... in their decision making processes.

It sounds like you've got an immediate workaround, which is great. If I'm understanding correctly, your fix is to straight-up remove the "itms-services" entry from the list of available options in parse.py - is that correct? Did you try the obfuscation approach (say, rot13-ing the string) or anything else?

Also: when did this rejection manifest in the process? Was it during initial validation for submission, or did you need to go through the full submission process before you received the rejection?

efroemling commented 3 weeks ago

Yes the review process was certainly a bit opaque here. After lots of 'we can provide you with no further information' I finally submitted an appeal for the rejection which at last resulted in Apple telling me that parse.py and its .pyc were the offending files, and at that point it wasn't hard to track down what was going on. Now in retrospect I'm frustrated I didn't think to run a full text search for itms-services over Python itself earlier or stumble across one of the other cases of folks hitting this.

These rejections manifested during full reviews; initial validation showed no problems.

And yeah with my workaround this is not a blocker for me in any way; I just wanted to mention this to hopefully save others some frustration.

And yes in my case I simply removed the full string, so I haven't tested what obfuscation would be necessary to pass the check. If they're simply looking for that char series then perhaps something as simple as 'itms'+'-services' could work?.. (unless that gets optimized into a single string in the .pyc or something).

I should be submitting my next app update within the next few days so I'd be happy to test an obfuscated version if you'd like.

freakboy3742 commented 3 weeks ago

If you're willing and able to act as a canary for testing prospective obfuscation approaches, that would be incredibly helpful.

One thing to keep in mind is that a string concatenation might get optimized by the bytecode compiler (as pre-concatenating 2 static strings is a trivial optimization). If the issue is the string in the pyc file, you might have obfuscated source, but not an obfuscated binary. Using rot13 encoding, .reverse(), or something else that won't be optimized by compilation might be necessary.

gpshead commented 3 weeks ago

Philosophy Q: Do we really want to waste maintainer time poking at a trillion dollar always changing black box illogical (choice adjectives elided) anti-developer approval system that keys off of a clearly useless metric by adding obfuscation into the source of the CPython codebase, pretending that'll avoid Apple's made up problems?

I'd much prefer that kind of transformation be done as a build time step for the iOS platform than embedding obfuscation tricks in the stdlib.

If it's "just this once" that obviously isn't worth creating. (so nothing to block on here) ... But if we find this needs to be done again and again in multiple places over time, we should rethink things and consider making obfuscation a build transformation. And realize that upon doing so the megacorp will eventually just add deobfuscation to their broken by design rejection-bot.

We have no meaningful way of knowing when obfuscations can be removed. They're forever-cruft.

freakboy3742 commented 3 weeks ago

That's a completely reasonable position; and, FWIW, I wouldn't object at all if the general opinion was to classify this as a distribution problem, and leave it to Briefcase (and similar distribution projects) to do whatever post-processing is needed for macOS/iOS distribution purposes.

If that is the decision that we make, I'd suggest we still need to document the problem, as the list of things you need to do to make your Python app App Store compliant should not be wrapped up as internal knowledge in projects like Briefcase (likely requiring independent discovery by every downstream project). To that end, I'd argue it's worth establishing exactly what the patching requirements are (as well as we can, for the rules as they exist right now).

Regardless, from a purely practical perspective, ISTM that some post-processing is going to be needed, because 3.11 is an affected platform; so unless we're going to call this a "security issue" (which would be a bit of a stretch IMHO), there's no way to make a stock Python 3.11 viable in the macOS App Store.

efroemling commented 3 weeks ago

Minor clarification: this change was introduced in 3.12. In my case, 3.11 got through review without any changes. (I had included 3.11 here under "CPython versions tested on:" but just removed it).

I'll go ahead and try an obfuscated string in my next update and will report whether that works.

The decision about where these sorts of workarounds should live is above my pay grade, but I do agree with the sentiment that at least maintaining an easy-to-find list of known hoops one must jump through to use this stuff on Apple's App Stores would be a benefit to developers like myself.

ronaldoussoren commented 3 weeks ago

.., but I do agree with the sentiment that at least maintaining an easy-to-find list of known hoops one must jump through to use this stuff on Apple's App Stores would be a benefit to developers like myself.

I agree, we should either fix the issue so that downstream users don't have to worry about this, or document what should be done to avoid problems.

The latter could be useful regardless of the outcome of this particular issue, for example by clearly documented what entitlements should be enabled when building using the hardened runtime (which is required these days to ship signed apps outside of the App Store as well).

I'd prefer adding a workaround when that doesn't complicate things to much, something like this would IMHO be acceptable (assuming this does pass App Store review):

diff --git a/Lib/urllib/parse.py b/Lib/urllib/parse.py
index c129b0d797..9cac21e14d 100644
--- a/Lib/urllib/parse.py
+++ b/Lib/urllib/parse.py
@@ -59,7 +59,10 @@
                'imap', 'wais', 'file', 'mms', 'https', 'shttp',
                'snews', 'prospero', 'rtsp', 'rtsps', 'rtspu', 'rsync',
                'svn', 'svn+ssh', 'sftp', 'nfs', 'git', 'git+ssh',
-               'ws', 'wss', 'itms-services']
+               'ws', 'wss',
+               # See gh-120522
+               'i!t!m!s!-!s!e!r!v!i!c!e!s'.replace('!', '')
+               ]

 uses_params = ['', 'ftp', 'hdl', 'prospero', 'http', 'imap',
                'https', 'shttp', 'rtsp', 'rtsps', 'rtspu', 'sip',
sobolevn commented 3 weeks ago

I propose a little bit more complex (or simplier depending on your taste):

''.join(reversed(['s', 'e', 'c', 'i', 'v', 'r', 'e', 's', '-', 's', 'm', 't', 'i']))
efroemling commented 3 weeks ago

Just a heads up: my app update containing the obfuscated string just made it through review. I went with the patch @ronaldoussoren posted above ('i!t!m!s!-!s!e!r!v!i!c!e!s'.replace('!', ''))

So we know the workaround works; now its just that philosophy question of whether such a thing should live upstream, get applied to Apple builds specifically, or merely be kept in a list somewhere for people needing to get on the App Store to reference...

freakboy3742 commented 3 weeks ago

Thanks for that update. I've kicked off a discussion to resolve whether we handle this as a documentation issue, a code issue (or just stick our fingers in our ears and start yelling 😝).

freakboy3742 commented 1 week ago

Following the discussion, I've submitted #120984, adding a new build-time option that will patch out the problematic strings from the standard library.

tusharsadhwani commented 1 week ago

my app update containing the obfuscated string just made it through review

Playing a cat and mouse game with apple's code detection algorithms doesn't sound like a good idea. Taking the code out from urllib in ios builds is a wise choice.

terryjreedy commented 1 week ago

There appears to still be a question as to whether the 3.12 backport should be merged or not.

ned-deily commented 6 days ago

Sorry for the late comment here but I believe the change as implemented in #120984 and backports introduces incompatibilities that need to be addressed prior to the next releases.

As implemented, the patch mechanism breaks build scenarios where a single source directory is used for multiple builds. Consider the following:

$ cd source
$ git status
On branch main
Your branch is up to date with 'origin/main'.

nothing to commit, working tree clean
$ mkdir build-1 build-2
$ cd build-1
$ ../configure --with-app-store-compliance [...] # default for iOS, optional for macOS
[...]
checking for --with-app-store-compliance... applying default app store compliance patch
[...]
$ make
patch  --forward --strip=1 --directory=".." --input "Mac/Resources/app-store-compliance.patch"
patching file 'Lib/test/test_urlparse.py'
patching file 'Lib/urllib/parse.py'
[...]
$ git status
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   ../Lib/test/test_urlparse.py
    modified:   ../Lib/urllib/parse.py

$ cd ../build-2
$ ../configure --with-app-store-compliance [...]
[...]
checking for --with-app-store-compliance... applying default app store compliance patch
[...]
$ make
patch  --forward --strip=1 --directory=".." --input "Mac/Resources/app-store-compliance.patch"
patching file 'Lib/test/test_urlparse.py'
Ignoring previously applied (or reversed) patch.
1 out of 1 hunks ignored--saving rejects to 'Lib/test/test_urlparse.py.rej'
patching file 'Lib/urllib/parse.py'
Ignoring previously applied (or reversed) patch.
1 out of 1 hunks ignored--saving rejects to 'Lib/urllib/parse.py.rej'
make: *** [build/app-store-compliant] Error 1

This is not an uncommon scenario: I use such a configuration to build all of the iOS variants from a single source tree.

But, even more problematic, the source directory is now contaminated with the two patched files:

$ git status
On branch main
Your branch is up to date with 'origin/main'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   ../Lib/test/test_urlparse.py
    modified:   ../Lib/urllib/parse.py

We can almost guarantee that, at some point, someone will inadvertently add those modified files to a PR in-progress and they will get checked-in, which is one of the reasons why we try to avoid making changes to the source directory during a build (the goal is that the source directory should be able to be read-only to allow fully out-of-tree builds).

I'm not 100% sure what the best solution here is. We could make the patch rule in the Makefile not fail on errors (if the patch was already applied) but that doesn't address the other concerns.

Without trying to implement it, my thought would be to move the patching from the compile phase of the Makefile to the install phase, i.e. only apply the patch to the installed files not the source directory. As the installed files are always overwritten, that would also eliminate the need for the sentinel file. While that does have the potential drawback of not being able to fully test execution of the patched files without installing, that seems to me to be a minor issue for iOS or macOS as the end product of a build that is going to eventually be part of an app store submission would need to be an installed Python, not one from a build directory.

freakboy3742 commented 6 days ago

@ned-deily Thanks for those details. Responding to let you know I've seen them; but I'm about to pack up my laptop and be offline until late next week, so I won't be able to respond in detail until then.

Committing the diffed files is definitely a risk; although such an error would would break CI (as the patch wouldn't apply cleanly to the macOS CI build).

Patching the installed files definitely sounds like the best option; it's problematic that it won't be inherently tested on macOS, but it would be on iOS, so maybe that's enough to retain confidence in the patch. I'll see if a better answer percolates to the top of my brain while I'm offline, and revisit this next week.

ned-deily commented 6 days ago

it's problematic that it won't be inherently tested on macOS

We should be CI testing an installed framework macOS anyway. Even more incentive to make that happen. :)

erlend-aasland commented 6 days ago

Patching the installed files definitely sounds like the best option [...]

Agreed.

We should be CI testing an installed framework macOS anyway. Even more incentive to make that happen. :)

💯