(texera) 01/01: fix(amber): report a malformed PVE websocket handshake (#7852)

[gi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7852-f8e3e9ef082f69b6409371502706b0af9b4d62ff
in repository https://gitbox.apache.org/repos/asf/texera.git

commit c9a0ddf7ae18f9fd9ecc75732c0b5e93f0203794
Author: Xinyuan Lin <xi...@uci.edu>
AuthorDate: Sun Aug 23 04:35:46 2026 +0000

    fix(amber): report a malformed PVE websocket handshake (#7852)
    
    ### What changes were proposed in this PR?
    
    `PveWebsocketResource.onOpen` read the handshake parameters in its
    prologue, outside the `Future` that owns both the `catch` arm that turns
    a failure into an `[ERR]` line and the pump that writes to the socket. A
    handshake missing `cuid`, carrying an empty value list for it, or
    carrying a non-numeric one threw out of `onOpen` itself, ahead of both —
    so the endpoint's own error contract could not see it:
    
    ```
    Before:  ?pveName=env&action=install  ->  NumberFormatException out of onOpen  ->  socket closes, nothing sent
    After:   ?pveName=env&action=install  ->  [ERR] Missing required parameter: cuid  ->  __DONE__
    ```
    
    The client side makes that silent close worse than a lost message:
    `computing-unit-selection.component.ts` clears `isInstalling`/`isLocked`
    only when `onmessage` sees `__DONE__`, and there is no `onclose`
    handler, so the pip modal stayed locked on "installing" with an empty
    log until the user navigated away.
    
    The three reads move inside the `try`, so a malformed handshake now
    travels the same path as any other failure — an `[ERR]` line followed by
    the sentinel. A `requiredParam` helper rejects an absent key, an empty
    value list, and a blank value alike; the last one is reachable from the
    wire, since `?pveName=` arrives as `[""]` and used to resolve to a venv
    directory named `" "` rather than failing.
    
    | Handshake | Before | After |
    | --- | --- | --- |
    | no `cuid` | NPE out of `onOpen`, socket closes empty | `[ERR] Missing
    required parameter: cuid` then `__DONE__` |
    | `cuid` present, value list empty | IndexOutOfBoundsException, same |
    same as above |
    | `cuid=abc` | NumberFormatException, same | `[ERR] Invalid cuid: abc`
    then `__DONE__` |
    | `pveName=` (blank) | resolved a venv path with a whitespace name |
    `[ERR] Missing required parameter: pveName` then `__DONE__` |
    
    Well-formed handshakes are untouched: the parsed `cuid`/`pveName` reach
    `PveManager` exactly as before.
    
    ### Any related issues, documentation, discussions?
    
    Follows #7847, which added `PveWebsocketResourceSpec` and deliberately
    left this path unpinned so that fixing it would not have to fight a test
    that had cemented it.
    
    ### How was this PR tested?
    
    Four cases added to `PveWebsocketResourceSpec` — `cuid` absent, `cuid`
    with an empty value list, a non-numeric `cuid`, and a blank `pveName` —
    each asserting the client receives both the `[ERR]` line and the
    sentinel, and that the pump stops rather than parking in `queue.take()`.
    
    ```bash
    sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.pythonvirtualenvironment.PveWebsocketResourceSpec"
    ```
    
    `Tests: succeeded 7, failed 0`. Written before the fix: all four failed
    against the old source with the escaping exception itself
    (`NullPointerException`, `IndexOutOfBoundsException`,
    `NumberFormatException`) rather than an assertion mismatch, which is the
    defect stated as a test. Hoisting the reads back out of the `Future`
    fails all four again.
    
    Mutation testing on the new code — 4 mutants, 4 killed: dropping the
    `values == null` arm, dropping the `isEmpty` arm, hardcoding `cuid` into
    the message instead of interpolating the parameter name, and swallowing
    a bad `cuid` as `0`.
    
    `PveResourceSpec`, the other suite in the package, is unaffected —
    locally it reports 6 pre-existing failures that all come from the
    Windows interpreter path (`Scripts\python.exe`), unrelated to the
    endpoint.
    
    ```bash
    sbt scalafmtCheckAll "scalafixAll --check"
    ```
    
    Clean.
    
    ### Was this PR authored or co-authored using generative AI tooling?
    
    Generated-by: Claude Code (Opus 5)
---
 .../PveWebsocketResource.scala                     | 32 +++++++--
 .../PveWebsocketResourceSpec.scala                 | 82 +++++++++++++++++++++-
 2 files changed, 109 insertions(+), 5 deletions(-)

diff --git a/amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResource.scala
index 34654c7604..81bf1a1085 100644
--- a/amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResource.scala
+++ b/amber/src/main/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResource.scala
@@ -34,19 +34,43 @@ import scala.concurrent.ExecutionContext.Implicits.global
 @ServerEndpoint("/wsapi/pve")
 class PveWebsocketResource {
 
+  /**
+    * Reads a single-valued handshake parameter, rejecting an absent key, an empty value list
+    * and a blank value alike: all three are malformed handshakes, and reading one straight
+    * through would either throw from the reader itself or hand `PveManager` an empty name to
+    * resolve into a directory path.
+    */
+  private def requiredParam(
+      params: java.util.Map[String, java.util.List[String]],
+      name: String
+  ): String = {
+    val values = params.get(name)
+    if (values == null || values.isEmpty || values.get(0).isBlank) {
+      throw new IllegalArgumentException(s"Missing required parameter: $name")
+    }
+    values.get(0)
+  }
+
   @OnOpen
   def onOpen(session: Session): Unit = {
 
     val params = session.getRequestParameterMap
 
-    val cuid = params.get("cuid").get(0).toInt
-    val pveName = params.get("pveName").get(0)
-    val action = params.getOrDefault("action", java.util.List.of("create")).get(0)
-
     val queue = new LinkedBlockingQueue[String]()
 
     Future {
       try {
+        // These reads belong inside the try, not in the prologue of `onOpen`: up there a
+        // malformed handshake throws before either this catch arm or the pump below exists,
+        // and the client is left with a socket that closes carrying neither an `[ERR]` line
+        // nor the `__DONE__` sentinel it waits for.
+        val cuidParam = requiredParam(params, "cuid")
+        val cuid = cuidParam.toIntOption.getOrElse(
+          throw new IllegalArgumentException(s"Invalid cuid: $cuidParam")
+        )
+        val pveName = requiredParam(params, "pveName")
+        val action = params.getOrDefault("action", java.util.List.of("create")).get(0)
+
         action match {
           case "create" =>
             PveManager.createNewPve(cuid, queue, pveName)
diff --git a/amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResourceSpec.scala
index ad1f48fa03..7ae0bbf2e3 100644
--- a/amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResourceSpec.scala
+++ b/amber/src/test/scala/org/apache/texera/web/resource/pythonvirtualenvironment/PveWebsocketResourceSpec.scala
@@ -45,7 +45,12 @@ import scala.jdk.CollectionConverters._
   *     it closes the socket and then leaves the loop rather than parking in `queue.take()`
   *     on a queue nothing will ever fill again,
   *   - the failure path is symmetric with the happy one — the catch arm reports the action's
-  *     own exception message and the `finally` still emits the sentinel, and
+  *     own exception message and the `finally` still emits the sentinel,
+  *   - a malformed handshake is a failure of that same shape and not a special case: an
+  *     absent, empty or blank `cuid` or `pveName`, or a `cuid` that is not an Int, produces
+  *     an `[ERR]` line and then the sentinel. The reads have to stay inside the Future for
+  *     that — hoisted back out of it they throw from `onOpen` itself, ahead of both the catch
+  *     arm and the pump, and the client sees a socket that closes with nothing on it, and
   *   - `action` selects the branch, with the cuid and pveName parsed off the handshake
   *     handed to `PveManager` unswapped.
   *
@@ -240,4 +245,79 @@ class PveWebsocketResourceSpec extends AnyFlatSpec with Matchers with MockFactor
     f.sentLines.last shouldBe "__DONE__"
     f.assertPumpStopped()
   }
+
+  it should "report a handshake with no cuid rather than dying on the socket" in {
+    // The frontend always sends `cuid`, so this is a hand-rolled or stale handshake. With the
+    // read hoisted out of the Future the absent key threw a NullPointerException from `onOpen`
+    // -- ahead of both the catch arm and the pump -- so the client got no error line, no
+    // sentinel, and nothing to tell this apart from a socket that simply went away.
+    val f = fixture(
+      "pveName" -> "ws-no-cuid",
+      "action" -> "install"
+    )
+
+    new PveWebsocketResource().onOpen(f.session)
+    f.awaitClose()
+
+    f.sentLines shouldBe List("[ERR] Missing required parameter: cuid", "__DONE__")
+    f.assertPumpStopped()
+  }
+
+  it should "report a cuid whose value list is empty" in {
+    // A present-but-empty value list is the other half of the presence guard: `.get(0)` on it
+    // raises IndexOutOfBoundsException rather than the NullPointerException the absent key
+    // above raises, so dropping either half of `values == null || values.isEmpty` has to fail
+    // one of these two tests.
+    val f = fixtureOf(
+      java.util.Map.of(
+        "cuid",
+        java.util.List.of[String](),
+        "pveName",
+        java.util.List.of("ws-empty-cuid"),
+        "action",
+        java.util.List.of("install")
+      )
+    )
+
+    new PveWebsocketResource().onOpen(f.session)
+    f.awaitClose()
+
+    f.sentLines shouldBe List("[ERR] Missing required parameter: cuid", "__DONE__")
+    f.assertPumpStopped()
+  }
+
+  it should "report a non-numeric cuid" in {
+    // Present and non-blank but not an Int, so it fails past the presence guard, in the parse.
+    // The message has to name the offending value: `NumberFormatException`'s own wording says
+    // "For input string" and never mentions which parameter it came from.
+    val f = fixture(
+      "cuid" -> "not-a-number",
+      "pveName" -> "ws-bad-cuid",
+      "action" -> "install"
+    )
+
+    new PveWebsocketResource().onOpen(f.session)
+    f.awaitClose()
+
+    f.sentLines shouldBe List("[ERR] Invalid cuid: not-a-number", "__DONE__")
+    f.assertPumpStopped()
+  }
+
+  it should "report a blank pveName, naming the parameter that is missing" in {
+    // Blank is what a servlet container hands back for `?pveName=`, and it is as malformed as
+    // an absent key: there is no PVE whose name is whitespace, and `PveManager` would resolve
+    // it to a venv directory named "" under the user's. Asserting on pveName rather than cuid
+    // also pins that the message interpolates the parameter's name instead of hardcoding one.
+    val f = fixture(
+      "cuid" -> "424244",
+      "pveName" -> "   ",
+      "action" -> "install"
+    )
+
+    new PveWebsocketResource().onOpen(f.session)
+    f.awaitClose()
+
+    f.sentLines shouldBe List("[ERR] Missing required parameter: pveName", "__DONE__")
+    f.assertPumpStopped()
+  }
 }