Skip to content

Java: Fix Gson's JsonArray.add models#13394

Merged
aschackmull merged 2 commits intogithub:mainfrom
atorralba:atorralba/java/fix-gson-jsonarray-models
Jun 8, 2023
Merged

Java: Fix Gson's JsonArray.add models#13394
aschackmull merged 2 commits intogithub:mainfrom
atorralba:atorralba/java/fix-gson-jsonarray-models

Conversation

@atorralba
Copy link
Copy Markdown
Contributor

Fixes a bug in Gson JsonArray.add models detected by @aschackmull in #13083.

When the type of the argument isn't JsonElement, the summary must be taint flow instead of value flow, because the type changes.

When the type of the argument isn't JsonElement, the summary must be taint flow instead of value flow
@atorralba atorralba added the no-change-note-required This PR does not need a change note label Jun 7, 2023
@atorralba atorralba requested a review from a team as a code owner June 7, 2023 12:14
@github-actions github-actions Bot added the Java label Jun 7, 2023
// "com.google.gson;JsonArray;true;add;(JsonArray);;Argument[0].Element;Argument[this].Element;value;manual"
JsonArray out = null;
String in = (String)source();
JsonElement in = (JsonElement)source();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something's off here. This test case is now a duplicate of the one two cases above. And we're no longer testing add(String), which I'm guessing is a much more important case than silly cases like a tainted boolean.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. Since JsonArray.addAll wasn't properly modeled, I got confused while updating the tests. c013567 should get it right.

Properly test JsonArray.add(String) and JsonArray.addAll(JsonArray) as well
Copy link
Copy Markdown
Contributor

@egregius313 egregius313 left a comment

Choose a reason for hiding this comment

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

LGTM

@aschackmull aschackmull merged commit cc45db7 into github:main Jun 8, 2023
@atorralba atorralba deleted the atorralba/java/fix-gson-jsonarray-models branch June 8, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Java no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants