Add security rules for JavaScript settings and base URL in Swift web views #48
Conversation
WalkthroughTwo new security rules have been added to YAML configuration files for Swift web views: one for managing JavaScript settings and another for ensuring proper base URL configurations. The JavaScript rule warns against enabling JavaScript unnecessarily, while the base URL rule addresses the risk of setting it to Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
tests/swift/swift-webview-config-base-url-swift-test.yml (2)
3-5: Consider adding more comprehensive valid test cases.While the current valid case tests WKWebView with nil baseURL, it would be beneficial to include additional valid cases:
- WKWebView with a proper baseURL
- WKWebView with different content loading methods
This would help ensure the rule doesn't generate false positives.
Example addition:
valid: - | let webview2 = WKWebView(...) webview2.loadHTMLString(someHtmlString, baseURL: nil) + - | + let webview3 = WKWebView(...) + webview3.loadHTMLString(someHtmlString, baseURL: URL(string: "https://example.com"))
1-12: Consider adding test cases for edge cases and security scenarios.While the current test cases cover basic usage, consider adding tests for:
- Handling of about:blank URLs
- File URL handling
- Custom URL schemes
- Data URL handling
These scenarios are common attack vectors in web view implementations.
Would you like me to provide example test cases for these security-critical scenarios?
tests/swift/swift-webview-config-allows-js-swift-test.yml (1)
9-18: Consider adding more invalid test cases for comprehensive coverage.The current invalid cases cover basic scenarios, but consider adding tests for:
- Missing configuration entirely
- Null/nil preferences
- Mixed usage of both APIs in the same configuration
Example additional test case:
- | let config = WKWebViewConfiguration() // Missing preferences configuration entirely - | let config = WKWebViewConfiguration() config.defaultWebpagePreferences = nilrules/swift/security/swift-webview-config-base-url-swift.yml (2)
1-12: Consider enhancing the security metadata.While the metadata is well-structured, consider the following improvements:
- The severity level might need to be elevated to "error" since origin abuse can lead to serious security vulnerabilities
- The message could explicitly mention potential attack vectors (e.g., XSS, data theft)
- Add more specific references such as:
- OWASP Top 10 Mobile risks
- Apple's WebKit security documentation
1-65: Consider expanding rule scope for WKWebView.While this rule correctly addresses baseURL security for UIWebView, note that:
- UIWebView is deprecated and should be migrated to WKWebView
- Similar baseURL security concerns apply to WKWebView
- The rule could be expanded to cover both view types or include a separate rule for WKWebView
Consider adding deprecation warnings when UIWebView is detected and suggesting migration to WKWebView.
tests/__snapshots__/swift-webview-config-base-url-swift-snapshot.yml (2)
1-114: Consider adding positive test casesThe current snapshots only demonstrate the unsafe patterns (negative cases). Consider adding positive test cases that show the correct usage with:
- WKWebView instead of UIWebView
- Proper baseURL configuration
- Secure content loading patterns
This would provide a complete picture of both what to avoid and what to do instead.
Would you like me to help create additional snapshot test cases demonstrating the secure patterns?
6-58: Improve test coverage with additional assertionsWhile the current labels provide good coverage for method calls and parameters, consider adding assertions for:
- Error handling scenarios
- Different baseURL values (not just nil)
- Various content types and encodings
This would help ensure the security rule catches all potential misconfigurations.
Also applies to: 62-114
rules/swift/security/swift-webview-config-allows-js-swift.yml (3)
1-11: Fix typo in rule message: "privelege" → "privilege"The rule message contains a spelling error that should be corrected for professionalism.
- the principle of least privelege. + the principle of least privilege.
12-108: Add documentation for the pattern matching logicThe complex AST pattern matching would benefit from documentation explaining:
- What code patterns are being matched
- Examples of code that would trigger the rule
- Examples of code that would not trigger the rule
Add a comment block above the
utilssection explaining the pattern matching logic:# Detects JavaScript enabled in WebViews through: # 1. Direct property assignments: # webView.configuration.preferences.javaScriptEnabled = true # webView.configuration.preferences.allowsContentJavaScript = true # 2. Property declarations with WKPreferences/WKWebpagePreferences initialization # # Does not flag when JavaScript is explicitly disabled or when the property # is not set (defaults to false). # # Example matches: # let preferences = WKPreferences() # preferences.javaScriptEnabled = true # # Example non-matches: # let preferences = WKPreferences() # preferences.javaScriptEnabled = false utils:
108-110: Consider adding severity conditionsThe rule currently treats all JavaScript enabling equally. Consider adding severity conditions based on the context:
- High severity: JavaScript enabled globally
- Warning: JavaScript enabled for specific URLs
Would you like me to propose a pattern for contextual severity?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- rules/swift/security/swift-webview-config-allows-js-swift.yml (1 hunks)
- rules/swift/security/swift-webview-config-base-url-swift.yml (1 hunks)
- tests/snapshots/swift-webview-config-allows-js-swift-snapshot.yml (1 hunks)
- tests/snapshots/swift-webview-config-base-url-swift-snapshot.yml (1 hunks)
- tests/swift/swift-webview-config-allows-js-swift-test.yml (1 hunks)
- tests/swift/swift-webview-config-base-url-swift-test.yml (1 hunks)
🔇 Additional comments (9)
tests/swift/swift-webview-config-base-url-swift-test.yml (2)
1-2: LGTM! Clear and descriptive test configuration ID.The configuration ID accurately reflects its purpose for Swift web view base URL validation.
6-12: Verify the completeness of invalid test cases.The test cases correctly identify UIWebView usage as invalid. However, let's verify if we're covering all potential misuse patterns.
Additionally, consider adding comments to explain why these cases are invalid:
invalid: - | + # UIWebView is deprecated and poses security risks let webview = UIWebView(...) webview.loadHTMLString(someHtmlString, baseURL: nil) - | + # Using nil baseURL with UIWebView can lead to origin abuse let webview3 = UIWebView(...) webview3.load(data, mimetype: "application/json", textEncodingName: "UTF8", baseURL: nil)✅ Verification successful
Test cases are comprehensive and well-structured
The current test cases adequately cover the main security concerns with UIWebView:
- They test both
loadHTMLStringandloadmethods with nil baseURL- The rule implementation properly detects these patterns through AST matching
- The security message clearly explains the risk of origin abuse
- The rule references relevant CWE-272 and MASVS standards
The suggestion to add comments is not necessary as the rule already includes detailed documentation in its message and note fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other UIWebView loading methods that should be tested rg "UIWebView.*load" --type swiftLength of output: 184
Script:
#!/bin/bash # Let's first check what Swift files exist in the repository fd -e swift # Then let's search for any UIWebView related patterns more broadly rg "UIWebView" -lLength of output: 218
Script:
#!/bin/bash # Let's examine the content of these files to understand the full context cat tests/swift/swift-webview-config-base-url-swift-test.yml rules/swift/security/swift-webview-config-base-url-swift.yml tests/__snapshots__/swift-webview-config-base-url-swift-snapshot.ymlLength of output: 5062
tests/swift/swift-webview-config-allows-js-swift-test.yml (1)
1-1: LGTM! Clear and descriptive test case identifier.The test case ID follows a clear naming convention and accurately reflects its purpose.
rules/swift/security/swift-webview-config-base-url-swift.yml (1)
32-45: Enhance nil detection patterns.The current pattern only catches literal
nilassignments but might miss:
- Implicit nil (e.g., optional parameters)
- Variables containing nil
- Optional chaining results
Let's verify if there are such cases in the codebase:
✅ Verification successful
The current pattern is sufficient for its security purpose
Based on the search results, we can see that the pattern is correctly targeting the security concern of using nil baseURL in WebView configurations. The test files show that the primary use case is indeed direct nil assignments (
baseURL: nil), which is precisely what the current pattern detects. While there could theoretically be other ways to pass nil values, the pattern focuses on the most common and direct security risk.The test cases in the codebase demonstrate:
- Direct nil assignments in
loadHTMLStringcalls- Similar patterns in related methods like
load(data, ...)withbaseURL: nilSince this is a security rule specifically targeting the unsafe practice of explicitly setting nil base URLs in WebView configurations, the current pattern appropriately captures the intended violations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential missed nil assignments in web view configurations # Look for optional parameters and nil assignments in web view related code ast-grep --pattern 'loadHTMLString($_: String, baseURL: $_?)' # Look for variable assignments that might contain nil rg -l "baseURL\s*=\s*\w+" | xargs rg "let\s+\w+\s*:\s*URL\?"Length of output: 126
Script:
#!/bin/bash # Let's try a different approach to find WebView configurations and nil assignments # Look for WebView configurations with baseURL rg -A 5 "loadHTMLString.*baseURL" # Search for WebView related code with optional URL types rg "URL\?" -A 3 -B 3 # Find any WebView configuration patterns ast-grep --pattern 'webView.loadHTMLString($$$)' # Look for optional URL declarations and assignments ast-grep --pattern 'var $_: URL?'Length of output: 1687
tests/__snapshots__/swift-webview-config-allows-js-swift-snapshot.yml (3)
1-2: LGTM: Clear snapshot identifier.The snapshot file is properly identified for the Swift web view JavaScript configuration security rule.
3-51: Verify the WKPreferences JavaScript configuration pattern.The snapshot correctly captures the pattern for detecting JavaScript enablement in WKPreferences, including:
- Primary label on the direct JavaScript enablement
- Secondary labels for precise AST matching
- Proper character positions for accurate source mapping
This will help identify instances where JavaScript is explicitly enabled, which is the security concern being addressed.
Let's verify this pattern against the codebase:
✅ Verification successful
Pattern verification successful for WKPreferences JavaScript configuration
The pattern correctly identifies instances of JavaScript enablement in WKPreferences, as evidenced by matching occurrences in both test and snapshot files. The pattern's structure accurately captures:
- The WKPreferences initialization
- Explicit JavaScript enablement via the
javaScriptEnabledproperty- Proper source mapping with accurate character positions
The matches found validate that this security rule is necessary for detecting potentially risky JavaScript configurations in WebKit preferences.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the prevalence of JavaScript enablement in WKPreferences # Expected: Find instances where JavaScript is being enabled, confirming the need for this security rule # Search for WKPreferences JavaScript configuration rg -U "let.*WKPreferences.*\n.*javaScriptEnabled.*=.*true"Length of output: 474
52-101: Verify the WKWebpagePreferences JavaScript configuration pattern.The snapshot correctly captures an alternative pattern for JavaScript enablement through WKWebpagePreferences, including:
- Primary label on allowsContentJavaScript enablement
- Secondary labels for precise AST matching
- Configuration chain with WKWebViewConfiguration
This comprehensively covers another vector for JavaScript enablement.
Let's verify this pattern against the codebase:
✅ Verification successful
The WKWebpagePreferences JavaScript configuration pattern is correctly captured
The verification confirms that the snapshot correctly represents the JavaScript enablement pattern through WKWebpagePreferences. The codebase shows consistent usage of both:
- Modern
WKWebpagePreferences.allowsContentJavaScriptpattern (as shown in snapshot)- Legacy
WKPreferences.javaScriptEnabledpatternThe test files and snapshots demonstrate proper coverage of both patterns, with appropriate labeling for AST matching. The rule file (swift-webview-config-allows-js-swift.yml) also confirms that both patterns are properly handled in security checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the prevalence of JavaScript enablement in WKWebpagePreferences # Expected: Find instances where content JavaScript is being enabled # Search for WKWebpagePreferences JavaScript configuration rg -U "let.*WKWebpagePreferences.*\n.*allowsContentJavaScript.*=.*true"Length of output: 474
Script:
#!/bin/bash # Let's search for both WKWebpagePreferences and WKPreferences patterns # to understand the complete JavaScript enablement landscape # Search for WKWebpagePreferences pattern rg -U "WKWebpagePreferences.*\n.*allowsContentJavaScript.*=.*true" -A 2 # Search for WKPreferences pattern rg -U "WKPreferences.*\n.*javaScriptEnabled.*=.*true" -A 2 # Search for any other potential JavaScript enablement in WebKit configurations rg -i "javascript.*enabled|javascript.*allow" -A 2Length of output: 4851
tests/__snapshots__/swift-webview-config-base-url-swift-snapshot.yml (1)
4-5: Warning: UIWebView is deprecated and poses security risksThe snapshots use
UIWebViewwhich has been deprecated by Apple since iOS 12 due to security and performance concerns. While these appear to be negative test cases demonstrating unsafe patterns, it's worth noting that:
- Production code should migrate to
WKWebView- Setting
nilasbaseURLcan lead to:
- Origin spoofing
- Cross-origin resource access issues
- Potential bypass of same-origin policy
Let's verify if there are any remaining UIWebView usages that need migration:
Also applies to: 60-61
rules/swift/security/swift-webview-config-allows-js-swift.yml (1)
12-108: Verify coverage of all JavaScript-enabling propertiesLet's verify that we're catching all possible ways to enable JavaScript in WebViews.
✅ Verification successful
JavaScript configuration properties are properly covered
Based on the search results, the current rule correctly covers all JavaScript-enabling properties in WebKit:
javaScriptEnabledfor WKPreferencesallowsContentJavaScriptfor WKWebpagePreferencesThe test files and snapshots confirm these are the main properties used to enable JavaScript in WebViews. The rule's pattern matches both properties and their respective preference classes (
WKPreferencesandWKWebpagePreferences). No additional JavaScript-enabling configuration properties were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for JavaScript-related properties in WebKit and UIKit frameworks # that might not be covered by the current patterns # Look for additional JavaScript-related properties rg -i "javascript.*enabled|javascript.*allow" # Look for WebView configuration patterns ast-grep --pattern 'class $_ { $$$ var $F: $_ $$$ }' | rg -i "javascript|webview|configuration"Length of output: 1220
Script:
#!/bin/bash # Let's search more specifically for WebView configurations and JavaScript-related settings # Search for WKWebView configuration patterns rg -i "wkwebview|uiwebview" -A 5 -B 5 # Search for WKPreferences and WKWebpagePreferences rg -i "wkpreferences|webpagepreferences" -A 5 -B 5 # Look for any other potential JavaScript configuration methods rg -i "javascript.*config|script.*enabled|script.*allow" -A 3Length of output: 20022
Summary by CodeRabbit
Release Notes
New Features
Tests