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..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 @@ -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): 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" } }