Repository navigation
Commit 181e8ed
Address review feedback about unsafe option abbreviations
The prefix check could reject otherwise allowed one-letter short options when an unrelated blocked long option began with the same character. Keep exact matches for every spelling, but restrict prefix matching to explicit long options and multi-character kwargs.
This addresses the review comment: single-letter short options and kwargs must not be treated as long-option abbreviations. It also adds coverage for abbreviated unsafe kwargs so both clone validation paths remain protected.
Reviewd-by: Sebastian Thiel <sebastian.thiel@icloud.com>1 parent 2ab0516 commit 181e8ed
3 files changed
Lines changed: 29 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -970,12 +970,20 @@ def check_unsafe_options(cls, options: List[str], unsafe_options: List[str]) -> | |||
| 970 | 970 | # Git accepts any unambiguous prefix of a long option, so an abbreviated | |
| 971 | 971 | # spelling such as `--upl` for `--upload-pack` must be rejected too. An | |
| 972 | 972 | # option is unsafe if its canonical name is a prefix of any blocked | |
| 973 | - # option's canonical name. | ||
| 973 | + # option's canonical name. Only long options and multi-character kwargs | ||
| 974 | + # can be abbreviations; single-character short options remain exact-match | ||
| 975 | + # only. | ||
| 974 | 976 | canonical_unsafe_options = {cls._canonicalize_option_name(option): option for option in unsafe_options} | |
| 977 | + options_are_kwargs = all(not option.startswith("-") for option in options) | ||
| 975 | 978 | for option in options: | |
| 976 | 979 | candidate = cls._canonicalize_option_name(option) | |
| 977 | 980 | if not candidate: | |
| 978 | 981 | continue | |
| 982 | + unsafe_option = canonical_unsafe_options.get(candidate) | ||
| 983 | + if unsafe_option is not None: | ||
| 984 | + raise UnsafeOptionError(f"{unsafe_option} is not allowed, use `allow_unsafe_options=True` to allow it.") | ||
| 985 | + if not (option.startswith("--") or (options_are_kwargs and len(candidate) > 1)): | ||
| 986 | + continue | ||
| 979 | 987 | for canonical, unsafe_option in canonical_unsafe_options.items(): | |
| 980 | 988 | if canonical.startswith(candidate): | |
| 981 | 989 | raise UnsafeOptionError( | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -153,6 +153,16 @@ def test_clone_unsafe_options_abbreviated(self, rw_repo): | |||
| 153 | 153 | rw_repo.clone(tmp_dir, multi_options=[unsafe_option]) | |
| 154 | 154 | assert not tmp_file.exists() | |
| 155 | 155 | ||
| 156 | + unsafe_kwargs = [ | ||
| 157 | + {"upl": f"touch {tmp_file}"}, | ||
| 158 | + {"upload_pac": f"touch {tmp_file}"}, | ||
| 159 | + {"conf": "protocol.ext.allow=always"}, | ||
| 160 | + ] | ||
| 161 | + for unsafe_option in unsafe_kwargs: | ||
| 162 | + with self.assertRaises(UnsafeOptionError): | ||
| 163 | + rw_repo.clone(tmp_dir, **unsafe_option) | ||
| 164 | + assert not tmp_file.exists() | ||
| 165 | + | ||
| 156 | 166 | @with_rw_repo("HEAD") | |
| 157 | 167 | def test_clone_unsafe_options_are_checked_after_splitting_multi_options(self, rw_repo): | |
| 158 | 168 | with tempfile.TemporaryDirectory() as tdir: | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -170,6 +170,16 @@ def test_check_unsafe_options_normalizes_kwargs(self): | |||
| 170 | 170 | with self.assertRaises(UnsafeOptionError): | |
| 171 | 171 | Git.check_unsafe_options(options=options, unsafe_options=unsafe_options) | |
| 172 | 172 | ||
| 173 | + def test_check_unsafe_options_does_not_treat_short_options_as_abbreviations(self): | ||
| 174 | + unsafe_options = ["--upload-pack"] | ||
| 175 | + | ||
| 176 | + Git.check_unsafe_options(options=["-u", "u", "-upl"], unsafe_options=unsafe_options) | ||
| 177 | + with self.assertRaises(UnsafeOptionError): | ||
| 178 | + Git.check_unsafe_options(options=["--u"], unsafe_options=unsafe_options) | ||
| 179 | + | ||
| 180 | + def test_check_unsafe_options_does_not_treat_option_values_as_abbreviations(self): | ||
| 181 | + Git.check_unsafe_options(options=["--branch", "conf"], unsafe_options=["--config"]) | ||
| 182 | + | ||
| 173 | 183 | _shell_cases = ( | |
| 174 | 184 | # value_in_call, value_from_class, expected_popen_arg | |
| 175 | 185 | (None, False, False), | |
0 commit comments