On Tue, Dec 01, 2020 at 10:47:56AM +0100, Patrick Steinhardt wrote:
> > +void git_config_push_env(const char *spec)
> > +{
> > + struct strbuf buf = STRBUF_INIT;
> > + const char *env_name;
> > + const char *env_value;
> > +
> > + env_name = strchr(spec, '=');
> > + if (!env_name)
> > + return; /* die or warn? */
> > + env_name++;
> > +
> > + env_value = getenv(env_name);
> > + if (!env_value)
> > + return; /* die or warn? */
> > +
> > + strbuf_add(&buf, spec, env_name - spec);
> > + strbuf_addstr(&buf, env_value);
> > + git_config_push_parameter(buf.buf);
> > + strbuf_release(&buf);
> > +}
>
> I realize that you say it's yet unpolished, but doesn't this have
> parsing issues? The first strchr(3P) probably needs to be a strrchr(3P)
> to correctly parse `includeIf./home/foo/=repo.path=MY_PATH_ENV`.
Without further changes to $GIT_CONFIG_PARAMETERS, there'd be little
point. The value we put in there has the same parsing issue when read
out of the environment (which we resolve by disallowing "=" in the
subsection, just as here).
I don't think it's actually that big of a deal in practice (it _could_
be an injection source, but it seems somewhat implausible that somebody
is generating complex config keys based on untrusted input). But if we
care, then we could pretty easily change the reading side to separately
quote the key/value in this case:
'foo.subsection=with=equals.bar'='value=with=equals'
And then doing strrchr() would make sense, with the explicitly
documented rule that the environment variable name cannot contain an
equals sign. (Doing a raw "git -c" wouldn't work unless we introduce
another option that lets you specify the key and value separately; that
might be worthwhile, too).
> But we'd also have to handle shell quoting for the user, don't we?
I'm not sure exactly what you mean here. We wouldn't typically see any
shell quoting from the user, since the shell would dequote it and give
us a NUL-terminated argv. Or if you meant we'd have to adjust the shell
quoting in $GIT_CONFIG_PARAMETERS, then see above.
-Peff
