Skip to content

Conversation

@andrewjschuang
Copy link
Contributor

@andrewjschuang andrewjschuang commented Jul 28, 2022

Primary fix:

  • Using Function to evaluate JSON / JS objects - regex failed on some cases: on time values (containing :) and if there were spaces in between the key/value (e.g. {key : "value"} - note space before :)

Refactors:

  • return parsedValue whenever possible
  • new value propDefinition
  • change some logic of summary exports (affects order of execution only)
  • minor changes

Resolves #3515.

@vercel
Copy link

vercel bot commented Jul 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
pipedream-docs ✅ Ready (Inspect) Visit Preview Sep 2, 2022 at 11:09AM (UTC)
pipedream-docs-redirect-do-not-edit ✅ Ready (Inspect) Visit Preview Sep 2, 2022 at 11:09AM (UTC)

@dylburger
Copy link
Contributor

Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:

@andrewjschuang
Copy link
Contributor Author

andrewjschuang commented Jul 28, 2022

@dylburger @vellames-turing @feyzullah would love your inputs on possible security issues - commit 1452494

@feyzullah
Copy link
Contributor

@dylburger @vellames-turing @feyzullah would love your inputs on possible security issues - commit 1452494

Hi @andrewjschuang we had a convo about using eval last time. We decided not to use eval because if a user input be a malicious function, eval would run that function no matter what and we have no control over it. After that @vellames-turing added and used xss package for this reason. I think using xss and JSON.parse would be safe.

@andrewjschuang
Copy link
Contributor Author

Hi @andrewjschuang we had a convo about using eval last time. We decided not to use eval because if a user input be a malicious function, eval would run that function no matter what and we have no control over it. After that @vellames-turing added and used xss package for this reason. I think using xss and JSON.parse would be safe.

Function would limit the access of scopes and variables at least, so it's safer than eval.
I'm not sure how Data Stores are implemented - if it's just a user-limited container would it be an issue to expose this?

I've used this array for testing:

[
  "1",
  2,
  3.5,
  {
    key: "value",
  },
  new Date(),
  console.log("Hello!"),
  { "time" : "18:01:31" },
]

Result:

Screenshot from 2022-07-28 11-26-48

Note that console.log is executed and returns null, so that's what is being saved into the data store.

I've added a while(true) test, but I don't know why it throws an exception and saves everything as a string, which is nice.

Result:

Screenshot from 2022-07-28 11-27-09

Don't really know about other types of injected code.

@jcortes jcortes self-requested a review July 28, 2022 22:07
Copy link
Collaborator

@jcortes jcortes left a comment

Choose a reason for hiding this comment

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

Hi @andrewjschuang Everything looks great. However I consider you should change versions in the rest of components as well since you are touching data_stores.app.mjs. Don't you think?

@andrewjschuang
Copy link
Contributor Author

Hi @andrewjschuang Everything looks great. However I consider you should change versions in the rest of components as well since you are touching data_stores.app.mjs. Don't you think?

Thanks, I don´t think it's necessary since the changed parts of the app file don't affect the other components.

@jcortes jcortes self-requested a review July 29, 2022 16:27
Copy link
Collaborator

@jcortes jcortes left a comment

Choose a reason for hiding this comment

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

@andrewjschuang LGTM! Ready for QA!

@vunguyenhung
Copy link
Collaborator

Hello everyone, I have tested this PR and there're some test cases failed or needed improvement.

Please check the test report below for more information
Data_Stores_3515_2202.pdf

@andrewjschuang
Copy link
Contributor Author

Hello everyone, I have tested this PR and there're some test cases failed or needed improvement.

Please check the test report below for more information Data_Stores_3515_2202.pdf

Thanks, should be fixed!

@vunguyenhung
Copy link
Collaborator

Hi everyone, all test cases are passed! Ready for release!

Test report
Data_Stores_3515_1829.pdf

@andrewjschuang
Copy link
Contributor Author

/approve

@andrewjschuang andrewjschuang merged commit a7fcbc6 into master Aug 2, 2022
@andrewjschuang andrewjschuang deleted the fix-data-stores-objects branch August 2, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Data Store: Add or Update Single Record saves JSON Object Value as String

7 participants