This repository was archived by the owner on Sep 30, 2024. It is now read-only.
gitserver: Use git-diff-tree to simplify validation and improve security#62709
Merged
eseliger merged 1 commit intoMay 16, 2024
Conversation
Member
Author
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1da530f to
66a5b51
Compare
evict
approved these changes
May 16, 2024
At some point in the past, we noticed that passing /dev/passwd and /dev/null to `git diff` allows to view the file contents, even though it's not part of the repo itself. That allows to read arbitrary files on disk, so we had to add in-app validation for file paths, including relative paths. That makes the validation much more complex and harder to reason about, and requires that we didn't make a mistake. Instead, we switch to `git diff-tree` which validates paths are part of the tree properly. By that, we can remove a bunch of complexity around git diff argument validation. See here for the difference: ``` ➜ sourcegraph git:(main) git diff-tree --find-renames --full-index --inter-hunk-context=3 --no-prefix -- /etc/hosts /dev/null fatal: /etc/hosts: '/etc/hosts' is outside repository at '/Users/erik/Code/sourcegraph/sourcegraph' ➜ sourcegraph git:(main) git --no-pager diff --find-renames --full-index --inter-hunk-context=3 --no-prefix -- /etc/hosts /dev/null diff --git etc/hosts etc/hosts deleted file mode 100644 index deadbeef..0000000000000000000000000000000000000000 --- etc/hosts +++ /dev/null @@ -1,16 +0,0 @@ -## -# Host Database -# -# localhost is used to configure the loopback interface -# when the system is booting. Do not change this entry. -## -127.0.0.1 localhost ``` This PR also skips over arguments past the `--` boundary which in git means "everything after this will not be interpreted as an argument". This previously caused `git cat-file HEAD -- -file.txt` to fail argument validation, although it's perfectly valid and not a risk. Test plan: Adjusted the tests for argument validation and existing tests for diff and integration and E2E tests are still passing.
e32fc38 to
d3fa027
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At some point in the past, we noticed that passing /etc/passwd and /dev/null to
git diffallows to view the file contents, even though it's not part of the repo itself.That allows to read arbitrary files on disk, so we had to add in-app validation for file paths, including relative paths.
That makes the validation much more complex and harder to reason about, and requires that we didn't make a mistake.
Instead, we switch to
git diff-treewhich validates paths are part of the tree properly.By that, we can remove a bunch of complexity around git diff argument validation.
See here for the difference:
This PR also skips over arguments past the
--boundary which in git means "everything after this will not be interpreted as an argument".This previously caused
git cat-file HEAD -- -file.txtto fail argument validation, although it's perfectly valid and not a risk.Test plan:
Adjusted the tests for argument validation and existing tests for diff and integration and E2E tests are still passing.