refactor: simplify CheckoutWidget implementation

[vscmaster]

No description provided.

[vscmaster]

@claude review

[claude]

Claude finished @vscmaster's task in 1m 23s —— View job

---

PR Review: refactor: simplify CheckoutWidget implementation

Refactoring Summary

This PR consolidates platform channel handling and payment callbacks from the Sample app layer into the SDK's OttuCheckoutWidget. Key changes:

• Moved method channel handler from checkout_screen.dart to checkout_widget.dart
• Removed duplicate channel com.ottu.sample/checkout/payment/verify and unified all communication on com.ottu.sample/checkout
• Added callback parameters to OttuCheckoutWidget for success/cancel/error
• Moved dynamic height management into the widget

---

Security Analysis (per CLAUDE.md review framework)

✅ Positive Changes

1. Channel consolidation reduces attack surface: Removing the separate com.ottu.sample/checkout/payment/verify channel is good - fewer channels means fewer potential interception points.

2. Callback architecture is correct: Payment result callbacks (success/error/cancel) remain as opaque strings from native SDK. The Flutter layer doesn't parse or handle payment data structures.

3. No CHD exposure: This refactoring doesn't introduce any cardholder data handling in Flutter - the wrapper remains a "view-only" layer.

---

⚠️ Security Issues Found

1. Print statement leaks runtime type information (Low severity)

Location: lib/src/checkout_widget.dart:86

print("CheckoutWidget, verifyPayment, result: ${result.runtimeType}");

Issue: This log was added in the refactoring and outputs to console in production builds. While not CHD, it establishes an unsafe logging pattern.

Recommendation: Remove this print statement or guard it with kDebugMode:

if (kDebugMode) {
  print("CheckoutWidget, verifyPayment, result: ${result.runtimeType}");
}

Fix this →

---

2. Missing error sanitization on channel boundary (Medium severity)

Location: lib/src/checkout_widget.dart:91

case _methodVerifyPayment:
  {
    final result = await widget._verifyPayment?.call(call.arguments);
    // ...
    case Failure(:final message):
      throw PlatformException(code: _methodVerifyPayment, message: message);
    case null:
      throw PlatformException(code: _methodVerifyPayment, message: "Unknown error");
  }

Issue: When _verifyPayment callback returns a Failure, the error message is passed directly from merchant app code into a PlatformException that crosses the platform channel boundary back to native. If the merchant app's error message accidentally contains sensitive context (e.g., "Payment failed for card ending 1234"), this would leak across the channel and potentially into native logs.

Specific Risk:

• Android: CheckoutView.kt:237-238 logs the error: Log.e(TAG, "verifyPayment, error") with the message
• The message could end up in logcat in production

Recommendation: Sanitize or constrain error messages before throwing PlatformException:

case Failure(:final message):
  // Sanitize: only allow simple failure message without details
  final sanitized = message.isEmpty ? "Verification failed" : "Verification failed";
  throw PlatformException(code: _methodVerifyPayment, message: sanitized);

Or document that merchant apps MUST NOT include sensitive data in verification failure messages.

Fix this →

---

3. Channel handler overwrites on each didChangeDependencies (Low-Medium severity)

Location: lib/src/checkout_widget.dart:58

@override
void didChangeDependencies() {
  super.didChangeDependencies();
  _methodChannel.setMethodCallHandler((call) async {
    // ... handler code
  });
}

Issue: didChangeDependencies() can be called multiple times during a widget's lifecycle (on theme changes, locale changes, parent rebuilds, etc.). Each call sets a new method call handler on the singleton MethodChannel instance, potentially:

