Add configurable outbound host validation#13866
Open
JanithaSampathBandara wants to merge 3 commits into
Open
Conversation
JanithaSampathBandara
requested review from
AnuGayan,
Arshardh,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
npamudika,
pubudu538,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
|
JanithaSampathBandara seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds tenant-scoped NetworkSecurityAccessControl: schema and constants, central APIUtil.validateRemoteURL with host/IP checks, new exception codes, and integrates outbound URL validation across Key Managers, API imports (WSDL/GraphQL/AsyncAPI), MCP servers, publisher import utilities, tests, and templates. ChangesNetwork Security Access Control Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/PublisherCommonUtils.javaThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.java (1)
147-149: 💤 Low valueDifferentiate error message and description for consistency.
The
errorMessageanderrorDescriptionparameters are identical forNETWORK_SECURITY_ACCESS_CONTROL_MISCONFIGURED. Throughout this file, the convention is to use a brief label forerrorMessageand a more detailed explanation forerrorDescription. Consider updating to match this pattern.Suggested improvement
NETWORK_SECURITY_ACCESS_CONTROL_MISCONFIGURED(900406, - "Internal server error. Please contact the system administrator.", 500, - "Internal server error. Please contact the system administrator."), + "Network security access control misconfigured", 500, + "Internal server error. Please contact the system administrator."),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.java` around lines 147 - 149, The enum entry NETWORK_SECURITY_ACCESS_CONTROL_MISCONFIGURED in ExceptionCodes currently uses identical strings for errorMessage and errorDescription; change the first parameter to a concise label (e.g., "Network security access control misconfigured" or "Network access control misconfigured") and keep the second parameter as the detailed user-facing explanation (retain the longer "Internal server error. Please contact the system administrator." or replace with a more specific description), ensuring the signature of NETWORK_SECURITY_ACCESS_CONTROL_MISCONFIGURED(...) remains unchanged.components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java (1)
344-355: ⚡ Quick winExtract endpoint URL trust-validation into a shared helper.
The same extraction/validation block is duplicated in three places (Lines 344-355, 844-855, 917-928). A single helper would keep behavior consistent and prevent drift in security validation logic.
Refactor sketch
+ private static void validateEndpointURLs(org.json.JSONObject endpointConfigObj, String tenantDomain) + throws APIManagementException { + if (APIConstants.ENDPOINT_TYPE_DEFAULT.equalsIgnoreCase( + endpointConfigObj.optString(APIConstants.API_ENDPOINT_CONFIG_PROTOCOL_TYPE))) { + return; + } + ArrayList<String> endpointURLs = new ArrayList<>(); + APIUtil.extractURLsFromEndpointConfig(endpointConfigObj, APIConstants.API_DATA_PRODUCTION_ENDPOINTS, + endpointURLs); + APIUtil.extractURLsFromEndpointConfig(endpointConfigObj, APIConstants.API_DATA_SANDBOX_ENDPOINTS, + endpointURLs); + APIUtil.extractURLsFromEndpointConfig(endpointConfigObj, APIConstants.ENDPOINT_PRODUCTION_FAILOVERS, + endpointURLs); + APIUtil.extractURLsFromEndpointConfig(endpointConfigObj, APIConstants.ENDPOINT_SANDBOX_FAILOVERS, + endpointURLs); + for (String endpointURL : endpointURLs) { + APIUtil.validateRemoteURL(endpointURL, tenantDomain); + } + }Also applies to: 844-855, 917-928
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java` around lines 344 - 355, Create a shared helper in ImportUtils (e.g., extractAndValidateEndpointURLs or validateEndpointConfigURLs) that takes the endpointConfigObj and tenantDomain, performs the ArrayList<String> endpointURLs creation, calls APIUtil.extractURLsFromEndpointConfig for APIConstants.API_DATA_PRODUCTION_ENDPOINTS, APIConstants.API_DATA_SANDBOX_ENDPOINTS, APIConstants.ENDPOINT_PRODUCTION_FAILOVERS and APIConstants.ENDPOINT_SANDBOX_FAILOVERS, and then iterates calling APIUtil.validateRemoteURL(endpointURL, tenantDomain); replace the duplicated blocks at the locations currently using the same extraction/validation logic (the blocks around the current calls shown using APIUtil.extractURLsFromEndpointConfig and APIUtil.validateRemoteURL) with a single call to this new helper to keep validation consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.java`:
- Line 4006: The HOSTS constant in APIConstants currently uses the singular key
value "Host" which mismatches the feature contract/tenant key expecting "Hosts";
update the value of the HOSTS constant (public static final String HOSTS) to use
the plural "Hosts" so it aligns with NetworkSecurityAccessControl/tenant keys
and downstream lookups; after changing the constant value, run a quick search
for references to APIConstants.HOSTS to ensure no callers rely on the old
singular key and update any test/fixture data that asserted the previous string.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.java`:
- Around line 12571-12576: The code in APIUtil currently treats a non-empty
hosts with blank mode as a warn-and-continue (fail-open); change this to surface
as a configuration error: in the same block that checks
StringUtils.isBlank(mode) and sees hosts != null && !hosts.isEmpty(), replace
the silent warn with logging an error and throwing an exception (e.g.,
APIManagementException or IllegalStateException) that contains a clear message
referencing the misconfigured `hosts` + blank `mode`, so callers of APIUtil will
get an HTTP 500 rather than silently ignoring the access-control list.
- Around line 12577-12599: The current early return when isHostInList(host,
hosts) is true bypasses private-network checks; change the logic so that after a
hostname allowlist hit you still call InetAddress.getAllByName(host) and
validate the resolved IPs against the private-network rules before returning: if
blockPrivateNetworkAccess is enabled (or the equivalent flag used elsewhere) and
any resolved IP falls into private ranges (10/8, 172.16/12, 192.168/16,
fc00::/7), throw buildURLBlockedException(host); only return immediately if the
hostname is allowlisted and none of the resolved addresses violate the
private-network restriction. Use the existing isHostInList,
InetAddress.getAllByName, isAnyResolvedIpInList and buildURLBlockedException
symbols to locate and implement the check.
- Around line 12501-12510: validateRemoteURL() incorrectly uses new
URL(url).getHost(), which triggers a MalformedURLException for ws:// or wss://
schemes; instead, change validateRemoteURL() to parse the host without requiring
a URLStreamHandler by using java.net.URI (e.g., new
URI(url).getHost()/getAuthority()) or by delegating to the existing
validateEndpointURL() logic that uses Apache Commons UrlValidator with
ALLOW_ALL_SCHEMES; update the exception mapping so legitimate websocket
endpoints are not classified as MALFORMED_URL and only throw
APIManagementException when URI/UrlValidator indicates a truly invalid endpoint.
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/resources/tenant/tenant-config-schema.json`:
- Around line 1366-1395: NetworkSecurityAccessControl allows missing or
arbitrary Mode values; update the JSON schema for the
NetworkSecurityAccessControl object to make the "Mode" property required and
restrict its value to the enum ["allow","deny"] (modify the "Mode" subschema to
add an "enum" with those two strings) and add "Mode" to the parent object's
required array so config validation fails for missing/invalid modes; reference
the NetworkSecurityAccessControl object and the "Mode" property in your change.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/KeyManagersApiServiceImpl.java`:
- Around line 355-359: In validateKeyManagerURL, stop blindly returning on any
MalformedURLException from new URL(url).getHost(); instead, explicitly check for
the legacy sentinel value(s) (e.g., the exact string "none") and only skip
validation for those; for any other malformed URL, propagate a 400 Bad Request
(e.g., throw the same BadRequestException used elsewhere) so
APIUtil.validateRemoteURL(...) still runs for valid URLs and network/host policy
checks are enforced. Ensure the change is applied inside
KeyManagersApiServiceImpl.validateKeyManagerURL around the new URL(...) call and
that APIUtil.validateRemoteURL(...) is invoked for non-sentinel, well-formed
URLs.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/PublisherCommonUtils.java`:
- Around line 2703-2706: PublisherCommonUtils currently calls
APIUtil.validateRemoteURL unconditionally for each endpoint (using tenantDomain
from RestApiCommonUtil.getLoggedInUserTenantDomain()), which causes non-URL
endpoint styles (e.g., JMS, consul(…), and parameterized endpoints with { }
allowed by validateEndpointURL) to fail; change the loop so
APIUtil.validateRemoteURL is only invoked for real HTTP(S) URLs — skip
validation when the endpoint starts with "jms:" or "consul(" or contains
parameter placeholders ("{" or "}") — keeping the existing tenantDomain usage
and leaving validateEndpointURL behavior intact.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.java`:
- Line 3208: Replace calls to APIUtil.validateRemoteURL(...,
RestApiCommonUtil.getLoggedInUserTenantDomain()) with the request-scoped
organization from RestApiUtil.getValidatedOrganization(messageContext); locate
each occurrence in ApisApiServiceImpl (e.g., the call shown and the other listed
ranges) and pass RestApiUtil.getValidatedOrganization(messageContext) as the
organization argument instead of
RestApiCommonUtil.getLoggedInUserTenantDomain(), ensuring messageContext is
available in the calling scope (retrieve or add it where needed) so the SSRF
checks use the validated organization for that request.
- Around line 4741-4748: The callers of APIUtil.validateRemoteURL in the
AsyncAPI validate/import/update flows are swallowing client-side 400 errors and
converting them to 500; update each AsyncAPI handler that currently catches
APIManagementException around APIUtil.validateRemoteURL to mirror the GraphQL
handling: inside the catch inspect e.getErrorHandler() and if
getHttpStatusCode()==400 rethrow
RestApiUtil.buildBadRequestException(e.getErrorHandler().getErrorDescription()),
otherwise rethrow or pass through to existing handleInternalServerError logic;
specifically change the catch blocks that surround APIUtil.validateRemoteURL in
the AsyncAPI validate/import/update methods so 400s are preserved as BadRequest
responses instead of becoming internal errors.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/McpServersApiServiceImpl.java`:
- Line 1008: Calls to APIUtil.validateRemoteURL in McpServersApiServiceImpl are
using RestApiCommonUtil.getLoggedInUserTenantDomain() which enforces the tenant
scope instead of the request-validated organization; update those calls (the
occurrences around APIUtil.validateRemoteURL) to pass
RestApiUtil.getValidatedOrganization(messageContext) as the org parameter
(ensure you import/use RestApiUtil and have access to messageContext) so
validation runs in the correct organization scope.
- Around line 2436-2450: The current conditional around
APIConstants.ENDPOINT_TYPE_DEFAULT prevents validateRemoteURL from running for
default protocol endpoints; remove or alter the if-check so that
APIUtil.validateRemoteURL(...) is invoked for all endpoint URLs extracted from
endpointConfig (including when APIConstants.API_ENDPOINT_CONFIG_PROTOCOL_TYPE
equals APIConstants.ENDPOINT_TYPE_DEFAULT). Locate the block using
endpointConfig and APIConstants.ENDPOINT_TYPE_DEFAULT in
McpServersApiServiceImpl and ensure the code always collects endpoints (via
APIUtil.extractURLsFromEndpointConfig) and calls
APIUtil.validateRemoteURL(endpoint, tenantDomain) for each entry, preserving the
existing extraction of production/sandbox/failover lists.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/utils/RestApiPublisherUtils.java`:
- Around line 748-751: Replace the tenant domain argument to
APIUtil.validateRemoteURL(...) so it uses the request-scoped organization from
the request context instead of RestApiCommonUtil.getLoggedInUserTenantDomain();
obtain the request organization via the appropriate RestApiCommonUtil
request-context accessor (the request-scoped organization helper provided by
RestApiCommonUtil) and pass that value to APIUtil.validateRemoteURL in this
method and the other occurrences where APIUtil.validateRemoteURL is called
(e.g., the calls referenced around lines 656-657).
---
Nitpick comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.java`:
- Around line 147-149: The enum entry
NETWORK_SECURITY_ACCESS_CONTROL_MISCONFIGURED in ExceptionCodes currently uses
identical strings for errorMessage and errorDescription; change the first
parameter to a concise label (e.g., "Network security access control
misconfigured" or "Network access control misconfigured") and keep the second
parameter as the detailed user-facing explanation (retain the longer "Internal
server error. Please contact the system administrator." or replace with a more
specific description), ensuring the signature of
NETWORK_SECURITY_ACCESS_CONTROL_MISCONFIGURED(...) remains unchanged.
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.java`:
- Around line 344-355: Create a shared helper in ImportUtils (e.g.,
extractAndValidateEndpointURLs or validateEndpointConfigURLs) that takes the
endpointConfigObj and tenantDomain, performs the ArrayList<String> endpointURLs
creation, calls APIUtil.extractURLsFromEndpointConfig for
APIConstants.API_DATA_PRODUCTION_ENDPOINTS,
APIConstants.API_DATA_SANDBOX_ENDPOINTS,
APIConstants.ENDPOINT_PRODUCTION_FAILOVERS and
APIConstants.ENDPOINT_SANDBOX_FAILOVERS, and then iterates calling
APIUtil.validateRemoteURL(endpointURL, tenantDomain); replace the duplicated
blocks at the locations currently using the same extraction/validation logic
(the blocks around the current calls shown using
APIUtil.extractURLsFromEndpointConfig and APIUtil.validateRemoteURL) with a
single call to this new helper to keep validation consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4fe2e25-0b31-4da0-82ca-5b85275ce1c1
📒 Files selected for processing (12)
components/apimgt/org.wso2.carbon.apimgt.api/src/main/java/org/wso2/carbon/apimgt/api/ExceptionCodes.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIConstants.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/utils/APIUtil.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/resources/tenant/tenant-config-schema.jsoncomponents/apimgt/org.wso2.carbon.apimgt.rest.api.admin.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/admin/v1/impl/KeyManagersApiServiceImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/ImportUtils.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/PublisherCommonUtils.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/mappings/PublisherCommonUtilsTest.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/ApisApiServiceImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/impl/McpServersApiServiceImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/utils/RestApiPublisherUtils.javafeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2
Author
|
Reviewed the AI-generated log improvement suggestions and validated them manually.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
API Manager performs outbound network requests using user-provided URLs across several features, including:
These outbound requests may access destinations outside the control of the API Manager runtime. This improvement introduces centralized outbound network access control and destination validation for user-provided URLs.
Goals
Approach
Platform-Level Configuration
Network access control can be configured using:
Platform administrators can:
Platform-level validation is always evaluated before tenant-level validation.
Tenant-Level Configuration
Introduced optional tenant-level network access control through tenant configuration.
{ "NetworkSecurityAccessControl": { "Mode": "allow", "Hosts": [ "*.allowed.example.com" ], "BlockPrivateNetworkAccess": true } }Behavior:
Validation Behavior
Covered Validation Entry Points
The following endpoints are protected by this implementation:
Error Handling
Client validation failures:
Configuration issues:
Detailed diagnostics remain available in server logs.
Testing
Added and updated coverage for:
Backward Compatibility
When network access control configuration is not present, existing behavior remains unchanged.