Repository navigation
Avoid dormant ATT signatures in app binaries - #496
Conversation
0cbc92a to
4ed0dfc
Compare
|
PR author is not in the allowed authors list. |
There was a problem hiding this comment.
ℹ️ No blocking issues — three deferrable observations below.
Reviewed changes — the full diff at 4ed0dfce4, plus the surrounding permissions tree and the mechanism by which each identifier reached the shipped binary.
PlistKey.trackingis ROT13-encoded — the bare"NSUserTrackingUsageDescription"literal (which lands verbatim in__TEXT,__cstring) is replaced by a mangled constant decoded at runtime. The encoding round-trips correctly and the new test pins it.FakeTrackingManagerdeleted — its@objcmembers emitted the literal ATT selectorstrackingAuthorizationStatusandrequestTrackingAuthorizationWithCompletionHandler:into__TEXT,__objc_methname, which is runtime metadata thatstripnever removes. This was the guaranteed leak, and removing it is the load-bearing part of the fix.requestTrackingAuthorization()renamed torequestAuthorization()— the stdlib'swithCheckedContinuation(function: String = #function, …)expanded the old name into a literal"requestTrackingAuthorization()"at the call site, which is precisely the third string the issue reporter recovered withstrings.TrackingManagerProxygains a designated initializer — the class lookup moves from astatic varto an injected instancelet, replacing the?? FakeTrackingManager.selffallback with aguard let … else { return notDetermined }in both entry points.- Project and changelog housekeeping —
project.pbxprojshows a cleanxcodegenregeneration (4 entries removed, no ID churn), and the changelog entry sits under4.16.2, matchingsdkVersionand the podspec.
I checked the parts most likely to go wrong and they hold up: the nil-class path returns notDetermined exactly as the old fake-class round trip did, the "class exists but selector missing" branch is structurally untouched, declaring a designated init on an NSObject subclass still leaves TrackingManagerProxy() resolvable through the default argument, and the two new tests assert exact equality against FakeTrackingAuthorizationStatus.notDetermined.rawValue so they fail if the guard regresses. The plaintext strings remaining in comments and in the test target don't ship. trackingAuthorizationStatus() keeps the exact ATT name but isn't @objc and has no #function site, so it survives only as a substring of an internal mangled symbol — not worth renaming.
ℹ️ Two @objc members on TrackingManagerProxy still emit dormant selector metadata
trackingStatusSelectorName and requestTrackingSelectorName (TrackingManagerProxy.swift:39 and :43) are marked @objc, so their names are emitted into __objc_methname — the same section whose contents this PR just went to the trouble of deleting. They aren't Apple API names, so they won't trip ITMS-90683, but the class is internal and both properties are read only from Swift within the same file, so the attribute buys nothing.
Technical details
# Drop unused `@objc` from `TrackingManagerProxy`
## Affected sites
- `Sources/SuperwallKit/Permissions/Handlers/Tracking/TrackingManagerProxy.swift:39` — `@objc var trackingStatusSelectorName`
- `Sources/SuperwallKit/Permissions/Handlers/Tracking/TrackingManagerProxy.swift:43` — `@objc var requestTrackingSelectorName`
## Required outcome
- No Objective-C method metadata is emitted for members of `TrackingManagerProxy`, consistent with the PR's stated goal of removing dormant ATT-adjacent signatures from the framework binary.
## Suggested approach
- Remove `@objc` from both properties. Both are read only from Swift (`trackingAuthorizationStatus()`, `requestAuthorization()`, and `TrackingManagerProxyTests`), and neither is dispatched by selector.
- With no `@objc` members left, `: NSObject` on the class is also unnecessary and the explicit `super.init()` in the new initializer can go with it. `ContactStoreProxy` carries the same unused `@objc`/`NSObject` pairing if you want to keep the two proxies symmetric — that one is pre-existing and out of scope here.ℹ️ Nothing in CI prevents a plaintext identifier from coming back
The whole bug class is "an Apple privacy identifier reaches the shipped binary", and that is invisible to the unit test suite — the new tests pin the ROT13 round-trip but say nothing about what the compiler emits. Verification was a manual strings pass by the author, so a future edit that inlines PlistKey.tracking back to a literal or adds another @objc shim would regress this silently and only surface as an App Store Connect rejection for a customer.
Technical details
# Add a binary-scan guard for privacy identifiers
## Affected sites
- No single line — the gap is the absence of a check. `scripts/build.sh` already builds `SuperwallKit.framework`.
## Required outcome
- A CI step fails the build when a forbidden literal appears in the compiled framework, so the regression is caught in the repo rather than in a customer's App Store submission.
## Suggested approach
- After the existing framework build, run `strings` over the binary and fail on any of `NSUserTrackingUsageDescription`, `ATTrackingManager`, `requestTrackingAuthorizationWithCompletionHandler:`, or `trackingAuthorizationStatus`.
- Keep the forbidden list in one place so the sibling handlers (contacts, location) can be added if the answer to the scope question below is "yes".
## Open questions for the human
- Is the release CI workflow the right home for this, or does it belong in the pre-push hook where it would run more often but cost a full framework build?ℹ️ The sibling permission handlers still carry the exact pattern this PR removes
FakeContactStore and FakeLocationManager emit the exact CNContactStore and CLLocationManager selectors as @objc metadata, and every PlistKey other than tracking is still a plaintext literal. Issue #495 explicitly cites #421 — an App Store warning about microphone permission — as precedent, which suggests ATT may not be the only key Apple's scanner reacts to. Whether that matters is a product call, not something the diff can answer.
Technical details
# Decide whether the ATT hardening generalizes to the other permission handlers
## Affected sites
- `Sources/SuperwallKit/Permissions/Handlers/Contacts/FakeContactsStore.swift:12,17` — `@objc` members emitting `authorizationStatusForEntityType:` and `requestAccessForEntityType:completionHandler:`, the literal `CNContactStore` selectors.
- `Sources/SuperwallKit/Permissions/Handlers/Location/FakeLocationManager.swift:14,18,22` — `@objc` members emitting `authorizationStatus`, `requestWhenInUseAuthorization`, and `requestAlwaysAuthorization`.
- `Sources/SuperwallKit/Permissions/PermissionHandler.swift:18-28` — `camera`, `photoLibrary`, `contacts`, `locationWhenInUse`, `locationAlways`, and `microphone` remain plaintext literals; only `tracking` is mangled.
## Required outcome
- An explicit decision, recorded somewhere durable: either ATT is uniquely detected by App Store Connect and the asymmetry is intentional, or the same treatment (delete the fake ObjC shim, mangle the plist key) is applied to the sibling handlers as a follow-up.
## Open questions for the human
- Did #421 establish that non-ATT usage-description keys also trigger App Store Connect warnings? If so, this PR fixes one instance of a pattern that has five more.
- If the answer is "ATT only", a short comment on `PlistKey.mangledTracking` explaining why only this key is encoded would stop the next reader from either mangling all of them or un-mangling this one.Claude Opus | 𝕏
… too The tracking fix removed one instance of a pattern the other three permission handlers still carried. `FakeAudioSession`, `FakeLocationManager` and `FakeContactStore` were runtime stand-ins whose `@objc` members had to mirror Apple's real selectors to be reachable, so each emitted those names into `__objc_methname` — the section the proxies' ROT13 mangling exists to keep them out of. The microphone one partly undid the mangling added for superwall#421. Guard on the missing class instead, as the tracking proxy now does. Every fallback returns what the fake returned, with one exception worth stating: `requestWhenInUseAuthorization()` reported success when CoreLocation was absent, having called the fake's no-op, and its caller then waited forever for a delegate callback that was never coming. It now reports failure and the caller resumes `.unsupported`. `FakeASIdManager` stays: it is a compile-time shim, and `sharedManager` fingerprints nothing. Drop the unused `@objc` on the proxies' own selector-name properties for the same reason, and take the class by injection so the guarded paths are testable. The plist keys other than tracking stay legible — no scanner is known to react to them — with a note on the encoded one saying why it is alone. Add scripts/scan-privacy-signatures.sh, run in CI after the tests. Whether a name reaches the binary depends on what the compiler emits, so no unit test can see it. It reads the ObjC metadata and cstring sections rather than running `strings`: Swift mangles internal symbols from source names, so a debug binary legitimately contains `trackingAuthorizationStatus` inside the proxy's own symbols and matching on that would fail forever with nothing to fix. Verified both ways — it passes on this build and reports all six names on the last develop build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Important
The microphone handler still emits an Apple API name into the binary through the exact mechanism this PR's ATT rename fixed, and the new scanner is written so that it won't catch it.
Reviewed changes — the delta since the prior review at 4ed0dfce4, which extends the ATT hardening to the sibling permission handlers and adds a CI guard.
- Sibling fake classes deleted —
FakeAudioSession,FakeContactStore, andFakeLocationManagerare removed, andAudioSessionProxy,ContactStoreProxy, andLocationManagerProxyeach gain a designatedinit(<x>Class:)with aguard letearly return in place of the?? Fake….selffallback. @objcdropped from every proxy's selector-name property — addresses the first item of the prior review; the four proxies no longer emit their own names into__objc_methname.- New binary scanner wired into CI —
scripts/scan-privacy-signatures.shrunsotool -v -sover__objc_methname,__objc_classname, and__cstringandgrep -Fs aFORBIDDENlist; the test workflow now builds with-derivedDataPath .buildso the step has an artifact to scan. Addresses the second item of the prior review. - Scope decision recorded — the comment on
PlistKey.mangledTrackingexplains why only the tracking key is encoded, answering the third item of the prior review. - Fake-class test suites replaced — each is now a
*MissingClassTestssuite that constructs the proxy withniland pins the exact sentinel the guard returns, plus new coverage forContactStoreProxy, which had none.
I traced the four handlers end to end across all three runtime cases (class resolves, class absent, class present but selector missing) and the values the caller observes are unchanged, with one deliberate exception: LocationManagerProxy.requestWhenInUseAuthorization() / requestAlwaysAuthorization() now return false instead of true when CoreLocation is absent, so requestLocationPermission() resumes with .unsupported rather than hanging forever waiting on a delegate callback the fake never made. That is a fix, and the test comment documents it. All 29 construction sites still compile through the new inits' default arguments. The scanner's shell mechanics are fail-closed, not fail-open — set -e with the SECTIONS=$( { … } ) substitution, the cd before $1 is resolved, and bash 3.2 empty-array handling all degrade to a red build rather than a silent pass.
ℹ️ LocationPermissionDelegate still ships CoreLocation's exact delegate selectors
locationManagerDidChangeAuthorization(_:) and locationManager(_:didChangeAuthorization:) (LocationPermissionDelegate.swift:23 and :31) are @objc, so locationManagerDidChangeAuthorization: and locationManager:didChangeAuthorization: — verbatim CLLocationManagerDelegate selectors — land in __objc_methname, the same section this PR just emptied of the fake classes' selectors. Unlike those, these cannot simply lose the attribute: CLLocationManager dispatches them by selector, so the runtime needs the metadata. They are also absent from FORBIDDEN, so the new scanner reports green with them present.
Technical details
# Decide how `LocationPermissionDelegate`'s ObjC selectors fit the hardening story
## Affected sites
- `Sources/SuperwallKit/Permissions/Handlers/Location/LocationPermissionDelegate.swift:23` — `@objc func locationManagerDidChangeAuthorization(_:)` emits `locationManagerDidChangeAuthorization:`.
- `Sources/SuperwallKit/Permissions/Handlers/Location/LocationPermissionDelegate.swift:31` — `@objc func locationManager(_:didChangeAuthorization:)` emits `locationManager:didChangeAuthorization:`.
- `Sources/SuperwallKit/Permissions/Handlers/Location/LocationPermissionDelegate.swift:41` — `manager.value(forKey: "authorizationStatus")` puts the literal `authorizationStatus` in `__cstring`.
- `scripts/scan-privacy-signatures.sh:38-52` — `FORBIDDEN` was populated from the four proxy files, not from a sweep of every remaining `@objc` member under `Sources/SuperwallKit/Permissions/`.
## Required outcome
- Either these selectors stop reaching the binary, or the asymmetry is recorded so the next reader doesn't assume the scanner's green means "no CoreLocation names present".
## Suggested approach (optional)
- To remove them: build the delegate class at runtime with `objc_allocateClassPair` / `class_addMethod` and `imp_implementationWithBlock`, registering the ROT13-decoded selector names — the same trick the proxies already use for lookups.
- To accept them: say so in a comment on the class and add a short note to `scan-privacy-signatures.sh` explaining that delegate-callback selectors are out of the list's scope, so the omission reads as a decision rather than an oversight.
## Open questions for the human
- Is a `CLLocationManagerDelegate` callback selector the sort of thing App Store Connect's scanner reacts to, or is the trigger specifically the `NS…UsageDescription` key? Issue #495 only establishes the latter.ℹ️ Nitpicks
.github/workflows/tests.yml:5-9— thepaths:filter matches**/*.swift,project.yml, and the workflow itself, but notscripts/**. A push that only editsscan-privacy-signatures.shruns nothing, so a change that weakens the guard is never exercised by the guard.LocationManagerProxy.swift:33— the other three proxies inlineNSClassFromString(…rot13())directly in the init's default argument; this one keeps astatic var locationManagerClasswhose only remaining use is that default argument. Inlining it would make the four read identically.Tests/SuperwallKitTests/Permissions/MicrophonePermissionTests.swift:116—ContactStoreProxyMissingClassTestssits in the microphone test file.Location/andTracking/have their own directories; aContacts/ContactStoreProxyTests.swiftwould match.CHANGELOG.md:19— "Keeps unused microphone, location, and contacts permission code out of your app's binary" reads as dead-code elimination. What actually changed is that Apple's API names no longer appear in it.
Claude Opus | 𝕏
Renaming a proxy method away from Apple's name only works if every method gets the treatment: `withCheckedContinuation`'s `function: String = #function` default expands the enclosing method name into a string literal, which put `requestRecordPermission()` — and, more weakly, `requestAccess()` — into __cstring after the fakes were already gone. The scanner missed the first because its entry carried the ObjC trailing colon and `grep -F` matches exact substrings. Rename both to `requestPermission()`, and switch the FORBIDDEN list to bare names so one entry catches a selector, a #function expansion, or any future suffix. Validating the bare entries against the binary surfaced a scope boundary worth recording: the camera handler calls `AVCaptureDevice.requestAccess(for:)` directly, so `requestAccessForMediaType:completionHandler:` is legitimately present — camera, photos, and notifications never joined the proxy scheme. The list notes that, keeps the contacts entry to `requestAccessForEntityType`, and documents the other deliberate omission: `LocationPermissionDelegate`'s two callback selectors, which CLLocationManager dispatches by name at runtime and which therefore cannot lose their metadata. Its KVC key now decodes from the existing mangled constant instead of sitting in __cstring as plaintext. Also from the review: run the tests workflow when only `scripts/**` changes, so edits to the scanner are exercised by the scanner; inline the location proxy's class lookup into its init default to match the other three; move the contacts proxy tests into their own file beside Location/ and Tracking/; and reword the changelog line that read as dead-code elimination. Scan verified both ways again: green here, and 8 names flagged on the last develop build now that bare entries also catch the #function forms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
ℹ️ No blocking issues — the two
#functionleaks are closed correctly. One scope question and one verification gap below.
Reviewed changes — the delta since the prior review at cbbb2535b, which closes the remaining #function leaks, hardens the scanner's match list, and clears the prior nits.
- The two remaining
#functionleaks are renamed away —AudioSessionProxy.requestRecordPermission()andContactStoreProxy.requestAccess()both becomerequestPermission(), with their single call sites updated. Each carries a comment naming the mechanism, so the next reader won't rename them back. FORBIDDENrewritten as bare names —requestRecordPermission,requestTrackingAuthorization,trackingAuthorizationStatus,recordPermission, andrequestAccessForEntityTypelose their selector colons and argument suffixes, so one entry now catches both the@objcselector form and any#functionexpansion. The accompanying comment records what is deliberately absent and why.LocationPermissionDelegate's ObjC selectors are recorded as accepted — a class doc comment explains thatCLLocationManagerdispatches those callbacks by selector so the metadata must exist, and cross-references the scanner's omission.- The last plaintext CoreLocation key is mangled —
value(forKey: "authorizationStatus")now decodesLocationManagerProxy.mangledAuthorizationStatusSelectorat runtime. - Prior nits cleared —
scripts/**added to the workflowpaths:filter,LocationManagerProxy's leftoverstatic var locationManagerClassinlined into the init default argument,ContactStoreProxyMissingClassTestsmoved into a newPermissions/Contacts/ContactStoreProxyTests.swiftwith a matching cleanxcodegenregeneration, and the CHANGELOG line reworded.
I re-swept Sources/SuperwallKit/ for all three leak mechanisms and found nothing further in the permissions tree: all 18 withCheckedContinuation / withCheckedThrowingContinuation call sites have enclosing names that don't match an Apple API, no proxy member is @objc any more, and every class and selector literal is ROT13'd. I also checked the broadened bare names for false positives and expect none — Swift's mangled symbol names live in the LC_SYMTAB string table under __LINKEDIT and its reflection metadata in __swift5_*, neither of which otool -s __TEXT <sect> can reach, so TrackingManagerProxy.trackingAuthorizationStatus() and AudioSessionProxy.recordPermission() stay invisible to the scan; the selectors that direct AVCaptureDevice / PHPhotoLibrary / UNUserNotificationCenter calls do put in __objc_methname contain no FORBIDDEN substring (requestAccessForEntityType ≠ requestAccessForMediaType); and the three scanned sections are all S_CSTRING_LITERALS, so otool -v prints strings rather than hex and grep -F can match. The renames break no callers, the new contacts test file introduces no duplicate suite names, and ROT13("nhgubevmngvbaFgnghf") is authorizationStatus.
ℹ️ The AdSupport proxy still carries the @objc shim pattern this PR deletes elsewhere
FakeASIdManager (ASIdManagerProxy.swift:21-26) is an @objc stand-in mirroring ASIdentifierManager's sharedManager selector — structurally the same thing as the four Fake* classes this PR removed, sitting in the IDFA path, which is the closest neighbour there is to the ATT problem #495 describes. Unlike those four it can't simply lose the attribute: it is what makes classType.sharedManager() at :44 typecheck through AnyObject dynamic member lookup, so deleting it breaks the build. It's also absent from FORBIDDEN, whose new "deliberately absent" comment enumerates camera, photos, and notifications but not AdSupport, so a green scan reads as a completeness claim it doesn't make.
Technical details
# `FakeASIdManager` is outside both the hardening and the scanner's recorded scope
## Affected sites
- `Sources/SuperwallKit/Analytics/Attribution/ASIdManagerProxy.swift:21-26` — `final class FakeASIdManager: NSObject` with `@objc static func sharedManager()`, emitting `sharedManager` into `__TEXT,__objc_methname` and `FakeASIdManager` into `__TEXT,__objc_classname`.
- `Sources/SuperwallKit/Analytics/Attribution/ASIdManagerProxy.swift:44` — `classType.sharedManager()`; the `@objc` declaration above is what makes this `AnyObject` lookup typecheck, so the class is load-bearing rather than dead.
- `scripts/scan-privacy-signatures.sh:44-54` — the "deliberately absent" comment covers `LocationPermissionDelegate` and the camera / photos / notification handlers, but says nothing about AdSupport.
## Required outcome
- Either the AdSupport names stop reaching the binary, or the omission is recorded the same way the `LocationPermissionDelegate` one now is, so the scanner's green is not read as "no tracking-adjacent names present".
## Suggested approach (optional)
- Cheapest honest option: extend the `FORBIDDEN` comment to say AdSupport is out of scope and why. `sharedManager` is a generic Cocoa selector shared by many classes, and Apple's real class name `ASIdentifierManager` is already mangled, so the residual signal is genuinely much weaker than `NSUserTrackingUsageDescription` — that reasoning is worth writing down rather than leaving implicit.
- If you'd rather remove it: replace the `classType.sharedManager()` lookup with the same `class_getClassMethod` + `unsafeBitCast` IMP call the four permission proxies use, which needs no `@objc` declaration at all, then delete `FakeASIdManager`.
## Open questions for the human
- Is AdSupport in scope for #495, or is this a separate follow-up? The file's own comment says App Review greps for AdSupport class names, which suggests someone already considered it a live risk.ℹ️ Nitpicks
CHANGELOG.md:19— "Stops Apple's microphone, location, and contacts permission API names appearing in your app's binary" is broader than what shipped:NSMicrophoneUsageDescription,NSContactsUsageDescription, andNSLocationWhenInUseUsageDescriptionare still plaintext literals, deliberately. A developer who reads that line and runsstringswill find them. "class and selector names" would be exact.
Claude Opus | 𝕏
Review follow-ups, both non-blocking. The scanner enumerated its deliberate omissions (the location delegate's runtime-dispatched callbacks; the camera/photos/notification handlers that call Apple directly) but said nothing about AdSupport, so a green scan read as a completeness claim it didn't make. `FakeASIdManager` is a real @objc shim, but unlike the four deleted permission fakes it's load-bearing — it's what makes `classType.sharedManager()` typecheck — and `sharedManager` is a generic selector that fingerprints nothing. The names that would fingerprint, `ASIdentifierManager` and `advertisingIdentifier`, are already mangled by ASIdManagerProxy, so add them to the forbidden list (both verified absent from the current binary) and record why the shim itself stays. Changelog said "permission API names", broader than what shipped — the usage-description keys are still plaintext by design. Say "class and selector names", which is what actually stopped appearing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found. All three items carried over from the prior review are closed.
Reviewed changes — the delta since the prior review at 110667d00, which brings AdSupport into the scanner's scope and tightens the changelog wording.
FORBIDDENextended to AdSupport —ASIdentifierManagerandadvertisingIdentifierjoin the list, and a new paragraph records thatFakeASIdManager's own class name andsharedManagerselector deliberately stay: the class is load-bearing for theAnyObjectlookup atASIdManagerProxy.swift:44, andsharedManageris a generic Cocoa selector that fingerprints nothing. That's the omission the prior review asked to have written down.- Changelog claim narrowed to what shipped —
CHANGELOG.md:19now says "class and selector names" rather than "permission API names", which is exact:NSMicrophoneUsageDescription,NSContactsUsageDescription, and theNSLocation*keys still ship plaintext by deliberate decision.
I checked the two new entries for false positives, since a bad entry here fails develop on merge with nothing to fix. ASIdentifierManager appears in Sources/ only inside a comment on ASIdManagerProxy.swift:19, advertisingIdentifier appears nowhere at all, and both are stored ROT13'd (NFVqragvsvreZnantre, nqiregvfvatVqragvsvre); the backend payload key is idfa, and FakeASIdManager's mangled Objective-C class name contains neither string. The author's local run at 64261cc confirms it empirically — exit 0 at the exact CI artifact path, exit 1 on eight names against a develop build, which settles the scanner's mechanics and gives it a negative control in the same shot. PrivacyInfo.xcprivacy also declares NSPrivacyTracking as false, consistent with the premise of the fix.
Claude Opus | 𝕏
|
Thanks reporting this and for creating this PR @thisislvca! Will be included in the next release 4.16.2. |

Summary
notDeterminedfallbacks so its method signature is not emitted into the framework binaryWhy
App Store Connect can infer that an app uses App Tracking Transparency from dormant identifiers and Objective-C method signatures in a bundled SDK, even when the app never requests tracking permission. This can trigger tracking declarations or review questions for apps that do not use ATT.
The ATT implementation remains available when
ATTrackingManagerexists. If it is unavailable, the proxy continues to returnnotDetermined, matching the previous fallback behavior.Fixes #495
Validation
TrackingManagerProxyTests: 6 tests passed on an iPhone 17 Pro simulatorSuperwallKit.frameworkscanned withstrings; no matches forNSUserTrackingUsageDescription,requestTrackingAuthorizationWithCompletionHandler:, orrequestTrackingAuthorization()git diff --checkpassescc @yusuftor @jakemor @anglinb