-
Notifications
You must be signed in to change notification settings - Fork 306
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
Improve sandbox detector for macOS apps #3549
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks for doing this!
/// - https://github.com/objective-see/ProcInfo/blob/master/procInfo/Signing.m#L184-L247 | ||
/// - https://gist.github.com/lukaskubanek/cbfcab29c0c93e0e9e0a16ab09586996#gistcomment-3993808 | ||
#if os(macOS) || targetEnvironment(macCatalyst) | ||
var isMacAppStore: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And should be private
. Maybe move it to an extension?
var code: SecStaticCode? | ||
status = SecStaticCodeCreateWithPath(Bundle.main.bundleURL as CFURL, [], &code) | ||
|
||
guard status == noErr, let code = code else { return false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log the error?
/// it checked the common name but was changed to an extension check to make it more | ||
/// future-proof. | ||
/// | ||
/// For more information, see the following references: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏🏻
&requirement | ||
) | ||
|
||
guard status == noErr, let requirement = requirement else { return false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's log a warning here too?
return path.contains("Xcode/DerivedData") | ||
} else { | ||
#if os(macOS) || targetEnvironment(macCatalyst) | ||
return !isMacAppStore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return !isMacAppStore | |
return !Self.isMacAppStore |
case xcode = "Xcode" | ||
|
||
/// Unknown environment | ||
case unknown = "Unknown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, there might also be ProductionVPP
and ProductionVPPSandbox
for Apple's Volume Purchase Program, but I believe those apps cannot have in-app purchases anyway.
} | ||
|
||
/// The receipt's environment. | ||
public let environment: Environment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it optional since the value is not publicly documented by Apple as a receipt field and could be changed. It's also not required for us since we can fall back to the bundle signature method.
throw Error.failedToLoadLocalReceipt(error) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved these to the main package while keeping them private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were here as convenience for this package because it doesn't have a receipt fetcher.
return self.sandboxEnvironmentDetector.isSandbox | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caching the result since the sandbox detector is a bit more CPU-intensive, and does not change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried this was suddenly a lot less efficient, but:
- It's macOS only
- It's cached 👏🏻
@@ -40,12 +44,19 @@ final class BundleSandboxEnvironmentDetector: SandboxEnvironmentDetector { | |||
return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue if appStoreReceiptUR
is nil then we are in sandbox environment, so this should be true?
I have not been able to find a case where appStoreReceiptURL
is nil tho. The URL is always returned, but then the file does not exist in some cases.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3549 +/- ##
==========================================
- Coverage 86.04% 85.97% -0.07%
==========================================
Files 241 242 +1
Lines 17586 17616 +30
==========================================
+ Hits 15131 15145 +14
- Misses 2455 2471 +16 ☔ View full report in Codecov by Sentry. |
@@ -18,6 +18,24 @@ import Foundation | |||
/// - Seealso: [the official documentation](https://rev.cat/apple-receipt-fields). | |||
public struct AppleReceipt: Equatable { | |||
|
|||
/// The server environment a receipt belongs to. | |||
public enum Environment: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this to an extension like we do in other types?
That makes it easier to see the type properties at a glance without the extra noise of other type definitions.
@@ -39,6 +39,7 @@ enum ReceiptStrings { | |||
case receipt_retrying_mechanism_not_available | |||
case local_receipt_missing_purchase(AppleReceipt, forProductIdentifier: String) | |||
case retrying_receipt_fetch_after(sleepDuration: TimeInterval) | |||
case validating_bundle_signature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case validating_bundle_signature | |
case error_validating_bundle_signature |
|
||
internal extension PurchasesReceiptParser { | ||
|
||
func fetchAndParseLocalReceipt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add this here, we already have ReceiptFetcher
for this
throw Error.failedToLoadLocalReceipt(error) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were here as convenience for this package because it doesn't have a receipt fetcher.
init(bundle: Bundle = .main, isRunningInSimulator: Bool = SystemInfo.isRunningInSimulator) { | ||
init(bundle: Bundle = .main, | ||
isRunningInSimulator: Bool = SystemInfo.isRunningInSimulator, | ||
receiptParser: PurchasesReceiptParser = PurchasesReceiptParser.default) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of injecting a protocol type so we don't tie this to receipt parser or fetcher (and make it easier to test)?
#endif | ||
} | ||
|
||
var isProductionReceipt: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move it to the extension below?
return path.contains("Xcode/DerivedData") | ||
} else { | ||
#if os(macOS) || targetEnvironment(macCatalyst) | ||
return !isProductionReceipt || !Self.isMacAppStore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return !isProductionReceipt || !Self.isMacAppStore | |
return !self.isProductionReceipt || !Self.isMacAppStore |
return self.sandboxEnvironmentDetector.isSandbox | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried this was suddenly a lot less efficient, but:
- It's macOS only
- It's cached 👏🏻
if isMASReceipt { | ||
return path.contains("Xcode/DerivedData") | ||
} else { | ||
#if os(macOS) || targetEnvironment(macCatalyst) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to do it on a separate PR (maybe pointing to the macOS tests branch), but this could really use tests. I'm afraid that if we don't add them now we'll forget.
5379cc6
to
897e432
Compare
func fetchAndParseLocalReceipt() throws -> AppleReceipt | ||
} | ||
|
||
internal final class LocalReceiptFetcher: LocalReceiptFetcherType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReceiptFetcher
depends on StoreKitRequestFetcher
and SystemInfo
(which would create a circular dependency BundleSandboxEnvironmentDetector).
So instead of adding these methods as an extension to PurchasesReceiptParser
, I've created LocalReceiptFetcher
as a simplified version of ReceiptFetcher
, with an accompanying protocol so it can be tested.
Ideally we would be able to use ReceiptFetcher
but the dependencies make it hard :/ any other suggestion to simplify this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's why we had this. In the future we'll be able to share code using the package
visibility.
} | ||
|
||
func testIsSandboxIfReceiptParsingFailsAndBundleSignatureIsNotMacAppStore() throws { | ||
try AvailabilityChecks.skipIfNotMacOS() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some tests for macOS, which do not currently run so I cannot properly test them. Once the macOS tests PR is ready I'll make sure to fix any errors if tests don't pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 👍🏻 The only thing left is the unknown / nil. Do you still think we need optional?
|
||
import Foundation | ||
|
||
public extension PurchasesReceiptParser { | ||
internal protocol LocalReceiptFetcherType: Sendable { | ||
func fetchAndParseLocalReceipt() throws -> AppleReceipt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style guide: spacing around this
receiptParser: .default | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Symmetry with opening (
)
receiptParser: .default | |
) | |
receiptParser: .default) |
|
||
private extension BundleSandboxEnvironmentDetector { | ||
|
||
var isProductionReceipt: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this inside of the #if os(macOS)
check too? It's potentially very inefficient for mobile devices, so that way we make sure we don't use it there.
@@ -84,6 +84,7 @@ private struct ReceiptDataView: View { | |||
init(receipt: AppleReceipt) { | |||
self.init( | |||
values: [ | |||
.init("Environment", (receipt.environment ?? .unknown).rawValue), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case in point :P we don't need optional + unknown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it non-optional, with .unknown
as default, and overridden by whatever comes in the receipt :D
@@ -108,8 +109,21 @@ class AppleReceiptBuilderTests: TestCase { | |||
} | |||
} | |||
|
|||
func testBuildDoesntThrowIfEnvironmentIsMissing() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
@@ -16,46 +16,96 @@ import XCTest | |||
|
|||
@testable import RevenueCat | |||
|
|||
final class MockLocalReceiptFetcher: LocalReceiptFetcherType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be private
. Can you move it to the bottom of the file, since it's just an implementation detail? That way the first thing in the file is the actual tests.
throw PurchasesReceiptParser.Error.receiptParsingError | ||
} | ||
return mockReceipt | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
} | |
if failReceiptParsing { | ||
throw PurchasesReceiptParser.Error.receiptParsingError | ||
} | ||
return mockReceipt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return mockReceipt | |
return self.mockReceipt |
func fetchAndParseLocalReceipt() throws -> AppleReceipt | ||
} | ||
|
||
internal final class LocalReceiptFetcher: LocalReceiptFetcherType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's why we had this. In the future we'll be able to share code using the package
visibility.
7b473d1
to
7f027a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Co-authored-by: NachoSoto <NachoSoto@users.noreply.github.com>
**This is an automatic release.** ### RevenueCatUI * `Paywalls`: remove unscrollable spacing in Template 5 (#3562) via NachoSoto (@NachoSoto) * `Paywalls`: improve template 5 checkmark icon (#3559) via NachoSoto (@NachoSoto) ### Bugfixes * Improve sandbox detector for macOS apps (#3549) via Mark Villacampa (@MarkVillacampa) ### Other Changes * `Paywalls`: new `PaywallViewControllerDelegate.paywallViewController(_:didChangeSizeTo:)` (#3563) via Cesar de la Vega (@vegaro) * `Tests`: running tests on `macOS` (#3533) via NachoSoto (@NachoSoto) * `Integration Tests`: split into separate jobs (#3560) via NachoSoto (@NachoSoto)
Checking for the presence of
Xcode/DerivedData
in the receipt path was failing for TestFlight builds or if the .app bundle generated by Xcode was moved from the folder.Additionally, checking for
MASReceipt/receipt
seems less robust than simply using compiler directives.