mengw15 commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3781283499
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -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:
Review Comment:
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.
##########
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"
Review Comment:
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.
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -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)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
