From d2b4c3bc79152e647a75ddd749cbe3025419f245 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Wed, 12 Aug 2026 15:39:16 -0700 Subject: [PATCH 1/2] refactor(notebook-migration-service): compute jupyter iframe url per request --- .../resource/NotebookMigrationResource.scala | 40 ++++++++++++------- .../NotebookMigrationResourceSpec.scala | 39 ++++++++++++++++-- 2 files changed, 62 insertions(+), 17 deletions(-) diff --git a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala index ac41ccd28d1..fbed6298215 100644 --- a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala +++ b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala @@ -75,13 +75,10 @@ object NotebookMigrationResource extends LazyLogging { private val jupyterUrl = StorageConfig.jupyterURL private val jupyterToken = StorageConfig.jupyterToken - // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook. - // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod - // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is - // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding - // per-user keying here, or one user's upload would overwrite another's iframe URL. - @volatile private var jupyterIframeURL = - s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken" + + // Default notebook name used when a request does not specify one, so a param-less + // getJupyterIframeURL call reproduces the URL from before this service became stateless. + private val defaultNotebookName = "notebook.ipynb" private def isJupyterAvailable(jupyterUrl: String): Boolean = { var conn: java.net.HttpURLConnection = null @@ -104,8 +101,17 @@ object NotebookMigrationResource extends LazyLogging { } } - // Returns the Jupyter iframe reference URL - def getJupyterIframeURL(): Response = { + // Returns the Jupyter iframe reference URL for the given notebook. + def getJupyterIframeURL(notebookName: String = defaultNotebookName): Response = { + // notebookName flows into the returned URL, so validate it the same way setNotebook does: + // block path traversal and keep it to a plain .ipynb filename. + if (!notebookName.matches("[A-Za-z0-9._-]+\\.ipynb")) { + return Response + .status(Response.Status.BAD_REQUEST) + .entity(errorJson(s"Invalid notebook name: $notebookName")) + .build() + } + if (!isJupyterAvailable(jupyterUrl)) { return Response .status(500) @@ -120,7 +126,9 @@ object NotebookMigrationResource extends LazyLogging { .build() } - Response.ok(successUrlJson(jupyterIframeURL)).build() + Response + .ok(successUrlJson(s"$jupyterUrl/notebooks/work/$notebookName?token=$jupyterToken")) + .build() } // Returns the URL of Jupyter @@ -217,8 +225,6 @@ object NotebookMigrationResource extends LazyLogging { .build() } - jupyterIframeURL = s"$jupyterUrl/notebooks/work/$notebookName?token=$jupyterToken" - Response .ok( s""" @@ -457,9 +463,15 @@ class NotebookMigrationResource extends LazyLogging { @GET @Path("/get-jupyter-iframe-url") - def getJupyterIframeURL(@Auth user: SessionUser): Response = { + def getJupyterIframeURL( + @QueryParam("notebookName") notebookName: String, + @Auth user: SessionUser + ): Response = { logger.info("Getting Jupyter iframe URL") - NotebookMigrationResource.getJupyterIframeURL() + val name = Option(notebookName) + .filter(_.nonEmpty) + .getOrElse(NotebookMigrationResource.defaultNotebookName) + NotebookMigrationResource.getJupyterIframeURL(name) } @GET diff --git a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala index bb1a29c7e57..531801e6f18 100644 --- a/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala +++ b/notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala @@ -449,7 +449,7 @@ class NotebookMigrationResourceSpec resource.setNotebook(validNotebook, user).getStatus shouldBe 500 resource.getJupyterURL(user).getStatus shouldBe 500 - resource.getJupyterIframeURL(user).getStatus shouldBe 500 + resource.getJupyterIframeURL(null, user).getStatus shouldBe 500 } it should "return 500 when the request body is malformed JSON" in { @@ -481,9 +481,42 @@ class NotebookMigrationResourceSpec urlResp.getStatus shouldBe Response.Status.OK.getStatusCode urlResp.getEntity.toString should include("localhost:9100") - val iframeResp = resource.getJupyterIframeURL(sessionUser(writerUid)) + val iframeResp = resource.getJupyterIframeURL(null, sessionUser(writerUid)) iframeResp.getStatus shouldBe Response.Status.OK.getStatusCode - iframeResp.getEntity.toString should include("/notebooks/work/") + iframeResp.getEntity.toString should include("/notebooks/work/notebook.ipynb") + } + } + + it should "build the iframe URL from an explicit notebook name" in { + withFakeJupyter(contentsStatus = 201) { + val resp = NotebookMigrationResource.getJupyterIframeURL("other.ipynb") + resp.getStatus shouldBe Response.Status.OK.getStatusCode + resp.getEntity.toString should include("/notebooks/work/other.ipynb") + } + } + + it should "reject an invalid notebook name for the iframe URL with 400" in { + // notebookName flows into the URL, so it is validated before any Jupyter call and + // rejected without a running server. + NotebookMigrationResource + .getJupyterIframeURL("../../etc/evil.ipynb") + .getStatus shouldBe Response.Status.BAD_REQUEST.getStatusCode + } + + it should "not be affected by a prior setNotebook call (no shared iframe state)" in { + // Pins the stateless refactor: getJupyterIframeURL builds its URL from the request, not + // from state left by setNotebook. A param-less iframe request after uploading other.ipynb + // must return the default notebook, not the just-uploaded name. + withFakeJupyter(contentsStatus = 201) { + val user = sessionUser(writerUid) + resource + .setNotebook("""{"notebookName": "other.ipynb", "notebookData": {"cells": []}}""", user) + .getStatus shouldBe Response.Status.OK.getStatusCode + + val iframe = resource.getJupyterIframeURL(null, user) + iframe.getStatus shouldBe Response.Status.OK.getStatusCode + iframe.getEntity.toString should include("/notebooks/work/notebook.ipynb") + iframe.getEntity.toString should not include "other.ipynb" } } From 13b4cddd23e67af5b9fa5be95c049800a2c5aca6 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Thu, 13 Aug 2026 10:45:16 -0700 Subject: [PATCH 2/2] refactor(notebook-migration-service): drop unused default arg on getJupyterIframeURL --- .../texera/service/resource/NotebookMigrationResource.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala index fbed6298215..84adccf4be6 100644 --- a/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala +++ b/notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala @@ -102,7 +102,7 @@ object NotebookMigrationResource extends LazyLogging { } // Returns the Jupyter iframe reference URL for the given notebook. - def getJupyterIframeURL(notebookName: String = defaultNotebookName): Response = { + def getJupyterIframeURL(notebookName: String): Response = { // notebookName flows into the returned URL, so validate it the same way setNotebook does: // block path traversal and keep it to a plain .ipynb filename. if (!notebookName.matches("[A-Za-z0-9._-]+\\.ipynb")) {