load all assets from localhost to set origin properly #387
Conversation
| const assetFilePath = path.join(appAssetsDir, assetPath); | ||
| const assetContent = fs.readFileSync(assetFilePath); | ||
| callback(assetContent); | ||
| } else { |
There was a problem hiding this comment.
Could you elaborate on what is happening in the else clause? My current understanding is that the if () clase is for hosting static assets and else assumes that if it is not a static asset, then this is a server request and modifies cookies and handles file upload, but it is not clear to me why specific parts are needed.
Should we split up the handler code into separate methods to better signal the intent and have easier time in the future?
There was a problem hiding this comment.
yes, you are right. since interceptBufferProtocol doesn't provide a default callback handler, we are having to make the requests to JupyterLab server in the else section. During my experiments, I found out that cookies were not properly set for some of the requests (websocket handshake?). So, to be on the safe side, I am setting them with both this._window.webContents.session.cookies.set(cookieObject); and requestHeaders['Cookie'] = Array.from(cookies.values()).join('; '); for every request. I am not sure at the moment how we can optimize this.
src/main/sessions.ts
Outdated
| if (qMark !== -1) { | ||
| assetPath = assetPath.substring(0, qMark); | ||
| } | ||
| const assetFilePath = path.join(appAssetsDir, assetPath); |
There was a problem hiding this comment.
What if assets path includes ..? That could potentially escalate access to filesystem. I think that we need to be very stringent here and check that the new assetFilePath is a child of appAssetsDir.
There was a problem hiding this comment.
We could also hard-code a list of static files that the client is allowed to access.
There was a problem hiding this comment.
I am not sure if asset path can include ... we are calculating it from request url. but still, It makes sense to check if assetFilePath is a child of appAssetsDir, good idea.
| const appConfig: IAppConfiguration = { | ||
| jlabPort: 8888, | ||
| token: 'jlab-token' | ||
| }; |
There was a problem hiding this comment.
I anticipate that a hard-coded global will cause issues once we will go for full front/back separation. But I am happy to address it myself later.
There was a problem hiding this comment.
yes, we can definitely improve this when we support remote backends etc. by the way, these are just default values and token is generated on the fly before server launch.
http://localhost:port/desktop-app-assets/index.htmlto be able to set window.origin properly