[HOTFIX] Require random auth token for terminal WebSocket connections - #5434
Open
jongyoul wants to merge 4 commits into
Open
[HOTFIX] Require random auth token for terminal WebSocket connections#5434jongyoul wants to merge 4 commits into
jongyoul wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This hotfix hardens the %sh.terminal WebSocket endpoint by adding a per-terminal-server cryptographic auth token that clients must present via the WebSocket query string, preventing unauthenticated non-browser clients from connecting when the terminal port is reachable.
Changes:
- Generate a 256-bit CSPRNG token in
TerminalInterpreterand include it in the terminal dashboard URL. - Propagate the token from the dashboard URL into the WebSocket connection URL in the terminal frontend JS.
- Enforce token validation in
TerminalSocketand add a unit test for missing-token rejection.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| shell/src/test/java/org/apache/zeppelin/shell/TerminalInterpreterTest.java | Updates test connections to include the token and adds a missing-token rejection test. |
| shell/src/main/resources/html/js/index.js | Reads token from query params and includes it in the WebSocket URL. |
| shell/src/main/java/org/apache/zeppelin/shell/TerminalInterpreter.java | Generates/stores the auth token and appends it to the dashboard URL; exposes token for tests. |
| shell/src/main/java/org/apache/zeppelin/shell/terminal/websocket/TerminalSocket.java | Validates token on connect and rejects unauthorized sessions. |
| shell/src/main/java/org/apache/zeppelin/shell/terminal/TerminalThread.java | Passes the expected auth token to the WebSocket endpoint via user properties. |
Suppressed comments (1)
shell/src/main/java/org/apache/zeppelin/shell/terminal/websocket/TerminalSocket.java:79
- Logging the full
Sessionobject here can leak the auth token (query string) into logs on successful connections. Log a non-sensitive identifier (e.g.,sess.getId()) instead.
authorized = true;
LOGGER.info("Socket Connected: {}", sess);
terminalService.onWebSocketConnect(sess);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tbonelee
previously approved these changes
Aug 24, 2026
…d terminal sockets
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.
What is this PR for?
This PR hardens the
%sh.terminalWebSocket endpoint by requiring a per-session 256-bit CSPRNG authentication token.Previously, the terminal WebSocket server only validated the
Originrequest header, which could be forged by non-browser clients to interact with the interactive bash shell without credentials when the terminal port is reachable.With this change:
TerminalInterpretergenerates a 256-bit random authentication token usingSecureRandomwhen creating the terminal server.?token=...) embedded in the paragraph result behind Zeppelin's note ACLs.TerminalSocketvalidates the incomingtokenquery parameter using constant-timeMessageDigest.isEqual.VIOLATED_POLICY(1008), and unauthenticated sessions ignore incoming messages.What type of PR is it?
Hot Fix
Todos
TerminalInterpreterTerminalSocketindex.jsWhat is the Jira issue?
N/A
How should this be tested?
./mvnw test -pl shell -Dtest=TerminalInterpreterTest ./mvnw clean org.apache.rat:apache-rat-plugin:check -Prat -pl shellScreenshots (if appropriate)
N/A
Questions: