LogfmtRenderer has a bug with escaped double quotes

[davellas]

In the code of LogFmtRenderer there is:

value = f"{value}".replace('"', '\\"')

However if you are trying to render an escaped double quote like
"I want to render this \"string\" with logfmtrenderer"

it ends up rendering it as
\"I want to render this \\"string\\" with logfmtrenderer\"

\\" is wrong and won't be parsed correctly by a logfmtparser.

[hynek]

Hmmm we've used the same approach as python-logfmter IIRC, so they've got the same bug if this is valid.

I'm trying to wrap my head around this, but is there a more precise definition of of logfmt than https://brandur.org/logfmt?

It feels like it accidentally did the right thing and escaped the backslash to preserve it?

[davellas]

Sorry for the late answer, I didn't notice your reply before.

The problem is that both the backslash and the quote needs to end up escaped, not only the backslash.
The resulting string should be
\"I want to render this \\\"string\\\" with logfmtrenderer\"

That allows a parser to translate \" back to the original ".

If, as it is implemented in structlog, you use \", then a parser will parse \ into , but then it will see just the double quote and interpret it as the end of the string.
I am not sure about the logfmt spec, but using the grafana LOKI/promtail logfmt parser breaks with the current structlog implementation, and it works with my associated PR solution

[madjar]

Chiming in, as requested.

So, this looks like an issue with double escaping (which are always fun). The result is not completely specific to logfmt, it's the same thing you'd get if you were producing strings for bash or python or something.

If I understand correctly:

• We want to output the string "I want to render this \"string\" with logfmtrenderer" (represented in python as r'"I want to render this \"string\" with logfmtrenderer"')
• Every " needs to be escaped as \", which the current implementation does
• Every \ needs to be espaced as \\, which the current implementation doesn't do.
• So we expect to see \\\" (\\, then \"), but we only see \\", which is invalid.
• When I feed the same thing to logrus, I indeed get \\\".

I'll now take a stab at reviewing the PR.

BTW, logfmt is a bit more specified in https://pkg.go.dev/github.com/kr/logfmt (there is an EBNF, ish). It is the reference implementation in go, anyways, and used in grafana and a lot of other tools.

[madjar]

Okay, after thinking more on it, here are a set of test cases that I've validated against the grafana parser, that should cover this:

• boring -> boring (`"boring" would also be valid)
• with space -> "with space" (spaces requires quotes)
• with=equal -> "with=equal" (equal requires quotes)
• a\slash -> a\slash (a slash by itself doesn't require espace. "a\\slash" would also be valid, a slash in quotes requires escape
• a"quote -> "a\"quote" (a quote requires quoting, and escaping the quote)
• a\slash with space or a"quote -> "a\\slash with space or a\"quote" (if anything triggers quoting of the string, then the slash must be escaped).

I don't think main nor the linked PR handles the last case (that \ needs escaping if and only if we are quoting the value). I believe this bug is also present in logfmter.

[hynek]

Thanks!

quick summary before the work begins:

• boring -> boring (`"boring" would also be valid)

test_reference_format (and others)

• with space -> "with space" (spaces requires quotes)

test_reference_format

• with=equal -> "with=equal" (equal requires quotes)

test_equal_sign_or_space_in_value

• a\slash -> a\slash (a slash by itself doesn't require espace. "a\\slash" would also be valid, a slash in quotes requires escape
• a"quote -> "a\"quote" (a quote requires quoting, and escaping the quote)
• a\slash with space or a"quote -> "a\\slash with space or a\"quote" (if anything triggers quoting of the string, then the slash must be escaped).

TODO :)