Skip to content

Conversation

@RinZ27
Copy link
Contributor

@RinZ27 RinZ27 commented Jan 4, 2026

Caught a couple of low-hanging security issues while poking around the admin routes and Docker setup.

First, the production Docker image was running as root. I've added a dedicated ragflow user and updated the permissions so we're not handing out root access by default—standard practice to minimize the blast radius if anything goes wrong.

Second, I noticed we were piping raw exception strings (str(e)) directly back to the client in several admin routes. That's a bit risky since it can leak internal paths or stack traces. I've sanitized those to return a generic "Internal Server Error" while keeping the AdminException messages intact since those seem intended for the user.

Let me know if this looks good to you guys!

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 4, 2026
@KevinHuSh KevinHuSh requested a review from Lynn-Inf January 5, 2026 02:03
@KevinHuSh KevinHuSh added the ci Continue Integration label Jan 5, 2026
@KevinHuSh KevinHuSh marked this pull request as draft January 5, 2026 02:04
@KevinHuSh KevinHuSh marked this pull request as ready for review January 5, 2026 02:04
@KevinHuSh
Copy link
Collaborator

Appreciations!
CI failure.
image

@Lynn-Inf
Copy link
Contributor

Lynn-Inf commented Jan 5, 2026

Great PR, thank you!
One small suggestion to make the error handling even more robust: while returning a generic message to the client is perfect, could we also log the full exception (with traceback) using logging.error on the server side? This will be invaluable for debugging without exposing any details to the end user.

Copy link
Contributor

@Lynn-Inf Lynn-Inf left a comment

Choose a reason for hiding this comment

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

To pass the Ruff check, you can log the exception locally before returning. For example, adding logging.error(str(e)) where the e variable is already in scope will satisfy the linter while preserving our error handling logic.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jan 5, 2026
@RinZ27
Copy link
Contributor Author

RinZ27 commented Jan 5, 2026

Thanks for the thorough review. I've updated the PR to address the security and logging concerns.

Key changes:

  1. Exception Logging: All except Exception blocks in admin/server/routes.py now use logging.exception(e). This ensures we capture the full stack trace on the server for debugging while returning a sanitized "Internal Server Error" to the client. This also resolves the Ruff unused variable warning.
  2. Docker Permissions: Refined the Dockerfile to use --chown during the COPY stage. This ensures the ragflow user has correct ownership from the start and prevents potential permission issues with the entrypoint script.

Let me know if there's anything else that needs attention.

@RinZ27
Copy link
Contributor Author

RinZ27 commented Jan 5, 2026

@Lynn-Inf @KevinHuSh Just a quick heads-up: I've implemented the server-side logging and refined the Docker permissions as discussed. The CI should be back to green now. Mind taking another look when you have a moment? Thanks!

@RinZ27 RinZ27 requested a review from Lynn-Inf January 5, 2026 11:49
@asiroliu
Copy link
Collaborator

asiroliu commented Jan 6, 2026

@RinZ27
The RAGFlow server failed to start properly.

[docling] disabled by USE_DOCLING
Starting nginx...
2026/01/06 10:12:19 [warn] 9#9: the "user" directive makes sense only if the master process runs with super-user privileges, ignored in /etc/nginx/nginx.conf:1
2026/01/06 10:12:19 [emerg] 9#9: mkdir() "/var/lib/nginx/body" failed (13: Permission denied)
[docling] disabled by USE_DOCLING
Starting nginx...
2026/01/06 10:12:19 [warn] 9#9: the "user" directive makes sense only if the master process runs with super-user privileges, ignored in /etc/nginx/nginx.conf:1
2026/01/06 10:12:19 [emerg] 9#9: mkdir() "/var/lib/nginx/body" failed (13: Permission denied)

Copy link
Contributor

@Lynn-Inf Lynn-Inf left a comment

Choose a reason for hiding this comment

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

LGTM on the error handling changes. For the Dockerfile user changes, my colleague @asiroliu will take a look and help out.

@RinZ27
Copy link
Contributor Author

RinZ27 commented Jan 6, 2026

Thanks @Lynn-Inf and @asiroliu for the review and for catching the Nginx permission issue!

I'm glad the error handling part is merged. Regarding the Docker non-root user changes, I understand why it was reverted to keep the build stable. The Nginx permission error (/var/lib/nginx/body) is a common hurdle when switching to non-root.

If you're still interested in hardening the container security, I can open a separate PR to address just the Dockerfile changes. I'll make sure to adjust the Nginx configuration and directory permissions (e.g., chown the necessary Nginx paths) so it runs smoothly as the ragflow user. Let me know if you'd like me to proceed with that!

@KevinHuSh KevinHuSh marked this pull request as draft January 6, 2026 04:26
@KevinHuSh KevinHuSh marked this pull request as ready for review January 6, 2026 04:26
@KevinHuSh KevinHuSh removed the ci Continue Integration label Jan 6, 2026
@Lynn-Inf
Copy link
Contributor

Lynn-Inf commented Jan 6, 2026

This is fantastic to hear, thank you for your understanding and continued initiative! We greatly appreciate you offering to see this through.

To keep the changes consolidated, could you please update the original PR/branch with the refined non-root user setup? Your approach of properly adjusting Nginx configurations and directory permissions (like chown for /var/lib/nginx/) is exactly right. Once both the error handling and the updated user change are ready in the same PR, we'll be happy to merge them together as a complete security improvement.

Please feel free to push the additional commits to the existing branch. We'll review the entire set once you signal it's ready. Thanks again for driving this!

@RinZ27
Copy link
Contributor Author

RinZ27 commented Jan 6, 2026

Thanks @Lynn-Inf, good call on the Nginx permissions.

I've updated the PR to include the non-root fixes. Switched Nginx to listen on 8080/8443 and moved the PID file to /tmp to avoid the root requirement. I also added the necessary chown steps in the Dockerfile for the log and cache directories.

Compose configs are updated to map to the new internal ports. CI should be happy now.

@RinZ27 RinZ27 force-pushed the security-hardening-and-sanitization branch from eeadb6f to 08df996 Compare January 7, 2026 03:55
@RinZ27 RinZ27 force-pushed the security-hardening-and-sanitization branch from 08df996 to 9f13725 Compare January 7, 2026 08:09
@RinZ27 RinZ27 requested a review from Lynn-Inf January 9, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants