Re: [PATCH 3/3] submodule-config.c: strengthen URL fsck check

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2024, 21:57 UTC
Message-ID: <xmqqplyaf9vp.fsf@gitster.g>
In-Reply-To: <893071530d3b77d6b72b7f69a6dfb9947579865e.1704822817.git.gitgitgadget@gmail.com>

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 14 quoted lines

> From: Victoria Dye <vdye@github.com>
>
> Update the validation of "curl URL" submodule URLs (i.e. those that specify
> an "http[s]" or "ftp[s]" protocol) in 'check_submodule_url()' to catch more
> invalid URLs. The existing validation using 'credential_from_url_gently()'
> parses certain URLs incorrectly, leading to invalid submodule URLs passing
> 'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote
> URLs in 'remote_get()' - correctly identifies the invalid URLs missed by
> 'credential_from_url_gently()'.
>
> To catch more invalid cases, replace 'credential_from_url_gently()' with
> 'url_normalize()' followed by a 'url_decode()' and a check for newlines
> (mirroring 'check_url_component()' in the 'credential_from_url_gently()'
> validation).

Thanks. Left hand and right hand checking the same thing in different ways and coming up with different result is never a happy situation. Making sure we consistently use the same definition of what the valid URLs are is a very welcome thing to do, of course.

Show 5 quoted lines

> -test_expect_failure 'check urls' '
> +test_expect_success 'check urls' '
>  	cat >expect <<-\EOF &&
>  	./bar/baz/foo.git
>  	https://example.com/foo.git

It is a bit unfortunate that from here we cannot tell which bogus URLs in this test that were incorrectly accepted are now rejected.

Among the many bogus URLs in the input, we used to allow

    http://example.com:test/foo.git

(we do not accept non-numeric representation of port numbers, so http://example.com:http/foo.git would also be rejected), but with this change, it is now rejected. All the other bogus ones are rejected just as before this change.

Will queue.  Thanks.