[json] Require a LiteralString for indent argument

[srittau]

Cf. https://discuss.python.org/t/should-json-restrict-string-indentation-to-json-whitespace/109060

Edit: If you read through the discussion, there seems to be a lot of arguments based on a misunderstanding about how LiteralString works. Please keep that in mind.

[github-actions]

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

[JelleZijlstra]

Yeah, that discussion isn't great.

Note that mypy-primer doesn't tell you much here because mypy doesn't meaningfully support LiteralString.

[picnixz]

misunderstanding about how LiteralString works

I want to understand what was misunderstood. Was it about the intent of LiteralString? because for me, LiteralString should work as explained in PEP-675 and if type checkers aren't able to follow the specs, it's not really the problem of the user (semantically it's even wrong to mandate that something being literal is passed if the quantity can be dynamically determined).

AFAIK, the following should be wrong right? (but AFAIU, not flagged by mypy because it's not implemented)

from typing import LiteralString

def f(n: int) -> str: ...
def g(s: LiteralString) -> None: ...

g(f(1))

The discussion on DPO, while not very great, still tells me that we shouldn't try breaking user's code. I would be annoyed if I needed to change call sites or adding cast() once mypy decides to implement LiteralString entirely.

[AlexWaygood]

I've set off an ecosystem run for ty that cherry-picks this change into ty's vendored stubs, so we can see the ecosystem impact with a type checker that supports LiteralString: astral-sh/ruff#28664

[srittau]

Of course your code example should be flagged, as str is not assignable to LiteralString. But from the discussion I got the feeling that some people commented that – while very active in all kinds of discussions on d.p.o – don't understand that LiteralString doesn't mean that you really have to provide a literal string as an argument, just a string flagged as LiteralString.

For example, a wrapper (that takes a LiteralString itself) can forward that argument without problems. You can even combine LiteralStrings in various ways, see https://typing.python.org/en/latest/spec/literal.html#inferring-literalstring. Therefore I doubt that this PR would have meaningful negative impact on existing code (except maybe wrappers needing to update their type annotations as well). Of course, this is hard to judge reliably, considering mypy's (and therefore primer's) incomplete LiteralString support.

[AlexWaygood]

9 added diagnostics on ty: astral-sh/ruff#28664 (comment). The full HTML report is at https://1e70c5fd.ty-ecosystem-ext.pages.dev/diff. (ty's ecosystem report runs on the same mypy_primer corpus as we run mypy on for typeshed's mypy_primer workflow.)

[srittau]

I've set off an ecosystem run for ty that cherry-picks this change into ty's vendored stubs, so we can see the ecosystem impact with a type checker that supports LiteralString: astral-sh/ruff#28664

Thanks, that was very helpful! I've looked through the hits, and all of them were simple wrappers that would need to update their own type annotation. I know that some people have other feelings about this, but I still believe that improving and modernizing our annotations in cases like this is the right approach, even if it means that users needs to also improve and modernize their annotations.

[AlexWaygood]

(Feel free to make your own experimental PRs like that directly to ty whenever it's helpful, BTW! And feel free to ping me if you need CI to be triggered, etc.)

[picnixz]

Therefore I doubt that this PR would have meaningful negative impact on existing code (except maybe wrappers needing to update their type annotations as well)

I can tell you that I'm very annoyed when type stubs change because we lose time just fixing CI failures just for it to be green. So I personally would appreciate that we avoid this. I still think that there is a lot of reasons why people would NOT use LiteralString, e.g., just because they need to import typing (I'd like to mention that I don't necessarily have a from typing import ... in every file, even when fully typed, and that it's also annoying to extend the list of from typing import [...] when necessary). I believe it's unnecessary chunk for unnecessary reasons.

and all of them were simple wrappers that would need to update their own type annotation

Those are also wrappers that users would use (e.g., rich's API; unless they want to have the cast themselves).

I still believe that improving and modernizing our annotations in cases like this is the right approach

I don't see it as an improvement nor a modernization. There is, IMO, no problem with accepting any string. The simple fact that the purpose of indent is for pretty-printing doesn't justify the cost of breaking downstream CIs (I don't understand why LiteralString would be any better; sure it could catch some issues such as typos but this not the same as literal strings for SQL where the notion of security comes).

just a string flagged as LiteralString.

That would be a wrong signal semantically speaking. Lying to the type checker by saying "it's a literal string" despite not being the case at runtime is honestly not ideal IMO.

You can even combine LiteralStrings in various ways

Yes, I know, and these operations don't surprise me since they are statically computable assuming that the input inferred types are reliable. What doesn't make sense for me is asking for a literal string when the runtime accepts any kind of string. That's just adding a barrier where there is no need for (I think the initial DPO question is wrong as well: there is nothing to change, nor reject).

[srittau]

What doesn't make sense for me is asking for a literal string when the runtime accepts any kind of string.

At runtime there is no difference between a "literal" and "any kind of" string, i.e. this attitude would render LiteralString redundant. But it has a very important role in preventing injection attacks, to which indent is susceptible. (Even though the attack vector is of course much smaller than with – for example – SQL queries.)

[JelleZijlstra]

I think I'd rather not do this.

LiteralString does have a role to play, but annotating something as taking only LiteralString is inherently an opinionated thing to do, and therefore it makes more sense in a private codebase or in a library that controls its own stubs.

In this case, I think the security risk is fairly remote, and making the change does impose some cost on users who have to change the annotations on wrapper functions.

[picnixz]

At runtime there is no difference between a "literal" and "any kind of" string, i.e. this attitude would render LiteralString redundant. But it has a very important role in preventing injection attacks, to which indent is susceptible. (Even though the attack vector is of course much smaller than with – for example – SQL queries.)

The attack surface is (1) very small (2) already known to be for debugging, so not taken as a security risk. For SQL, I'm not saying it's wrong to use it. It is good to use because SQL injections are much likely to occur when you don't reliably determine that your inputs are not attacker-controlled. But for JSON pprint, it doesn't make sense (there is no security to guarantee).

I'm not saying LiteralString is redundant. But it doesn't have its place as a security helper that we can (or need to) put everywhere. I do find very practical to have LiteralString when it comes to command strings, whether it's for autocompletion or for syncing the code across a large codebase.

[JelleZijlstra]

But for JSON pprint, it doesn't make sense (there is no security to guarantee).

There is a route by which this could be a security issue; it's a little roundabout but not impossible. (1) You allow the user to control the indent string; (2) you re-parse JSON serialized with this configuration; (3) an attacker supplies an indent that injects extra keys; (4) those keys break some invariant in your application.

[picnixz]

1. You allow the user to control the indent string; (2) you re-parse JSON serialized with this configuration; (3) an attacker supplies an indent that injects extra keys; (4) those keys break some invariant in your application.

For me that's covered by the warning in the json docs:

Be cautious when parsing JSON data from untrusted sources

and note that we are not talking about parsing JSON, but dumping them. JSON parsing is still strict so the application is responsible to give the correct JSON indents first.