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

fix(rest): sanitize json for JSON.parse() #2348

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
4 participants
@raymondfeng
Copy link
Member

raymondfeng commented Feb 6, 2019

Fixes strongloop-internal/scrum-apex#409

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@raymondfeng raymondfeng requested a review from bajtos as a code owner Feb 6, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 6, 2019

FYI: there is a package https://github.com/hapijs/bourne to sanitize JSON.parse but body-parser module only exposes reviver for customization.

@jannyHou
Copy link
Contributor

jannyHou left a comment

@raymondfeng Thanks the solution LGTM.
(Not a scope of this PR)It would be better if we can extract sanitizeJsonParse into a utility package like https://github.com/hapijs/bourne does.
I am thinking of creating a new utility package for customized functions that can be shared among multiple packages.

@bajtos

bajtos approved these changes Feb 7, 2019

Copy link
Member

bajtos left a comment

LGTM.

I am not confident that the reviver-based solution is equally good as what bourne does, i.e. covers all edge cases and has good performance, but the change proposed here is definitely an improvement compared to the current status.

Show resolved Hide resolved packages/rest/src/json-parse.ts Outdated
Show resolved Hide resolved packages/rest/src/json-parse.ts Outdated

@raymondfeng raymondfeng force-pushed the sanitize-json-parse branch from d7481d4 to ea237e5 Feb 7, 2019

@raymondfeng raymondfeng merged commit 5042698 into master Feb 7, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (dhmlau) No new issues
Details

@raymondfeng raymondfeng deleted the sanitize-json-parse branch Feb 7, 2019

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.