Re: [PATCH 14/16] promisor-remote: trust known remotes matching acceptFromServerUrl

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Obviously.
>
>> +3. Don't use globs (`*`) in the domain name. For example
>> +   `https://cdn.example.com/*` is much safer than
>> +   `https://*.example.com/*`, because the latter matches
>> +   `https://evil-hacker.net/fake.example.com/repo`.
>
> Is there a practical use case where allowing '*' to match anything
> that contains a slash '/' is useful?
>
>> +4. Make sure to have a `/` at the end of the domain name (or the end
>> +   of specific directories). For example `https://cdn.example.com/*`
>> +   is much safer than `https://cdn.example.com*`, because the latter
>> +   matches `https://cdn.example.com.hacker.net/repo`.
>
> Ditto.  The above two points sound like excuses to keep sloppy
> asterisk matching logic.  Yes, retroactively tightening rules always
> have risk to break existing deployments, but if existing code paths
> of urlmatch do not have any good reason to allow '*' to match a
> string that contains a slash '/', perhaps there is no fallout.

I probably should caution the readers not to take the above too
literally.  Forbidding an asterisk '*' glob not to match '/'
anywhere in urlmatch will obviusly break existing deployments that
does this

    [http "https://example.com/*"]
	var = val

and expects it to catch any URL pointing into the site.

But I still do think the matcher should be more intelligent than the
current implementation to avoid pitfalls like #3 and #4 above.

Perhaps if an additional rule says that '*' after the scheme:// part
before the first '/' in the pattern, e.g.,

    https://*.example.com/
    https://*.example.com
    https://example.com*

unlike '*' that appear anywhere else, never matches a substring that
contains a slash '/' in it, it would cover plausible mistakes that
the above #3 and #4 are trying to catch, without hurting any real
world use case?