Skip to content

Conversation

@fotiDim
Copy link

@fotiDim fotiDim commented Mar 11, 2025

PR Type

Enhancement, Documentation


Description

  • Added IS_IN_REVIEW source detection for iOS/macOS.

  • Integrated package_info_plus and http dependencies for version checks.

  • Updated documentation to reflect new IS_IN_REVIEW feature.

  • Minor updates to macOS project configuration and example files.


Changes walkthrough 📝

Relevant files
Enhancement
GeneratedPluginRegistrant.swift
Register `package_info_plus` plugin for macOS.                     

example/macos/Flutter/GeneratedPluginRegistrant.swift

  • Added package_info_plus plugin registration.
+2/-0     
AppDelegate.swift
Update macOS AppDelegate for secure state support.             

example/macos/Runner/AppDelegate.swift

  • Updated @NSApplicationMain to @main.
  • Added support for secure restorable state.
  • +5/-1     
    main.dart
    Handle `IS_IN_REVIEW` source in main app.                               

    example/lib/main.dart

    • Added IS_IN_REVIEW case handling in source detection.
    +4/-0     
    store_checker.dart
    Add `IS_IN_REVIEW` detection logic and version checks.     

    lib/store_checker.dart

  • Added IS_IN_REVIEW to Source enum.
  • Integrated package_info_plus for app version retrieval.
  • Added fetchAppStoreVersion method for App Store version checks.
  • Implemented logic to detect IS_IN_REVIEW source.
  • +30/-1   
    Documentation
    README.md
    Update README for `IS_IN_REVIEW` feature.                               

    README.md

  • Documented IS_IN_REVIEW source detection feature.
  • Clarified macOS-specific requirements for IS_IN_REVIEW.
  • +7/-1     
    Configuration changes
    Runner.xcscheme
    Enable GPU validation mode in macOS scheme.                           

    example/macos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme

    • Enabled GPU validation mode in macOS scheme.
    +1/-0     
    Dependencies
    pubspec.yaml
    Add new dependencies for version checks.                                 

    pubspec.yaml

    • Added package_info_plus and http dependencies.
    +3/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @fotiDim fotiDim requested a review from rohitsangwan01 March 11, 2025 23:15
    @codiumai-pr-agent-free
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Insecure HTTP request:
    The code in lib/store_checker.dart (line 104) uses an HTTP connection instead of HTTPS when fetching data from the iTunes API. This creates a vulnerability where sensitive data could be intercepted through a man-in-the-middle attack. The URL should be changed to use HTTPS protocol.

    ⚡ Recommended focus areas for review

    Security Risk

    The fetchAppStoreVersion method uses an insecure HTTP connection to query the iTunes API. This should be changed to HTTPS to prevent man-in-the-middle attacks.

    String url = 'http://itunes.apple.com/lookup?bundleId=$bundleId';
    Error Handling

    The version comparison logic assumes that version strings can be directly compared with compareTo, which may not work correctly for semantic versioning (e.g., "1.10.0" would be considered less than "1.2.0").

    } else if (appStoreVersion != null &&
        currentVersion.compareTo(appStoreVersion) > 0) {
      return Source.IS_IN_REVIEW;
    Exception Handling

    The fetchAppStoreVersion method catches all exceptions and only prints them, potentially hiding important errors. Consider propagating errors or implementing more robust error handling.

    } catch (e) {
      print("Error fetching App Store version: $e");
    }

    pubspec.yaml Outdated
    Comment on lines 13 to 14
    package_info_plus: ^8.3.0
    http: ^1.3.0
    Copy link
    Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I had to add 2 dependencies. Not sure if we can avoid it.

    @codiumai-pr-agent-free
    Copy link

    codiumai-pr-agent-free bot commented Mar 11, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Use HTTPS for API requests

    Use HTTPS instead of HTTP for the iTunes API request. Apple's App Store Connect
    API requires secure connections, and using HTTP may fail in production
    environments or when App Transport Security is enforced.

    lib/store_checker.dart [103-106]

     static Future<String?> fetchAppStoreVersion(String bundleId) async {
       try {
    -    String url = 'http://itunes.apple.com/lookup?bundleId=$bundleId';
    +    String url = 'https://itunes.apple.com/lookup?bundleId=$bundleId';
         final response = await http.get(Uri.parse(url));
     
         if (response.statusCode == 200) {
           final data = jsonDecode(response.body);
           if (data['resultCount'] > 0) {
             return data['results'][0]['version'];
           }
         }
       } catch (e) {
         print("Error fetching App Store version: $e");
       }
       return null;
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 9

    __

    Why: Using HTTP instead of HTTPS for API requests is a security vulnerability. Apple's App Store Connect API requires secure connections, and HTTP requests may fail in production environments or when App Transport Security is enforced.

    High
    Possible issue
    Fix semantic version comparison

    The version comparison logic might not handle semantic versioning correctly. The
    string comparison with compareTo may not work as expected for versions like
    "1.10.0" vs "1.2.0". Consider using a semantic version comparison package or
    implementing proper semantic version parsing.

    lib/store_checker.dart [90-92]

     } else if (appStoreVersion != null &&
    -    currentVersion.compareTo(appStoreVersion) > 0) {
    +    _isNewerVersion(currentVersion, appStoreVersion)) {
       return Source.IS_IN_REVIEW;
     
    +// Add this helper method elsewhere in the class
    +// static bool _isNewerVersion(String current, String appStore) {
    +//   // TODO: Implement proper semantic version comparison
    +//   // or use a package like 'version' for this purpose
    +//   return current.compareTo(appStore) > 0;
    +// }
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The current string comparison approach for version numbers can lead to incorrect results with semantic versioning (e.g., "1.10.0" would be considered less than "1.2.0"). This could cause incorrect detection of review status, which is a significant functional issue.

    Medium
    General
    Use proper logging mechanism

    Replace direct print statements with a proper logging mechanism. According to
    Effective Dart guidelines, production code should avoid using print for logging
    errors as it doesn't provide control over log levels and can't be easily
    disabled in production.

    lib/store_checker.dart [113-115]

     } catch (e) {
    +  // TODO: Replace with proper logging
    +  // Consider using a logging package or custom logger
    +  // that can be configured for different environments
       print("Error fetching App Store version: $e");
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    __

    Why: While the suggestion correctly identifies that using print statements for error logging isn't ideal, it only adds a TODO comment rather than providing a concrete implementation. The impact is relatively minor in this context.

    Low
    • Update

    @fotiDim fotiDim force-pushed the Add-in-review-check branch from 777fb9c to 47a04e8 Compare March 11, 2025 23:22
    } else if (sourceName.compareTo('AppStore') == 0) {
    // Installed ipa from App Store
    return Source.IS_INSTALLED_FROM_APP_STORE;
    } else if (appStoreVersion != null &&
    Copy link
    Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @rohitsangwan01 what do you think about the condition here?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants