fix: blocks http requests #22
Conversation
WalkthroughThe changes introduce stricter validation and enforcement of HTTPS URLs within the SDK. The setup process now explicitly checks for valid and secure URLs, aborting initialization if the URL is invalid or not HTTPS. The API client similarly blocks non-HTTPS URLs. A commented-out variable is removed from the survey manager. The test suite was updated to use a valid HTTPS URL for setup. Additionally, the APIClient test suite was expanded with new tests covering error cases and response handling. Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the security of HTTP requests in the FormbricksSDK module and cleans up unused code. Key changes include enforcing HTTPS in both APIClient and Formbricks, improving error handling during SDK setup, and removing an unused property from SurveyManager.
- Enforced HTTPS for all URL requests with improved logging and error handling.
- Updated URL validation in the SDK setup to ensure only HTTPS URLs are accepted.
- Removed redundant code and unnecessary whitespace in the SurveyManager.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Sources/FormbricksSDK/Networking/Base/APIClient.swift | Added HTTPS check in buildFinalURL to block HTTP requests for security. |
| Sources/FormbricksSDK/Formbricks.swift | Enhanced appUrl validation and enforced HTTPS, with improved error logging. |
| Sources/FormbricksSDK/Manager/SurveyManager.swift | Removed an unused property and cleaned up redundant whitespace. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/FormbricksSDK/Formbricks.swift(1 hunks)Sources/FormbricksSDK/Manager/SurveyManager.swift(0 hunks)Sources/FormbricksSDK/Networking/Base/APIClient.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Sources/FormbricksSDK/Manager/SurveyManager.swift
🧰 Additional context used
🧬 Code Graph Analysis (2)
Sources/FormbricksSDK/Networking/Base/APIClient.swift (1)
Sources/FormbricksSDK/Logger/Logger.swift (1)
error(37-39)
Sources/FormbricksSDK/Formbricks.swift (1)
Sources/FormbricksSDK/Logger/Logger.swift (1)
error(37-39)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-analyze
🔇 Additional comments (3)
Sources/FormbricksSDK/Networking/Base/APIClient.swift (1)
32-38: Excellent security enhancement with proper error handling.The HTTPS enforcement is well-implemented with case-insensitive scheme validation, appropriate error logging, and graceful failure handling through the completion callback.
Sources/FormbricksSDK/Formbricks.swift (2)
65-70: HTTPS enforcement implemented correctly.The HTTPS validation with error logging and early return provides appropriate security without crashing the application.
72-72: Service initialization simplification looks good.The direct assignment approach is cleaner than the previous implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/FormbricksSDKTests/Networking/APIClientTests.swift (2)
359-589: Excellent comprehensive error handling test coverage.These tests thoroughly validate various error scenarios including nil data, invalid responses, and multiple decoding error cases. This significantly improves the robustness of the test suite.
Consider using the non-optional
Data(_:)initializer for string conversions to align with SwiftLint recommendations:-let corruptedData = "corrupted data".data(using: .utf8) +let corruptedData = Data("corrupted data".utf8)-let incompleteData = "{\"id\": \"123\"}".data(using: .utf8) +let incompleteData = Data("{\"id\": \"123\"}".utf8)-let nullValueData = "{\"id\": \"123\", \"name\": null}".data(using: .utf8) +let nullValueData = Data("{\"id\": \"123\", \"name\": null}".utf8)-let typeMismatchData = "{\"id\": 123, \"name\": \"Test\"}".data(using: .utf8) +let typeMismatchData = Data("{\"id\": 123, \"name\": \"Test\"}".utf8)🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 461-461: Prefer non-optional
Data(_:)initializer when convertingStringtoData(non_optional_string_data_conversion)
[Warning] 496-496: Prefer non-optional
Data(_:)initializer when convertingStringtoData(non_optional_string_data_conversion)
[Warning] 531-531: Prefer non-optional
Data(_:)initializer when convertingStringtoData(non_optional_string_data_conversion)
[Warning] 566-566: Prefer non-optional
Data(_:)initializer when convertingStringtoData(non_optional_string_data_conversion)
591-628: Good test for requests with body data.The test properly validates API calls with request bodies and path parameters.
Minor improvement: Consider using the non-optional initializer at line 603:
-request.requestBody = "test".data(using: .utf8) +request.requestBody = Data("test".utf8)🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 595-595: Force tries should be avoided
(force_try)
[Warning] 603-603: Prefer non-optional
Data(_:)initializer when convertingStringtoData(non_optional_string_data_conversion)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/FormbricksSDK/Networking/Base/APIClient.swift(3 hunks)Tests/FormbricksSDKTests/Networking/APIClientTests.swift(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/FormbricksSDK/Networking/Base/APIClient.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Tests/FormbricksSDKTests/Networking/APIClientTests.swift
[Error] 262-262: Force tries should be avoided
(force_try)
[Error] 595-595: Force tries should be avoided
(force_try)
[Warning] 461-461: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 496-496: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 531-531: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 566-566: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
[Warning] 603-603: Prefer non-optional Data(_:) initializer when converting String to Data
(non_optional_string_data_conversion)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-analyze
🔇 Additional comments (7)
Tests/FormbricksSDKTests/Networking/APIClientTests.swift (7)
31-47: LGTM! Well-structured test double for void responses.The
MockVoidRequestfollows the same pattern asMockRequestand is appropriately designed for testing endpoints that returnVoidResponse.
121-124: Good addition to test headers in error scenarios.Testing header handling in failure cases ensures comprehensive coverage.
171-171: Correct error type for invalid URL scenarios.The change to
.invalidAppUrlaligns with the SDK's stricter URL validation.
222-256: Well-implemented test for void responses with path parameters.The test correctly validates
VoidResponsehandling and includes path parameter substitution testing.
258-294: Good test coverage for query parameters.The test properly validates multiple query parameters in successful requests.
Note: While the force try at line 262 is flagged by SwiftLint, it's acceptable in test code where failure would indicate a test setup issue.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 262-262: Force tries should be avoided
(force_try)
334-357: Critical test for enforcing HTTPS security.This test validates the key security enhancement of blocking HTTP URLs, ensuring only HTTPS requests are allowed.
630-677: Well-structured test for environment response parsing.The test properly validates nested JSON parsing and includes proper setup/teardown of
Formbricks.appUrl. Good use ofaddTeardownBlockfor cleanup.
|
This pull request introduces security enhancements and code cleanup in the
FormbricksSDKmodule. The most significant changes include enforcing HTTPS for all URLs, improving error handling during SDK setup, and removing unused code for better maintainability.Security Enhancements:
Sources/FormbricksSDK/Formbricks.swift: Added validation to ensureappUrluses HTTPS and block HTTP requests for security. Improved error handling with descriptive messages whenappUrlis invalid.Sources/FormbricksSDK/Networking/Base/APIClient.swift: Modified thebuildFinalURLmethod to enforce HTTPS for requests, logging errors and aborting operations if HTTP is detected.Code Cleanup:
Sources/FormbricksSDK/Manager/SurveyManager.swift: Removed unusedserviceproperty and cleaned up redundant whitespace in theSurveyManagerclass and its extension. [1] [2]Summary by CodeRabbit