Refactor for security and modularity #9
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements by refactoring the code for better modularity and adding cross-platform support. The introduction of a non-root user in the Docker container is a good security enhancement. However, this change has introduced some critical permission issues in the Docker setup that will cause the application to fail at runtime. I've added two critical comments with details on the issues and suggestions for fixing them. Once these are addressed, this will be an excellent update.
| RUN useradd -ms /bin/bash coderunner | ||
|
|
There was a problem hiding this comment.
The newly created coderunner user needs write permissions for directories within /app, such as /app/jupyter_runtime, which are currently owned by root. Without this, the Jupyter server may fail at runtime. Please change the ownership of the /app directory after creating the user.
RUN useradd -ms /bin/bash coderunner
RUN chown -R coderunner:coderunner /app
| docker run -d --rm --name coderunner \ | ||
| -p 8222:8222 \ | ||
| -v "$ASSETS_SRC:/app/uploads" \ | ||
| instavm/coderunner |
There was a problem hiding this comment.
There's a critical permission issue with the mounted volume. The /app/uploads directory inside the container will be owned by the host user's UID/GID, but the processes inside the container run as the coderunner user, which likely has a different UID. This will prevent the application and Jupyter from writing files to the shared directory, causing runtime failures.
A common solution is to handle this at container startup. For example, the entrypoint script could be run as root to chown the mounted directory, and then switch to the coderunner user before executing the main application.
There was a problem hiding this comment.
I think we should revert this commit. Or maybe I will handle it in a new PR soon.
No description provided.