Re: [PATCH] http: redact curl h2h3 headers in info

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 11, 2022, 02:35 UTC
Message-ID: <Y22038eJwS6sQN/y@nand.local>
In-Reply-To: <kl6lmt8y31ak.fsf@chooglen-macbookpro.roam.corp.google.com>

On Thu, Nov 10, 2022 at 02:14:11PM -0800, Glen Choo wrote:

Show 14 quoted lines

> >> @@ -560,8 +560,10 @@ static void set_curl_keepalive(CURL *c)
> >>  }
> >>  #endif
> >>
> >> -static void redact_sensitive_header(struct strbuf *header)
> >> +/* Return 0 if redactions been made, 1 otherwise. */
> >
> > Does it make sense to reverse the retval here?
> >
> > `if (!redact_sensitive_header())` sounds like "if not redacted, ..." -
> > but here it means the opposite, right?
>
> I struggled with this for a bit since I wasn't sure what the convention
> is here. Enumerating some off the top of my head, we have:

For what it's worth, the "return zero if redactions were made" is what I would have expected. I think of it as returning zero if we didn't encounter an error (and returning a negative, non-zero value if we did).

Show 10 quoted lines

> - For functions that don't fail we have "0" for "nothing was done" and
>   "1" for something was done (e.g. skip_prefix()).
>
> (Tangent: from a readability perspective, this is pretty poor. I need to
> know beforehand whether or not the function may fail with error before I
> know what the return value means?)
>
> This probably falls into the last category, so for consistency, I think
> this should return "1" for "redactions have happened" (as you
> suggested).

...But I don't really care that much ;-). As long as you choose consistently, and document your choice where it is unclear, it is fine.

Show 21 quoted lines

> >> +/* Redact headers in info */
> >> +static void redact_sensitive_info_header(struct strbuf *header)
> >> +{
> >> +	const char *sensitive_header;
> >> +
> >> +	if (trace_curl_redact &&
> >> +	    skip_iprefix(header->buf, "h2h3 [", &sensitive_header)) {
> >> +		struct strbuf inner = STRBUF_INIT;
> >> +
> >> +		/* Drop the trailing "]" */
> >> +		strbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);
> >> +		if (!redact_sensitive_header(&inner)) {
> >> +			strbuf_setlen(header, strlen("h2h3 ["));
> >> +			strbuf_addbuf(header, &inner);
> >> +			strbuf_addch(header, ']');
> >
> > I'd really like some more comments in this function - even just one
> > describing the string we're trying to redact, or showing a sample line.
> > Navigating string parsing is always a bit difficult.
>
> Ah yes, I should include a description of the string.

Eh. To be honest, I probably wouldn't have documented it any more than you did. At most, I would add an example "before" and "after" string that shows what we're trying to generate.

I agree that string manipulation can end up with some fairly convoluted code. But I think what is written here is straightforward, and that any attempt to comment it more than suggested would end up just repeating what the code does in English.

Thanks, Taylor