Re: [PATCH 1/2] bugreport: generate config safelist based on docs

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Jun 23, 2020 at 09:35:15PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > ... To mark a config as safe,
> > add "annotate:bugreport[include]" to the corresponding line in the
> > config documentation; to mark it as unsafe, add
> > "annotate:bugreport[exclude]" instead.
> 
> Hmph,...
> 
> > -sendemail.smtpEncryption::
> > +sendemail.smtpEncryption annotate:bugreport[include] ::
> >  	See linkgit:git-send-email[1] for description.  Note that this
> >  	setting is not subject to the 'identity' mechanism.
> >  
> > @@ -15,7 +15,7 @@ sendemail.smtpsslcertpath::
> >  	Path to ca-certificates (either a directory or a single file).
> >  	Set it to an empty string to disable certificate verification.
> >  
> > -sendemail.<identity>.*::
> > +sendemail.<identity>.* annotate:bugreport[exclude] ::
> 
> So "sendemail.git-devel.cc" is not included due to [exclude] here,
> but ...

Hm, I can change this for the example, but I think it may still not be
included correctly because of the wildcard.  We had talked about whether
to include wildcarded options like this in the past too - I think that's
part of why I removed these couple patches from the initial bugreport
series.

If this line becomes included, then what we're really searching for is
anything matching "sendemail.*.*"; is this syntax regular enough to
count on in the generator script? Could the generator script do some
magic to turn <foo> and * into valid regex in the generated header and
then use that regex during the config parse?

Maybe using regex is overkill and I should just check for "*" during the
parse. Or maybe there's already a library to use. I'll dig into this
some today. Thanks for pointing it out.

> 
> > +sendemail.annotate annotate:bugreport[include] ::
> > +sendemail.bcc annotate:bugreport[include] ::
> > +sendemail.cc annotate:bugreport[include] ::
> 
> ... "sendemail.cc" that is a fallback value for other "sendemail.*.cc"
> is included?  
> 
> > +++ b/generate-bugreport-config-safelist.sh
> > @@ -0,0 +1,18 @@
> > +#!/bin/sh
> > +
> > +cat <<"EOF"
> > +/* Automatically generated by bugreport-generate-config-safelist.sh */
> > +
> > +
> > +static const char *bugreport_config_safelist[] = {
> > +EOF
> > +
> > +# cat all regular files in Documentation/config
> > +find Documentation/config -type f -exec cat {} \; |
> > +# print the command name which matches the annotate-bugreport macro
> > +sed -n 's/^\([^ ]*\)  *annotate:bugreport\[include\].* ::$/  "\1",/p' |
> > +	sort
> 
> We just care about "include" entries, so it does not matter whether
> we mark entries with [exclude] or not anyway?
> 
> Puzzled...

I wonder where I forgot to include the context for encouraging
"exclude". My thinking was that some organizations might have a lower
expectation of employee privacy, and therefore opt to use a blocklist
rather than a safelist for bug reports included internally. I suppose I
was thinking then that organization only needs to carry a patch to
invert the generator script, and not have to comb through all the
configs they care or don't care about themselves.

But as I'm revisiting this, it seems like that means a user who works
for Big Brother Corp but is told to "file that bug upstream" could then
leak their confidential info by accident, not realizing their
git-bugreport is using a wider net than upstream wants.

What do others think?

 - Emily