Re: [PATCH v5 09/15] bugreport: generate config safelist based on docs
From: Martin Ågren <hidden>
Date: 2020-01-30 22:34:38
On Fri, 24 Jan 2020 at 04:35, [off-list ref] wrote:
Add a new step to the build to generate a safelist of git-config variables which are appropriate to include in the output of git-bugreport. New variables can be added to the safelist by annotating their documentation in Documentation/config with the "bugreport" macro, which is recognized by AsciiDoc and AsciiDoctor. Some configs are private in nature, and can contain remote URLs, passwords, or other sensitive information. In the event that a user doesn't notice their information while reviewing a bugreport, that user may leak their credentials to other individuals, mailing lists, or bug tracking tools inadvertently. Heuristic blocklisting of configuration keys is imperfect and prone to false negatives; given the nature of the information which can be leaked, a safelist is more reliable.
I sort of wonder whether safelist/blocklist is to prefer over whitelist/blacklist, or if it's the other way round. The former are new to me, whereas the latter are the terms I would have used. But that's just me, of course. I was a little surprised, that's all.
Implement a new no-op "bugreport" macro for use as "bugreport:include[x]" to annotate the config keys that should be included in the automatically generated safelist. Use "exclude" for the others. With Asciidoctor, it's ok to say "bugreport:include[]", but AsciiDoc seems to want something between the brackets. A bit unfortunate, but not a huge problem -- we'll just provide an "x".
I recognize this reasoning :-) and I'm not terribly opposed to it, but after some nights' sleeping on this, I have to wonder if "annotate:bugreport[include]" wouldn't be better than "bugreport[x]" with that ugly "x". Maybe this isn't the biggest problem, but if we expect this macro to eventually sit right next to ~90% of all our config variables...
"doc-diff" reports that this macro doesn't render at all. That is, these are both empty after this commit: cd Documentation ./doc-diff --asciidoctor :/"bugreport: add tool" HEAD ./doc-diff --asciidoc :/"bugreport: add tool" HEAD
That was true in [1], but alas, no more. In that patch, it's sort of obvious from the diff how it adds a "class" which "end"s. [1] https://lore.kernel.org/git/20190817203846.31609-1-martin.agren@gmail.com/ (local)
quoted hunk ↗ jump to hunk
--- a/Documentation/asciidoctor-extensions.rb +++ b/Documentation/asciidoctor-extensions.rb@@ -37,6 +37,10 @@ module Git output = output.sub(/<\/refmeta>/, new_tags + "</refmeta>") end output + + class BugReportProcessor < Asciidoctor::Extensions::InlineMacroProcessor + def process(parent, action, attrs) + "" end end end
But this one doesn't add an "end", and Asciidoctor trips up badly.
+ # The bugreport macro does nothing as far as rendering is + # concerned -- we just grep for it in the sources. + inline_macro Git::Documentation::BugReportProcessor, :bugreport
(I never much liked this copy-paste comment then, and I still don't, to be honest.) Martin