Protect __evaluate_entry and __evaluate_exit callbacks
#700
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adamscybot
added a commit
to adamscybot/jsonata-exerciser
that referenced
this pull request
Jun 8, 2024
adamscybot
added a commit
to adamscybot/jsonata-exerciser
that referenced
this pull request
Jun 8, 2024
adamscybot
added a commit
to adamscybot/jsonata
that referenced
this pull request
Jun 8, 2024
This is helpful in scenarios where you wish to debug the environment behavior, or modify the env for advanced scenarios. As the callback is set via a Symbol, it can not be manipulated or read inside a query. See jsonata-js/jsonata/jsonata-js#700.
These two internal APIs are very useful in debugging scenarios, and for imposing time/depth constraints on queries. However, this internal API should only be accessible when configuring an expression programmatically, and not from inside of a query. Otherwise, a query can be manipulated to remove such diagnostics or constraints. By changing both binding keys to Symbol, they can no longer be accessed inside of the query since the Symbol API is not accessible there.
|
@andrew-coleman This seems like a good change to pick up if you don't see any problems |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #695
These two internal/undocumented APIs are very useful in debugging scenarios and for imposing time/depth constraints on queries. However, this internal API should only be accessible when configuring an expression programmatically and not from inside a query. Otherwise, a query can be manipulated to remove such diagnostics or constraints. Addressing this could potentially protect consumers with certain use cases from targeted attacks.
By changing both binding keys to
Symbol, they can no longer be accessed inside of the query since the Symbol API is not accessible there. Without access to the Symbol, a query is fundamentally unable to address these bindings.I am building a pluggable and security-minded framework around JSONata (open source), which makes considerable use of these properties, and I want to protect them. But this is generally applicable. Many people processing queries on the server will not want a query to be able to remove these hooks.
At first, I considered increasing the API surface to do this, but then I realized that using
Symbol.foris a super pragmatic and ergonomic way of solving this problem with minimal changes, which makes sense for something internal like this. Symbols' primary use case is implementing this kind of separation between public and internal naming, so I think this is the way to go.Attempting to manipulate the old properties inside a query as described in #695 is now not possible. There's nothing special anymore about those references, and they work like any other reference.
I have also made the corresponding change to jsonata-js/jsonata-exerciser#36.
I'm pinging @andrew-coleman as this is something we previously discussed over email. Also, see #701. These two small changes will unlock a whole realm of possibilities and allow me to do it safely and securely.
Signed-off-by: Adam Thomas adam@adam-thomas.co.uk