Skip to content

Conversation

@AnirudhBhat
Copy link
Contributor

@AnirudhBhat AnirudhBhat commented Jan 9, 2025

Closes: #12760

Note to reviewers: Please see if the changes are worth the fix

Description

This PR addresses the following improvements for the receipt preview feature:

  1. Enhanced Receipt Content Visibility

Configured the WebView to scale content appropriately using viewport settings and JavaScript injection to ensure the receipt content fits the screen on all devices.

As a result of using Javascript, below security measures have been taken care of:

  1. Mitigation of Security Vulnerabilities in WebView

Strengthened WebView security by adding a domain name validation mechanism to ensure only trusted URLs are loaded.

  1. Additional Security Measures

Restricted unnecessary WebView features, such as file and content access, to reduce the attack surface.

Changes Made

  1. WebView Content Scaling

Injected a viewport meta tag and zoom scaling via JavaScript in onPageFinished.

  1. Updated WebView settings:
    Enabled useWideViewPort and loadWithOverviewMode for responsive rendering.

Disabled unnecessary features like allowFileAccess and allowContentAccess.

  1. Domain Validation

Added validation in shouldOverrideUrlLoading to block untrusted domains and handle them appropriately.
Implemented a fallback for older APIs to ensure consistent behavior across devices.

Security Vulnerability Concern

The primary concern was the potential Cross-Site Scripting (XSS) and phishing attacks due to enabling JavaScript in the WebView. While JavaScript is essential for scaling and rendering rich content like receipts, it also opens up the possibility for:

  1. XSS Attacks:

If an untrusted URL or malicious script is loaded into the WebView, it could execute arbitrary JavaScript, potentially compromising sensitive user data or app security.

  1. Phishing Attacks:

If the WebView loads a malicious URL that mimics a trusted domain, it could deceive users into sharing sensitive information.

  1. File or Content Access Exploits:

By default, WebView settings like allowFileAccess and allowContentAccess could expose local files or content providers to unauthorized access if not properly secured.

Why Domain Validation Was Added
To mitigate these risks, domain validation was introduced as a safeguard to ensure the WebView only loads content from trusted sources. This addresses the vulnerabilities by:

  1. Blocking Untrusted URLs:

Any URL that doesn't belong to the trusted domain(s) is rejected in the shouldOverrideUrlLoading method, ensuring only authorized content is rendered.

  1. Preventing Malicious Content Execution:

By restricting URL loading to a specific domain (e.g., https://your-trusted-domain.com), the app avoids loading scripts or assets from external or potentially harmful sources.

Steps to reproduce

  1. Navigate to any order with status "completed" (Open app -> Orders tab)
  2. In the order detail screen, click on "see receipt" button
  3. Ensure the receipt contents are displayed in enlarged font that fits to the screen

The tests that have been performed

Above mentioned steps

Images/gif

Before and After Comparison

Before After
receipt_before receipts_after
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@AnirudhBhat AnirudhBhat added type: enhancement A request for an enhancement. feature: order details Related to order details. labels Jan 9, 2025
@AnirudhBhat AnirudhBhat added this to the 21.5 milestone Jan 9, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 9, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit1167603
Direct Downloadwoocommerce-wear-prototype-build-pr13266-1167603.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 9, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit1167603
Direct Downloadwoocommerce-prototype-build-pr13266-1167603.apk

@kidinov kidinov self-assigned this Jan 9, 2025
@kidinov
Copy link
Contributor

kidinov commented Jan 9, 2025

@AnirudhBhat Thanks for your work here!

It works fine, but I am wondering if we could just inject HTML in the page and avoid all these changes related to the JS enabling? E.g.

    private fun initViews(binding: FragmentReceiptPreviewBinding, savedInstanceState: Bundle?) {
        if (savedInstanceState != null) {
            binding.receiptPreviewPreviewWebview.restoreState(savedInstanceState)
        } else {
            with(binding.receiptPreviewPreviewWebview) {
                webViewClient = object : WebViewClient() {
                    override fun onPageFinished(view: WebView, url: String) {
                        viewModel.onReceiptLoaded()
                    }

                    override fun shouldInterceptRequest(
                        view: WebView,
                        request: WebResourceRequest
                    ): WebResourceResponse? {
                        val url = request.url.toString()

                        val connection = URL(url).openConnection()
                        val inputStream = connection.getInputStream()

                        val htmlContent = inputStream.bufferedReader().use { it.readText() }

                        val modifiedHtmlContent = htmlContent.replace(
                            "<head>",
                           "<head><meta name=\"viewport\" content=\"width=device-width, initial-scale=1.0\">"
                        )

                        val newInputStream = modifiedHtmlContent.byteInputStream()
                        return WebResourceResponse(
                            "text/html",
                            "UTF-8",
                            newInputStream
                        )

                        return super.shouldInterceptRequest(view, request)
                    }
                }
            }
        }
    }

And maybe to extract some code from shouldInterceptRequest to a helper to be able to have tests, but not sure if it worth it.

image

wdyt?

@AnirudhBhat
Copy link
Contributor Author

@kidinov Thanks for the good suggestion! I've implemented the solution that modifies html content instead of injecting javascript. Please take a look.

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.

Project coverage is 41.08%. Comparing base (d167642) to head (1167603).
Report is 16 commits behind head on trunk.

Files with missing lines Patch % Lines
...ayments/receipt/preview/ReceiptPreviewViewModel.kt 45.45% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #13266   +/-   ##
=========================================
  Coverage     41.07%   41.08%           
- Complexity     6417     6422    +5     
=========================================
  Files          1321     1322    +1     
  Lines         77221    77238   +17     
  Branches      10657    10659    +2     
=========================================
+ Hits          31722    31733   +11     
- Misses        42687    42690    +3     
- Partials       2812     2815    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

"UTF-8",
modifiedHtml.byteInputStream()
)
} catch (e: MalformedURLException) {
Copy link
Contributor

@kidinov kidinov Jan 10, 2025

Choose a reason for hiding this comment

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

I'd propose to crash in both cases, as in both cases, it means the feature doesn't function, and the only difference is that in the case of crash, we will catch it early, while in the second later or even never

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Done: 38f7f65

Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

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

LGTM! I left, imo important comment regarding error handling, please take a look

@AnirudhBhat AnirudhBhat enabled auto-merge January 17, 2025 04:04
@AnirudhBhat AnirudhBhat merged commit df4ac5a into trunk Jan 17, 2025
15 checks passed
@AnirudhBhat AnirudhBhat deleted the issue/12760-enlarge-receipt-contents branch January 17, 2025 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: order details Related to order details. type: enhancement A request for an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Receipt size(HTML preview)

5 participants