[Draft] Add external WebSub Hub integration and wildcard topic matching support#13857
Open
ashanhr wants to merge 4 commits into
Open
[Draft] Add external WebSub Hub integration and wildcard topic matching support#13857ashanhr wants to merge 4 commits into
ashanhr wants to merge 4 commits into
Conversation
ashanhr
requested review from
AnuGayan,
Arshardh,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
npamudika,
pubudu538,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
Comment on lines
+365
to
+369
| private String invokeExtensionListenerForTopicMatching(MessageContext synCtx, String providedTopic, | ||
| List<String> urlPatterns) { | ||
| Map<String, Object> existingCustomProps = (Map<String, Object>) synCtx.getProperty( | ||
| APIMgtGatewayConstants.CUSTOM_PROPERTY); | ||
|
|
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| private String invokeExtensionListenerForTopicMatching(MessageContext synCtx, String providedTopic, | |
| List<String> urlPatterns) { | |
| Map<String, Object> existingCustomProps = (Map<String, Object>) synCtx.getProperty( | |
| APIMgtGatewayConstants.CUSTOM_PROPERTY); | |
| private String invokeExtensionListenerForTopicMatching(MessageContext synCtx, String providedTopic, | |
| List<String> urlPatterns) { | |
| if (log.isDebugEnabled()) { | |
| log.debug("Invoking extension listener for topic matching. Topic: " + providedTopic); | |
| } | |
| Map<String, Object> existingCustomProps = (Map<String, Object>) synCtx.getProperty( | |
| APIMgtGatewayConstants.CUSTOM_PROPERTY); |
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.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | N | Unnecessary logs |
| #### Log Improvement Suggestion No: 2 | N | Unnecessary logs |
| #### Log Improvement Suggestion No: 3 | N | Unnecessary logs |
| #### Log Improvement Suggestion No: 4 | N | Unnecessary logs |
| #### Log Improvement Suggestion No: 5 | N | Unnecessary logs |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds external WebSub hub support and topic resolution: a new ChangesWebSub External Hub Support and Topic Matching
sequenceDiagram
participant Client
participant WebhookApiHandler
participant ExtensionListener
participant externalWebSubHubCaller
participant ExtensionListenerUtil
Client->>WebhookApiHandler: handleRequest()
WebhookApiHandler->>ExtensionListener: invokeExtensionListenerForTopicMatching(topic)
ExtensionListener-->>WebhookApiHandler: matchedTopic / abort
alt external hub needed
WebhookApiHandler->>externalWebSubHubCaller: call subscribe/unsubscribe/publish
externalWebSubHubCaller-->>WebhookApiHandler: HTTP status / response
end
WebhookApiHandler->>ExtensionListenerUtil: postProcess/request delegation
ExtensionListenerUtil-->>WebhookApiHandler: continuation
🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds support for delegating WebSub subscribe/unsubscribe/publish operations to an external WebSub Hub and enhances WebSub topic handling in the gateway to support wildcard topic matching via URI-template matching and extension hooks.
Changes:
- Adds external WebSub Hub configuration wiring (config template + runtime config parsing + Velocity context flags).
- Refactors
WebhookApiHandlerto support wildcard topic matching and introducesExtensionType.WEBSUB_TOPIC_MATCHING. - Updates the WebSub API Synapse template logic to branch to an external-hub sequence for publish/subscribe flows.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| features/apimgt/org.wso2.carbon.apimgt.gateway.feature/src/main/resources/p2.inf | Installs the external hub caller sequence into gateway synapse sequences. |
| features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2 | Adds external hub config template and includes the external hub sequence in the gateway sync skip list. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/resources/repository/resources/api_templates/websub_api_template.xml | Updates WebSub Synapse template to route via external hub and adds topic-validity handling in publish flow. |
| components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/APITemplateBuilderImpl.java | Injects external hub settings into the Velocity context for WebSub APIs. |
| components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java | Parses <WebSubHubConfig> from api-manager.xml into a new WebSubHubConfig DTO. |
| components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.java | Adds wildcard topic matching and extension-based topic matching hooks for WebSub. |
| components/apimgt/org.wso2.carbon.apimgt.common.gateway/src/main/java/org/wso2/carbon/apimgt/common/gateway/dto/ExtensionType.java | Adds WEBSUB_TOPIC_MATCHING extension type. |
Comments suppressed due to low confidence (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java:68
WebSubHubConfigis referenced/imported and instantiated here, but the DTO implementation is missing fromorg.wso2.carbon.apimgt.impl.dtoin this branch. This will fail compilation and blocks the configuration from loading.
import org.wso2.carbon.apimgt.impl.dto.RedisConfig;
import org.wso2.carbon.apimgt.impl.dto.TenantSharingConfigurationDTO;
import org.wso2.carbon.apimgt.impl.dto.SolaceConfig;
import org.wso2.carbon.apimgt.impl.dto.WebSubHubConfig;
import org.wso2.carbon.apimgt.impl.dto.ThrottleProperties;
import org.wso2.carbon.apimgt.impl.dto.TokenValidationDto;
import org.wso2.carbon.apimgt.impl.dto.WorkflowProperties;
import org.wso2.carbon.apimgt.impl.dto.ai.AIAPIConfigurationsDTO;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+588
to
+589
| } | ||
| webSubHubConfig.setEnabled(true); |
Comment on lines
78
to
82
| <class name="org.wso2.carbon.apimgt.gateway.mediators.webhooks.SubscriberInfoLoader"/> | ||
| <property name="TRANSPORT_HEADERS" action="remove" scope="axis2"/> | ||
| <property name="REST_URL_POSTFIX" scope="axis2" action="remove"/> | ||
| <property name="link" expression="$ctx:SUBSCRIBER_LINK_HEADER" scope="transport"/> | ||
| <header name="To" expression="$ctx:SUBSCRIBER_CALLBACK"/> | ||
| <filter source="boolean($ctx:SUBSCRIBER_SECRET)" regex="true"> |
Comment on lines
121
to
+124
| synCtx.setProperty(APIConstants.API_TYPE, APIConstants.API_TYPE_WEBSUB); | ||
| synCtx.setProperty(APIConstants.API_ELECTED_RESOURCE, | ||
| hubParameters.get(APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME)); | ||
|
|
||
| String providedTopic = hubParameters.get(APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME); | ||
| String electedResource = resolveElectedResource(synCtx, providedTopic); |
ashanhr
force-pushed
the
external-websubhub
branch
from
d48ba7d to
abe6836
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.java`:
- Around line 144-156: After calling
invokeExtensionListenerForTopicMatching(synCtx, providedTopic) check
isExtensionListenerAborted(synCtx) immediately and return false if true (i.e.,
honor the extension abort signal regardless of whether matchedTopic is
non-null), then proceed to use matchedTopic or call validateTopic(synCtx,
providedTopic) as before and set APIConstants.TOPIC_VALIDITY before calling
handleEventReceiver(synCtx); apply the identical change to the other call site
that mirrors lines 336-342 so the extension veto is honored in both places.
- Around line 123-124: hasMandatorySubscriptionParameters() uses the
configurable topicQueryParamName but the code here reads the topic using the
hard-coded APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME; this
can yield null and cause NPEs when comparing providedTopic to urlPattern.
Replace the hard-coded key with the configurable topicQueryParamName lookup on
hubParameters (the same variable used by hasMandatorySubscriptionParameters()),
and ensure resolveElectedResource(synCtx, ...) and subsequent exact-match checks
use that value (and null-safe comparisons) so providedTopic is read
consistently; apply the same change to the other occurrences around
resolveElectedResource and the blocks at lines referenced (188-200, 414-422).
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java`:
- Around line 583-588: In APIManagerConfiguration, the hub property values are
added directly from the XML (see webSubHubConfig.addHubProperty(...) inside the
properties iterator) without resolving vault-protected secrets; update the
property value extraction to pass the raw text through
MiscellaneousUtil.resolve(...) before calling webSubHubConfig.addHubProperty so
that any secure-vault placeholders are decrypted (mirror the approach used for
analytics and JMS properties elsewhere in this class).
In
`@components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/resources/repository/resources/api_templates/websub_api_template.xml`:
- Around line 139-153: The QUERY_PARAMS property concatenates raw values and
must URL-encode verification values to avoid corrupting the callback URI; update
the expression that builds QUERY_PARAMS to encode at least $ctx:SUBSCRIBER_TOPIC
and $ctx:HUB_CHALLENGE (and optionally
$ctx:SUBSCRIBER_LEASE_SECONDS/$ctx:SUBSCRIBER_MODE) using the XPath encoding
function (e.g. fn:encode-for-uri or equivalent supported function) so the
property named QUERY_PARAMS contains percent-encoded parameter values before
uri.var.callback is built from SUBSCRIBER_CALLBACK and QUERY_PARAMS.
- Around line 187-191: The template currently extracts ECHO_CHALLENGE using
json-eval($.text) which treats the echoed hub.challenge as JSON and causes the
HUB_CHALLENGE vs ECHO_CHALLENGE comparison to fail; update the <property
name="ECHO_CHALLENGE" .../> assignment to read the raw request body as plain
text (i.e., use the Synapse/raw-body property or get-property variant that
returns the raw payload) instead of json-eval so the filter comparing
get-property('HUB_CHALLENGE') to ECHO_CHALLENGE works correctly; keep the
surrounding ENABLE_SUBSCRIBER_VERIFICATION block and existing property names
(ECHO_CHALLENGE, HUB_CHALLENGE, VERIFICATION_SC) intact.
🪄 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: ff124225-7231-4c3c-9fc2-770ead0e435b
📒 Files selected for processing (7)
components/apimgt/org.wso2.carbon.apimgt.common.gateway/src/main/java/org/wso2/carbon/apimgt/common/gateway/dto/ExtensionType.javacomponents/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/APITemplateBuilderImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/resources/repository/resources/api_templates/websub_api_template.xmlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2features/apimgt/org.wso2.carbon.apimgt.gateway.feature/src/main/resources/p2.inf
Comment on lines
+123
to
+124
| String providedTopic = hubParameters.get(APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME); | ||
| String electedResource = resolveElectedResource(synCtx, providedTopic); |
There was a problem hiding this comment.
Use topicQueryParamName consistently when reading the requested topic.
hasMandatorySubscriptionParameters() honors the configurable topicQueryParamName, but these new reads hard-code the default constant again. In deployments that override the topic param name, providedTopic becomes null here and the exact-match branch later blows up on providedTopic.equals(urlPattern).
Proposed fix
- String providedTopic = hubParameters.get(APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME);
+ String providedTopic = hubParameters.get(topicQueryParamName);
@@
- if (APIConstants.Webhooks.TOPIC_QUERY_PARAM.equals(nvPair.getName())) {
+ if (topicQueryParamName.equals(nvPair.getName())) {
return nvPair.getValue();
}
@@
private String getMatchingTopic(String providedTopic, List<String> urlPatterns) {
- if (urlPatterns == null || urlPatterns.isEmpty()) {
+ if (StringUtils.isEmpty(providedTopic) || urlPatterns == null || urlPatterns.isEmpty()) {
return null;
}Also applies to: 188-200, 414-422
🤖 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.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.java`
around lines 123 - 124, hasMandatorySubscriptionParameters() uses the
configurable topicQueryParamName but the code here reads the topic using the
hard-coded APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME; this
can yield null and cause NPEs when comparing providedTopic to urlPattern.
Replace the hard-coded key with the configurable topicQueryParamName lookup on
hubParameters (the same variable used by hasMandatorySubscriptionParameters()),
and ensure resolveElectedResource(synCtx, ...) and subsequent exact-match checks
use that value (and null-safe comparisons) so providedTopic is read
consistently; apply the same change to the other occurrences around
resolveElectedResource and the blocks at lines referenced (188-200, 414-422).
Comment on lines
+144
to
+156
| String matchedTopic = invokeExtensionListenerForTopicMatching(synCtx, providedTopic); | ||
| if (matchedTopic == null && isExtensionListenerAborted(synCtx)) { | ||
| return false; | ||
| } | ||
|
|
||
| boolean isValidTopic; | ||
| if (matchedTopic != null) { | ||
| isValidTopic = true; | ||
| } else { | ||
| isValidTopic = validateTopic(synCtx, providedTopic); | ||
| } | ||
| synCtx.setProperty(APIConstants.TOPIC_VALIDITY, String.valueOf(isValidTopic)); | ||
| return handleEventReceiver(synCtx); |
There was a problem hiding this comment.
Honor the extension abort signal before using matchedTopic.
Both call sites only stop when WEBSUB_EXTENSION_ABORTED is set and matchedTopic is null. If an extension resolves a match and then vetoes the request by returning continueFlow=false, the handler still accepts it, which makes the new abort path ineffective.
Proposed fix
String matchedTopic = invokeExtensionListenerForTopicMatching(synCtx, providedTopic);
- if (matchedTopic == null && isExtensionListenerAborted(synCtx)) {
+ if (isExtensionListenerAborted(synCtx)) {
return false;
}
@@
String matchedTopic = invokeExtensionListenerForTopicMatching(synCtx, providedTopic, urlPatterns);
- if (matchedTopic == null && isExtensionListenerAborted(synCtx)) {
+ if (isExtensionListenerAborted(synCtx)) {
return null;
}Also applies to: 336-342
🤖 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.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.java`
around lines 144 - 156, After calling
invokeExtensionListenerForTopicMatching(synCtx, providedTopic) check
isExtensionListenerAborted(synCtx) immediately and return false if true (i.e.,
honor the extension abort signal regardless of whether matchedTopic is
non-null), then proceed to use matchedTopic or call validateTopic(synCtx,
providedTopic) as before and set APIConstants.TOPIC_VALIDITY before calling
handleEventReceiver(synCtx); apply the identical change to the other call site
that mirrors lines 336-342 so the extension veto is honored in both places.
Comment on lines
+583
to
+588
| Iterator<OMElement> properties = hubProperties.getChildElements(); | ||
| while (properties.hasNext()) { | ||
| OMElement propertyNode = properties.next(); | ||
| webSubHubConfig.addHubProperty(propertyNode.getLocalName(), propertyNode.getText()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Resolve secrets for hub properties.
Hub properties are added directly from XML text without resolving secrets from the secure vault. If WebSubHubProperties contains sensitive values (e.g., authentication tokens, API keys, passwords), they will not be decrypted.
Other similar property iterators in this file use MiscellaneousUtil.resolve() to handle vault-protected secrets (e.g., analytics properties at line 448, JMS connection parameters at line 1933, gateway additional properties at line 2911).
🔒 Proposed fix to resolve secrets
if (hubProperties != null) {
Iterator<OMElement> properties = hubProperties.getChildElements();
while (properties.hasNext()) {
OMElement propertyNode = properties.next();
- webSubHubConfig.addHubProperty(propertyNode.getLocalName(), propertyNode.getText());
+ String value = MiscellaneousUtil.resolve(propertyNode, secretResolver);
+ webSubHubConfig.addHubProperty(propertyNode.getLocalName(), value);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Iterator<OMElement> properties = hubProperties.getChildElements(); | |
| while (properties.hasNext()) { | |
| OMElement propertyNode = properties.next(); | |
| webSubHubConfig.addHubProperty(propertyNode.getLocalName(), propertyNode.getText()); | |
| } | |
| } | |
| Iterator<OMElement> properties = hubProperties.getChildElements(); | |
| while (properties.hasNext()) { | |
| OMElement propertyNode = properties.next(); | |
| String value = MiscellaneousUtil.resolve(propertyNode, secretResolver); | |
| webSubHubConfig.addHubProperty(propertyNode.getLocalName(), value); | |
| } | |
| } |
🤖 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.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java`
around lines 583 - 588, In APIManagerConfiguration, the hub property values are
added directly from the XML (see webSubHubConfig.addHubProperty(...) inside the
properties iterator) without resolving vault-protected secrets; update the
property value extraction to pass the raw text through
MiscellaneousUtil.resolve(...) before calling webSubHubConfig.addHubProperty so
that any secure-vault placeholders are decrypted (mirror the approach used for
analytics and JMS properties elsewhere in this class).
Comment on lines
+139
to
+153
| <property name="QUERY_PARAMS" | ||
| expression="fn:concat('hub.mode=', $ctx:SUBSCRIBER_MODE, '&','hub.topic=',$ctx:SUBSCRIBER_TOPIC, '&','hub.challenge=', $ctx:HUB_CHALLENGE, '&','hub.lease_seconds=', $ctx:SUBSCRIBER_LEASE_SECONDS)" | ||
| scope="default" type="STRING"/> | ||
| <property name="SUBSCRIBER_APPLICATION_ID" expression="$ctx:api.ut.application.id"/> | ||
|
|
||
| <filter source="fn:contains($ctx:SUBSCRIBER_CALLBACK, '?')" regex="true"> | ||
| <then> | ||
| <property name="uri.var.callback" | ||
| expression="fn:concat($ctx:SUBSCRIBER_CALLBACK,'&', $ctx:QUERY_PARAMS)" | ||
| scope="default" type="STRING"/> | ||
| </then> | ||
| <else> | ||
| <property name="uri.var.callback" | ||
| expression="fn:concat($ctx:SUBSCRIBER_CALLBACK,'?', $ctx:QUERY_PARAMS)" | ||
| scope="default" type="STRING"/> |
There was a problem hiding this comment.
URL-encode the verification query values.
hub.topic is itself a URL. Concatenating it raw into QUERY_PARAMS corrupts the callback URI as soon as the topic or the existing callback already contains ?, &, = or other reserved characters, so the subscriber receives different verification parameters from the original subscription request.
🤖 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/test/resources/repository/resources/api_templates/websub_api_template.xml`
around lines 139 - 153, The QUERY_PARAMS property concatenates raw values and
must URL-encode verification values to avoid corrupting the callback URI; update
the expression that builds QUERY_PARAMS to encode at least $ctx:SUBSCRIBER_TOPIC
and $ctx:HUB_CHALLENGE (and optionally
$ctx:SUBSCRIBER_LEASE_SECONDS/$ctx:SUBSCRIBER_MODE) using the XPath encoding
function (e.g. fn:encode-for-uri or equivalent supported function) so the
property named QUERY_PARAMS contains percent-encoded parameter values before
uri.var.callback is built from SUBSCRIBER_CALLBACK and QUERY_PARAMS.
Comment on lines
+187
to
+191
| <property name="VERIFICATION_SC" expression="$axis2:HTTP_SC"/> | ||
|
|
||
| #if($enableSubscriberVerification) | ||
| <property name="ECHO_CHALLENGE" expression="json-eval($.text)"/> | ||
| <filter source="get-property('HUB_CHALLENGE') = get-property('ECHO_CHALLENGE')" regex="true"> |
There was a problem hiding this comment.
Fix WebSub challenge verification: treat the echoed callback body as plain text, not JSON.
WebSub verification callbacks echo hub.challenge as the raw response body; this template sets messageType to application/xml and then derives ECHO_CHALLENGE via json-eval($.text), so the comparison against HUB_CHALLENGE will fail and route successful verifications to the failure path.
🤖 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/test/resources/repository/resources/api_templates/websub_api_template.xml`
around lines 187 - 191, The template currently extracts ECHO_CHALLENGE using
json-eval($.text) which treats the echoed hub.challenge as JSON and causes the
HUB_CHALLENGE vs ECHO_CHALLENGE comparison to fail; update the <property
name="ECHO_CHALLENGE" .../> assignment to read the raw request body as plain
text (i.e., use the Synapse/raw-body property or get-property variant that
returns the raw payload) instead of json-eval so the filter comparing
get-property('HUB_CHALLENGE') to ECHO_CHALLENGE works correctly; keep the
surrounding ENABLE_SUBSCRIBER_VERIFICATION block and existing property names
(ECHO_CHALLENGE, HUB_CHALLENGE, VERIFICATION_SC) intact.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.java (2)
144-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor the extension abort signal before using
matchedTopic.If a topic resolver extension returns
continueFlow=falseand also suppliesWEBSUB_MATCHED_TOPIC, both call sites still continue. That makes the new veto path ineffective.Proposed fix
String matchedTopic = invokeExtensionListenerForTopicMatching(synCtx, providedTopic); - if (matchedTopic == null && isExtensionListenerAborted(synCtx)) { + if (isExtensionListenerAborted(synCtx)) { return false; } @@ String matchedTopic = invokeExtensionListenerForTopicMatching(synCtx, providedTopic, urlPatterns); - if (matchedTopic == null && isExtensionListenerAborted(synCtx)) { + if (isExtensionListenerAborted(synCtx)) { return null; }Also applies to: 336-342
🤖 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.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.java` around lines 144 - 146, The handler currently reads invokeExtensionListenerForTopicMatching(synCtx, providedTopic) and uses its returned matchedTopic without first honoring the extension veto; ensure you check isExtensionListenerAborted(synCtx) immediately after invoking invokeExtensionListenerForTopicMatching and return false if it is true so any WEBSUB_MATCHED_TOPIC set by a continueFlow=false extension is ignored; apply the same change to the second call site (the later block that also calls invokeExtensionListenerForTopicMatching and then uses matchedTopic) so both paths honor the abort signal.
123-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
topicQueryParamNameconsistently when reading the requested topic.
hasMandatorySubscriptionParameters()already honors the configurable topic param, but these paths still hard-code the default key. In deployments that overridetopicQueryParamName, subscription resolution will miss the topic, and Line 421 can still throw onprovidedTopic.equals(...).Proposed fix
- String providedTopic = hubParameters.get(APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME); + String providedTopic = hubParameters.get(topicQueryParamName); @@ - if (APIConstants.Webhooks.TOPIC_QUERY_PARAM.equals(nvPair.getName())) { + if (topicQueryParamName.equals(nvPair.getName())) { return nvPair.getValue(); } @@ - if (urlPatterns == null || urlPatterns.isEmpty()) { + if (StringUtils.isEmpty(providedTopic) || urlPatterns == null || urlPatterns.isEmpty()) { return null; }Also applies to: 188-200, 414-422
🤖 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.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.java` around lines 123 - 124, The code reads the requested topic using the hard-coded APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME and then calls resolveElectedResource(synCtx, providedTopic), which breaks when deployments override the configurable topicQueryParamName; update all reads (e.g., the providedTopic assignment and similar occurrences around the cited blocks) to use the configured topicQueryParamName value that hasMandatorySubscriptionParameters() already checks, and make the call null-safe (avoid providedTopic.equals(...) — either check providedTopic for null first or use a null-safe comparison) so subscription resolution cannot miss the topic or throw NPEs.
🤖 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
`@features/apimgt/org.wso2.carbon.apimgt.gateway.feature/src/main/resources/externalWebSubHubCaller.xml`:
- Around line 19-28: The normalization currently strips a leading '/' into
SUBSCRIBER_TOPIC_NORMALIZED but later code branches reintroduce the leading
slash causing '/topic' and 'topic' to diverge; update all occurrences that build
topics (including the filter blocks around SUBSCRIBER_TOPIC_NORMALIZED and the
similar blocks at the other locations called out) to always use
SUBSCRIBER_TOPIC_NORMALIZED directly without prefixing '/' again, and remove any
logic that concatenates '/' + get-property('SUBSCRIBER_TOPIC') or similar so the
normalized value is the single source of truth for topic comparisons in
subscribe/unsubscribe/publish flows.
---
Duplicate comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.java`:
- Around line 144-146: The handler currently reads
invokeExtensionListenerForTopicMatching(synCtx, providedTopic) and uses its
returned matchedTopic without first honoring the extension veto; ensure you
check isExtensionListenerAborted(synCtx) immediately after invoking
invokeExtensionListenerForTopicMatching and return false if it is true so any
WEBSUB_MATCHED_TOPIC set by a continueFlow=false extension is ignored; apply the
same change to the second call site (the later block that also calls
invokeExtensionListenerForTopicMatching and then uses matchedTopic) so both
paths honor the abort signal.
- Around line 123-124: The code reads the requested topic using the hard-coded
APIConstants.WebHookProperties.DEFAULT_TOPIC_QUERY_PARAM_NAME and then calls
resolveElectedResource(synCtx, providedTopic), which breaks when deployments
override the configurable topicQueryParamName; update all reads (e.g., the
providedTopic assignment and similar occurrences around the cited blocks) to use
the configured topicQueryParamName value that
hasMandatorySubscriptionParameters() already checks, and make the call null-safe
(avoid providedTopic.equals(...) — either check providedTopic for null first or
use a null-safe comparison) so subscription resolution cannot miss the topic or
throw NPEs.
🪄 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: b4e3c99d-a512-460b-9a1c-2eb7b7c2e066
📒 Files selected for processing (9)
components/apimgt/org.wso2.carbon.apimgt.common.gateway/src/main/java/org/wso2/carbon/apimgt/common/gateway/dto/ExtensionType.javacomponents/apimgt/org.wso2.carbon.apimgt.gateway/src/main/java/org/wso2/carbon/apimgt/gateway/handlers/streaming/webhook/WebhookApiHandler.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.javacomponents/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/dto/WebSubHubConfig.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/APITemplateBuilderImpl.javacomponents/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/resources/repository/resources/api_templates/websub_api_template.xmlfeatures/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2features/apimgt/org.wso2.carbon.apimgt.gateway.feature/src/main/resources/externalWebSubHubCaller.xmlfeatures/apimgt/org.wso2.carbon.apimgt.gateway.feature/src/main/resources/p2.inf
🚧 Files skipped from review as they are similar to previous changes (6)
- features/apimgt/org.wso2.carbon.apimgt.core.feature/src/main/resources/conf_templates/templates/repository/conf/api-manager.xml.j2
- components/apimgt/org.wso2.carbon.apimgt.common.gateway/src/main/java/org/wso2/carbon/apimgt/common/gateway/dto/ExtensionType.java
- features/apimgt/org.wso2.carbon.apimgt.gateway.feature/src/main/resources/p2.inf
- components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/main/java/org/wso2/carbon/apimgt/rest/api/publisher/v1/common/template/APITemplateBuilderImpl.java
- components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java
- components/apimgt/org.wso2.carbon.apimgt.rest.api.publisher.v1.common/src/test/resources/repository/resources/api_templates/websub_api_template.xml
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
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.
Purpose
Introduce support for routing WebSub subscribe/unsubscribe/publish operations
to an external WebSub Hub (e.g. Ballerina WebSub Hub), and refactor
WebhookApiHandler to support wildcard topic matching via custom extension
handlers.
External hub integration:
hubProperties fields; parse from in api-manager.xml
registration (subscribe), subscription, unsubscription and publish against
an external hub; prefixes topics with the API context to avoid cross-API
collisions; installed to synapse-configs/default/sequences/ via p2.inf
subscribe, unsubscribe and publish when useExternalWebSubHub is set
Wildcard topic matching:
match subscriber topics against registered URI template patterns using
URITemplateHelper, enabling wildcard topic support (e.g. /orders/*)
so custom handlers can override topic matching logic
Related issue: wso2/api-manager#4948