{"thread":{"id":"60233","subject":"[PATCH 0/2] updating curl http/2 header matching (again)","startedAt":"2023-09-15T11:32:41Z","lastAt":"2023-09-19T17:56:57Z","messageCount":10,"participants":["Jeff King","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"481871","messageId":"20230915113237.GA3531328@coredump.intra.peff.net","threadId":"60233","inReplyTo":null,"subject":"[PATCH 0/2] updating curl http/2 header matching (again)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-15T11:32:37Z","receivedAt":"2023-09-15T11:32:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is similar to the issue discussed/fixed a few months ago in:\n\n https://lore.kernel.org/git/20230617051559.GD562686@coredump.intra.peff.net/\n\nbut it looks like curl has updated the trace line format again. This\nupdates our matching code to handle it. It would obviously be nice to\navoid the dependency altogether, but I don't think there is another\noption here. I expected the previous update to last longer than it did,\nbut hopefully things will settle a bit more now. :)\n\n(If anyone wants to confirm the bug or test, the new version of curl\njust hit debian unstable).\n\n [1/2]: http: factor out matching of curl http/2 trace lines\n [2/2]: http: update curl http/2 info matching for curl 8.3.0\n\n http.c | 37 +++++++++++++++++++++++++++++++------\n 1 file changed, 31 insertions(+), 6 deletions(-)\n\n-Peff\n"},{"id":"481872","messageId":"20230915113316.GA3531587@coredump.intra.peff.net","threadId":"60233","inReplyTo":"20230915113237.GA3531328@coredump.intra.peff.net","subject":"[PATCH 1/2] http: factor out matching of curl http/2 trace lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-15T11:33:16Z","receivedAt":"2023-09-15T11:33:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have to parse out curl's http/2 trace lines so we can redact their\nheaders. We already match two different types of lines from various\nvintages of curl. In preparation for adding another (which will be\nslightly more complex), let's pull the matching into its own function,\nrather than doing it in the middle of a conditional.\n\nWhile we're doing so, let's expand the comment a bit to describe the two\nmatches. That probably should have been part of db30130165 (http: handle\nboth \"h2\" and \"h2h3\" in curl info lines, 2023-06-17), but will become\neven more important as we add new types.\n\nSigned-off-by: Jeff King \n---\n http.c | 23 +++++++++++++++++------\n 1 file changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex e138b4b96f..c93e0a7ea6 100644\n--- a/http.c\n+++ b/http.c\n@@ -738,18 +738,29 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)\n \treturn ret;\n }\n \n+static int match_curl_h2_trace(const char *line, const char **out)\n+{\n+\t/*\n+\t * curl prior to 8.1.0 gives us:\n+\t *\n+\t * h2h3 [: ]\n+\t *\n+\t * Starting in 8.1.0, the first token became just \"h2\".\n+\t */\n+\tif (skip_iprefix(line, \"h2h3 [\", out) ||\n+\t skip_iprefix(line, \"h2 [\", out))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n /* Redact headers in info */\n static void redact_sensitive_info_header(struct strbuf *header)\n {\n \tconst char *sensitive_header;\n \n-\t/*\n-\t * curl's h2h3 prints headers in info, e.g.:\n-\t * h2h3 [: ]\n-\t */\n \tif (trace_curl_redact &&\n-\t (skip_iprefix(header->buf, \"h2h3 [\", &sensitive_header) ||\n-\t skip_iprefix(header->buf, \"h2 [\", &sensitive_header))) {\n+\t match_curl_h2_trace(header->buf, &sensitive_header)) {\n \t\tif (redact_sensitive_header(header, sensitive_header - header->buf)) {\n \t\t\t/* redaction ate our closing bracket */\n \t\t\tstrbuf_addch(header, ']');\n-- \n2.42.0.661.g2507eb519e\n\n"},{"id":"481873","messageId":"20230915113443.GB3531587@coredump.intra.peff.net","threadId":"60233","inReplyTo":"20230915113237.GA3531328@coredump.intra.peff.net","subject":"[PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-15T11:34:43Z","receivedAt":"2023-09-15T11:34:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To redact header lines in http/2 curl traces, we have to parse past some\nprefix bytes that curl sticks in the info lines it passes to us. That\nchanged once already, and we adapted in db30130165 (http: handle both\n\"h2\" and \"h2h3\" in curl info lines, 2023-06-17).\n\nNow it has changed again, in curl's fbacb14c4 (http2: cleanup trace\nmessages, 2023-08-04), which was released in curl 8.3.0. Running a build\nof git linked against that version will fail to redact the trace (and as\nbefore, t5559 notices and complains).\n\nThe format here is a little more complicated than the other ones, as it\nnow includes a \"stream id\". This is not constant but is always numeric,\nso we can easily parse past it.\n\nWe'll continue to match the old versions, of course, since we want to\nwork with many different versions of curl. We can't even select one\nformat at compile time, because the behavior depends on the runtime\nversion of curl we use, not the version we build against.\n\nSigned-off-by: Jeff King \n---\n http.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex c93e0a7ea6..8f71bf00d8 100644\n--- a/http.c\n+++ b/http.c\n@@ -740,6 +740,8 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)\n \n static int match_curl_h2_trace(const char *line, const char **out)\n {\n+\tconst char *p;\n+\n \t/*\n \t * curl prior to 8.1.0 gives us:\n \t *\n@@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)\n \t skip_iprefix(line, \"h2 [\", out))\n \t\treturn 1;\n \n+\t/*\n+\t * curl 8.3.0 uses:\n+\t * [HTTP/2] [] [: ]\n+\t * where is numeric.\n+\t */\n+\tif (skip_iprefix(line, \"[HTTP/2] [\", &p)) {\n+\t\twhile (isdigit(*p))\n+\t\t\tp++;\n+\t\tif (skip_prefix(p, \"] [\", out))\n+\t\t\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \n-- \n2.42.0.661.g2507eb519e\n"},{"id":"481885","messageId":"xmqqsf7fe1q4.fsf@gitster.g","threadId":"60233","inReplyTo":"20230915113443.GB3531587@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-15T18:21:55Z","receivedAt":"2023-09-15T18:23:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> @@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)\n> \t skip_iprefix(line, \"h2 [\", out))\n> \t\treturn 1;\n> \n> +\t/*\n> +\t * curl 8.3.0 uses:\n> +\t * [HTTP/2] [] [: ]\n> +\t * where is numeric.\n> +\t */\n> +\tif (skip_iprefix(line, \"[HTTP/2] [\", &p)) {\n> +\t\twhile (isdigit(*p))\n> +\t\t\tp++;\n> +\t\tif (skip_prefix(p, \"] [\", out))\n> +\t\t\treturn 1;\n> +\t}\n\nLooking good assuming that part will never be updated to\nallow spaces around the ID, or allow non-digits in the ID, in the\nfuture. Is there much harm if this code allowed false positives and\nsent something that is *not* a curl trace, like \"foo]\" parsed out of\n\"[HTTP/2] [PATCH] [foo]\", to redact_sensitive_header() function?\n\nBy the way, would this patch make sense? Everybody in the function\nthat try to notice a sensitive header seems to check the sentting\nindependently, which seems error prone for those who want to add a\nnew header to redact.\n\n http.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git c/http.c w/http.c\nindex 8f71bf00d8..3dfa34fe65 100644\n--- c/http.c\n+++ w/http.c\n@@ -684,8 +684,10 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)\n \tint ret = 0;\n \tconst char *sensitive_header;\n \n-\tif (trace_curl_redact &&\n-\t (skip_iprefix(header->buf + offset, \"Authorization:\", &sensitive_header) ||\n+\tif (!trace_curl_redact)\n+\t\treturn ret;\n+\n+\tif ((skip_iprefix(header->buf + offset, \"Authorization:\", &sensitive_header) ||\n \t skip_iprefix(header->buf + offset, \"Proxy-Authorization:\", &sensitive_header))) {\n \t\t/* The first token is the type, which is OK to log */\n \t\twhile (isspace(*sensitive_header))\n@@ -696,8 +698,7 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)\n \t\tstrbuf_setlen(header, sensitive_header - header->buf);\n \t\tstrbuf_addstr(header, \" \");\n \t\tret = 1;\n-\t} else if (trace_curl_redact &&\n-\t\t skip_iprefix(header->buf + offset, \"Cookie:\", &sensitive_header)) {\n+\t} else if (skip_iprefix(header->buf + offset, \"Cookie:\", &sensitive_header)) {\n \t\tstruct strbuf redacted_header = STRBUF_INIT;\n \t\tconst char *cookie;\n \n"},{"id":"481887","messageId":"ZQSiWXm3y07afjCv@nand.local","threadId":"60233","inReplyTo":"20230915113237.GA3531328@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] updating curl http/2 header matching (again)","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-15T18:28:41Z","receivedAt":"2023-09-15T18:29:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 15, 2023 at 07:32:37AM -0400, Jeff King wrote:\n> (If anyone wants to confirm the bug or test, the new version of curl\n> just hit debian unstable).\n\nThese two patches look sensible, and I can confirm that they resolve the\nissue with the newest version of curl. With the latest 'master' (which\nis bda494f404 (The ninth batch, 2023-09-14), at the time of writing) and\nan old version of curl, I get:\n\n $ ./t5559-http-fetch-smart-http2.sh -vdi\n [...]\n expecting success of 5559.17 'GIT_TRACE_CURL redacts auth details':\n rm -rf redact-auth trace &&\n set_askpass user@host pass@host &&\n GIT_TRACE_CURL=\"$(pwd)/trace\" git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n expect_askpass both user@host &&\n\n # Ensure that there is no \"Basic\" followed by a base64 string, but that\n # the auth details are redacted\n ! grep -i \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n grep -i \"Authorization: Basic \" trace\n\n Cloning into bare repository 'redact-auth'...\n remote: Enumerating objects: 6, done.\n remote: Counting objects: 100% (6/6), done.\n remote: Compressing objects: 100% (2/2), done.\n remote: Total 6 (delta 0), reused 0 (delta 0), pack-reused 0\n Receiving objects: 100% (6/6), done.\n == Info: [HTTP/2] [3] [authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==]\n == Info: [HTTP/2] [5] [authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==]\n == Info: [HTTP/2] [7] [authorization: Basic dXNlckBob3N0OnBhc3NAaG9zdA==]\n not ok 17 - GIT_TRACE_CURL redacts auth details\n #\n #\t\trm -rf redact-auth trace &&\n #\t\tset_askpass user@host pass@host &&\n #\t\tGIT_TRACE_CURL=\"$(pwd)/trace\" git clone --bare \"$HTTPD_URL/auth/smart/repo.git\" redact-auth &&\n #\t\texpect_askpass both user@host &&\n #\n #\t\t# Ensure that there is no \"Basic\" followed by a base64 string, but that\n #\t\t# the auth details are redacted\n #\t\t! grep -i \"Authorization: Basic [0-9a-zA-Z+/]\" trace &&\n #\t\tgrep -i \"Authorization: Basic \" trace\n #\n\nand after applying these patches, those tests pass.\n\n $ ./t5559-http-fetch-smart-http2.sh -di\n [...]\n # passed all 54 test(s)\n 1..54\n\nThanks,\nTaylor\n"},{"id":"481888","messageId":"ZQSikid1kgbjCDJV@nand.local","threadId":"60233","inReplyTo":"20230915113316.GA3531587@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] http: factor out matching of curl http/2 trace lines","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-15T18:29:38Z","receivedAt":"2023-09-15T18:30:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 15, 2023 at 07:33:16AM -0400, Jeff King wrote:\n> We have to parse out curl's http/2 trace lines so we can redact their\n> headers. We already match two different types of lines from various\n> vintages of curl. In preparation for adding another (which will be\n> slightly more complex), let's pull the matching into its own function,\n> rather than doing it in the middle of a conditional.\n>\n> While we're doing so, let's expand the comment a bit to describe the two\n> matches. That probably should have been part of db30130165 (http: handle\n> both \"h2\" and \"h2h3\" in curl info lines, 2023-06-17), but will become\n> even more important as we add new types.\n>\n> Signed-off-by: Jeff King \n\nMakes sense, and this sets us up well for the next step in fixing the\ntest fallout from newer versions of curl.\n\nThanks,\nTaylor\n"},{"id":"481889","messageId":"ZQSkjiyrOac4DK8q@nand.local","threadId":"60233","inReplyTo":"20230915113443.GB3531587@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-15T18:38:06Z","receivedAt":"2023-09-15T18:38:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 15, 2023 at 07:34:43AM -0400, Jeff King wrote:\n> @@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)\n> \t skip_iprefix(line, \"h2 [\", out))\n> \t\treturn 1;\n>\n> +\t/*\n> +\t * curl 8.3.0 uses:\n> +\t * [HTTP/2] [] [: ]\n> +\t * where is numeric.\n> +\t */\n> +\tif (skip_iprefix(line, \"[HTTP/2] [\", &p)) {\n> +\t\twhile (isdigit(*p))\n> +\t\t\tp++;\n> +\t\tif (skip_prefix(p, \"] [\", out))\n> +\t\t\treturn 1;\n> +\t}\n> +\n\nThis looks good, too, though I do have one question. The HTTP/2\nspecification in 5.1 says (among other things):\n\n Streams are identified with an unsigned 31-bit integer. Streams\n initiated by a client MUST use odd-numbered stream identifiers; those\n initiated by the server MUST use even-numbered stream identifiers. A\n stream identifier of zero (0x0) is used for connection control messages;\n the stream identifier of zero cannot be used to establish a new stream.\n\nSo the parsing you wrote here makes sense in that we consume digits\nbetween the pair of square brackets enclosing the stream identifier.\n\nBut I think we would happily eat a line like:\n\n [HTTP/2] [] [Secret: xyz]\n\neven lacking a stream identifier. I think that's reasonably OK in\npractice, because we're being over-eager in redacting instead of the\nother way around. And we're unlikely to see such a line from curl\nanyway, so I don't think that it matters.\n\nIf you feel otherwise, though, I think something as simple as:\n\n if (skip_iprefix(line, \"[HTTP/2] [\", &p)) {\n if (!*p)\n return 0;\n while (isdigit(*p))\n p++;\n if (skip_prefix(p, \"] [\", out))\n return 1;\n }\n\nwould do the trick. I *think* that this would also work:\n\n if (skip_iprefix(line, \"[HTTP/2] [\", &p)) {\n do {\n p++;\n } while (isdigit(*p))\n if (skip_prefix(p, \"] [\", out))\n return 1;\n }\n\nsince we know that p is non-NULL, and if it's the end of the line, *p\nwill be NUL and isdigit(*p) will return 0. But it's arguably less\ndirect, and requires some extra reasoning, so I have a vague preference\nfor the former.\n\nBut this may all be moot anyway, I don't feel strongly one way or the\nother.\n\nThanks,\nTaylor\n"},{"id":"481915","messageId":"20230916052540.GA13092@coredump.intra.peff.net","threadId":"60233","inReplyTo":"xmqqsf7fe1q4.fsf@gitster.g","subject":"Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-16T05:25:40Z","receivedAt":"2023-09-16T05:26:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 15, 2023 at 11:21:55AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > @@ -751,6 +753,18 @@ static int match_curl_h2_trace(const char *line, const char **out)\n> > \t skip_iprefix(line, \"h2 [\", out))\n> > \t\treturn 1;\n> > \n> > +\t/*\n> > +\t * curl 8.3.0 uses:\n> > +\t * [HTTP/2] [] [: ]\n> > +\t * where is numeric.\n> > +\t */\n> > +\tif (skip_iprefix(line, \"[HTTP/2] [\", &p)) {\n> > +\t\twhile (isdigit(*p))\n> > +\t\t\tp++;\n> > +\t\tif (skip_prefix(p, \"] [\", out))\n> > +\t\t\treturn 1;\n> > +\t}\n> \n> Looking good assuming that part will never be updated to\n> allow spaces around the ID, or allow non-digits in the ID, in the\n> future. Is there much harm if this code allowed false positives and\n> sent something that is *not* a curl trace, like \"foo]\" parsed out of\n> \"[HTTP/2] [PATCH] [foo]\", to redact_sensitive_header() function?\n\nThe current code on the generating side is pretty strict. It's\nliterally a printf using \"[HTTP/2] [%d] [%.*s: %.*s]\". As far as future\nchanges, I'm hesitant to make any changes based on guesses of what\n_could_ happen. Our chance of hitting the mark is not high (I never\nwould have dreamed about this format after seeing the existing h2h3\nones), and it always carries the risk of misinterpretation.\n\nYou are right that the cost of a false positive is probably not too high\n(the absolute worst case is that we redact something that looks\nheader-ish in the trace output). But even still, I'd prefer not to\ncomplicate the code with extra parsing for a format that may or may not\never come to exist.\n\nIf we were to loosen the parsing, it would make more sense to me to\nloosen _much_ more, and just look for anything inside brackets.\nSomething like:\n\n\tp = header->buf;\n\twhile ((p = strchr(p, '['))) {\n\t\tif (redact_sensitive_header(header, p - header->buf + 1)) {\n\t\t\t/* redaction ate our closing bracket */\n\t\t\tstrbuf_addch(header, ']');\n\t\t\tbreak;\n\t\t}\n\t\tp++; /* skip past to look for next opening bracket */\n\t}\n\nThen we are relying on redact_sensitive_header() to match the header\nstrings, and we'll pass it lots of garbage which it will reject. But at\nleast we've bought something: all of the h2 formats we know about will\njust work, and any future ones which retain the bracketing will as well.\n\nThat said, I'm still somewhat inclined to the stricter parsing, just\nbecause it's possible for us to see arbitrary bytes here. So if you had\na header that happened to have brackets in it, we'd match those.\nProbably nothing too bad could come of it, but it just feels sloppy to\nme.\n\n> By the way, would this patch make sense? Everybody in the function\n> that try to notice a sensitive header seems to check the sentting\n> independently, which seems error prone for those who want to add a\n> new header to redact.\n> [...]\n> +\tif (!trace_curl_redact)\n> +\t\treturn ret;\n\nYeah, that looks a reasonable simplification to me (though obviously\northogonal to the patch under discussion).\n\n-Peff\n"},{"id":"481916","messageId":"20230916053201.GB13092@coredump.intra.peff.net","threadId":"60233","inReplyTo":"ZQSkjiyrOac4DK8q@nand.local","subject":"Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-16T05:32:01Z","receivedAt":"2023-09-16T05:32:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 15, 2023 at 02:38:06PM -0400, Taylor Blau wrote:\n\n> This looks good, too, though I do have one question. The HTTP/2\n> specification in 5.1 says (among other things):\n> \n> Streams are identified with an unsigned 31-bit integer. Streams\n> initiated by a client MUST use odd-numbered stream identifiers; those\n> initiated by the server MUST use even-numbered stream identifiers. A\n> stream identifier of zero (0x0) is used for connection control messages;\n> the stream identifier of zero cannot be used to establish a new stream.\n> \n> So the parsing you wrote here makes sense in that we consume digits\n> between the pair of square brackets enclosing the stream identifier.\n\nYes, though I'm less concerned with what the standard says than with\nwhat curl's code does (and it uses %d).\n\n> But I think we would happily eat a line like:\n> \n> [HTTP/2] [] [Secret: xyz]\n> \n> even lacking a stream identifier. I think that's reasonably OK in\n> practice, because we're being over-eager in redacting instead of the\n> other way around. And we're unlikely to see such a line from curl\n> anyway, so I don't think that it matters.\n\nYes, you're correct that we'd allow an empty stream identifier. I'm\ncontent to leave it in the name of simplicity.\n\n> If you feel otherwise, though, I think something as simple as:\n> \n> if (skip_iprefix(line, \"[HTTP/2] [\", &p)) {\n> if (!*p)\n> return 0;\n> while (isdigit(*p))\n> p++;\n> if (skip_prefix(p, \"] [\", out))\n> return 1;\n> }\n\nYes, that would work, but...\n\n> would do the trick. I *think* that this would also work:\n> \n> if (skip_iprefix(line, \"[HTTP/2] [\", &p)) {\n> do {\n> p++;\n> } while (isdigit(*p))\n> if (skip_prefix(p, \"] [\", out))\n> return 1;\n> }\n>\n> since we know that p is non-NULL, and if it's the end of the line, *p\n> will be NUL and isdigit(*p) will return 0. But it's arguably less\n> direct, and requires some extra reasoning, so I have a vague preference\n> for the former.\n\nYour do-while is too eager, I think. It advances the first \"p\" before\nwe've looked at it, so:\n\n - we'd match \"[HTTP/2] [x1] [foo]\", allowing one byte of non-digit\n cruft\n\n - if the string is \"[HTTP/2] [\", then \"p\" is at the NUL after the\n skip_iprefix call, and p++ walks us off the end of the array.\n\n> But this may all be moot anyway, I don't feel strongly one way or the\n> other.\n\nMy inclination is to leave it. I was actually tempted to just allow\n_anything_ in the brackets if only because it makes the code even\nsimpler, but the \"skip past digits\" seemed like a reasonable middle\nground.\n\n-Peff\n"},{"id":"482020","messageId":"ZQng4UNugsw6eKhq@nand.local","threadId":"60233","inReplyTo":"20230916053201.GB13092@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] http: update curl http/2 info matching for curl 8.3.0","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-19T17:56:49Z","receivedAt":"2023-09-19T17:56:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Sep 16, 2023 at 01:32:01AM -0400, Jeff King wrote:\n> > But I think we would happily eat a line like:\n> >\n> > [HTTP/2] [] [Secret: xyz]\n> >\n> > even lacking a stream identifier. I think that's reasonably OK in\n> > practice, because we're being over-eager in redacting instead of the\n> > other way around. And we're unlikely to see such a line from curl\n> > anyway, so I don't think that it matters.\n>\n> Yes, you're correct that we'd allow an empty stream identifier. I'm\n> content to leave it in the name of simplicity.\n\nYeah, I am definitely OK with that as well. I don't think it's worth\nbeing overly specific in what we accept for redaction, since we're\nerring on the side of being less restrictive.\n\n> > But this may all be moot anyway, I don't feel strongly one way or the\n> > other.\n>\n> My inclination is to leave it. I was actually tempted to just allow\n> _anything_ in the brackets if only because it makes the code even\n> simpler, but the \"skip past digits\" seemed like a reasonable middle\n> ground.\n\nYep, same. Thanks for the sanity check :-).\n\nThanks,\nTaylor\n"}]}