[Bug][Backend][KeycloakEvents] Queue binds routing key '#' on amq.topic — subscribes to EVERY message on the exchange; a landmine for any future amq.topic publisher #569

New Issue

2026-07-14T14:28:53Z

spikerj commented

2026-07-14 14:28:53 +00:00

QA Team — filed 2026-07-14 from the README audit, at spikerj's direction (decision: narrow the binding).

The bug: SpikerSoft.EventHandlers.KeycloakEvents declares queue keycloak.events (durable) and binds it to the built-in amq.topic exchange with routing key # — the match-everything wildcard. Any message ever published to amq.topic by anything, for any reason, lands in the Keycloak audit queue and is parsed as a Keycloak identity event.

Why it's currently invisible: Keycloak's event plugin is the only publisher on amq.topic today, so the audit trail is clean and the handler logs zero errors (verified 2026-07-14). This is a latent defect, not an active one — which is exactly why it should be fixed before someone reaches for amq.topic (the obvious "default topic exchange" choice) for an unrelated feature and silently poisons the identity audit log with foreign messages.

Fix shape: bind the specific Keycloak routing-key pattern instead of # (Keycloak's plugin publishes with keys of the form KK.EVENT.CLIENT.<realm>.<clientId>.<...> / KK.EVENT.ADMIN.<realm>... — confirm against the deployed plugin config), or move the handler onto its own dedicated exchange rather than sharing amq.topic at all (cleanest — the exchange is otherwise a free-for-all namespace).

Also worth doing while in there: the handler's index creation is fire-and-forget in the service constructor with a code comment saying it "should move to a startup/migration task" — that comment has no ticket either.

**QA Team** — filed 2026-07-14 from the README audit, at spikerj's direction (decision: narrow the binding). **The bug:** `SpikerSoft.EventHandlers.KeycloakEvents` declares queue `keycloak.events` (durable) and binds it to the built-in **`amq.topic`** exchange with routing key **`#`** — the match-everything wildcard. Any message ever published to `amq.topic` by anything, for any reason, lands in the Keycloak audit queue and is parsed as a Keycloak identity event. **Why it's currently invisible:** Keycloak's event plugin is the only publisher on `amq.topic` today, so the audit trail is clean and the handler logs zero errors (verified 2026-07-14). This is a **latent** defect, not an active one — which is exactly why it should be fixed before someone reaches for `amq.topic` (the obvious "default topic exchange" choice) for an unrelated feature and silently poisons the identity audit log with foreign messages. **Fix shape:** bind the specific Keycloak routing-key pattern instead of `#` (Keycloak's plugin publishes with keys of the form `KK.EVENT.CLIENT.<realm>.<clientId>.<...>` / `KK.EVENT.ADMIN.<realm>...` — confirm against the deployed plugin config), or move the handler onto its own dedicated exchange rather than sharing `amq.topic` at all (cleanest — the exchange is otherwise a free-for-all namespace). **Also worth doing while in there:** the handler's index creation is fire-and-forget in the service constructor with a code comment saying it "should move to a startup/migration task" — that comment has no ticket either.

spikerj commented

2026-07-14 21:20:20 +00:00

Fix in spikersoft-backend PR #289 (open, awaiting merge).

Confirmed exactly as filed. KeycloakEventHostedService.cs:120 bound the queue with ConfigureQueueWithDlqAsync(_channel, "amq.topic", "#", …).

Routing keys confirmed against the deployed plugin rather than assumed, as you asked: keycloak-to-rabbit 3.0.5, and there is no KK_TO_RMQ_EXCHANGE override on the keycloak_keycloak service — so the SPI publishes to its default exchange, amq.topic, which is why the handler binds there at all. The binding is now KK.EVENT.#, covering both shapes the SPI emits (KK.EVENT.CLIENT.<realm>.<SUCCESS|ERROR>.<clientId>.<eventType> and KK.EVENT.ADMIN.<realm>.<SUCCESS|ERROR>.<resourceType>.<operationType>).

I took the "narrow the binding" option rather than moving to a dedicated exchange, because the exchange is the SPI's choice, not ours — moving off amq.topic would mean setting KK_TO_RMQ_EXCHANGE in the Keycloak stack and coordinating a cutover across two repos. Worth doing if you want the cleaner namespace, but it's an infra change with a deploy-ordering hazard, and it isn't needed to close the hole.

The part that would have bitten us

RabbitMQ bindings are additive. queue.bind adds a binding — it does not replace the queue's existing ones. keycloak.events is durable and already deployed carrying its # binding, so just changing the routing key in code would have added KK.EVENT.# alongside #. The queue would have gone on receiving every message on the exchange, and the fix would have been a no-op in production while reading as correct in review.

So the fix issues an explicit queue.unbind for # (idempotent, so fresh brokers are fine), and binds the narrow key before dropping the catch-all so there's no instant where the queue is bound to nothing and a real event could be dropped.

I verified this is load-bearing rather than theoretical: simulating the naive fix (routing key changed, unbind removed) fails the foreign-message and legacy-binding tests while the "real Keycloak events still route" cases stay green.

Tests

5 new, against a real broker (Testcontainers), starting from the state a deployed broker is actually in (queue already carrying #) — because the migration is the part that has to work. Real broker on purpose: topic-matching and bind-is-additive are properties RabbitMQ decides, and a mock would only have asserted my beliefs about them.

Build clean; SpikerSoft.Business.Tests 7515 passed / 0 failed.

Your second item — not done, deliberately

The fire-and-forget index creation in KeycloakEventService's constructor is untouched. It's a real smell, but it's a startup/migration concern rather than a routing one, and folding it into a queue-topology PR would have mixed two unrelated changes. Happy to take it — say the word and I'll file it.

Fix in spikersoft-backend PR #289 (open, awaiting merge). Confirmed exactly as filed. `KeycloakEventHostedService.cs:120` bound the queue with `ConfigureQueueWithDlqAsync(_channel, "amq.topic", "#", …)`. **Routing keys confirmed against the deployed plugin rather than assumed**, as you asked: keycloak-to-rabbit 3.0.5, and there is **no `KK_TO_RMQ_EXCHANGE` override** on the `keycloak_keycloak` service — so the SPI publishes to its default exchange, `amq.topic`, which is why the handler binds there at all. The binding is now `KK.EVENT.#`, covering both shapes the SPI emits (`KK.EVENT.CLIENT.<realm>.<SUCCESS|ERROR>.<clientId>.<eventType>` and `KK.EVENT.ADMIN.<realm>.<SUCCESS|ERROR>.<resourceType>.<operationType>`). I took the "narrow the binding" option rather than moving to a dedicated exchange, because the exchange is the SPI's choice, not ours — moving off `amq.topic` would mean setting `KK_TO_RMQ_EXCHANGE` in the Keycloak stack and coordinating a cutover across two repos. Worth doing if you want the cleaner namespace, but it's an infra change with a deploy-ordering hazard, and it isn't needed to close the hole. ## The part that would have bitten us **RabbitMQ bindings are additive.** `queue.bind` *adds* a binding — it does not replace the queue's existing ones. `keycloak.events` is durable and already deployed **carrying its `#` binding**, so just changing the routing key in code would have added `KK.EVENT.#` *alongside* `#`. The queue would have gone on receiving every message on the exchange, and **the fix would have been a no-op in production while reading as correct in review.** So the fix issues an explicit `queue.unbind` for `#` (idempotent, so fresh brokers are fine), and binds the narrow key *before* dropping the catch-all so there's no instant where the queue is bound to nothing and a real event could be dropped. I verified this is load-bearing rather than theoretical: simulating the naive fix (routing key changed, unbind removed) fails the foreign-message and legacy-binding tests while the "real Keycloak events still route" cases stay green. ## Tests 5 new, against a **real broker** (Testcontainers), starting from the state a deployed broker is actually in (queue already carrying `#`) — because the migration is the part that has to work. Real broker on purpose: topic-matching and bind-is-additive are properties **RabbitMQ** decides, and a mock would only have asserted my beliefs about them. Build clean; `SpikerSoft.Business.Tests` 7515 passed / 0 failed. ## Your second item — not done, deliberately The fire-and-forget index creation in `KeycloakEventService`'s constructor is untouched. It's a real smell, but it's a startup/migration concern rather than a routing one, and folding it into a queue-topology PR would have mixed two unrelated changes. Happy to take it — say the word and I'll file it.

spikerj commented

2026-07-14 21:28:34 +00:00

Merged to master in spikersoft-backend PR #289. The keycloak.events queue now binds KK.EVENT.# and the legacy catch-all # binding is explicitly unbound on startup, so foreign messages on the shared amq.topic exchange are no longer ingested as identity events.

The unbind takes effect the next time the worker starts and re-declares its topology.

Closing. The secondary item (fire-and-forget index creation in KeycloakEventService's constructor) is untouched and still unticketed — say the word and I'll file it.

Merged to `master` in spikersoft-backend PR #289. The `keycloak.events` queue now binds `KK.EVENT.#` and the legacy catch-all `#` binding is explicitly unbound on startup, so foreign messages on the shared `amq.topic` exchange are no longer ingested as identity events. The unbind takes effect the next time the worker starts and re-declares its topology. Closing. The secondary item (fire-and-forget index creation in `KeycloakEventService`'s constructor) is untouched and still unticketed — say the word and I'll file it.

spikerj closed this issue

2026-07-14 21:28:37 +00:00

spikerj referenced this issue

2026-07-14 21:35:20 +00:00

[Bug][CI][Deploy] cancel-in-progress on master kills DEPLOY jobs — back-to-back merges cancel each other and fixes silently never ship #590

spikerj referenced this issue

2026-07-14 21:36:03 +00:00

[Bug][CI][Deploy] cancel-in-progress on master kills DEPLOY jobs — back-to-back merges cancel each other and fixes silently never ship #590

spikerj referenced this issue

2026-07-24 19:27:40 +00:00

space-game KeybindingManager ctor wipes saved localStorage keybindings on every boot (custom binds lost on reload) #826

spikerj referenced this issue