git: Fix commit message generation in untrusted projects and block external diff#51323
git: Fix commit message generation in untrusted projects and block external diff#51323Anthony-Eid merged 2 commits intomainfrom
Conversation
When on a untrusted project, if one was to try and use the commit generation functionality, the command would fail because of the `-c diff.external` configuration provided in `GitBinary::build_command`, as git would interpret this as `""` and try to run that command. This `-c diff.external` is a good safeguard to have on untrusted repositories because it prevents random commands, configured in `.git/config` from being run. For example, if one uses `git config diff.external "touch bananas.txt"` and then run `git diff`, a new `bananas.txt` file would be created. However, it was still possible to bypass this safeguard using the following strategy: 1. Specify a custom diff for a specific file format. For example, for markdown files, with `printf '*.md diff=pwned\n' > .gitattributes` 2. Update the command run by the `pwned` diff, for example, `git config diff.pwned.command "touch bananas.txt"` 3. Open Zed and attempt to generate a commit message in an untrusted repository and check that a new `bananas.txt` file was created This is only prevented by using the `--no-ext-diff` flag on the `diff` command, so a new `GitBinary::build_diff_command` has been introduced which simply wraps `GitBinary::build_command` and adds the `--no-ext-diff` flag, if necessary. As a side-effect, this also makes it so that generating a commit message in an untrusted repository works again.
|
Damn, nice research! Is there any reason we couldn't unify |
Anthony-Eid
left a comment
There was a problem hiding this comment.
This looks good to me! I appreciate the descriptive PR message
Sorry, I opened this late at night and was actually planning to leave a comment on this, we don't necessarily need If we do wish to unify, I think we could probably update Thank you for the review! |
|
If If you end up going with the separate build command functions we could put a debug assert in I also think this is good to merge now, we can always improve forward and this seems important enough to get in and cherry pick to preview and maybe stable if it's affected there as well |
* Remove `git::repository::GitBinary::build_diff_command` * Update `git::repository::GitBinary::build_command` to check if `diff` is being used and, if that's the case and the repository is untrusted, add the `--no-ext-diff` flag * Update the type signature of `git::repository::GitBinary::build_command` to avoid having to clone the `args` iterator, by accepting a `&[S]` instead of `impl IntoIterator<Item = S>`
Makes total sense!
I ended up pushing a commit – ae0906d – that does the check in
I've confirmed this is only affecting Preview 🫡 |
|
/cherry-pick preview |
…ternal diff (#51323) When on a untrusted project, if one was to try and use the commit generation functionality, the command would fail because of the `-c diff.external` configuration provided in `GitBinary::build_command`, as git would interpret this as `""` and try to run that command. This `-c diff.external` is a good safeguard to have on untrusted repositories because it prevents random commands, configured in `.git/config` from being run. For example, if one uses `git config diff.external "touch bananas.txt"` and then run `git diff`, a new `bananas.txt` file would be created. However, it was still possible to bypass this safeguard using the following strategy: 1. Specify a custom diff for a specific file format. For example, for markdown files, with `printf '*.md diff=pwned\n' > .gitattributes` 2. Update the command run by the `pwned` diff, for example, `git config diff.pwned.command "touch bananas.txt"` 3. Open Zed and attempt to generate a commit message in an untrusted repository and check that a new `bananas.txt` file was created This is only prevented by using the `--no-ext-diff` flag on the `diff` command, so a new `GitBinary::build_diff_command` has been introduced which simply wraps `GitBinary::build_command` and adds the `--no-ext-diff` flag, if necessary. As a side-effect, this also makes it so that generating a commit message in an untrusted repository works again, which was accidentally broken on #50649 . Before you mark this PR as ready for review, make sure that you have: - [X] Added a solid test coverage and/or screenshots from doing manual testing - [X] Done a self-review taking into account security and performance aspects - [X] Aligned any UI changes with the [UI checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) Release Notes: - Fixed commit message generation in untrusted repositories
…ternal diff (#51323) (cherry-pick to preview) (#51492) Cherry-pick of #51323 to preview ---- When on a untrusted project, if one was to try and use the commit generation functionality, the command would fail because of the `-c diff.external` configuration provided in `GitBinary::build_command`, as git would interpret this as `""` and try to run that command. This `-c diff.external` is a good safeguard to have on untrusted repositories because it prevents random commands, configured in `.git/config` from being run. For example, if one uses `git config diff.external "touch bananas.txt"` and then run `git diff`, a new `bananas.txt` file would be created. However, it was still possible to bypass this safeguard using the following strategy: 1. Specify a custom diff for a specific file format. For example, for markdown files, with `printf '*.md diff=pwned\n' > .gitattributes` 2. Update the command run by the `pwned` diff, for example, `git config diff.pwned.command "touch bananas.txt"` 3. Open Zed and attempt to generate a commit message in an untrusted repository and check that a new `bananas.txt` file was created This is only prevented by using the `--no-ext-diff` flag on the `diff` command, so a new `GitBinary::build_diff_command` has been introduced which simply wraps `GitBinary::build_command` and adds the `--no-ext-diff` flag, if necessary. As a side-effect, this also makes it so that generating a commit message in an untrusted repository works again, which was accidentally broken on #50649 . Before you mark this PR as ready for review, make sure that you have: - [X] Added a solid test coverage and/or screenshots from doing manual testing - [X] Done a self-review taking into account security and performance aspects - [X] Aligned any UI changes with the [UI checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) Release Notes: - Fixed commit message generation in untrusted repositories Co-authored-by: Dino <dinojoaocosta@gmail.com>
…ternal diff (zed-industries#51323) When on a untrusted project, if one was to try and use the commit generation functionality, the command would fail because of the `-c diff.external` configuration provided in `GitBinary::build_command`, as git would interpret this as `""` and try to run that command. This `-c diff.external` is a good safeguard to have on untrusted repositories because it prevents random commands, configured in `.git/config` from being run. For example, if one uses `git config diff.external "touch bananas.txt"` and then run `git diff`, a new `bananas.txt` file would be created. However, it was still possible to bypass this safeguard using the following strategy: 1. Specify a custom diff for a specific file format. For example, for markdown files, with `printf '*.md diff=pwned\n' > .gitattributes` 2. Update the command run by the `pwned` diff, for example, `git config diff.pwned.command "touch bananas.txt"` 3. Open Zed and attempt to generate a commit message in an untrusted repository and check that a new `bananas.txt` file was created This is only prevented by using the `--no-ext-diff` flag on the `diff` command, so a new `GitBinary::build_diff_command` has been introduced which simply wraps `GitBinary::build_command` and adds the `--no-ext-diff` flag, if necessary. As a side-effect, this also makes it so that generating a commit message in an untrusted repository works again, which was accidentally broken on zed-industries#50649 . Before you mark this PR as ready for review, make sure that you have: - [X] Added a solid test coverage and/or screenshots from doing manual testing - [X] Done a self-review taking into account security and performance aspects - [X] Aligned any UI changes with the [UI checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) Release Notes: - Fixed commit message generation in untrusted repositories
When on a untrusted project, if one was to try and use the commit generation functionality, the command would fail because of the
-c diff.externalconfiguration provided inGitBinary::build_command, as git would interpret this as""and try to run that command.This
-c diff.externalis a good safeguard to have on untrusted repositories because it prevents random commands, configured in.git/configfrom being run. For example, if one usesgit config diff.external "touch bananas.txt"and then rungit diff, a newbananas.txtfile would be created.However, it was still possible to bypass this safeguard using the following strategy:
printf '*.md diff=pwned\n' > .gitattributespwneddiff, for example,git config diff.pwned.command "touch bananas.txt"bananas.txtfile was createdThis is only prevented by using the
--no-ext-diffflag on thediffcommand, so a newGitBinary::build_diff_commandhas been introduced which simply wrapsGitBinary::build_commandand adds the--no-ext-diffflag, if necessary.As a side-effect, this also makes it so that generating a commit message in an untrusted repository works again, which was accidentally broken on #50649 .
Before you mark this PR as ready for review, make sure that you have:
Closes #51249.
Release Notes: