-
-
Notifications
You must be signed in to change notification settings - Fork 598
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: revert breaking change in results creation #2591
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2591 +/- ##
=======================================
Coverage 90.32% 90.32%
=======================================
Files 71 71
Lines 15717 15725 +8
Branches 1334 1339 +5
=======================================
+ Hits 14196 14204 +8
Misses 1521 1521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Let's run some benchmarks before merging |
@sidorares, I rewrote the logic and description of this PR. Could you check again? 🙋🏻♂️ |
Basic benchmark: const arrayIncludes = (field) => {
console.time('Array.includes');
const privateObjectProps = [
'__defineGetter__',
'__defineSetter__',
'__lookupGetter__',
'__lookupSetter__',
'__proto__',
];
for (let i = 0; i < 1000000; i++) privateObjectProps.includes(field);
console.timeEnd('Array.includes');
};
const setHas = (field) => {
console.time('Set.has');
const privateObjectProps = new Set([
'__defineGetter__',
'__defineSetter__',
'__lookupGetter__',
'__lookupSetter__',
'__proto__',
]);
for (let i = 0; i < 1000000; i++) privateObjectProps.has(field);
console.timeEnd('Set.has');
};
for (let i = 0; i < 5; i++) arrayIncludes('field');
for (let i = 0; i < 5; i++) setHas('field'); Results:
Compatibility:
|
…ation (#2572) Fixes a potential RCE attack vulnerability reported by Vsevolod Kokorin (Slonser) of Solidlab
Bringing a comment here: 74abf9e#r140992502 🤝 |
Instead of checking the field name manually, we have some alternatives:
Also, if we consider the current solution, it's possible to return an empty object instead of throwing an error, as in mysqljs/mysql. |
Merging this. For a major release, I recommend reverting this PR and include a safer approach in docs, such as: Object.hasOwn(row, 'column'); |
thank you so much for the solution |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [mysql2](https://sidorares.github.io/node-mysql2/docs) ([source](https://github.com/sidorares/node-mysql2)) | dependencies | patch | [`3.9.3` -> `3.9.7`](https://renovatebot.com/diffs/npm/mysql2/3.9.3/3.9.7) | --- ### Release Notes <details> <summary>sidorares/node-mysql2 (mysql2)</summary> ### [`v3.9.7`](https://github.com/sidorares/node-mysql2/blob/HEAD/Changelog.md#397-2024-04-21) [Compare Source](sidorares/node-mysql2@v3.9.6...v3.9.7) ##### Bug Fixes - **security:** sanitize timezone parameter value to prevent code injection ([#​2608](sidorares/node-mysql2#2608)) ([7d4b098](sidorares/node-mysql2@7d4b098)) ### [`v3.9.6`](https://github.com/sidorares/node-mysql2/blob/HEAD/Changelog.md#396-2024-04-18) [Compare Source](sidorares/node-mysql2@v3.9.5...v3.9.6) ##### Bug Fixes - binary parser sometimes reads out of packet bounds when results contain null and typecast is false ([#​2601](sidorares/node-mysql2#2601)) ([705835d](sidorares/node-mysql2@705835d)) ### [`v3.9.5`](https://github.com/sidorares/node-mysql2/blob/HEAD/Changelog.md#395-2024-04-17) [Compare Source](sidorares/node-mysql2@v3.9.4...v3.9.5) ##### Bug Fixes - revert breaking change in results creation ([#​2591](sidorares/node-mysql2#2591)) ([f7c60d0](sidorares/node-mysql2@f7c60d0)) ### [`v3.9.4`](https://github.com/sidorares/node-mysql2/blob/HEAD/Changelog.md#394-2024-04-09) [Compare Source](sidorares/node-mysql2@v3.9.3...v3.9.4) ##### Bug Fixes - **docs:** improve the contribution guidelines ([#​2552](sidorares/node-mysql2#2552)) ([8a818ce](sidorares/node-mysql2@8a818ce)) - **security:** improve results object creation ([#​2574](sidorares/node-mysql2#2574)) ([4a964a3](sidorares/node-mysql2@4a964a3)) - **security:** improve supportBigNumbers and bigNumberStrings sanitization ([#​2572](sidorares/node-mysql2#2572)) ([74abf9e](sidorares/node-mysql2@74abf9e)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4wLjAiLCJ1cGRhdGVkSW5WZXIiOiIzNy4wLjAiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=--> Reviewed-on: https://git.vylpes.xyz/RabbitLabs/Droplet/pulls/304 Co-authored-by: Renovate Bot <renovate@vylpes.com> Co-committed-by: Renovate Bot <renovate@vylpes.com>
The previous solution (#2574) aimed to create a clean object, but it caused a major change.
Instead, I just grouped the object's private properties:
Then, it will perform a simple validation to ensure that the
fields[i].name
is safe:By this way, it's possible to:
prototype
after the query has been finished, they can