Skip to content

Ruby: JSON flow summaries #11136

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

Merged
merged 10 commits into from
Dec 1, 2022
Merged

Ruby: JSON flow summaries #11136

merged 10 commits into from
Dec 1, 2022

Conversation

hmac
Copy link
Contributor

@hmac hmac commented Nov 6, 2022

Add flow summaries for ActiveSupport::JSON and various JSON methods. We consider JSON parsing and generation to be taint-preserving.

@github-actions github-actions bot added the Ruby label Nov 6, 2022
@hmac hmac force-pushed the json-flow-summaries branch from 97dcfdc to fc85996 Compare November 7, 2022 21:28
@hmac hmac force-pushed the json-flow-summaries branch from fc85996 to a56696f Compare November 24, 2022 03:43
@hmac hmac marked this pull request as ready for review November 24, 2022 06:15
@hmac hmac requested a review from a team as a code owner November 24, 2022 06:15
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

There are a few instance methods for oj that look relevant for flow from the self arg to the return value, e.g. as_json, to_json etc: https://rubydoc.info/gems/oj/JSON/GenericObject Are these in scope for this PR - I recognize that there's probably a ton of potential methods to model that might not be used much in practice.

row =
[
"json;;Member[JSON].Method[parse,parse!,load,restore];Argument[0];ReturnValue;taint",
"json;;Member[JSON].Method[generate,fast_generate,dump,unparse,fast_unparse];Argument[0];ReturnValue;taint",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"json;;Member[JSON].Method[generate,fast_generate,dump,unparse,fast_unparse];Argument[0];ReturnValue;taint",
"json;;Member[JSON].Method[generate,fast_generate,pretty_generate,dump,unparse,fast_unparse];Argument[0];ReturnValue;taint",

https://rubyapi.org/3.1/o/json#method-i-pretty_generate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 added

@hmac
Copy link
Contributor Author

hmac commented Nov 24, 2022

There are a few instance methods for oj that look relevant for flow from the self arg to the return value, e.g. as_json, to_json etc: https://rubydoc.info/gems/oj/JSON/GenericObject Are these in scope for this PR - I recognize that there's probably a ton of potential methods to model that might not be used much in practice.

That's a good question. As you say, there's a lot of potential methods we should model. My preference is to tackle the obvious low-hanging fruit here, and then tackle the various JSON gems in full later on, as part of general non-rails library modelling. What do you think?

@alexrford
Copy link
Contributor

That's a good question. As you say, there's a lot of potential methods we should model. My preference is to tackle the obvious low-hanging fruit here, and then tackle the various JSON gems in full later on, as part of general non-rails library modelling. What do you think?

Sounds like a good approach.

alexrford
alexrford previously approved these changes Nov 25, 2022
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

The tests need updating, but otherwise LGTM - I'll reapprove when the tests are fixed.

@hmac hmac force-pushed the json-flow-summaries branch from b862c7b to dab7970 Compare November 30, 2022 00:19
@hmac hmac merged commit bd129ed into github:main Dec 1, 2022
@hmac hmac deleted the json-flow-summaries branch December 1, 2022 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants