search

ietf-wg-jsonpath / draft-ietf-jsonpath-base

Development of a JSONPath internet draft
https://ietf-wg-jsonpath.github.io/draft-ietf-jsonpath-base/
Other
59 stars 20 forks source link

Editorial nits and suggestions #275

Closed timbray closed 1 year ago

timbray commented 1 year ago

Signed-off-by: Tim Bray tbray@textuality.com

Obviously, this PR is designed for cherry-picking rather than merging.

gregsdennis commented 1 year ago

If this PR isn't intended for merging, should we concert it to a draft PR so it can't be merged?

glyn commented 1 year ago

If this PR isn't intended for merging, should we concert it to a draft PR so it can't be merged?

Done.

glyn commented 1 year ago

@timbray Before we close this PR, you wanted to include a caution to developers about using naive code to build paths using dot notation. @cabo pointed out that (I would argue somewhat less) caution is also needed when building paths using bracket notation. Are you still keen to see this kind of cautionary material in the spec? If so, which section would best contain it? The most natural could be the Security Considerations section, but we don't yet cover naive programming slips there, so those cautions would look out of place.

My feeling is that developers who are sufficiently incompetent to mess up in this way are (a) very unlikely to read the spec and (b) likely to screw up in some other ways that we can't anticipate. So I'd prefer not to add these cautions.

cabo commented 1 year ago

We could add a short third subsection on security considerations from building JSONPath queries out of potentially attacker-controlled variables, mostly pointing out that similar considerations exist as for SQL injections. I'd do this as a separate PR.

timbray commented 1 year ago

At this point, I'll defer to your editorial judgment. If I were writing this I'd find a way to work the example in, but it's really not essential.

cabo commented 1 year ago

A potential PR is in https://github.com/ietf-wg-jsonpath/draft-ietf-jsonpath-base/pull/307