Skip to content
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

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

yadnyeshkolte
Copy link
Contributor

@yadnyeshkolte yadnyeshkolte commented Nov 6, 2024

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.

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.
Copy link

linux-foundation-easycla bot commented Nov 6, 2024

CLA Signed

  • ✅login: yadnyeshkolte / (dac735e)

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit dac735e
🔍 Latest deploy log https://app.netlify.com/sites/endearing-brigadeiros-63f9d0/deploys/672afcbc679f69000847845e

@yadnyeshkolte yadnyeshkolte marked this pull request as draft November 6, 2024 05:29
@yadnyeshkolte yadnyeshkolte changed the title Fix: pullRemote clone dir permissions are extraneous #673 fix: Update pullRemote.js to set appropriate directory permissions Nov 6, 2024
@github-actions github-actions bot added the fix label Nov 6, 2024
@yadnyeshkolte yadnyeshkolte marked this pull request as ready for review November 6, 2024 05:32
@yadnyeshkolte
Copy link
Contributor Author

@JamieSlome could you please review this PR.

Copy link
Contributor

@06kellyjac 06kellyjac left a 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.

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.23%. Comparing base (39dd45c) to head (dac735e).
Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@JamieSlome JamieSlome removed the request for review from coopernetes November 7, 2024 13:04
Copy link
Member

@JamieSlome JamieSlome left a 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 👍

@JamieSlome JamieSlome merged commit fac98ca into finos:main Nov 7, 2024
14 of 15 checks passed
@coopernetes
Copy link
Contributor

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.

@yadnyeshkolte
Copy link
Contributor Author

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.

Psingle20 pushed a commit to Psingle20/git-proxy that referenced this pull request Nov 27, 2024
fix: Update pullRemote.js to set appropriate directory permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pullRemote clone dir permissions are extraneous
4 participants