Skip to content

fix: Trust server in the iOS webview to allow to load the survey package #5024

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.

Sign up for GitHub

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

Jump to bottom
Merged
mattinannt merged 1 commit into from
Mar 23, 2025
Merged

fix: Trust server in the iOS webview to allow to load the survey package #5024

mattinannt merged 1 commit into from
Mar 23, 2025

Conversation

@vaxi87
Copy link
Contributor

@vaxi87 vaxi87 commented Mar 20, 2025 •
edited by coderabbitai bot
Loading

What does this PR do?

Trust server in the iOS webview to allow to load the survey package

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read How we Code at Formbricks
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues
  • First PR at Formbricks? Please sign the CLA! Without it we wont be able to merge it 🙏

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

Summary by CodeRabbit

  • New Features
    • Enhanced the survey view to improve secure connection handling during web navigation, ensuring smoother interactions when users access survey content.

@vercel
Copy link

vercel bot commented Mar 20, 2025

@vaxi87 is attempting to deploy a commit to the formbricks Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 20, 2025 •
edited
Loading

Thank you for following the naming conventions for pull request titles! 🙏

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 20, 2025

Walkthrough

The changes extend the functionality of the SurveyWebView by enhancing its Coordinator class. The Coordinator now conforms to the WKNavigationDelegate protocol in addition to WKUIDelegate. A new method, webView(_:didReceive:completionHandler:), is introduced to handle URL authentication challenges by checking the protection space for server trust and responding with the appropriate credential or nil. Additionally, the makeUIView(context:) method is updated to assign the context.coordinator as the navigationDelegate for the WKWebView. These modifications enable the SurveyWebView to better manage secure navigation events without altering its overall structure.

Suggested labels

🕹️ oss.gg, 🕹️ 150 points

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa2588d and 134b294.

📒 Files selected for processing (1)
  • packages/ios/FormbricksSDK/FormbricksSDK/WebView/SurveyWebView.swift (2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/ios/FormbricksSDK/FormbricksSDK/WebView/SurveyWebView.swift (1)
packages/android/formbricksSDK/src/main/java/com/formbricks/formbrickssdk/webview/WebAppInterface.kt (1) (1)
  • message (24-47)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Run Linters / Linters
  • GitHub Check: Build Formbricks-web / Build Formbricks-web
  • GitHub Check: Run E2E Tests / Run E2E Tests
🔇 Additional comments (3)
packages/ios/FormbricksSDK/FormbricksSDK/WebView/SurveyWebView.swift (3)

29-29: Good addition of navigationDelegate assignment.

The assignment of context.coordinator as the navigation delegate is correctly implemented here, which is necessary for handling URL authentication challenges. This change works in conjunction with the WKNavigationDelegate protocol conformance added to the Coordinator class.

55-55: Appropriate protocol conformance addition.

Adding the WKNavigationDelegate protocol to the Coordinator class is the correct approach for handling navigation-related events, including authentication challenges. This change aligns properly with the navigationDelegate assignment in the makeUIView method.

64-70:

Details

❓ Verification inconclusive

Security consideration for unconditional server trust acceptance.

While this implementation successfully addresses the goal of allowing the WebView to load content from servers with SSL certificates that might not be trusted by default, it does so by unconditionally accepting any server trust credential. This approach could potentially expose the app to man-in-the-middle attacks.

Consider implementing a more selective approach by:

  1. Validating against specific domains you control
  2. Implementing certificate pinning for known servers
  3. Adding additional validation logic for certificates in production environments

For development/testing purposes, this implementation may be acceptable, but please verify that this approach aligns with your security requirements.

🌐 Web query:

What are the security implications of using URLCredential(trust: serverTrust) without validation in iOS?

💡 Result:

Using URLCredential(trust: serverTrust) without proper validation in iOS poses significant security risks by bypassing critical certificate checks. Here's a breakdown of the implications:

Undermines Transport Layer Security

  • Disables Certificate Chain Validation: iOS normally validates the server's certificate against trusted root certificates and checks for expiration/revocation[2][6][14]. Bypassing this allows connections to servers with invalid, expired, or revoked certificates[5][8].
  • Enables Man-in-the-Middle Attacks: By accepting any certificate, attackers can intercept encrypted traffic using a self-signed or compromised certificate[2][7][10]. This violates TLS's core security premise.

Weakens SSL Pinning Benefits

Proper certificate pinning workflows compare server certificates against known-good values:

// Safe validation example from search results[3][6]
if serverCertificateData == pinnedCertificateData {
    completionHandler(.useCredential, URLCredential(trust: serverTrust))
}

Without this comparison, any certificate (including malicious ones) would be trusted[7][10].

Circumvents Apple Security Features

  • Bypasses App Transport Security (ATS): While ATS exceptions are sometimes needed for development, production apps should maintain strict validation[8][14].
  • Ignores Certificate Transparency: The system's certificate transparency checks become ineffective when validation is skipped[2][6].

Implementation Risks

Developers might use this pattern for local testing with self-signed certificates:

// Dangerous pattern from search results[5][8]
completionHandler(.useCredential, URLCredential(trust: challenge.protectionSpace.serverTrust!))

This creates security debt that could accidentally ship to production[5][7].

Recommended Secure Alternatives

  1. Certificate Pinning: Compare server certificate against embedded public key or certificate[3][6][10]
  2. Certified Validation: Use SecTrustEvaluateWithError for system-approved validation[4][14]
  3. Limited Exceptions: If required, scope ATS exceptions narrowly:
<key>NSExceptionDomains</key>
<dict>
    <key>example.com</key>
    <dict>
        <key>NSIncludesSubdomains</key>
        <true/>
    </dict>
</dict>
  1. Certificate Persistence: For local devices, validate once then persist approved certificates[7]

While convenient for development, using unvalidated URLCredential(trust:) eliminates multiple layers of iOS security designed to protect network communications. Production apps should always implement certificate validation through pinning or proper chain-of-trust verification[2][6][10][14].

Citations:

Security Review: Unconditional Trust in Server Credentials

The current implementation bypasses critical SSL/TLS validations by unconditionally accepting the server trust. This approach:

  • Disables Certificate Chain Validation: It skips the built-in checks that verify certificate validity (e.g., chain-of-trust, expiration, and revocation).
  • Exposes the App to Man-in-the-Middle Attacks: Without certificate pinning or domain-specific validation, attackers can intercept and manipulate encrypted communications.
  • Bypasses Apple Security Protections: It negates the benefits of App Transport Security (ATS) and certificate transparency, which are key security features in iOS.

For development and testing, this may be acceptable. However, for a production environment, please ensure that:

  • Validation against specific controlled domains is implemented.
  • Certificate pinning (or another robust, system-approved validation mechanism like SecTrustEvaluateWithError) is applied.
  • Any necessary ATS exceptions are tightly scoped.

Please verify that this approach aligns with your overall security requirements before shipping to production.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@mattinannt mattinannt merged commit d98eb5b into formbricks:main Mar 23, 2025
14 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattinannt mattinannt mattinannt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vaxi87 @mattinannt