Skip to content

Fix CredentialHelper::add_command #1137

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
Merged
merged 2 commits into from
Mar 17, 2025
Merged

Fix CredentialHelper::add_command #1137

merged 2 commits into from
Mar 17, 2025

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Mar 5, 2025 •
edited
Loading

Previously the add_helper function would check whether the command contains a \ or / character to determine whether it is absolute. It should have used starts_with instead of contains. In addition an absolute on Windows a path my start with a drive letter. The git cli implements this by checking if the second character is a ':'.

See also: https://github.com/git/git/blob/6a64ac7b014fa2cfa7a69af3c253bcd53a94b428/credential.c#L492-L493

Fixes #1136

@rustbot rustbot added the S-waiting-on-review Status: Waiting on review label Mar 5, 2025
@aibaars aibaars force-pushed the fix-credential-helper branch from 16bd0dc to ef3bca9 Compare March 10, 2025 10:41
@aibaars aibaars force-pushed the fix-credential-helper branch from ef3bca9 to c776e8d Compare March 10, 2025 10:42
ehuss
ehuss approved these changes Mar 17, 2025
View reviewed changes
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/cred.rs
fn is_absolute_path(path: &str) -> bool {
path.starts_with('/')
|| path.starts_with('\\')
|| cfg!(windows) && path.chars().nth(1).is_some_and(|x| x == ':')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor note, this is slightly different from https://github.com/git-for-windows/git/blob/cca1f38702730b35f52c29efd62864b85e85ddcc/compat/win32/path-utils.c#L9-L31, but probably not enough to matter here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, git2-rs uses the str type while the git C uses a char* and if I understand the git C code correctly, it just tries to handle the case where the drive letter is a UTF8 character. I'd expect the str::nth function to take care of that already. However, there might indeed be some behaviour differences in corner cases at I'm not aware of.

@ehuss ehuss added this pull request to the merge queue Mar 17, 2025
Merged via the queue into rust-lang:master with commit 1e1687e Mar 17, 2025
7 checks passed
@ehuss
Copy link
Contributor

ehuss commented Mar 17, 2025

Note that this was relaxed in #429 which previously looked for starts_with, but I agree it is probably better to match behavior with git itself, and the : check should accommodate the original issue.

facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request May 15, 2025
@facebook-github-bot
update libgit2 0.20.0 to 0.20.2
7fcfb87 
Summary:
# Changelog

## 0.20.2 - 2025-05-05
[0.20.1...0.20.2](rust-lang/git2-rs@git2-0.20.1...git2-0.20.2)

### Added

- Added `Status::WT_UNREADABLE`.
  [#1151](rust-lang/git2-rs#1151)

### Fixed

- Added missing codes for `GIT_EDIRECTORY`, `GIT_EMERGECONFLICT`, `GIT_EUNCHANGED`, `GIT_ENOTSUPPORTED`, and `GIT_EREADONLY` to `Error::raw_code`.
  [#1153](rust-lang/git2-rs#1153)
- Fixed missing initialization in `Indexer::new`.
  [#1160](rust-lang/git2-rs#1160)

## 0.20.1 - 2025-03-17
[0.20.0...0.20.1](rust-lang/git2-rs@git2-0.20.0...git2-0.20.1)

### Added

- Added `Repository::branch_upstream_merge()`
  [#1131](rust-lang/git2-rs#1131)
- Added `Index::conflict_get()`
  [#1134](rust-lang/git2-rs#1134)
- Added `Index::conflict_remove()`
  [#1133](rust-lang/git2-rs#1133)
- Added `opts::set_cache_object_limit()`
  [#1118](rust-lang/git2-rs#1118)
- Added `Repo::merge_file_from_index()` and associated `MergeFileOptions` and `MergeFileResult`.
  [#1062](rust-lang/git2-rs#1062)

### Changed

- The `url` dependency minimum raised to 2.5.4
  [#1128](rust-lang/git2-rs#1128)
- Changed the tracing callback to abort the process if the callback panics instead of randomly detecting the panic in some other function.
  [#1121](rust-lang/git2-rs#1121)
- Credential helper config (loaded with `CredentialHelper::config`) now checks for helpers that start with something that looks like an absolute path, rather than checking for a `/` or `\` anywhere in the helper string (which resolves an issue if the helper had arguments with `/` or `\`).
  [#1137](rust-lang/git2-rs#1137)

### Fixed

- Fixed panic in `Remote::url_bytes` if the url is empty.
  [#1120](rust-lang/git2-rs#1120)
- Fixed incorrect lifetimes on `Patch::delta`, `Patch::hunk`, and `Patch::line_in_hunk`. The return values must not outlive the `Patch`.
  [#1141](rust-lang/git2-rs#1141)
- Bumped requirement to libgit2-sys 0.18.1, which fixes linking of advapi32 on Windows.
  [#1143](rust-lang/git2-rs#1143)

ignore-conflict-markers

Reviewed By: JakobDegen

Differential Revision: D74659779

fbshipit-source-id: a18bcd8f58bc62c7eedbfa5939a791002e18d7bc
facebook-github-bot pushed a commit to facebookincubator/reindeer that referenced this pull request May 15, 2025
@facebook-github-bot
update libgit2 0.20.0 to 0.20.2
cabe41f 
Summary:
# Changelog

## 0.20.2 - 2025-05-05
[0.20.1...0.20.2](rust-lang/git2-rs@git2-0.20.1...git2-0.20.2)

### Added

- Added `Status::WT_UNREADABLE`.
  [#1151](rust-lang/git2-rs#1151)

### Fixed

- Added missing codes for `GIT_EDIRECTORY`, `GIT_EMERGECONFLICT`, `GIT_EUNCHANGED`, `GIT_ENOTSUPPORTED`, and `GIT_EREADONLY` to `Error::raw_code`.
  [#1153](rust-lang/git2-rs#1153)
- Fixed missing initialization in `Indexer::new`.
  [#1160](rust-lang/git2-rs#1160)

## 0.20.1 - 2025-03-17
[0.20.0...0.20.1](rust-lang/git2-rs@git2-0.20.0...git2-0.20.1)

### Added

- Added `Repository::branch_upstream_merge()`
  [#1131](rust-lang/git2-rs#1131)
- Added `Index::conflict_get()`
  [#1134](rust-lang/git2-rs#1134)
- Added `Index::conflict_remove()`
  [#1133](rust-lang/git2-rs#1133)
- Added `opts::set_cache_object_limit()`
  [#1118](rust-lang/git2-rs#1118)
- Added `Repo::merge_file_from_index()` and associated `MergeFileOptions` and `MergeFileResult`.
  [#1062](rust-lang/git2-rs#1062)

### Changed

- The `url` dependency minimum raised to 2.5.4
  [#1128](rust-lang/git2-rs#1128)
- Changed the tracing callback to abort the process if the callback panics instead of randomly detecting the panic in some other function.
  [#1121](rust-lang/git2-rs#1121)
- Credential helper config (loaded with `CredentialHelper::config`) now checks for helpers that start with something that looks like an absolute path, rather than checking for a `/` or `\` anywhere in the helper string (which resolves an issue if the helper had arguments with `/` or `\`).
  [#1137](rust-lang/git2-rs#1137)

### Fixed

- Fixed panic in `Remote::url_bytes` if the url is empty.
  [#1120](rust-lang/git2-rs#1120)
- Fixed incorrect lifetimes on `Patch::delta`, `Patch::hunk`, and `Patch::line_in_hunk`. The return values must not outlive the `Patch`.
  [#1141](rust-lang/git2-rs#1141)
- Bumped requirement to libgit2-sys 0.18.1, which fixes linking of advapi32 on Windows.
  [#1143](rust-lang/git2-rs#1143)

ignore-conflict-markers

Reviewed By: JakobDegen

Differential Revision: D74659779

fbshipit-source-id: a18bcd8f58bc62c7eedbfa5939a791002e18d7bc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss approved these changes

Assignees
No one assigned
Labels
S-waiting-on-review Status: Waiting on review
Projects
None yet
Milestone
No milestone
3 participants
@aibaars @ehuss @rustbot