KM-16629: storekit2 - #355
Conversation
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
KB review — 7 findings on this PR🟠 1 HIGH · 🟡 1 MEDIUM · ⚪ 4 LOW · ℹ️ 1 INFO
kb · head |
df445f5 to
5b9ebfc
Compare
![kpkb-1f8e9813897fe9831983e89f7143[bot]](https://avatars.githubusercontent.com/u/75775826?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
kp-diego-trevisan
left a comment
kp-diego-trevisan
left a comment
There was a problem hiding this comment.
Code-wise it looks fine to me.
Added a couple suggestions, maybe it's worth checking the latest Claude concerns, but I see the most relevant ones were already tackled 👍
This comment was marked as outdated.
This comment was marked as outdated.
ca055d5 to
3492991
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Claude finished @kp-mario-nachbaur's task in 1m 16s —— View job PR Re-review: StoreKit 2 Migration — branch sync (
|
| File | Source |
|---|---|
PIAKPI/Internal/KPIEventUtils.swift |
master (KM-17456) |
PIALibrary/Util/Keychain.swift |
master (KM-17456) |
No new StoreKit-related code has been added since my last pass, so there is nothing new to review here — the merge is clean and the diff is limited to unrelated master fixes. ✅
Still open (carry over — unchanged)
All four items from my previous review remain in the same state:
-
📝 Comment fix half done —
AppStoreProvider.swift:77-78still reads "With our current architecture is difficult to handle them." — missing subject, should be "…architecture it is difficult…". Purely cosmetic. Fix this → -
⚠️ Dead pending-recovery gating — unchanged.DashboardViewController.swift:436andRootCoordinator.swift:113still hardcodepreset.shouldRecoverPendingSignup = false(with the commented-out//!TransientState.didRetryPendingSignup), while theif !TransientState.didRetryPendingSignup { … }blocks below still write the flag. Because the value is alwaysfalse,tryRecoverSignupProcess()stays unreachable. Either wire it to!TransientState.didRetryPendingSignupor delete the dead recovery code. Fix this → -
🔒 JWS debug export (LOW — KB
SEC-L-4and related) — unchanged.DebugMenuView.swift:188still exports rawtransactionJWS.valuevia a per-sectionShareLink, andDebugMenuView+Values.swiftstill appends the full JWS to the "Export All" bundle. Neither path is gated. Since the debug menu is shake-reachable in TestFlight, consider gating both behind#if DEVELOPMENT || STAGING. Acceptable if intentional. -
🔐 Unverified transaction → backend (MEDIUM — KB
SEC-M-3) — the author confirmed the backend validates the full JWS cert chain to Apple's Root CA and verifies the signature, so forwarding.unverifiedis safe by design. Resolved by discussion.AppStoreProvider.swift:172-173still has no explanatory code comment — worth a one-liner documenting why.unverifiedis intentionally forwarded so the next reader doesn't re-flag it. -
🧪 Test coverage — unchanged. Still no new unit tests for the new provider branches (
currentEntitlementJWSnewest/verified selection, restore-with-cached-entitlement,purchasePending/userCancelledmapping,badReceiptfallback, retry-with-sync), andSignupProviderTestsremains removed.
Summary
This sync is a clean merge from master with no new StoreKit 2 changes — nothing new to flag. The carry-over items are all still minor/optional except the dead pending-recovery gating, which continues to read as unfinished WIP and is the one item worth resolving before merge. PR remains in good shape and is already approved by @kp-diego-trevisan.
· branch KM-16629-storekit2
| func productsRequest(_ request: SKProductsRequest, didReceive response: SKProductsResponse) { | ||
| guard (request == productsRequest) else { | ||
| return | ||
| log.debug("Purchasing product with identifier: \(product.id)") |
There was a problem hiding this comment.
🟡 MEDIUM · SECURITY_REVIEW — StoreKit 2 unverified purchase forwarded to backend without client-side rejection
In AppStoreProvider.purchase(), the VerificationResult.unverified case — which StoreKit 2 returns when the cryptographic signature on the transaction JWS cannot be verified on-device — is treated identically to the VerificationResult.verified case: the function returns .success(AppStoreTransaction(...)) and the caller proceeds to authenticate with the PIA backend using the unverified JWS: ```swift case .unverified(let transaction, let error): log.debug("(#function) success unverifi…
kb · SEC-M-3
|
|
||
| var receiptBase64: String? { | ||
| Client.store.paymentReceipt?.base64EncodedString() | ||
| var transactionJWS: JWS? { |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — StoreKit 2 JWS receipt exported in plaintext via debug menu share sheet
The new DebugMenuView and its DebugMenuView+Values extension expose the device's current active subscription entitlement as a raw JWS string through two iOS share sheet entry points: 1. Dedicated Transaction section (receiptSection): renders a 300-character preview and offers a ShareLink that exports the full JWS value to any iOS share target (AirDrop, Messages, Files, email, etc.). 2. "Export All" toolbar button (buildExportContent()): appends the full transactionJWS?.value…
kb · F-2XB2EX6VW9J6SR88
| @State private var refundTransactionId: UInt64 = 0 | ||
| @State private var isRefundSheetPresented = false | ||
| @State private var availableTransactions: [StoreKit.Transaction] = [] | ||
| @State private var isTransactionPickerPresented = false |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — Debug menu exports full StoreKit 2 JWS credential accessible in TestFlight builds
The PIADebugMenu package's DebugMenuView fetches the active entitlement JWS on view appear and renders it with a share-sheet export button: swift .task { entitlementJWS = await Client.store.currentEntitlementJWS() } The "Export" share link in the receiptSection writes the full JWS string to a DebugExportFile (transaction_<timestamp>.txt), and the same value is appended to the "Export All" log bundle under === Transaction (JWS) ===. The debug menu is gated by: ```swift…
kb · F-A8GCEDDANS1C70XW
| preview: SharePreview("Receipt") | ||
| preview: SharePreview("Transaction JWS") | ||
| ) { | ||
| Label("Export", systemImage: "square.and.arrow.up") |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — Debug menu exports raw StoreKit 2 JWS receipt via iOS share sheet
DebugMenuView (introduced in this PR as part of the StoreKit 2 migration) displays the current entitlement's JWS representation and exposes it through two iOS share sheet entry points: 1. "Transaction JWS" section – a dedicated ShareLink that exports the raw transactionJWS.value string to a timestamped .txt file: ```swift ShareLink( item: DebugExportFile( content: transactionJWS.value, filename: "transaction_(Int(Date().timeIntervalSince1970)).txt…
kb · F-XGFXR4MEDSCV754S
|
|
||
| var receiptBase64: String? { | ||
| Client.store.paymentReceipt?.base64EncodedString() | ||
| var transactionJWS: JWS? { |
There was a problem hiding this comment.
⚪ LOW · SECURITY_REVIEW — StoreKit 2 JWS receipt exposed in TestFlight debug export
The debug menu's buildExportContent() now includes the full StoreKit 2 JWS value in its "Export All" share sheet output (section "=== Transaction (JWS) ==="). A dedicated per-section "Export" ShareLink in receiptSection also exports the raw JWS as a .txt file. The JWS is loaded in DebugMenuView.onAppear via Client.store.currentEntitlementJWS() — the identical value passed to loginWithReceipt(receipt:) for authentication. The debug menu is activated by shake gesture in TestFlight b…
kb · SEC-L-4
2 participants
JWS(just a wrapper over string).PIABasepackage for base types.