-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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(frontend): allow "constructor" property in response data #25407
fix(frontend): allow "constructor" property in response data #25407
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…constructor-property
Thanks for the rebase @SpencerTorres :) Running CI now and added a few reviewers. Hopefully we can get this across the finish line and close out the issue! |
…constructor-property
looks like CI was broken on master when I pulled. Updated again |
…constructor-property
Looks like master is consistently failing to run, however I did fix a new one-line lint issue related to test code added in this commit. Updated again |
CI running... fingers crossed! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #25407 +/- ##
=======================================
Coverage 67.39% 67.39%
=======================================
Files 1909 1909
Lines 74736 74738 +2
Branches 8326 8326
=======================================
+ Hits 50367 50369 +2
Misses 22319 22319
Partials 2050 2050
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Looks like CI is happy now! Re-pinging @betodealmeida @john-bodley @villebro to see if they have a moment to help with a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, an interesting issue, thanks for the fix and great tests! TIL
@SpencerTorres would you be able to rebase the PR? The mandatory CI workflows have changed since CI last run, causing a few of them to now be missing. |
kicking off CI. Fingers crossed! |
(cherry picked from commit a1983e4)
(cherry picked from commit a1983e4)
(cherry picked from commit a1983e4)
(cherry picked from commit a1983e4)
SUMMARY
Fixes #23953
Allows response JSON to be parsed with a property containing the name "
constructor
".The
json-bigint
module has some safety options in place to prevent prototype pollution, but it prevents the queries from loading data where the columns containconstructor
in the name/value. For example,test_constructor
andconstructor
would both throw an error.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
See #23953 for example query
ADDITIONAL INFORMATION