-
Notifications
You must be signed in to change notification settings - Fork 158
INTENG-22685 - Integration validator protected endpoint bugfix #1269
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.
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
base: master
Are you sure you want to change the base?
INTENG-22685 - Integration validator protected endpoint bugfix #1269
Conversation
… be used for that server request Added bool to getAPIBaseUrl for whether or not custom endpoint should be used for that server request
Updated call in testbed code with the new parameter
ReferenceINTENG-22685 -- Integration validator protected endpoint bugfix. DescriptionSummary By MatterAI
🔄 What Changed
🔍 Impact of the ChangeThis change directly addresses a bug where the Integration Validator, when used by customers with Advanced Compliance (who use protected API endpoints), would fail because the validator's 📁 Total Files Changed5 files changed. 🧪 Test AddedNo new dedicated unit or integration tests are explicitly added in the patch. However, the fix is for the Integration Validator, which is a testing tool itself. The expectation is that existing integration validator tests, which previously failed under Advanced Compliance configurations, will now pass successfully due to this change. 🔒Security VulnerabilitiesNo new security vulnerabilities are introduced. On the contrary, this change enhances security by ensuring that a specific internal testing endpoint ( Testing InstructionsN/A Risk Assessment [
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses an important bug fix for the integration validator protected endpoint. The implementation looks good overall, with a clear approach to ensure protected endpoints always use the default URL while allowing other endpoints to use custom URLs when appropriate. I have a few suggestions to improve the code:
public String getAPIBaseUrl(boolean useCustom) { | ||
if (useCustom && URLUtil.isHttpsUrl(customServerURL_)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Code Refactor
Issue: The URL validation is only checking if the URL starts with https, but not validating if it's a properly formatted URL.
Fix: Use additional URL validation to ensure the custom URL is properly formatted.
Impact: Prevents potential issues with malformed URLs that happen to start with https.
public String getAPIBaseUrl(boolean useCustom) { | |
if (useCustom && URLUtil.isHttpsUrl(customServerURL_)) { | |
public String getAPIBaseUrl(boolean useCustom) { | |
if (useCustom && !TextUtils.isEmpty(customServerURL_) && URLUtil.isHttpsUrl(customServerURL_)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omitting this change since that is something outside of this PR (pre-existing code not related to the change)
Branch-SDK/src/main/java/io/branch/referral/validators/ServerRequestGetAppConfig.java
Show resolved
Hide resolved
Branch-SDK-TestBed/src/main/java/io/branch/branchandroidtestbed/SettingsActivity.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a boolean flag to PrefHelper.getAPIBaseUrl()
to allow test code (like the integration validator) to bypass the protected endpoint, and updates all callers to pass the appropriate flag value.
- Change
getAPIBaseUrl()
signature to accept aboolean
for endpoint selection - Update validator and core request classes to pass
false
ortrue
as needed - Adjust TestBed settings to use the new method
Comments suppressed due to low confidence (6)
Branch-SDK/src/main/java/io/branch/referral/PrefHelper.java:242
- [nitpick] Parameter name 'useCustom' is ambiguous; consider renaming it to something more descriptive like 'includeCustomServerUrl' or splitting into two methods (e.g.,
getDefaultAPIBaseUrl()
andgetProtectedAPIBaseUrl()
).
public String getAPIBaseUrl(boolean useCustom) {
Branch-SDK/src/main/java/io/branch/referral/PrefHelper.java:239
- Javadoc is missing a @param entry for the new 'useCustom' parameter; please add a description explaining its purpose and effects.
* @return A {@link String} variable containing the hard-coded base URL that the Branch API uses.
Branch-SDK/src/main/java/io/branch/referral/PrefHelper.java:242
- The new boolean flag introduces branching behavior; consider adding unit tests covering both
true
andfalse
cases to ensure protected and default endpoints behave as expected.
public String getAPIBaseUrl(boolean useCustom) {
Branch-SDK/src/main/java/io/branch/referral/validators/ServerRequestGetAppConfig.java:46
- [nitpick] Passing a raw boolean literal ('false') makes the call’s intent unclear; consider providing a named helper method or constant to indicate why the protected endpoint is skipped here.
return prefHelper_.getAPIBaseUrl(false) + getRequestPath() + "/" + prefHelper_.getBranchKey();
Branch-SDK/src/main/java/io/branch/referral/ServerRequest.java:255
- [nitpick] Using a literal 'true' hides the fact that this call should hit the protected endpoint; consider overloading or renaming for clarity (e.g.,
getProtectedAPIBaseUrl()
).
return prefHelper_.getAPIBaseUrl(true) + requestPath_.getPath();
Branch-SDK/src/main/java/io/branch/referral/Branch.java:1796
- [nitpick] Literal boolean arguments can be confusing; extracting this into a clearly named method (e.g.,
getProtectedAPIBaseUrl()
) would improve readability.
prefHelper_.getAPIBaseUrl(true) + Defines.RequestPath.GetURL.getPath(),
…equestGetAppConfig.java Added comment for code clarity Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
…d/SettingsActivity.java Added null check to prevent NPE Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Reference
INTENG-22685
Description
If a customer is using Advanced Compliance and then runs the Integration Validator code, the Integration Validator will not work. This is because the protected-api endpoint gets used for the requests, and there is no app-settings protected equivalent for that endpoint. Since the Integration Validator code is never released into the wild, but only used for testing, we can safely keep that endpoint going to the regular Branch endpoint, with all other calls like opens, installs, etc... still going through the protected endpoint for Advanced Compliance customers. To address this, I've added a boolean parameter to the getAPIBaseUrl() function, which can take in false in the case of any test code, like the integration validator, so that the regular endpoint (instead of protected-api) is used.
Testing Instructions
Risk Assessment [
LOW
]cc @BranchMetrics/saas-sdk-devs for visibility.