Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiplayer API now respects allow_object_decoding #27398

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@Faless
Copy link
Contributor

commented Mar 25, 2019

Note that var decoding is still allowed for StreamPeer.get_var.
I think we need to add an object_decoding property to StreamPeer too (which could be probably cherry picked to 3.0.).
2.1 is not affected by any of this as object decoding is not implemented.

I'm also making a separate branch to fix in 3.0 (as the bug is there, but everything related to High Level multiplayer is in SceneTree so cherry picking is not an option).

Fixes #27395 .

I'm also wondering, if we should change the defaults for decode_variant and encode_variant to be safe (no object decoding), and update all their usage where we want object encoding/decoding with the extra parameter to true.

@akien-mga

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Note that var decoding is still allowed for StreamPeer.get_var.
I think we need to add an object_decoding property to StreamPeer too (which could be probably cherry picked to 3.0.).

That sounds good to me.

I'm also wondering, if we should change the defaults for decode_variant and encode_variant to be safe (no object decoding), and update all their usage where we want object encoding/decoding with the extra parameter to true.

I agree, it's better that the defaults are safe. We can't expect people to read the documentation to ensure that their use of these APIs is safe, so it's better that they have to read them to find out how to use the unsafe behaviour when needed :)

It's a slight compat breakage but that's justified for security purposes IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.