Repository navigation
fix(socket): validate credentials on push-feed WebSocket handshakes under all configurations - #36143
Conversation
…ions Socket handshakes on the push-feed endpoint are now always authenticated: a presented token is validated via the authorizer/authentication configuration, a session cookie is validated against the session store, and a handshake carrying neither is rejected. Previously, token validation was gated on the authorizer's secure-socket flag, so under the default configuration a handshake with no credential could bind to any user id it presented. The socket identity and session no longer fall back to raw query params in the connection handler, and the enableSecureSocketConnection authorizer setting is now ignored and marked deprecated in the schema and sample configuration.
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
Ingestion
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
… rule - Browsers cannot set headers on a WebSocket upgrade, so a bearer token may be presented on the push-feed handshake via a token query parameter; SocketAddressFilter validates it like a header token. Public-client SSO sessions (IdP token, no session cookie) need this to keep working. - Externally-issued tokens carry no session claim and are accepted on token validation alone; internally-issued tokens revalidate against the session store. - The handshake servlet no longer logs the query string. - The UI passes its stored token on the socket.io handshake query. - enableSecureSocketConnection is marked deprecated: true in the schema and the committed TS/dereferenced outputs are regenerated. - ADR:2026-10-26-push-feed-handshakes-always-authenticate records the rule.
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
The Jetty (ee10) upgrade negotiation runs before the servlet filter chain, so SocketAddressFilter's validation and header stamping only covered the polling transport; an upgrade arrived with raw client headers and, after the query fallback was removed, WebSocketManager could not key the socket under any identity. Collate's ChatWebSocketIT saw no chat events and the UI's jobs tray never refreshed, failing the merge queue (collate ITs and the Playwright export/import async-job suites). The upgrade creator now runs the same handshake validation (SocketAddressFilter.authenticate): a rejected upgrade answers the HTTP status the filter would send, and the accepted identity is stamped into the socket's connection headers, mirroring the servlet dispatch.
…dshakes The upgrade handler copied every raw upgrade header into the engine.io connection headers and only overwrote UserId/SessionId when the validated identity carried a value, so a client-sent SessionId survived next to an external IdP token (no session claim) and a client-sent UserId survived a cookie session with no stored user id. The polling path had the same gap: HeaderRequestWrapper only shadowed a header the filter had stamped. Both paths now drop the client's identity headers (case-insensitively) before stamping, and a handshake that yields no user id at all is rejected with 401 instead of being accepted and never registered by WebSocketManager. The handler also logged the parsed query map at INFO and the upgrade URI and parameter map at DEBUG; since the token now rides the query, every browser connection wrote a bearer token into the server log. Those lines are gone and a log-capture test guards them. Also: a real-JwtFilter regression test with enableSecureSocketConnection explicitly false (the mocked-validator suite stayed green when the old gate was restored), a startup warning when no authorizer configuration leaves the filter without a validator or session store, the stale jwtFilter comment, and the ADR consequences (upgrade handler logging, proxy/access-log redaction of the token parameter, no-authorizer deployments).
validatePrefixedTokenRequest is shared with the MCP transport, so its switch to JwtFilter.resolveUserName also changes which MCP bearer tokens authenticate: with emailClaim configured, a token carrying only an email claim now resolves to the stored username as it does over HTTP. Pin that with a unit test on authenticateRequest.
🚦 Removed from the merge queue —
|
authenticateUpgrade answers a rejected handshake with 401/403 and returns null, but the upgrade-creator lambda passed that null straight into Jetty12WebSocketHandler, whose constructor rejects it with Objects.requireNonNull. Every rejected upgrade (no credentials, bad token, user mismatch) then surfaced as a NullPointerException out of the WebSocket creator, which Jetty logs as an unexpected upgrade failure that an anonymous client can trigger at will. A null creator result is Jetty's supported way to stop the negotiation, so the lambda now builds the handler only for an authenticated upgrade.
|
Addressed in 8b7782f. The upgrade creator now returns |
🚦 Removed from the merge queue —
|
Code Review ✅ Approved 6 closed / 6 findings🔴 High risk · The new token/session gate and identity binding change authorization decisions; misvalidation could expose another user's push-feed data. Adds credential validation to push-feed WebSocket handshakes under every configuration, accepting a bearer token from the header or ✅ 6 closed✅ Bug: UI sockets now rejected for public-client SSO (no cookie, no token sent)
✅ Quality: Socket handshake auth rule and deprecated schema field have no ADR
✅ Bug: Async token refresh lands after reconnect attempt already sent
✅ Security: Client-sent UserId/SessionId upgrade headers survive when identity lacks one
✅ Bug: Rejected upgrades now throw NPE: creator wraps null identity in handler
...and 1 more closed from earlier reviews VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
|
Describe your changes:
Push-feed WebSocket handshakes are now authenticated under every configuration:
enableSecureSocketConnectionauthorizer setting).tokenquery parameter (required for public-client SSO, which holds an IdP token and noOM_SESSIONcookie); without it, those handshakes would be rejected.JwtFilter.resolveUserName, theJwtIdentityResolverpath): the stored username resolved from the email whenemailClaimis configured, the legacy principal claims otherwise. Previously the handshake resolved the principal with the legacy helper only, so email-first users could authenticate over HTTP yet lose the push feed.UserId/SessionIdheaders before stamping the validated identity, so a validated field that is absent never falls back to a client value; a handshake that yields no user id is rejected with 401.tokenquery parameter on/api/v1/push/feed.enableSecureSocketConnectionon the authorizer configuration no longer has any effect; it is marked deprecated (deprecated: true) in the schema (generated classes and TS types carry the annotation), the dereferenced schemas, and the sample configuration (the field is kept for compatibility — existing YAML continues to load).The rule is recorded in
docs/decisions/2026-10-26-push-feed-handshakes-always-authenticate.md.This change came out of an internal security review; the specifics are tracked with the security team out-of-band and are intentionally not described here.
Type of change:
authorizerConfigurationhas no validator and no session store, so it can no longer open push-feed sockets and logs a warning at startup)High-level design:
SocketAddressFilteris the only path into the push feed, so enforcing credential validation there closes the gap regardless of servlet registration:jwtFilterForbuilds the token validator whenever authentication and authorizer configuration are present, independent of the socket-security flag.resolveHandshakeTokenprefers theAuthorizationheader and falls back to thetokenquery parameter (WebSocket upgrades cannot set headers); a presented token must validate even when a session cookie rides along.validatePrefixedTokenRequestbinds the token toJwtFilter.resolveUserName(claims)— the one principal resolution shared with the HTTP filter — rather than reading the legacy principal claims itself.WebSocketManagerderives the socket key and session exclusively from the filter-stamped headers. The Jetty upgrade creator runs the sameSocketAddressFilter.authenticate(the servlet filter chain never sees a WebSocket upgrade) and stamps the identity into the engine.io connection headers; both paths drop client-suppliedUserId/SessionIdfirst.WebSocketProvidersetsreconnection: falseand schedules its own reconnect: awaitgetOidcToken(), replacesocket.io.opts.query, thensocket.connect(), so the engine is created only after the current token is in place. socket.io'sreconnect_attempthook fires synchronously right before the engine copies the query, which is too early for an asynchronous token read.Alternatives considered: defaulting the authorizer flag to
truein the schema (still allowed uncredentialed sockets when setfalse, i.e. the flag remained a foot-gun) and validating inWebSocketManageronly (the identity would still be adopted before a credential check could reach it). Both rejected in favour of a single fail-closed gate at the filter.Tests:
Use cases covered
emailClaimconfigured and a token carrying only anemailclaim, the socket binds to the stored username resolved from the email (realJwtFilter; rejected on the previous resolver, verified RED).io client disconnect/io server disconnectnot retried; unmount closes the socket and drops pending reconnects; a setup superseded by a user change while its token read is pending never creates or publishes a socket (all RED on the previous provider).UserId/SessionIdupgrade headers (mixed casing, multiple values) are dropped before the validated identity is stamped; an external token with no session claim leaves noSessionIdat all (WebSocket handler and polling wrapper, both verified RED before the fix).enableSecureSocketConnection: falseand no injected validator rejects a garbagetoken=even alongside a valid session cookie (RED against the pre-PR flag gate).validatePrefixedTokenRequestnow resolves the principal throughJwtFilter.resolveUserName, so an email-first token authenticates on the MCP bearer path like it does over HTTP (new case inOAuthHttpStatelessServerTransportProviderTest).SocketAddressFilterSpoofRevalidationBypassTest) still passes.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/socket/SocketAddressFilterTest.java(22 tests),Jetty12WebSocketHandlerTest.java(7),HeaderRequestWrapperTest.java(6),openmetadata-mcp/.../OAuthHttpStatelessServerTransportProviderTest.java(+1),openmetadata-ui/src/main/resources/ui/src/context/WebSocketProvider/WebSocketProvider.test.tsx(8 tests, new)SocketAddressFilter,Jetty12WebSocketHandlerandHeaderRequestWrappervia the socket test suite plusJwtFilterTest(org.openmetadata.service.socket.*Test+JwtFilterTest— 142 tests, all green;OAuthHttpStatelessServerTransportProviderTest— 30, green); the credential-less, email-first, header-stripping, token-logging and real-validator regression tests were verified RED before their fixes and GREEN after.Backend integration tests
Ingestion integration tests
Playwright (UI) tests
WebSocketProvideradds the handshake token it already holds and drives its own reconnect, covered by the Jest suite above). The full Playwright suite runs in the merge queue, and it is the regression signal for this change: the queue failed atd59475afbecause WebSocket upgrades lost their stamped identity and the jobs tray never refreshed (import/export and async-job suites);9002c05moved handshake validation into the upgrade creator, and the next queue run verifies it.Manual testing performed
mvn teston the socket test suite ofopenmetadata-servicebefore the change: the new credential-less-handshake test failed, demonstrating the unauthenticated bind.yarn jest src/context/WebSocketProvider— 8/8 green on the new provider, all red against the previous one;yarn ui-checkstyle:changedclean.UI screen recording:
N/A — backend-only change with a single provider hand-rolled parameter addition, no UI surface change.
The PR appears safe to merge; no new blocking issue was established.Summary
The PR requires credentials for push-feed connections and shares identity checks with HTTP authentication.
No new actionable issues were established. All supplied previous threads were unnumbered, so none receive numbered previous-finding entries.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Push-feed request] --> B{Transport} B -->|Polling| C[Servlet filter] B -->|WebSocket| D[Upgrade creator] C --> E[SocketAddressFilter.authenticate] D --> E E -->|Rejected| F[HTTP error] E -->|Accepted| G[Validated identity] G --> H[Stamp connection headers] H --> I[WebSocketManager]Reviews (6) · Last reviewed commit: "Authenticate the push-feed identity on W..." · Reviewed by Greptile