Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Comment on lines +79 to +81

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block being removed also carried a deployment warning — "Do NOT deploy this service as a shared multi-user instance without adding per-user keying here". Removing the state removes one reason for it, but not the hazard: jupyterUrl and jupyterToken are still single process-wide values, so a shared instance would hand every user the same Jupyter and the same token.

Since this is explicitly stage 1, worth keeping a line to that effect so the constraint survives to the stage that actually lifts it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up: #7390 notes the shared state "blocks running the service as a single global instance", and it closes with this PR — so with the warning comment going too, I'm not sure what's left tracking the jupyterUrl/jupyterToken half. Maybe a follow-up issue like #7636?


private def isJupyterAvailable(jupyterUrl: String): Boolean = {
var conn: java.net.HttpURLConnection = null
Expand All @@ -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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mirrors setNotebook's validation, but the comment over there still justifies itself partly with "keeps notebookName out of the raw-interpolated jupyterIframeURL JSON" — and jupyterIframeURL is gone as of this PR. Same stale-reference class Copilot flagged on the frontend. Its path-traversal half still stands; worth fixing the other half while it's in scope.

// 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)
Expand All @@ -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
Expand Down Expand Up @@ -217,8 +225,6 @@ object NotebookMigrationResource extends LazyLogging {
.build()
}

jupyterIframeURL = s"$jupyterUrl/notebooks/work/$notebookName?token=$jupyterToken"

Response
.ok(
s"""
Expand Down Expand Up @@ -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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fallback is only reachable because the frontend still sends a fixed notebook.ipynb for every workflow. Its comment there justifies that with "each user runs their own pod, so a single notebook.ipynb never collides" — true across users, but not across one user's workflows: they all land on the same work/notebook.ipynb, so switching workflows overwrites it, and since nothing writes back from Jupyter, edits made in the panel are gone. Two tabs on different workflows likewise end up on the same file while each keeps its own cell-highlight mapping.

This PR is what makes the fix possible — notebook_<wid>.ipynb already passes the regex and needs nothing further from the backend.

NotebookMigrationResource.getJupyterIframeURL(name)
}

@GET
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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")
}
}
Comment on lines +490 to +496

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"
}
}

Expand Down
Loading