Skip to content

Fix json encoder to reject user defined iterables #687

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.

Sign up for GitHub

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

Open
GKotsifos wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix json encoder to reject user defined iterables #687

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions larky/src/main/java/com/verygood/security/larky/modules/JsonModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import net.starlark.java.eval.StarlarkThread;
import net.starlark.java.eval.StarlarkValue;
import net.starlark.java.eval.Structure;
import net.starlark.java.eval.Tuple;
import net.starlark.java.lib.json.Json;

// Tests at //src/test/java/net/starlark/java/eval:testdata/json.star
Expand Down Expand Up @@ -223,8 +224,8 @@ private void encode(Object x) throws EvalException {
return;
}

// e.g. tuple, list
if (x instanceof StarlarkIterable) {
// only real tuple, list JSON arrays
if (x instanceof StarlarkList || x instanceof Tuple) {
Copy link
Collaborator

@mahmoudimus mahmoudimus Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test for this? I understand why, but would this break behavior that folks are depending on?

Copy link
Author

@GKotsifos GKotsifos Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, if anyone is serializing custom iterables then this change will break them. Regarding the test, I was thinking of adding one but saw this line:

starlarky/larky/src/main/java/com/verygood/security/larky/modules/JsonModule.java

Line 36 in ae1cc24

// Tests at //src/test/java/net/starlark/java/eval:testdata/json.star

and could not find a json.star file so I assumed there are not tests for json. I can create some if needed.

Copy link
Collaborator

@mahmoudimus mahmoudimus Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GKotsifos the tests are here, I can see how that's confusing:

https://github.com/verygoodsecurity/starlarky/blob/master/libstarlark/src/test/java/net/starlark/java/eval/testdata/json.star

Maybe update the comment to say it's at libstarlark/...

Copy link
Author

@GKotsifos GKotsifos Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahmoudimus correct me if I am wrong but these tests are for this class:

https://github.com/verygoodsecurity/starlarky/blob/master/libstarlark/src/main/java/net/starlark/java/lib/json/Json.java

and not the JsonModule.java where the changes are:

https://github.com/verygoodsecurity/starlarky/blob/master/larky/src/main/java/com/verygood/security/larky/modules/JsonModule.java

although now that you mention it we should probably apply the same fix to Json.java as well.

Also looking at these tests, I saw this line:

starlarky/libstarlark/src/test/java/net/starlark/java/eval/testdata/json.star

Line 31 in ae1cc24

assert_eq(json.encode(range(3)), "[0,1,2]") # a built-in iterable

where even though range is a built-in iterable it should still fail since in python only lists and tuples are mapped by default to JSON arrays and I assume we want to mimic the exact behavior of python. But the description of the issue only states we should reject user-defined iterables. What do you think?

out.append('[');
String sep = "";
int i = 0;
Expand All @@ -242,6 +243,11 @@ private void encode(Object x) throws EvalException {
return;
}

//StarlarkIterable not list, tuple
if (x instanceof StarlarkIterable) {
throw Starlark.errorf("Object of type %s is not JSON serializable", Starlark.type(x));
}

// e.g. struct
if (x instanceof Structure) {
Structure obj = (Structure) x;
Expand Down