@matheusbsbs / matheus-bagliano / commits / 4c5971e18a

Fork 0

credential: treat "?" and "#" in URLs as end of host

It's unusual to see: https://example.com?query-parameters without an intervening slash, like: https://example.com/some-path?query-parameters or even: https://example.com/?query-parameters but it is a valid end to the hostname (actually "authority component") according to RFC 3986. Likewise for "#". And curl will parse the URL according to the standard, meaning it will contact example.com, but our credential code would ask about a bogus hostname with a "?" in it. Let's make sure we follow the standard, and more importantly ask about the same hosts that curl will be talking to. It would be nice if we could just ask curl to parse the URL for us. But it didn't grow a URL-parsing API until 7.62, so we'd be stuck with fallback code either way. Plus we'd need this code in the main Git binary, where we've tried to avoid having a link dependency on libcurl. But let's at least fix our parser. Moving to curl's parser would prevent other potential discrepancies, but this gives us immediate relief for the known problem, and would help our fallback code if we eventually use curl. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Apr 14, 2020 at 17:43 UTC 4c5971e18a181c68aec03262fb467cb5d21a5b0d

2 files changed +43 -2

credential.c

+8 -1

@@ -388,7 +388,14 @@ int credential_from_url_gently(struct credential *c, const char *url,

388 cp = proto_end + 3;

389 at = strchr(cp, '@');

390 colon = strchr(cp, ':');

391 - slash = strchrnul(cp, '/');

391 +

392 + /*

393 + * A query or fragment marker before the slash ends the host portion.

394 + * We'll just continue to call this "slash" for simplicity. Notably our

395 + * "trim leading slashes" part won't skip over this part of the path,

396 + * but that's what we'd want.

397 + */

398 + slash = cp + strcspn(cp, "/?#");

399

400 if (!at || slash <= at) {

401 /* Case (1) */

t/t0300-credentials.sh

+35 -1

@@ -443,11 +443,45 @@ test_expect_success 'url parser ignores embedded newlines' '

443 username=askpass-username

444 password=askpass-password

445 --

446 - warning: url contains a newline in its host component: https://one.example.com?%0ahost=two.example.com/

446 + warning: url contains a newline in its path component: https://one.example.com?%0ahost=two.example.com/

447 warning: skipping credential lookup for url: https://one.example.com?%0ahost=two.example.com/

448 askpass: Username:

449 askpass: Password:

450 EOF

451 '

452

453 +# usage: check_host_and_path <url> <expected-host> <expected-path>

454 +check_host_and_path () {

455 + # we always parse the path component, but we need this to make sure it

456 + # is passed to the helper

457 + test_config credential.useHTTPPath true &&

458 + check fill "verbatim user pass" <<-EOF

459 + url=$1

460 + --

461 + protocol=https

462 + host=$2

463 + path=$3

464 + username=user

465 + password=pass

466 + --

467 + verbatim: get

468 + verbatim: protocol=https

469 + verbatim: host=$2

470 + verbatim: path=$3

471 + EOF

472 +}

473 +

474 +test_expect_success 'url parser handles bare query marker' '

475 + check_host_and_path https://example.com?foo.git example.com ?foo.git

476 +'

477 +

478 +test_expect_success 'url parser handles bare fragment marker' '

479 + check_host_and_path https://example.com#foo.git example.com "#foo.git"

480 +'

481 +

482 +test_expect_success 'url parser not confused by encoded markers' '

483 + check_host_and_path https://example.com%23%3f%2f/foo.git \

484 + "example.com#?/" foo.git

485 +'

486 +

487 test_done