On Thu, Jul 11, 2013 at 02:50:03PM -0700, Kyle J. McKay wrote:
> The <url> value is considered a match to a url if the <url> value
> is either an exact match or a prefix of the url which ends on a
> path component boundary ('/'). So "https://example.com/test"; will
> match "https://example.com/test"; and "https://example.com/test/too";
> but not "https://example.com/testextra";.
>
> Longer matches take precedence over shorter matches with
> environment variable settings taking precedence over all.
>
> With this configuration:
>
> [http]
> useragent = other-agent
> noEPSV = true
> [http "https://example.com";]
> useragent = example-agent
> sslVerify = false
> [http "https://example.com/path";]
> useragent = path-agent
I like the general direction you are going, and especially the path
prefix matching is something I had always meant to implement for the
credential code.
A few comments on the implementation:
> +enum http_option_type {
> + opt_post_buffer = 0,
> + opt_min_sessions,
> +#ifdef USE_CURL_MULTI
> + opt_max_requests,
> +#endif
> + opt_ssl_verify,
> + opt_ssl_try,
> + opt_ssl_cert,
> +#if LIBCURL_VERSION_NUM >= 0x070903
> + opt_ssl_key,
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070908
> + opt_ssl_capath,
> +#endif
> + opt_ssl_cainfo,
> + opt_low_speed,
> + opt_low_time,
> + opt_no_epsv,
> + opt_http_proxy,
> + opt_cookie_file,
> + opt_user_agent,
> + opt_passwd_req,
> + opt_max
> +};
We usually put enum fields in ALL_CAPS to mark them as constants (though
you can find few exceptions in the code).
> +static size_t http_options_url_match_prefix(const char *url,
> + const char *url_prefix,
> + size_t url_prefix_len)
> +{
It looks like you're matching the URLs as raw strings, and I don't see
any canonicalization going on. What happens if I have
"https://example.com/foo+bar"; in my config, but then I visit
"https://example.comfoo%20bar";?
Or what about optional components? If I have "https://example.com"; in my
config, but I am visiting "https://[email protected]/";, shouldn't it
match? The config spec is more general than my specific URL; you would
not want it to match in the opposite direction, though.
I think you would want to break down the URL into fields, canonicalize
each field by undoing any encoding, and then compare the broken-down
URLs, as credential_match does (adding in your prefix/boundary matching to
the path instead of a straight string comparison).
I think you could mostly reuse the code there by doing:
1. Create a "struct url" that contains the broken-down form; "struct
credential" would contain a url.
2. Pull out credential_from_url into "int url_parse(struct url *,
const char *)".
3. Pull out credential_match into "int url_match(struct url *, struct
url *)".
4. Parse and compare URLs from "http.$URL.*" during the config read
(similar to credential_config_callback).
That does make your by-length ordering impossible, but I don't think you
can do it in the general case. If I have:
[http "http://[email protected]";] foo = 1
[http "http://example.com:8080";] foo = 2
and I visit "http://[email protected]:8080";, which one is the winner? I
don't think there is an unambiguous definition. I'd suggest instead just
using the usual "last one wins" strategy that our config uses. It has
the unfortunate case that:
[http "http://example.com";]
foo = 1
[http]
foo = 2
will always choose http.foo=2, but I don't think that is a big problem
in practice. You often only set one or the other, and in the cases where
you want to have a non-standard default and then override it with
another host-specific case, the "last one wins" rule makes it simple to
explain that you need to specify keys in most-general to most-specific
order.
> static int http_options(const char *var, const char *value, void *cb)
> {
> - if (!strcmp("http.sslverify", var)) {
> + const char *url = (const char *)cb;
No need to cast from void, this isn't C++. :)
The rest of the http_options() changes look like what I'd expect.
> @@ -344,7 +479,7 @@ void http_init(struct remote *remote, const char *url,
> int proactive_auth)
>
> http_is_verbose = 0;
>
> - git_config(http_options, NULL);
> + git_config(http_options, (void *)url);
No need to cast again. :)
So this is the URL that we got on the command line. Which means that if
we get a redirect, we will not re-examine the options. I think that's
OK; we do not even _see_ the redirect, as it happens internally within
curl. The credential code has the same problem, but it is not a security
issue because curl clears the credentials on redirect.
However, it may be worth mentioning in the documentation that the config
options operate on the URL you give git, _not_ necessarily on the actual
URL you are hitting at any given moment (the gitcredentials(7) page
should probably make the same distinction).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