1. Creating memory leaks (old handlers not GC'd)
2. Allowing handler replacement by other widget instances if multiple OttuCheckoutWidget instances exist
3. Race conditions if messages arrive during handler replacement

Recommendation: Move handler registration to initState() (called once) and deregister in dispose():

@override
void initState() {
  super.initState();
  _methodChannel.setMethodCallHandler(_handleMethodCall);
}

@override
void dispose() {
  _methodChannel.setMethodCallHandler(null);
  print("OttuCheckoutWidget.dispose()");
  _methodChannel.invokeMethod(_methodOnWidgetDetached);
  super.dispose();
}

Future<void> _handleMethodCall(MethodCall call) async {
  switch (call.method) {
    // ... existing handler code
  }
}

Fix this →

---

📋 Architectural Concerns (Not introduced by this PR, but relevant)

These issues exist in the codebase and are highlighted by CLAUDE.md - not caused by this refactoring, but worth noting:

1. API key crosses channel in plaintext (checkout_widget.dart:103 → creationParams) - pre-existing issue
2. Native callbacks use .toString() on SDK objects which could leak sensitive fields if SDK changes - pre-existing issue in Android/iOS
3. No channel message schema validation - dynamic maps accepted without type/key validation - pre-existing architectural issue
4. Sample app teaches insecure patterns (unconditional logging, cleartext traffic) - pre-existing, outside PR scope

---

📊 Summary

Architecture: The refactoring is sound - consolidating channel handling into the widget reduces duplication and simplifies the SDK's public API.

Security Impact:

• ✅ No new CHD exposure risks
• ⚠️ Adds one print statement (minor)
• ⚠️ Error message sanitization gap (medium)
• ⚠️ Handler lifecycle bug (low-medium)

Recommendation: Approve with requested changes - fix the three issues above before merging. The refactoring itself is good, but the implementation has a few bugs that should be addressed.

---

[vscmaster]

@claude review

[claude]

Claude finished @vscmaster's task in 1m 42s —— View job

---

PR Review: refactor: simplify CheckoutWidget implementation

Summary

This PR consolidates platform channel handling by moving payment callbacks and dynamic height management from the Sample app layer into the SDK's OttuCheckoutWidget. The refactoring reduces code duplication and simplifies the SDK's public API by eliminating a redundant channel.

Key Changes:

• Removed duplicate channel com.ottu.sample/checkout/payment/verify
• Moved method channel handler from checkout_screen.dart to checkout_widget.dart
• Added callback parameters (successCallback, cancelCallback, errorCallback) to OttuCheckoutWidget
• Integrated dynamic height management directly into the widget

---

Security Analysis (per CLAUDE.md)

✅ Positive Security Changes

1. Reduced attack surface: Consolidating from two channels to one (com.ottu.sample/checkout) reduces potential interception points.

2. No CHD exposure introduced: This refactoring maintains the "wrapper-only" pattern - the Flutter layer still does not handle any cardholder data. Payment results remain as opaque strings from the native SDK.

3. Callback architecture is sound: Payment callbacks (success/error/cancel) correctly pass opaque strings without parsing payment structures.

---

🔴 Critical Issues Found

1. Handler Registration Lifecycle Bug (Medium Severity)

Location: lib/src/checkout_widget.dart:52

Issue: The method call handler is registered in initState(), which is correct. However, there's a critical bug in Flutter's MethodChannel behavior: MethodChannel instances are singletons per channel name. When multiple OttuCheckoutWidget instances exist (or widget rebuilds occur), the LAST widget to call setMethodCallHandler() wins, and previous handlers are silently overwritten.

@override
void initState() {
  super.initState();
  _methodChannel.setMethodCallHandler(_handleChannelMethod);  // Line 52
}

Current Code State (GOOD):

@override
void dispose() {
  if (kDebugMode) {
    print("OttuCheckoutWidget.dispose()");
  }
  _methodChannel.setMethodCallHandler(null);  // Line 60 - correctly clears handler
  _methodChannel.invokeMethod(_methodOnWidgetDetached);
  super.dispose();
}

Risks:

• If a merchant app creates multiple OttuCheckoutWidget instances, only the last one receives callbacks
• Race conditions if native sends messages during widget disposal/recreation
• Memory leaks from uncollected handler closures in some scenarios

Recommendation: Document that only ONE OttuCheckoutWidget should exist at a time, or refactor to use a global static handler that routes to the active widget instance.

Fix this →

---

2. Missing Error Message Sanitization (Medium Severity)

Location: lib/src/checkout_widget.dart:99

Issue: When the merchant app's verifyPayment callback returns a Failure, the error message is passed directly into a PlatformException that crosses back to native code. If the merchant app's error message accidentally contains sensitive context, it will leak across the channel and into native logs.

case Failure(:final message):
  throw PlatformException(code: _methodVerifyPayment, message: message);  // Line 99

Attack Vector:

• Merchant app returns Failure("Payment verification failed for card ending 1234")
• Message crosses channel boundary
• Android logs it: CheckoutView.kt:237 → Log.e(TAG, "verifyPayment, error") with the full message
• CHD-adjacent data appears in logcat in production builds

Recommendation: Sanitize error messages before crossing the platform boundary:

case Failure(:final message):
  // Only allow generic failure message
  const sanitized = "Verification failed";
  throw PlatformException(code: _methodVerifyPayment, message: sanitized);

Or alternatively, document in LOUD warnings that merchant apps MUST NOT include sensitive data in verification failure messages.

Fix this →

---

3. Print Statement in Production (Low Severity - FIXED in this PR)

Location: lib/src/checkout_widget.dart:93

Status: ✅ FIXED - The PR correctly guards the debug print with kDebugMode:

if (kDebugMode) {
  print("CheckoutWidget, verifyPayment, result: ${result.runtimeType}");
}

Previous Review Finding: The earlier Claude review noted this as an issue, but the current code shows it's properly guarded and will not appear in release builds. Good work!

Similarly, the dispose print at line 58 is also correctly guarded:

if (kDebugMode) {
  print("OttuCheckoutWidget.dispose()");
}

---

⚠️ Pre-Existing Issues (Not Introduced by This PR)

These issues exist in the codebase and are highlighted by CLAUDE.md review rules, but were NOT caused by this refactoring:

1. API key crosses channel in plaintext (checkout_widget.dart:111 in build() method) - The apiKey is included in CheckoutArguments.toJson() and sent via creationParams. This is a pre-existing design decision.

2. Native callbacks use .toString() on SDK objects - Android (CheckoutView.kt:198,202,208) and iOS (CheckoutPlatformView.swift:482,506,524) both use .description on payment result dictionaries. If the native SDK includes sensitive fields in these descriptions, they would leak. This is pre-existing.

3. No channel message schema validation - The platform channel accepts dynamic Map<String, dynamic> without strict validation of keys/types. Pre-existing architectural issue.

4. Sample app teaches insecure patterns - The Sample app still has insecure logging (logger.d at checkout_screen.dart:96 logs the verification payload), but this is outside the SDK layer and pre-existing.

5. Channel name contains "sample" - The channel is named com.ottu.sample/checkout instead of com.ottu/checkout. Inherited from demo code, pre-existing naming issue.

---

📊 Refactoring Quality Assessment

Architecture (✅ Good)

• Separation of Concerns: Moving channel handling into the widget is the right architectural choice. The SDK should own its communication layer, not the consumer app.
• API Simplification: The new callback parameters (successCallback, cancelCallback, errorCallback) make the widget's API more explicit and easier to use correctly.
• Height Management: Integrating the ValueListenableBuilder into the widget is good - the widget knows its own sizing needs.

Code Quality (✅ Good)

• Proper use of kDebugMode guards for debug prints
• Clean callback pattern with nullable function parameters
• Correct lifecycle management (handler registered in initState, cleared in dispose)

Testing (❌ Missing - Pre-existing)

• Zero tests - This refactoring moved critical channel handling logic but has no tests to verify:
  • Handler registration/deregistration works correctly
  • Callbacks are invoked with correct arguments
  • Error paths are handled properly
  • Multiple widget instances don't interfere (if supported)

This is a pre-existing issue (no test/ directory exists), but this refactoring would have been an ideal time to add tests.

---

🎯 Recommendation

APPROVE WITH REQUESTED CHANGES

The refactoring is architecturally sound and improves the SDK's design. However, two medium-severity issues should be fixed before merging:

1. Critical: Address the error message sanitization issue (Issue Failed to clone repository git@github.com:ottuco/ottu-flutter-ios.git #2 above)
2. Important: Document the single-instance limitation or implement static handler routing (Issue Added ApplePay merchant IDs #1 above)

The positive changes (channel consolidation, API simplification, debug print guards) outweigh the issues, which are fixable without changing the refactoring approach.

---

📝 Suggested Follow-up Work (Future PRs)

1. Add unit tests for the refactored OttuCheckoutWidget channel handling
2. Implement schema validation for channel messages
3. Rename channel from com.ottu.sample/checkout to com.ottu/checkout
4. Add integration tests to verify callbacks don't carry sensitive fields
5. Create merchant app documentation warning against including sensitive data in error messages

---