Fix Event Gateway WebSub consumer group ID collisions and return 401 for policy denials#1858
Fix Event Gateway WebSub consumer group ID collisions and return 401 for policy denials#1858AnujaKalahara99 wants to merge 3 commits intowso2:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummaryThis PR addresses two runtime issues in the Event Gateway WebSub handler:
Changesevent-gateway/gateway-runtime/internal/connectors/receiver/websub/handler.go
event-gateway/gateway-runtime/internal/connectors/receiver/websub/consumer_manager.go
TestingUnit tests verified for WalkthroughThe changes modify the WebSub connector within the event gateway runtime. The consumer group ID generation logic increases the SHA-256 hash slice from 16 characters to 32 characters while preserving the existing naming format. Additionally, three policy short-circuit response handlers are updated to return HTTP 401 Unauthorized status instead of HTTP 403 Forbidden, with corresponding status text changes in the hub subscription handler, unsubscription handler, and webhook receiver handler. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![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
🧹 Nitpick comments (1)
event-gateway/gateway-runtime/internal/connectors/receiver/websub/consumer_manager.go (1)
221-226: Consumer group ID derivation is correct; note offset continuity impact on rollout.The increase from 16 to 32 hex characters raises effective hash entropy from 64 to 128 bits — a correct, low-risk change. No truncation or encoding issues; the resulting ID length is well within Kafka's limit.
One operational consideration: all existing callback URLs will get a new consumer group ID on deployment. Kafka treats this as a fresh group with no committed offsets; depending on
auto.offset.reset, in-flight subscriptions will either replay from the earliest offset or skip to latest. If zero-skip or zero-replay is required, a migration or offset transfer should be planned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@event-gateway/gateway-runtime/internal/connectors/receiver/websub/consumer_manager.go` around lines 221 - 226, The consumerGroupID change in consumerGroupID(callbackURL) increases the hex truncation from 16 to 32 characters, which will produce new Kafka consumer group IDs (cm.groupPrefix + "-websub-" + hex(...)) for all existing callback URLs and therefore will be treated as new groups with no committed offsets; to fix operational risk either revert to the prior 16-char truncation or implement a migration: add a migration/tool to map oldGroupID -> newGroupID (compute both hashes from callbackURL) and copy committed offsets in Kafka before rollout, and also update release notes and mention auto.offset.reset behavior (earliest/latest) so operators can plan rollback or offset-transfer steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@event-gateway/gateway-runtime/internal/connectors/receiver/websub/handler.go`:
- Around line 108-111: The code returns http.StatusUnauthorized unconditionally
for policy denials and drops the policy-provided status/message; update the
handlers that call writePolicyResponse (the short-circuit branches and other
policy-rejection sites) to capture and use the policy engine's status and
message instead of discarding them, map authentication failures to 401 and
authorization (policy) denials to 403, and modify writePolicyResponse to set a
proper WWW-Authenticate header when sending a 401 response (and only fall back
to http.Error when no message is available), ensuring the response status comes
from the captured policy status or the corrected 401/403 mapping; refer to
writePolicyResponse and the short-circuit handling locations in the handler
functions to implement these changes.
---
Nitpick comments:
In
`@event-gateway/gateway-runtime/internal/connectors/receiver/websub/consumer_manager.go`:
- Around line 221-226: The consumerGroupID change in
consumerGroupID(callbackURL) increases the hex truncation from 16 to 32
characters, which will produce new Kafka consumer group IDs (cm.groupPrefix +
"-websub-" + hex(...)) for all existing callback URLs and therefore will be
treated as new groups with no committed offsets; to fix operational risk either
revert to the prior 16-char truncation or implement a migration: add a
migration/tool to map oldGroupID -> newGroupID (compute both hashes from
callbackURL) and copy committed offsets in Kafka before rollout, and also update
release notes and mention auto.offset.reset behavior (earliest/latest) so
operators can plan rollback or offset-transfer steps.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b464ca3-eb48-4ae0-a4a5-4438d43f85eb
📒 Files selected for processing (2)
event-gateway/gateway-runtime/internal/connectors/receiver/websub/consumer_manager.goevent-gateway/gateway-runtime/internal/connectors/receiver/websub/handler.go
AnujaKalahara99
force-pushed
the
egw/fix-consumer-grp
branch
from
28c36c4 to
e02e1ca
Compare
1 participant
Purpose
WebSub runtime requests need consistent auth failure semantics and safer consumer-group identifiers. This change fixes two issues in event-gateway: policy-denied WebSub requests were returning 403 instead of 401, and callback-based
consumer group IDs were using a shorter hash suffix that increases collision risk across subscriptions. Resolves N/A.
Goals
Return 401 Unauthorized for WebSub policy short-circuit responses.
Reduce the risk of consumer group ID collisions for WebSub callback consumers.
Approach
Updated the WebSub receiver fallback policy response in event-gateway/gateway-runtime/internal/connectors/receiver/websub/handler.go to use 401 Unauthorized for subscribe, unsubscribe, and inbound webhook policy denials.
Updated event-gateway/gateway-runtime/internal/connectors/receiver/websub/consumer_manager.go to use a longer SHA-256 prefix when deriving callback-based consumer group IDs.
User stories
As a client calling WebSub endpoints, I get a 401 response when access is denied by policy.
As a runtime managing callback consumers, I get more stable and collision-resistant consumer group IDs.
Documentation
N/A. This is a small runtime behavior fix and does not introduce new user-facing configuration or APIs.
Automation tests
GOCACHE=/tmp/go-build-cache go test ./internal/connectors/receiver/websub
N/A. No integration test changes were added in this branch.
Security checks
Samples
N/A. No sample changes.
Related PRs
N/A
Test environment
Ubuntu 24.04.4 LTS, Go test run in local dev environment with GOCACHE=/tmp/go-build-cache.