Skip to content

Add second part of a solution for secure JSON validation#1405

Merged
david0xd merged 11 commits intomainfrom
dd/fix-json-validation-security
May 31, 2023
Merged

Add second part of a solution for secure JSON validation#1405
david0xd merged 11 commits intomainfrom
dd/fix-json-validation-security

Conversation

@david0xd
Copy link
Copy Markdown
Contributor

@david0xd david0xd commented May 16, 2023

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/574

This PR is the second part of the solution for fixing JSON Validation security issue which Superstruct does not handle internally.
First PR required for this is in utils package: MetaMask/utils#103

This PR will add sanitization process for JSON structures. New function getSafeJson added to utils is now used in this PR.

@codecov
Copy link
Copy Markdown

codecov bot commented May 16, 2023

Codecov Report

Merging #1405 (aadffc0) into main (5f08ea4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head aadffc0 differs from pull request most recent head 92a2633. Consider uploading reports for the commit 92a2633 to get more accurate results

@@           Coverage Diff           @@
##             main    #1405   +/-   ##
=======================================
  Coverage   96.34%   96.35%           
=======================================
  Files         151      151           
  Lines        4598     4606    +8     
  Branches      747      747           
=======================================
+ Hits         4430     4438    +8     
  Misses        168      168           
Impacted Files Coverage Δ
...cution-environments/src/common/BaseSnapExecutor.ts 88.73% <100.00%> (+0.07%) ⬆️
...ps-execution-environments/src/common/validation.ts 100.00% <100.00%> (ø)

@david0xd david0xd force-pushed the dd/fix-json-validation-security branch from 031ac0c to aadffc0 Compare May 18, 2023 16:29
@david0xd david0xd marked this pull request as ready for review May 18, 2023 16:41
@david0xd david0xd requested a review from a team as a code owner May 18, 2023 16:41
@david0xd david0xd force-pushed the dd/fix-json-validation-security branch from 92a2633 to 8cb6fcb Compare May 31, 2023 09:46
@socket-security
Copy link
Copy Markdown

socket-security bot commented May 31, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No dependency changes detected in pull request

Pull request alert summary
Issue Status
Critical CVE ✅ 0 issues
CVE ✅ 0 issues
Mild CVE ✅ 0 issues
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Filesystem access ✅ 0 issues
Network access ✅ 0 issues
Shell access ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
No bug tracker ✅ 0 issues
No contributors or author data ✅ 0 issues
No README ✅ 0 issues
Deprecated ✅ 0 issues
New author ✅ 0 issues
Unstable ownership ✅ 0 issues
Non-existent author ✅ 0 issues
Unmaintained ✅ 0 issues
Unpublished package ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected security risk ✅ 0 issues
AI warning ✅ 0 issues

@david0xd david0xd force-pushed the dd/fix-json-validation-security branch from 47b7300 to b3b1530 Compare May 31, 2023 12:11
Copy link
Copy Markdown
Member

@FrederikBolding FrederikBolding left a comment

Choose a reason for hiding this comment

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

LGTM

@david0xd david0xd merged commit 2331911 into main May 31, 2023
@david0xd david0xd deleted the dd/fix-json-validation-security branch May 31, 2023 14:25
FrederikBolding pushed a commit that referenced this pull request May 31, 2023
* Add second part of a solution for secure JSON validation

* Add sanitization on few more places

* Review refactoring (1)

* Refactor solution (2)

* Update function name

* Update types related to getSafeJson

* Review request refactoring

* Update coverage thresholds

* Review requests addressed for errors combined

* Fix coverage

* Replace part of an error
FrederikBolding pushed a commit that referenced this pull request May 31, 2023
* Add second part of a solution for secure JSON validation

* Add sanitization on few more places

* Review refactoring (1)

* Refactor solution (2)

* Update function name

* Update types related to getSafeJson

* Review request refactoring

* Update coverage thresholds

* Review requests addressed for errors combined

* Fix coverage

* Replace part of an error
Gudahtt pushed a commit that referenced this pull request May 31, 2023
* Add second part of a solution for secure JSON validation

* Add sanitization on few more places

* Review refactoring (1)

* Refactor solution (2)

* Update function name

* Update types related to getSafeJson

* Review request refactoring

* Update coverage thresholds

* Review requests addressed for errors combined

* Fix coverage

* Replace part of an error
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.

6 participants