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.

Sign up for GitHub

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

Jump to bottom

Allow to set fetch options in .lfsconfig #4111

Open
JungPhilipp opened this issue Apr 27, 2020 · 5 comments · May be fixed by #4114
Open

Allow to set fetch options in .lfsconfig #4111

JungPhilipp opened this issue Apr 27, 2020 · 5 comments · May be fixed by #4114
Labels
enhancement
Projects
Backlog

Comments

@JungPhilipp
Copy link

At the end of config/git_fetcher.go there is a list of safe keys which limit what can be read from .lfsconfig. Some of the fetch options like lfs.fetchinclude/exclude are marked as safe, while others are not.
I agree that some settings are not safe, however, there are others that are but are disabled regardless.
Namely, the remaining fetch settings:

lfs.fetchrecentrefsdays
lfs.fetchrecentremoterefs
lfs.fetchrecentcommitsdays
lfs.fetchrecentalways

In my case, we have a team of testers which switch regularly between branches. Without specifying --recent or setting lfs.fetchrecentalways you quickly end up on branchA wanting to switch to branchB but lfs files are missing while recent commits have been received viagit pull on branchA. Now a working network connection is required to switch branches and access required lfs files on branchB
We want to specify sensible defaults for our testers without setting these properties locally.

But whatever the use-case, I think these settings can be considered safe and there shouldn't be a reason to exclude them even if the use-case seems limited.
@JungPhilipp JungPhilipp mentioned this issue Apr 27, 2020
Closed
@bk2204
Copy link
Member

bk2204 commented Apr 29, 2020

Hey,

I don't know how other folks on @git-lfs/core, but my personal preference is to strictly limit the keys in the .lfsconfig file, since they apply by default if any user clones that repository without anyone setting them. That means we have to consider not only legitimate use cases, but use cases where someone intentionally tries to act maliciously or with ill intent, like setting lfs.fetchrecentcommitsdays to 9999999 and thereby force people to download the entirety of the history and run them out of disk space. This wouldn't be terrible, but it would be annoying and inconvenient.

In general, I prefer not to add to the keys here because they have such a huge security problem potential. But, having said that, perhaps you could convince one of the other core team members to review and merge a PR that added these to the allowlist and documentation, and if so, I wouldn't stand in the way. I may just be overly cautious about security.

@JungPhilipp
Copy link
Author

JungPhilipp commented Apr 30, 2020 •
edited

Hi,
I definitely agree that we should shield unsuspecting users from harm, which is why I wouldn't add keys like lfs.standalonetransferagent. But as you have said yourself, with the aforementioned fetch keys the result is not terrible. Additionally, if someone wanted to be maliciously and force users to download huge amounts of data, they could already do that by having the data in HEAD.

Overall, I think we shouldn't prevent legitimate use-cases (in this case the desire to provide sensible defaults for a specific use-case) in order to shield users from inconveniences caused by maliciously repository owners. As long as the keys are generally safe, which the fetch options are, I'd argue for a liberal approach.
Not allowing these keys would in our case mean, we'd either have to provide a script for users to execute after cloning or (probably) frequent situations where users. for some reason, don't have network access and can't work on other branches due to missing LFS files.

This goes directly to the intends of Git LFS, the usage should be for end-users. By not allowing these customization to be set by an expert for the entire repository, it actually becomes more complicated for end-users.

I'm a C++ dev, so I might be biased, since we make it a point to allow users to shoot themselves in the
foot 😉

@JungPhilipp JungPhilipp linked a pull request Apr 30, 2020 that will close this issue
Open
@JungPhilipp
Copy link
Author

JungPhilipp commented Apr 30, 2020 •
edited

I have added a pull request which adds the mentioned keys and and updates documentation.
The PR additionally adds the lfs.pruneoffsetdays to the list of safe keys.

@JungPhilipp
Copy link
Author

There hasn't been any new comments in almost a month. I would still be interested to get this working tho. Our current workaround is to add an setup_git_config.sh and hope nobody forgets to execute it :(

@bk2204 bk2204 added this to Optional Enhancements in Backlog Aug 14, 2020
@bk2204 bk2204 changed the title Allow to set fetch options in .lsfconfig Allow to set fetch options in .lfsconfig Aug 14, 2020
@Pierre-Bartet
Copy link

like setting lfs.fetchrecentcommitsdays to 9999999

As @jungstar said, adding large data to the HEAD would have the same result, so what is the point ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
Backlog
Optional Enhancements
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add fetch/prune options to safe keys for lfsconfig JungPhilipp/git-lfs
3 participants
@bk2204 @Pierre-Bartet @JungPhilipp