Skip to content

fix: group prototype-named keys safely#3855

Open
hiSandog wants to merge 1 commit into
faker-js:nextfrom
hiSandog:fix/group-by-proto-key-20260522
Open

fix: group prototype-named keys safely#3855
hiSandog wants to merge 1 commit into
faker-js:nextfrom
hiSandog:fix/group-by-proto-key-20260522

Conversation

@hiSandog
Copy link
Copy Markdown
Contributor

Summary

  • Accumulate groupBy buckets in a Map so keys like proto are handled as ordinary group names
  • Convert the grouped entries back to the existing record return shape
  • Add regression coverage for grouping under a prototype property name

Tests

  • pnpm vitest run test/internal/group-by.spec.ts
  • pnpm run ts-check
  • git diff --check

@hiSandog hiSandog requested a review from a team as a code owner May 22, 2026 11:43
@netlify
Copy link
Copy Markdown

netlify Bot commented May 22, 2026

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ea5b9fd
🔍 Latest deploy log https://app.netlify.com/projects/fakerjs/deploys/6a1041591363e40008fee438
😎 Deploy Preview https://deploy-preview-3855.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.91%. Comparing base (8b4e2b1) to head (ea5b9fd).

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #3855   +/-   ##
=======================================
  Coverage   98.91%   98.91%           
=======================================
  Files         905      905           
  Lines        3146     3148    +2     
  Branches      581      581           
=======================================
+ Hits         3112     3114    +2     
  Misses         30       30           
  Partials        4        4           
Files with missing lines Coverage Δ
src/internal/group-by.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent s: pending triage Pending Triage c: security Indicates a vulnerability or a potentially (security) risky change. labels May 22, 2026
@ST-DDT ST-DDT added this to the v10.x milestone May 22, 2026
@ST-DDT ST-DDT requested a review from Copilot May 22, 2026 12:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the internal groupBy helper to safely handle prototype-named keys (e.g. __proto__) by accumulating groups in a Map, and adds a regression test to ensure those keys are treated as ordinary group names.

Changes:

  • Switched groupBy’s internal accumulator from a plain object to a Map to avoid special-case prototype keys during grouping.
  • Converted grouped Map entries back into the existing Record<string, …> return shape via Object.fromEntries.
  • Added a regression test covering grouping under __proto__.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/internal/group-by.ts Uses a Map for safe bucket accumulation and converts back to an object on return.
test/internal/group-by.spec.ts Adds regression coverage for grouping under prototype property names.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/internal/group-by.ts
Comment on lines +42 to 50
const result = new Map<string | number, TMappedValue[]>();

for (const value of values) {
const key = keyMapper(value);
if (result[key] === undefined) {
result[key] = [];
let group = result.get(key);
if (group === undefined) {
group = [];
result.set(key, group);
}
(value) => value
);

expect(result.__proto__).toEqual(['first', 'second']);
Copy link
Copy Markdown
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

I'm not sure, whether I would want anyone to use __proto__ as a key.
Do you have a need for that?

What do the others think?

@ST-DDT ST-DDT added s: awaiting more info Additional information are requested s: needs decision Needs team/maintainer decision and removed s: pending triage Pending Triage labels May 22, 2026
Copy link
Copy Markdown
Member

@xDivisionByZerox xDivisionByZerox left a comment

Choose a reason for hiding this comment

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

The method groupBy is an internal method that mimics the API of the official Object#groupBy function. We cannot use the native function due to it requiring at least Node v21 - we currently still support Node v20.

While you are correct that Object#groupBy does support the use case you are adding, I do not see any reason for us to add it to our code base as of right now. groupBy is [only used in 3 different places](https://github.com/search?q=repo%3Afaker-js%2Ffaker%20groupBy(&type=code) and none of those require safe prototype name grouping:

Furthermore, only the last of these cases is actually used during the runtime of Faker and it clearly maps a numeric value to the object keys - so no prototype names.

Because of the stated reasons, I would decline this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: bug Something isn't working c: security Indicates a vulnerability or a potentially (security) risky change. p: 1-normal Nothing urgent s: awaiting more info Additional information are requested s: needs decision Needs team/maintainer decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants