villebro opened a new pull request, #43696:
URL: https://github.com/apache/superset/pull/43696
### SUMMARY
An external reviewer audited the realtime/websocket work already merged onto
`gaq-to-gtf` (at `933da58917`, which includes the per-tab task-status fanout
in
#43689) and filed 8 findings, ranging from concrete correctness bugs to
hardening,
a wire-protocol design concern, and docs. This PR addresses all of them.
**Why this matters.** The GAQ→GTF cutover introduced a two-mechanism
transport:
a cursor-based REST poll that is the *correctness* path, and a best-effort
websocket that *accelerates* it. The per-tab work (#43689) then made async
chart-data cancellation and task-status delivery tab-scoped. The review found
places where that per-tab model was only half-wired (a producer that didn't
send
the tab id, a cancel that read the wrong tab id), an unbounded
client-controlled
value flowing into routing keys, a future footgun in the extension point that
lets a task type route its own status messages, a passive-surface gap where
the
socket dies at JWT expiry with no refresh, and a websocket envelope that
conflated
*what* a message is with *who* receives it — which would force every future
realtime surface (notifications, report/export progress) to keep inventing
channel
strings and overloading payload shapes. Left alone these are latent
correctness,
hardening, and maintainability problems as more features ride the transport.
The invariant preserved throughout: **the websocket stays a best-effort
acceleration layer; the REST poll and each feature's authorized fetch remain
the
correctness path.** No change here makes the socket load-bearing.
#### Changes by finding
**Correctness**
- **#1 — `StatefulChart` bypassed the per-tab model.** Its async chart-data
POST
sent `async_mode` but no `tab_id`, so its tasks were tracked
principal-grain and
a cancel could abort work another tab awaited. `StatefulChart` lives in
`@superset-ui/core` and can't import the app's tab-id hook, so `getTabId`
is now
injected through the existing chart `Hooks` channel (like
`resolveAsyncMode`),
wired in `ChartRenderer`, and sent on the async payload — matching the
Redux
chart path.
- **#2 — cancel read the current tab id, not the submit-time one.** A
duplicate-tab
id collision can reassign this tab's id between submit and cancel,
orphaning the
original per-tab subscription. `asyncEvent` now captures the tab id once at
submit and threads it through cancel. (The websocket
reconnect-on-reassignment
is intentionally left alone: routing must follow the *new* id, while
submit/cancel
pairing must follow the id used at submit — two separate concerns.)
**Hardening**
- **#4 — `tab_id` was unbounded and unvalidated.** A client-supplied string
flowed
into routing keys, private task props, Redis channels, logs, and URLs. One
shared
rule (≤64 chars, `[A-Za-z0-9_-]`) is now enforced at both trust
boundaries: the
Flask ingress (`get_request_tab_id`) and the Node upgrade (`httpUpgrade`).
- **#5 — task subscription policies returned raw routing strings forwarded
verbatim.** A policy bug could route a task's status to another principal.
`TaskManager.publish_task_status` now validates every policy-returned key
against
the task's own subscriber principals (equal to, or a per-client suffix of,
an
authorized principal), dropping the rest and falling back to
principal-grain if
none survive. The public extension-point contract is unchanged.
**Design (discussed and agreed before implementing)**
- **#3 — semantic topic vs. recipient route were conflated.** The browser
envelope
was `{channel, payload}` where the client dispatched by parsing a routing
string
(`realtime:user:5:tabA`), leaking the server-side route to the browser.
The two
prior Redis channels (`entity-changes:*`, `task-status`) are now a single
`realtime` channel carrying a self-describing envelope:
`{ topic, scope, routes?, payload }` — `topic` is the semantic stream
(`task.status`, `entity.changed`, future `notification.*`), `scope` is the
delivery breadth (`authenticated_global` broadcast vs. `principal`/`tab`
targeted), and `routes` are server-side only and **never** forwarded to the
browser. The browser now dispatches purely on `topic`
(`subscribeRealtime(topic, handler)`), so a new surface adds a topic
instead of a
channel-string prefix. This is a coordinated Flask + Node + frontend wire
change,
which the epic already deploys together (a version mismatch degrades to
polling,
never crashes).
- **#6 — websocket JWT had no proactive refresh.** The token has a fixed
15-min
life and the Node server terminates the socket at expiry; the cookie was
only
re-minted *after* it had expired. An active surface survived via poll/HTTP
traffic, but an idle-but-open realtime list view (no nudges for 15 min)
would
silently lose its socket. Two-part sliding session: the Flask
`after_request`
hook re-mints the cookie when a request lands in the second half of the
token's
life, and the browser client proactively refreshes the cookie
(`GET /api/v1/me/`, which the hook re-mints on) and reconnects at
half-life — so
the surface stays connected while revocation stays bounded by the same
lifetime.
**Docs**
- **#7** — README and `SECURITY.md` now state the broadcast scope is
**authenticated-global**, not public: anonymous requests get no realtime
principal, no cookie, and no socket. True anonymous/Public-role realtime is
explicitly out of scope.
- **#8** — README corrected: `task.status` is published on **terminal**
completion
only; intermediate transitions ride the `entity.changed` broadcast for list
views.
### TESTING INSTRUCTIONS
- Node: `cd superset-websocket && NODE_ENV=test npx vitest --run --dir spec`
(78 pass — envelope routing by scope, tab_id validation, dedup, broadcast).
- Python: `pytest tests/unit_tests/tasks tests/unit_tests/websocket
tests/unit_tests/charts/test_chart_data_api.py` (new `test_subscription.py` for
tab-id validation; `test_manager.py` routing-validation + envelope;
`test_channel.py` sliding-window re-mint).
- Frontend: `npm run test -- realtime asyncEvent StatefulChart ChartRenderer
hooks` (topic dispatch, keepalive, submit-time tab-id capture,
tab_id-in-payload).
- Manual (per the reviewer's handoff): two tabs submitting/cancelling the
same shared chart independently; a tab-id collision mid-flight; a user-level
entity nudge reaching all tabs without bleed.
Pre-commit ruff/mypy/pylint/oxfmt/oxlint are green on the changed files. The
frontend type-check hook has pre-existing failures on `gaq-to-gtf`
(`ListView.tsx`
and four others, unrelated to this PR).
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
> Note: one open call — the JWT refresh (#6) reuses `GET /api/v1/me/` to
trigger the cookie re-mint rather than adding a dedicated endpoint (less
surface; any authed response re-mints via `after_request`). Happy to switch to
a purpose-built endpoint if preferred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
