-
Notifications
You must be signed in to change notification settings - Fork 121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Update pullRemote.js to set appropriate directory permissions #782
Conversation
This commit modifies the directory creation permissions in the pullRemote function of pullRemote.js. Previously, the function was setting directory permissions to 0777 (full read, write, and execute permissions for user, group, and others). This approach is not aligned with best practices for security, particularly in secure environments such as OpenShift, where overly permissive settings can lead to vulnerabilities. The updated code now sets the permissions to 0755 (read, write, and execute for the user; read and execute for group and others). This change enhances security by restricting write access to the owner only while still allowing necessary read and execute permissions.
The committers listed above are authorized under a signed CLA. |
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
@JamieSlome could you please review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly I'm always anti 777
so I'm happy to see this change. 🎉
If we're already changing this then 700
might be more appropriate. IDK if we need any other groups or public looking at these files.
Some extra nuance:
Regarding deployment in a container on a platform such as OpenShift this shouldn't have been a particularly dangerous permission level as your container is typically single purpose, single user.
If someone is going to break into the container they'd probably do it via the service itself making them the 7
in 755
anyway.
If someone was able to get into the container not as root or the user running git-proxy, but as some other user, then this would help.
This is also a much larger problem if git-proxy is running alongside many other processes/users so good to fix either way.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #782 +/- ##
=======================================
Coverage 60.23% 60.23%
=======================================
Files 47 47
Lines 1647 1647
=======================================
Hits 992 992
Misses 655 655 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🍰
Thanks for the comments @06kellyjac 👍
Thank you @yadnyeshkolte for the contribution. In the future, please reference any related issue(s) that your pull request is intended to close. In this case, I've edited the description to reference #673 See GitHub docs for details. |
If there are any other best practices or tips you think would be helpful for a new contributor like me, I would love to hear them! Thank you @coopernetes for correcting me. |
fix: Update pullRemote.js to set appropriate directory permissions
Closes #673
This PR modifies the directory creation permissions in the pullRemote function of pullRemote.js.
Previously, the function was setting directory permissions to 0777 (full read, write, and execute permissions for user, group, and others). This approach is not aligned with best practices for security, particularly in secure environments such as OpenShift, where overly permissive settings can lead to vulnerabilities.
The updated code now sets the permissions to 0755 (read, write, and execute for the user; read and execute for group and others). This change enhances security by restricting write access to the owner only while still allowing necessary read and execute permissions.