GeezerVison is showing github.com — review · gitpython-developers/GitPython@f89c343
File treeExpand file treeCollapse file tree Open diff view settings Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: git/util.py +14-14Lines changed: 14 additions & 14 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -672,9 +672,9 @@ def _redact_url_credentials(text: str) -> Optional[str]: | 672 | 672 | def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: | 673 | 673 | """Redact credentials in URLs and HTTP Authorization extra headers in a command line. | 674 | 674 |
| 675 | | - URLs are also redacted when they are the value of a configuration assignment, such | 676 | | - as ``http.proxy=<url>``, and assignments are recognized in both the ``-c <key>=<value>`` | 677 | | - and the ``--config=<key>=<value>`` form. | | 675 | + URLs are also redacted in the key and value of a configuration assignment, such | | 676 | + as ``http.proxy=<url>``. Assignments are recognized in the ``-c <key>=<value>``, | | 677 | + ``-c<key>=<value>``, and ``--config=<key>=<value>`` forms. | 678 | 678 |
| 679 | 679 | If nothing is found, this just returns the command line as-is. | 680 | 680 |
| @@ -683,8 +683,15 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: | 683 | 683 | """ | 684 | 684 | new_cmdline = [] | 685 | 685 | for to_parse in cmdline: | 686 | | - option = "--config=" if to_parse.startswith("--config=") else "" | | 686 | + if to_parse.startswith("--config="): | | 687 | + option = "--config=" | | 688 | + elif to_parse.startswith("-c"): | | 689 | + option = "-c" | | 690 | + else: | | 691 | + option = "" | 687 | 692 | assignment = to_parse[len(option) :] | | 693 | + # Redact the whole URL first, before splitting on a possible '=' in userinfo. | | 694 | + assignment = _redact_url_credentials(assignment) or assignment | 688 | 695 | config_key, separator, value = assignment.partition("=") | 689 | 696 | header_name, colon, _ = value.partition(":") | 690 | 697 | if ( | @@ -696,16 +703,9 @@ def remove_password_if_present(cmdline: Sequence[str]) -> List[str]: | 696 | 703 | new_cmdline.append("%s%s%s%s%s *****" % (option, config_key, separator, header_name, colon)) | 697 | 704 | continue | 698 | 705 |
| 699 | | - redacted = _redact_url_credentials(to_parse) | 700 | | - if redacted is None and option: | 701 | | - redacted = _redact_url_credentials(assignment) | 702 | | - if redacted is not None: | 703 | | - redacted = option + redacted | 704 | | - if redacted is None and separator: | 705 | | - redacted = _redact_url_credentials(value) | 706 | | - if redacted is not None: | 707 | | - redacted = option + config_key + separator + redacted | 708 | | - new_cmdline.append(to_parse if redacted is None else redacted) | | 706 | + if separator: | | 707 | + value = _redact_url_credentials(value) or value | | 708 | + new_cmdline.append(option + config_key + separator + value) | 709 | 709 | return new_cmdline | 710 | 710 |
| 711 | 711 |
| |
Collapse file Copy file name to clipboardExpand all lines: test/test_git.py +33-2Lines changed: 33 additions & 2 deletions | Original file line number | Diff line number | Diff line change |
|---|
| 10 | 10 | import logging | 11 | 11 | import os | 12 | 12 | import os.path as osp | 13 | | -from pathlib import Path | 14 | 13 | import pickle | 15 | 14 | import re | 16 | 15 | import shutil | | 20 | 19 | import tempfile | 21 | 20 | import threading | 22 | 21 | import time | | 22 | +from pathlib import Path | 23 | 23 | from unittest import mock, skipUnless | 24 | 24 |
| 25 | 25 | import ddt | 26 | 26 |
| 27 | 27 | from git import Git, GitCommandError, GitCommandNotFound, Repo, cmd, refresh | 28 | 28 | from git.exc import UnsafeOptionError | 29 | 29 | from git.util import cwd, finalize_process | 30 | | - | 31 | 30 | from test.lib import TestBase, fixture_path, with_rw_directory | 32 | 31 |
| 33 | 32 |
| @@ -547,6 +546,38 @@ def test_it_redacts_authorization_extra_header_from_error(self): | 547 | 546 | self.assertNotIn(token, message) | 548 | 547 | self.assertIn("http.extraHeader=Authorization: *****", message) | 549 | 548 |
| | 549 | + @ddt.data( | | 550 | + ( | | 551 | + | | 552 | + "--config=http.https://*****@host.proxy=http://*****:*****@proxy", | | 553 | + ), | | 554 | + ( | | 555 | + "-curl.https://user:token@host/.insteadOf=https://old/", | | 556 | + "-curl.https://*****:*****@host/.insteadOf=https://old/", | | 557 | + ), | | 558 | + ) | | 559 | + @ddt.unpack | | 560 | + def test_clone_redacts_config_urls_from_trace_and_error(self, option, expected): | | 561 | + for trace in (True, "full"): | | 562 | + with self.subTest(trace=trace), tempfile.TemporaryDirectory() as directory: | | 563 | + with mock.patch.object(Git, "GIT_PYTHON_TRACE", trace): | | 564 | + with self.assertLogs("git.cmd", level="INFO") as logs: | | 565 | + with self.assertRaises(GitCommandError) as context: | | 566 | + Repo.clone_from( | | 567 | + osp.join(directory, "missing-source"), | | 568 | + osp.join(directory, "destination"), | | 569 | + multi_options=[option], | | 570 | + allow_unsafe_options=True, | | 571 | + ) | | 572 | + self.assertIn(expected, context.exception.command) | | 573 | + message = str(context.exception) | | 574 | + assert logs is not None | | 575 | + log_output = " ".join(logs.output) | | 576 | + self.assertIn(expected, log_output) | | 577 | + for credential in ("alice", "bob", "user", "token"): | | 578 | + self.assertNotIn(credential, message) | | 579 | + self.assertNotIn(credential, log_output) | | 580 | + | 550 | 581 | def test_it_accepts_environment_variables(self): | 551 | 582 | filename = fixture_path("ls_tree_empty") | 552 | 583 | with open(filename, "r") as fh: | |
Collapse file Copy file name to clipboardExpand all lines: test/test_util.py +30Lines changed: 30 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -849,6 +849,7 @@ def test_remove_empty_password_keeps_host_intact(self): | 849 | 849 | ("//user:pass@[2001:db8::1]:0080/repo", "//*****:*****@[2001:db8::1]:0080/repo"), | 850 | 850 | | 851 | 851 | ("https://user:@example.com/repo", "https://*****:*****@example.com/repo"), | | 852 | + | 852 | 853 | ("https://@example.com/repo", "https://*****@example.com/repo"), | 853 | 854 | ("https://example.com/repo@name?q=a@b#c@d", "https://example.com/repo@name?q=a@b#c@d"), | 854 | 855 | ) | @@ -877,7 +878,36 @@ def test_remove_password_preserves_url_components(self, url, expected): | 877 | 878 | ["--config=http.extraHeader=Authorization: Bearer pass"], | 878 | 879 | ["--config=http.extraHeader=Authorization: *****"], | 879 | 880 | ), | | 881 | + ( | | 882 | + | | 883 | + ["--config=http.https://*****@host.proxy=http://*****:*****@proxy"], | | 884 | + ), | | 885 | + ( | | 886 | + | | 887 | + ["-c", "http.https://*****@host.proxy=http://*****:*****@proxy"], | | 888 | + ), | | 889 | + ( | | 890 | + | | 891 | + ["-chttp.https://*****@host.proxy=http://*****:*****@proxy"], | | 892 | + ), | | 893 | + ( | | 894 | + ["-curl.https://user:token@host/.insteadOf=https://old/"], | | 895 | + ["-curl.https://*****:*****@host/.insteadOf=https://old/"], | | 896 | + ), | | 897 | + ( | | 898 | + ["-chttp.proxy=http://bob:token@proxy"], | | 899 | + ["-chttp.proxy=http://*****:*****@proxy"], | | 900 | + ), | | 901 | + ( | | 902 | + ["--config=http.https://alice:token@host/.extraHeader=Authorization: Bearer pass"], | | 903 | + ["--config=http.https://*****:*****@host/.extraHeader=Authorization: *****"], | | 904 | + ), | | 905 | + ( | | 906 | + ["-chttp.extraHeader=Authorization: Bearer pass"], | | 907 | + ["-chttp.extraHeader=Authorization: *****"], | | 908 | + ), | 880 | 909 | (["-c", "user.name=pass"], ["-c", "user.name=pass"]), | | 910 | + (["-cuser.name=pass"], ["-cuser.name=pass"]), | 881 | 911 | ) | 882 | 912 | @ddt.unpack | 883 | 913 | def test_remove_password_from_config_values(self, cmdline, expected): | |
|