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(security): improve results object creation #2574

Merged
merged 3 commits into from Apr 9, 2024

Conversation

wellwelwel
Copy link
Collaborator

@wellwelwel wellwelwel commented Apr 9, 2024

Finishing the tasks started in #2424 (and #2572).

This PR ensures a clean object evaluation for every item in results.

This is the final evaluation for both text and binary parsers:

const result = Object.create(null);

Object.defineProperty(result, 'constructor', {
  value: Object.create(null),
  writable: false,
  configurable: false,
  enumerable: false
});

// ...

// nestTables
if (!result[${tableName}]) result[${tableName}] = Object.create(null);

I also tried a simple Object.freeze(result.prototype), but the unit tests (specific to this) didn't pass as expected 🔓


Note

These changes preserve security without affecting the end user:

const [results] = await connection.query('SELECT 1+1 AS `test`'); // or execute

results[0].test.toFixed(2); // '2.00' ✅
results[0].addCustomProp = true; // true ✅

Copy link

codecov bot commented Apr 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.32%. Comparing base (71115d8) to head (7e1a7e6).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2574   +/-   ##
=======================================
  Coverage   90.31%   90.32%           
=======================================
  Files          71       71           
  Lines       15708    15717    +9     
  Branches     1333     1334    +1     
=======================================
+ Hits        14187    14196    +9     
  Misses       1521     1521           
Flag Coverage Δ
compression-0 90.32% <100.00%> (+<0.01%) ⬆️
compression-1 90.32% <100.00%> (+<0.01%) ⬆️
tls-0 89.84% <100.00%> (+<0.01%) ⬆️
tls-1 90.14% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wellwelwel wellwelwel marked this pull request as ready for review April 9, 2024 06:06
@wellwelwel
Copy link
Collaborator Author

@sidorares, as done in #2572, I'll merge and attribute the credits manually.

Also, I've sent you some more detailed information by e-mail 🙋🏻‍♂️

@wellwelwel wellwelwel merged commit 4a964a3 into sidorares:master Apr 9, 2024
64 checks passed
@wellwelwel wellwelwel deleted the proto branch April 9, 2024 09:46
@ert78gb
Copy link

ert78gb commented Apr 10, 2024

Hi,

Sorry for commenting on a closed PR, but it introduced a breaking changes for me.
I use node:assert/strict library for assertion and it recognise the constructor own property of the "mysql object" as a new property. I construct the expected object as object literals, that does not contains the null prototype constructor

I created a reproduction repo https://github.com/ert78gb/mysql2-object-creation

I would like to know what is your opinion about this "breaking changes". Do you think node:assert library should change the comparison algorithm or you would like to modify the mysql2 lib.

If you would like I could open a new issue.

Thx,
Robi

@wellwelwel
Copy link
Collaborator Author

wellwelwel commented Apr 10, 2024

Hm... 🤔

Hi @ert78gb, I can't answer yet whether it's an unintentional breaking change for deep tests comparisons or if it can be fixed. I need to elaborate some tests.

Thanks for the basic repro, I'll start with that.

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.

None yet

2 participants