[UX] Update dashboard routes and verify test package before releasing #5394
Conversation
sky/server/server.py
Outdated
| FileResponse for static files or index.html for client-side routing. | ||
| """ | ||
| # Try to serve the file directly e.g. /skypilot.svg, | ||
| # /favicon.ico, and static files in /_next/static | ||
| file_path = os.path.join(server_constants.DASHBOARD_DIR, full_path) |
There was a problem hiding this comment.
Actually, would this cause security issue, if a user a path is something like ../../../../?
There was a problem hiding this comment.
Can we just use this? https://fastapi.tiangolo.com/tutorial/static-files/
There was a problem hiding this comment.
With StaticFiles, if the user installed skypilot from source without building the dashboard, the API server will fail to start. I think we should avoid this since users may not need to check the dashboard.
There was a problem hiding this comment.
Agree. Could we check if the dir exists before mounting the StaticFiles?
There was a problem hiding this comment.
OK, I will have a try. Thanks!
There was a problem hiding this comment.
Updated to use StaticFiles.
For the ../../../../ case, the browser will normalize the path, but I also add a middleware to check this.
There was a problem hiding this comment.
Find another issue with StaticFiles, if the out directory does not exist, the static mount will fail which will cause API server fails to start, so we only mount the static routes when the directory exists, however, when the user run npm ru build, the API server needs to be restarted to mount the static routes. In this case, I think we should keep the original way to serve static files, and I have also added a middleware to check the relative path issue, WDYT? @cg505 @Michaelvll
There was a problem hiding this comment.
Yeah, I see what you mean.
Can we just create an empty out directory if it doesn't exist, instead of doing the mount in an an if statement? Will that cause some problems?
| class BrowserNoPostPutMiddleware(starlette.middleware.base.BaseHTTPMiddleware): | ||
| """Middleware to prevent POST and PUT requests from browsers.""" | ||
|
|
||
| async def dispatch(self, request: fastapi.Request, call_next): | ||
| if request.url.path.startswith('/dashboard/') and is_browser_request( | ||
| request) and request.method in ['POST', 'PUT']: | ||
| raise fastapi.HTTPException(status_code=405, | ||
| detail='Method not allowed') | ||
| return await call_next(request) |
| # Serve static files from the dashboard directory | ||
| if os.path.exists(server_constants.DASHBOARD_DIR): | ||
| app.mount('/dashboard/_next', | ||
| staticfiles.StaticFiles( | ||
| directory=f'{server_constants.DASHBOARD_DIR}/_next'), | ||
| name='dashboard') | ||
| else: | ||
| logger.warning( | ||
| f'Dashboard directory {server_constants.DASHBOARD_DIR} does not exist.' | ||
| f' Skipping dashboard static files mount.') |
There was a problem hiding this comment.
I assume this is for testing, and we will remove it if we decide to stick with the manually serving method. Is that right?
sky/server/server.py
Outdated
| FileResponse for static files or index.html for client-side routing. | ||
| """ | ||
| # Try to serve the file directly e.g. /skypilot.svg, | ||
| # /favicon.ico, and static files in /_next/static | ||
| file_path = os.path.join(server_constants.DASHBOARD_DIR, full_path) |
There was a problem hiding this comment.
Yeah, I see what you mean.
Can we just create an empty out directory if it doesn't exist, instead of doing the mount in an an if statement? Will that cause some problems?
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)